RE: [PATCH v3 09/22] kvm: x86: Enable dynamic XSAVE features at KVM_SET_CPUID2

[Tian, Kevin]

> From: Sean Christopherson <seanjc@xxxxxxxxxx>
> Sent: Wednesday, December 29, 2021 7:54 AM
> 
> On Wed, Dec 22, 2021, Jing Liu wrote:
> > Statically enable all xfeatures allowed by guest perm in
> 
> Statically isn't the right word.  It's not dymanic with respect to running the
> vCPU, but it's certainly not static.  I think you can just omit "Statically"
> entirely.

make sense.

> 
> > KVM_SET_CPUID2, with fpstate buffer sized accordingly. This avoids
> > run-time expansion in the emulation and restore path of XCR0 and
> > XFD MSR [1].
> >
> > Change kvm_vcpu_after_set_cpuid() to return error given fpstate
> > reallocation may fail.
> >
> > [1] https://lore.kernel.org/all/20211214024948.048572883@xxxxxxxxxxxxx/
> >
> > Suggested-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > Suggested-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > Signed-off-by: Jing Liu <jing2.liu@xxxxxxxxx>
> > ---
> >  arch/x86/kvm/cpuid.c | 24 +++++++++++++++++-------
> >  1 file changed, 17 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index a068373a7fbd..eb5a5070accb 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -204,10 +204,12 @@ void kvm_update_cpuid_runtime(struct
> kvm_vcpu *vcpu)
> >  }
> >  EXPORT_SYMBOL_GPL(kvm_update_cpuid_runtime);
> >
> > -static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> > +static int kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> >  {
> >  	struct kvm_lapic *apic = vcpu->arch.apic;
> >  	struct kvm_cpuid_entry2 *best;
> > +	u64 xfeatures;
> > +	int r;
> >
> >  	best = kvm_find_cpuid_entry(vcpu, 1, 0);
> >  	if (best && apic) {
> > @@ -222,9 +224,17 @@ static void kvm_vcpu_after_set_cpuid(struct
> kvm_vcpu *vcpu)
> >  	best = kvm_find_cpuid_entry(vcpu, 0xD, 0);
> >  	if (!best)
> >  		vcpu->arch.guest_supported_xcr0 = 0;
> > -	else
> > -		vcpu->arch.guest_supported_xcr0 =
> > -			(best->eax | ((u64)best->edx << 32)) &
> supported_xcr0;
> > +	else {
> > +		xfeatures = best->eax | ((u64)best->edx << 32);
> > +
> > +		vcpu->arch.guest_supported_xcr0 = xfeatures &
> supported_xcr0;
> > +
> > +		if (xfeatures != vcpu->arch.guest_fpu.xfeatures) {
> > +			r = fpu_update_guest_perm_features(&vcpu-
> >arch.guest_fpu);
> > +			if (r)
> > +				return r;
> 
> IMO, this should be done and check before "committing" state, otherwise
> KVM will
> set the vCPU's CPUID info and update a variety of state, but then tell
> userspace
> that it failed.  The -EPERM case in particular falls squarely into the "check"
> category.

We did consider your suggestion in the first place, but then adopted
the current way with the impression that all vcpu related changes are 
done in this function. We can surely change it back to the 'check' point.

Thanks
Kevin