Re: [PATCH RFC 10/15] x86: add an arch helper function to invalidate all cache for nvdimm

Re: [PATCH RFC 10/15] x86: add an arch helper function to invalidate all cache for nvdimm

[Jonathan Cameron]

On Tue, 19 Jul 2022 12:07:03 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> On 7/17/2022 10:30 PM, Davidlohr Bueso wrote:
> > On Fri, 15 Jul 2022, Dave Jiang wrote:
> >  
> >> The original implementation to flush all cache after unlocking the 
> >> nvdimm
> >> resides in drivers/acpi/nfit/intel.c. This is a temporary stop gap until
> >> nvdimm with security operations arrives on other archs. With support CXL
> >> pmem supporting security operations, specifically "unlock" dimm, the 
> >> need
> >> for an arch supported helper function to invalidate all CPU cache for
> >> nvdimm has arrived. Remove original implementation from acpi/nfit and 
> >> add
> >> cross arch support for this operation.
> >>
> >> Add CONFIG_ARCH_HAS_NVDIMM_INVAL_CACHE Kconfig and allow x86_64 to 
> >> opt in
> >> and provide the support via wbinvd_on_all_cpus() call.  
> >
> > So the 8.2.9.5.5 bits will also need wbinvd - and I guess arm64 will need
> > its own semantics (iirc there was a flush all call in the past). Cc'ing
> > Jonathan as well.
> >
> > Anyway, I think this call should not be defined in any place other 
> > than core
> > kernel headers, and not in pat/nvdimm. I was trying to make it fit in 
> > smp.h,
> > for example, but conviniently we might be able to hijack 
> > flush_cache_all()
> > for our purposes as of course neither x86-64 arm64 uses it :)
> >
> > And I see this as safe (wrt not adding a big hammer on unaware 
> > drivers) as
> > the 32bit archs that define the call are mostly contained thin their 
> > arch/,
> > and the few in drivers/ are still specific to those archs.
> >
> > Maybe something like the below.  
> 
> Ok. I'll replace my version with yours.

Careful with flush_cache_all(). The stub version in 
include/asm-generic/cacheflush.h has a comment above it that would
need updating at very least (I think).  
Note there 'was' a flush_cache_all() for ARM64, but:
https://patchwork.kernel.org/project/linux-arm-kernel/patch/1429521875-16893-1-git-send-email-mark.rutland@arm.com/

Also, I'm far from sure it will be the right choice on all CXL supporting
architectures.
+CC linux-arch, linux-arm and Arnd.

> 
> 
> >
> > Thanks,
> > Davidlohr
> >
> > ------8<----------------------------------------
> > Subject: [PATCH] arch/x86: define flush_cache_all as global wbinvd
> >
> > With CXL security features, global CPU cache flushing nvdimm
> > requirements are no longer specific to that subsystem, even
> > beyond the scope of security_ops. CXL will need such semantics
> > for features not necessarily limited to persistent memory.
> >
> > So use the flush_cache_all() for the wbinvd across all
> > CPUs on x86. arm64, which is another platform to have CXL
> > support can also define its own semantics here.
> >
> > Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> > ---
> >  arch/x86/Kconfig                  |  1 -
> >  arch/x86/include/asm/cacheflush.h |  5 +++++
> >  arch/x86/mm/pat/set_memory.c      |  8 --------
> >  drivers/acpi/nfit/intel.c         | 11 ++++++-----
> >  drivers/cxl/security.c            |  5 +++--
> >  include/linux/libnvdimm.h         |  9 ---------
> >  6 files changed, 14 insertions(+), 25 deletions(-)
> >
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index 8dbe89eba639..be0b95e51df6 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -83,7 +83,6 @@ config X86
> >     select ARCH_HAS_MEMBARRIER_SYNC_CORE
> >     select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
> >     select ARCH_HAS_PMEM_API        if X86_64
> > -    select ARCH_HAS_NVDIMM_INVAL_CACHE    if X86_64
> >     select ARCH_HAS_PTE_DEVMAP        if X86_64
> >     select ARCH_HAS_PTE_SPECIAL
> >     select ARCH_HAS_UACCESS_FLUSHCACHE    if X86_64
> > diff --git a/arch/x86/include/asm/cacheflush.h 
> > b/arch/x86/include/asm/cacheflush.h
> > index b192d917a6d0..05c79021665d 100644
> > --- a/arch/x86/include/asm/cacheflush.h
> > +++ b/arch/x86/include/asm/cacheflush.h
> > @@ -10,4 +10,9 @@
> >
> >  void clflush_cache_range(void *addr, unsigned int size);
> >
> > +#define flush_cache_all()        \
> > +do {                    \
> > +    wbinvd_on_all_cpus();        \
> > +} while (0)
> > +
> >  #endif /* _ASM_X86_CACHEFLUSH_H */
> > diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> > index e4cd1286deef..1abd5438f126 100644
> > --- a/arch/x86/mm/pat/set_memory.c
> > +++ b/arch/x86/mm/pat/set_memory.c
> > @@ -330,14 +330,6 @@ void arch_invalidate_pmem(void *addr, size_t size)
> >  EXPORT_SYMBOL_GPL(arch_invalidate_pmem);
> >  #endif
> >
> > -#ifdef CONFIG_ARCH_HAS_NVDIMM_INVAL_CACHE
> > -void arch_invalidate_nvdimm_cache(void)
> > -{
> > -    wbinvd_on_all_cpus();
> > -}
> > -EXPORT_SYMBOL_GPL(arch_invalidate_nvdimm_cache);
> > -#endif
> > -
> >  static void __cpa_flush_all(void *arg)
> >  {
> >     unsigned long cache = (unsigned long)arg;
> > diff --git a/drivers/acpi/nfit/intel.c b/drivers/acpi/nfit/intel.c
> > index 242d2e9203e9..1b0ecb4d67e6 100644
> > --- a/drivers/acpi/nfit/intel.c
> > +++ b/drivers/acpi/nfit/intel.c
> > @@ -1,6 +1,7 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >  /* Copyright(c) 2018 Intel Corporation. All rights reserved. */
> >  #include <linux/libnvdimm.h>
> > +#include <linux/cacheflush.h>
> >  #include <linux/ndctl.h>
> >  #include <linux/acpi.h>
> >  #include <asm/smp.h>
> > @@ -226,7 +227,7 @@ static int __maybe_unused 
> > intel_security_unlock(struct nvdimm *nvdimm,
> >     }
> >
> >     /* DIMM unlocked, invalidate all CPU caches before we read it */
> > -    arch_invalidate_nvdimm_cache();
> > +    flush_cache_all();
> >
> >     return 0;
> >  }
> > @@ -296,7 +297,7 @@ static int __maybe_unused 
> > intel_security_erase(struct nvdimm *nvdimm,
> >         return -ENOTTY;
> >
> >     /* flush all cache before we erase DIMM */
> > -    arch_invalidate_nvdimm_cache();
> > +    flush_cache_all();
> >     memcpy(nd_cmd.cmd.passphrase, key->data,
> >             sizeof(nd_cmd.cmd.passphrase));
> >     rc = nvdimm_ctl(nvdimm, ND_CMD_CALL, &nd_cmd, sizeof(nd_cmd), NULL);
> > @@ -316,7 +317,7 @@ static int __maybe_unused 
> > intel_security_erase(struct nvdimm *nvdimm,
> >     }
> >
> >     /* DIMM erased, invalidate all CPU caches before we read it */
> > -    arch_invalidate_nvdimm_cache();
> > +    flush_cache_all();
> >     return 0;
> >  }
> >
> > @@ -353,7 +354,7 @@ static int __maybe_unused 
> > intel_security_query_overwrite(struct nvdimm *nvdimm)
> >     }
> >
> >     /* flush all cache before we make the nvdimms available */
> > -    arch_invalidate_nvdimm_cache();
> > +    flush_cache_all();
> >     return 0;
> >  }
> >
> > @@ -379,7 +380,7 @@ static int __maybe_unused 
> > intel_security_overwrite(struct nvdimm *nvdimm,
> >         return -ENOTTY;
> >
> >     /* flush all cache before we erase DIMM */
> > -    arch_invalidate_nvdimm_cache();
> > +    flush_cache_all();
> >     memcpy(nd_cmd.cmd.passphrase, nkey->data,
> >             sizeof(nd_cmd.cmd.passphrase));
> >     rc = nvdimm_ctl(nvdimm, ND_CMD_CALL, &nd_cmd, sizeof(nd_cmd), NULL);
> > diff --git a/drivers/cxl/security.c b/drivers/cxl/security.c
> > index 3dc04b50afaf..e2977872bf2f 100644
> > --- a/drivers/cxl/security.c
> > +++ b/drivers/cxl/security.c
> > @@ -6,6 +6,7 @@
> >  #include <linux/ndctl.h>
> >  #include <linux/async.h>
> >  #include <linux/slab.h>
> > +#include <linux/cacheflush.h>
> >  #include "cxlmem.h"
> >  #include "cxl.h"
> >
> > @@ -137,7 +138,7 @@ static int cxl_pmem_security_unlock(struct nvdimm 
> > *nvdimm,
> >         return rc;
> >
> >     /* DIMM unlocked, invalidate all CPU caches before we read it */
> > -    arch_invalidate_nvdimm_cache();
> > +    flush_cache_all();
> >     return 0;
> >  }
> >
> > @@ -165,7 +166,7 @@ static int 
> > cxl_pmem_security_passphrase_erase(struct nvdimm *nvdimm,
> >         return rc;
> >
> >     /* DIMM erased, invalidate all CPU caches before we read it */
> > -    arch_invalidate_nvdimm_cache();
> > +    flush_cache_all();
> >     return 0;
> >  }
> >
> > diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
> > index 07e4e7572089..0769afb73380 100644
> > --- a/include/linux/libnvdimm.h
> > +++ b/include/linux/libnvdimm.h
> > @@ -309,13 +309,4 @@ static inline void arch_invalidate_pmem(void 
> > *addr, size_t size)
> >  {
> >  }
> >  #endif
> > -
> > -#ifdef CONFIG_ARCH_HAS_NVDIMM_INVAL_CACHE
> > -void arch_invalidate_nvdimm_cache(void);
> > -#else
> > -static inline void arch_invalidate_nvdimm_cache(void)
> > -{
> > -}
> > -#endif
> > -
> >  #endif /* __LIBNVDIMM_H__ */
> > -- 
> > 2.36.1
> >  
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC 10/15] x86: add an arch helper function to invalidate all cache for nvdimm

