Re: [PATCH v4 06/12] xen/common: add dom0 xen command line argument for Arm

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.03.2023 12:59, Luca Fancellu wrote:
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -59,6 +59,11 @@ static int __init parse_dom0_mem(const char *s)
>  }
>  custom_param("dom0_mem", parse_dom0_mem);
>  
> +int __init parse_arch_dom0_param(const char *str_begin, const char *str_end)
> +{
> +    return -1;
> +}

Please also use -E... here, like x86 does.

> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -266,42 +266,30 @@ bool __initdata opt_dom0_pvh = !IS_ENABLED(CONFIG_PV);
>  bool __initdata opt_dom0_verbose = IS_ENABLED(CONFIG_VERBOSE_DEBUG);
>  bool __initdata opt_dom0_msr_relaxed;
>  
> -static int __init cf_check parse_dom0_param(const char *s)
> +int __init parse_arch_dom0_param(const char *str_begin, const char *str_end)

Is there any reason you couldn't stick to the original variable names (s
and ss) or use other meaningful by shorter names like s and e or str and
end (my personal preference among the three in the order given)? That'll
help with line length and hence readability.

>  {
> -    const char *ss;
> -    int rc = 0;
> -
> -    do {
> -        int val;
> -
> -        ss = strchr(s, ',');
> -        if ( !ss )
> -            ss = strchr(s, '\0');
> +    int val, rc = 0;
>  
> -        if ( IS_ENABLED(CONFIG_PV) && !cmdline_strcmp(s, "pv") )
> -            opt_dom0_pvh = false;
> -        else if ( IS_ENABLED(CONFIG_HVM) && !cmdline_strcmp(s, "pvh") )
> -            opt_dom0_pvh = true;
> +    if ( IS_ENABLED(CONFIG_PV) && !cmdline_strcmp(str_begin, "pv") )
> +        opt_dom0_pvh = false;
> +    else if ( IS_ENABLED(CONFIG_HVM) && !cmdline_strcmp(str_begin, "pvh") )
> +        opt_dom0_pvh = true;
>  #ifdef CONFIG_SHADOW_PAGING
> -        else if ( (val = parse_boolean("shadow", s, ss)) >= 0 )
> -            opt_dom0_shadow = val;
> +    else if ( (val = parse_boolean("shadow", str_begin, str_end)) >= 0 )
> +        opt_dom0_shadow = val;
>  #endif
> -        else if ( (val = parse_boolean("verbose", s, ss)) >= 0 )
> -            opt_dom0_verbose = val;
> -        else if ( IS_ENABLED(CONFIG_PV) &&
> -                  (val = parse_boolean("cpuid-faulting", s, ss)) >= 0 )
> -            opt_dom0_cpuid_faulting = val;
> -        else if ( (val = parse_boolean("msr-relaxed", s, ss)) >= 0 )
> -            opt_dom0_msr_relaxed = val;
> -        else
> -            rc = -EINVAL;
> -
> -        s = ss + 1;
> -    } while ( *ss );
> +    else if ( (val = parse_boolean("verbose", str_begin, str_end)) >= 0 )
> +        opt_dom0_verbose = val;
> +    else if ( IS_ENABLED(CONFIG_PV) &&
> +              (val = parse_boolean("cpuid-faulting", str_begin, str_end)) >= 
> 0 )
> +        opt_dom0_cpuid_faulting = val;
> +    else if ( (val = parse_boolean("msr-relaxed", str_begin, str_end)) >= 0 )
> +        opt_dom0_msr_relaxed = val;
> +    else
> +        rc = -EINVAL;
>  
>      return rc;
>  }

While largely a style aspect (and hence I'm not going to insist), I don't
see the value of the "rc" local variable with the changed logic. With at
least the other aspects addressed
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan