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

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 27/01/2017 20:41, Stefano Stabellini wrote:
> On Fri, 27 Jan 2017, 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.
> >
> > Also expose the xc_domain_cacheflush through xenctrl.h.
> >
> > Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>
> > Acked-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> > ---
> > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> > 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.
> Sorry to come in the discussion late, but there has been a lot of
> traffic on this in the last two days. The patch is clear and well
> written, thanks for that. However, I am concerned about the performance
> impact of the icache flush.
>
> What stale icache issues is it trying to solve?
The icache issue for Tamas is when the monitor application is writing 
into the guest memory.
Even if we put aside this problem, the instruction cache is not able to 
snoop into the data cache. So if we don't invalidate the instruction 
cache, the guest may see stale instruction when booting or may 
requesting memory and then use it (e.g ballooning).
> This patch introduces the icache flush in flush_page_to_ram, which is
> called in two instances:
>
> 1) arch_do_domctl(XEN_DOMCTL_cacheflush) -> p2m_cache_flush -> flush_page_to_ram
>
> 2) alloc_xenheap_pages
>
> It looks like we don't need the icache flush in 2). We should probably
> avoid icache flushes for that. Julien, do you agree?
I disagree, in both case the full icache flush is necessary. As Tamas 
wrote in the comment, it is not possible to invalidate by VA because it 
does not ensure that all aliases will be removed in non-PIPT cache.
For the first instance, we could avoid the icache flush in each DOMCTL 
by flushing the instruction cache before the domain is running for the 
first time.
For the second instance, we have no other choice.

> I am also wondering about all the libxc/libxl callers, many of whom
> don't need an icache flush. Ideally we would have an argument in
> XEN_DOMCTL_cacheflush to specify the type of cache flush we need,
> similar to GNTTABOP_cache_flush.
Unless the instruction cache will be flushed before the guest is 
booting, all the callers of DOMCTL_cacheflush will require the invalidation.
Looking at GNTTABOP_cache_flush, I think we will also need to invalidate 
the instruction cache (or at least partially).
Regards,

--
Julien Grall

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