diff mbox series

[kvmtool,10/16] kvmtool: Allow standard size specifiers for memory

Message ID 1569245722-23375-11-git-send-email-alexandru.elisei@arm.com (mailing list archive)
State New, archived
Headers show
Series arm: Allow the user to define the memory layout | expand

Commit Message

Alexandru Elisei Sept. 23, 2019, 1:35 p.m. UTC 
From: Suzuki K Poulose <suzuki.poulose@arm.com>

Allow standard suffixes, K, M, G, T & P suffixes (lowercase and uppercase)
for sizes and addresses for memory bank parameters. By default, the size is
specified in MB.

Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 builtin-run.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 49 insertions(+), 6 deletions(-)

Comments

Andre Przywara Nov. 7, 2019, 1:55 p.m. UTC | #1 
On Mon, 23 Sep 2019 14:35:16 +0100
Alexandru Elisei <alexandru.elisei@arm.com> wrote:

Hi,

> From: Suzuki K Poulose <suzuki.poulose@arm.com>
> 
> Allow standard suffixes, K, M, G, T & P suffixes (lowercase and uppercase)
> for sizes and addresses for memory bank parameters. By default, the size is
> specified in MB.
> 
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  builtin-run.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 49 insertions(+), 6 deletions(-)
> 
> diff --git a/builtin-run.c b/builtin-run.c
> index df255cc44078..757ede4ac0d1 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)
>  
>  __thread struct kvm_cpu *current_kvm_cpu;
>  
> @@ -87,6 +89,48 @@ void kvm_run_set_wrapper_sandbox(void)
>  	kvm_run_wrapper = KVM_RUN_SANDBOX;
>  }
>  
> +static int parse_unit(char **next)
> +{
> +	int shift = 0;
> +
> +	switch(**next) {
> +	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)
> +		(*next)++;
> +
> +	return shift;
> +}
> +
> +static u64 parse_size(char **next)
> +{
> +	int shift = parse_unit(next);
> +
> +	/* By default the size is in MB, if none is specified. */
> +	if (!shift)
> +		shift = 20;
> +
> +	while (**next != '\0' && **next != '@')
> +		(*next)++;

Mmh, doesn't that skip over invalid characters, which should be reported as an error? Like "12Three"? Why do we need to skip something here anyway?

> +
> +	return ((u64)1) << shift;

Is there any reason we don't just return the shift value back? Seems more efficient to me.

> +}
> +
> +static u64 parse_addr(char **next)
> +{
> +	int shift = parse_unit(next);
> +
> +	while (**next != '\0')
> +		(*next)++;

Same here, why do we skip characters? Especially since we check for \0 in the caller below.

Cheers,
Andre

> +
> +	return ((u64)1) << shift;
> +}
> +
>  static int mem_parser(const struct option *opt, const char *arg, int unset)
>  {
>  	struct kvm_config *cfg = opt->value;
> @@ -99,15 +143,12 @@ static int mem_parser(const struct option *opt, const char *arg, int unset)
>  	if (next == p)
>  		die("Invalid memory size");
>  
> -	/* The user specifies the memory in MB, we use bytes. */
> -	size <<= MB_SHIFT;
> +	size *= parse_size(&next);
>  
>  	if (*next == '\0')
>  		goto out;
> -	else if (*next == '@')
> -		p = next + 1;
>  	else
> -		die("Unexpected character after memory size: %c", *next);
> +		p = next + 1;
>  
>  	addr = strtoll(p, &next, 0);
>  	if (next == p)
> @@ -118,6 +159,8 @@ static int mem_parser(const struct option *opt, const char *arg, int unset)
>  		die("Specifying the memory address not supported by the architecture");
>  #endif
>  
> +	addr *= parse_addr(&next);
> +
>  out:
>  	cfg->ram_base = addr;
>  	cfg->ram_size = size;
diff mbox series

Patch

diff --git a/builtin-run.c b/builtin-run.c
index df255cc44078..757ede4ac0d1 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)
 
 __thread struct kvm_cpu *current_kvm_cpu;
 
@@ -87,6 +89,48 @@  void kvm_run_set_wrapper_sandbox(void)
 	kvm_run_wrapper = KVM_RUN_SANDBOX;
 }
 
+static int parse_unit(char **next)
+{
+	int shift = 0;
+
+	switch(**next) {
+	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)
+		(*next)++;
+
+	return shift;
+}
+
+static u64 parse_size(char **next)
+{
+	int shift = parse_unit(next);
+
+	/* By default the size is in MB, if none is specified. */
+	if (!shift)
+		shift = 20;
+
+	while (**next != '\0' && **next != '@')
+		(*next)++;
+
+	return ((u64)1) << shift;
+}
+
+static u64 parse_addr(char **next)
+{
+	int shift = parse_unit(next);
+
+	while (**next != '\0')
+		(*next)++;
+
+	return ((u64)1) << shift;
+}
+
 static int mem_parser(const struct option *opt, const char *arg, int unset)
 {
 	struct kvm_config *cfg = opt->value;
@@ -99,15 +143,12 @@  static int mem_parser(const struct option *opt, const char *arg, int unset)
 	if (next == p)
 		die("Invalid memory size");
 
-	/* The user specifies the memory in MB, we use bytes. */
-	size <<= MB_SHIFT;
+	size *= parse_size(&next);
 
 	if (*next == '\0')
 		goto out;
-	else if (*next == '@')
-		p = next + 1;
 	else
-		die("Unexpected character after memory size: %c", *next);
+		p = next + 1;
 
 	addr = strtoll(p, &next, 0);
 	if (next == p)
@@ -118,6 +159,8 @@  static int mem_parser(const struct option *opt, const char *arg, int unset)
 		die("Specifying the memory address not supported by the architecture");
 #endif
 
+	addr *= parse_addr(&next);
+
 out:
 	cfg->ram_base = addr;
 	cfg->ram_size = size;