[Dave Jiang]

On 8/3/2022 10:37 AM, Jonathan Cameron wrote:
> On Tue, 19 Jul 2022 12:07:03 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
>
>> On 7/17/2022 10:30 PM, Davidlohr Bueso wrote:
>>> On Fri, 15 Jul 2022, Dave Jiang wrote:
>>>   
>>>> The original implementation to flush all cache after unlocking the
>>>> nvdimm
>>>> resides in drivers/acpi/nfit/intel.c. This is a temporary stop gap until
>>>> nvdimm with security operations arrives on other archs. With support CXL
>>>> pmem supporting security operations, specifically "unlock" dimm, the
>>>> need
>>>> for an arch supported helper function to invalidate all CPU cache for
>>>> nvdimm has arrived. Remove original implementation from acpi/nfit and
>>>> add
>>>> cross arch support for this operation.
>>>>
>>>> Add CONFIG_ARCH_HAS_NVDIMM_INVAL_CACHE Kconfig and allow x86_64 to
>>>> opt in
>>>> and provide the support via wbinvd_on_all_cpus() call.
>>> So the 8.2.9.5.5 bits will also need wbinvd - and I guess arm64 will need
>>> its own semantics (iirc there was a flush all call in the past). Cc'ing
>>> Jonathan as well.
>>>
>>> Anyway, I think this call should not be defined in any place other
>>> than core
>>> kernel headers, and not in pat/nvdimm. I was trying to make it fit in
>>> smp.h,
>>> for example, but conviniently we might be able to hijack
>>> flush_cache_all()
>>> for our purposes as of course neither x86-64 arm64 uses it :)
>>>
>>> And I see this as safe (wrt not adding a big hammer on unaware
>>> drivers) as
>>> the 32bit archs that define the call are mostly contained thin their
>>> arch/,
>>> and the few in drivers/ are still specific to those archs.
>>>
>>> Maybe something like the below.
>> Ok. I'll replace my version with yours.
> Careful with flush_cache_all(). The stub version in
> include/asm-generic/cacheflush.h has a comment above it that would
> need updating at very least (I think).
> Note there 'was' a flush_cache_all() for ARM64, but:
> https://patchwork.kernel.org/project/linux-arm-kernel/patch/1429521875-16893-1-git-send-email-mark.rutland@arm.com/


flush_and_invalidate_cache_all() instead given it calls wbinvd on x86? I 
think other archs, at least ARM, those are separate instructions aren't 
they?

