[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v2] xen/arm: flush icache as well when XEN_DOMCTL_cacheflush is issued
Hello Tamas,
Please give a bit more time to people for reviewing before sending a new
version. Patches adding cache instruction are never easy to review.
On 27/01/17 16:45, Tamas K Lengyel wrote:
When the toolstack modifies memory of a running ARM VM it may happen
that the underlying memory of a current vCPU PC is changed. Without
flushing the icache the vCPU may continue executing stale instructions.
In this patch we introduce VA-based icache flushing macros. Also expose
the xc_domain_cacheflush through xenctrl.h.
Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>
---
Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
Cc: Julien Grall <julien.grall@xxxxxxx>
Note: patch has been verified to solve stale icache issues on the
HiKey platform.
In the future, please include a reference to the ARM ARM to corroborate
your testing. This would speed-up the review.
v2: Return 0 on x86 and clarify comment in xenctrl.h
---
tools/libxc/include/xenctrl.h | 8 ++++++++
tools/libxc/xc_domain.c | 6 +++---
tools/libxc/xc_private.h | 3 ---
xen/arch/arm/mm.c | 1 +
xen/include/asm-arm/arm32/page.h | 3 +++
xen/include/asm-arm/arm64/page.h | 3 +++
xen/include/asm-arm/page.h | 31 +++++++++++++++++++++++++++++++
7 files changed, 49 insertions(+), 6 deletions(-)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 63c616ff6a..a2f23fcd5a 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2720,6 +2720,14 @@ int xc_livepatch_revert(xc_interface *xch, char *name,
uint32_t timeout);
int xc_livepatch_unload(xc_interface *xch, char *name, uint32_t timeout);
int xc_livepatch_replace(xc_interface *xch, char *name, uint32_t timeout);
+/*
+ * Ensure cache coherency after memory modifications. A call to this function
+ * is only required on ARM as the x86 architecture provides cache coherency
+ * guarantees. Calling this function on x86 is allowed but has no effect.
+ */
+int xc_domain_cacheflush(xc_interface *xch, uint32_t domid,
+ xen_pfn_t start_pfn, xen_pfn_t nr_pfns);
+
/* Compat shims */
#include "xenctrl_compat.h"
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 296b8523b5..98ab6ba3fd 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -74,10 +74,10 @@ int xc_domain_cacheflush(xc_interface *xch, uint32_t domid,
/*
* The x86 architecture provides cache coherency guarantees which prevent
* the need for this hypercall. Avoid the overhead of making a hypercall
- * just for Xen to return -ENOSYS.
+ * just for Xen to return -ENOSYS. It is safe to ignore this call on x86
+ * so we just return 0.
*/
- errno = ENOSYS;
- return -1;
+ return 0;
#else
DECLARE_DOMCTL;
domctl.cmd = XEN_DOMCTL_cacheflush;
diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
index 97445ae1fe..fddebdc917 100644
--- a/tools/libxc/xc_private.h
+++ b/tools/libxc/xc_private.h
@@ -366,9 +366,6 @@ void bitmap_byte_to_64(uint64_t *lp, const uint8_t *bp, int
nbits);
/* Optionally flush file to disk and discard page cache */
void discard_file_cache(xc_interface *xch, int fd, int flush);
-int xc_domain_cacheflush(xc_interface *xch, uint32_t domid,
- xen_pfn_t start_pfn, xen_pfn_t nr_pfns);
-
#define MAX_MMU_UPDATES 1024
struct xc_mmu {
mmu_update_t updates[MAX_MMU_UPDATES];
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 99588a330d..43e5b3d9e2 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -389,6 +389,7 @@ void flush_page_to_ram(unsigned long mfn)
void *v = map_domain_page(_mfn(mfn));
clean_and_invalidate_dcache_va_range(v, PAGE_SIZE);
+ invalidate_icache_va_range(v, PAGE_SIZE);
I was about to say that the instruction cache flush would not be
necessary for the current use case. But in fact, even if the domain is
not yet running or we are allocating a page, we need to flush the I-Cache.
However, I am afraid that invalidating the I-Cache by VA range will not
work as you expect on all platforms. This will highly depend on the
behavior of the I-Cache (See D4.9.2 in ARM DDI 0487A.k_iss10775).
For some of the instruction cache (such as VIPT), you would need to
flush the entire I-Cache to guarantee that all the aliases of a given
physical address will be removed from the cache.
There are 2 options to fix the issue:
1) Always flush the entire cache
2) Either flush by VA or the entire cache depending of the I-cache
implementation
I don't mind if you implement option 1 for now.
unmap_domain_page(v);
}
diff --git a/xen/include/asm-arm/arm32/page.h b/xen/include/asm-arm/arm32/page.h
index ea4b312c70..10e5288d0f 100644
--- a/xen/include/asm-arm/arm32/page.h
+++ b/xen/include/asm-arm/arm32/page.h
@@ -19,6 +19,9 @@ static inline void write_pte(lpae_t *p, lpae_t pte)
: : "r" (pte.bits), "r" (p) : "memory");
}
+/* Inline ASM to invalidate icache on register R (may be an inline asm
operand) */
+#define __invalidate_icache_one(R) STORE_CP32(R, ICIMVAU)
For ARM32, you would also need to invalidate the branch predictor.
+
/* Inline ASM to invalidate dcache on register R (may be an inline asm
operand) */
#define __invalidate_dcache_one(R) STORE_CP32(R, DCIMVAC)
diff --git a/xen/include/asm-arm/arm64/page.h b/xen/include/asm-arm/arm64/page.h
index 23d778154d..0f380b95b4 100644
--- a/xen/include/asm-arm/arm64/page.h
+++ b/xen/include/asm-arm/arm64/page.h
@@ -16,6 +16,9 @@ static inline void write_pte(lpae_t *p, lpae_t pte)
: : "r" (pte.bits), "r" (p) : "memory");
}
+/* Inline ASM to invalidate icache on register R (may be an inline asm
operand) */
+#define __invalidate_icache_one(R) "ic ivau, %" #R ";"
+
/* Inline ASM to invalidate dcache on register R (may be an inline asm
operand) */
#define __invalidate_dcache_one(R) "dc ivac, %" #R ";"
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index c492d6df50..a618d0e556 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -371,6 +371,37 @@ static inline int clean_and_invalidate_dcache_va_range
: : "r" (_p), "m" (*_p)); \
} while (0)
+static inline int invalidate_icache_va_range(const void *p, unsigned long size)
+{
+ size_t off;
+ const void *end = p + size;
+
+ dsb(sy); /* So the CPU issues all writes to the range */
The invalidation will be happen on the innershareable domain, so
dsb(ish) is enough here.
+
+ off = (unsigned long)p % cacheline_bytes;
cacheline_bytes contains the cacheline size of the data cache, not
instruction cache.
+ if ( off )
+ {
+ p -= off;
+ asm volatile (__invalidate_icache_one(0) : : "r" (p));
+ p += cacheline_bytes;
+ size -= cacheline_bytes - off;
+ }
+ off = (unsigned long)end % cacheline_bytes;
+ if ( off )
+ {
+ end -= off;
+ size -= off;
+ asm volatile (__invalidate_icache_one(0) : : "r" (end));
+ }
+
+ for ( ; p < end; p += cacheline_bytes )
+ asm volatile (__invalidate_icache_one(0) : : "r" (p));
+
+ dsb(sy); /* So we know the flushes happen before continuing */
dsb(ish)
+
+ return 0;
+}
+
/*
* Flush a range of VA's hypervisor mappings from the data TLB of the
* local processor. This is not sufficient when changing code mappings
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
