On Mon, 23 Sep 2019 14:35:16 +0100
Alexandru Elisei <alexandru.elisei@xxxxxxx> wrote:
Hi,
> From: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
>
> 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@xxxxxxx>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
> ---
> 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;
