[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

  • To: Julien Grall <julien@xxxxxxx>
  • From: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
  • Date: Mon, 22 Feb 2021 13:48:58 +0000
  • Accept-language: en-GB, en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=h8m3exn3bRMvrS0WFhcJORaBO+3ZwcCWXcVBfe7b/LY=; b=M2IJ0cSmBudjC2HlfjVgrr7PUBFRFlWAtdwCzNdyCaP9I8QD0PFNmcBkT3yTQfJhyJDfVrc1y5zIBX+8S84yaTOwGoWk9zQjzkn8piQfpjy77snBQCW3bfF2bCzdX1LU7Xcsu4nI6zzdDHbAZuGhXkUbPAN4EIWa6YNwy2i2TuFRxsQr/wUXcZB0jDoMAkSe0lplanw0ZueIpYD9N42WH25dLZIjF7nxaxnlVE9vJutVgEmWl5V5gVgDZlBRYFPzoEMVaTgN3X1FPh+FWdxGvhSTyKmDQWBZbbaKhAH8/+7LlX0VQD14HjAsYAL4YJmcLyDmP7rHVLCJ8XsCmrTNBA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Ez+k5sFTWSftMQ7kmsOB9HTKSqegx+O9KeqkhW71kQtfxAgLQCs1PMAN1nbA79g1nnWSNPoYF36Pt0IH1tbVTQa8985ZsePbyD1tU3Q7XtiiKJvr6EpAqrmkLTYnYup0+CU6v7PmafEIbMETgGP9YBQoWviZaHQBBXEfYMhbJSd1KwB7Z8+jGEukMaW9+M76BPmYGHBB64Q5Cm73kXXEF6DiaHu2yJpXSJu0LaQ1i8f4i5ycM9H4wvhfp78ZUj8Fyt+k5ARyHbX42pkhH2OClyVE+AoMl6mbATUiGXvhVMSMN+XfOpGw/zzh5yepz0wT+QfQ00cNNWPkptkxYV89jA==
  • Authentication-results-original: xen.org; dkim=none (message not signed) header.d=none;xen.org; dmarc=none action=none header.from=arm.com;
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Julien Grall <jgrall@xxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • Delivery-date: Mon, 22 Feb 2021 13:49:22 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: true
  • Original-authentication-results: xen.org; dkim=none (message not signed) header.d=none;xen.org; dmarc=none action=none header.from=arm.com;
  • Thread-index: AQHXB7GCcUtPYwKMVk+6z4A6IyPFF6pkFLEAgAAbkICAAANLAA==
  • Thread-topic: [PATCH for-next] xen/arm: mm: flush_page_to_ram() only need to clean to PoC

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.


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


> -- 
> Julien Grall



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