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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Luca,

> On 12 Apr 2023, at 11:49, Luca Fancellu <Luca.Fancellu@xxxxxxx> wrote:
> 
> Currently x86 defines a Xen command line argument dom0=<list> where
> there can be specified dom0 controlling sub-options, to use it also
> on Arm, move the code that loops through the list of arguments from
> x86 to the common code and from there, call architecture specific
> functions to handle the comma separated sub-options.
> 
> No functional changes are intended.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>

Cheers
Bertrand


> ---
> Changes from v4:
> - return EINVAL in Arm implementation of parse_arch_dom0_param,
>   shorten variable name in the funtion from str_begin, str_end to
>   s, e. Removed variable rc from x86 parse_arch_dom0_param
>   implementation. (Jan)
> - Add R-By Jan
> Changes from v3:
> - new patch
> ---
> xen/arch/arm/domain_build.c |  5 ++++
> xen/arch/x86/dom0_build.c   | 48 ++++++++++++++-----------------------
> xen/common/domain.c         | 23 ++++++++++++++++++
> xen/include/xen/domain.h    |  1 +
> 4 files changed, 47 insertions(+), 30 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 4f9d4f9d8867..eeb4662f0eee 100644
> --- 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 *s, const char *e)
> +{
> +    return -EINVAL;
> +}
> +
> /* Override macros from asm/page.h to make them work with mfn_t */
> #undef virt_to_mfn
> #define virt_to_mfn(va) _mfn(__virt_to_mfn(va))
> diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
> index 79234f18ff01..9f5300a3efbb 100644
> --- 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 *s, const char *e)
> {
> -    const char *ss;
> -    int rc = 0;
> +    int val;
> 
> -    do {
> -        int val;
> -
> -        ss = strchr(s, ',');
> -        if ( !ss )
> -            ss = strchr(s, '\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(s, "pv") )
> +        opt_dom0_pvh = false;
> +    else if ( IS_ENABLED(CONFIG_HVM) && !cmdline_strcmp(s, "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", s, e)) >= 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", s, e)) >= 0 )
> +        opt_dom0_verbose = val;
> +    else if ( IS_ENABLED(CONFIG_PV) &&
> +              (val = parse_boolean("cpuid-faulting", s, e)) >= 0 )
> +        opt_dom0_cpuid_faulting = val;
> +    else if ( (val = parse_boolean("msr-relaxed", s, e)) >= 0 )
> +        opt_dom0_msr_relaxed = val;
> +    else
> +        return -EINVAL;
> 
> -    return rc;
> +    return 0;
> }
> -custom_param("dom0", parse_dom0_param);
> 
> static char __initdata opt_dom0_ioports_disable[200] = "";
> string_param("dom0_ioports_disable", opt_dom0_ioports_disable);
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 626debbae095..7779ba088675 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -364,6 +364,29 @@ static int __init cf_check parse_extra_guest_irqs(const 
> char *s)
> }
> custom_param("extra_guest_irqs", parse_extra_guest_irqs);
> 
> +static int __init cf_check parse_dom0_param(const char *s)
> +{
> +    const char *ss;
> +    int rc = 0;
> +
> +    do {
> +        int ret;
> +
> +        ss = strchr(s, ',');
> +        if ( !ss )
> +            ss = strchr(s, '\0');
> +
> +        ret = parse_arch_dom0_param(s, ss);
> +        if ( ret && !rc )
> +            rc = ret;
> +
> +        s = ss + 1;
> +    } while ( *ss );
> +
> +    return rc;
> +}
> +custom_param("dom0", parse_dom0_param);
> +
> /*
>  * Release resources held by a domain.  There may or may not be live
>  * references to the domain, and it may or may not be fully constructed.
> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
> index 26f9c4f6dd5b..1df8f933d076 100644
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -16,6 +16,7 @@ typedef union {
> struct vcpu *vcpu_create(struct domain *d, unsigned int vcpu_id);
> 
> unsigned int dom0_max_vcpus(void);
> +int parse_arch_dom0_param(const char *s, const char *e);
> struct vcpu *alloc_dom0_vcpu0(struct domain *dom0);
> 
> int vcpu_reset(struct vcpu *);
> -- 
> 2.34.1
>