arm64: alternative:flush cache with unpatched code

In the current implementation, __apply_alternatives patches flush_icache_range and then executes it without invalidating the icache. Thus, icache can contain some of the old instructions for flush_icache_range. This can cause unpredictable behavior as during execution we can get a mix of old and new instructions for flush_icache_range.

This patch :
1. Adds a new function flush_cache_kernel_range for flushing kernel memory range. This function uses non hot-patched code and can be safely used to flush cache during code patching.

2. Modifies __apply_alternatives so that it uses flush_cache_kernel_range to flush the cache range after patching code.

---
 arch/arm64/kernel/alternative.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

index 5c4bce4ac381..e93cfd26a314 100644
@@ -122,6 +122,33 @@ static void patch_alternative(struct alt_instr *alt,
 	}
 }
 
+/* This is used for flushing kernel memory range after
+ * __apply_alternatives has patched kernel code
+ */
+static void flush_cache_kernel_range(void *start, void *end)
+{
+	u64 d_start, i_start, d_size, i_size;
+
+	/* use sanitized value of ctr_el0 rather than raw value from CPU */
+	d_size = 4 << ((arm64_ftr_reg_ctrel0.sys_val >> 0x10) & 0xF); /* bytes */
+	i_size = 4 << (arm64_ftr_reg_ctrel0.sys_val & 0xF); /* bytes */
+
+	d_start = (u64)start & ~(d_size - 1);
+	while (d_start <= (u64)end) {
+		asm volatile("dc civac, %0" : : "r" (d_start));
+		d_start += d_size;
+	}
+	dsb(ish);
+
+	i_start = (u64)start & ~(i_size - 1);
+	while (i_start <= (u64)end) {
+		asm volatile("ic ivau, %0" : : "r" (i_start));
+		i_start += i_size;
+	}
+	dsb(ish);
+	isb();
+}
+
 static void __apply_alternatives(void *alt_region, bool use_linear_alias)
 {
 	struct alt_instr *alt;

@@ -155,8 +182,8 @@ static void __apply_alternatives(void *alt_region, bool use_linear_alias)
 
 		alt_cb(alt, origptr, updptr, nr_inst);
 
-		flush_icache_range((uintptr_t)origptr,
-				   (uintptr_t)(origptr + nr_inst));
+		flush_cache_kernel_range((void *)origptr,
+				(void *)(origptr + nr_inst));
 	}
 }
 

2.1.4

Hi Rohit,

Please keep me on cc for future versions of this patch. Comments inline.

Will, thanks for the comments.
I will push a new patch set.
We want to avoid using __flush_icache_all as much as possible and hence trying to flush cache by range.

Thanks
Rohit
________________________________________
From: Will Deacon <[email protected]>
Sent: Wednesday, May 30, 2018 2:00 AM
To: Rohit Khanna
Cc: catalin.marinas at arm.com; robin.murphy at arm.com; mark.rutland at arm.com; Suzuki.Poulose at arm.com; Bo Yan; Alexander Van Brunt; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH] arm64: alternative:flush cache with unpatched code

Hi Rohit,

Please keep me on cc for future versions of this patch. Comments inline.

Why? We're going to do it once at boot and once on each module load, so I can't see it being a performance concern.

Will

__flush_icache_all all cache is slow in large systems. It flushes the instruction caches cache in the inner-shareable domain. That includes other NUMA nodes in multi-socket systems. The CPU issuing the invalidate has to wait for all of the other CPUs to complete the invalidate instruction. The remote CPU's responding to the request all need to slow down.

In contrast, flushes by range can be checked in a snoop filter to see if the addresses are relevant to the CPU. So, it scales to systems with more than two clusters much better.

The flush ignores the VMID. So, one guest OS can slow down others. That creates a big covert channel between guests unless the hypervisor traps and emulates it by invalidating an entire VMID. By flushing by VA range, the hardware only flushes lines associated with the VMID, ASID, and VA associated with the line.

