[PATCH] Revert "KVM: x86: WARN and reject loading KVM if NX is supported but not enabled"

[PATCH] Revert "KVM: x86: WARN and reject loading KVM if NX is supported but not enabled"

[Sean Christopherson]

Let KVM load if EFER.NX=0 even if NX is supported, the analysis and
testing (or lack thereof) for the non-PAE host case was garbage.

If the kernel won't be using PAE paging, .Ldefault_entry in head_32.S
skips over the entire EFER sequence.  Hopefully that can be changed in
the future to allow KVM to require EFER.NX, but the motivation behind
KVM's requirement isn't yet merged.  Reverting and revisiting the mess
at a later date is by far the safest approach.

This reverts commit 8bbed95d2cb6e5de8a342d761a89b0a04faed7be.

Fixes: 8bbed95d2cb6 ("KVM: x86: WARN and reject loading KVM if NX is supported but not enabled")
Signed-off-by: Sean Christopherson <seanjc@google.com>
---

Hopefully it's not too late to just drop the original patch...

 arch/x86/kvm/x86.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 4a597aafe637..1cc02a3685d0 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10981,9 +10981,6 @@ 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.93.g670b81a890-goog

Re: [PATCH] Revert "KVM: x86: WARN and reject loading KVM if NX is supported but not enabled"

[Paolo Bonzini]

On 25/06/21 02:18, Sean Christopherson wrote:
> Let KVM load if EFER.NX=0 even if NX is supported, the analysis and
> testing (or lack thereof) for the non-PAE host case was garbage.
> 
> If the kernel won't be using PAE paging, .Ldefault_entry in head_32.S
> skips over the entire EFER sequence.  Hopefully that can be changed in
> the future to allow KVM to require EFER.NX, but the motivation behind
> KVM's requirement isn't yet merged.  Reverting and revisiting the mess
> at a later date is by far the safest approach.
> 
> This reverts commit 8bbed95d2cb6e5de8a342d761a89b0a04faed7be.
> 
> Fixes: 8bbed95d2cb6 ("KVM: x86: WARN and reject loading KVM if NX is supported but not enabled")
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> 
> Hopefully it's not too late to just drop the original patch...
> 
>   arch/x86/kvm/x86.c | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 4a597aafe637..1cc02a3685d0 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -10981,9 +10981,6 @@ 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);
> 

So do we want this or "depends on X86_64 || X86_PAE"?

Paolo

Re: [PATCH] Revert "KVM: x86: WARN and reject loading KVM if NX is supported but not enabled"

[Sean Christopherson]

On Thu, Jul 08, 2021, Paolo Bonzini wrote:
> On 25/06/21 02:18, Sean Christopherson wrote:
> > Let KVM load if EFER.NX=0 even if NX is supported, the analysis and
> > testing (or lack thereof) for the non-PAE host case was garbage.
> > 
> > If the kernel won't be using PAE paging, .Ldefault_entry in head_32.S
> > skips over the entire EFER sequence.  Hopefully that can be changed in
> > the future to allow KVM to require EFER.NX, but the motivation behind
> > KVM's requirement isn't yet merged.  Reverting and revisiting the mess
> > at a later date is by far the safest approach.
> > 
> > This reverts commit 8bbed95d2cb6e5de8a342d761a89b0a04faed7be.
> > 
> > Fixes: 8bbed95d2cb6 ("KVM: x86: WARN and reject loading KVM if NX is supported but not enabled")
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> > 
> > Hopefully it's not too late to just drop the original patch...
> > 
> >   arch/x86/kvm/x86.c | 3 ---
> >   1 file changed, 3 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 4a597aafe637..1cc02a3685d0 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -10981,9 +10981,6 @@ 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);
> > 
> 
> So do we want this or "depends on X86_64 || X86_PAE"?

