Message ID | 20221004160810.25364-2-andrew.cooper3@citrix.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | x86: Activate DOITM | expand |
On 04.10.2022 18:08, Andrew Cooper wrote: > A future change will want a cpuid-like identifier which doesn't have a mapping > to a feature bit. > > * Pass the feature name into the parse callback. > * Exclude a feature value of ~0u from falling into the general set/clear bit > paths. > * In gen-cpuid.py, insert a placeholder to collect all the pseudo feature > names. Hmm, I was envisioning this to be fitted into the existing model in a less adhoc way: (parts of) MSRs holding feature bits aren't very different from individual (pairs of) registers of CPUID output (in the case of ARCH_CAPS there would be a perhaps just abstract mask limiting things to the subset of bits which actually act as feature indicators). Hence I'd have expected them to gain proper entries in the public interface (cpufeatureset.h) and then be represented / processed the same way in featuresets and policies. All that would be left out at this point would be the exposing of the bit to guests (in patch 2, assuming the split into two patches is then actually still warranted), integration into guest_rdmsr(), and at least some of the tool stack side (xen-cpuid, for example, could easily learn of such right away). However, since I'm pretty sure you've considered such an approach, I guess I might be overlooking some caveat? > --- a/xen/tools/gen-cpuid.py > +++ b/xen/tools/gen-cpuid.py > @@ -297,6 +297,19 @@ def crunch_numbers(state): > RTM: [TSXLDTRK], > } > > + # > + # Pseudo feature names. These don't map to a feature bit, but are > + # inserted into the values dictionary so they can be parsed and handled > + # specially > + # > + pseduo_names = ( > + ) > + > + for n in pseduo_names: > + if n in state.values: > + raise Fail("Pseduo feature name %s aliases real feature" % (n, )) > + state.values[n] = 0xffffffff Throughout this hunk: s/pseduo/pseudo/g. Jan
On Tue, Oct 04, 2022 at 05:08:09PM +0100, Andrew Cooper wrote: > A future change will want a cpuid-like identifier which doesn't have a mapping > to a feature bit. Why we prefer this logic over just giving such pseudo features a synthetic bit or akin? Could we have a bit more of a description about what would be considered a pseudo feature? Thanks, Roger.
On 05/10/2022 07:59, Jan Beulich wrote: > On 04.10.2022 18:08, Andrew Cooper wrote: >> A future change will want a cpuid-like identifier which doesn't have a mapping >> to a feature bit. >> >> * Pass the feature name into the parse callback. >> * Exclude a feature value of ~0u from falling into the general set/clear bit >> paths. >> * In gen-cpuid.py, insert a placeholder to collect all the pseudo feature >> names. > Hmm, I was envisioning this to be fitted into the existing model in a > less adhoc way: (parts of) MSRs holding feature bits aren't very different > from individual (pairs of) registers of CPUID output (in the case of > ARCH_CAPS there would be a perhaps just abstract mask limiting things to > the subset of bits which actually act as feature indicators). Hence I'd > have expected them to gain proper entries in the public interface > (cpufeatureset.h) and then be represented / processed the same way in > featuresets and policies. All that would be left out at this point would > be the exposing of the bit to guests (in patch 2, assuming the split into > two patches is then actually still warranted), integration into > guest_rdmsr(), and at least some of the tool stack side (xen-cpuid, for > example, could easily learn of such right away). > > However, since I'm pretty sure you've considered such an approach, I guess > I might be overlooking some caveat? I have on multiple occasions considered putting MSR_ARCH_CAPS into the existing X86_FEATURE_* infrastructure. In the future, it's likely the right course of action to take. However, doing so now will break speculation safety for guests in some cases. The featureset API intended to be safe to treat as a regular bitmap, and this is how it is used in practice. Honestly, I didn't imagine that we'd get bits like RSBA and RRSBA that need to be treated with opposite polarity to be levelled safely. Even if we do end up putting MSR_ARCH_CAPS in X86_FEATURE_*, we still need to remove the featureset api (replaced by the cpu policy infrastructure and libx86) to retain migration safety. > >> --- a/xen/tools/gen-cpuid.py >> +++ b/xen/tools/gen-cpuid.py >> @@ -297,6 +297,19 @@ def crunch_numbers(state): >> RTM: [TSXLDTRK], >> } >> >> + # >> + # Pseudo feature names. These don't map to a feature bit, but are >> + # inserted into the values dictionary so they can be parsed and handled >> + # specially >> + # >> + pseduo_names = ( >> + ) >> + >> + for n in pseduo_names: >> + if n in state.values: >> + raise Fail("Pseduo feature name %s aliases real feature" % (n, )) >> + state.values[n] = 0xffffffff > Throughout this hunk: s/pseduo/pseudo/g. Oops, yes. Will fix. ~Andrew
On 05.10.2022 14:58, Andrew Cooper wrote: > On 05/10/2022 07:59, Jan Beulich wrote: >> On 04.10.2022 18:08, Andrew Cooper wrote: >>> A future change will want a cpuid-like identifier which doesn't have a mapping >>> to a feature bit. >>> >>> * Pass the feature name into the parse callback. >>> * Exclude a feature value of ~0u from falling into the general set/clear bit >>> paths. >>> * In gen-cpuid.py, insert a placeholder to collect all the pseudo feature >>> names. >> Hmm, I was envisioning this to be fitted into the existing model in a >> less adhoc way: (parts of) MSRs holding feature bits aren't very different >> from individual (pairs of) registers of CPUID output (in the case of >> ARCH_CAPS there would be a perhaps just abstract mask limiting things to >> the subset of bits which actually act as feature indicators). Hence I'd >> have expected them to gain proper entries in the public interface >> (cpufeatureset.h) and then be represented / processed the same way in >> featuresets and policies. All that would be left out at this point would >> be the exposing of the bit to guests (in patch 2, assuming the split into >> two patches is then actually still warranted), integration into >> guest_rdmsr(), and at least some of the tool stack side (xen-cpuid, for >> example, could easily learn of such right away). >> >> However, since I'm pretty sure you've considered such an approach, I guess >> I might be overlooking some caveat? > > I have on multiple occasions considered putting MSR_ARCH_CAPS into the > existing X86_FEATURE_* infrastructure. In the future, it's likely the > right course of action to take. > > However, doing so now will break speculation safety for guests in some Considering further text - s/speculation/migration/, I guess? > cases. The featureset API intended to be safe to treat as a regular > bitmap, and this is how it is used in practice. > > Honestly, I didn't imagine that we'd get bits like RSBA and RRSBA that > need to be treated with opposite polarity to be levelled safely. > > Even if we do end up putting MSR_ARCH_CAPS in X86_FEATURE_*, we still > need to remove the featureset api (replaced by the cpu policy > infrastructure and libx86) to retain migration safety. Well, I didn't mean to suggest to put all of the feature-like bits there right away. Just the one bit we're after would do for now. Afaict that wouldn't affect migration safety, while it would allow doing things in Xen in a more "natural" way. Jan
On 05/10/2022 10:55, Roger Pau Monné wrote: > On Tue, Oct 04, 2022 at 05:08:09PM +0100, Andrew Cooper wrote: >> A future change will want a cpuid-like identifier which doesn't have a mapping >> to a feature bit. > Why we prefer this logic over just giving such pseudo features a > synthetic bit or akin? Synthetic bits are (intentionally) not available to cmdline parsing. We need something that is available for parsing. > Could we have a bit more of a description about what would be > considered a pseudo feature? I don't really have anything further to add beyond the comment in gen-cpuid.py It's a misc collection of things requiring special handling. ~Andrew
On 05/10/2022 14:11, Jan Beulich wrote: > On 05.10.2022 14:58, Andrew Cooper wrote: >> On 05/10/2022 07:59, Jan Beulich wrote: >>> On 04.10.2022 18:08, Andrew Cooper wrote: >>>> A future change will want a cpuid-like identifier which doesn't have a mapping >>>> to a feature bit. >>>> >>>> * Pass the feature name into the parse callback. >>>> * Exclude a feature value of ~0u from falling into the general set/clear bit >>>> paths. >>>> * In gen-cpuid.py, insert a placeholder to collect all the pseudo feature >>>> names. >>> Hmm, I was envisioning this to be fitted into the existing model in a >>> less adhoc way: (parts of) MSRs holding feature bits aren't very different >>> from individual (pairs of) registers of CPUID output (in the case of >>> ARCH_CAPS there would be a perhaps just abstract mask limiting things to >>> the subset of bits which actually act as feature indicators). Hence I'd >>> have expected them to gain proper entries in the public interface >>> (cpufeatureset.h) and then be represented / processed the same way in >>> featuresets and policies. All that would be left out at this point would >>> be the exposing of the bit to guests (in patch 2, assuming the split into >>> two patches is then actually still warranted), integration into >>> guest_rdmsr(), and at least some of the tool stack side (xen-cpuid, for >>> example, could easily learn of such right away). >>> >>> However, since I'm pretty sure you've considered such an approach, I guess >>> I might be overlooking some caveat? >> I have on multiple occasions considered putting MSR_ARCH_CAPS into the >> existing X86_FEATURE_* infrastructure. In the future, it's likely the >> right course of action to take. >> >> However, doing so now will break speculation safety for guests in some > Considering further text - s/speculation/migration/, I guess? More "speculation safety on migrate". Except its not just on migrate. It can go wrong for suspend/resume on the same host across a reboot which changes the microcode version. >> cases. The featureset API intended to be safe to treat as a regular >> bitmap, and this is how it is used in practice. >> >> Honestly, I didn't imagine that we'd get bits like RSBA and RRSBA that >> need to be treated with opposite polarity to be levelled safely. >> >> Even if we do end up putting MSR_ARCH_CAPS in X86_FEATURE_*, we still >> need to remove the featureset api (replaced by the cpu policy >> infrastructure and libx86) to retain migration safety. > Well, I didn't mean to suggest to put all of the feature-like bits there > right away. Just the one bit we're after would do for now. Afaict that > wouldn't affect migration safety, while it would allow doing things in > Xen in a more "natural" way. That requires reworking how the MSR policies get set up, patches for which have been stagnent on xen-devel, as well as a reasonable amount of plumbing because the featureset<->policy conversions which only have CPUID data right now. If I go down this route, realise it is going to have to go out in an XSA so will be backported, and that you're also committing to taking the VMX MSRs in due course. (which is all on my plan, but I don't want to start the work now and have an argument down the line.) ~Andrew
On Wed, Oct 05, 2022 at 01:34:06PM +0000, Andrew Cooper wrote: > On 05/10/2022 10:55, Roger Pau Monné wrote: > > On Tue, Oct 04, 2022 at 05:08:09PM +0100, Andrew Cooper wrote: > >> A future change will want a cpuid-like identifier which doesn't have a mapping > >> to a feature bit. > > Why we prefer this logic over just giving such pseudo features a > > synthetic bit or akin? > > Synthetic bits are (intentionally) not available to cmdline parsing. We > need something that is available for parsing. I think Jan expressed my view better, in that it would be nicer to just have the MSR_ARCH_CAPS bits in the featureset, and listed in cpufeatureset.h like we handle CPUID features. Maybe we want to go your proposed route now if that's easier however. Thanks, Roger.
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c index 822f9ace1087..112ee63a9449 100644 --- a/xen/arch/x86/cpuid.c +++ b/xen/arch/x86/cpuid.c @@ -39,7 +39,8 @@ static const struct feature_name { * function pointer call in the middle of the loop. */ static int __init always_inline parse_cpuid( - const char *s, void (*callback)(unsigned int feat, bool val)) + const char *s, void (*callback)(const char *name, + unsigned int feat, bool val)) { const char *ss; int val, rc = 0; @@ -81,7 +82,7 @@ static int __init always_inline parse_cpuid( if ( (val = parse_boolean(mid->name, s, ss)) >= 0 ) { - callback(mid->bit, val); + callback(mid->name, mid->bit, val); mid = NULL; } @@ -101,8 +102,12 @@ static int __init always_inline parse_cpuid( return rc; } -static void __init cf_check _parse_xen_cpuid(unsigned int feat, bool val) +static void __init cf_check _parse_xen_cpuid( + const char *name, unsigned int feat, bool val) { + if ( unlikely(feat == ~0u) ) + return; + if ( !val ) setup_clear_cpu_cap(feat); else if ( feat == X86_FEATURE_RDRAND && @@ -120,8 +125,12 @@ static bool __initdata dom0_cpuid_cmdline; static uint32_t __initdata dom0_enable_feat[FSCAPINTS]; static uint32_t __initdata dom0_disable_feat[FSCAPINTS]; -static void __init cf_check _parse_dom0_cpuid(unsigned int feat, bool val) +static void __init cf_check _parse_dom0_cpuid( + const char *name, unsigned int feat, bool val) { + if ( unlikely(feat == ~0u) ) + return; + __set_bit (feat, val ? dom0_enable_feat : dom0_disable_feat); __clear_bit(feat, val ? dom0_disable_feat : dom0_enable_feat ); } diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py index 4f7c8d78cce7..f3045b3bfd36 100755 --- a/xen/tools/gen-cpuid.py +++ b/xen/tools/gen-cpuid.py @@ -297,6 +297,19 @@ def crunch_numbers(state): RTM: [TSXLDTRK], } + # + # Pseudo feature names. These don't map to a feature bit, but are + # inserted into the values dictionary so they can be parsed and handled + # specially + # + pseduo_names = ( + ) + + for n in pseduo_names: + if n in state.values: + raise Fail("Pseduo feature name %s aliases real feature" % (n, )) + state.values[n] = 0xffffffff + deep_features = tuple(sorted(deps.keys())) state.deep_deps = {}
A future change will want a cpuid-like identifier which doesn't have a mapping to a feature bit. * Pass the feature name into the parse callback. * Exclude a feature value of ~0u from falling into the general set/clear bit paths. * In gen-cpuid.py, insert a placeholder to collect all the pseudo feature names. No practical change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> CC: Henry Wang <Henry.Wang@arm.com> CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> CC: Demi Marie Obenour <demi@invisiblethingslab.com> --- xen/arch/x86/cpuid.c | 17 +++++++++++++---- xen/tools/gen-cpuid.py | 13 +++++++++++++ 2 files changed, 26 insertions(+), 4 deletions(-)