Hi Julien, > On 22 Feb 2021, at 13:37, Julien Grall <jul...@xen.org> wrote: > > > > On 22/02/2021 11:58, Bertrand Marquis wrote: >> Hi Julien, >>> On 20 Feb 2021, at 17:54, Julien Grall <jul...@xen.org> wrote: >>> >>> From: Julien Grall <jgr...@amazon.com> >>> >>> 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.marq...@arm.com> Thanks for details Cheers Bertrand > -- > Julien Grall
