[PATCH] KVM: x86/mmu: Skip !MMU-present SPTEs when removing SP in exclusive mode

[PATCH] KVM: x86/mmu: Skip !MMU-present SPTEs when removing SP in exclusive mode

[Sean Christopherson]

If mmu_lock is held for write, don't bother setting !PRESENT SPTEs to
REMOVED_SPTE when recursively zapping SPTEs as part of shadow page
removal.  The concurrent write protections provided by REMOVED_SPTE are
not needed, there are no backing page side effects to record, and MMIO
SPTEs can be left as is since they are protected by the memslot
generation, not by ensuring that the MMIO SPTE is unreachable (which
is racy with respect to lockless walks regardless of zapping behavior).

Skipping !PRESENT drastically reduces the number of updates needed to
tear down sparsely populated MMUs, e.g. when tearing down a 6gb VM that
didn't touch much memory, 6929/7168 (~96.6%) of SPTEs were '0' and could
be skipped.

Avoiding the write itself is likely close to a wash, but avoiding
__handle_changed_spte() is a clear-cut win as that involves saving and
restoring all non-volatile GPRs (it's a subtly big function), as well as
several conditional branches before bailing out.

Cc: Ben Gardon <bgardon@google.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/mmu/tdp_mmu.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index 50ef757c5586..f0c99fa04ef2 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -323,7 +323,18 @@ static void handle_removed_tdp_mmu_page(struct kvm *kvm, u64 *pt,
 				cpu_relax();
 			}
 		} else {
+			/*
+			 * If the SPTE is not MMU-present, there is no backing
+			 * page associated with the SPTE and so no side effects
+			 * that need to be recorded, and exclusive ownership of
+			 * mmu_lock ensures the SPTE can't be made present.
+			 * Note, zapping MMIO SPTEs is also unnecessary as they
+			 * are guarded by the memslots generation, not by being
+			 * unreachable.
+			 */
 			old_child_spte = READ_ONCE(*sptep);
+			if (!is_shadow_present_pte(old_child_spte))
+				continue;
 
 			/*
 			 * Marking the SPTE as a removed SPTE is not
-- 
2.30.1.766.gb4fecdf3b7-goog

Re: [PATCH] KVM: x86/mmu: Skip !MMU-present SPTEs when removing SP in exclusive mode

[Paolo Bonzini]

On 10/03/21 01:30, Sean Christopherson wrote:
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index 50ef757c5586..f0c99fa04ef2 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -323,7 +323,18 @@ static void handle_removed_tdp_mmu_page(struct kvm *kvm, u64 *pt,
>   				cpu_relax();
>   			}
>   		} else {
> +			/*
> +			 * If the SPTE is not MMU-present, there is no backing
> +			 * page associated with the SPTE and so no side effects
> +			 * that need to be recorded, and exclusive ownership of
> +			 * mmu_lock ensures the SPTE can't be made present.
> +			 * Note, zapping MMIO SPTEs is also unnecessary as they
> +			 * are guarded by the memslots generation, not by being
> +			 * unreachable.
> +			 */
>   			old_child_spte = READ_ONCE(*sptep);
> +			if (!is_shadow_present_pte(old_child_spte))
> +				continue;
>   
>   			/*
>   			 * Marking the SPTE as a removed SPTE is not

Ben, do you plan to make this path take mmu_lock for read?  If so, this 
wouldn't be too useful IIUC.

Paolo

Re: [PATCH] KVM: x86/mmu: Skip !MMU-present SPTEs when removing SP in exclusive mode

[Sean Christopherson]

On Wed, Mar 10, 2021, Paolo Bonzini wrote:
> On 10/03/21 01:30, Sean Christopherson wrote:
> > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> > index 50ef757c5586..f0c99fa04ef2 100644
> > --- a/arch/x86/kvm/mmu/tdp_mmu.c
> > +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> > @@ -323,7 +323,18 @@ static void handle_removed_tdp_mmu_page(struct kvm *kvm, u64 *pt,
> >   				cpu_relax();
> >   			}
> >   		} else {
> > +			/*
> > +			 * If the SPTE is not MMU-present, there is no backing
> > +			 * page associated with the SPTE and so no side effects
> > +			 * that need to be recorded, and exclusive ownership of
> > +			 * mmu_lock ensures the SPTE can't be made present.
> > +			 * Note, zapping MMIO SPTEs is also unnecessary as they
> > +			 * are guarded by the memslots generation, not by being
> > +			 * unreachable.
> > +			 */
> >   			old_child_spte = READ_ONCE(*sptep);
> > +			if (!is_shadow_present_pte(old_child_spte))
> > +				continue;
> >   			/*
> >   			 * Marking the SPTE as a removed SPTE is not
> 
> Ben, do you plan to make this path take mmu_lock for read?  If so, this
> wouldn't be too useful IIUC.

I can see kvm_mmu_zap_all_fast()->kvm_tdp_mmu_zap_all() moving to a shared-mode
flow, but I don't think we'll ever want to move away from exclusive-mode zapping
for kvm_arch_flush_shadow_all()->kvm_mmu_zap_all()->kvm_tdp_mmu_zap_all().  In
that case, the VM is dead or dying; freeing memory should be done as quickly as
possible.

Re: [PATCH] KVM: x86/mmu: Skip !MMU-present SPTEs when removing SP in exclusive mode

[Ben Gardon]

On Wed, Mar 10, 2021 at 1:14 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Wed, Mar 10, 2021, Paolo Bonzini wrote:
> > On 10/03/21 01:30, Sean Christopherson wrote:
> > > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> > > index 50ef757c5586..f0c99fa04ef2 100644
> > > --- a/arch/x86/kvm/mmu/tdp_mmu.c
> > > +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> > > @@ -323,7 +323,18 @@ static void handle_removed_tdp_mmu_page(struct kvm *kvm, u64 *pt,
> > >                             cpu_relax();
> > >                     }
> > >             } else {
> > > +                   /*
> > > +                    * If the SPTE is not MMU-present, there is no backing
> > > +                    * page associated with the SPTE and so no side effects
> > > +                    * that need to be recorded, and exclusive ownership of
> > > +                    * mmu_lock ensures the SPTE can't be made present.
> > > +                    * Note, zapping MMIO SPTEs is also unnecessary as they
> > > +                    * are guarded by the memslots generation, not by being
> > > +                    * unreachable.
> > > +                    */
> > >                     old_child_spte = READ_ONCE(*sptep);
> > > +                   if (!is_shadow_present_pte(old_child_spte))
> > > +                           continue;
> > >                     /*
> > >                      * Marking the SPTE as a removed SPTE is not
> >
> > Ben, do you plan to make this path take mmu_lock for read?  If so, this
> > wouldn't be too useful IIUC.
>
> I can see kvm_mmu_zap_all_fast()->kvm_tdp_mmu_zap_all() moving to a shared-mode
> flow, but I don't think we'll ever want to move away from exclusive-mode zapping
> for kvm_arch_flush_shadow_all()->kvm_mmu_zap_all()->kvm_tdp_mmu_zap_all().  In
> that case, the VM is dead or dying; freeing memory should be done as quickly as
> possible.

Yeah, as Sean said, zapping under the MMU lock in write mode probably
shouldn't go away, even if we find we're able to do it in read mode in
some flows.

This optimization also makes me think we could also skip the
__handle_changed_spte call in the read mode case if the SPTE change
was !PRESENT -> REMOVED.

Re: [PATCH] KVM: x86/mmu: Skip !MMU-present SPTEs when removing SP in exclusive mode

[Paolo Bonzini]

On 10/03/21 23:24, Ben Gardon wrote:
> On Wed, Mar 10, 2021 at 1:14 PM Sean Christopherson <seanjc@google.com> wrote:
>>
>> On Wed, Mar 10, 2021, Paolo Bonzini wrote:
>>> On 10/03/21 01:30, Sean Christopherson wrote:
>>>> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
>>>> index 50ef757c5586..f0c99fa04ef2 100644
>>>> --- a/arch/x86/kvm/mmu/tdp_mmu.c
>>>> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
>>>> @@ -323,7 +323,18 @@ static void handle_removed_tdp_mmu_page(struct kvm *kvm, u64 *pt,
>>>>                              cpu_relax();
>>>>                      }
>>>>              } else {
>>>> +                   /*
>>>> +                    * If the SPTE is not MMU-present, there is no backing
>>>> +                    * page associated with the SPTE and so no side effects
>>>> +                    * that need to be recorded, and exclusive ownership of
>>>> +                    * mmu_lock ensures the SPTE can't be made present.
>>>> +                    * Note, zapping MMIO SPTEs is also unnecessary as they
>>>> +                    * are guarded by the memslots generation, not by being
>>>> +                    * unreachable.
>>>> +                    */
>>>>                      old_child_spte = READ_ONCE(*sptep);
>>>> +                   if (!is_shadow_present_pte(old_child_spte))
>>>> +                           continue;
>>>>                      /*
>>>>                       * Marking the SPTE as a removed SPTE is not
> 
> This optimization also makes me think we could also skip the
> __handle_changed_spte call in the read mode case if the SPTE change
> was !PRESENT -> REMOVED.
> 
Yes, I think so.  It should be a separate patch anyway, so I've queued 
this one.

Paolo