Hmm, I'm leaning towards keeping !PAE support purely for testing the !PAE<->PAE
MMU transitions for nested virtualization.  It's not much coverage, and the !PAE
NPT horror is a much bigger testing gap (because KVM doesn't support it), but on
the other hand setting EFER.NX for !PAE kernels appears to be trivial, e.g.

diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
index 67f590425d90..bfbea25a9fe8 100644
--- a/arch/x86/kernel/head_32.S
+++ b/arch/x86/kernel/head_32.S
@@ -214,12 +214,6 @@ SYM_FUNC_START(startup_32_smp)
        andl $~1,%edx                   # Ignore CPUID.FPU
        jz .Lenable_paging              # No flags or only CPUID.FPU = no CR4

-       movl pa(mmu_cr4_features),%eax
-       movl %eax,%cr4
-
-       testb $X86_CR4_PAE, %al         # check if PAE is enabled
-       jz .Lenable_paging
-
        /* Check if extended functions are implemented */
        movl $0x80000000, %eax
        cpuid

My only hesitation is the risk of somehow breaking ancient CPUs by falling into
the NX path.  Maybe try forcing EFER.NX=1 for !PAE, and fall back to requiring
PAE if that gets NAK'd or needs to be reverted for whatever reason?

Re: [PATCH] Revert "KVM: x86: WARN and reject loading KVM if NX is supported but not enabled"

[Paolo Bonzini]

On 09/07/21 19:21, Sean Christopherson wrote:
> On Thu, Jul 08, 2021, Paolo Bonzini wrote:
>> On 25/06/21 02:18, Sean Christopherson wrote:
>>> Let KVM load if EFER.NX=0 even if NX is supported, the analysis and
>>> testing (or lack thereof) for the non-PAE host case was garbage.
>>>
>>> If the kernel won't be using PAE paging, .Ldefault_entry in head_32.S
>>> skips over the entire EFER sequence.  Hopefully that can be changed in
>>> the future to allow KVM to require EFER.NX, but the motivation behind
>>> KVM's requirement isn't yet merged.  Reverting and revisiting the mess
>>> at a later date is by far the safest approach.
>>>
>>> This reverts commit 8bbed95d2cb6e5de8a342d761a89b0a04faed7be.
>>>
>>> Fixes: 8bbed95d2cb6 ("KVM: x86: WARN and reject loading KVM if NX is supported but not enabled")
>>> Signed-off-by: Sean Christopherson <seanjc@google.com>
>>> ---
>>>
>>> Hopefully it's not too late to just drop the original patch...
>>>
>>>    arch/x86/kvm/x86.c | 3 ---
>>>    1 file changed, 3 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index 4a597aafe637..1cc02a3685d0 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -10981,9 +10981,6 @@ 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);
>>>
>>
>> So do we want this or "depends on X86_64 || X86_PAE"?
> 
> Hmm, I'm leaning towards keeping !PAE support purely for testing the !PAE<->PAE
> MMU transitions for nested virtualization.  It's not much coverage, and the !PAE
> NPT horror is a much bigger testing gap (because KVM doesn't support it), but on
> the other hand setting EFER.NX for !PAE kernels appears to be trivial, e.g.
> 
> diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
> index 67f590425d90..bfbea25a9fe8 100644
> --- a/arch/x86/kernel/head_32.S
> +++ b/arch/x86/kernel/head_32.S
> @@ -214,12 +214,6 @@ SYM_FUNC_START(startup_32_smp)
>          andl $~1,%edx                   # Ignore CPUID.FPU
>          jz .Lenable_paging              # No flags or only CPUID.FPU = no CR4
> 
> -       movl pa(mmu_cr4_features),%eax
> -       movl %eax,%cr4
> -
> -       testb $X86_CR4_PAE, %al         # check if PAE is enabled
> -       jz .Lenable_paging
> -
>          /* Check if extended functions are implemented */
>          movl $0x80000000, %eax
>          cpuid
> 
> My only hesitation is the risk of somehow breaking ancient CPUs by falling into
> the NX path.  Maybe try forcing EFER.NX=1 for !PAE, and fall back to requiring
> PAE if that gets NAK'd or needs to be reverted for whatever reason?
> 

For now let's revert anyway.  Thanks!

Paolo

Re: [PATCH] Revert "KVM: x86: WARN and reject loading KVM if NX is supported but not enabled"

[Yu Zhang]

On Fri, Jul 09, 2021 at 05:21:52PM +0000, Sean Christopherson wrote:
> On Thu, Jul 08, 2021, Paolo Bonzini wrote:
> > On 25/06/21 02:18, Sean Christopherson wrote:
> > > Let KVM load if EFER.NX=0 even if NX is supported, the analysis and
> > > testing (or lack thereof) for the non-PAE host case was garbage.
> > > 
> > > If the kernel won't be using PAE paging, .Ldefault_entry in head_32.S
> > > skips over the entire EFER sequence.  Hopefully that can be changed in
> > > the future to allow KVM to require EFER.NX, but the motivation behind
> > > KVM's requirement isn't yet merged.  Reverting and revisiting the mess
> > > at a later date is by far the safest approach.
> > > 
> > > This reverts commit 8bbed95d2cb6e5de8a342d761a89b0a04faed7be.
> > > 
> > > Fixes: 8bbed95d2cb6 ("KVM: x86: WARN and reject loading KVM if NX is supported but not enabled")
> > > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > > ---
> > > 
> > > Hopefully it's not too late to just drop the original patch...
> > > 
> > >   arch/x86/kvm/x86.c | 3 ---
> > >   1 file changed, 3 deletions(-)
> > > 
> > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > index 4a597aafe637..1cc02a3685d0 100644
> > > --- a/arch/x86/kvm/x86.c
> > > +++ b/arch/x86/kvm/x86.c
> > > @@ -10981,9 +10981,6 @@ 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);
> > > 
> > 
> > So do we want this or "depends on X86_64 || X86_PAE"?
> 
> Hmm, I'm leaning towards keeping !PAE support purely for testing the !PAE<->PAE
> MMU transitions for nested virtualization.  It's not much coverage, and the !PAE

