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