>
> Also, I'm far from sure it will be the right choice on all CXL supporting
> architectures.
> +CC linux-arch, linux-arm and Arnd.
>
>>
>>> Thanks,
>>> Davidlohr
>>>
>>> ------8<----------------------------------------
>>> Subject: [PATCH] arch/x86: define flush_cache_all as global wbinvd
>>>
>>> With CXL security features, global CPU cache flushing nvdimm
>>> requirements are no longer specific to that subsystem, even
>>> beyond the scope of security_ops. CXL will need such semantics
>>> for features not necessarily limited to persistent memory.
>>>
>>> So use the flush_cache_all() for the wbinvd across all
>>> CPUs on x86. arm64, which is another platform to have CXL
>>> support can also define its own semantics here.
>>>
>>> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
>>> ---
>>>   arch/x86/Kconfig                  |  1 -
>>>   arch/x86/include/asm/cacheflush.h |  5 +++++
>>>   arch/x86/mm/pat/set_memory.c      |  8 --------
>>>   drivers/acpi/nfit/intel.c         | 11 ++++++-----
>>>   drivers/cxl/security.c            |  5 +++--
>>>   include/linux/libnvdimm.h         |  9 ---------
>>>   6 files changed, 14 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>>> index 8dbe89eba639..be0b95e51df6 100644
>>> --- a/arch/x86/Kconfig
>>> +++ b/arch/x86/Kconfig
>>> @@ -83,7 +83,6 @@ config X86
>>>      select ARCH_HAS_MEMBARRIER_SYNC_CORE
>>>      select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
>>>      select ARCH_HAS_PMEM_API        if X86_64
>>> -    select ARCH_HAS_NVDIMM_INVAL_CACHE    if X86_64
>>>      select ARCH_HAS_PTE_DEVMAP        if X86_64
>>>      select ARCH_HAS_PTE_SPECIAL
>>>      select ARCH_HAS_UACCESS_FLUSHCACHE    if X86_64
>>> diff --git a/arch/x86/include/asm/cacheflush.h
>>> b/arch/x86/include/asm/cacheflush.h
>>> index b192d917a6d0..05c79021665d 100644
>>> --- a/arch/x86/include/asm/cacheflush.h
>>> +++ b/arch/x86/include/asm/cacheflush.h
>>> @@ -10,4 +10,9 @@
>>>
>>>   void clflush_cache_range(void *addr, unsigned int size);
>>>
>>> +#define flush_cache_all()        \
>>> +do {                    \
>>> +    wbinvd_on_all_cpus();        \
>>> +} while (0)
>>> +
>>>   #endif /* _ASM_X86_CACHEFLUSH_H */
>>> diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
>>> index e4cd1286deef..1abd5438f126 100644
>>> --- a/arch/x86/mm/pat/set_memory.c
>>> +++ b/arch/x86/mm/pat/set_memory.c
>>> @@ -330,14 +330,6 @@ void arch_invalidate_pmem(void *addr, size_t size)
>>>   EXPORT_SYMBOL_GPL(arch_invalidate_pmem);
>>>   #endif
>>>
>>> -#ifdef CONFIG_ARCH_HAS_NVDIMM_INVAL_CACHE
>>> -void arch_invalidate_nvdimm_cache(void)
>>> -{
>>> -    wbinvd_on_all_cpus();
>>> -}
>>> -EXPORT_SYMBOL_GPL(arch_invalidate_nvdimm_cache);
>>> -#endif
>>> -
>>>   static void __cpa_flush_all(void *arg)
>>>   {
>>>      unsigned long cache = (unsigned long)arg;
>>> diff --git a/drivers/acpi/nfit/intel.c b/drivers/acpi/nfit/intel.c
>>> index 242d2e9203e9..1b0ecb4d67e6 100644
>>> --- a/drivers/acpi/nfit/intel.c
>>> +++ b/drivers/acpi/nfit/intel.c
>>> @@ -1,6 +1,7 @@
>>>   // SPDX-License-Identifier: GPL-2.0
>>>   /* Copyright(c) 2018 Intel Corporation. All rights reserved. */
>>>   #include <linux/libnvdimm.h>
>>> +#include <linux/cacheflush.h>
>>>   #include <linux/ndctl.h>
>>>   #include <linux/acpi.h>
>>>   #include <asm/smp.h>
>>> @@ -226,7 +227,7 @@ static int __maybe_unused
>>> intel_security_unlock(struct nvdimm *nvdimm,
>>>      }
>>>
>>>      /* DIMM unlocked, invalidate all CPU caches before we read it */
>>> -    arch_invalidate_nvdimm_cache();
>>> +    flush_cache_all();
>>>
>>>      return 0;
>>>   }
>>> @@ -296,7 +297,7 @@ static int __maybe_unused
>>> intel_security_erase(struct nvdimm *nvdimm,
>>>          return -ENOTTY;
>>>
>>>      /* flush all cache before we erase DIMM */
>>> -    arch_invalidate_nvdimm_cache();
>>> +    flush_cache_all();
>>>      memcpy(nd_cmd.cmd.passphrase, key->data,
>>>              sizeof(nd_cmd.cmd.passphrase));
>>>      rc = nvdimm_ctl(nvdimm, ND_CMD_CALL, &nd_cmd, sizeof(nd_cmd), NULL);
>>> @@ -316,7 +317,7 @@ static int __maybe_unused
>>> intel_security_erase(struct nvdimm *nvdimm,
>>>      }
>>>
>>>      /* DIMM erased, invalidate all CPU caches before we read it */
>>> -    arch_invalidate_nvdimm_cache();
>>> +    flush_cache_all();
>>>      return 0;
>>>   }
>>>
>>> @@ -353,7 +354,7 @@ static int __maybe_unused
>>> intel_security_query_overwrite(struct nvdimm *nvdimm)
>>>      }
>>>
>>>      /* flush all cache before we make the nvdimms available */
>>> -    arch_invalidate_nvdimm_cache();
>>> +    flush_cache_all();
>>>      return 0;
>>>   }
>>>
>>> @@ -379,7 +380,7 @@ static int __maybe_unused
>>> intel_security_overwrite(struct nvdimm *nvdimm,
>>>          return -ENOTTY;
>>>
>>>      /* flush all cache before we erase DIMM */
>>> -    arch_invalidate_nvdimm_cache();
>>> +    flush_cache_all();
>>>      memcpy(nd_cmd.cmd.passphrase, nkey->data,
>>>              sizeof(nd_cmd.cmd.passphrase));
>>>      rc = nvdimm_ctl(nvdimm, ND_CMD_CALL, &nd_cmd, sizeof(nd_cmd), NULL);
>>> diff --git a/drivers/cxl/security.c b/drivers/cxl/security.c
>>> index 3dc04b50afaf..e2977872bf2f 100644
>>> --- a/drivers/cxl/security.c
>>> +++ b/drivers/cxl/security.c
>>> @@ -6,6 +6,7 @@
>>>   #include <linux/ndctl.h>
>>>   #include <linux/async.h>
>>>   #include <linux/slab.h>
>>> +#include <linux/cacheflush.h>
>>>   #include "cxlmem.h"
>>>   #include "cxl.h"
>>>
>>> @@ -137,7 +138,7 @@ static int cxl_pmem_security_unlock(struct nvdimm
>>> *nvdimm,
>>>          return rc;
>>>
>>>      /* DIMM unlocked, invalidate all CPU caches before we read it */
>>> -    arch_invalidate_nvdimm_cache();
>>> +    flush_cache_all();
>>>      return 0;
>>>   }
>>>
>>> @@ -165,7 +166,7 @@ static int
>>> cxl_pmem_security_passphrase_erase(struct nvdimm *nvdimm,
>>>          return rc;
>>>
>>>      /* DIMM erased, invalidate all CPU caches before we read it */
>>> -    arch_invalidate_nvdimm_cache();
>>> +    flush_cache_all();
>>>      return 0;
>>>   }
>>>
>>> diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
>>> index 07e4e7572089..0769afb73380 100644
>>> --- a/include/linux/libnvdimm.h
>>> +++ b/include/linux/libnvdimm.h
>>> @@ -309,13 +309,4 @@ static inline void arch_invalidate_pmem(void
>>> *addr, size_t size)
>>>   {
>>>   }
>>>   #endif
>>> -
>>> -#ifdef CONFIG_ARCH_HAS_NVDIMM_INVAL_CACHE
>>> -void arch_invalidate_nvdimm_cache(void);
>>> -#else
>>> -static inline void arch_invalidate_nvdimm_cache(void)
>>> -{
>>> -}
>>> -#endif
>>> -
>>>   #endif /* __LIBNVDIMM_H__ */
>>> -- 
>>> 2.36.1
>>>   

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC 10/15] x86: add an arch helper function to invalidate all cache for nvdimm

[Mark Rutland]

On Tue, Aug 09, 2022 at 02:47:06PM -0700, Dave Jiang wrote:
> 
> On 8/3/2022 10:37 AM, Jonathan Cameron wrote:
> > On Tue, 19 Jul 2022 12:07:03 -0700
> > Dave Jiang <dave.jiang@intel.com> wrote:
> > 
> > > On 7/17/2022 10:30 PM, Davidlohr Bueso wrote:
> > > > On Fri, 15 Jul 2022, Dave Jiang wrote:
> > > > > The original implementation to flush all cache after unlocking the
> > > > > nvdimm
> > > > > resides in drivers/acpi/nfit/intel.c. This is a temporary stop gap until
> > > > > nvdimm with security operations arrives on other archs. With support CXL
> > > > > pmem supporting security operations, specifically "unlock" dimm, the
> > > > > need
> > > > > for an arch supported helper function to invalidate all CPU cache for
> > > > > nvdimm has arrived. Remove original implementation from acpi/nfit and
> > > > > add
> > > > > cross arch support for this operation.
> > > > > 
> > > > > Add CONFIG_ARCH_HAS_NVDIMM_INVAL_CACHE Kconfig and allow x86_64 to
> > > > > opt in
> > > > > and provide the support via wbinvd_on_all_cpus() call.
> > > > So the 8.2.9.5.5 bits will also need wbinvd - and I guess arm64 will need
> > > > its own semantics (iirc there was a flush all call in the past). Cc'ing
> > > > Jonathan as well.
> > > > 
> > > > Anyway, I think this call should not be defined in any place other
> > > > than core
> > > > kernel headers, and not in pat/nvdimm. I was trying to make it fit in
> > > > smp.h,
> > > > for example, but conviniently we might be able to hijack
> > > > flush_cache_all()
> > > > for our purposes as of course neither x86-64 arm64 uses it :)
> > > > 
> > > > And I see this as safe (wrt not adding a big hammer on unaware
> > > > drivers) as
> > > > the 32bit archs that define the call are mostly contained thin their
> > > > arch/,
> > > > and the few in drivers/ are still specific to those archs.
> > > > 
> > > > Maybe something like the below.
> > > Ok. I'll replace my version with yours.
> > Careful with flush_cache_all(). The stub version in
> > include/asm-generic/cacheflush.h has a comment above it that would
> > need updating at very least (I think).
> > Note there 'was' a flush_cache_all() for ARM64, but:
> > https://patchwork.kernel.org/project/linux-arm-kernel/patch/1429521875-16893-1-git-send-email-mark.rutland@arm.com/
> 
> 
> flush_and_invalidate_cache_all() instead given it calls wbinvd on x86? I
> think other archs, at least ARM, those are separate instructions aren't
> they?