Selfishly, NVIDIA's Denver CPU's are even more sensitive because the optimized code stored in DRAM is effectively a very large (on the order of 64 MB) instruction cache. "ic ialluis" can result in the entire optimization cache for all guests to be invalidated.

I am aware that the arguments I made apply to TLB invalidates and the other places that Linux calls __flush_icache_all. But, that doesn't mean I don't like those either. For now, I just don't want more calls to __flush_icache_all.

Alex VB
________________________________________
From: Will Deacon <[email protected]>
Sent: Monday, June 4, 2018 2:16 AM
To: Rohit Khanna
Cc: catalin.marinas at arm.com; robin.murphy at arm.com; mark.rutland at arm.com; Suzuki.Poulose at arm.com; Bo Yan; Alexander Van Brunt; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH] arm64: alternative:flush cache with unpatched code

Why? We're going to do it once at boot and once on each module load, so I can't see it being a performance concern.

Will

Hi Alex,

Sure, but there are only two cases to consider here:

1. Boot. This happens once, and we end up putting *all* secondary cores into a tight loop anyway, so I don't see that the performance of __flush_icache_all is relevant

2. On module load. This happens right before we start remapping the module areas and sending out TLB invalidation so, again, not sure I see why this is such a big deal. In fact, it looks like the core module loading code does invalidation by range, so if we remove it from __apply_alternatives then we can avoid doing it twice.

In other words, something like the diff below.

Will

--->8

index a91933b1e2e6..934f99215bd4 100644
@@ -28,7 +28,12 @@ typedef void (*alternative_cb_t)(struct alt_instr *alt,
 				 __le32 *origptr, __le32 *updptr, int nr_inst);
 
 void __init apply_alternatives_all(void);
-void apply_alternatives(void *start, size_t length);
+
+#ifdef CONFIG_MODULES
+void apply_alternatives_module(void *start, size_t length);
+#else
+void apply_alternatives_module(void *start, size_t length) { }
+#endif
 
 #define ALTINSTR_ENTRY(feature,cb)					      \
 	" .word 661b - .\n"				/* label           */ \

index 9bbffc7a301f..627d62ff2ddd 100644
@@ -21,6 +21,7 @@
 #define CTR_L1IP_SHIFT		14
 #define CTR_L1IP_MASK		3
 #define CTR_DMINLINE_SHIFT	16
+#define CTR_IMINLINE_SHIFT	0
 #define CTR_ERG_SHIFT		20
 #define CTR_CWG_SHIFT		24
 #define CTR_CWG_MASK		15

index 155fd91e78f4..f0f27aeefb73 100644
@@ -448,9 +448,8 @@ int module_finalize(const Elf_Ehdr *hdr,
 	const char *secstrs = (void *)hdr + sechdrs[hdr->e_shstrndx].sh_offset;
 
 	for (s = sechdrs, se = sechdrs + hdr->e_shnum; s < se; s++) {
-		if (strcmp(".altinstructions", secstrs + s->sh_name) == 0) {
-			apply_alternatives((void *)s->sh_addr, s->sh_size);
-		}
+		if (strcmp(".altinstructions", secstrs + s->sh_name) == 0)
+			apply_alternatives_module((void *)s->sh_addr, s->sh_size);
 #ifdef CONFIG_ARM64_MODULE_PLTS
 		if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE) &&
 		    !strcmp(".text.ftrace_trampoline", secstrs + s->sh_name))

Native boot happens once. But, each VM that boots will slow down all of the other VM's on the system. A VM boot can happen thousands of times.

I really don't want to cause the whole system to hiccup for a millisecond or more when there are only a few cache lines that need to be invalidated.

You know we already do this on boot, right? If it's a real issue, I'd argue that it's the hypervisor's problem to solve.

Anyway, please can you back this up with some real numbers that show the impact on a real use-case? It feels like we've quickly getting into hypothetical territory here because you have a instinctive dislike for full I-cache invalidation.

Will

I am very aware. I would like to fix the rest of boot and change KVM to trap when the guest does something like that.

But, if you don't want the patch without using an invalidate all, I would rather get Rohit's patch in with an invalidate all than have a system that fails to boot.