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.
> 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?
Will
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