On arm and arm64 there is no way to perform maintenance on *all* caches; it has
to be done in cacheline increments by address. It's not realistic to do that
for the entire address space, so we need to know the relevant address ranges
(as per the commit referenced above).

So we probably need to think a bit harder about the geenric interface, since
"all" isn't possible to implement. :/

Thanks,
Mark.

> 
> > 
> > Also, I'm far from sure it will be the right choice on all CXL supporting
> > architectures.
> > +CC linux-arch, linux-arm and Arnd.
> > 
> > > 
> > > > Thanks,
> > > > Davidlohr
> > > > 
> > > > ------8<----------------------------------------
> > > > Subject: [PATCH] arch/x86: define flush_cache_all as global wbinvd
> > > > 
> > > > With CXL security features, global CPU cache flushing nvdimm
> > > > requirements are no longer specific to that subsystem, even
> > > > beyond the scope of security_ops. CXL will need such semantics
> > > > for features not necessarily limited to persistent memory.
> > > > 
> > > > So use the flush_cache_all() for the wbinvd across all
> > > > CPUs on x86. arm64, which is another platform to have CXL
> > > > support can also define its own semantics here.
> > > > 
> > > > Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> > > > ---
> > > >   arch/x86/Kconfig                  |  1 -
> > > >   arch/x86/include/asm/cacheflush.h |  5 +++++
> > > >   arch/x86/mm/pat/set_memory.c      |  8 --------
> > > >   drivers/acpi/nfit/intel.c         | 11 ++++++-----
> > > >   drivers/cxl/security.c            |  5 +++--
> > > >   include/linux/libnvdimm.h         |  9 ---------
> > > >   6 files changed, 14 insertions(+), 25 deletions(-)
> > > > 
> > > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > > > index 8dbe89eba639..be0b95e51df6 100644
> > > > --- a/arch/x86/Kconfig
> > > > +++ b/arch/x86/Kconfig
> > > > @@ -83,7 +83,6 @@ config X86
> > > >      select ARCH_HAS_MEMBARRIER_SYNC_CORE
> > > >      select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
> > > >      select ARCH_HAS_PMEM_API        if X86_64
> > > > -    select ARCH_HAS_NVDIMM_INVAL_CACHE    if X86_64
> > > >      select ARCH_HAS_PTE_DEVMAP        if X86_64
> > > >      select ARCH_HAS_PTE_SPECIAL
> > > >      select ARCH_HAS_UACCESS_FLUSHCACHE    if X86_64
> > > > diff --git a/arch/x86/include/asm/cacheflush.h
> > > > b/arch/x86/include/asm/cacheflush.h
> > > > index b192d917a6d0..05c79021665d 100644
> > > > --- a/arch/x86/include/asm/cacheflush.h
> > > > +++ b/arch/x86/include/asm/cacheflush.h
> > > > @@ -10,4 +10,9 @@
> > > > 
> > > >   void clflush_cache_range(void *addr, unsigned int size);
> > > > 
> > > > +#define flush_cache_all()        \
> > > > +do {                    \
> > > > +    wbinvd_on_all_cpus();        \
> > > > +} while (0)
> > > > +
> > > >   #endif /* _ASM_X86_CACHEFLUSH_H */
> > > > diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> > > > index e4cd1286deef..1abd5438f126 100644
> > > > --- a/arch/x86/mm/pat/set_memory.c
> > > > +++ b/arch/x86/mm/pat/set_memory.c
> > > > @@ -330,14 +330,6 @@ void arch_invalidate_pmem(void *addr, size_t size)
> > > >   EXPORT_SYMBOL_GPL(arch_invalidate_pmem);
> > > >   #endif
> > > > 
> > > > -#ifdef CONFIG_ARCH_HAS_NVDIMM_INVAL_CACHE
> > > > -void arch_invalidate_nvdimm_cache(void)
> > > > -{
> > > > -    wbinvd_on_all_cpus();
> > > > -}
> > > > -EXPORT_SYMBOL_GPL(arch_invalidate_nvdimm_cache);
> > > > -#endif
> > > > -
> > > >   static void __cpa_flush_all(void *arg)
> > > >   {
> > > >      unsigned long cache = (unsigned long)arg;
> > > > diff --git a/drivers/acpi/nfit/intel.c b/drivers/acpi/nfit/intel.c
> > > > index 242d2e9203e9..1b0ecb4d67e6 100644
> > > > --- a/drivers/acpi/nfit/intel.c
> > > > +++ b/drivers/acpi/nfit/intel.c
> > > > @@ -1,6 +1,7 @@
> > > >   // SPDX-License-Identifier: GPL-2.0
> > > >   /* Copyright(c) 2018 Intel Corporation. All rights reserved. */
> > > >   #include <linux/libnvdimm.h>
> > > > +#include <linux/cacheflush.h>
> > > >   #include <linux/ndctl.h>
> > > >   #include <linux/acpi.h>
> > > >   #include <asm/smp.h>
> > > > @@ -226,7 +227,7 @@ static int __maybe_unused
> > > > intel_security_unlock(struct nvdimm *nvdimm,
> > > >      }
> > > > 
> > > >      /* DIMM unlocked, invalidate all CPU caches before we read it */
> > > > -    arch_invalidate_nvdimm_cache();
> > > > +    flush_cache_all();
> > > > 
> > > >      return 0;
> > > >   }
> > > > @@ -296,7 +297,7 @@ static int __maybe_unused
> > > > intel_security_erase(struct nvdimm *nvdimm,
> > > >          return -ENOTTY;
> > > > 
> > > >      /* flush all cache before we erase DIMM */
> > > > -    arch_invalidate_nvdimm_cache();
> > > > +    flush_cache_all();
> > > >      memcpy(nd_cmd.cmd.passphrase, key->data,
> > > >              sizeof(nd_cmd.cmd.passphrase));
> > > >      rc = nvdimm_ctl(nvdimm, ND_CMD_CALL, &nd_cmd, sizeof(nd_cmd), NULL);
> > > > @@ -316,7 +317,7 @@ static int __maybe_unused
> > > > intel_security_erase(struct nvdimm *nvdimm,
> > > >      }
> > > > 
> > > >      /* DIMM erased, invalidate all CPU caches before we read it */
> > > > -    arch_invalidate_nvdimm_cache();
> > > > +    flush_cache_all();
> > > >      return 0;
> > > >   }
> > > > 
> > > > @@ -353,7 +354,7 @@ static int __maybe_unused
> > > > intel_security_query_overwrite(struct nvdimm *nvdimm)
> > > >      }
> > > > 
> > > >      /* flush all cache before we make the nvdimms available */
> > > > -    arch_invalidate_nvdimm_cache();
> > > > +    flush_cache_all();
> > > >      return 0;
> > > >   }
> > > > 
> > > > @@ -379,7 +380,7 @@ static int __maybe_unused
> > > > intel_security_overwrite(struct nvdimm *nvdimm,
> > > >          return -ENOTTY;
> > > > 
> > > >      /* flush all cache before we erase DIMM */
> > > > -    arch_invalidate_nvdimm_cache();
> > > > +    flush_cache_all();
> > > >      memcpy(nd_cmd.cmd.passphrase, nkey->data,
> > > >              sizeof(nd_cmd.cmd.passphrase));
> > > >      rc = nvdimm_ctl(nvdimm, ND_CMD_CALL, &nd_cmd, sizeof(nd_cmd), NULL);
> > > > diff --git a/drivers/cxl/security.c b/drivers/cxl/security.c
> > > > index 3dc04b50afaf..e2977872bf2f 100644
> > > > --- a/drivers/cxl/security.c
> > > > +++ b/drivers/cxl/security.c
> > > > @@ -6,6 +6,7 @@
> > > >   #include <linux/ndctl.h>
> > > >   #include <linux/async.h>
> > > >   #include <linux/slab.h>
> > > > +#include <linux/cacheflush.h>
> > > >   #include "cxlmem.h"
> > > >   #include "cxl.h"
> > > > 
> > > > @@ -137,7 +138,7 @@ static int cxl_pmem_security_unlock(struct nvdimm
> > > > *nvdimm,
> > > >          return rc;
> > > > 
> > > >      /* DIMM unlocked, invalidate all CPU caches before we read it */
> > > > -    arch_invalidate_nvdimm_cache();
> > > > +    flush_cache_all();
> > > >      return 0;
> > > >   }
> > > > 
> > > > @@ -165,7 +166,7 @@ static int
> > > > cxl_pmem_security_passphrase_erase(struct nvdimm *nvdimm,
> > > >          return rc;
> > > > 
> > > >      /* DIMM erased, invalidate all CPU caches before we read it */
> > > > -    arch_invalidate_nvdimm_cache();
> > > > +    flush_cache_all();
> > > >      return 0;
> > > >   }
> > > > 
> > > > diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
> > > > index 07e4e7572089..0769afb73380 100644
> > > > --- a/include/linux/libnvdimm.h
> > > > +++ b/include/linux/libnvdimm.h
> > > > @@ -309,13 +309,4 @@ static inline void arch_invalidate_pmem(void
> > > > *addr, size_t size)
> > > >   {
> > > >   }
> > > >   #endif
> > > > -
> > > > -#ifdef CONFIG_ARCH_HAS_NVDIMM_INVAL_CACHE
> > > > -void arch_invalidate_nvdimm_cache(void);
> > > > -#else
> > > > -static inline void arch_invalidate_nvdimm_cache(void)
> > > > -{
> > > > -}
> > > > -#endif
> > > > -
> > > >   #endif /* __LIBNVDIMM_H__ */
> > > > -- 
> > > > 2.36.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC 10/15] x86: add an arch helper function to invalidate all cache for nvdimm

[Eliot Moss]

On 8/10/2022 10:15 AM, Mark Rutland wrote:
> On Tue, Aug 09, 2022 at 02:47:06PM -0700, Dave Jiang wrote:
>>
>> On 8/3/2022 10:37 AM, Jonathan Cameron wrote:
>>> On Tue, 19 Jul 2022 12:07:03 -0700
>>> Dave Jiang <dave.jiang@intel.com> wrote:
>>>
>>>> On 7/17/2022 10:30 PM, Davidlohr Bueso wrote:
>>>>> On Fri, 15 Jul 2022, Dave Jiang wrote:
>>>>>> The original implementation to flush all cache after unlocking the
>>>>>> nvdimm
>>>>>> resides in drivers/acpi/nfit/intel.c. This is a temporary stop gap until
>>>>>> nvdimm with security operations arrives on other archs. With support CXL
>>>>>> pmem supporting security operations, specifically "unlock" dimm, the
>>>>>> need
>>>>>> for an arch supported helper function to invalidate all CPU cache for
>>>>>> nvdimm has arrived. Remove original implementation from acpi/nfit and
>>>>>> add
>>>>>> cross arch support for this operation.
>>>>>>
>>>>>> Add CONFIG_ARCH_HAS_NVDIMM_INVAL_CACHE Kconfig and allow x86_64 to
>>>>>> opt in
>>>>>> and provide the support via wbinvd_on_all_cpus() call.
>>>>> So the 8.2.9.5.5 bits will also need wbinvd - and I guess arm64 will need
>>>>> its own semantics (iirc there was a flush all call in the past). Cc'ing
>>>>> Jonathan as well.
>>>>>
>>>>> Anyway, I think this call should not be defined in any place other
>>>>> than core
>>>>> kernel headers, and not in pat/nvdimm. I was trying to make it fit in
>>>>> smp.h,
>>>>> for example, but conviniently we might be able to hijack
>>>>> flush_cache_all()
>>>>> for our purposes as of course neither x86-64 arm64 uses it :)
>>>>>
>>>>> And I see this as safe (wrt not adding a big hammer on unaware
>>>>> drivers) as
>>>>> the 32bit archs that define the call are mostly contained thin their
>>>>> arch/,
>>>>> and the few in drivers/ are still specific to those archs.
>>>>>
>>>>> Maybe something like the below.
>>>> Ok. I'll replace my version with yours.
>>> Careful with flush_cache_all(). The stub version in
>>> include/asm-generic/cacheflush.h has a comment above it that would
>>> need updating at very least (I think).
>>> Note there 'was' a flush_cache_all() for ARM64, but:
>>> https://patchwork.kernel.org/project/linux-arm-kernel/patch/1429521875-16893-1-git-send-email-mark.rutland@arm.com/
>>
>>
>> flush_and_invalidate_cache_all() instead given it calls wbinvd on x86? I
>> think other archs, at least ARM, those are separate instructions aren't
>> they?
> 
> On arm and arm64 there is no way to perform maintenance on *all* caches; it has
> to be done in cacheline increments by address. It's not realistic to do that
> for the entire address space, so we need to know the relevant address ranges
> (as per the commit referenced above).
> 
> So we probably need to think a bit harder about the geenric interface, since
> "all" isn't possible to implement. :/

Can you not do flushing by set and way on each cache,
probably working outwards from L1?

Eliot Moss

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC 10/15] x86: add an arch helper function to invalidate all cache for nvdimm

[Mark Rutland]

On Wed, Aug 10, 2022 at 10:31:12AM -0400, Eliot Moss wrote:
> On 8/10/2022 10:15 AM, Mark Rutland wrote:
> > On Tue, Aug 09, 2022 at 02:47:06PM -0700, Dave Jiang wrote:
> > > 
> > > On 8/3/2022 10:37 AM, Jonathan Cameron wrote:
> > > > On Tue, 19 Jul 2022 12:07:03 -0700
> > > > Dave Jiang <dave.jiang@intel.com> wrote:
> > > > 
> > > > > On 7/17/2022 10:30 PM, Davidlohr Bueso wrote:
> > > > > > On Fri, 15 Jul 2022, Dave Jiang wrote:
> > > > > > > The original implementation to flush all cache after unlocking the
> > > > > > > nvdimm
> > > > > > > resides in drivers/acpi/nfit/intel.c. This is a temporary stop gap until
> > > > > > > nvdimm with security operations arrives on other archs. With support CXL
> > > > > > > pmem supporting security operations, specifically "unlock" dimm, the
> > > > > > > need
> > > > > > > for an arch supported helper function to invalidate all CPU cache for
> > > > > > > nvdimm has arrived. Remove original implementation from acpi/nfit and
> > > > > > > add
> > > > > > > cross arch support for this operation.
> > > > > > > 
> > > > > > > Add CONFIG_ARCH_HAS_NVDIMM_INVAL_CACHE Kconfig and allow x86_64 to
> > > > > > > opt in
> > > > > > > and provide the support via wbinvd_on_all_cpus() call.
> > > > > > So the 8.2.9.5.5 bits will also need wbinvd - and I guess arm64 will need
> > > > > > its own semantics (iirc there was a flush all call in the past). Cc'ing
> > > > > > Jonathan as well.
> > > > > > 
> > > > > > Anyway, I think this call should not be defined in any place other
> > > > > > than core
> > > > > > kernel headers, and not in pat/nvdimm. I was trying to make it fit in
> > > > > > smp.h,
> > > > > > for example, but conviniently we might be able to hijack
> > > > > > flush_cache_all()
> > > > > > for our purposes as of course neither x86-64 arm64 uses it :)
> > > > > > 
> > > > > > And I see this as safe (wrt not adding a big hammer on unaware
> > > > > > drivers) as
> > > > > > the 32bit archs that define the call are mostly contained thin their
> > > > > > arch/,
> > > > > > and the few in drivers/ are still specific to those archs.
> > > > > > 
> > > > > > Maybe something like the below.
> > > > > Ok. I'll replace my version with yours.
> > > > Careful with flush_cache_all(). The stub version in
> > > > include/asm-generic/cacheflush.h has a comment above it that would
> > > > need updating at very least (I think).
> > > > Note there 'was' a flush_cache_all() for ARM64, but:
> > > > https://patchwork.kernel.org/project/linux-arm-kernel/patch/1429521875-16893-1-git-send-email-mark.rutland@arm.com/
> > > 
> > > 
> > > flush_and_invalidate_cache_all() instead given it calls wbinvd on x86? I
> > > think other archs, at least ARM, those are separate instructions aren't
> > > they?
> > 
> > On arm and arm64 there is no way to perform maintenance on *all* caches; it has
> > to be done in cacheline increments by address. It's not realistic to do that
> > for the entire address space, so we need to know the relevant address ranges
> > (as per the commit referenced above).
> > 
> > So we probably need to think a bit harder about the geenric interface, since
> > "all" isn't possible to implement. :/
> 
> Can you not do flushing by set and way on each cache,
> probably working outwards from L1?

Unfortunately, for a number of reasons, that doeesn't work. For better or
worse, the *only* way which is guaranteed to work is to do this by address.

If you look at the latest ARM ARM (ARM DDI 0487H.a):

  https://developer.arm.com/documentation/ddi0487/ha/

... on page D4-4754, in the block "Example code for cache maintenance
instructions", there's note with a treatise on this.

The gist is that:

* Set/Way ops are only guaranteed to affect the caches local to the CPU
  issuing them, and are not guaranteed to affect caches owned by other CPUs.

* Set/Way ops are not guaranteed to affect system-level caches, which are
  fairly popular these days (whereas VA ops are required to affect those).

* Set/Way ops race with the natural behaviour of caches (so e.g. a line could
  bounce between layers of cache, or between caches in the system, and avoid
  being operated upon).

So unless you're on a single CPU system, with translation disabled, and you
*know* that there are no system-level caches, you can't rely upon Set/Way ops
to do anything useful.

They're really there for firmware to use for IMPLEMENTATION DEFINED power-up
and power-down sequences, and aren'y useful to portable code.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC 10/15] x86: add an arch helper function to invalidate all cache for nvdimm

[Eliot Moss]

On 8/10/2022 2:09 PM, Mark Rutland wrote:
> On Wed, Aug 10, 2022 at 10:31:12AM -0400, Eliot Moss wrote:
>> On 8/10/2022 10:15 AM, Mark Rutland wrote:
>>> On Tue, Aug 09, 2022 at 02:47:06PM -0700, Dave Jiang wrote:
>>>>
>>>> On 8/3/2022 10:37 AM, Jonathan Cameron wrote:
>>>>> On Tue, 19 Jul 2022 12:07:03 -0700
>>>>> Dave Jiang <dave.jiang@intel.com> wrote:
>>>>>
>>>>>> On 7/17/2022 10:30 PM, Davidlohr Bueso wrote:
>>>>>>> On Fri, 15 Jul 2022, Dave Jiang wrote:
>>>>>>>> The original implementation to flush all cache after unlocking the
>>>>>>>> nvdimm
>>>>>>>> resides in drivers/acpi/nfit/intel.c. This is a temporary stop gap until
>>>>>>>> nvdimm with security operations arrives on other archs. With support CXL
>>>>>>>> pmem supporting security operations, specifically "unlock" dimm, the
>>>>>>>> need
>>>>>>>> for an arch supported helper function to invalidate all CPU cache for
>>>>>>>> nvdimm has arrived. Remove original implementation from acpi/nfit and
>>>>>>>> add
>>>>>>>> cross arch support for this operation.
>>>>>>>>
>>>>>>>> Add CONFIG_ARCH_HAS_NVDIMM_INVAL_CACHE Kconfig and allow x86_64 to
>>>>>>>> opt in
>>>>>>>> and provide the support via wbinvd_on_all_cpus() call.
>>>>>>> So the 8.2.9.5.5 bits will also need wbinvd - and I guess arm64 will need
>>>>>>> its own semantics (iirc there was a flush all call in the past). Cc'ing
>>>>>>> Jonathan as well.
>>>>>>>
>>>>>>> Anyway, I think this call should not be defined in any place other
>>>>>>> than core
>>>>>>> kernel headers, and not in pat/nvdimm. I was trying to make it fit in
>>>>>>> smp.h,
>>>>>>> for example, but conviniently we might be able to hijack
>>>>>>> flush_cache_all()
>>>>>>> for our purposes as of course neither x86-64 arm64 uses it :)
>>>>>>>
>>>>>>> And I see this as safe (wrt not adding a big hammer on unaware
>>>>>>> drivers) as
>>>>>>> the 32bit archs that define the call are mostly contained thin their
>>>>>>> arch/,
>>>>>>> and the few in drivers/ are still specific to those archs.
>>>>>>>
>>>>>>> Maybe something like the below.
>>>>>> Ok. I'll replace my version with yours.
>>>>> Careful with flush_cache_all(). The stub version in
>>>>> include/asm-generic/cacheflush.h has a comment above it that would
>>>>> need updating at very least (I think).
>>>>> Note there 'was' a flush_cache_all() for ARM64, but:
>>>>> https://patchwork.kernel.org/project/linux-arm-kernel/patch/1429521875-16893-1-git-send-email-mark.rutland@arm.com/
>>>>
>>>>
>>>> flush_and_invalidate_cache_all() instead given it calls wbinvd on x86? I
>>>> think other archs, at least ARM, those are separate instructions aren't
>>>> they?
>>>
>>> On arm and arm64 there is no way to perform maintenance on *all* caches; it has
>>> to be done in cacheline increments by address. It's not realistic to do that
>>> for the entire address space, so we need to know the relevant address ranges
>>> (as per the commit referenced above).
>>>
>>> So we probably need to think a bit harder about the geenric interface, since
>>> "all" isn't possible to implement. :/
>>
>> Can you not do flushing by set and way on each cache,
>> probably working outwards from L1?
> 
> Unfortunately, for a number of reasons, that doeesn't work. For better or
> worse, the *only* way which is guaranteed to work is to do this by address.
> 
> If you look at the latest ARM ARM (ARM DDI 0487H.a):
> 
>    https://developer.arm.com/documentation/ddi0487/ha/
> 
> ... on page D4-4754, in the block "Example code for cache maintenance
> instructions", there's note with a treatise on this.
> 
> The gist is that:
> 
> * Set/Way ops are only guaranteed to affect the caches local to the CPU
>    issuing them, and are not guaranteed to affect caches owned by other CPUs.
> 
> * Set/Way ops are not guaranteed to affect system-level caches, which are
>    fairly popular these days (whereas VA ops are required to affect those).
> 
> * Set/Way ops race with the natural behaviour of caches (so e.g. a line could
>    bounce between layers of cache, or between caches in the system, and avoid
>    being operated upon).
> 
> So unless you're on a single CPU system, with translation disabled, and you
> *know* that there are no system-level caches, you can't rely upon Set/Way ops
> to do anything useful.
> 
> They're really there for firmware to use for IMPLEMENTATION DEFINED power-up
> and power-down sequences, and aren'y useful to portable code.

Thanks for the explanation.  Really does seem that
ARM could use the equivalent on wbnoinvd/wbinvd/invd.

Regards - Eliot

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC 10/15] x86: add an arch helper function to invalidate all cache for nvdimm

[Dan Williams]

