KVM: VMX: Introduce Intel Mode-Based Execute Control (MBEC)

[RFC PATCH 15/18] KVM: x86/mmu: Extend make_spte to understand MBEC

Posted by Jon Kohler 9 months, 1 week ago

Extend make_spte to mask in and out bits depending on MBEC enablement.

Note: For the RFC/v1 series, I've added several 'For Review' items that
may require a bit deeper inspection, as well as some long winded
comments/annotations. These will be cleaned up for the next iteration
of the series after initial review.

Signed-off-by: Jon Kohler <jon@nutanix.com>
Co-developed-by: Sergey Dyasli <sergey.dyasli@nutanix.com>
Signed-off-by: Sergey Dyasli <sergey.dyasli@nutanix.com>

---
 arch/x86/kvm/mmu/paging_tmpl.h |  3 +++
 arch/x86/kvm/mmu/spte.c        | 30 ++++++++++++++++++++++++++----
 2 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h
index a3a5cacda614..7675239f2dd1 100644
--- a/arch/x86/kvm/mmu/paging_tmpl.h
+++ b/arch/x86/kvm/mmu/paging_tmpl.h
@@ -840,6 +840,9 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault
 		 * then we should prevent the kernel from executing it
 		 * if SMEP is enabled.
 		 */
+		// FOR REVIEW:
+		// ACC_USER_EXEC_MASK seems not necessary to add here since
+		// SMEP is for kernel-only.
 		if (is_cr4_smep(vcpu->arch.mmu))
 			walker.pte_access &= ~ACC_EXEC_MASK;
 	}
