On July 9, 2025 10:57:37 PM PDT, dan.j.williams@xxxxxxxxx wrote:
>Jonathan Cameron wrote:
>> From: Yicong Yang <yangyicong@xxxxxxxxxxxxx>
>>
>> ARCH_HAS_CPU_CACHE_INVALIDATE_MEMREGION provides the mechanism for
>> invalidate certain memory regions in a cache-incoherent manner.
>> Currently is used by NVIDMM adn CXL memory. This is mainly done
>> by the system component and is implementation define per spec.
>> Provides a method for the platforms register their own invalidate
>> method and implement ARCH_HAS_CPU_CACHE_INVALIDATE_MEMREGION.
>
>Please run spell-check on changelogs.
>
>>
>> Architectures can opt in for this support via
>> CONFIG_GENERIC_CPU_CACHE_INVALIDATE_MEMREGION.
>>
>> Signed-off-by: Yicong Yang <yangyicong@xxxxxxxxxxxxx>
>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
>> ---
>> drivers/base/Kconfig | 3 +++
>> drivers/base/Makefile | 1 +
>> drivers/base/cache.c | 46 ++++++++++++++++++++++++++++++++
>
>I do not understand what any of this has to do with drivers/base/.
>
>See existing cache management memcpy infrastructure in lib/Kconfig.
>
>> include/asm-generic/cacheflush.h | 12 +++++++++
>> 4 files changed, 62 insertions(+)
>>
>> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
>> index 064eb52ff7e2..cc6df87a0a96 100644
>> --- a/drivers/base/Kconfig
>> +++ b/drivers/base/Kconfig
>> @@ -181,6 +181,9 @@ config SYS_HYPERVISOR
>> bool
>> default n
>>
>> +config GENERIC_CPU_CACHE_INVALIDATE_MEMREGION
>> + bool
>> +
>> config GENERIC_CPU_DEVICES
>> bool
>> default n
>> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
>> index 8074a10183dc..0fbfa4300b98 100644
>> --- a/drivers/base/Makefile
>> +++ b/drivers/base/Makefile
>> @@ -26,6 +26,7 @@ obj-$(CONFIG_DEV_COREDUMP) += devcoredump.o
>> obj-$(CONFIG_GENERIC_MSI_IRQ) += platform-msi.o
>> obj-$(CONFIG_GENERIC_ARCH_TOPOLOGY) += arch_topology.o
>> obj-$(CONFIG_GENERIC_ARCH_NUMA) += arch_numa.o
>> +obj-$(CONFIG_GENERIC_CPU_CACHE_INVALIDATE_MEMREGION) += cache.o
>> obj-$(CONFIG_ACPI) += physical_location.o
>>
>> obj-y += test/
>> diff --git a/drivers/base/cache.c b/drivers/base/cache.c
>> new file mode 100644
>> index 000000000000..8d351657bbef
>> --- /dev/null
>> +++ b/drivers/base/cache.c
>> @@ -0,0 +1,46 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Generic support for CPU Cache Invalidate Memregion
>> + */
>> +
>> +#include <linux/spinlock.h>
>> +#include <linux/export.h>
>> +#include <asm/cacheflush.h>
>> +
>> +
>> +static const struct system_cache_flush_method *scfm_data;
>> +DEFINE_SPINLOCK(scfm_lock);
>> +
>> +void generic_set_sys_cache_flush_method(const struct system_cache_flush_method *method)
>> +{
>> + guard(spinlock_irqsave)(&scfm_lock);
>> + if (scfm_data || !method || !method->invalidate_memregion)
>> + return;
>> +
>> + scfm_data = method;
>
>The lock looks unnecessary here, this is just atomic_cmpxchg().
>
>> +}
>> +EXPORT_SYMBOL_GPL(generic_set_sys_cache_flush_method);
>> +
>> +void generic_clr_sys_cache_flush_method(const struct system_cache_flush_method *method)
>> +{
>> + guard(spinlock_irqsave)(&scfm_lock);
>> + if (scfm_data && scfm_data == method)
>> + scfm_data = NULL;
>
>Same here, but really what is missing is a description of the locking
>requirements of cpu_cache_invalidate_memregion().
>
>
>> +}
>> +
>> +int cpu_cache_invalidate_memregion(int res_desc, phys_addr_t start, size_t len)
>> +{
>> + guard(spinlock_irqsave)(&scfm_lock);
>> + if (!scfm_data)
>> + return -EOPNOTSUPP;
>> +
>> + return scfm_data->invalidate_memregion(res_desc, start, len);
>
>Is it really the case that you need to disable interrupts during cache
>operations? For potentially flushing 10s to 100s of gigabytes, is it
>really the case that all archs can support holding interrupts off for
>that event?
>
>A read lock (rcu or rwsem) seems sufficient to maintain registration
>until the invalidate operation completes.
>
>If an arch does need to disable interrupts while it manages caches that
>does not feel like something that should be enforced for everyone at
>this top-level entry point.
>
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cpu_cache_invalidate_memregion, "DEVMEM");
>> +
>> +bool cpu_cache_has_invalidate_memregion(void)
>> +{
>> + guard(spinlock_irqsave)(&scfm_lock);
>> + return !!scfm_data;
>
>Lock seems pointless here.
>
>More concerning is this diverges from the original intent of this
>function which was to disable physical address space manipulation from
>virtual environments.
>
>Now, different archs may have reason to diverge here but the fact that
>the API requirements are non-obvious points at a minimum to missing
>documentation if not missing cross-arch consensus.
>
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cpu_cache_has_invalidate_memregion, "DEVMEM");
>> diff --git a/include/asm-generic/cacheflush.h b/include/asm-generic/cacheflush.h
>> index 7ee8a179d103..87e64295561e 100644
>> --- a/include/asm-generic/cacheflush.h
>> +++ b/include/asm-generic/cacheflush.h
>> @@ -124,4 +124,16 @@ static inline void flush_cache_vunmap(unsigned long start, unsigned long end)
>> } while (0)
>> #endif
>>
>> +#ifdef CONFIG_GENERIC_CPU_CACHE_INVALIDATE_MEMREGION
>> +
>> +struct system_cache_flush_method {
>> + int (*invalidate_memregion)(int res_desc,
>> + phys_addr_t start, size_t len);
>> +};
>
>The whole point of ARCH_HAS facilities is to resolve symbols like this
>at compile time. Why does this need a indirect function call at all?
Yes, blocking interrupts is much like the problem with WBINVD.
More or less, once user space is running, this isn't acceptable.
