[PATCH v2 01/18] KVM: x86: host-initiated EFER.LME write affects the MMU

[PATCH v2 01/18] KVM: x86: host-initiated EFER.LME write affects the MMU

[Paolo Bonzini]

While the guest runs, EFER.LME cannot change unless CR0.PG is clear, and therefore
EFER.NX is the only bit that can affect the MMU role.  However, set_efer accepts
a host-initiated change to EFER.LME even with CR0.PG=1.  In that case, the
MMU has to be reset.

Fixes: 11988499e62b ("KVM: x86: Skip EFER vs. guest CPUID checks for host-initiated writes")
Cc: stable@vger.kernel.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/mmu.h | 1 +
 arch/x86/kvm/x86.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
index 51faa2c76ca5..a5a50cfeffff 100644
--- a/arch/x86/kvm/mmu.h
+++ b/arch/x86/kvm/mmu.h
@@ -48,6 +48,7 @@
 			       X86_CR4_SMEP | X86_CR4_SMAP | X86_CR4_PKE)
 
 #define KVM_MMU_CR0_ROLE_BITS (X86_CR0_PG | X86_CR0_WP)
+#define KVM_MMU_EFER_ROLE_BITS (EFER_LME | EFER_NX)
 
 static __always_inline u64 rsvd_bits(int s, int e)
 {
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d3da64106685..99a58c25f5c2 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1647,7 +1647,7 @@ static int set_efer(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	}
 
 	/* Update reserved bits */
-	if ((efer ^ old_efer) & EFER_NX)
+	if ((efer ^ old_efer) & KVM_MMU_EFER_ROLE_BITS)
 		kvm_mmu_reset_context(vcpu);
 
 	return 0;
-- 
2.31.1

Re: [PATCH v2 01/18] KVM: x86: host-initiated EFER.LME write affects the MMU

[Sean Christopherson]

The shortlog doesn't come remotely close to saying what this patch does, it's
simply a statement.

  KVM: x86: Reset the MMU context if host userspace toggles EFER.LME

On Thu, Feb 17, 2022, Paolo Bonzini wrote:
> While the guest runs, EFER.LME cannot change unless CR0.PG is clear, and therefore
> EFER.NX is the only bit that can affect the MMU role.  However, set_efer accepts
> a host-initiated change to EFER.LME even with CR0.PG=1.  In that case, the
> MMU has to be reset.

Wrap at ~75 please.

> Fixes: 11988499e62b ("KVM: x86: Skip EFER vs. guest CPUID checks for host-initiated writes")
> Cc: stable@vger.kernel.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---

With nits addressed,

Reviewed-by: Sean Christopherson <seanjc@google.com>

>  arch/x86/kvm/mmu.h | 1 +
>  arch/x86/kvm/x86.c | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
> index 51faa2c76ca5..a5a50cfeffff 100644
> --- a/arch/x86/kvm/mmu.h
> +++ b/arch/x86/kvm/mmu.h
> @@ -48,6 +48,7 @@
>  			       X86_CR4_SMEP | X86_CR4_SMAP | X86_CR4_PKE)
>  
>  #define KVM_MMU_CR0_ROLE_BITS (X86_CR0_PG | X86_CR0_WP)
> +#define KVM_MMU_EFER_ROLE_BITS (EFER_LME | EFER_NX)
>  
>  static __always_inline u64 rsvd_bits(int s, int e)
>  {
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index d3da64106685..99a58c25f5c2 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1647,7 +1647,7 @@ static int set_efer(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  	}
>  
>  	/* Update reserved bits */

This comment needs to be dropped, toggling EFER.LME affects more than just reserved
bits.

> -	if ((efer ^ old_efer) & EFER_NX)
> +	if ((efer ^ old_efer) & KVM_MMU_EFER_ROLE_BITS)
>  		kvm_mmu_reset_context(vcpu);
>  
>  	return 0;
> -- 
> 2.31.1
> 
> 

Re: [PATCH v2 01/18] KVM: x86: host-initiated EFER.LME write affects the MMU

[Paolo Bonzini]

On 2/18/22 18:08, Sean Christopherson wrote:
> The shortlog doesn't come remotely close to saying what this patch does, it's
> simply a statement.
> 
>    KVM: x86: Reset the MMU context if host userspace toggles EFER.LME

I'd like not to use "reset the MMU context" because 1) the meaning 
changes at the end of the series so it's not the best time to use the 
expression, 2) actually I hope to get rid of it completely and just use 
kvm_init_mmu.