diff --git a/arch/x86/kvm/mmu/spte.c b/arch/x86/kvm/mmu/spte.c
index 6f4994b3e6d0..89bdae3f9ada 100644
--- a/arch/x86/kvm/mmu/spte.c
+++ b/arch/x86/kvm/mmu/spte.c
@@ -178,6 +178,9 @@ bool make_spte(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
 	else if (kvm_mmu_page_ad_need_write_protect(sp))
 		spte |= SPTE_TDP_AD_WRPROT_ONLY;
 
+	// For LKML Review:
+	// In MBEC case, you can have exec only and also bit 10
+	// set for user exec only. Do we need to cater for that here?
 	spte |= shadow_present_mask;
 	if (!prefetch)
 		spte |= spte_shadow_accessed_mask(spte);
@@ -197,12 +200,31 @@ bool make_spte(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
 	if (level > PG_LEVEL_4K && (pte_access & ACC_EXEC_MASK) &&
 	    is_nx_huge_page_enabled(vcpu->kvm)) {
 		pte_access &= ~ACC_EXEC_MASK;
+		if (vcpu->arch.pt_guest_exec_control)
+			pte_access &= ~ACC_USER_EXEC_MASK;
 	}
 
-	if (pte_access & ACC_EXEC_MASK)
-		spte |= shadow_x_mask;
-	else
-		spte |= shadow_nx_mask;
+	// For LKML Review:
+	// We could probably optimize the logic here, but typing it out
+	// long hand for now to make it clear how we're changing the control
+	// flow to support MBEC.
+	if (!vcpu->arch.pt_guest_exec_control) { // non-mbec logic
+		if (pte_access & ACC_EXEC_MASK)
+			spte |= shadow_x_mask;
+		else
+			spte |= shadow_nx_mask;
+	} else { // mbec logic
+		if (pte_access & ACC_EXEC_MASK) { /* mbec: kernel exec */
+			if (pte_access & ACC_USER_EXEC_MASK)
+				spte |= shadow_x_mask | shadow_ux_mask; // KMX = 1, UMX = 1
+			else
+				spte |= shadow_x_mask;  // KMX = 1, UMX = 0
+		} else if (pte_access & ACC_USER_EXEC_MASK) { /* mbec: user exec, no kernel exec */
+			spte |= shadow_ux_mask; // KMX = 0, UMX = 1
+		} else { /* mbec: nx */
+			spte |= shadow_nx_mask; // KMX = 0, UMX = 0
+		}
+	}
 
 	if (pte_access & ACC_USER_MASK)
 		spte |= shadow_user_mask;
-- 
2.43.0

Re: [RFC PATCH 15/18] KVM: x86/mmu: Extend make_spte to understand MBEC

Posted by Sean Christopherson 7 months, 1 week ago

On Thu, Mar 13, 2025, Jon Kohler wrote:
> Extend make_spte to mask in and out bits depending on MBEC enablement.

Same complaints about the shortlog and changelog not saying anything useful.

> 
> Note: For the RFC/v1 series, I've added several 'For Review' items that
> may require a bit deeper inspection, as well as some long winded
> comments/annotations. These will be cleaned up for the next iteration
> of the series after initial review.
> 
> Signed-off-by: Jon Kohler <jon@nutanix.com>
> Co-developed-by: Sergey Dyasli <sergey.dyasli@nutanix.com>
> Signed-off-by: Sergey Dyasli <sergey.dyasli@nutanix.com>
> 
> ---
>  arch/x86/kvm/mmu/paging_tmpl.h |  3 +++
>  arch/x86/kvm/mmu/spte.c        | 30 ++++++++++++++++++++++++++----
>  2 files changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h
> index a3a5cacda614..7675239f2dd1 100644
> --- a/arch/x86/kvm/mmu/paging_tmpl.h
> +++ b/arch/x86/kvm/mmu/paging_tmpl.h
> @@ -840,6 +840,9 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault
>  		 * then we should prevent the kernel from executing it
>  		 * if SMEP is enabled.
>  		 */
> +		// FOR REVIEW:
> +		// ACC_USER_EXEC_MASK seems not necessary to add here since
> +		// SMEP is for kernel-only.
>  		if (is_cr4_smep(vcpu->arch.mmu))
>  			walker.pte_access &= ~ACC_EXEC_MASK;

I would straight up WARN, because it should be impossible to reach this code with
ACC_USER_EXEC_MASK set.  In fact, this entire blob of code should be #ifdef'd
out for PTTYPE_EPT.  AFAICT, the only reason it doesn't break nEPT is because
its impossible to have a WRITE EPT violation without READ (a.k.a. USER) being
set.

>  	}
> diff --git a/arch/x86/kvm/mmu/spte.c b/arch/x86/kvm/mmu/spte.c
> index 6f4994b3e6d0..89bdae3f9ada 100644
> --- a/arch/x86/kvm/mmu/spte.c
> +++ b/arch/x86/kvm/mmu/spte.c
> @@ -178,6 +178,9 @@ bool make_spte(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
>  	else if (kvm_mmu_page_ad_need_write_protect(sp))
>  		spte |= SPTE_TDP_AD_WRPROT_ONLY;
>  
> +	// For LKML Review:
> +	// In MBEC case, you can have exec only and also bit 10
> +	// set for user exec only. Do we need to cater for that here?
>  	spte |= shadow_present_mask;
>  	if (!prefetch)
>  		spte |= spte_shadow_accessed_mask(spte);
> @@ -197,12 +200,31 @@ bool make_spte(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
>  	if (level > PG_LEVEL_4K && (pte_access & ACC_EXEC_MASK) &&

Needs to check ACC_USER_EXEC_MASK.

>  	    is_nx_huge_page_enabled(vcpu->kvm)) {
>  		pte_access &= ~ACC_EXEC_MASK;
> +		if (vcpu->arch.pt_guest_exec_control)
> +			pte_access &= ~ACC_USER_EXEC_MASK;
>  	}
>  
> -	if (pte_access & ACC_EXEC_MASK)
> -		spte |= shadow_x_mask;
> -	else
> -		spte |= shadow_nx_mask;
> +	// For LKML Review:
> +	// We could probably optimize the logic here, but typing it out
> +	// long hand for now to make it clear how we're changing the control
> +	// flow to support MBEC.

I appreciate the effort, but this did far more harm than good.  Reviewing code
that has zero chance of being the end product is a waste of time.  And unless I'm
overlooking a subtlety, you're making this way harder than it needs to be:

	if (pte_access & (ACC_EXEC_MASK | ACC_USER_EXEC_MASK)) {
		if (pte_access & ACC_EXEC_MASK)
			spte |= shadow_x_mask;

		if (pte_access & ACC_USER_EXEC_MASK)
			spte |= shadow_ux_mask;
	} else {
		spte |= shadow_nx_mask;
	}

KVM needs to ensure ACC_USER_EXEC_MASK isn't spuriously set, but KVM should be
doing that anyways.

> +	if (!vcpu->arch.pt_guest_exec_control) { // non-mbec logic
> +		if (pte_access & ACC_EXEC_MASK)
> +			spte |= shadow_x_mask;
> +		else
> +			spte |= shadow_nx_mask;
> +	} else { // mbec logic
> +		if (pte_access & ACC_EXEC_MASK) { /* mbec: kernel exec */
> +			if (pte_access & ACC_USER_EXEC_MASK)
> +				spte |= shadow_x_mask | shadow_ux_mask; // KMX = 1, UMX = 1
> +			else
> +				spte |= shadow_x_mask;  // KMX = 1, UMX = 0
> +		} else if (pte_access & ACC_USER_EXEC_MASK) { /* mbec: user exec, no kernel exec */
> +			spte |= shadow_ux_mask; // KMX = 0, UMX = 1
> +		} else { /* mbec: nx */
> +			spte |= shadow_nx_mask; // KMX = 0, UMX = 0
> +		}
> +	}
>  
>  	if (pte_access & ACC_USER_MASK)
>  		spte |= shadow_user_mask;
> -- 
> 2.43.0
>

Re: [RFC PATCH 15/18] KVM: x86/mmu: Extend make_spte to understand MBEC

Posted by Jon Kohler 7 months, 1 week ago


> On May 12, 2025, at 5:29 PM, Sean Christopherson <seanjc@google.com> wrote:
> 
> !-------------------------------------------------------------------|
>  CAUTION: External Email
> 
> |-------------------------------------------------------------------!
> 
> On Thu, Mar 13, 2025, Jon Kohler wrote:
>> Extend make_spte to mask in and out bits depending on MBEC enablement.
> 
> Same complaints about the shortlog and changelog not saying anything useful.

ack

> 
>> 
>> Note: For the RFC/v1 series, I've added several 'For Review' items that
>> may require a bit deeper inspection, as well as some long winded
>> comments/annotations. These will be cleaned up for the next iteration
>> of the series after initial review.
>> 
>> Signed-off-by: Jon Kohler <jon@nutanix.com>
>> Co-developed-by: Sergey Dyasli <sergey.dyasli@nutanix.com>
>> Signed-off-by: Sergey Dyasli <sergey.dyasli@nutanix.com>
>> 
>> ---
>> arch/x86/kvm/mmu/paging_tmpl.h |  3 +++
>> arch/x86/kvm/mmu/spte.c        | 30 ++++++++++++++++++++++++++----
>> 2 files changed, 29 insertions(+), 4 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h
>> index a3a5cacda614..7675239f2dd1 100644
>> --- a/arch/x86/kvm/mmu/paging_tmpl.h
>> +++ b/arch/x86/kvm/mmu/paging_tmpl.h
>> @@ -840,6 +840,9 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault
>>  * then we should prevent the kernel from executing it
>>  * if SMEP is enabled.
>>  */
>> + // FOR REVIEW:
>> + // ACC_USER_EXEC_MASK seems not necessary to add here since
>> + // SMEP is for kernel-only.
>> if (is_cr4_smep(vcpu->arch.mmu))
>> walker.pte_access &= ~ACC_EXEC_MASK;
> 
> I would straight up WARN, because it should be impossible to reach this code with
> ACC_USER_EXEC_MASK set.  In fact, this entire blob of code should be #ifdef'd
> out for PTTYPE_EPT.  AFAICT, the only reason it doesn't break nEPT is because
> its impossible to have a WRITE EPT violation without READ (a.k.a. USER) being
> set.

Would you like me to send a separate patch out for that to clean up as
I go? Or make such ifdef’ery as part of this series?

> 
>> }
>> diff --git a/arch/x86/kvm/mmu/spte.c b/arch/x86/kvm/mmu/spte.c
>> index 6f4994b3e6d0..89bdae3f9ada 100644
>> --- a/arch/x86/kvm/mmu/spte.c
>> +++ b/arch/x86/kvm/mmu/spte.c
>> @@ -178,6 +178,9 @@ bool make_spte(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
>> else if (kvm_mmu_page_ad_need_write_protect(sp))
>> spte |= SPTE_TDP_AD_WRPROT_ONLY;
>> 
>> + // For LKML Review:
>> + // In MBEC case, you can have exec only and also bit 10
>> + // set for user exec only. Do we need to cater for that here?
>> spte |= shadow_present_mask;
>> if (!prefetch)
>> spte |= spte_shadow_accessed_mask(spte);
>> @@ -197,12 +200,31 @@ bool make_spte(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
>> if (level > PG_LEVEL_4K && (pte_access & ACC_EXEC_MASK) &&
> 
> Needs to check ACC_USER_EXEC_MASK.
> 
>>     is_nx_huge_page_enabled(vcpu->kvm)) {
>> pte_access &= ~ACC_EXEC_MASK;
>> + if (vcpu->arch.pt_guest_exec_control)
>> + pte_access &= ~ACC_USER_EXEC_MASK;
>> }
>> 
>> - if (pte_access & ACC_EXEC_MASK)
>> - spte |= shadow_x_mask;
>> - else
>> - spte |= shadow_nx_mask;
>> + // For LKML Review:
>> + // We could probably optimize the logic here, but typing it out
>> + // long hand for now to make it clear how we're changing the control
>> + // flow to support MBEC.
> 
> I appreciate the effort, but this did far more harm than good.  Reviewing code
> that has zero chance of being the end product is a waste of time.  And unless I'm
> overlooking a subtlety, you're making this way harder than it needs to be:
> 
> if (pte_access & (ACC_EXEC_MASK | ACC_USER_EXEC_MASK)) {
> if (pte_access & ACC_EXEC_MASK)
> spte |= shadow_x_mask;
> 
> if (pte_access & ACC_USER_EXEC_MASK)
> spte |= shadow_ux_mask;
> } else {
> spte |= shadow_nx_mask;
> }

Ack, my apologies, wasn’t trying to make things harder, but I appreciate the
candid feedback. Thanks for the suggested code, I’ll incorporate that on the next
go. 

> 
> KVM needs to ensure ACC_USER_EXEC_MASK isn't spuriously set, but KVM should be
> doing that anyways.
> 
>> + if (!vcpu->arch.pt_guest_exec_control) { // non-mbec logic
>> + if (pte_access & ACC_EXEC_MASK)
>> + spte |= shadow_x_mask;
>> + else
>> + spte |= shadow_nx_mask;
>> + } else { // mbec logic
>> + if (pte_access & ACC_EXEC_MASK) { /* mbec: kernel exec */
>> + if (pte_access & ACC_USER_EXEC_MASK)
>> + spte |= shadow_x_mask | shadow_ux_mask; // KMX = 1, UMX = 1
>> + else
>> + spte |= shadow_x_mask;  // KMX = 1, UMX = 0
>> + } else if (pte_access & ACC_USER_EXEC_MASK) { /* mbec: user exec, no kernel exec */
>> + spte |= shadow_ux_mask; // KMX = 0, UMX = 1
>> + } else { /* mbec: nx */
>> + spte |= shadow_nx_mask; // KMX = 0, UMX = 0
>> + }
>> + }
>> 
>> if (pte_access & ACC_USER_MASK)
>> spte |= shadow_user_mask;
>> -- 
>> 2.43.0

Re: [RFC PATCH 15/18] KVM: x86/mmu: Extend make_spte to understand MBEC

Posted by Sean Christopherson 7 months, 1 week ago

On Tue, May 13, 2025, Jon Kohler wrote:
> > I would straight up WARN, because it should be impossible to reach this code with
> > ACC_USER_EXEC_MASK set.  In fact, this entire blob of code should be #ifdef'd
> > out for PTTYPE_EPT.  AFAICT, the only reason it doesn't break nEPT is because
> > its impossible to have a WRITE EPT violation without READ (a.k.a. USER) being
> > set.
> 
> Would you like me to send a separate patch out for that to clean up as
> I go? Or make such ifdef’ery as part of this series?

I'll send a patch.  It's not at all urgent, not a hard dependency for MBEC, the
comment(s) needs to be rewritten, I want to do an audit of paging_tmpl.h to see
if there is more code that'd be worth #idef'ing out for nEPT.