[1/2] kvm: x86/mmu: Reduce the update to the spte in FNAME(sync_page)

  From: Lai Jiangshan <jiangshan.ljs@antgroup.com>

Sometimes when the guest updates its pagetable, it adds only new gptes
to it without changing any existed one, so there is no point to update
the sptes for these existed gptes.

Also when the sptes for these unchanged gptes are updated, the AD
bits are also removed since make_spte() is called with prefetch=true
which might result unneeded TLB flushing.

Do nothing if the permissions are unchanged or only write-access is
being added.  Only update the spte when write-access is being removed.
Drop the SPTE otherwise.

Signed-off-by: Lai Jiangshan <jiangshan.ljs@antgroup.com>
---
 arch/x86/kvm/mmu/paging_tmpl.h | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)
  

On Mon, Dec 12, 2022, Lai Jiangshan wrote:
> From: Lai Jiangshan <jiangshan.ljs@antgroup.com>
> 
> Sometimes when the guest updates its pagetable, it adds only new gptes
> to it without changing any existed one, so there is no point to update
> the sptes for these existed gptes.
>
> Also when the sptes for these unchanged gptes are updated, the AD
> bits are also removed since make_spte() is called with prefetch=true
> which might result unneeded TLB flushing.

If either of the proposed changes is kept, please move this to a separate patch.
Skipping updates for PTEs with the same protections is separate logical change
from skipping updates when making the SPTE writable.

Actually, can't we just pass @prefetch=false to make_spte()?  FNAME(prefetch_invalid_gpte)
has already verified the Accessed bit is set in the GPTE, so at least for guest
correctness there's no need to access-track the SPTE.  Host page aging is already
fuzzy so I don't think there are problems there.

> Do nothing if the permissions are unchanged or only write-access is
> being added.

I'm pretty sure skipping the "make writable" case is architecturally wrong.  On a
#PF, any TLB entries for the faulting virtual address are required to be removed.
That means KVM _must_ refresh the SPTE if a vCPU takes a !WRITABLE fault on an
unsync page.  E.g. see kvm_inject_emulated_page_fault().

> Only update the spte when write-access is being removed.  Drop the SPTE
> otherwise.

Correctness aside, there needs to be far more analysis and justification for a
change like this, e.g. performance numbers for various workloads.

> ---
>  arch/x86/kvm/mmu/paging_tmpl.h | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h
> index e5662dbd519c..613f043a3e9e 100644
> --- a/arch/x86/kvm/mmu/paging_tmpl.h
> +++ b/arch/x86/kvm/mmu/paging_tmpl.h
> @@ -1023,7 +1023,7 @@ static int FNAME(sync_page)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp)
>  	for (i = 0; i < SPTE_ENT_PER_PAGE; i++) {
>  		u64 *sptep, spte;
>  		struct kvm_memory_slot *slot;
> -		unsigned pte_access;
> +		unsigned old_pte_access, pte_access;
>  		pt_element_t gpte;
>  		gpa_t pte_gpa;
>  		gfn_t gfn;
> @@ -1064,6 +1064,23 @@ static int FNAME(sync_page)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp)
>  			continue;
>  		}
>  
> +		/*
> +		 * Drop the SPTE if the new protections would result in access
> +		 * permissions other than write-access is changing.  Do nothing
> +		 * if the permissions are unchanged or only write-access is
> +		 * being added.  Only update the spte when write-access is being
> +		 * removed.
> +		 */
> +		old_pte_access = kvm_mmu_page_get_access(sp, i);
> +		if (old_pte_access == pte_access ||
> +		    (old_pte_access | ACC_WRITE_MASK) == pte_access)
> +			continue;
> +		if (old_pte_access != (pte_access | ACC_WRITE_MASK)) {
> +			drop_spte(vcpu->kvm, &sp->spt[i]);
> +			flush = true;
> +			continue;
> +		}
> +
>  		/* Update the shadowed access bits in case they changed. */
>  		kvm_mmu_page_set_access(sp, i, pte_access);
>  
> -- 
> 2.19.1.6.gb485710b
>
  

Hello Sean,

On Wed, Dec 14, 2022 at 2:12 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Mon, Dec 12, 2022, Lai Jiangshan wrote:
> > From: Lai Jiangshan <jiangshan.ljs@antgroup.com>
> >
> > Sometimes when the guest updates its pagetable, it adds only new gptes
> > to it without changing any existed one, so there is no point to update
> > the sptes for these existed gptes.
> >
> > Also when the sptes for these unchanged gptes are updated, the AD
> > bits are also removed since make_spte() is called with prefetch=true
> > which might result unneeded TLB flushing.
>
> If either of the proposed changes is kept, please move this to a separate patch.
> Skipping updates for PTEs with the same protections is separate logical change
> from skipping updates when making the SPTE writable.
>
> Actually, can't we just pass @prefetch=false to make_spte()?  FNAME(prefetch_invalid_gpte)
> has already verified the Accessed bit is set in the GPTE, so at least for guest
> correctness there's no need to access-track the SPTE.  Host page aging is already
> fuzzy so I don't think there are problems there.

FNAME(prefetch_invalid_gpte) has already verified the Accessed bit is set
in the GPTE and FNAME(protect_clean_gpte) has already verified the Dirty
bit is set in the GPTE.  These are only for guest AD bits.

And I don't think it is a good idea to pass @prefetch=false to make_spte(),
since the host might have cleared AD bit in the spte for aging or dirty-log,
The AD bits in the spte are better to be kept as before.

