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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 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




[Index of Archives]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux