[PATCH 0/4] KVM: x86: Require EFER.NX support unless EPT is on

[PATCH 0/4] KVM: x86: Require EFER.NX support unless EPT is on

[Sean Christopherson]

KVM has silently required EFER.NX support for shadow paging for well over
a year, and for NPT for roughly the same amount of time.  Attempting to
run any VM with shadow paging on a system without NX support will fail due
to invalid state, while enabling nx_huge_pages with NPT and no NX will
explode due to setting a reserved bit in the page tables.

I really, really wanted to require NX across the board, because the lack
of bug reports for the shadow paging change strongly suggests no one is
running KVM on a CPU that truly doesn't have NX.  But, Intel CPUs let
firmware disable NX via MISC_ENABLES, so it's plausible that there are
users running KVM with EPT and no NX.

Sean Christopherson (4):
  KVM: VMX: Refuse to load kvm_intel if EPT and NX are disabled
  KVM: SVM: Refuse to load kvm_amd if NX support is not available
  KVM: x86: WARN and reject loading KVM if NX is supported but not
    enabled
  KVM: x86: Simplify logic to handle lack of host NX support

 arch/x86/kvm/cpuid.c   | 13 +++++--------
 arch/x86/kvm/svm/svm.c | 13 ++++++++++---
 arch/x86/kvm/vmx/vmx.c |  6 ++++++
 arch/x86/kvm/x86.c     |  3 +++
 4 files changed, 24 insertions(+), 11 deletions(-)

-- 
2.32.0.272.g935e593368-goog

[PATCH 1/4] KVM: VMX: Refuse to load kvm_intel if EPT and NX are disabled

[Sean Christopherson]

Refuse to load KVM if NX support is not available and EPT is not enabled.
Shadow paging has assumed NX support since commit 9167ab799362 ("KVM:
vmx, svm: always run with EFER.NXE=1 when shadow paging is active"), so
for all intents and purposes this has been a de facto requirement for
over a year.

Do not require NX support if EPT is enabled purely because Intel CPUs let
firmware disable NX support via MSR_IA32_MISC_ENABLES.  If not for that,
VMX (and KVM as a whole) could require NX support with minimal risk to
breaking userspace.

Fixes: 9167ab799362 ("KVM: vmx, svm: always run with EFER.NXE=1 when shadow paging is active")
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/vmx.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 68a72c80bd3f..889e83f71235 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7723,6 +7723,12 @@ static __init int hardware_setup(void)
 	    !cpu_has_vmx_invept_global())
 		enable_ept = 0;
 
+	/* NX support is required for shadow paging. */
+	if (!enable_ept && !boot_cpu_has(X86_FEATURE_NX)) {
+		pr_err_ratelimited("kvm: NX (Execute Disable) not supported\n");
+		return -EOPNOTSUPP;
+	}
+
 	if (!cpu_has_vmx_ept_ad_bits() || !enable_ept)
 		enable_ept_ad_bits = 0;
 
-- 
2.32.0.272.g935e593368-goog

Re: [PATCH 1/4] KVM: VMX: Refuse to load kvm_intel if EPT and NX are disabled

[Jim Mattson]

On Tue, Jun 15, 2021 at 9:45 AM Sean Christopherson <seanjc@google.com> wrote:
>
> Refuse to load KVM if NX support is not available and EPT is not enabled.
> Shadow paging has assumed NX support since commit 9167ab799362 ("KVM:
> vmx, svm: always run with EFER.NXE=1 when shadow paging is active"), so
> for all intents and purposes this has been a de facto requirement for
> over a year.
>
> Do not require NX support if EPT is enabled purely because Intel CPUs let
> firmware disable NX support via MSR_IA32_MISC_ENABLES.  If not for that,
> VMX (and KVM as a whole) could require NX support with minimal risk to
> breaking userspace.
>
> Fixes: 9167ab799362 ("KVM: vmx, svm: always run with EFER.NXE=1 when shadow paging is active")
> Signed-off-by: Sean Christopherson <seanjc@google.com>
Reviewed-by: Jim Mattson <jmattson@google.com>

[PATCH 2/4] KVM: SVM: Refuse to load kvm_amd if NX support is not available

[Sean Christopherson]

Refuse to load KVM if NX support is not available.  Shadow paging has
assumed NX support since commit 9167ab799362 ("KVM: vmx, svm: always run
with EFER.NXE=1 when shadow paging is active"), and NPT has assumed NX
support since commit b8e8c8303ff2 ("kvm: mmu: ITLB_MULTIHIT mitigation").
While the NX huge pages mitigation should not be enabled by default for
AMD CPUs, it can be turned on by userspace at will.

Unlike Intel CPUs, AMD does not provide a way for firmware to disable NX
support, and Linux always sets EFER.NX=1 if it is supported.  Given that
it's extremely unlikely that a CPU supports NPT but not NX, making NX a
formal requirement is far simpler than adding requirements to the
mitigation flow.

Fixes: 9167ab799362 ("KVM: vmx, svm: always run with EFER.NXE=1 when shadow paging is active")
Fixes: b8e8c8303ff2 ("kvm: mmu: ITLB_MULTIHIT mitigation")
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/svm.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index b6afa6b63c8f..12c06ea28f5c 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -952,6 +952,16 @@ static __init int svm_hardware_setup(void)
 	int r;
 	unsigned int order = get_order(IOPM_SIZE);
 
+	/*
+	 * NX is required for shadow paging and for NPT if the NX huge pages
+	 * mitigation is enabled.
+	 */
+	if (!boot_cpu_has(X86_FEATURE_NX)) {
+		pr_err_ratelimited("NX (Execute Disable) not supported\n");
+		return -EOPNOTSUPP;
+	}
+	kvm_enable_efer_bits(EFER_NX);
+
 	iopm_pages = alloc_pages(GFP_KERNEL, order);
 
 	if (!iopm_pages)
@@ -965,9 +975,6 @@ static __init int svm_hardware_setup(void)
 
 	supported_xcr0 &= ~(XFEATURE_MASK_BNDREGS | XFEATURE_MASK_BNDCSR);
 
-	if (boot_cpu_has(X86_FEATURE_NX))
-		kvm_enable_efer_bits(EFER_NX);
-
 	if (boot_cpu_has(X86_FEATURE_FXSR_OPT))
 		kvm_enable_efer_bits(EFER_FFXSR);
 
-- 
2.32.0.272.g935e593368-goog

Re: [PATCH 2/4] KVM: SVM: Refuse to load kvm_amd if NX support is not available

[Jim Mattson]

On Tue, Jun 15, 2021 at 9:45 AM Sean Christopherson <seanjc@google.com> wrote:
>
> Refuse to load KVM if NX support is not available.  Shadow paging has
> assumed NX support since commit 9167ab799362 ("KVM: vmx, svm: always run
> with EFER.NXE=1 when shadow paging is active"), and NPT has assumed NX
> support since commit b8e8c8303ff2 ("kvm: mmu: ITLB_MULTIHIT mitigation").
> While the NX huge pages mitigation should not be enabled by default for
> AMD CPUs, it can be turned on by userspace at will.
>
> Unlike Intel CPUs, AMD does not provide a way for firmware to disable NX
> support, and Linux always sets EFER.NX=1 if it is supported.  Given that
> it's extremely unlikely that a CPU supports NPT but not NX, making NX a
> formal requirement is far simpler than adding requirements to the
> mitigation flow.
>
> Fixes: 9167ab799362 ("KVM: vmx, svm: always run with EFER.NXE=1 when shadow paging is active")
> Fixes: b8e8c8303ff2 ("kvm: mmu: ITLB_MULTIHIT mitigation")
> Signed-off-by: Sean Christopherson <seanjc@google.com>
Reviewed-by: Jim Mattson <jmattson@google.com>

[PATCH 3/4] KVM: x86: WARN and reject loading KVM if NX is supported but not enabled

[Sean Christopherson]

WARN if NX is reported as supported but not enabled in EFER.  All flavors
of the kernel, including non-PAE 32-bit kernels, set EFER.NX=1 if NX is
supported, even if NX usage is disable via kernel command line.  KVM relies
on NX being enabled if it's supported, e.g. KVM will generate illegal NPT
entries if nx_huge_pages is enabled and NX is supported but not enabled.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index acc28473dec7..1f6595df45de 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10981,6 +10981,9 @@ int kvm_arch_hardware_setup(void *opaque)
 	int r;
 
 	rdmsrl_safe(MSR_EFER, &host_efer);
+	if (WARN_ON_ONCE(boot_cpu_has(X86_FEATURE_NX) &&
+			 !(host_efer & EFER_NX)))
+		return -EIO;
 
 	if (boot_cpu_has(X86_FEATURE_XSAVES))
 		rdmsrl(MSR_IA32_XSS, host_xss);
-- 
2.32.0.272.g935e593368-goog

Re: [PATCH 3/4] KVM: x86: WARN and reject loading KVM if NX is supported but not enabled

[Jim Mattson]

On Tue, Jun 15, 2021 at 9:45 AM Sean Christopherson <seanjc@google.com> wrote:
>
> WARN if NX is reported as supported but not enabled in EFER.  All flavors
> of the kernel, including non-PAE 32-bit kernels, set EFER.NX=1 if NX is
> supported, even if NX usage is disable via kernel command line.  KVM relies
> on NX being enabled if it's supported, e.g. KVM will generate illegal NPT
> entries if nx_huge_pages is enabled and NX is supported but not enabled.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/x86.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index acc28473dec7..1f6595df45de 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -10981,6 +10981,9 @@ int kvm_arch_hardware_setup(void *opaque)
>         int r;
>
>         rdmsrl_safe(MSR_EFER, &host_efer);
> +       if (WARN_ON_ONCE(boot_cpu_has(X86_FEATURE_NX) &&
> +                        !(host_efer & EFER_NX)))
> +               return -EIO;

Input/output error? Is that really the most appropriate error here?
Why not, say, -ENOTSUP?

I'm sure there's some arcane convention here that I'm not privy to. :-)

Reviewed-by: Jim Mattson <jmattson@google.com>

Re: [PATCH 3/4] KVM: x86: WARN and reject loading KVM if NX is supported but not enabled

[Paolo Bonzini]

On 16/06/21 00:39, Jim Mattson wrote:
>>
>>          rdmsrl_safe(MSR_EFER, &host_efer);
>> +       if (WARN_ON_ONCE(boot_cpu_has(X86_FEATURE_NX) &&
>> +                        !(host_efer & EFER_NX)))
>> +               return -EIO;
> Input/output error? Is that really the most appropriate error here?
> Why not, say, -ENOTSUP?
> 
> I'm sure there's some arcane convention here that I'm not privy to.:-)
> 
> Reviewed-by: Jim Mattson<jmattson@google.com>
> 

EIO often means "how the heck did we get here?" or "look in dmesg to get 
more info", both of which I think are appropriate after a WARN.

Paolo

Re: [PATCH 3/4] KVM: x86: WARN and reject loading KVM if NX is supported but not enabled

[Sean Christopherson]

On Tue, Jun 15, 2021, Sean Christopherson wrote:
> WARN if NX is reported as supported but not enabled in EFER.  All flavors
> of the kernel, including non-PAE 32-bit kernels, set EFER.NX=1 if NX is
> supported, even if NX usage is disable via kernel command line.

Ugh, I misread .Ldefault_entry in head_32.S, it skips over the entire EFER code
if PAE=0.  Apparently I didn't test this with non-PAE paging and EPT?

Paolo, I'll send a revert since it's in kvm/next, but even better would be if
you can drop the patch :-)  Lucky for me you didn't pick up patch 4/4 that
depends on this...

I'll revisit this mess in a few weeks.

> on NX being enabled if it's supported, e.g. KVM will generate illegal NPT
> entries if nx_huge_pages is enabled and NX is supported but not enabled.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/x86.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index acc28473dec7..1f6595df45de 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -10981,6 +10981,9 @@ int kvm_arch_hardware_setup(void *opaque)
>  	int r;
>  
>  	rdmsrl_safe(MSR_EFER, &host_efer);
> +	if (WARN_ON_ONCE(boot_cpu_has(X86_FEATURE_NX) &&
> +			 !(host_efer & EFER_NX)))
> +		return -EIO;
>  
>  	if (boot_cpu_has(X86_FEATURE_XSAVES))
>  		rdmsrl(MSR_IA32_XSS, host_xss);
> -- 
> 2.32.0.272.g935e593368-goog
> 

Re: [PATCH 3/4] KVM: x86: WARN and reject loading KVM if NX is supported but not enabled

[Paolo Bonzini]

On 25/06/21 00:33, Sean Christopherson wrote:
> On Tue, Jun 15, 2021, Sean Christopherson wrote:
>> WARN if NX is reported as supported but not enabled in EFER.  All flavors
>> of the kernel, including non-PAE 32-bit kernels, set EFER.NX=1 if NX is
>> supported, even if NX usage is disable via kernel command line.
> 
> Ugh, I misread .Ldefault_entry in head_32.S, it skips over the entire EFER code
> if PAE=0.  Apparently I didn't test this with non-PAE paging and EPT?
> 
> Paolo, I'll send a revert since it's in kvm/next, but even better would be if
> you can drop the patch :-)  Lucky for me you didn't pick up patch 4/4 that
> depends on this...
> 
> I'll revisit this mess in a few weeks.

Rather, let's keep this, see if anyone complains and possibly add a 
"depends on X86_PAE || X86_64" to KVM.

Paolo

Re: [PATCH 3/4] KVM: x86: WARN and reject loading KVM if NX is supported but not enabled

[Naresh Kamboju]

On Fri, 25 Jun 2021 at 14:35, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 25/06/21 00:33, Sean Christopherson wrote:
> > On Tue, Jun 15, 2021, Sean Christopherson wrote:
> >> WARN if NX is reported as supported but not enabled in EFER.  All flavors
> >> of the kernel, including non-PAE 32-bit kernels, set EFER.NX=1 if NX is
> >> supported, even if NX usage is disable via kernel command line.
> >
> > Ugh, I misread .Ldefault_entry in head_32.S, it skips over the entire EFER code
> > if PAE=0.  Apparently I didn't test this with non-PAE paging and EPT?
> >
> > Paolo, I'll send a revert since it's in kvm/next, but even better would be if
> > you can drop the patch :-)  Lucky for me you didn't pick up patch 4/4 that
> > depends on this...
> >
> > I'll revisit this mess in a few weeks.
>
> Rather, let's keep this, see if anyone complains and possibly add a
> "depends on X86_PAE || X86_64" to KVM.

[ please ignore if this is already reported ]

The following kernel warning noticed while booting linus master branch and
Linux next 20210707 tag on i386 kernel booting on x86_64 machine.

This is easily reproducible on qemu boot also.

Crash log:
[    3.269888] ------------[ cut here ]------------
[    3.274522] WARNING: CPU: 1 PID: 1 at
/usr/src/kernel/arch/x86/kvm/x86.c:10985
kvm_arch_hardware_setup+0x70/0x1660
[    3.284876] Modules linked in:
[    3.287942] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.13.0 #1
[    3.293869] Hardware name: Supermicro SYS-5019S-ML/X11SSH-F, BIOS
2.2 05/23/2018
[    3.301262] EIP: kvm_arch_hardware_setup+0x70/0x1660
[    3.306236] Code: d2 a8 08 0f 85 61 14 00 00 8b 43 0c e8 19 0c f5
00 85 c0 89 45 c4 74 2a 8b 45 c4 8d 65 f4 5b 5e 5f 5d c3 8d b4 26 00
00 00 00 <0f> 0b c7 45 c4 fb ff ff ff 8b 45 c4 8d 65 f4 5b 5e 5f 5d c3
8d 74
[    3.324983] EAX: 2c100000 EBX: d2270360 ECX: 00000000 EDX: 00000000
[    3.331255] ESI: 00000000 EDI: 00000000 EBP: c118df00 ESP: c118de98
[    3.337522] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068 EFLAGS: 00010246
[    3.344310] CR0: 80050033 CR2: 00000000 CR3: 12322000 CR4: 003506d0
[    3.350582] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
[    3.356849] DR6: fffe0ff0 DR7: 00000400
[    3.360687] Call Trace:
[    3.363142]  ? __mutex_unlock_slowpath+0x25/0x280
[    3.367868]  ? mutex_unlock+0x10/0x20
[    3.371549]  ? alloc_workqueue+0x28d/0x3c0
[    3.375668]  ? kvm_irqfd_init+0x16/0x30
[    3.379522]  kvm_init+0x51/0x2a0
[    3.382773]  ? vmx_check_processor_compat+0x8f/0x8f
[    3.387666]  vmx_init+0x21/0x96
[    3.390813]  ? vmx_check_processor_compat+0x8f/0x8f
[    3.395702]  do_one_initcall+0x47/0x290
[    3.399547]  ? parse_args+0x70/0x3a0
[    3.403130]  ? rcu_read_lock_sched_held+0x2f/0x50
[    3.407850]  ? trace_initcall_level+0x6d/0x95
[    3.412220]  ? kernel_init_freeable+0x1f8/0x25a
[    3.416769]  kernel_init_freeable+0x217/0x25a
[    3.421138]  ? rest_init+0x240/0x240
[    3.424724]  kernel_init+0x17/0x100
[    3.428217]  ret_from_fork+0x1c/0x28
[    3.431813] irq event stamp: 1214031
[    3.435402] hardirqs last  enabled at (1214041): [<d0b86436>]
console_unlock+0x306/0x500
[    3.443497] hardirqs last disabled at (1214050): [<d0b864a5>]
console_unlock+0x375/0x500
[    3.451591] softirqs last  enabled at (1213878): [<d199ac28>]
__do_softirq+0x2b8/0x3c9
[    3.459510] softirqs last disabled at (1213873): [<d0ab6e55>]
call_on_stack+0x45/0x50
[    3.467348] ---[ end trace f4fc6886f36388fb ]---
[    3.472180] has_svm: not amd or hygon
[    3.475865] kvm: no hardware support

Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>

