Re: [PATCH 10/10] arm: KVM: Use common implementation for all flushes to PoC

[Christoffer Dall <cdall@linaro.org>]

On Tue, Oct 17, 2017 at 01:40:00PM +0100, Marc Zyngier wrote:
> On 16/10/17 21:06, Christoffer Dall wrote:
> > On Mon, Oct 09, 2017 at 04:20:32PM +0100, Marc Zyngier wrote:
> >> We currently have no less than three implementations for the
> >> "flush to PoC" code. Let standardize on a single one. This
> >> requires a bit of unpleasant moving around, and relies on
> >> __kvm_flush_dcache_pte and co being #defines so that they can
> >> call into coherent_dcache_guest_page...
> >>
> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> >> ---
> >>  arch/arm/include/asm/kvm_mmu.h | 28 ++++------------------------
> >>  virt/kvm/arm/mmu.c             | 20 ++++++++++----------
> >>  2 files changed, 14 insertions(+), 34 deletions(-)
> >>
> >> diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
> >> index 5f1ac88a5951..011b0db85c02 100644
> >> --- a/arch/arm/include/asm/kvm_mmu.h
> >> +++ b/arch/arm/include/asm/kvm_mmu.h
> >> @@ -235,31 +235,11 @@ static inline void __coherent_icache_guest_page(kvm_pfn_t pfn,
> >>  	}
> >>  }
> >>  
> >> -static inline void __kvm_flush_dcache_pte(pte_t pte)
> >> -{
> >> -	void *va = kmap_atomic(pte_page(pte));
> >> -
> >> -	kvm_flush_dcache_to_poc(va, PAGE_SIZE);
> >> -
> >> -	kunmap_atomic(va);
> >> -}
> >> -
> >> -static inline void __kvm_flush_dcache_pmd(pmd_t pmd)
> >> -{
> >> -	unsigned long size = PMD_SIZE;
> >> -	kvm_pfn_t pfn = pmd_pfn(pmd);
> >> -
> >> -	while (size) {
> >> -		void *va = kmap_atomic_pfn(pfn);
> >> +#define __kvm_flush_dcache_pte(p)				\
> >> +	coherent_dcache_guest_page(pte_pfn((p)), PAGE_SIZE)
> >>  
> >> -		kvm_flush_dcache_to_poc(va, PAGE_SIZE);
> >> -
> >> -		pfn++;
> >> -		size -= PAGE_SIZE;
> >> -
> >> -		kunmap_atomic(va);
> >> -	}
> >> -}
> >> +#define __kvm_flush_dcache_pmd(p)				\
> >> +	coherent_dcache_guest_page(pmd_pfn((p)), PMD_SIZE)
> > 
> > Why can't these just be static inlines which call
> > __coherent_dcache_guest_page already in the header file directly?
> 
> Because if we do that, we get a significant code expansion in the
> resulting binary (all the call sites end up having a copy of that function.
> 
> > I'm really not too crazy about these #defines.
> 
> Neither am I. But actually, this patch is completely wrong. Using the
> same functions as the guest cleaning doesn't provide the guarantees
> documented next to unmap_stage2_ptes, as we need a clean+invalidate, not
> just a clean.
> 
> I'll rework this patch (or just drop it).
> 
> > In fact, why do we need the coherent_Xcache_guest_page static
> > indirection functions in mmu.c in the first place?
> 
> Code expansion. That's the only reason.
> 
Then maybe a reworked patch needs a function defined in some
arch-specific object file that we can just call.  The functions don't
look that complicated to me, but I suppose if they inline the things
they call, it could become a bit hairy.

Thanks,
-Christoffer