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

From: Davidlohr Bueso <dave@stgolabs.net>
To: Dan Williams <dan.j.williams@intel.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
	Dave Jiang <dave.jiang@intel.com>,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	linux-cxl@vger.kernel.org, nvdimm@lists.linux.dev,
	bwidawsk@kernel.org, ira.weiny@intel.com,
	vishal.l.verma@intel.com, alison.schofield@intel.com,
	a.manzanares@samsung.com, linux-arch@vger.kernel.org,
	Arnd Bergmann <arnd@arndb.de>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH RFC 10/15] x86: add an arch helper function to invalidate all cache for nvdimm
Date: Wed, 10 Aug 2022 14:31:12 -0700	[thread overview]
Message-ID: <20220810213112.p4fdclh4zbd3vta6@offworld> (raw)
In-Reply-To: <62f4238b5ce8a_3ce6829447@dwillia2-xfh.jf.intel.com.notmuch>

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