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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 22 Feb 2021, at 13:37, Julien Grall <julien@xxxxxxx> wrote:
> 
> 
> 
> 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.

Ok.

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

Make sense.

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

With this as your commit message you can add my:

Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>

Thanks for details

Cheers
Bertrand

> -- 
> Julien Grall