Re: [PATCH v1] kvm: x86: Revise guest_fpu xcomp_bv field

[Sean Christopherson]

On Mon, Feb 22, 2021, Liu, Jing2 wrote:
> 
> On 2/9/2021 1:24 AM, Sean Christopherson wrote:
> > On Mon, Feb 08, 2021, Dave Hansen wrote:
> > > On 2/8/21 8:16 AM, Jing Liu wrote:
> > > > -#define XSTATE_COMPACTION_ENABLED (1ULL << 63)
> > > > -
> > > >   static void fill_xsave(u8 *dest, struct kvm_vcpu *vcpu)
> > > >   {
> > > >   	struct xregs_state *xsave = &vcpu->arch.guest_fpu->state.xsave;
> > > > @@ -4494,7 +4492,8 @@ static void load_xsave(struct kvm_vcpu *vcpu, u8 *src)
> > > >   	/* Set XSTATE_BV and possibly XCOMP_BV.  */
> > > >   	xsave->header.xfeatures = xstate_bv;
> > > >   	if (boot_cpu_has(X86_FEATURE_XSAVES))
> > > > -		xsave->header.xcomp_bv = host_xcr0 | XSTATE_COMPACTION_ENABLED;
> > > > +		xsave->header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT |
> > > > +					 xfeatures_mask_all;
> > This is wrong, xfeatures_mask_all also tracks supervisor states.
> When looking at SDM Vol2 XSAVES instruction Operation part, it says as
> follows,
> 
> RFBM ← (XCR0 OR IA32_XSS) AND EDX:EAX;
> COMPMASK ← RFBM OR 80000000_00000000H;
> ...
> 
> XCOMP_BV field in XSAVE header ← COMPMASK;
> 
> 
> So it seems xcomp_bv also tracks supervisor states?

Yes, sorry, I got distracted by Dave's question and didn't read the changelog
closely.

Now that I have, I find "Since fpstate_init() has initialized xcomp_bv, let's
just use that." confusing.  I think what you intend to say is that we can use
the same _logic_ as fpstate_init_xstate() for calculating xcomp_bv.

That said, it would be helpful for the changelog to explain why it's correct to
use xfeatures_mask_all, e.g. just a short comment stating that the variable holds
all XCR0 and XSS bits enabled by the host kernel.  Justifying a change with
"because other code does it" is sketchy, becuse there's no guarantee that what
something else does is also correct for KVM, or that the existing code itself is
even correct.