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