Full test log on next:
https://qa-reports.linaro.org/lkft/linux-next-master/build/next-20210707/testrun/5065136/suite/linux-log-parser/test/check-kernel-exception-3009544/log

Full test log on mainline:
https://qa-reports.linaro.org/lkft/linux-mainline-master/build/v5.13-11855-g77d34a4683b0/testrun/5063622/suite/linux-log-parser/test/check-kernel-warning-3008934/log

metadata:
  git branch: master
  git repo: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
  git commit: 122fa8c588316aacafe7e5a393bb3e875eaf5b25
  kernel-config:
http://snapshots.linaro.org/openembedded/lkft/lkft/sumo/intel-core2-32/lkft/linux-mainline/3687/config

steps to build kernel:
https://builds.tuxbuild.com/1uyNgJdyxxpRQLDF0kxuvlK0tLD/tuxmake_reproducer.sh

Steps to reproduce:
# qemu-system-i386 -cpu host -enable-kvm -nographic -net
nic,model=virtio,macaddr=DE:AD:BE:EF:66:10 -net tap -m 4096 -monitor
none -kernel bzImage --append "root=/dev/sda  rootwait
console=ttyS0,115200" -hda
rpb-console-image-lkft-intel-core2-32-20210525221022.rootfs.ext4


--
Linaro LKFT
https://lkft.linaro.org

