| Message ID | 20210615164535.2146172-5-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Require EFER.NX support unless EPT is on | expand |
On Tue, Jun 15, 2021 at 9:45 AM Sean Christopherson <seanjc@google.com> wrote: > > Use boot_cpu_has() to check for NX support now that KVM requires > host_efer.NX=1 if NX is supported. Opportunistically avoid the guest > CPUID lookup in cpuid_fix_nx_cap() if NX is supported, which is by far > the common case. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/cpuid.c | 13 +++++-------- > 1 file changed, 5 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index b4da665bb892..786f556302cd 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -208,16 +208,14 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) > kvm_mmu_reset_context(vcpu); > } > > -static int is_efer_nx(void) > -{ > - return host_efer & EFER_NX; > -} > - > static void cpuid_fix_nx_cap(struct kvm_vcpu *vcpu) > { > int i; > struct kvm_cpuid_entry2 *e, *entry; > > + if (boot_cpu_has(X86_FEATURE_NX)) > + return; > + > entry = NULL; > for (i = 0; i < vcpu->arch.cpuid_nent; ++i) { > e = &vcpu->arch.cpuid_entries[i]; > @@ -226,7 +224,7 @@ static void cpuid_fix_nx_cap(struct kvm_vcpu *vcpu) > break; > } > } > - if (entry && cpuid_entry_has(entry, X86_FEATURE_NX) && !is_efer_nx()) { > + if (entry && cpuid_entry_has(entry, X86_FEATURE_NX)) { > cpuid_entry_clear(entry, X86_FEATURE_NX); > printk(KERN_INFO "kvm: guest NX capability removed\n"); > } It would be nice if we chose one consistent approach to dealing with invalid guest CPUID information and stuck with it. Silently modifying the table provided by userspace seems wrong to me. I much prefer the kvm_check_cpuid approach of telling userspace that the guest CPUID information is invalid. (Of course, once we return -EINVAL for more than one field, good luck figuring out which field is invalid!) Reviewed-by: Jim Mattson <jmattson@google.com>
On Tue, Jun 15, 2021, Jim Mattson wrote: > On Tue, Jun 15, 2021 at 9:45 AM Sean Christopherson <seanjc@google.com> wrote: > > @@ -226,7 +224,7 @@ static void cpuid_fix_nx_cap(struct kvm_vcpu *vcpu) > > break; > > } > > } > > - if (entry && cpuid_entry_has(entry, X86_FEATURE_NX) && !is_efer_nx()) { > > + if (entry && cpuid_entry_has(entry, X86_FEATURE_NX)) { > > cpuid_entry_clear(entry, X86_FEATURE_NX); > > printk(KERN_INFO "kvm: guest NX capability removed\n"); > > } > > It would be nice if we chose one consistent approach to dealing with > invalid guest CPUID information and stuck with it. Silently modifying > the table provided by userspace seems wrong to me. I much prefer the > kvm_check_cpuid approach of telling userspace that the guest CPUID > information is invalid. (Of course, once we return -EINVAL for more > than one field, good luck figuring out which field is invalid!) Yeah. I suspect this one can be dropped if EFER.NX is required for everything except EPT, but I didn't fully grok the problem that this was fixing, and it's such an esoteric case that I both don't care and am terrified of breaking some bizarre case.
On 16/06/21 01:33, Sean Christopherson wrote: >> It would be nice if we chose one consistent approach to dealing with >> invalid guest CPUID information and stuck with it. Silently modifying >> the table provided by userspace seems wrong to me. I much prefer the >> kvm_check_cpuid approach of telling userspace that the guest CPUID >> information is invalid. (Of course, once we return -EINVAL for more >> than one field, good luck figuring out which field is invalid!) > Yeah. I suspect this one can be dropped if EFER.NX is required for everything > except EPT, but I didn't fully grok the problem that this was fixing, and it's > such an esoteric case that I both don't care and am terrified of breaking some > bizarre case. > It's dating back to 2007 when EPT didn't even exist, I would just drop it. Paolo
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index b4da665bb892..786f556302cd 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -208,16 +208,14 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) kvm_mmu_reset_context(vcpu); } -static int is_efer_nx(void) -{ - return host_efer & EFER_NX; -} - static void cpuid_fix_nx_cap(struct kvm_vcpu *vcpu) { int i; struct kvm_cpuid_entry2 *e, *entry; + if (boot_cpu_has(X86_FEATURE_NX)) + return; + entry = NULL; for (i = 0; i < vcpu->arch.cpuid_nent; ++i) { e = &vcpu->arch.cpuid_entries[i]; @@ -226,7 +224,7 @@ static void cpuid_fix_nx_cap(struct kvm_vcpu *vcpu) break; } } - if (entry && cpuid_entry_has(entry, X86_FEATURE_NX) && !is_efer_nx()) { + if (entry && cpuid_entry_has(entry, X86_FEATURE_NX)) { cpuid_entry_clear(entry, X86_FEATURE_NX); printk(KERN_INFO "kvm: guest NX capability removed\n"); } @@ -401,7 +399,6 @@ static __always_inline void kvm_cpu_cap_mask(enum cpuid_leafs leaf, u32 mask) void kvm_set_cpu_caps(void) { - unsigned int f_nx = is_efer_nx() ? F(NX) : 0; #ifdef CONFIG_X86_64 unsigned int f_gbpages = F(GBPAGES); unsigned int f_lm = F(LM); @@ -515,7 +512,7 @@ void kvm_set_cpu_caps(void) F(CX8) | F(APIC) | 0 /* Reserved */ | F(SYSCALL) | F(MTRR) | F(PGE) | F(MCA) | F(CMOV) | F(PAT) | F(PSE36) | 0 /* Reserved */ | - f_nx | 0 /* Reserved */ | F(MMXEXT) | F(MMX) | + F(NX) | 0 /* Reserved */ | F(MMXEXT) | F(MMX) | F(FXSR) | F(FXSR_OPT) | f_gbpages | F(RDTSCP) | 0 /* Reserved */ | f_lm | F(3DNOWEXT) | F(3DNOW) );
Use boot_cpu_has() to check for NX support now that KVM requires host_efer.NX=1 if NX is supported. Opportunistically avoid the guest CPUID lookup in cpuid_fix_nx_cap() if NX is supported, which is by far the common case. Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/cpuid.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)