Re: [PATCH 10/22] KVM: x86/mmu: split XS/XU bits for MBEC

[Huang, Kai]

On Tue, 2026-03-24 at 12:24 +0100, Paolo Bonzini wrote:
> On 3/24/26 11:45, Huang, Kai wrote:
> > On Sat, 2026-03-21 at 01:09 +0100, Paolo Bonzini wrote:
> > > When EPT is in use, replace ACC_USER_MASK with ACC_USER_EXEC_MASK,
> > > so that supervisor and user-mode execution can be controlled
> > > independently (ACC_USER_MASK would not allow a setting similar to
> > > XU=0 XS=1 W=1 R=1).
> > > 
> > > Replace shadow_x_mask with shadow_xs_mask/shadow_xu_mask, to allow
> > > setting XS and XU bits separately in EPT entries.
> > > 
> > > Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > > ---
> > >   arch/x86/include/asm/vmx.h     |  1 +
> > >   arch/x86/kvm/mmu/mmu.c         | 15 ++++++++---
> > >   arch/x86/kvm/mmu/mmutrace.h    |  6 ++---
> > >   arch/x86/kvm/mmu/paging_tmpl.h |  4 +++
> > >   arch/x86/kvm/mmu/spte.c        | 47 ++++++++++++++++++++++------------
> > >   arch/x86/kvm/mmu/spte.h        |  8 +++---
> > >   6 files changed, 55 insertions(+), 26 deletions(-)
> > > 
> > > diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
> > > index 4a0804cc7c82..0041f8a77447 100644
> > > --- a/arch/x86/include/asm/vmx.h
> > > +++ b/arch/x86/include/asm/vmx.h
> > > @@ -538,6 +538,7 @@ enum vmcs_field {
> > >   #define VMX_EPT_IPAT_BIT    			(1ull << 6)
> > >   #define VMX_EPT_ACCESS_BIT			(1ull << 8)
> > >   #define VMX_EPT_DIRTY_BIT			(1ull << 9)
> > > +#define VMX_EPT_USER_EXECUTABLE_MASK		(1ull << 10)
> > >   #define VMX_EPT_SUPPRESS_VE_BIT			(1ull << 63)
> > >   #define VMX_EPT_RWX_MASK                        (VMX_EPT_READABLE_MASK |       \
> > >   						 VMX_EPT_WRITABLE_MASK |       \
> > 
> > Should we include VMX_EPT_USER_EXECUTABLE_MASK to VMX_EPT_RWX_MASK?
> 
> No, because it is used for many cases to refer to bits 0-2, for example:
> 
>   #define EPT_VIOLATION_RWX_TO_PROT(__epte)
>      (((__epte) & VMX_EPT_RWX_MASK) << 3)
> 
> Bit 10 is handled separately because it's not contiguous and has a 
> different mapping to the exit qualification (to bit 6 instead of bit 13).

OK.  It's a bit unfortunate but we can always explicitly get
EPT_VIOLATION_PROT_USER_EXEC from the VMX_EPT_USER_EXECUTABLE_MASK.

> 
> (However, there is a bug later in the series where shadow_acc_track_mask 
> needs to have VMX_EPT_USER_EXECUTABLE_MASK in it).

Right we need to track the XU bit too.

> 
> > 
> > [...]
> > 
> > > @@ -496,7 +507,8 @@ void kvm_mmu_set_ept_masks(bool has_ad_bits)
> > >   	shadow_accessed_mask	= VMX_EPT_ACCESS_BIT;
> > >   	shadow_dirty_mask	= VMX_EPT_DIRTY_BIT;
> > >   	shadow_nx_mask		= 0ull;
> > > -	shadow_x_mask		= VMX_EPT_EXECUTABLE_MASK;
> > > +	shadow_xs_mask		= VMX_EPT_EXECUTABLE_MASK;
> > > +	shadow_xu_mask		= VMX_EPT_EXECUTABLE_MASK;
> > 
> > Shouldn't 'shadow_xu_mask' be VMX_EPT_USER_EXECUTABLE_MASK?
> 
> Not yet, because shadow_xu_mask is used to set executable permissions as 
> well.  I suppose you could make it 0 when MBEC is disabled instead of 
> VMX_EPT_EXECUTABLE_MASK, but it can only be VMX_EPT_USER_EXECUTABLE_MASK 
> when MBEC is enabled.

I see.  It's changed to the right value in a later patch which actually
turns on MBEC.

> 
> > 
> > 
> > Btw, with MBEC it's a bit weird to me that we continue to just use
> > 110 (R=0,W=1,X=1) to trigger EPT misconfig for MMIO caching:
> > 
> >      /*
> >       * EPT Misconfigurations are generated if the value of bits 2:0
> >       * of an EPT paging-structure entry is 110b (write/execute).
> >       */
> >      kvm_mmu_set_mmio_spte_mask(VMX_EPT_MISCONFIG_WX_VALUE,
> >                                 VMX_EPT_RWX_MASK | VMX_EPT_SUPPRESS_VE_BIT,
> > 			       0);
> > 
> > Per SDM, R=0 and W=1 is always guaranteed to trigger EPT misconfig (see
> >   30.3.3.1 EPT Misconfigurations).  Maybe we can just use that for MMIO
> > caching?
> > 
> > We can then remove both X and XU bit from mmio_mask too.
> 
> Maybe but is it worth it?  (Based on this we could keep bit 10 in 
> MMIO_SPTE_GEN_LOW_END, after all, because W=1 R=0 would give a 
> misconfiguration independent of the value of XU; but again I'm not sure 
> it's worth it).

It looks promising to me since we can have a slightly clearer code (IMHO)
and one more bit for MMIO gen.  But no strong opinion :-)