Re: [PATCH 4/4] KVM: x86: Simplify logic to handle lack of host NX support

From: Jim Mattson <jmattson@google.com>
To: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	Vitaly Kuznetsov <vkuznets@redhat.com>,
	Wanpeng Li <wanpengli@tencent.com>,
	Joerg Roedel <joro@8bytes.org>, kvm list <kvm@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 4/4] KVM: x86: Simplify logic to handle lack of host NX support
Date: Tue, 15 Jun 2021 15:58:24 -0700	[thread overview]
Message-ID: <CALMp9eRGj_5+dZXQazVEkeKeDnc7GFm1Vnt2RS_V6akAR=rZsA@mail.gmail.com> (raw)
In-Reply-To: <20210615164535.2146172-5-seanjc@google.com>

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>