On 29.04.2024 17:16, Andrew Cooper wrote: > --- a/xen/include/public/arch-x86/cpufeatureset.h > +++ b/xen/include/public/arch-x86/cpufeatureset.h > @@ -358,6 +358,20 @@ XEN_CPUFEATURE(RFDS_CLEAR, 16*32+28) /*!A > Register File(s) cleared by VE > /* Intel-defined CPU features, MSR_ARCH_CAPS 0x10a.edx, word 17 */ > > /* AMD-defined CPU features, CPUID level 0x8000000a.edx, word 18 */ > +XEN_CPUFEATURE(NPT, 18*32+ 0) /*h Nested PageTables */ > +XEN_CPUFEATURE(V_LBR, 18*32+ 1) /*h Virtualised LBR */ > +XEN_CPUFEATURE(SVM_LOCK, 18*32+ 2) /* SVM locking MSR */ > +XEN_CPUFEATURE(NRIPS, 18*32+ 3) /*h Next-RIP saved on VMExit */ > +XEN_CPUFEATURE(V_TSC_RATE, 18*32+ 4) /* Virtualised TSC Ratio */ > +XEN_CPUFEATURE(VMCB_CLEANBITS, 18*32+ 5) /*! VMCB Clean-bits */
Wouldn't this better be marked !h nevertheless? As to the name - does it need to be this long? VMCB_CLEAN would be in line with the PM. But yeah, while CLEANBITS would be clear in the context of this leaf, there might be whatever other "cleanbits" elsewhere, so some qualifier is wanted, I guess. As to the V_ prefixes you use for several of the features: Isn't there a risk of this being ambiguous towards VT-x? Maybe they should all be SVM_*, even if ... > +XEN_CPUFEATURE(FLUSH_BY_ASID, 18*32+ 6) /* TLB Flush by ASID */ > +XEN_CPUFEATURE(DECODE_ASSIST, 18*32+ 7) /*h Decode assists */ > +XEN_CPUFEATURE(PAUSE_FILTER, 18*32+10) /*h Pause filter */ > +XEN_CPUFEATURE(PAUSE_THRESH, 18*32+12) /* Pause filter threshold */ > +XEN_CPUFEATURE(V_LOADSAVE, 18*32+15) /* Virtualised VMLOAD/SAVE */ > +XEN_CPUFEATURE(V_GIF, 18*32+16) /* Virtualised GIF */ ... these two at least are clearly SVM terminology and hence already unambiguous. > +XEN_CPUFEATURE(NPT_SSS, 18*32+19) /* NPT Supervisor Shadow > Stacks */ > +XEN_CPUFEATURE(V_SPEC_CTRL, 18*32+20) /* Virtualised MSR_SPEC_CTRL > */ Whereas this, when used somewhere in isolation, would not make clear whether AMD's or Intel's is meant. > --- a/xen/tools/gen-cpuid.py > +++ b/xen/tools/gen-cpuid.py > @@ -280,6 +280,9 @@ def crunch_numbers(state): > # standard 3DNow in the earlier K6 processors. > _3DNOW: [_3DNOWEXT], > > + # The SVM bit enumerates the whole SVM leave. > + SVM: list(range(NPT, NPT + 32)), Seeing this and taking it together with the somewhat confusing (to me) part of the description of patch 1: What is it then that you try to avoid there, when adding the dependencies here is okay, while doing so there would be entirely impossible (short of there being identifiers)? Jan
