Re: [PATCH v5 2/6] KVM: arm64: Move D-cache flush to the fault handlers

[Marc Zyngier <maz@kernel.org>]

On Thu, 15 Apr 2021 12:50:28 +0100,
Yanan Wang <wangyanan55@huawei.com> wrote:
> 
> We currently uniformly permorm CMOs of D-cache and I-cache in function
> user_mem_abort before calling the fault handlers. If we get concurrent
> guest faults(e.g. translation faults, permission faults) or some really
> unnecessary guest faults caused by BBM, CMOs for the first vcpu are
> necessary while the others later are not.
> 
> By moving CMOs to the fault handlers, we can easily identify conditions
> where they are really needed and avoid the unnecessary ones. As it's a
> time consuming process to perform CMOs especially when flushing a block
> range, so this solution reduces much load of kvm and improve efficiency
> of the page table code.
> 
> This patch only moves clean of D-cache to the map path, and drop the
> original APIs in mmu.c/mmu.h for D-cache maintenance by using what we
> already have in pgtable.c. Change about the I-side will come from a
> later patch.

But this means that until patch #5, this is broken (invalidation on
the i-side will happen before the clean to PoU on the d-side). You
need to keep the series bisectable and not break things in the middle.
It would be OK to have two set of D-cache CMOs in the interval, for
example.