Re: [PATCH 3/4] KVM: x86: WARN and reject loading KVM if NX is supported but not enabled

[Paolo Bonzini]

On 07/07/21 14:09, Naresh Kamboju wrote:
> On Fri, 25 Jun 2021 at 14:35, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> On 25/06/21 00:33, Sean Christopherson wrote:
>>> On Tue, Jun 15, 2021, Sean Christopherson wrote:
>>>> WARN if NX is reported as supported but not enabled in EFER.  All flavors
>>>> of the kernel, including non-PAE 32-bit kernels, set EFER.NX=1 if NX is
>>>> supported, even if NX usage is disable via kernel command line.
>>>
>>> Ugh, I misread .Ldefault_entry in head_32.S, it skips over the entire EFER code
>>> if PAE=0.  Apparently I didn't test this with non-PAE paging and EPT?
>>>
>>> Paolo, I'll send a revert since it's in kvm/next, but even better would be if
>>> you can drop the patch :-)  Lucky for me you didn't pick up patch 4/4 that
>>> depends on this...
>>>
>>> I'll revisit this mess in a few weeks.
>>
>> Rather, let's keep this, see if anyone complains and possibly add a
>> "depends on X86_PAE || X86_64" to KVM.
> 
> [ please ignore if this is already reported ]
> 
> The following kernel warning noticed while booting linus master branch and
> Linux next 20210707 tag on i386 kernel booting on x86_64 machine.

