Hi Will,
On Mon, Mar 8, 2021 at 10:49 AM Will Deacon <will@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
