Re: [PATCH] arch/cacheflush: Introduce flush_all_caches()
Davidlohr Bueso wrote: > On Tue, 16 Aug 2022, Dan Williams wrote: > > >On Tue, Aug 16, 2022 at 10:30 AM Davidlohr Bueso wrote: > >> > >> On Tue, 16 Aug 2022, Dan Williams wrote: > >> > >> >Peter Zijlstra wrote: > >> >> On Mon, Aug 15, 2022 at 09:07:06AM -0700, Davidlohr Bueso wrote: > >> >> > diff --git a/arch/x86/include/asm/cacheflush.h > >> >> > b/arch/x86/include/asm/cacheflush.h > >> >> > index b192d917a6d0..ce2ec9556093 100644 > >> >> > --- a/arch/x86/include/asm/cacheflush.h > >> >> > +++ b/arch/x86/include/asm/cacheflush.h > >> >> > @@ -10,4 +10,7 @@ > >> >> > > >> >> > void clflush_cache_range(void *addr, unsigned int size); > >> >> > > >> >> > +#define flush_all_caches() \ > >> >> > + do { wbinvd_on_all_cpus(); } while(0) > >> >> > + > >> >> > >> >> This is horrific... we've done our utmost best to remove all WBINVD > >> >> usage and here you're adding it back in the most horrible form possible > >> >> ?!? > >> >> > >> >> Please don't do this, do *NOT* use WBINVD. > >> > > >> >Unfortunately there are a few good options here, and the changelog did > >> >not make clear that this is continuing legacy [1], not adding new wbinvd > >> >usage. > >> > >> While I was hoping that it was obvious from the intel.c changes that this > >> was not a new wbinvd, I can certainly improve the changelog with the below. > > > >I also think this cache_flush_region() API wants a prominent comment > >clarifying the limited applicability of this API. I.e. that it is not > >for general purpose usage, not for VMs, and only for select bare metal > >scenarios that instantaneously invalidate wide swaths of memory. > >Otherwise, I can now see how this looks like a potentially scary > >expansion of the usage of wbinvd. > > Sure. > > Also, in the future we might be able to bypass this hammer in the presence > of persistent cpu caches. What would have helped is if the secure-erase and unlock definition in the specification mandated that the device emit cache invalidations for everything it has mapped when it is erased. However, that has some holes, and it also makes me think there is a gap in the current region provisioning code. If I have device-A mapped at physical-address-X and then tear that down and instantiate device-B at that same physical address there needs to be CPU cache invalidation between those 2 events.
Re: [PATCH] arch/cacheflush: Introduce flush_all_caches()
On Tue, 16 Aug 2022, Dan Williams wrote: On Tue, Aug 16, 2022 at 10:30 AM Davidlohr Bueso wrote: On Tue, 16 Aug 2022, Dan Williams wrote: >Peter Zijlstra wrote: >> On Mon, Aug 15, 2022 at 09:07:06AM -0700, Davidlohr Bueso wrote: >> > diff --git a/arch/x86/include/asm/cacheflush.h b/arch/x86/include/asm/cacheflush.h >> > index b192d917a6d0..ce2ec9556093 100644 >> > --- a/arch/x86/include/asm/cacheflush.h >> > +++ b/arch/x86/include/asm/cacheflush.h >> > @@ -10,4 +10,7 @@ >> > >> > void clflush_cache_range(void *addr, unsigned int size); >> > >> > +#define flush_all_caches() \ >> > + do { wbinvd_on_all_cpus(); } while(0) >> > + >> >> This is horrific... we've done our utmost best to remove all WBINVD >> usage and here you're adding it back in the most horrible form possible >> ?!? >> >> Please don't do this, do *NOT* use WBINVD. > >Unfortunately there are a few good options here, and the changelog did >not make clear that this is continuing legacy [1], not adding new wbinvd >usage. While I was hoping that it was obvious from the intel.c changes that this was not a new wbinvd, I can certainly improve the changelog with the below. I also think this cache_flush_region() API wants a prominent comment clarifying the limited applicability of this API. I.e. that it is not for general purpose usage, not for VMs, and only for select bare metal scenarios that instantaneously invalidate wide swaths of memory. Otherwise, I can now see how this looks like a potentially scary expansion of the usage of wbinvd. Sure. Also, in the future we might be able to bypass this hammer in the presence of persistent cpu caches. Thanks, Davidlohr
Re: [PATCH] arch/cacheflush: Introduce flush_all_caches()
On Tue, Aug 16, 2022 at 10:30 AM Davidlohr Bueso wrote: > > On Tue, 16 Aug 2022, Dan Williams wrote: > > >Peter Zijlstra wrote: > >> On Mon, Aug 15, 2022 at 09:07:06AM -0700, Davidlohr Bueso wrote: > >> > diff --git a/arch/x86/include/asm/cacheflush.h > >> > b/arch/x86/include/asm/cacheflush.h > >> > index b192d917a6d0..ce2ec9556093 100644 > >> > --- a/arch/x86/include/asm/cacheflush.h > >> > +++ b/arch/x86/include/asm/cacheflush.h > >> > @@ -10,4 +10,7 @@ > >> > > >> > void clflush_cache_range(void *addr, unsigned int size); > >> > > >> > +#define flush_all_caches() \ > >> > + do { wbinvd_on_all_cpus(); } while(0) > >> > + > >> > >> This is horrific... we've done our utmost best to remove all WBINVD > >> usage and here you're adding it back in the most horrible form possible > >> ?!? > >> > >> Please don't do this, do *NOT* use WBINVD. > > > >Unfortunately there are a few good options here, and the changelog did > >not make clear that this is continuing legacy [1], not adding new wbinvd > >usage. > > While I was hoping that it was obvious from the intel.c changes that this > was not a new wbinvd, I can certainly improve the changelog with the below. I also think this cache_flush_region() API wants a prominent comment clarifying the limited applicability of this API. I.e. that it is not for general purpose usage, not for VMs, and only for select bare metal scenarios that instantaneously invalidate wide swaths of memory. Otherwise, I can now see how this looks like a potentially scary expansion of the usage of wbinvd.
Re: [PATCH] arch/cacheflush: Introduce flush_all_caches()
On Tue, 16 Aug 2022, Dan Williams wrote: Peter Zijlstra wrote: On Mon, Aug 15, 2022 at 09:07:06AM -0700, Davidlohr Bueso wrote: > diff --git a/arch/x86/include/asm/cacheflush.h b/arch/x86/include/asm/cacheflush.h > index b192d917a6d0..ce2ec9556093 100644 > --- a/arch/x86/include/asm/cacheflush.h > +++ b/arch/x86/include/asm/cacheflush.h > @@ -10,4 +10,7 @@ > > void clflush_cache_range(void *addr, unsigned int size); > > +#define flush_all_caches() \ > + do { wbinvd_on_all_cpus(); } while(0) > + This is horrific... we've done our utmost best to remove all WBINVD usage and here you're adding it back in the most horrible form possible ?!? Please don't do this, do *NOT* use WBINVD. Unfortunately there are a few good options here, and the changelog did not make clear that this is continuing legacy [1], not adding new wbinvd usage. While I was hoping that it was obvious from the intel.c changes that this was not a new wbinvd, I can certainly improve the changelog with the below. Thanks, Davidlohr The functionality this is enabling is to be able to instantaneously secure erase potentially terabytes of memory at once and the kernel needs to be sure that none of the data from before the secure is still present in the cache. It is also used when unlocking a memory device where speculative reads and firmware accesses could have cached poison from before the device was unlocked. This capability is typically only used once per-boot (for unlock), or once per bare metal provisioning event (secure erase), like when handing off the system to another tenant. That small scope plus the fact that none of this is available to a VM limits the potential damage. So, similar to the mitigation we did in [2] that did not kill off wbinvd completely, this is limited to specific scenarios and should be disabled in any scenario where wbinvd is painful / forbidden. [1]: 4c6926a23b76 ("acpi/nfit, libnvdimm: Add unlock of nvdimm support for Intel DIMMs") [2]: e2efb6359e62 ("ACPICA: Avoid cache flush inside virtual machines")
Re: [PATCH] arch/cacheflush: Introduce flush_all_caches()
Peter Zijlstra wrote: > On Mon, Aug 15, 2022 at 09:07:06AM -0700, Davidlohr Bueso wrote: > > diff --git a/arch/x86/include/asm/cacheflush.h > > b/arch/x86/include/asm/cacheflush.h > > index b192d917a6d0..ce2ec9556093 100644 > > --- a/arch/x86/include/asm/cacheflush.h > > +++ b/arch/x86/include/asm/cacheflush.h > > @@ -10,4 +10,7 @@ > > > > void clflush_cache_range(void *addr, unsigned int size); > > > > +#define flush_all_caches() \ > > + do { wbinvd_on_all_cpus(); } while(0) > > + > > This is horrific... we've done our utmost best to remove all WBINVD > usage and here you're adding it back in the most horrible form possible > ?!? > > Please don't do this, do *NOT* use WBINVD. Unfortunately there are a few good options here, and the changelog did not make clear that this is continuing legacy [1], not adding new wbinvd usage. The functionality this is enabling is to be able to instantaneously secure erase potentially terabytes of memory at once and the kernel needs to be sure that none of the data from before the secure is still present in the cache. It is also used when unlocking a memory device where speculative reads and firmware accesses could have cached poison from before the device was unlocked. This capability is typically only used once per-boot (for unlock), or once per bare metal provisioning event (secure erase), like when handing off the system to another tenant. That small scope plus the fact that none of this is available to a VM limits the potential damage. So, similar to the mitigation we did in [2] that did not kill off wbinvd completely, this is limited to specific scenarios and should be disabled in any scenario where wbinvd is painful / forbidden. [1]: 4c6926a23b76 ("acpi/nfit, libnvdimm: Add unlock of nvdimm support for Intel DIMMs") [2]: e2efb6359e62 ("ACPICA: Avoid cache flush inside virtual machines")
Re: [PATCH] arch/cacheflush: Introduce flush_all_caches()
On Mon, Aug 15, 2022 at 09:07:06AM -0700, Davidlohr Bueso wrote: > diff --git a/arch/x86/include/asm/cacheflush.h > b/arch/x86/include/asm/cacheflush.h > index b192d917a6d0..ce2ec9556093 100644 > --- a/arch/x86/include/asm/cacheflush.h > +++ b/arch/x86/include/asm/cacheflush.h > @@ -10,4 +10,7 @@ > > void clflush_cache_range(void *addr, unsigned int size); > > +#define flush_all_caches() \ > + do { wbinvd_on_all_cpus(); } while(0) > + This is horrific... we've done our utmost best to remove all WBINVD usage and here you're adding it back in the most horrible form possible ?!? Please don't do this, do *NOT* use WBINVD.