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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 27 Jan 2017, Julien Grall wrote:
> 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).

I understand. We probably need to add an icache flush on certain
ballooning operations.


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

This seems like a good way forward.


> For the second instance, we have no other choice.

Most alloc_heap_pages (alloc_xenheap_pages and alloc_domheap_pages) are
done at domain initialization, so they would also be taken care by
flushing the instruction cache before the domain is running. There are
only very few exceptions to that, the main one being ballooning, and we
need another icache flush in that case. But I think we should avoid
doing global icache flushes every time alloc_heap_pages is called.


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

DOMCTL_cacheflush is called several time during the domain build, it is
certainly better to do the icache flush once, rather than many times.

If we decide to perform one icache flush at domain creation in Xen at
the right time, we still need XEN_DOMCTL_cacheflush in its current form
to flush the dcache.

Also we still need a way to flush the icache to solve Tamas' problem
with mem_access at run time.

As a consequence, even after introducing one icache flush in Xen at
domain creation, I think we still need a new "icache flush" flag in
XEN_DOMCTL_cacheflush: all the current callers would not use it, but
mem_access userspace components will be able to use it.


> Looking at GNTTABOP_cache_flush, I think we will also need to invalidate the
> instruction cache (or at least partially).

GNTTABOP_cache_flush is used for DMA coherency with devices. Maybe there
are cases where an icache flush might be needed, but Linux doesn't use
it that way. In fact, if Linux needed a icache flush, it would need it
on native as well, but it does not.

For completeness, we could add a flag to the hypercall to request an
icache flush, but today it would be left unused.

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