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

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

On Fri, Jan 27, 2017 at 7:49 AM, Wei Liu <wei.liu2@xxxxxxxxxx> wrote:
> On Thu, Jan 26, 2017 at 03:16:22PM -0700, 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.
>>
>
> Why is this not an issue for x86? Is this because ARM handles coherency
> differently from x86?
>
>> 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.
>> ---
>>  tools/libxc/include/xenctrl.h    |  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 +++++++++++++++++++++++++++++++
>>  6 files changed, 44 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>> index 63c616ff6a..cb80a2b07c 100644
>> --- a/tools/libxc/include/xenctrl.h
>> +++ b/tools/libxc/include/xenctrl.h
>> @@ -2720,6 +2720,12 @@ 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);
>>
>> +/*
>> + * ARM only. Ensure cache coherency after memory modifications.
>> + */
>
> The existing code doesn't suggest this is ARM only. This function is
> used in xc_dom_unmap_one etc. This comment looks wrong.
>
> I don't object to making this function externally visible though.
>
> Wei.

Hi Wei,
the comment is correct, this domctl is for ARM only. If you check the
code in xc_domain.c for the function, you will find this:

#if defined (__i386__) || defined (__x86_64__)
    /*
     * 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.
     */
    errno = ENOSYS;
    return -1;
#else

I guess I could move this comment to xenctrl.h to avoid confusions like this.

Cheers,
Tamas

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