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

[Will Deacon <will@kernel.org>]

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@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.

> 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel