[PATCH] KVM: arm64: Remove redundant check for S2FWB

[Jing Zhang]

Hi Will,

On Mon, Mar 8, 2021 at 10:49 AM Will Deacon <will at kernel.org> wrote:
>
> On Fri, Feb 05, 2021 at 04:44:03AM +0000, Jing Zhang wrote:
> > Remove redundant check for CPU feature S2FWB in dcache flush code
> > to save some CPU cycles for every memslot flush and unmapping.
> > And move the S2FWB check to outer functions to avoid future
> > redundancy and keep consistent with other usage like in
> > access_dcsw and kvm_arch_prepare_memory_region.
> >
> > Signed-off-by: Jing Zhang <jingzhangos at google.com>
> > ---
> >  arch/arm64/kvm/hyp/pgtable.c | 9 ++-------
> >  arch/arm64/kvm/mmu.c         | 3 ++-
> >  2 files changed, 4 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> > index bdf8e55ed308..afd57564b1cb 100644
> > --- a/arch/arm64/kvm/hyp/pgtable.c
> > +++ b/arch/arm64/kvm/hyp/pgtable.c
> > @@ -642,9 +642,6 @@ int kvm_pgtable_stage2_map(struct kvm_pgtable *pgt, u64 addr, u64 size,
> >
> >  static void stage2_flush_dcache(void *addr, u64 size)
> >  {
> > -     if (cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))
> > -             return;
> > -
> >       __flush_dcache_area(addr, size);
> >  }
> >
> > @@ -670,7 +667,8 @@ static int stage2_unmap_walker(u64 addr, u64 end, u32 level, kvm_pte_t *ptep,
> >
> >               if (page_count(virt_to_page(childp)) != 1)
> >                       return 0;
> > -     } else if (stage2_pte_cacheable(pte)) {
> > +     } else if (stage2_pte_cacheable(pte) &&
> > +                     !cpus_have_const_cap(ARM64_HAS_STAGE2_FWB)) {
> >               need_flush = true;
> >       }
> >
> > @@ -846,9 +844,6 @@ int kvm_pgtable_stage2_flush(struct kvm_pgtable *pgt, u64 addr, u64 size)
> >               .flags  = KVM_PGTABLE_WALK_LEAF,
> >       };
> >
> > -     if (cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))
> > -             return 0;
> > -
> >       return kvm_pgtable_walk(pgt, addr, size, &walker);
> >  }
>
> I think we should leave pgtable.c as it is: there's no benefit from this
> change on the unmap path, and the other path involves the case where the
> caller has asked for a flush and we can elide it.
Agreed.
>
> > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> > index 7d2257cc5438..53130ed23304 100644
> > --- a/arch/arm64/kvm/mmu.c
> > +++ b/arch/arm64/kvm/mmu.c
> > @@ -1458,7 +1458,8 @@ void kvm_toggle_cache(struct kvm_vcpu *vcpu, bool was_enabled)
> >        * If switching it off, need to clean the caches.
> >        * Clean + invalidate does the trick always.
> >        */
> > -     if (now_enabled != was_enabled)
> > +     if (now_enabled != was_enabled &&
> > +                     !cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))
> >               stage2_flush_vm(vcpu->kvm);
>
> This change looks fine, but I don't grok the justification in your follow-up
> email. You say:
>
>   | The saving is from the code path of memslot flush. The S2FWB check was
>   | in stage2_flush_dcache, in which case, for a memslot flush, the check
>   | was done for every page.
>
> but I don't see where this is called for every page. It looks to me like it's
> called for every pgd in the range, which is a very different kettle of frogs.
>
> What am I missing?
You are right. It is called for every pgd in the range instead of for
every page.
>
> Will

Thanks,
Jing