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

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

On Fri, Jan 27, 2017 at 1:41 PM, Stefano Stabellini
<sstabellini@xxxxxxxxxx> 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 case where it is very easy to trigger a stale icache is during an
SMC trap on multi-core boards (like the HiKey). If the monitor
application removes the SMC from memory in the callback (through
xc_map_foreign_range), the SMC instruction will still happen
repeatedly afterwards until the CPU gives up and fetches the actual
contents of the memory. The same icache issue could get triggered any
time the user is modifying instructions in the memory of an active VM.
But flushing the dcache would also be required any time memory is
modified anywhere, so exposing the libxc function would be necessary
without the icache issue anyway.

Speaking of which, after reading
https://events.linuxfoundation.org/sites/events/files/slides/slides_17.pdf,
shouldn't the cache flush happen both before and after memory is
modified (as shown on slide 33)?

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

I'm OK with that, though that would probably require a bump in
XEN_DOMCTL_INTERFACE_VERSION.

Tamas

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