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, 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?

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


> v4: Fix commit message
> v3: Flush the entire icache instead of flush by VA
> v2: Return 0 on x86 and clarify comment in xenctrl.h
> ---
>  tools/libxc/include/xenctrl.h |  8 ++++++++
>  tools/libxc/xc_domain.c       |  6 +++---
>  tools/libxc/xc_private.h      |  3 ---
>  xen/arch/arm/mm.c             | 10 ++++++++++
>  4 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 63c616ff6a..a2f23fcd5a 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -2720,6 +2720,14 @@ 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);
>  
> +/*
> + * Ensure cache coherency after memory modifications. A call to this function
> + * is only required on ARM as the x86 architecture provides cache coherency
> + * guarantees. Calling this function on x86 is allowed but has no effect.
> + */
> +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_domain.c b/tools/libxc/xc_domain.c
> index 296b8523b5..98ab6ba3fd 100644
> --- a/tools/libxc/xc_domain.c
> +++ b/tools/libxc/xc_domain.c
> @@ -74,10 +74,10 @@ int xc_domain_cacheflush(xc_interface *xch, uint32_t 
> domid,
>      /*
>       * 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.
> +     * just for Xen to return -ENOSYS.  It is safe to ignore this call on x86
> +     * so we just return 0.
>       */
> -    errno = ENOSYS;
> -    return -1;
> +    return 0;
>  #else
>      DECLARE_DOMCTL;
>      domctl.cmd = XEN_DOMCTL_cacheflush;
> 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..596283fc99 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -390,6 +390,16 @@ void flush_page_to_ram(unsigned long mfn)
>  
>      clean_and_invalidate_dcache_va_range(v, PAGE_SIZE);
>      unmap_domain_page(v);
> +
> +    /*
> +     * For some of the instruction cache (such as VIPT), the entire I-Cache
> +     * needs to be flushed to guarantee that all the aliases of a given
> +     * physical address will be removed from the cache.
> +     * Invalidating the I-Cache by VA highly depends on the behavior of the
> +     * I-Cache (See D4.9.2 in ARM DDI 0487A.k_iss10775). Instead of using 
> flush
> +     * by VA on select platforms, we just flush the entire cache here.
> +     */
> +    invalidate_icache();
>  }
>  
>  void __init arch_init_memory(void)
> -- 
> 2.11.0
> 

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