May I ask what "!PAE<->PAE MMU transition for nested virtualization" means?
Running L1 KVM with !PAE and L0 in PAE? I had thought KVM can only function
with PAE set(though I did not see any check of CR4 in kvm_arch_init()). Did
I miss something?

> NPT horror is a much bigger testing gap (because KVM doesn't support it), but on
> the other hand setting EFER.NX for !PAE kernels appears to be trivial, e.g.
> 
> diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
> index 67f590425d90..bfbea25a9fe8 100644
> --- a/arch/x86/kernel/head_32.S
> +++ b/arch/x86/kernel/head_32.S
> @@ -214,12 +214,6 @@ SYM_FUNC_START(startup_32_smp)
>         andl $~1,%edx                   # Ignore CPUID.FPU
>         jz .Lenable_paging              # No flags or only CPUID.FPU = no CR4
> 
> -       movl pa(mmu_cr4_features),%eax
> -       movl %eax,%cr4
> -
> -       testb $X86_CR4_PAE, %al         # check if PAE is enabled
> -       jz .Lenable_paging
> -
>         /* Check if extended functions are implemented */
>         movl $0x80000000, %eax
>         cpuid
> 
> My only hesitation is the risk of somehow breaking ancient CPUs by falling into
> the NX path.  Maybe try forcing EFER.NX=1 for !PAE, and fall back to requiring
> PAE if that gets NAK'd or needs to be reverted for whatever reason?
> 

One more dumb question: are you planning to set NX for linux with !PAE? Why do
we need EFER in that case? Thanks! :)


B.R.
Yu

Re: [PATCH] Revert "KVM: x86: WARN and reject loading KVM if NX is supported but not enabled"

