| Message ID | 20170126221622.7148-1-tamas.lengyel@zentific.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jan 26, 2017 at 03:16:22PM -0700, 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. > Why is this not an issue for x86? Is this because ARM handles coherency differently from x86? > 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@zentific.com> > --- > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Wei Liu <wei.liu2@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. > --- > tools/libxc/include/xenctrl.h | 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 +++++++++++++++++++++++++++++++ > 6 files changed, 44 insertions(+), 3 deletions(-) > > diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h > index 63c616ff6a..cb80a2b07c 100644 > --- a/tools/libxc/include/xenctrl.h > +++ b/tools/libxc/include/xenctrl.h > @@ -2720,6 +2720,12 @@ 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); > > +/* > + * ARM only. Ensure cache coherency after memory modifications. > + */ The existing code doesn't suggest this is ARM only. This function is used in xc_dom_unmap_one etc. This comment looks wrong. I don't object to making this function externally visible though. Wei.
On Fri, Jan 27, 2017 at 7:49 AM, Wei Liu <wei.liu2@citrix.com> wrote: > On Thu, Jan 26, 2017 at 03:16:22PM -0700, 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. >> > > Why is this not an issue for x86? Is this because ARM handles coherency > differently from x86? > >> 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@zentific.com> >> --- >> Cc: Ian Jackson <ian.jackson@eu.citrix.com> >> Cc: Wei Liu <wei.liu2@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. >> --- >> tools/libxc/include/xenctrl.h | 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 +++++++++++++++++++++++++++++++ >> 6 files changed, 44 insertions(+), 3 deletions(-) >> >> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h >> index 63c616ff6a..cb80a2b07c 100644 >> --- a/tools/libxc/include/xenctrl.h >> +++ b/tools/libxc/include/xenctrl.h >> @@ -2720,6 +2720,12 @@ 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); >> >> +/* >> + * ARM only. Ensure cache coherency after memory modifications. >> + */ > > The existing code doesn't suggest this is ARM only. This function is > used in xc_dom_unmap_one etc. This comment looks wrong. > > I don't object to making this function externally visible though. > > Wei. Hi Wei, the comment is correct, this domctl is for ARM only. If you check the code in xc_domain.c for the function, you will find this: #if defined (__i386__) || defined (__x86_64__) /* * 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. */ errno = ENOSYS; return -1; #else I guess I could move this comment to xenctrl.h to avoid confusions like this. Cheers, Tamas
On Fri, Jan 27, 2017 at 08:37:33AM -0700, Tamas K Lengyel wrote: > On Fri, Jan 27, 2017 at 7:49 AM, Wei Liu <wei.liu2@citrix.com> wrote: > > On Thu, Jan 26, 2017 at 03:16:22PM -0700, 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. > >> > > > > Why is this not an issue for x86? Is this because ARM handles coherency > > differently from x86? > > > >> 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@zentific.com> > >> --- > >> Cc: Ian Jackson <ian.jackson@eu.citrix.com> > >> Cc: Wei Liu <wei.liu2@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. > >> --- > >> tools/libxc/include/xenctrl.h | 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 +++++++++++++++++++++++++++++++ > >> 6 files changed, 44 insertions(+), 3 deletions(-) > >> > >> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h > >> index 63c616ff6a..cb80a2b07c 100644 > >> --- a/tools/libxc/include/xenctrl.h > >> +++ b/tools/libxc/include/xenctrl.h > >> @@ -2720,6 +2720,12 @@ 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); > >> > >> +/* > >> + * ARM only. Ensure cache coherency after memory modifications. > >> + */ > > > > The existing code doesn't suggest this is ARM only. This function is > > used in xc_dom_unmap_one etc. This comment looks wrong. > > > > I don't object to making this function externally visible though. > > > > Wei. > > Hi Wei, > the comment is correct, this domctl is for ARM only. If you check the > code in xc_domain.c for the function, you will find this: > > #if defined (__i386__) || defined (__x86_64__) > /* > * 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. > */ > errno = ENOSYS; > return -1; > #else > > I guess I could move this comment to xenctrl.h to avoid confusions like this. > Maybe it is just me: I read this comment differently. It suggests only ARM code can call this function. In reality x86 can call it too, but it has no effect. I would suggest either just drop the comment or make clear it only has effect on ARM. Wei. > Cheers, > Tamas
On Jan 27, 2017 08:43, "Wei Liu" <wei.liu2@citrix.com> wrote: On Fri, Jan 27, 2017 at 08:37:33AM -0700, Tamas K Lengyel wrote: > On Fri, Jan 27, 2017 at 7:49 AM, Wei Liu <wei.liu2@citrix.com> wrote: > > On Thu, Jan 26, 2017 at 03:16:22PM -0700, 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. > >> > > > > Why is this not an issue for x86? Is this because ARM handles coherency > > differently from x86? > > > >> 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@zentific.com> > >> --- > >> Cc: Ian Jackson <ian.jackson@eu.citrix.com> > >> Cc: Wei Liu <wei.liu2@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. > >> --- > >> tools/libxc/include/xenctrl.h | 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 +++++++++++++++++++++++++++++++ > >> 6 files changed, 44 insertions(+), 3 deletions(-) > >> > >> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h > >> index 63c616ff6a..cb80a2b07c 100644 > >> --- a/tools/libxc/include/xenctrl.h > >> +++ b/tools/libxc/include/xenctrl.h > >> @@ -2720,6 +2720,12 @@ 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); > >> > >> +/* > >> + * ARM only. Ensure cache coherency after memory modifications. > >> + */ > > > > The existing code doesn't suggest this is ARM only. This function is > > used in xc_dom_unmap_one etc. This comment looks wrong. > > > > I don't object to making this function externally visible though. > > > > Wei. > > Hi Wei, > the comment is correct, this domctl is for ARM only. If you check the > code in xc_domain.c for the function, you will find this: > > #if defined (__i386__) || defined (__x86_64__) > /* > * 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. > */ > errno = ENOSYS; > return -1; > #else > > I guess I could move this comment to xenctrl.h to avoid confusions like this. > Maybe it is just me: I read this comment differently. It suggests only ARM code can call this function. In reality x86 can call it too, but it has no effect. I would suggest either just drop the comment or make clear it only has effect on ARM. Well, yes, only ARM could _should_ call this function. The comment I think is important to tell the user don't expect it to do anything on x86. Doesn't mean they can't call it though - if that was the case it would be wrapped in an ifdef like all the other architecture specific bits in the header. I would think that's pretty straight forward. No objection to clarifing the comment though if it helps. Tamas
On Fri, Jan 27, 2017 at 08:52:50AM -0700, Tamas K Lengyel wrote: > On Jan 27, 2017 08:43, "Wei Liu" <wei.liu2@citrix.com> wrote: > > On Fri, Jan 27, 2017 at 08:37:33AM -0700, Tamas K Lengyel wrote: > > On Fri, Jan 27, 2017 at 7:49 AM, Wei Liu <wei.liu2@citrix.com> wrote: > > > On Thu, Jan 26, 2017 at 03:16:22PM -0700, 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. > > >> > > > > > > Why is this not an issue for x86? Is this because ARM handles coherency > > > differently from x86? > > > > > >> 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@zentific.com> > > >> --- > > >> Cc: Ian Jackson <ian.jackson@eu.citrix.com> > > >> Cc: Wei Liu <wei.liu2@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. > > >> --- > > >> tools/libxc/include/xenctrl.h | 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 +++++++++++++++++++++++++++++++ > > >> 6 files changed, 44 insertions(+), 3 deletions(-) > > >> > > >> diff --git a/tools/libxc/include/xenctrl.h > b/tools/libxc/include/xenctrl.h > > >> index 63c616ff6a..cb80a2b07c 100644 > > >> --- a/tools/libxc/include/xenctrl.h > > >> +++ b/tools/libxc/include/xenctrl.h > > >> @@ -2720,6 +2720,12 @@ 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); > > >> > > >> +/* > > >> + * ARM only. Ensure cache coherency after memory modifications. > > >> + */ > > > > > > The existing code doesn't suggest this is ARM only. This function is > > > used in xc_dom_unmap_one etc. This comment looks wrong. > > > > > > I don't object to making this function externally visible though. > > > > > > Wei. > > > > Hi Wei, > > the comment is correct, this domctl is for ARM only. If you check the > > code in xc_domain.c for the function, you will find this: > > > > #if defined (__i386__) || defined (__x86_64__) > > /* > > * 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. > > */ > > errno = ENOSYS; > > return -1; > > #else > > > > I guess I could move this comment to xenctrl.h to avoid confusions like > this. > > > > Maybe it is just me: I read this comment differently. It suggests only > ARM code can call this function. In reality x86 can call it too, but it > has no effect. > > I would suggest either just drop the comment or make clear it only has > effect on ARM. > > > Well, yes, only ARM could _should_ call this function. The comment I think > is important to tell the user don't expect it to do anything on x86. > Doesn't mean they can't call it though - if that was the case it would be > wrapped in an ifdef like all the other architecture specific bits in the > header. I would think that's pretty straight forward. No objection to > clarifing the comment though if it helps. > Clarifying would be good enough for me. > Tamas
Hello, On 27/01/17 15:52, Tamas K Lengyel wrote: > Well, yes, only ARM could _should_ call this function. The comment I > think is important to tell the user don't expect it to do anything on > x86. Doesn't mean they can't call it though - if that was the case it > would be wrapped in an ifdef like all the other architecture specific > bits in the header. I would think that's pretty straight forward. No > objection to clarifing the comment though if it helps. If you looked at the commit log, the #ifdef was added to avoid calling the hypervisor for nothing and therefore saving few hundred cycles bit of time. Technically speaking, this helper abstracts the architectural behavior of the cache. So it makes sense to call it on x86 even if it is a nop. Now, if you are saying that on x86 it might be necessary to also clean the guest memory range. Then it would be worth to consider implementing the hypercall. Cheers,
On Fri, Jan 27, 2017 at 9:15 AM, Julien Grall <julien.grall@arm.com> wrote: > Hello, > > On 27/01/17 15:52, Tamas K Lengyel wrote: >> >> Well, yes, only ARM could _should_ call this function. The comment I >> think is important to tell the user don't expect it to do anything on >> x86. Doesn't mean they can't call it though - if that was the case it >> would be wrapped in an ifdef like all the other architecture specific >> bits in the header. I would think that's pretty straight forward. No >> objection to clarifing the comment though if it helps. > > > If you looked at the commit log, the #ifdef was added to avoid calling the > hypervisor for nothing and therefore saving few hundred cycles bit of time. > Technically speaking, this helper abstracts the architectural behavior of > the cache. So it makes sense to call it on x86 even if it is a nop. Except that on x86 the user should be aware that it returns an error, which is normal and can be ignored. > > Now, if you are saying that on x86 it might be necessary to also clean the > guest memory range. Then it would be worth to consider implementing the > hypercall. > I'm not aware of this being needed on x86. Tamas
Hi Tamas, On 27/01/17 16:23, Tamas K Lengyel wrote: > On Fri, Jan 27, 2017 at 9:15 AM, Julien Grall <julien.grall@arm.com> wrote: >> Hello, >> >> On 27/01/17 15:52, Tamas K Lengyel wrote: >>> >>> Well, yes, only ARM could _should_ call this function. The comment I >>> think is important to tell the user don't expect it to do anything on >>> x86. Doesn't mean they can't call it though - if that was the case it >>> would be wrapped in an ifdef like all the other architecture specific >>> bits in the header. I would think that's pretty straight forward. No >>> objection to clarifing the comment though if it helps. >> >> >> If you looked at the commit log, the #ifdef was added to avoid calling the >> hypervisor for nothing and therefore saving few hundred cycles bit of time. >> Technically speaking, this helper abstracts the architectural behavior of >> the cache. So it makes sense to call it on x86 even if it is a nop. > > Except that on x86 the user should be aware that it returns an error, > which is normal and can be ignored. It looks like the current callers does not check the return. However, it would more make sense to return 0 if we expect nothing to be done rather than -ENOSYS. Regards,
On Fri, Jan 27, 2017 at 9:29 AM, Julien Grall <julien.grall@arm.com> wrote: > Hi Tamas, > > On 27/01/17 16:23, Tamas K Lengyel wrote: >> >> On Fri, Jan 27, 2017 at 9:15 AM, Julien Grall <julien.grall@arm.com> >> wrote: >>> >>> Hello, >>> >>> On 27/01/17 15:52, Tamas K Lengyel wrote: >>>> >>>> >>>> Well, yes, only ARM could _should_ call this function. The comment I >>>> think is important to tell the user don't expect it to do anything on >>>> x86. Doesn't mean they can't call it though - if that was the case it >>>> would be wrapped in an ifdef like all the other architecture specific >>>> bits in the header. I would think that's pretty straight forward. No >>>> objection to clarifing the comment though if it helps. >>> >>> >>> >>> If you looked at the commit log, the #ifdef was added to avoid calling >>> the >>> hypervisor for nothing and therefore saving few hundred cycles bit of >>> time. >>> Technically speaking, this helper abstracts the architectural behavior of >>> the cache. So it makes sense to call it on x86 even if it is a nop. >> >> >> Except that on x86 the user should be aware that it returns an error, >> which is normal and can be ignored. > > > It looks like the current callers does not check the return. However, it > would more make sense to return 0 if we expect nothing to be done rather > than -ENOSYS. > That would be fine by me. Wei, what do you think? Tamas
On Fri, Jan 27, 2017 at 09:32:25AM -0700, Tamas K Lengyel wrote: > On Fri, Jan 27, 2017 at 9:29 AM, Julien Grall <julien.grall@arm.com> wrote: > > Hi Tamas, > > > > On 27/01/17 16:23, Tamas K Lengyel wrote: > >> > >> On Fri, Jan 27, 2017 at 9:15 AM, Julien Grall <julien.grall@arm.com> > >> wrote: > >>> > >>> Hello, > >>> > >>> On 27/01/17 15:52, Tamas K Lengyel wrote: > >>>> > >>>> > >>>> Well, yes, only ARM could _should_ call this function. The comment I > >>>> think is important to tell the user don't expect it to do anything on > >>>> x86. Doesn't mean they can't call it though - if that was the case it > >>>> would be wrapped in an ifdef like all the other architecture specific > >>>> bits in the header. I would think that's pretty straight forward. No > >>>> objection to clarifing the comment though if it helps. > >>> > >>> > >>> > >>> If you looked at the commit log, the #ifdef was added to avoid calling > >>> the > >>> hypervisor for nothing and therefore saving few hundred cycles bit of > >>> time. > >>> Technically speaking, this helper abstracts the architectural behavior of > >>> the cache. So it makes sense to call it on x86 even if it is a nop. > >> > >> > >> Except that on x86 the user should be aware that it returns an error, > >> which is normal and can be ignored. > > > > > > It looks like the current callers does not check the return. However, it > > would more make sense to return 0 if we expect nothing to be done rather > > than -ENOSYS. > > > > That would be fine by me. Wei, what do you think? > No objection from me. > Tamas
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h index 63c616ff6a..cb80a2b07c 100644 --- a/tools/libxc/include/xenctrl.h +++ b/tools/libxc/include/xenctrl.h @@ -2720,6 +2720,12 @@ 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); +/* + * ARM only. Ensure cache coherency after memory modifications. + */ +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_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); 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) + /* 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 */ + + off = (unsigned long)p % cacheline_bytes; + 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 */ + + 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
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@zentific.com> --- Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Wei Liu <wei.liu2@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. --- tools/libxc/include/xenctrl.h | 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 +++++++++++++++++++++++++++++++ 6 files changed, 44 insertions(+), 3 deletions(-)