From: Sean Christopherson <seanjc@google.com>
Sent: Wednesday, December 29, 2021 7:54 AM

On Wed, Dec 22, 2021, Jing Liu wrote:

quoted

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.

quoted

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/ (local)

Suggested-by: Thomas Gleixner <redacted>
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Jing Liu <redacted>
---
 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)

quoted

 }
 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)

quoted

 	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;

quoted

+	else {
+		xfeatures = best->eax | ((u64)best->edx << 32);
+
+		vcpu->arch.guest_supported_xcr0 = xfeatures &

supported_xcr0;

quoted

+
+		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