Mark Rutland wrote:
> On Tue, Aug 09, 2022 at 02:47:06PM -0700, Dave Jiang wrote:
> > 
> > On 8/3/2022 10:37 AM, Jonathan Cameron wrote:
> > > On Tue, 19 Jul 2022 12:07:03 -0700
> > > Dave Jiang <dave.jiang@intel.com> wrote:
> > > 
> > > > On 7/17/2022 10:30 PM, Davidlohr Bueso wrote:
> > > > > On Fri, 15 Jul 2022, Dave Jiang wrote:
> > > > > > The original implementation to flush all cache after unlocking the
> > > > > > nvdimm
> > > > > > resides in drivers/acpi/nfit/intel.c. This is a temporary stop gap until
> > > > > > nvdimm with security operations arrives on other archs. With support CXL
> > > > > > pmem supporting security operations, specifically "unlock" dimm, the
> > > > > > need
> > > > > > for an arch supported helper function to invalidate all CPU cache for
> > > > > > nvdimm has arrived. Remove original implementation from acpi/nfit and
> > > > > > add
> > > > > > cross arch support for this operation.
> > > > > > 
> > > > > > Add CONFIG_ARCH_HAS_NVDIMM_INVAL_CACHE Kconfig and allow x86_64 to
> > > > > > opt in
> > > > > > and provide the support via wbinvd_on_all_cpus() call.
> > > > > So the 8.2.9.5.5 bits will also need wbinvd - and I guess arm64 will need
> > > > > its own semantics (iirc there was a flush all call in the past). Cc'ing
> > > > > Jonathan as well.
> > > > > 
> > > > > Anyway, I think this call should not be defined in any place other
> > > > > than core
> > > > > kernel headers, and not in pat/nvdimm. I was trying to make it fit in
> > > > > smp.h,
> > > > > for example, but conviniently we might be able to hijack
> > > > > flush_cache_all()
> > > > > for our purposes as of course neither x86-64 arm64 uses it :)
> > > > > 
> > > > > And I see this as safe (wrt not adding a big hammer on unaware
> > > > > drivers) as
> > > > > the 32bit archs that define the call are mostly contained thin their
> > > > > arch/,
> > > > > and the few in drivers/ are still specific to those archs.
> > > > > 
> > > > > Maybe something like the below.
> > > > Ok. I'll replace my version with yours.
> > > Careful with flush_cache_all(). The stub version in
> > > include/asm-generic/cacheflush.h has a comment above it that would
> > > need updating at very least (I think).
> > > Note there 'was' a flush_cache_all() for ARM64, but:
> > > https://patchwork.kernel.org/project/linux-arm-kernel/patch/1429521875-16893-1-git-send-email-mark.rutland@arm.com/
> > 
> > 
> > flush_and_invalidate_cache_all() instead given it calls wbinvd on x86? I
> > think other archs, at least ARM, those are separate instructions aren't
> > they?
> 
> On arm and arm64 there is no way to perform maintenance on *all* caches; it has
> to be done in cacheline increments by address. It's not realistic to do that
> for the entire address space, so we need to know the relevant address ranges
> (as per the commit referenced above).
> 
> So we probably need to think a bit harder about the geenric interface, since
> "all" isn't possible to implement. :/
> 

I expect the interface would not be in the "flush_cache_" namespace
since those functions are explicitly for virtually tagged caches that
need maintenance on TLB operations that change the VA to PA association.
In this case the cache needs maintenance because the data at the PA
changes. That also means that putting it in the "nvdimm_" namespace is
also wrong because there are provisions in the CXL spec where volatile
memory ranges can also change contents at a given PA, for example caches
might need to be invalidated if software resets the device, but not the
platform.

Something like:

    region_cache_flush(resource_size_t base, resource_size_t n, bool nowait)

...where internally that function can decide if it can rely on an
instruction like wbinvd, use set / way based flushing (if set / way
maintenance can be made to work which sounds like no for arm64), or map
into VA space and loop. If it needs to fall back to that VA-based loop
it might be the case that the caller would want to just fail the
security op rather than suffer the loop latency.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC 10/15] x86: add an arch helper function to invalidate all cache for nvdimm

[Davidlohr Bueso]

On Wed, 10 Aug 2022, Dan Williams wrote:

>I expect the interface would not be in the "flush_cache_" namespace
>since those functions are explicitly for virtually tagged caches that
>need maintenance on TLB operations that change the VA to PA association.
>In this case the cache needs maintenance because the data at the PA
>changes. That also means that putting it in the "nvdimm_" namespace is
>also wrong because there are provisions in the CXL spec where volatile
>memory ranges can also change contents at a given PA, for example caches
>might need to be invalidated if software resets the device, but not the
>platform.
>
>Something like:
>
>    region_cache_flush(resource_size_t base, resource_size_t n, bool nowait)
>
>...where internally that function can decide if it can rely on an
>instruction like wbinvd, use set / way based flushing (if set / way
>maintenance can be made to work which sounds like no for arm64), or map
>into VA space and loop. If it needs to fall back to that VA-based loop
>it might be the case that the caller would want to just fail the
>security op rather than suffer the loop latency.

Yep, I was actually prototyping something similar, but want to still
reuse cacheflush.h machinery and just introduce cache_flush_region()
or whatever name, which returns any error. So all the logic would
just be per-arch, where x86 will do the wbinv and return 0, and arm64
can just do -EINVAL until VA-based is no longer the only way.

Thanks,
Davidlohr

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC 10/15] x86: add an arch helper function to invalidate all cache for nvdimm

[Dan Williams]

Davidlohr Bueso wrote:
> On Wed, 10 Aug 2022, Dan Williams wrote:
> 
> >I expect the interface would not be in the "flush_cache_" namespace
> >since those functions are explicitly for virtually tagged caches that
> >need maintenance on TLB operations that change the VA to PA association.
> >In this case the cache needs maintenance because the data at the PA
> >changes. That also means that putting it in the "nvdimm_" namespace is
> >also wrong because there are provisions in the CXL spec where volatile
> >memory ranges can also change contents at a given PA, for example caches
> >might need to be invalidated if software resets the device, but not the
> >platform.
> >
> >Something like:
> >
> >    region_cache_flush(resource_size_t base, resource_size_t n, bool nowait)
> >
> >...where internally that function can decide if it can rely on an
> >instruction like wbinvd, use set / way based flushing (if set / way
> >maintenance can be made to work which sounds like no for arm64), or map
> >into VA space and loop. If it needs to fall back to that VA-based loop
> >it might be the case that the caller would want to just fail the
> >security op rather than suffer the loop latency.
> 
> Yep, I was actually prototyping something similar, but want to still
> reuse cacheflush.h machinery and just introduce cache_flush_region()
> or whatever name, which returns any error. So all the logic would
> just be per-arch, where x86 will do the wbinv and return 0, and arm64
> can just do -EINVAL until VA-based is no longer the only way.

cache_flush_region() works for me, but I wonder if there should be a
cache_flush_region_capable() call to shut off dependent code early
rather than discovering it at runtime? For example, even archs like x86,
that have wbinvd, have scenarios where wbinvd is prohibited, or painful.
TDX, and virtualization in general, comes to mind.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC 10/15] x86: add an arch helper function to invalidate all cache for nvdimm

[Davidlohr Bueso]

On Wed, 10 Aug 2022, Dan Williams wrote:

>Davidlohr Bueso wrote:
>> On Wed, 10 Aug 2022, Dan Williams wrote:
>>
>> >I expect the interface would not be in the "flush_cache_" namespace
>> >since those functions are explicitly for virtually tagged caches that
>> >need maintenance on TLB operations that change the VA to PA association.
>> >In this case the cache needs maintenance because the data at the PA
>> >changes. That also means that putting it in the "nvdimm_" namespace is
>> >also wrong because there are provisions in the CXL spec where volatile
>> >memory ranges can also change contents at a given PA, for example caches
>> >might need to be invalidated if software resets the device, but not the
>> >platform.
>> >
>> >Something like:
>> >
>> >    region_cache_flush(resource_size_t base, resource_size_t n, bool nowait)
>> >
>> >...where internally that function can decide if it can rely on an
>> >instruction like wbinvd, use set / way based flushing (if set / way
>> >maintenance can be made to work which sounds like no for arm64), or map
>> >into VA space and loop. If it needs to fall back to that VA-based loop
>> >it might be the case that the caller would want to just fail the
>> >security op rather than suffer the loop latency.
>>
>> Yep, I was actually prototyping something similar, but want to still
>> reuse cacheflush.h machinery and just introduce cache_flush_region()
>> or whatever name, which returns any error. So all the logic would
>> just be per-arch, where x86 will do the wbinv and return 0, and arm64
>> can just do -EINVAL until VA-based is no longer the only way.
>
>cache_flush_region() works for me, but I wonder if there should be a
>cache_flush_region_capable() call to shut off dependent code early
>rather than discovering it at runtime? For example, even archs like x86,
>that have wbinvd, have scenarios where wbinvd is prohibited, or painful.
>TDX, and virtualization in general, comes to mind.

Yeah I'm no fan of wbinv, but in these cases (cxl/nvdimm), at least from
the performance angle, I am not worried: the user is explicity doing a
security/cleaning specific op, probably decomisioning, so it's rare and
should not expect better.

Thanks,
Davidlohr

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] arch/cacheflush: Introduce flush_all_caches()

[Davidlohr Bueso]

With CXL security features, global CPU cache flushing nvdimm
requirements are no longer specific to that subsystem, even
beyond the scope of security_ops. CXL will need such semantics
for features not necessarily limited to persistent memory.

