On Mon, Oct 23 2017 at 1:05:23 pm BST, Will Deacon <[email protected]> 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 <[email protected]>
>> Signed-off-by: Marc Zyngier <[email protected]>
>> ---
>> 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.
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
