[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH for-next] xen/arm: mm: flush_page_to_ram() only need to clean to PoC





On 22/02/2021 11:58, Bertrand Marquis wrote:
Hi Julien,

On 20 Feb 2021, at 17:54, Julien Grall <julien@xxxxxxx> wrote:

From: Julien Grall <jgrall@xxxxxxxxxx>

At the moment, flush_page_to_ram() is both cleaning and invalidate to
PoC the page. However, the cache line can be speculated and pull in the
cache right after as it is part of the direct map.

If we go further through this logic maybe all calls to
clean_and_invalidate_dcache_va_range could be transformed in a
clean_dcache_va_range.

Likely yes. But I need to go through them one by one to confirm this is fine to do it (it also depends on the caching attributes used). I have sent this one in advance because this was discussed as part of XSA-364.



So it is pointless to try to invalidate the line in the data cache.


But what about processors which would not speculate ?

Do you expect any performance optimization here ?

When invalidating a line, you effectively remove it from the cache. If the page is going to be access a bit after, then you will have to load from the memory (or another cache).

With this change, you would only need to re-fetch the line if it wasn't evicted by the time it is accessed.

The line would be clean, so I would expect the eviction to have less an impact over re-fetching the memory.


If so it might be good to explain it as I am not quite sure I get it.

This change is less about performance and more about unnecessary work.

The processor is likely going to be more clever than the developper and the exact numbers will vary depending on how the processor decide to manage the cache.

In general, we should avoid interferring too much with the cache without a good reason to do it.

How about the following commit message:

"
At the moment, flush_page_to_ram() is both cleaning and invalidate to
PoC the page.

The goal of flush_page_to_ram() is to prevent corruption when the guest has disabled the cache (the cache line may be dirty) and read the guest to read previous content.

Per this defintion, the invalidating the line is not necessary. So invalidating the cache is unnecessary. In fact, it may be counter-productive as the line may be (speculatively) accessed a bit after. So this will incurr an expensive access to the memory.

More generally, we should avoid interferring too much with cache. Therefore, flush_page_to_ram() is updated to only clean to PoC the page.

The performance impact of this change will depend on your workload/processor.
"

--
Julien Grall



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.