| Message ID | 20211222124052.644626-10-jing2.liu@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | AMX Support in KVM | expand |
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. > 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@linutronix.de/ > > Suggested-by: Thomas Gleixner <tglx@linutronix.de> > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Jing Liu <jing2.liu@intel.com> > --- > 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.
> From: Sean Christopherson <seanjc@google.com> > 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@linutronix.de/ > > > > Suggested-by: Thomas Gleixner <tglx@linutronix.de> > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > > Signed-off-by: Jing Liu <jing2.liu@intel.com> > > --- > > 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
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; + } + } /* * Bits 127:0 of the allowed SECS.ATTRIBUTES (CPUID.0x12.0x1) enumerate @@ -260,6 +270,8 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) * adjustments to the reserved GPA bits. */ kvm_mmu_after_set_cpuid(vcpu); + + return 0; } int cpuid_query_maxphyaddr(struct kvm_vcpu *vcpu) @@ -301,9 +313,7 @@ static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2, kvm_update_kvm_cpuid_base(vcpu); kvm_update_cpuid_runtime(vcpu); - kvm_vcpu_after_set_cpuid(vcpu); - - return 0; + return kvm_vcpu_after_set_cpuid(vcpu); } /* when an old userspace process fills a new kernel module */
Statically enable all xfeatures allowed by guest perm in 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@linutronix.de/ Suggested-by: Thomas Gleixner <tglx@linutronix.de> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Jing Liu <jing2.liu@intel.com> --- arch/x86/kvm/cpuid.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)