On Mon, Oct 23 2017 at 1:05:23 pm BST, Will Deacon <will.deacon@xxxxxxx> wrote: > Hi Marc, > > On Fri, Oct 20, 2017 at 04:48:57PM +0100, Marc Zyngier wrote: >> We currently tightly couple dcache clean with icache invalidation, >> but KVM could do without the initial flush to PoU, as we've >> already flushed things to PoC. >> >> Let's introduce invalidate_icache_range which is limited to >> invalidating the icache from the linear mapping (and thus >> has none of the userspace fault handling complexity), and >> wire it in KVM instead of flush_icache_range. >> >> Reviewed-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx> >> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx> >> --- >> arch/arm64/include/asm/assembler.h | 26 ++++++++++++++++++++++++++ >> arch/arm64/include/asm/cacheflush.h | 8 ++++++++ >> arch/arm64/include/asm/kvm_mmu.h | 4 ++-- >> arch/arm64/mm/cache.S | 26 ++++++++++++++++---------- >> 4 files changed, 52 insertions(+), 12 deletions(-) >> >> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h >> index d58a6253c6ab..efdbecc8f2ef 100644 >> --- a/arch/arm64/include/asm/assembler.h >> +++ b/arch/arm64/include/asm/assembler.h >> @@ -374,6 +374,32 @@ alternative_endif >> dsb \domain >> .endm >> >> +/* >> + * Macro to perform an instruction cache maintenance for the interval >> + * [start, end) >> + * >> + * start, end: virtual addresses describing the region >> + * label: A label to branch to on user fault, none if >> + * only used on a kernel address >> + * Corrupts: tmp1, tmp2 >> + */ >> + .macro invalidate_icache_by_line start, end, tmp1, tmp2, label=none >> + icache_line_size \tmp1, \tmp2 >> + sub \tmp2, \tmp1, #1 >> + bic \tmp2, \start, \tmp2 >> +9997: >> + .if (\label == none) >> + ic ivau, \tmp2 // invalidate I line PoU >> + .else >> +USER(\label, ic ivau, \tmp2) // invalidate I line PoU >> + .endif >> + add \tmp2, \tmp2, \tmp1 >> + cmp \tmp2, \end >> + b.lo 9997b >> + dsb ish >> + isb >> + .endm > > Code looks fine to me, but I'd like to drop the \label == none case. See > below. > >> /* >> * reset_pmuserenr_el0 - reset PMUSERENR_EL0 if PMUv3 present >> */ >> diff --git a/arch/arm64/include/asm/cacheflush.h b/arch/arm64/include/asm/cacheflush.h >> index 76d1cc85d5b1..ad56406944c6 100644 >> --- a/arch/arm64/include/asm/cacheflush.h >> +++ b/arch/arm64/include/asm/cacheflush.h >> @@ -52,6 +52,13 @@ >> * - start - virtual start address >> * - end - virtual end address >> * >> + * invalidate_icache_range(start, end) >> + * >> + * Invalidate the I-cache in the region described by start, end. >> + * Linear mapping only! >> + * - start - virtual start address >> + * - end - virtual end address >> + * >> * __flush_cache_user_range(start, end) >> * >> * Ensure coherency between the I-cache and the D-cache in the >> @@ -66,6 +73,7 @@ >> * - size - region size >> */ >> extern void flush_icache_range(unsigned long start, unsigned long end); >> +extern void invalidate_icache_range(unsigned long start, unsigned long end); >> extern void __flush_dcache_area(void *addr, size_t len); >> extern void __inval_dcache_area(void *addr, size_t len); >> extern void __clean_dcache_area_poc(void *addr, size_t len); >> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h >> index 8034b96fb3a4..56b3e03c85e7 100644 >> --- a/arch/arm64/include/asm/kvm_mmu.h >> +++ b/arch/arm64/include/asm/kvm_mmu.h >> @@ -250,8 +250,8 @@ static inline void __invalidate_icache_guest_page(struct kvm_vcpu *vcpu, >> /* PIPT or VPIPT at EL2 (see comment in __kvm_tlb_flush_vmid_ipa) */ >> void *va = page_address(pfn_to_page(pfn)); >> >> - flush_icache_range((unsigned long)va, >> - (unsigned long)va + size); >> + invalidate_icache_range((unsigned long)va, >> + (unsigned long)va + size); >> } >> } >> >> diff --git a/arch/arm64/mm/cache.S b/arch/arm64/mm/cache.S >> index 7f1dbe962cf5..8e71d237f85e 100644 >> --- a/arch/arm64/mm/cache.S >> +++ b/arch/arm64/mm/cache.S >> @@ -60,16 +60,7 @@ user_alt 9f, "dc cvau, x4", "dc civac, x4", ARM64_WORKAROUND_CLEAN_CACHE >> b.lo 1b >> dsb ish >> >> - icache_line_size x2, x3 >> - sub x3, x2, #1 >> - bic x4, x0, x3 >> -1: >> -USER(9f, ic ivau, x4 ) // invalidate I line PoU >> - add x4, x4, x2 >> - cmp x4, x1 >> - b.lo 1b >> - dsb ish >> - isb >> + invalidate_icache_by_line x0, x1, x2, x3, 9f >> mov x0, #0 >> 1: >> uaccess_ttbr0_disable x1 >> @@ -80,6 +71,21 @@ USER(9f, ic ivau, x4 ) // invalidate I line PoU >> ENDPROC(flush_icache_range) >> ENDPROC(__flush_cache_user_range) >> >> +/* >> + * invalidate_icache_range(start,end) >> + * >> + * Ensure that the I cache is invalid within specified region. This >> + * assumes that this is done on the linear mapping. Do not use it >> + * on a userspace range, as this may fault horribly. > > I'm still not sure why we're not hooking up the user fault handling here. > In the worst case, we'd end up returning -EFAULT if we got passed a > faulting user address and there's already precedence for this with e.g. > flush_icache_range. Fair enough. Confusingly, flush_icache_range is declared as returning void... I'll cook a separate patch addressing this. How about the following on top of that patch: diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h index efdbecc8f2ef..27093cf2b91f 100644 --- a/arch/arm64/include/asm/assembler.h +++ b/arch/arm64/include/asm/assembler.h @@ -379,20 +379,15 @@ alternative_endif * [start, end) * * start, end: virtual addresses describing the region - * label: A label to branch to on user fault, none if - * only used on a kernel address + * label: A label to branch to on user fault. * Corrupts: tmp1, tmp2 */ - .macro invalidate_icache_by_line start, end, tmp1, tmp2, label=none + .macro invalidate_icache_by_line start, end, tmp1, tmp2, label icache_line_size \tmp1, \tmp2 sub \tmp2, \tmp1, #1 bic \tmp2, \start, \tmp2 9997: - .if (\label == none) - ic ivau, \tmp2 // invalidate I line PoU - .else USER(\label, ic ivau, \tmp2) // invalidate I line PoU - .endif add \tmp2, \tmp2, \tmp1 cmp \tmp2, \end b.lo 9997b diff --git a/arch/arm64/include/asm/cacheflush.h b/arch/arm64/include/asm/cacheflush.h index ad56406944c6..e671e73c6453 100644 --- a/arch/arm64/include/asm/cacheflush.h +++ b/arch/arm64/include/asm/cacheflush.h @@ -55,7 +55,6 @@ * invalidate_icache_range(start, end) * * Invalidate the I-cache in the region described by start, end. - * Linear mapping only! * - start - virtual start address * - end - virtual end address * @@ -73,7 +72,7 @@ * - size - region size */ extern void flush_icache_range(unsigned long start, unsigned long end); -extern void invalidate_icache_range(unsigned long start, unsigned long end); +extern int invalidate_icache_range(unsigned long start, unsigned long end); extern void __flush_dcache_area(void *addr, size_t len); extern void __inval_dcache_area(void *addr, size_t len); extern void __clean_dcache_area_poc(void *addr, size_t len); diff --git a/arch/arm64/mm/cache.S b/arch/arm64/mm/cache.S index 8e71d237f85e..a89f23610b7e 100644 --- a/arch/arm64/mm/cache.S +++ b/arch/arm64/mm/cache.S @@ -74,15 +74,17 @@ ENDPROC(__flush_cache_user_range) /* * invalidate_icache_range(start,end) * - * Ensure that the I cache is invalid within specified region. This - * assumes that this is done on the linear mapping. Do not use it - * on a userspace range, as this may fault horribly. + * Ensure that the I cache is invalid within specified region. * * - start - virtual start address of region * - end - virtual end address of region */ ENTRY(invalidate_icache_range) - invalidate_icache_by_line x0, x1, x2, x3 + invalidate_icache_by_line x0, x1, x2, x3, 1f + mov x0, xzr + ret +1: + mov x0, #-EFAULT ret ENDPROC(invalidate_icache_range) Does this address your concerns? Thanks, M. -- Jazz is not dead. It just smells funny.
