| Message ID | 20200226202221.6555-3-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: Default vs Max policies | expand |
On 26.02.2020 21:22, Andrew Cooper wrote: > Drop XC_FEATUREMASK_DEEP_FEATURES. It isn't used by any callers, and unlike > the other static masks, won't be of interest to anyone without other pieces of > cpuid-autogen.h > > In xc_get_static_cpu_featuremask(), use a 2d array instead of individually > named variables, and drop the switch statement completely. > > No practical change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> with three remarks: > --- a/tools/libxc/xc_cpuid_x86.c > +++ b/tools/libxc/xc_cpuid_x86.c > @@ -90,43 +90,23 @@ uint32_t xc_get_cpu_featureset_size(void) > const uint32_t *xc_get_static_cpu_featuremask( > enum xc_static_cpu_featuremask mask) > { > - const static uint32_t known[FEATURESET_NR_ENTRIES] = INIT_KNOWN_FEATURES, > - special[FEATURESET_NR_ENTRIES] = INIT_SPECIAL_FEATURES, > - pv[FEATURESET_NR_ENTRIES] = INIT_PV_FEATURES, > - hvm_shadow[FEATURESET_NR_ENTRIES] = INIT_HVM_SHADOW_FEATURES, > - hvm_hap[FEATURESET_NR_ENTRIES] = INIT_HVM_HAP_FEATURES, > - deep_features[FEATURESET_NR_ENTRIES] = INIT_DEEP_FEATURES; > - > - BUILD_BUG_ON(ARRAY_SIZE(known) != FEATURESET_NR_ENTRIES); > - BUILD_BUG_ON(ARRAY_SIZE(special) != FEATURESET_NR_ENTRIES); > - BUILD_BUG_ON(ARRAY_SIZE(pv) != FEATURESET_NR_ENTRIES); > - BUILD_BUG_ON(ARRAY_SIZE(hvm_shadow) != FEATURESET_NR_ENTRIES); > - BUILD_BUG_ON(ARRAY_SIZE(hvm_hap) != FEATURESET_NR_ENTRIES); > - BUILD_BUG_ON(ARRAY_SIZE(deep_features) != FEATURESET_NR_ENTRIES); > - > - switch ( mask ) > - { > - case XC_FEATUREMASK_KNOWN: > - return known; > - > - case XC_FEATUREMASK_SPECIAL: > - return special; > - > - case XC_FEATUREMASK_PV: > - return pv; > + const static uint32_t masks[][FEATURESET_NR_ENTRIES] = { Would you mind switching to the more conventional "static const"? > +#define MASK(x) [XC_FEATUREMASK_ ## x] = INIT_ ## x ## _FEATURES I'm surprised to see you introduce such a construct, when more than once I heard you say that you dislike macros using ## in ways like it is done here. > + BUILD_BUG_ON(ARRAY_SIZE(masks[0]) != FEATURESET_NR_ENTRIES); Isn't this quite pointless with the now changed definition of the array? Jan
On 27/02/2020 07:47, Jan Beulich wrote: > On 26.02.2020 21:22, Andrew Cooper wrote: >> Drop XC_FEATUREMASK_DEEP_FEATURES. It isn't used by any callers, and unlike >> the other static masks, won't be of interest to anyone without other pieces of >> cpuid-autogen.h >> >> In xc_get_static_cpu_featuremask(), use a 2d array instead of individually >> named variables, and drop the switch statement completely. >> >> No practical change. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> > with three remarks: > >> --- a/tools/libxc/xc_cpuid_x86.c >> +++ b/tools/libxc/xc_cpuid_x86.c >> @@ -90,43 +90,23 @@ uint32_t xc_get_cpu_featureset_size(void) >> const uint32_t *xc_get_static_cpu_featuremask( >> enum xc_static_cpu_featuremask mask) >> { >> - const static uint32_t known[FEATURESET_NR_ENTRIES] = INIT_KNOWN_FEATURES, >> - special[FEATURESET_NR_ENTRIES] = INIT_SPECIAL_FEATURES, >> - pv[FEATURESET_NR_ENTRIES] = INIT_PV_FEATURES, >> - hvm_shadow[FEATURESET_NR_ENTRIES] = INIT_HVM_SHADOW_FEATURES, >> - hvm_hap[FEATURESET_NR_ENTRIES] = INIT_HVM_HAP_FEATURES, >> - deep_features[FEATURESET_NR_ENTRIES] = INIT_DEEP_FEATURES; >> - >> - BUILD_BUG_ON(ARRAY_SIZE(known) != FEATURESET_NR_ENTRIES); >> - BUILD_BUG_ON(ARRAY_SIZE(special) != FEATURESET_NR_ENTRIES); >> - BUILD_BUG_ON(ARRAY_SIZE(pv) != FEATURESET_NR_ENTRIES); >> - BUILD_BUG_ON(ARRAY_SIZE(hvm_shadow) != FEATURESET_NR_ENTRIES); >> - BUILD_BUG_ON(ARRAY_SIZE(hvm_hap) != FEATURESET_NR_ENTRIES); >> - BUILD_BUG_ON(ARRAY_SIZE(deep_features) != FEATURESET_NR_ENTRIES); >> - >> - switch ( mask ) >> - { >> - case XC_FEATUREMASK_KNOWN: >> - return known; >> - >> - case XC_FEATUREMASK_SPECIAL: >> - return special; >> - >> - case XC_FEATUREMASK_PV: >> - return pv; >> + const static uint32_t masks[][FEATURESET_NR_ENTRIES] = { > Would you mind switching to the more conventional "static const"? Ok. > >> +#define MASK(x) [XC_FEATUREMASK_ ## x] = INIT_ ## x ## _FEATURES > I'm surprised to see you introduce such a construct, when more > than once I heard you say that you dislike macros using ## in > ways like it is done here. It is all about positioning. Like this, the details are always visible (and clear) to anyone inspecting the function, because the macro only exists adjacent to its use. It is also not an interesting function (logic wise), so the fact it doesn't show up on trivial greps is a lesser evil, compared to the opencoded version. My issue with ## generally is that its use obfuscates logic, both in terms of hiding the operation going on, and making it difficult to locate related code patterns. > >> + BUILD_BUG_ON(ARRAY_SIZE(masks[0]) != FEATURESET_NR_ENTRIES); > Isn't this quite pointless with the now changed definition of > the array? I'd need to double check, but I suspect it is still necessary (in terms of the condition which might cause it to trip - whether this condition is an interesting thing to break the build over might be a different story). ~Andrew
On 27/02/2020 09:55, Andrew Cooper wrote: >>> + BUILD_BUG_ON(ARRAY_SIZE(masks[0]) != FEATURESET_NR_ENTRIES); >> Isn't this quite pointless with the now changed definition of >> the array? > I'd need to double check Double check says it isn't necessary. I'll drop. ~Andrew
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h index 99552a5f73..dec3c5de2b 100644 --- a/tools/libxc/include/xenctrl.h +++ b/tools/libxc/include/xenctrl.h @@ -2488,7 +2488,6 @@ enum xc_static_cpu_featuremask { XC_FEATUREMASK_PV, XC_FEATUREMASK_HVM_SHADOW, XC_FEATUREMASK_HVM_HAP, - XC_FEATUREMASK_DEEP_FEATURES, }; const uint32_t *xc_get_static_cpu_featuremask(enum xc_static_cpu_featuremask); diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c index 21b15b86ec..53cb72438a 100644 --- a/tools/libxc/xc_cpuid_x86.c +++ b/tools/libxc/xc_cpuid_x86.c @@ -90,43 +90,23 @@ uint32_t xc_get_cpu_featureset_size(void) const uint32_t *xc_get_static_cpu_featuremask( enum xc_static_cpu_featuremask mask) { - const static uint32_t known[FEATURESET_NR_ENTRIES] = INIT_KNOWN_FEATURES, - special[FEATURESET_NR_ENTRIES] = INIT_SPECIAL_FEATURES, - pv[FEATURESET_NR_ENTRIES] = INIT_PV_FEATURES, - hvm_shadow[FEATURESET_NR_ENTRIES] = INIT_HVM_SHADOW_FEATURES, - hvm_hap[FEATURESET_NR_ENTRIES] = INIT_HVM_HAP_FEATURES, - deep_features[FEATURESET_NR_ENTRIES] = INIT_DEEP_FEATURES; - - BUILD_BUG_ON(ARRAY_SIZE(known) != FEATURESET_NR_ENTRIES); - BUILD_BUG_ON(ARRAY_SIZE(special) != FEATURESET_NR_ENTRIES); - BUILD_BUG_ON(ARRAY_SIZE(pv) != FEATURESET_NR_ENTRIES); - BUILD_BUG_ON(ARRAY_SIZE(hvm_shadow) != FEATURESET_NR_ENTRIES); - BUILD_BUG_ON(ARRAY_SIZE(hvm_hap) != FEATURESET_NR_ENTRIES); - BUILD_BUG_ON(ARRAY_SIZE(deep_features) != FEATURESET_NR_ENTRIES); - - switch ( mask ) - { - case XC_FEATUREMASK_KNOWN: - return known; - - case XC_FEATUREMASK_SPECIAL: - return special; - - case XC_FEATUREMASK_PV: - return pv; + const static uint32_t masks[][FEATURESET_NR_ENTRIES] = { +#define MASK(x) [XC_FEATUREMASK_ ## x] = INIT_ ## x ## _FEATURES - case XC_FEATUREMASK_HVM_SHADOW: - return hvm_shadow; + MASK(KNOWN), + MASK(SPECIAL), + MASK(PV), + MASK(HVM_SHADOW), + MASK(HVM_HAP), - case XC_FEATUREMASK_HVM_HAP: - return hvm_hap; +#undef MASK + }; + BUILD_BUG_ON(ARRAY_SIZE(masks[0]) != FEATURESET_NR_ENTRIES); - case XC_FEATUREMASK_DEEP_FEATURES: - return deep_features; - - default: + if ( (unsigned int)mask >= ARRAY_SIZE(masks) ) return NULL; - } + + return masks[mask]; } int xc_get_cpu_policy_size(xc_interface *xch, uint32_t *nr_leaves,
Drop XC_FEATUREMASK_DEEP_FEATURES. It isn't used by any callers, and unlike the other static masks, won't be of interest to anyone without other pieces of cpuid-autogen.h In xc_get_static_cpu_featuremask(), use a 2d array instead of individually named variables, and drop the switch statement completely. No practical change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wl@xen.org> CC: Roger Pau Monné <roger.pau@citrix.com> --- tools/libxc/include/xenctrl.h | 1 - tools/libxc/xc_cpuid_x86.c | 46 ++++++++++++------------------------------- 2 files changed, 13 insertions(+), 34 deletions(-)