Re: [PATCH 16/54] KVM: x86/mmu: Drop smep_andnot_wp check from "uses NX" for shadow MMUs

[Sean Christopherson]

On Wed, Jun 23, 2021, Paolo Bonzini wrote:
> On 22/06/21 19:57, Sean Christopherson wrote:
> > Drop the smep_andnot_wp role check from the "uses NX" calculation now
> > that all non-nested shadow MMUs treat NX as used via the !TDP check.
> > 
> > The shadow MMU for nested NPT, which shares the helper, does not need to
> > deal with SMEP (or WP) as NPT walks are always "user" accesses and WP is
> > explicitly noted as being ignored:
> > 
> >    Table walks for guest page tables are always treated as user writes at
> >    the nested page table level.
> > 
> >    A table walk for the guest page itself is always treated as a user
> >    access at the nested page table level
> > 
> >    The host hCR0.WP bit is ignored under nested paging.
> > 
> > Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> > ---
> >   arch/x86/kvm/mmu/mmu.c | 3 +--
> >   1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index 96c16a6e0044..ca7680d1ea24 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -4223,8 +4223,7 @@ reset_shadow_zero_bits_mask(struct kvm_vcpu *vcpu, struct kvm_mmu *context)
> >   	 * NX can be used by any non-nested shadow MMU to avoid having to reset
> >   	 * MMU contexts.  Note, KVM forces EFER.NX=1 when TDP is disabled.
> >   	 */
> > -	bool uses_nx = context->nx || !tdp_enabled ||
> > -		context->mmu_role.base.smep_andnot_wp;
> > +	bool uses_nx = context->nx || !tdp_enabled;
> >   	struct rsvd_bits_validate *shadow_zero_check;
> >   	int i;
> > 
> 
> Good idea, but why not squash it into patch 2?

Because that patch is marked for stable and dropping the smep_andnot_wp is not
necessary to fix the bug.  At worst, the too-liberal uses_nx will suppress the
WARN in handle_mmio_page_fault() because this is for checking KVM's SPTEs, not
the guest's SPTEs, i.e. KVM won't miss a guest reserved NX #PF.

That said, I'm not at all opposed to squashing this.  I have a feeling I originally
split the patches because I wasn't super confident about either change, and never
revisited them.