[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v12 01/37] x86/cpufeatures: Add the cpu feature bit for WRMSRNS
On Mon, Oct 02, 2023 at 11:24:22PM -0700, Xin Li wrote: > Subject: Re: [PATCH v12 01/37] x86/cpufeatures: Add the cpu feature bit for > WRMSRNS ^^^^ For all your text: s/cpu/CPU/g > WRMSRNS is an instruction that behaves exactly like WRMSR, with > the only difference being that it is not a serializing instruction > by default. Under certain conditions, WRMSRNS may replace WRMSR to > improve performance. > > Add the CPU feature bit for WRMSRNS. > > Tested-by: Shan Kang <shan.kang@xxxxxxxxx> > Signed-off-by: Xin Li <xin3.li@xxxxxxxxx> > --- > arch/x86/include/asm/cpufeatures.h | 1 + > tools/arch/x86/include/asm/cpufeatures.h | 1 + > 2 files changed, 2 insertions(+) It looks to me like you can merge the first three patches into one as all they do is add that insn support. Then, further down in the patchset, it says: + if (cpu_feature_enabled(X86_FEATURE_FRED)) { + /* WRMSRNS is a baseline feature for FRED. */ but WRMSRNS is not mentioned in the FRED spec "Document Number: 346446-005US, Revision: 5.0" which, according to https://www.intel.com/content/www/us/en/content-details/780121/flexible-return-and-event-delivery-fred-specification.html is the latest. Am I looking at the wrong one? > diff --git a/arch/x86/include/asm/cpufeatures.h > b/arch/x86/include/asm/cpufeatures.h > index 58cb9495e40f..330876d34b68 100644 > --- a/arch/x86/include/asm/cpufeatures.h > +++ b/arch/x86/include/asm/cpufeatures.h > @@ -322,6 +322,7 @@ > #define X86_FEATURE_FSRS (12*32+11) /* "" Fast short REP STOSB */ > #define X86_FEATURE_FSRC (12*32+12) /* "" Fast short REP > {CMPSB,SCASB} */ > #define X86_FEATURE_LKGS (12*32+18) /* "" Load "kernel" > (userspace) GS */ > +#define X86_FEATURE_WRMSRNS (12*32+19) /* "" Non-Serializing Write > to Model Specific Register instruction */ /* "" Non-serializing WRMSR */ is more than enough. And now I'm wondering: when you're adding a separate CPUID bit, then the above should be + if (cpu_feature_enabled(X86_FEATURE_WRMSRNS)) { + /* WRMSRNS is a baseline feature for FRED. */ I see that you're adding a dependency: + { X86_FEATURE_FRED, X86_FEATURE_WRMSRNS }, which then means you don't need the X86_FEATURE_WRMSRNS definition at all and can use X86_FEATURE_FRED only. So, what's up? Thx. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |