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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Fri, Jan 27, 2017 at 08:52:50AM -0700, Tamas K Lengyel wrote:
> 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.
> 

Clarifying would be good enough for me.

> Tamas

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