Message ID | 20210615164535.2146172-4-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: > > 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>
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
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 >
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
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
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
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);
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(+)