Though passing @prefetch=false would not cause any correctness problem
in the view of maintaining guest AD bits.

>
> > Do nothing if the permissions are unchanged or only write-access is
> > being added.
>
> I'm pretty sure skipping the "make writable" case is architecturally wrong.  On a
> #PF, any TLB entries for the faulting virtual address are required to be removed.
> That means KVM _must_ refresh the SPTE if a vCPU takes a !WRITABLE fault on an
> unsync page.  E.g. see kvm_inject_emulated_page_fault().

I might misunderstand what you meant or I failed to connect it with
the SDM properly.

I think there is no #PF here.

And even if the guest is requesting writable, the hypervisor is allowed to
set it non-writable and prepared to handle it in the ensuing write-fault.

Skipping to make it writable is a kind of lazy operation and considered
to be "the hypervisor doesn't grant the writable permission for a period
before next write-fault".

Thanks
Lai
  

On Wed, Dec 14, 2022, Lai Jiangshan wrote:
> Hello Sean,
> 
> On Wed, Dec 14, 2022 at 2:12 AM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Mon, Dec 12, 2022, Lai Jiangshan wrote:
> > > From: Lai Jiangshan <jiangshan.ljs@antgroup.com>
> > >
> > > Sometimes when the guest updates its pagetable, it adds only new gptes
> > > to it without changing any existed one, so there is no point to update
> > > the sptes for these existed gptes.
> > >
> > > Also when the sptes for these unchanged gptes are updated, the AD
> > > bits are also removed since make_spte() is called with prefetch=true
> > > which might result unneeded TLB flushing.
> >
> > If either of the proposed changes is kept, please move this to a separate patch.
> > Skipping updates for PTEs with the same protections is separate logical change
> > from skipping updates when making the SPTE writable.
> >
> > Actually, can't we just pass @prefetch=false to make_spte()?  FNAME(prefetch_invalid_gpte)
> > has already verified the Accessed bit is set in the GPTE, so at least for guest
> > correctness there's no need to access-track the SPTE.  Host page aging is already
> > fuzzy so I don't think there are problems there.
> 
> FNAME(prefetch_invalid_gpte) has already verified the Accessed bit is set
> in the GPTE and FNAME(protect_clean_gpte) has already verified the Dirty
> bit is set in the GPTE.  These are only for guest AD bits.
> 
> And I don't think it is a good idea to pass @prefetch=false to make_spte(),
> since the host might have cleared AD bit in the spte for aging or dirty-log,
> The AD bits in the spte are better to be kept as before.

Drat, I was thinking KVM never flushes when aging SPTEs, but forgot about
clear_flush_young().

Rather than skipping if the Accessed bit is the only thing that's changing, what
about simply preserving the Accessed bit?  And s/prefetch/accessed in make_spte()
so that future changes to make_spte() don't make incorrect assumptions about the
meaning of "prefetch".

Another alternative would be to conditionally preserve the Accessed bit, i.e. clear
it if a flush is needed anyways, but that seems unnecessarily complex.

> Though passing @prefetch=false would not cause any correctness problem
> in the view of maintaining guest AD bits.
> 
> >
> > > Do nothing if the permissions are unchanged or only write-access is
> > > being added.
> >
> > I'm pretty sure skipping the "make writable" case is architecturally wrong.  On a
> > #PF, any TLB entries for the faulting virtual address are required to be removed.
> > That means KVM _must_ refresh the SPTE if a vCPU takes a !WRITABLE fault on an
> > unsync page.  E.g. see kvm_inject_emulated_page_fault().
> 
> I might misunderstand what you meant or I failed to connect it with
> the SDM properly.
> 
> I think there is no #PF here.
> 
> And even if the guest is requesting writable, the hypervisor is allowed to
> set it non-writable and prepared to handle it in the ensuing write-fault.

Yeah, you're right.  The host will see the "spurious" page fault but it will
never get injected into the guest.

> Skipping to make it writable is a kind of lazy operation and considered
> to be "the hypervisor doesn't grant the writable permission for a period
> before next write-fault".

But that raises the question of why?  No TLB flush is needed precisely because any
!WRITABLE fault will be treated as a spurious fault.  The cost of writing the
SPTE is minimal.  So why skip?  Skipping just to reclaim a low SPTE bit doesn't
seem like a good tradeoff, especially without a concrete use case for the SPTE bit.

E.g. on pre-Nehalem Intel CPUs, i.e. CPUs that don't support EPT and thus have
to use shadow paging, the CPU automatically retries accesses after the TLB flush
on permission faults.  The lazy approach might introduce a noticeable performance
regression on such CPUs due to causing more #PF VM-Exits than the current approach.
  

On Wed, Dec 14, 2022 at 2:12 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Mon, Dec 12, 2022, Lai Jiangshan wrote:
> > From: Lai Jiangshan <jiangshan.ljs@antgroup.com>
> >
> > Sometimes when the guest updates its pagetable, it adds only new gptes
> > to it without changing any existed one, so there is no point to update
> > the sptes for these existed gptes.
> >
> > Also when the sptes for these unchanged gptes are updated, the AD
> > bits are also removed since make_spte() is called with prefetch=true
> > which might result unneeded TLB flushing.
>
> If either of the proposed changes is kept, please move this to a separate patch.
> Skipping updates for PTEs with the same protections is separate logical change
> from skipping updates when making the SPTE writable.
>

Did as you suggested:
https://lore.kernel.org/lkml/20230105095848.6061-5-jiangshanlai@gmail.com/