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

[Julien Grall <julien.grall@xxxxxxx>]

Hi Tamas,

On 27/01/17 18:17, 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.
>
> In this patch we introduce VA-based icache flushing macros.
I think you forgot to update the commit message.

The code looks good to me.

Cheers,

> 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.
>
> 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)
--
Julien Grall

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