All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] xen/arm: flush icache as well when XEN_DOMCTL_cacheflush is issued
@ 2017-01-26 22:16 Tamas K Lengyel
  2017-01-27 14:49 ` Wei Liu
  0 siblings, 1 reply; 11+ messages in thread
From: Tamas K Lengyel @ 2017-01-26 22:16 UTC (permalink / raw)
  To: xen-devel
  Cc: Tamas K Lengyel, Julien Grall, Stefano Stabellini, Ian Jackson, Wei Liu

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(-)

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
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] xen/arm: flush icache as well when XEN_DOMCTL_cacheflush is issued
  2017-01-26 22:16 [PATCH] xen/arm: flush icache as well when XEN_DOMCTL_cacheflush is issued Tamas K Lengyel
@ 2017-01-27 14:49 ` Wei Liu
  2017-01-27 15:37   ` Tamas K Lengyel
  0 siblings, 1 reply; 11+ messages in thread
From: Wei Liu @ 2017-01-27 14:49 UTC (permalink / raw)
  To: Tamas K Lengyel
  Cc: xen-devel, Julien Grall, Stefano Stabellini, Ian Jackson, Wei Liu

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] xen/arm: flush icache as well when XEN_DOMCTL_cacheflush is issued
  2017-01-27 14:49 ` Wei Liu
@ 2017-01-27 15:37   ` Tamas K Lengyel
  2017-01-27 15:43     ` Wei Liu
  0 siblings, 1 reply; 11+ messages in thread
From: Tamas K Lengyel @ 2017-01-27 15:37 UTC (permalink / raw)
  To: Wei Liu; +Cc: xen-devel, Julien Grall, Stefano Stabellini, Ian Jackson

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] xen/arm: flush icache as well when XEN_DOMCTL_cacheflush is issued
  2017-01-27 15:37   ` Tamas K Lengyel
@ 2017-01-27 15:43     ` Wei Liu
  2017-01-27 15:52       ` Tamas K Lengyel
  0 siblings, 1 reply; 11+ messages in thread
From: Wei Liu @ 2017-01-27 15:43 UTC (permalink / raw)
  To: Tamas K Lengyel
  Cc: xen-devel, Julien Grall, Stefano Stabellini, Wei Liu, Ian Jackson

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] xen/arm: flush icache as well when XEN_DOMCTL_cacheflush is issued
  2017-01-27 15:43     ` Wei Liu
@ 2017-01-27 15:52       ` Tamas K Lengyel
  2017-01-27 15:54         ` Wei Liu
  2017-01-27 16:15         ` Julien Grall
  0 siblings, 2 replies; 11+ messages in thread
From: Tamas K Lengyel @ 2017-01-27 15:52 UTC (permalink / raw)
  To: Wei Liu; +Cc: xen-devel, Julien Grall, Stefano Stabellini, Ian Jackson


[-- Attachment #1.1: Type: text/plain, Size: 3477 bytes --]

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

[-- Attachment #1.2: Type: text/html, Size: 5141 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] xen/arm: flush icache as well when XEN_DOMCTL_cacheflush is issued
  2017-01-27 15:52       ` Tamas K Lengyel
@ 2017-01-27 15:54         ` Wei Liu
  2017-01-27 16:15         ` Julien Grall
  1 sibling, 0 replies; 11+ messages in thread
From: Wei Liu @ 2017-01-27 15:54 UTC (permalink / raw)
  To: Tamas K Lengyel
  Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] xen/arm: flush icache as well when XEN_DOMCTL_cacheflush is issued
  2017-01-27 15:52       ` Tamas K Lengyel
  2017-01-27 15:54         ` Wei Liu
@ 2017-01-27 16:15         ` Julien Grall
  2017-01-27 16:23           ` Tamas K Lengyel
  1 sibling, 1 reply; 11+ messages in thread
From: Julien Grall @ 2017-01-27 16:15 UTC (permalink / raw)
  To: Tamas K Lengyel, Wei Liu; +Cc: xen-devel, Stefano Stabellini, Ian Jackson

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,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] xen/arm: flush icache as well when XEN_DOMCTL_cacheflush is issued
  2017-01-27 16:15         ` Julien Grall
@ 2017-01-27 16:23           ` Tamas K Lengyel
  2017-01-27 16:29             ` Julien Grall
  0 siblings, 1 reply; 11+ messages in thread
From: Tamas K Lengyel @ 2017-01-27 16:23 UTC (permalink / raw)
  To: Julien Grall; +Cc: Ian Jackson, Stefano Stabellini, Wei Liu, xen-devel

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] xen/arm: flush icache as well when XEN_DOMCTL_cacheflush is issued
  2017-01-27 16:23           ` Tamas K Lengyel
@ 2017-01-27 16:29             ` Julien Grall
  2017-01-27 16:32               ` Tamas K Lengyel
  0 siblings, 1 reply; 11+ messages in thread
From: Julien Grall @ 2017-01-27 16:29 UTC (permalink / raw)
  To: Tamas K Lengyel; +Cc: Ian Jackson, Stefano Stabellini, Wei Liu, xen-devel

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,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] xen/arm: flush icache as well when XEN_DOMCTL_cacheflush is issued
  2017-01-27 16:29             ` Julien Grall
@ 2017-01-27 16:32               ` Tamas K Lengyel
  2017-01-27 16:35                 ` Wei Liu
  0 siblings, 1 reply; 11+ messages in thread
From: Tamas K Lengyel @ 2017-01-27 16:32 UTC (permalink / raw)
  To: Julien Grall; +Cc: Ian Jackson, Stefano Stabellini, Wei Liu, xen-devel

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] xen/arm: flush icache as well when XEN_DOMCTL_cacheflush is issued
  2017-01-27 16:32               ` Tamas K Lengyel
@ 2017-01-27 16:35                 ` Wei Liu
  0 siblings, 0 replies; 11+ messages in thread
From: Wei Liu @ 2017-01-27 16:35 UTC (permalink / raw)
  To: Tamas K Lengyel
  Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2017-01-27 16:35 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-26 22:16 [PATCH] xen/arm: flush icache as well when XEN_DOMCTL_cacheflush is issued Tamas K Lengyel
2017-01-27 14:49 ` Wei Liu
2017-01-27 15:37   ` Tamas K Lengyel
2017-01-27 15:43     ` Wei Liu
2017-01-27 15:52       ` Tamas K Lengyel
2017-01-27 15:54         ` Wei Liu
2017-01-27 16:15         ` Julien Grall
2017-01-27 16:23           ` Tamas K Lengyel
2017-01-27 16:29             ` Julien Grall
2017-01-27 16:32               ` Tamas K Lengyel
2017-01-27 16:35                 ` Wei Liu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.