> 
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  arch/arm64/include/asm/kvm_mmu.h | 16 ----------------
>  arch/arm64/kvm/hyp/pgtable.c     | 20 ++++++++++++++------
>  arch/arm64/kvm/mmu.c             | 14 +++-----------
>  3 files changed, 17 insertions(+), 33 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> index 25ed956f9af1..e9b163c5f023 100644
> --- a/arch/arm64/include/asm/kvm_mmu.h
> +++ b/arch/arm64/include/asm/kvm_mmu.h
> @@ -187,22 +187,6 @@ static inline bool vcpu_has_cache_enabled(struct kvm_vcpu *vcpu)
>  	return (vcpu_read_sys_reg(vcpu, SCTLR_EL1) & 0b101) == 0b101;
>  }
>  
> -static inline void __clean_dcache_guest_page(kvm_pfn_t pfn, unsigned long size)
> -{
> -	void *va = page_address(pfn_to_page(pfn));
> -
> -	/*
> -	 * With FWB, we ensure that the guest always accesses memory using
> -	 * cacheable attributes, and we don't have to clean to PoC when
> -	 * faulting in pages. Furthermore, FWB implies IDC, so cleaning to
> -	 * PoU is not required either in this case.
> -	 */
> -	if (cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))
> -		return;
> -
> -	kvm_flush_dcache_to_poc(va, size);
> -}
> -
>  static inline void __invalidate_icache_guest_page(kvm_pfn_t pfn,
>  						  unsigned long size)
>  {
> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> index c37c1dc4feaf..e3606c9dcec7 100644
> --- a/arch/arm64/kvm/hyp/pgtable.c
> +++ b/arch/arm64/kvm/hyp/pgtable.c
> @@ -562,6 +562,12 @@ static bool stage2_pte_is_counted(kvm_pte_t pte)
>  	return !!pte;
>  }
>  
> +static bool stage2_pte_cacheable(struct kvm_pgtable *pgt, kvm_pte_t pte)
> +{
> +	u64 memattr = pte & KVM_PTE_LEAF_ATTR_LO_S2_MEMATTR;
> +	return memattr == KVM_S2_MEMATTR(pgt, NORMAL);
> +}
> +
>  static void stage2_put_pte(kvm_pte_t *ptep, struct kvm_s2_mmu *mmu, u64 addr,
>  			   u32 level, struct kvm_pgtable_mm_ops *mm_ops)
>  {
> @@ -583,6 +589,7 @@ static int stage2_map_walker_try_leaf(u64 addr, u64 end, u32 level,
>  {
>  	kvm_pte_t new, old = *ptep;
>  	u64 granule = kvm_granule_size(level), phys = data->phys;
> +	struct kvm_pgtable *pgt = data->mmu->pgt;
>  	struct kvm_pgtable_mm_ops *mm_ops = data->mm_ops;
>  
>  	if (!kvm_block_mapping_supported(addr, end, phys, level))
> @@ -606,6 +613,13 @@ static int stage2_map_walker_try_leaf(u64 addr, u64 end, u32 level,
>  		stage2_put_pte(ptep, data->mmu, addr, level, mm_ops);
>  	}
>  
> +	/* Perform CMOs before installation of the guest stage-2 PTE */
> +	if (pgt->flags & KVM_PGTABLE_S2_GUEST) {
> +		if (stage2_pte_cacheable(pgt, new) && !stage2_has_fwb(pgt))
> +			__flush_dcache_area(mm_ops->phys_to_virt(phys),
> +					    granule);
> +	}

Rather than this, why not provide new callbacks in mm_ops, even if we
have to provide one that is specific to guests (and let the protected
stuff do its own thing)?

One thing I really dislike though is that the page-table code is
starting to be littered with things that are not directly related to
page tables. We are re-creating the user_mem_abort() mess in a
different place.

> +
>  	smp_store_release(ptep, new);
>  	if (stage2_pte_is_counted(new))
>  		mm_ops->get_page(ptep);
> @@ -798,12 +812,6 @@ int kvm_pgtable_stage2_set_owner(struct kvm_pgtable *pgt, u64 addr, u64 size,
>  	return ret;
>  }
>  
> -static bool stage2_pte_cacheable(struct kvm_pgtable *pgt, kvm_pte_t pte)
> -{
> -	u64 memattr = pte & KVM_PTE_LEAF_ATTR_LO_S2_MEMATTR;
> -	return memattr == KVM_S2_MEMATTR(pgt, NORMAL);
> -}
> -
>  static int stage2_unmap_walker(u64 addr, u64 end, u32 level, kvm_pte_t *ptep,
>  			       enum kvm_pgtable_walk_flags flag,
>  			       void * const arg)
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 2cfcfc5f4e4e..86f7dd1c234f 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -694,11 +694,6 @@ void kvm_arch_mmu_enable_log_dirty_pt_masked(struct kvm *kvm,
>  	kvm_mmu_write_protect_pt_masked(kvm, slot, gfn_offset, mask);
>  }
>  
> -static void clean_dcache_guest_page(kvm_pfn_t pfn, unsigned long size)
> -{
> -	__clean_dcache_guest_page(pfn, size);
> -}
> -
>  static void invalidate_icache_guest_page(kvm_pfn_t pfn, unsigned long size)
>  {
>  	__invalidate_icache_guest_page(pfn, size);
> @@ -972,9 +967,6 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>  	if (writable)
>  		prot |= KVM_PGTABLE_PROT_W;
>  
> -	if (fault_status != FSC_PERM && !device)
> -		clean_dcache_guest_page(pfn, vma_pagesize);
> -
>  	if (exec_fault) {
>  		prot |= KVM_PGTABLE_PROT_X;
>  		invalidate_icache_guest_page(pfn, vma_pagesize);
> @@ -1234,10 +1226,10 @@ int kvm_set_spte_hva(struct kvm *kvm, unsigned long hva, pte_t pte)
>  	trace_kvm_set_spte_hva(hva);
>  
>  	/*
> -	 * We've moved a page around, probably through CoW, so let's treat it
> -	 * just like a translation fault and clean the cache to the PoC.
> +	 * We've moved a page around, probably through CoW, so let's treat
> +	 * it just like a translation fault and the map handler will clean
> +	 * the cache to the PoC.
>  	 */
> -	clean_dcache_guest_page(pfn, PAGE_SIZE);
>  	handle_hva_to_gpa(kvm, hva, end, &kvm_set_spte_handler, &pfn);
>  	return 0;
>  }

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

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