While the scope of this is for physical address space, add a
new flush_all_caches() in cacheflush headers such that each
architecture can define it, when capable. For x86 just use the
wbinvd hammer and prevent any other arch from being capable.
While there can be performance penalties or delays response
times, these calls are both rare and explicitly security
related, and therefore become less important.

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---

After a few iterations I circled back to an interface without granularity.
It just doesn't make sense right now to define a range if arm64 will not
support this (won't do VA-based physical address space flushes) and, until
it comes up with consistent caches, security operations will simply be
unsupported.

  arch/x86/include/asm/cacheflush.h |  3 +++
  drivers/acpi/nfit/intel.c         | 41 ++++++++++++++-----------------
  include/asm-generic/cacheflush.h  | 22 +++++++++++++++++
  3 files changed, 43 insertions(+), 23 deletions(-)

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)
+
  #endif /* _ASM_X86_CACHEFLUSH_H */
diff --git a/drivers/acpi/nfit/intel.c b/drivers/acpi/nfit/intel.c
index 8dd792a55730..f2f6c31e6ab7 100644
--- a/drivers/acpi/nfit/intel.c
+++ b/drivers/acpi/nfit/intel.c
@@ -4,6 +4,7 @@
  #include <linux/ndctl.h>
  #include <linux/acpi.h>
  #include <asm/smp.h>
+#include <linux/cacheflush.h>
  #include "intel.h"
  #include "nfit.h"

@@ -190,8 +191,6 @@ static int intel_security_change_key(struct nvdimm *nvdimm,
	}
  }

-static void nvdimm_invalidate_cache(void);
-
  static int __maybe_unused intel_security_unlock(struct nvdimm *nvdimm,
		const struct nvdimm_key_data *key_data)
  {
@@ -210,6 +209,9 @@ static int __maybe_unused intel_security_unlock(struct nvdimm *nvdimm,
	};
	int rc;

+	if (!flush_all_caches_capable())
+		return -EINVAL;
+
	if (!test_bit(NVDIMM_INTEL_UNLOCK_UNIT, &nfit_mem->dsm_mask))
		return -ENOTTY;

@@ -228,7 +230,7 @@ static int __maybe_unused intel_security_unlock(struct nvdimm *nvdimm,
	}

	/* DIMM unlocked, invalidate all CPU caches before we read it */
-	nvdimm_invalidate_cache();
+	flush_all_caches();

	return 0;
  }
@@ -294,11 +296,14 @@ static int __maybe_unused intel_security_erase(struct nvdimm *nvdimm,
		},
	};

+	if (!flush_all_caches_capable())
+		return -EINVAL;
+
	if (!test_bit(cmd, &nfit_mem->dsm_mask))
		return -ENOTTY;

	/* flush all cache before we erase DIMM */
-	nvdimm_invalidate_cache();
+	flush_all_caches();
	memcpy(nd_cmd.cmd.passphrase, key->data,
			sizeof(nd_cmd.cmd.passphrase));
	rc = nvdimm_ctl(nvdimm, ND_CMD_CALL, &nd_cmd, sizeof(nd_cmd), NULL);
@@ -318,7 +323,7 @@ static int __maybe_unused intel_security_erase(struct nvdimm *nvdimm,
	}

	/* DIMM erased, invalidate all CPU caches before we read it */
-	nvdimm_invalidate_cache();
+	flush_all_caches();
	return 0;
  }

@@ -338,6 +343,9 @@ static int __maybe_unused intel_security_query_overwrite(struct nvdimm *nvdimm)
		},
	};

+	if (!flush_all_caches_capable())
+		return -EINVAL;
+
	if (!test_bit(NVDIMM_INTEL_QUERY_OVERWRITE, &nfit_mem->dsm_mask))
		return -ENOTTY;

@@ -355,7 +363,7 @@ static int __maybe_unused intel_security_query_overwrite(struct nvdimm *nvdimm)
	}

	/* flush all cache before we make the nvdimms available */
-	nvdimm_invalidate_cache();
+	flush_all_caches();
	return 0;
  }

@@ -377,11 +385,14 @@ static int __maybe_unused intel_security_overwrite(struct nvdimm *nvdimm,
		},
	};

+	if (!flush_all_caches_capable())
+		return -EINVAL;
+
	if (!test_bit(NVDIMM_INTEL_OVERWRITE, &nfit_mem->dsm_mask))
		return -ENOTTY;

	/* flush all cache before we erase DIMM */
-	nvdimm_invalidate_cache();
+	flush_all_caches();
	memcpy(nd_cmd.cmd.passphrase, nkey->data,
			sizeof(nd_cmd.cmd.passphrase));
	rc = nvdimm_ctl(nvdimm, ND_CMD_CALL, &nd_cmd, sizeof(nd_cmd), NULL);
@@ -401,22 +412,6 @@ static int __maybe_unused intel_security_overwrite(struct nvdimm *nvdimm,
	}
  }

-/*
- * TODO: define a cross arch wbinvd equivalent when/if
- * NVDIMM_FAMILY_INTEL command support arrives on another arch.
- */
-#ifdef CONFIG_X86
-static void nvdimm_invalidate_cache(void)
-{
-	wbinvd_on_all_cpus();
-}
-#else
-static void nvdimm_invalidate_cache(void)
-{
-	WARN_ON_ONCE("cache invalidation required after unlock\n");
-}
-#endif
-
  static const struct nvdimm_security_ops __intel_security_ops = {
	.get_flags = intel_security_flags,
	.freeze = intel_security_freeze,
diff --git a/include/asm-generic/cacheflush.h b/include/asm-generic/cacheflush.h
index 4f07afacbc23..f249142b4908 100644
--- a/include/asm-generic/cacheflush.h
+++ b/include/asm-generic/cacheflush.h
@@ -115,4 +115,26 @@ static inline void flush_cache_vunmap(unsigned long start, unsigned long end)
	memcpy(dst, src, len)
  #endif

+/*
+ * Flush the entire caches across all CPUs. It is considered
+ * a big hammer (latency and performance). Unlike the APIs
+ * above, this function can be defined on architectures which
+ * have VIPT or PIPT caches, and thus is beyond the scope of
+ * virtual to physical mappings/page tables changing.
+ *
+ * The limitation here is that the architectures that make
+ * use of it must can actually comply with the semantics,
+ * such as those which caches are in a consistent state. The
+ * caller can verify the situation early on.
+ */
+#ifndef flush_all_caches
+# define flush_all_caches_capable() false
+static inline void flush_all_caches(void)
+{
+	WARN_ON_ONCE("cache invalidation required\n");
+}
+#else
+# define flush_all_caches_capable() true
+#endif
+
  #endif /* _ASM_GENERIC_CACHEFLUSH_H */
--
2.37.2

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arch/cacheflush: Introduce flush_all_caches()

[Peter Zijlstra]

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arch/cacheflush: Introduce flush_all_caches()

[Dan Williams]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arch/cacheflush: Introduce flush_all_caches()

[Davidlohr Bueso]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arch/cacheflush: Introduce flush_all_caches()

[Dan Williams]

On Tue, Aug 16, 2022 at 10:30 AM Davidlohr Bueso <dave@stgolabs.net> 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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arch/cacheflush: Introduce flush_all_caches()

[Davidlohr Bueso]

On Tue, 16 Aug 2022, Dan Williams wrote:

>On Tue, Aug 16, 2022 at 10:30 AM Davidlohr Bueso <dave@stgolabs.net> 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arch/cacheflush: Introduce flush_all_caches()

[Dan Williams]

Davidlohr Bueso wrote:
> On Tue, 16 Aug 2022, Dan Williams wrote:
> 
> >On Tue, Aug 16, 2022 at 10:30 AM Davidlohr Bueso <dave@stgolabs.net> 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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arch/cacheflush: Introduce flush_all_caches()

[Peter Zijlstra]

On Tue, Aug 16, 2022 at 11:49:59AM -0700, Dan Williams wrote:

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

Can we pretty please get those holes fixed ASAP such that future
generations can avoid the WBINVD nonsense?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arch/cacheflush: Introduce flush_all_caches()

[Peter Zijlstra]

On Tue, Aug 16, 2022 at 10:42:03AM -0700, Dan Williams wrote:

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

This; because adding a generic API like this makes it ripe for usage.
And this is absolutely the very last thing we want used.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel