On 11/19/2022 12:47 AM, Dave Hansen wrote: > On 11/18/22 06:15, Jiaxi Chen wrote: >> CMPccXADD is a new set of instructions in the latest Intel platform >> Sierra Forest. This new instruction set includes a semaphore operation >> that can compare and add the operands if condition is met, which can >> improve database performance. >> >> The bit definition: >> CPUID.(EAX=7,ECX=1):EAX[bit 7] >> >> This CPUID is exposed to userspace. Besides, there is no other VMX >> control for this instruction. > > The last time you posted these, I asked: > >> Intel folks, when you add these bits, can you please include information >> about the "vetting" that you performed? > > I think you're alluding to that in your comment about VMX contols. > Could you be more explicit here and include *all* of your logic about > why this feature is OK to pass through to guests? > Yes, that's very rigorous. Will check and add these for all features in this patch series. > Also, do we *want* this showing up in /proc/cpuinfo? > > There are also two distinct kinds of features that you're adding here. > These: > >> +#define X86_FEATURE_CMPCCXADD (12*32+ 7) /* CMPccXADD instructions */ > > and these: > > +#define X86_FEATURE_PREFETCHITI KVM_X86_FEATURE(CPUID_7_1_EDX, 14) > > Could you also please include a sentence or two about why the feature > was treated on way versus another? That's frankly a lot more important > than telling us which random Intel codename this shows up on first, or > wasting space on telling us what the CPUID bit definition is. We can > kinda get that from the patch. Yes. A few words of description is necessary here. Features which has been enabled in kernel usually should be added to /proc/cpuinfo. The first way is often used for bit whose leaf has many other bits in use. It's very simple to do, just adding one line for each feature based on existing words in can get the effect. For those bits whose leaf has just a few bits in use, they should be defined in a 'scattered' way. However, this kind of features in this patch series have no other kernel usage and they just need to be advertised to kvm userspace. Therefore, define them in a kvm-only way is more explicit. > > Another nit on these: > >> This CPUID is exposed to userspace. Besides, there is no other VMX >> control for this instruction. > > Please remember to use imperative voice when describing what the patch > in question does. Using passive voice like that makes it seem like > you're describing the state of the art rather than the patch. > > For example, that should probably be: > > Expose CMPCCXADD to KVM userspace. This is safe because there > are no new VMX controls or host enabling required for guests to > use this feature. > > See how that first sentence is giving orders? It's *telling* you what > to do. That's imperative voice and that's what you use to describe the > actions of *this* patch. Appreciate your very detailed suggestions. Thanks very much! -- Regards, Jiaxi
