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

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Fri, Jan 27, 2017 at 10:25 AM, Julien Grall <julien.grall@xxxxxxx> wrote:
> Hello Tamas,
>
> Please give a bit more time to people for reviewing before sending a new
> version. Patches adding cache instruction are never easy to review.
>
> On 27/01/17 16:45, 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. 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.
>
>
> In the future, please include a reference to the ARM ARM to corroborate your
> testing. This would speed-up the review.

Ack.

>
>
>>
>> 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                |  1 +
>>  xen/include/asm-arm/arm32/page.h |  3 +++
>>  xen/include/asm-arm/arm64/page.h |  3 +++
>>  xen/include/asm-arm/page.h       | 31 +++++++++++++++++++++++++++++++
>>  7 files changed, 49 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..43e5b3d9e2 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -389,6 +389,7 @@ void flush_page_to_ram(unsigned long mfn)
>>      void *v = map_domain_page(_mfn(mfn));
>>
>>      clean_and_invalidate_dcache_va_range(v, PAGE_SIZE);
>> +    invalidate_icache_va_range(v, PAGE_SIZE);
>
>
> I was about to say that the instruction cache flush would not be necessary
> for the current use case. But in fact, even if the domain is not yet running
> or we are allocating a page, we need to flush the I-Cache.
>
> However, I am afraid that invalidating the I-Cache by VA range will not work
> as you expect on all platforms. This will highly depend on the behavior of
> the I-Cache (See D4.9.2 in ARM DDI 0487A.k_iss10775).
>
> For some of the instruction cache (such as VIPT), you would need to flush
> the entire I-Cache to guarantee that all the aliases of a given physical
> address will be removed from the cache.
>
> There are 2 options to fix the issue:
>         1) Always flush the entire cache
>         2) Either flush by VA or the entire cache depending of the I-cache
> implementation
>
> I don't mind if you implement option 1 for now.

That is certainly fine by me. Thanks for looking into this!

Tamas

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