[Sean Christopherson]

On Mon, Jul 12, 2021, Yu Zhang wrote:
> On Fri, Jul 09, 2021 at 05:21:52PM +0000, Sean Christopherson wrote:
> > On Thu, Jul 08, 2021, Paolo Bonzini wrote:
> > > So do we want this or "depends on X86_64 || X86_PAE"?
> > 
> > Hmm, I'm leaning towards keeping !PAE support purely for testing the !PAE<->PAE
> > MMU transitions for nested virtualization.  It's not much coverage, and the !PAE
> 
> May I ask what "!PAE<->PAE MMU transition for nested virtualization" means?
> Running L1 KVM with !PAE and L0 in PAE? I had thought KVM can only function
> with PAE set(though I did not see any check of CR4 in kvm_arch_init()). Did
> I miss something?

When L1 uses shadow paging, L0 KVM's uses a single MMU instance for both L1 and
L2, and relies on the MMU role to differentiate between L1 and L2.  KVM requires
PAE for shadow paging, but does not require PAE in the host kernel.  So when L1
KVM uses shadow paging, it can effectively use !PAE paging for L1 and PAE paging
for L2.  L0 KVM needs to handle that the !PAE<->PAE transitions when switching
between L1 and L2, e.g. needs to correctly reinitialize the MMU context.

> > NPT horror is a much bigger testing gap (because KVM doesn't support it), but on
> > the other hand setting EFER.NX for !PAE kernels appears to be trivial, e.g.
> > 
> > diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
> > index 67f590425d90..bfbea25a9fe8 100644
> > --- a/arch/x86/kernel/head_32.S
> > +++ b/arch/x86/kernel/head_32.S
> > @@ -214,12 +214,6 @@ SYM_FUNC_START(startup_32_smp)
> >         andl $~1,%edx                   # Ignore CPUID.FPU
> >         jz .Lenable_paging              # No flags or only CPUID.FPU = no CR4
> > 
> > -       movl pa(mmu_cr4_features),%eax
> > -       movl %eax,%cr4
> > -
> > -       testb $X86_CR4_PAE, %al         # check if PAE is enabled
> > -       jz .Lenable_paging
> > -
> >         /* Check if extended functions are implemented */
> >         movl $0x80000000, %eax
> >         cpuid
> > 
> > My only hesitation is the risk of somehow breaking ancient CPUs by falling into
> > the NX path.  Maybe try forcing EFER.NX=1 for !PAE, and fall back to requiring
> > PAE if that gets NAK'd or needs to be reverted for whatever reason?
> > 
> 
> One more dumb question: are you planning to set NX for linux with !PAE?

Yep.

> Why do we need EFER in that case? Thanks! :)

Because as you rightly remembered above, KVM always uses PAE paging for the guest,
even when the host is !PAE.  And KVM also requires EFER.NX=1 for the guest when
using shadow paging to handle a potential SMEP and !WP case.  

Re: [PATCH] Revert "KVM: x86: WARN and reject loading KVM if NX is supported but not enabled"

[Yu Zhang]

On Mon, Jul 12, 2021 at 02:36:53PM +0000, Sean Christopherson wrote:
> On Mon, Jul 12, 2021, Yu Zhang wrote:
> > On Fri, Jul 09, 2021 at 05:21:52PM +0000, Sean Christopherson wrote:
> > > On Thu, Jul 08, 2021, Paolo Bonzini wrote:
> > > > So do we want this or "depends on X86_64 || X86_PAE"?
> > > 
> > > Hmm, I'm leaning towards keeping !PAE support purely for testing the !PAE<->PAE
> > > MMU transitions for nested virtualization.  It's not much coverage, and the !PAE
> > 
> > May I ask what "!PAE<->PAE MMU transition for nested virtualization" means?
> > Running L1 KVM with !PAE and L0 in PAE? I had thought KVM can only function
> > with PAE set(though I did not see any check of CR4 in kvm_arch_init()). Did
> > I miss something?
> 
> When L1 uses shadow paging, L0 KVM's uses a single MMU instance for both L1 and
> L2, and relies on the MMU role to differentiate between L1 and L2.  KVM requires
> PAE for shadow paging, but does not require PAE in the host kernel.  So when L1
> KVM uses shadow paging, it can effectively use !PAE paging for L1 and PAE paging
> for L2.  L0 KVM needs to handle that the !PAE<->PAE transitions when switching
> between L1 and L2, e.g. needs to correctly reinitialize the MMU context.

Hah... Actually, I do have a misunderstanding here. The host does not need to be
PAE. Thanks for the explanation! :)

> 
> > > NPT horror is a much bigger testing gap (because KVM doesn't support it), but on
> > > the other hand setting EFER.NX for !PAE kernels appears to be trivial, e.g.
> > > 
> > > diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
> > > index 67f590425d90..bfbea25a9fe8 100644
> > > --- a/arch/x86/kernel/head_32.S
> > > +++ b/arch/x86/kernel/head_32.S
> > > @@ -214,12 +214,6 @@ SYM_FUNC_START(startup_32_smp)
> > >         andl $~1,%edx                   # Ignore CPUID.FPU
> > >         jz .Lenable_paging              # No flags or only CPUID.FPU = no CR4
> > > 
> > > -       movl pa(mmu_cr4_features),%eax
> > > -       movl %eax,%cr4
> > > -
> > > -       testb $X86_CR4_PAE, %al         # check if PAE is enabled
> > > -       jz .Lenable_paging
> > > -
> > >         /* Check if extended functions are implemented */
> > >         movl $0x80000000, %eax
> > >         cpuid
> > > 
> > > My only hesitation is the risk of somehow breaking ancient CPUs by falling into
> > > the NX path.  Maybe try forcing EFER.NX=1 for !PAE, and fall back to requiring
> > > PAE if that gets NAK'd or needs to be reverted for whatever reason?
> > > 
> > 
> > One more dumb question: are you planning to set NX for linux with !PAE?
> 
> Yep.
> 
> > Why do we need EFER in that case? Thanks! :)
> 
> Because as you rightly remembered above, KVM always uses PAE paging for the guest,
> even when the host is !PAE.  And KVM also requires EFER.NX=1 for the guest when
> using shadow paging to handle a potential SMEP and !WP case.  
> 

Just saw this in update_transition_efer(), which now enables efer.nx in shadow
unconditionally. But I guess the host kernel still needs to set efer.nx for
!PAE(e.g. in head_32.S), because the guest may not touch efer at all. Is this
correct?

B.R.
Yu

Re: [PATCH] Revert "KVM: x86: WARN and reject loading KVM if NX is supported but not enabled"

[Sean Christopherson]

On Tue, Jul 13, 2021, Yu Zhang wrote:
> On Mon, Jul 12, 2021 at 02:36:53PM +0000, Sean Christopherson wrote:
> > On Mon, Jul 12, 2021, Yu Zhang wrote:
> > > Why do we need EFER in that case? Thanks! :)
> > 
> > Because as you rightly remembered above, KVM always uses PAE paging for the guest,
> > even when the host is !PAE.  And KVM also requires EFER.NX=1 for the guest when
> > using shadow paging to handle a potential SMEP and !WP case.  
> 
> Just saw this in update_transition_efer(), which now enables efer.nx in shadow
> unconditionally. But I guess the host kernel still needs to set efer.nx for
> !PAE(e.g. in head_32.S),

Yep, and that's what I messed up.

> because the guest may not touch efer at all. Is this correct?

KVM doesn't require EFER.NX "because the guest may not touch efer at all", it
requires EFER.NX to handle scenarios where KVM needs to make a guest page
!EXECUTABLE even if EFER is not exposed to the guest (thanks to SMEP && !WP).