| Message ID | 20200226202221.6555-5-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: > For now, write the same content for both. Update the users of the > initialisers to use the new name, and extend xen-cpuid to dump both default > and max featuresets. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Hypervisor and libxc parts Reviewed-by: Jan Beulich <jbeulich@suse.com> > --- a/tools/misc/xen-cpuid.c > +++ b/tools/misc/xen-cpuid.c > @@ -283,12 +283,18 @@ static void dump_info(xc_interface *xch, bool detail) > nr_features, "Known", detail); > decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_SPECIAL), > nr_features, "Special", detail); > - decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_PV), > - nr_features, "PV Mask", detail); > - decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_SHADOW), > - nr_features, "HVM Shadow Mask", detail); > - decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_HAP), > - nr_features, "HVM Hap Mask", detail); > + decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_PV_MAX), > + nr_features, "PV Max", detail); > + decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_PV_DEF), > + nr_features, "PV Default", detail); > + decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_SHADOW_MAX), > + nr_features, "HVM Shadow Max", detail); > + decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_SHADOW_DEF), > + nr_features, "HVM Shadow Default", detail); > + decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_HAP_MAX), > + nr_features, "HVM Hap Max", detail); > + decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_HAP_DEF), > + nr_features, "HVM Hap Default", detail); Spotting differences between max and default this way is, I assume, going to be quite difficult / error prone. Wouldn't it be better to produce the default set in full, and then list just the extra items in max? Aiui max is always going to be a superset of def. Jan
On 27/02/2020 08:02, Jan Beulich wrote: > On 26.02.2020 21:22, Andrew Cooper wrote: >> For now, write the same content for both. Update the users of the >> initialisers to use the new name, and extend xen-cpuid to dump both default >> and max featuresets. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Hypervisor and libxc parts > Reviewed-by: Jan Beulich <jbeulich@suse.com> Which other bit are you concerned with? xen-cpuid.c is explicitly under x86 maintainership. > >> --- a/tools/misc/xen-cpuid.c >> +++ b/tools/misc/xen-cpuid.c >> @@ -283,12 +283,18 @@ static void dump_info(xc_interface *xch, bool detail) >> nr_features, "Known", detail); >> decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_SPECIAL), >> nr_features, "Special", detail); >> - decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_PV), >> - nr_features, "PV Mask", detail); >> - decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_SHADOW), >> - nr_features, "HVM Shadow Mask", detail); >> - decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_HAP), >> - nr_features, "HVM Hap Mask", detail); >> + decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_PV_MAX), >> + nr_features, "PV Max", detail); >> + decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_PV_DEF), >> + nr_features, "PV Default", detail); >> + decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_SHADOW_MAX), >> + nr_features, "HVM Shadow Max", detail); >> + decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_SHADOW_DEF), >> + nr_features, "HVM Shadow Default", detail); >> + decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_HAP_MAX), >> + nr_features, "HVM Hap Max", detail); >> + decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_HAP_DEF), >> + nr_features, "HVM Hap Default", detail); > Spotting differences between max and default this way is, I assume, > going to be quite difficult / error prone. Not any more or less than between other similar sets (most obviously, hap and shadow, but raw and host also tend to fairly similar). > Wouldn't it be better to > produce the default set in full, and then list just the extra items > in max? I don't see how that would work. The sets are either rendered as a hex bitmap (so spotting a different is fairly easy), or tabulated with feature names subdivided by word. > Aiui max is always going to be a superset of def. It is. I did consider distinguishing using lower and upper case, which is about the only way I can think of sensibly merging the two sets together. However, this is a pain to do in C, and it would result in the set being rendered differently depending on whether it was a static set, or a user-provided one. It would also result in the case being inverted compared to the annotation character. For now, I'm honestly not sure that it matters too much. I'm probably going to give xen-cpuid an overhaul anyway (perhaps into python) to be a more useful calculator for policy settings. ~Andrew
On 27.02.2020 11:29, Andrew Cooper wrote: > On 27/02/2020 08:02, Jan Beulich wrote: >> On 26.02.2020 21:22, Andrew Cooper wrote: >>> For now, write the same content for both. Update the users of the >>> initialisers to use the new name, and extend xen-cpuid to dump both default >>> and max featuresets. >>> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> Hypervisor and libxc parts >> Reviewed-by: Jan Beulich <jbeulich@suse.com> > > Which other bit are you concerned with? xen-cpuid.c is explicitly under > x86 maintainership. > >> >>> --- a/tools/misc/xen-cpuid.c >>> +++ b/tools/misc/xen-cpuid.c >>> @@ -283,12 +283,18 @@ static void dump_info(xc_interface *xch, bool detail) >>> nr_features, "Known", detail); >>> decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_SPECIAL), >>> nr_features, "Special", detail); >>> - decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_PV), >>> - nr_features, "PV Mask", detail); >>> - decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_SHADOW), >>> - nr_features, "HVM Shadow Mask", detail); >>> - decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_HAP), >>> - nr_features, "HVM Hap Mask", detail); >>> + decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_PV_MAX), >>> + nr_features, "PV Max", detail); >>> + decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_PV_DEF), >>> + nr_features, "PV Default", detail); >>> + decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_SHADOW_MAX), >>> + nr_features, "HVM Shadow Max", detail); >>> + decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_SHADOW_DEF), >>> + nr_features, "HVM Shadow Default", detail); >>> + decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_HAP_MAX), >>> + nr_features, "HVM Hap Max", detail); >>> + decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_HAP_DEF), >>> + nr_features, "HVM Hap Default", detail); >> Spotting differences between max and default this way is, I assume, >> going to be quite difficult / error prone. > > Not any more or less than between other similar sets (most obviously, > hap and shadow, but raw and host also tend to fairly similar). Well, yes, but it may matter more for the (def,max) pairs. >> Wouldn't it be better to >> produce the default set in full, and then list just the extra items >> in max? > > I don't see how that would work. The sets are either rendered as a hex > bitmap (so spotting a different is fairly easy), or tabulated with > feature names subdivided by word. For the hex printing I agree it's fine this way. For the tabulated printing, why not simply mask out all "default" bits from "max" before producing the output, adjusting the headline accordingly? >> Aiui max is always going to be a superset of def. > > It is. I did consider distinguishing using lower and upper case, which > is about the only way I can think of sensibly merging the two sets together. > > However, this is a pain to do in C, and it would result in the set being > rendered differently depending on whether it was a static set, or a > user-provided one. It would also result in the case being inverted > compared to the annotation character. > > For now, I'm honestly not sure that it matters too much. I'm probably > going to give xen-cpuid an overhaul anyway (perhaps into python) to be a > more useful calculator for policy settings. Oh, okay. In that case perhaps indeed not worth to spend too much effort here anymore. Jan
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h index dec3c5de2b..fc6e57a1a0 100644 --- a/tools/libxc/include/xenctrl.h +++ b/tools/libxc/include/xenctrl.h @@ -2485,9 +2485,12 @@ uint32_t xc_get_cpu_featureset_size(void); enum xc_static_cpu_featuremask { XC_FEATUREMASK_KNOWN, XC_FEATUREMASK_SPECIAL, - XC_FEATUREMASK_PV, - XC_FEATUREMASK_HVM_SHADOW, - XC_FEATUREMASK_HVM_HAP, + XC_FEATUREMASK_PV_MAX, + XC_FEATUREMASK_PV_DEF, + XC_FEATUREMASK_HVM_SHADOW_MAX, + XC_FEATUREMASK_HVM_SHADOW_DEF, + XC_FEATUREMASK_HVM_HAP_MAX, + XC_FEATUREMASK_HVM_HAP_DEF, }; 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 53cb72438a..c2ea0db25c 100644 --- a/tools/libxc/xc_cpuid_x86.c +++ b/tools/libxc/xc_cpuid_x86.c @@ -95,9 +95,12 @@ const uint32_t *xc_get_static_cpu_featuremask( MASK(KNOWN), MASK(SPECIAL), - MASK(PV), - MASK(HVM_SHADOW), - MASK(HVM_HAP), + MASK(PV_MAX), + MASK(PV_DEF), + MASK(HVM_SHADOW_MAX), + MASK(HVM_SHADOW_DEF), + MASK(HVM_HAP_MAX), + MASK(HVM_HAP_DEF), #undef MASK }; diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c index 36c17bf777..585b530b21 100644 --- a/tools/misc/xen-cpuid.c +++ b/tools/misc/xen-cpuid.c @@ -283,12 +283,18 @@ static void dump_info(xc_interface *xch, bool detail) nr_features, "Known", detail); decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_SPECIAL), nr_features, "Special", detail); - decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_PV), - nr_features, "PV Mask", detail); - decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_SHADOW), - nr_features, "HVM Shadow Mask", detail); - decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_HAP), - nr_features, "HVM Hap Mask", detail); + decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_PV_MAX), + nr_features, "PV Max", detail); + decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_PV_DEF), + nr_features, "PV Default", detail); + decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_SHADOW_MAX), + nr_features, "HVM Shadow Max", detail); + decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_SHADOW_DEF), + nr_features, "HVM Shadow Default", detail); + decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_HAP_MAX), + nr_features, "HVM Hap Max", detail); + decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_HAP_DEF), + nr_features, "HVM Hap Default", detail); printf("\nDynamic sets:\n"); for ( i = 0; i < ARRAY_SIZE(featuresets); ++i ) diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c index aee221dc44..546ae31bb9 100644 --- a/xen/arch/x86/cpuid.c +++ b/xen/arch/x86/cpuid.c @@ -15,9 +15,9 @@ const uint32_t known_features[] = INIT_KNOWN_FEATURES; const uint32_t special_features[] = INIT_SPECIAL_FEATURES; -static const uint32_t pv_featuremask[] = INIT_PV_FEATURES; -static const uint32_t hvm_shadow_featuremask[] = INIT_HVM_SHADOW_FEATURES; -static const uint32_t hvm_hap_featuremask[] = INIT_HVM_HAP_FEATURES; +static const uint32_t pv_max_featuremask[] = INIT_PV_MAX_FEATURES; +static const uint32_t hvm_shadow_max_featuremask[] = INIT_HVM_SHADOW_MAX_FEATURES; +static const uint32_t hvm_hap_max_featuremask[] = INIT_HVM_HAP_MAX_FEATURES; static const uint32_t deep_features[] = INIT_DEEP_FEATURES; static int __init parse_xen_cpuid(const char *s) @@ -359,7 +359,7 @@ static void __init calculate_pv_max_policy(void) cpuid_policy_to_featureset(p, pv_featureset); for ( i = 0; i < ARRAY_SIZE(pv_featureset); ++i ) - pv_featureset[i] &= pv_featuremask[i]; + pv_featureset[i] &= pv_max_featuremask[i]; /* * If Xen isn't virtualising MSR_SPEC_CTRL for PV guests because of @@ -391,7 +391,7 @@ static void __init calculate_hvm_max_policy(void) cpuid_policy_to_featureset(p, hvm_featureset); hvm_featuremask = hvm_hap_supported() ? - hvm_hap_featuremask : hvm_shadow_featuremask; + hvm_hap_max_featuremask : hvm_shadow_max_featuremask; for ( i = 0; i < ARRAY_SIZE(hvm_featureset); ++i ) hvm_featureset[i] &= hvm_featuremask[i]; @@ -500,7 +500,7 @@ void recalculate_cpuid_policy(struct domain *d) if ( !hap_enabled(d) ) { for ( i = 0; i < ARRAY_SIZE(max_fs); i++ ) - max_fs[i] &= hvm_shadow_featuremask[i]; + max_fs[i] &= hvm_shadow_max_featuremask[i]; } /* Hide nested-virt if it hasn't been explicitly configured. */ @@ -964,7 +964,7 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf, /* * PSE36 is not supported in shadow mode. This bit should be - * clear in hvm_shadow_featuremask[]. + * clear in hvm_shadow_max_featuremask[]. * * However, an unspecified version of Hyper-V from 2011 refuses to * start as the "cpu does not provide required hw features" if it @@ -1003,9 +1003,9 @@ static void __init __maybe_unused build_assertions(void) { BUILD_BUG_ON(ARRAY_SIZE(known_features) != FSCAPINTS); BUILD_BUG_ON(ARRAY_SIZE(special_features) != FSCAPINTS); - BUILD_BUG_ON(ARRAY_SIZE(pv_featuremask) != FSCAPINTS); - BUILD_BUG_ON(ARRAY_SIZE(hvm_shadow_featuremask) != FSCAPINTS); - BUILD_BUG_ON(ARRAY_SIZE(hvm_hap_featuremask) != FSCAPINTS); + BUILD_BUG_ON(ARRAY_SIZE(pv_max_featuremask) != FSCAPINTS); + BUILD_BUG_ON(ARRAY_SIZE(hvm_shadow_max_featuremask) != FSCAPINTS); + BUILD_BUG_ON(ARRAY_SIZE(hvm_hap_max_featuremask) != FSCAPINTS); BUILD_BUG_ON(ARRAY_SIZE(deep_features) != FSCAPINTS); /* Find some more clever allocation scheme if this trips. */ diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py index 99b2e7aeee..af5610a5e6 100755 --- a/xen/tools/gen-cpuid.py +++ b/xen/tools/gen-cpuid.py @@ -28,9 +28,12 @@ def __init__(self, input, output): # State calculated self.nr_entries = 0 # Number of words in a featureset self.common_1d = 0 # Common features between 1d and e1d - self.pv = set() # PV features - self.hvm_shadow = set() # HVM shadow features - self.hvm_hap = set() # HVM HAP features + self.pv_def = set() # PV default features + self.hvm_shadow_def = set() # HVM shadow default features + self.hvm_hap_def = set() # HVM HAP default features + self.pv_max = set() # PV max features + self.hvm_shadow_max = set() # HVM shadow max features + self.hvm_hap_max = set() # HVM HAP max features self.bitfields = [] # Text to declare named bitfields in C self.deep_deps = {} # { feature num => dependant features } self.nr_deep_deps = 0 # Number of entries in deep_deps @@ -126,9 +129,13 @@ def crunch_numbers(state): MTRR, PGE, MCA, CMOV, PAT, PSE36, MMX, FXSR) state.common_1d = common_1d - state.pv = state.raw['A'] - state.hvm_shadow = state.pv | state.raw['S'] - state.hvm_hap = state.hvm_shadow | state.raw['H'] + state.pv_def = state.raw['A'] + state.hvm_shadow_def = state.pv_def | state.raw['S'] + state.hvm_hap_def = state.hvm_shadow_def | state.raw['H'] + + state.pv_max = state.pv_def + state.hvm_shadow_max = state.hvm_shadow_def + state.hvm_hap_max = state.hvm_hap_def # # Feature dependency information. @@ -351,11 +358,17 @@ def write_results(state): #define INIT_SPECIAL_FEATURES { \\\n%s\n} -#define INIT_PV_FEATURES { \\\n%s\n} +#define INIT_PV_DEF_FEATURES { \\\n%s\n} + +#define INIT_PV_MAX_FEATURES { \\\n%s\n} + +#define INIT_HVM_SHADOW_DEF_FEATURES { \\\n%s\n} + +#define INIT_HVM_SHADOW_MAX_FEATURES { \\\n%s\n} -#define INIT_HVM_SHADOW_FEATURES { \\\n%s\n} +#define INIT_HVM_HAP_DEF_FEATURES { \\\n%s\n} -#define INIT_HVM_HAP_FEATURES { \\\n%s\n} +#define INIT_HVM_HAP_MAX_FEATURES { \\\n%s\n} #define NR_DEEP_DEPS %sU @@ -366,9 +379,12 @@ def write_results(state): next(featureset_to_uint32s(state.common_1d, 1)), format_uint32s(state, state.names.keys(), 4), format_uint32s(state, state.raw['!'], 4), - format_uint32s(state, state.pv, 4), - format_uint32s(state, state.hvm_shadow, 4), - format_uint32s(state, state.hvm_hap, 4), + format_uint32s(state, state.pv_def, 4), + format_uint32s(state, state.pv_max, 4), + format_uint32s(state, state.hvm_shadow_def, 4), + format_uint32s(state, state.hvm_shadow_max, 4), + format_uint32s(state, state.hvm_hap_def, 4), + format_uint32s(state, state.hvm_hap_max, 4), state.nr_deep_deps, format_uint32s(state, state.deep_features, 4), ))
For now, write the same content for both. Update the users of the initialisers to use the new name, and extend xen-cpuid to dump both default and max featuresets. 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 | 9 ++++++--- tools/libxc/xc_cpuid_x86.c | 9 ++++++--- tools/misc/xen-cpuid.c | 18 ++++++++++++------ xen/arch/x86/cpuid.c | 20 ++++++++++---------- xen/tools/gen-cpuid.py | 40 ++++++++++++++++++++++++++++------------ 5 files changed, 62 insertions(+), 34 deletions(-)