Re: [Xen-devel] [PATCH] xen/arm: flush icache as well when XEN_DOMCTL_cacheflush is issued

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Jan 27, 2017 08:43, "Wei Liu" <wei.liu2@xxxxxxxxxx> 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@xxxxxxxxxx> 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@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.
> >> ---
> >>  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

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