Re: [PATCH RFC 5/7] kvm: x86: Revise CPUID.D.1.EBX for alignment rule

[Liu, Jing2]

On 5/25/2021 5:28 AM, Sean Christopherson wrote:
> On Sun, Feb 07, 2021, Jing Liu wrote:
> > CPUID.0xD.1.EBX[1] is set if, when the compacted format of an XSAVE
> > area is used, this extended state component located on the next
> > 64-byte boundary following the preceding state component (otherwise,
> > it is located immediately following the preceding state component).
> >
> > AMX tileconfig and tiledata are the first to use 64B alignment.
> > Revise the runtime cpuid modification for this rule.
> >
> > Signed-off-by: Jing Liu<jing2.liu@xxxxxxxxxxxxxxx>
> > ---
> >   arch/x86/kvm/cpuid.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> >
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index 04a73c395c71..ee1fac0a865e 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -35,12 +35,17 @@ static u32 xstate_required_size(u64 xstate_bv, bool compacted)
> >   {
> >   	int feature_bit = 0;
> >   	u32 ret = XSAVE_HDR_SIZE + XSAVE_HDR_OFFSET;
> > +	bool is_aligned = false;
> >   
> >   	xstate_bv &= XFEATURE_MASK_EXTEND;
> >   	while (xstate_bv) {
> >   		if (xstate_bv & 0x1) {
> >   		        u32 eax, ebx, ecx, edx, offset;
> >   		        cpuid_count(0xD, feature_bit, &eax, &ebx, &ecx, &edx);
> > +			/* ECX[2]: 64B alignment in compacted form */
> > +			is_aligned = !!(ecx & 2);
> > +			if (is_aligned && compacted)
> I'd forego the local is_aligned, and also check "compacted" first so that the
> uncompacted variant isn't required to evaluated ecx.
Non-compacted only works for XCR0 (user states), do we need add a check 
or simply do
'state_bv &= XFEATURE_MASK_USER_ENABLED'?

        xstate_bv &= XFEATURE_MASK_EXTEND;
+       /* Only user states use non-compacted format. */
+       if (!compacted)
+               xstate_bv &= XFEATURE_MASK_USER_SUPPORTED;
+
        while (xstate_bv) {
                if (xstate_bv & 0x1) {
                        u32 eax, ebx, ecx, edx, offset;
                        cpuid_count(0xD, feature_bit, &eax, &ebx, &ecx, 
&edx);
-                       offset = compacted ? ret : ebx;
+                       offset = compacted ? ((ecx & 0x2) ? ALIGN(ret, 
64) : ret) : ebx;
                        ret = max(ret, offset + eax);
                }

> And the real reason I am responding... can you post this as a standalone patch?
Sure. Let me separate it.
> I stumbled across the "aligned" flag when reading through the SDM for a completely
> unrelated reason, and also discovered that the flag has been documented since
> 2016.  While AMX may be the first to "officially" utilize the alignment flag,
> the flag itself is architectural and not strictly limited to AMX.
Yes, this is not a new feature, but seems no one use it before.

Thanks,
Jing
> > +				ret = ALIGN(ret, 64);
> >   			offset = compacted ? ret : ebx;
> >   			ret = max(ret, offset + eax);
> >   		}
> > --
> > 2.18.4