Re: [PATCH v2 1/6] KVM: x86/mmu: Tag disallowed NX huge pages even if they're not tracked

[Sean Christopherson <seanjc@google.com>]

On Mon, Jul 25, 2022, David Matlack wrote:
> On Sat, Jul 23, 2022 at 01:23:20AM +0000, Sean Christopherson wrote:
> > Tag shadow pages that cannot be replaced with an NX huge page even if
> > zapping the page would not allow KVM to create a huge page, e.g. because
> > something else prevents creating a huge page.
> 
> This sentence looks messed up :). Should it read:
> 
>   Tag shadow pages that cannot be replaced with an NX huge page, e.g.
>   because something else prevents creating a huge page.
> 
> ?

Hmm, not quite.  Does this read better?

  Tag shadow pages that cannot be replaced with an NX huge page regardless
  of whether or not zapping the page would allow KVM to immediately create
  a huge page, e.g. because something else prevents creating a huge page.

What I'm trying to call out is that, today, KVM tracks pages that were disallowed
from being huge due to the NX workaround if and only if the page could otherwise
be huge.  After this patch, KVM will track pages that were disallowed regardless
of whether or they could have been huge at the time of fault.

> > +void account_nx_huge_page(struct kvm *kvm, struct kvm_mmu_page *sp,
> > +			  bool nx_huge_page_possible)
> > +{
> > +	sp->nx_huge_page_disallowed = true;
> > +
> > +	if (!nx_huge_page_possible)
> > +		untrack_possible_nx_huge_page(kvm, sp);
> 
> What would be a scenario where calling untrack_possible_nx_huge_page()
> is actually necessary here?

The only scenario that jumps to mind is the non-coherent DMA with funky MTRRs
case.  There might be others, but it's been a while since I wrote this...

The MTRRs are per-vCPU (KVM really should just track them as per-VM, but whatever),
so it's possible that KVM could encounter a fault with a lower fault->req_level
than a previous fault that set nx_huge_page_disallowed=true (and added the page
to the possible_nx_huge_pages list because it had a higher req_level).

> > @@ -5970,7 +5993,7 @@ int kvm_mmu_init_vm(struct kvm *kvm)
> >  
> >  	INIT_LIST_HEAD(&kvm->arch.active_mmu_pages);
> >  	INIT_LIST_HEAD(&kvm->arch.zapped_obsolete_pages);
> > -	INIT_LIST_HEAD(&kvm->arch.lpage_disallowed_mmu_pages);
> > +	INIT_LIST_HEAD(&kvm->arch.possible_nx_huge_pages);
> >  	spin_lock_init(&kvm->arch.mmu_unsync_pages_lock);
> >  
> >  	r = kvm_mmu_init_tdp_mmu(kvm);
> > @@ -6845,23 +6868,25 @@ static void kvm_recover_nx_lpages(struct kvm *kvm)
> 
> Can you rename this to kvm_recover_nx_huge_pages() while you're here?

Will do.

> > @@ -1134,7 +1136,7 @@ static int tdp_mmu_link_sp(struct kvm *kvm, struct tdp_iter *iter,
> >  	spin_lock(&kvm->arch.tdp_mmu_pages_lock);
> >  	list_add(&sp->link, &kvm->arch.tdp_mmu_pages);
> >  	if (account_nx)
> > -		account_huge_nx_page(kvm, sp);
> > +		account_nx_huge_page(kvm, sp, true);
> 
> 
> account_nx is fault->huge_page_disallowed && fault->req_level >=
> iter.level. So this is equivalent to:
> 
>   if (fault->huge_page_disallowed && fault->req_level >= iter.level)
>           account_nx_huge_page(kvm, sp, true);
> 
> Whereas __direct_map() uses:
> 
>   if (fault->is_tdp && fault->huge_page_disallowed)
>         account_nx_huge_page(vcpu->kvm, sp, fault->req_level >= it.level);
> 
> Aside from is_tdp (which you cover in another patch), why is there a
> discrepancy in the NX Huge Page accounting?

That wart gets fixed in patch 3.  Fixing the TDP MMU requires more work due to
mmu_lock being held for read and so I wanted to separate it out.  And as a minor
detail, the Fixes: from this patch predates the TDP MMU, so in a way it's kinda
sorta a different bug.

> >  	spin_unlock(&kvm->arch.tdp_mmu_pages_lock);
> >  
> >  	return 0;
> > -- 
> > 2.37.1.359.gd136c6c3e2-goog
> >