Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 8 Feb 2021 09:24:30 -0800
From:   Sean Christopherson <seanjc@google.com>
To:     Dave Hansen <dave.hansen@intel.com>
Cc:     Jing Liu <jing2.liu@linux.intel.com>, pbonzini@redhat.com,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
        kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1] kvm: x86: Revise guest_fpu xcomp_bv field
Message-ID: <YCFzztFESzcnKRqQ@google.com>
References: <20210208161659.63020-1-jing2.liu@linux.intel.com>
 <4e4b37d1-e2f8-6757-003c-d19ae8184088@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4e4b37d1-e2f8-6757-003c-d19ae8184088@intel.com>
Precedence: bulk

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.

> Are 'host_xcr0' and 'xfeatures_mask_all' really interchangeable?  If so,
> shouldn't we just remove 'host_xcr0' everywhere?

I think so?  But use xfeatures_mask_user().

In theory, host_xss can also be replaced with the _supervisor() and _dynamic()
variants.  That code needs a good hard look at the _dynamic() features, which is
currently just architectural LBRs.  E.g. I wouldn't be surprised if KVM currently
fails to save/restore arch LBRs due to the bit not being set in host_xss.