| Message ID | 20230327105944.1360856-7-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | SVE feature for arm guests | expand |
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@suse.com> Jan
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 9707eb7b1bb1..35dbe964fc8b 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 *str_begin, const char *str_end) +{ + return -1; +} + /* 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..5e143814c854 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 *str_begin, const char *str_end) { - 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; } -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..b1259a3392b2 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 *str_begin, const char *str_end); struct vcpu *alloc_dom0_vcpu0(struct domain *dom0); int vcpu_reset(struct vcpu *);
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@arm.com> --- Changes from v3: - new patch --- xen/arch/arm/domain_build.c | 5 ++++ xen/arch/x86/dom0_build.c | 46 ++++++++++++++----------------------- xen/common/domain.c | 23 +++++++++++++++++++ xen/include/xen/domain.h | 1 + 4 files changed, 46 insertions(+), 29 deletions(-)