Ok, so the "depends on" is needed.  Let's add it, I'm getting back to 
KVM work and will send the patch today or tomorrow.

Paolo

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

[Sean Christopherson]

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");
 	}
@@ -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)
 	);
-- 
2.32.0.272.g935e593368-goog

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

[Jim Mattson]

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>

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

[Sean Christopherson]

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.

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

[Paolo Bonzini]

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

Re: [PATCH 0/4] KVM: x86: Require EFER.NX support unless EPT is on

[Paolo Bonzini]

On 15/06/21 18:45, Sean Christopherson wrote:
> KVM has silently required EFER.NX support for shadow paging for well over
> a year, and for NPT for roughly the same amount of time.  Attempting to
> run any VM with shadow paging on a system without NX support will fail due
> to invalid state, while enabling nx_huge_pages with NPT and no NX will
> explode due to setting a reserved bit in the page tables.
> 
> I really, really wanted to require NX across the board, because the lack
> of bug reports for the shadow paging change strongly suggests no one is
> running KVM on a CPU that truly doesn't have NX.  But, Intel CPUs let
> firmware disable NX via MISC_ENABLES, so it's plausible that there are
> users running KVM with EPT and no NX.
> 
> Sean Christopherson (4):
>    KVM: VMX: Refuse to load kvm_intel if EPT and NX are disabled
>    KVM: SVM: Refuse to load kvm_amd if NX support is not available
>    KVM: x86: WARN and reject loading KVM if NX is supported but not
>      enabled
>    KVM: x86: Simplify logic to handle lack of host NX support
> 
>   arch/x86/kvm/cpuid.c   | 13 +++++--------
>   arch/x86/kvm/svm/svm.c | 13 ++++++++++---
>   arch/x86/kvm/vmx/vmx.c |  6 ++++++
>   arch/x86/kvm/x86.c     |  3 +++
>   4 files changed, 24 insertions(+), 11 deletions(-)
> 

Queued 1-3, thanks.

Paolo