Re: [PATCH v3 kvmtool 09/13] builtin_run: Allow standard size specifiers for memory

On Wed, 1 Jun 2022 15:17:50 +0100
Alexandru Elisei [off-list ref] wrote:

Hi Alex,

Hi,

Thank you for having a look! Replies below.

On Wed, Jun 01, 2022 at 02:39:55PM +0100, Andre Przywara wrote:

quoted

On Wed, 25 May 2022 12:23:41 +0100
Alexandru Elisei [off-list ref] wrote:

Hi,
  

quoted

From: Suzuki K Poulose <suzuki.poulose@arm.com>

Allow the user to use the standard B (bytes), K (kilobytes), M (megabytes),
G (gigabytes), T (terabytes) and P (petabytes) suffixes for memory size.
When none are specified, the default is megabytes.

Also raise an error if the guest specifies 0 as the memory size, instead
of treating it as uninitialized, as kvmtool has done so far.

Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Signed-off-by: Alexandru Elisei <redacted>
---
 builtin-run.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 60 insertions(+), 5 deletions(-)

diff --git a/builtin-run.c b/builtin-run.c
index 2ef159cdb2a3..a49698d5b2fe 100644
--- a/builtin-run.c
+++ b/builtin-run.c

@@ -49,9 +49,11 @@
 #include <ctype.h>
 #include <stdio.h>
 
-#define MB_SHIFT		(20)
 #define KB_SHIFT		(10)
+#define MB_SHIFT		(20)
 #define GB_SHIFT		(30)
+#define TB_SHIFT		(40)
+#define PB_SHIFT		(50)  

Can we lose the parentheses?  

Yes.

quoted

quoted

 
 __thread struct kvm_cpu *current_kvm_cpu;
 

@@ -87,6 +89,60 @@ void kvm_run_set_wrapper_sandbox(void)
 	kvm_run_wrapper = KVM_RUN_SANDBOX;
 }
 
+static int parse_mem_unit(char **next)
+{
+	int shift = -1;
+
+	switch (**next) {
+	case 'B': case 'b': shift = 0; break;
+	case 'K': case 'k': shift = KB_SHIFT; break;
+	case 'M': case 'm': shift = MB_SHIFT; break;
+	case 'G': case 'g': shift = GB_SHIFT; break;
+	case 'T': case 't': shift = TB_SHIFT; break;
+	case 'P': case 'p': shift = PB_SHIFT; break;
+	}
+
+	if (shift == -1) {
+		/* The default is megabytes. */
+		shift = MB_SHIFT;  

Doesn't that look better inside the switch/case?
	default: return MB_SHIFT;  

I think that change alone breaks the logic.

The code needs to advance next if and only if it matches on one of the
characters. I'll have a go at advancing next in each of the switch arms
above (with the exception of the default one, which I'll add) to see how it
ends up looking.

Mmh, but I meant:
{
	switch (**next) {
	case 'B': case 'b': shift = 0; break;
	case 'K': case 'k': shift = KB_SHIFT; break;
	case 'M': case 'm': shift = MB_SHIFT; break;
	case 'G': case 'g': shift = GB_SHIFT; break;
	case 'T': case 't': shift = TB_SHIFT; break;
	case 'P': case 'p': shift = PB_SHIFT; break;
	default: return MB_SHIFT;  
	}

	(*next)++;

	return shift;
}

that should solve it, shouldn't it?

quoted

quoted

+	} else {
+		(*next)++;
+	}
+
+	return shift;
+}
+
+static u64 parse_mem_option(const char *nptr, char **next)
+{
+	u64 shift;
+	u64 val;
+
+	val = strtoull(nptr, next, 10);
+	if (errno == ERANGE)
+		die("Memory too large: %s", nptr);  

strtoull does not clear errno if it succeeds, so it retains the
previous error value. So we would need to set errno to 0 just before
calling strtoull.  

This was intentional on my part, because I was under the impression that
kvmtool treats all instances where errno != 0 as a fatal error. I think I
was wrong about that, I see at least one instance when that isn't the case,
in kvm_setup_guest_init -> extract_file. So it isn't a rule that a non-zero
errno is a fatal error.

I'll change the code to zero errno before calling strtoull.

Thanks!

quoted

quoted

+	shift = parse_mem_unit(next);
+
+	if ((val << shift) < val)
+		die("Memory too large: %s", nptr);
+
+	return val << shift;
+}
+
+static int mem_parser(const struct option *opt, const char *arg, int unset)
+{
+	struct kvm *kvm = opt->ptr;
+	char *next;
+
+	kvm->cfg.ram_size = parse_mem_option(arg, &next);
+	if (kvm->cfg.ram_size == 0)
+		die("Invalid RAM size: %s", arg);  

Does 0 hold any significant value (anymore)? I think we die() if we
encounter invalid values in parse_mem_option()?  

strtoull does not consider an error to convert the string "0" to an
unsigned long long.

I was wondering if we treat 0 as an indicator of a conversion error, or as
a too-low memory size value here. I don't think we should special case the
latter, as even 1MB or 2MB are typically too low values for a "normal"
kvmtool run ("Fatal: kernel image too big to contain in guest memory.").
On the other hand, with --firmware I think we can run a (admittedly very
limited) guest with 0MB of RAM.

So if we care about garbage as an argument, we should do it by the book
(strtoul manpage), and compare the next pointer to the input string
address, plus checking for the returned value being 0, so that
"-m gimme-all" is explicitly denied. But that would need to happen in
parse_mem_option(), I think, not here.

If we cannot be asked, that's probably fine, but I just wanted to
check this.

Cheers,
Andre

quoted

quoted

+
+	if (*next != '\0')
+		die("Invalid memory specifier: %s", arg);
+
+	return 0;
+}
+
 #ifndef OPT_ARCH_RUN
 #define OPT_ARCH_RUN(...)
 #endif

@@ -97,8 +153,9 @@ void kvm_run_set_wrapper_sandbox(void)
 	OPT_STRING('\0', "name", &(cfg)->guest_name, "guest name",	\
 			"A name for the guest"),			\
 	OPT_INTEGER('c', "cpus", &(cfg)->nrcpus, "Number of CPUs"),	\
-	OPT_U64('m', "mem", &(cfg)->ram_size, "Virtual machine memory"	\
-		" size in MB."),					\
+	OPT_CALLBACK('m', "mem", NULL, "size[BKMGTP]",			\
+		     "Virtual machine memory size, by default measured"	\
+		     " in megabytes (M)", mem_parser, kvm),		\
 	OPT_CALLBACK('d', "disk", kvm, "image or rootfs_dir", "Disk "	\
 			" image or rootfs directory", img_name_parser,	\
 			kvm),						\

@@ -522,8 +579,6 @@ static void kvm_run_validate_cfg(struct kvm *kvm)
 		pr_warning("Ignoring initrd file when loading a firmware image");
 
 	if (kvm->cfg.ram_size) {
-		/* User specifies RAM size in megabytes. */
-		kvm->cfg.ram_size <<= MB_SHIFT;
 		available_ram = host_ram_size();
 		if (available_ram && kvm->cfg.ram_size > available_ram) {
 			pr_warning("Guest memory size %lluMB exceeds host physical RAM size %lluMB",  

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel