Re: [PATCH v4 1/6] x86: KVM: Advertise CMPccXADD CPUID to user space

[Dave Hansen]

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?

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.

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.