I'll use "Reinitialize MMU" which is the important part of 
kvm_reset_mmu_context().

Paolo

> On Thu, Feb 17, 2022, Paolo Bonzini wrote:
>> While the guest runs, EFER.LME cannot change unless CR0.PG is clear, and therefore
>> EFER.NX is the only bit that can affect the MMU role.  However, set_efer accepts
>> a host-initiated change to EFER.LME even with CR0.PG=1.  In that case, the
>> MMU has to be reset.
> 
> Wrap at ~75 please.
> 
>> Fixes: 11988499e62b ("KVM: x86: Skip EFER vs. guest CPUID checks for host-initiated writes")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
> 
> With nits addressed,
> 
> Reviewed-by: Sean Christopherson <seanjc@google.com>
> 
>>   arch/x86/kvm/mmu.h | 1 +
>>   arch/x86/kvm/x86.c | 2 +-
>>   2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
>> index 51faa2c76ca5..a5a50cfeffff 100644
>> --- a/arch/x86/kvm/mmu.h
>> +++ b/arch/x86/kvm/mmu.h
>> @@ -48,6 +48,7 @@
>>   			       X86_CR4_SMEP | X86_CR4_SMAP | X86_CR4_PKE)
>>   
>>   #define KVM_MMU_CR0_ROLE_BITS (X86_CR0_PG | X86_CR0_WP)
>> +#define KVM_MMU_EFER_ROLE_BITS (EFER_LME | EFER_NX)
>>   
>>   static __always_inline u64 rsvd_bits(int s, int e)
>>   {
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index d3da64106685..99a58c25f5c2 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -1647,7 +1647,7 @@ static int set_efer(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>   	}
>>   
>>   	/* Update reserved bits */
> 
> This comment needs to be dropped, toggling EFER.LME affects more than just reserved
> bits.
> 
>> -	if ((efer ^ old_efer) & EFER_NX)
>> +	if ((efer ^ old_efer) & KVM_MMU_EFER_ROLE_BITS)
>>   		kvm_mmu_reset_context(vcpu);
>>   
>>   	return 0;
>> -- 
>> 2.31.1
>>
>>
> 

Re: [PATCH v2 01/18] KVM: x86: host-initiated EFER.LME write affects the MMU

[Maxim Levitsky]

On Thu, 2022-02-17 at 16:03 -0500, Paolo Bonzini wrote:
> While the guest runs, EFER.LME cannot change unless CR0.PG is clear, and therefore
> EFER.NX is the only bit that can affect the MMU role.  However, set_efer accepts
> a host-initiated change to EFER.LME even with CR0.PG=1.  In that case, the
> MMU has to be reset.
> 
> Fixes: 11988499e62b ("KVM: x86: Skip EFER vs. guest CPUID checks for host-initiated writes")
> Cc: stable@vger.kernel.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/mmu.h | 1 +
>  arch/x86/kvm/x86.c | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
> index 51faa2c76ca5..a5a50cfeffff 100644
> --- a/arch/x86/kvm/mmu.h
> +++ b/arch/x86/kvm/mmu.h
> @@ -48,6 +48,7 @@
>  			       X86_CR4_SMEP | X86_CR4_SMAP | X86_CR4_PKE)
>  
>  #define KVM_MMU_CR0_ROLE_BITS (X86_CR0_PG | X86_CR0_WP)
> +#define KVM_MMU_EFER_ROLE_BITS (EFER_LME | EFER_NX)
>  
>  static __always_inline u64 rsvd_bits(int s, int e)
>  {
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index d3da64106685..99a58c25f5c2 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1647,7 +1647,7 @@ static int set_efer(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  	}
>  
>  	/* Update reserved bits */
> -	if ((efer ^ old_efer) & EFER_NX)
> +	if ((efer ^ old_efer) & KVM_MMU_EFER_ROLE_BITS)
>  		kvm_mmu_reset_context(vcpu);
>  
>  	return 0;

It makes sense.

I am just curios, is there a report of failure
due to this issue? I can imagine something like this breaking
nested migration of 32 bit guests and such and/or smm and such.

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky