| Message ID | 20170127181756.16184-1-tamas.lengyel@zentific.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Tamas, On 27/01/17 18:17, 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. I think you forgot to update the commit message. The code looks good to me. Cheers, > Also expose > the xc_domain_cacheflush through xenctrl.h. > > Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com> > Acked-by: Wei Liu <wei.liu2@citrix.com> > --- > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Julien Grall <julien.grall@arm.com> > > Note: patch has been verified to solve stale icache issues on the > HiKey platform. > > v3: Flush the entire icache instead of flush by VA > 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 | 10 ++++++++++ > 4 files changed, 21 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..596283fc99 100644 > --- a/xen/arch/arm/mm.c > +++ b/xen/arch/arm/mm.c > @@ -390,6 +390,16 @@ void flush_page_to_ram(unsigned long mfn) > > clean_and_invalidate_dcache_va_range(v, PAGE_SIZE); > unmap_domain_page(v); > + > + /* > + * For some of the instruction cache (such as VIPT), the entire I-Cache > + * needs to be flushed to guarantee that all the aliases of a given > + * physical address will be removed from the cache. > + * Invalidating the I-Cache by VA highly depends on the behavior of the > + * I-Cache (See D4.9.2 in ARM DDI 0487A.k_iss10775). Instead of using flush > + * by VA on select platforms, we just flush the entire cache here. > + */ > + invalidate_icache(); > } > > void __init arch_init_memory(void) >
On Fri, Jan 27, 2017 at 11:20 AM, Julien Grall <julien.grall@arm.com> wrote: > Hi Tamas, > > On 27/01/17 18:17, 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. > > > I think you forgot to update the commit message. > > The code looks good to me. > Doh =) One sec.. Tamas
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..596283fc99 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -390,6 +390,16 @@ void flush_page_to_ram(unsigned long mfn) clean_and_invalidate_dcache_va_range(v, PAGE_SIZE); unmap_domain_page(v); + + /* + * For some of the instruction cache (such as VIPT), the entire I-Cache + * needs to be flushed to guarantee that all the aliases of a given + * physical address will be removed from the cache. + * Invalidating the I-Cache by VA highly depends on the behavior of the + * I-Cache (See D4.9.2 in ARM DDI 0487A.k_iss10775). Instead of using flush + * by VA on select platforms, we just flush the entire cache here. + */ + invalidate_icache(); } void __init arch_init_memory(void)