Re: [PATCH v3 04/11] arm64/mm: Add set_memory_device() and set_memory_normal()

[Thierry Reding <thierry.reding@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 6356 bytes --]

On Thu, Jul 09, 2026 at 09:58:01PM +0200, Thierry Reding wrote:
> On Tue, Jul 07, 2026 at 12:27:13PM +0100, Will Deacon wrote:
> > On Mon, Jul 06, 2026 at 03:49:24PM +0200, Thierry Reding wrote:
> > > On Fri, Jul 03, 2026 at 06:13:31PM +0100, Will Deacon wrote:
> > > > On Thu, Jul 02, 2026 at 06:41:23PM +0200, Thierry Reding wrote:
> > > > > On Thu, Jul 02, 2026 at 03:46:44PM +0200, Thierry Reding wrote:
> > > > > > On Thu, Jul 02, 2026 at 10:18:47AM +0100, Will Deacon wrote:
> > > > > > > On Wed, Jul 01, 2026 at 06:08:15PM +0200, Thierry Reding wrote:
> > > > > > > > From: Chun Ng <chunn@nvidia.com>
> > > > > > > > 
> > > > > > > > Add helpers to swap PROT_NORMAL and PROT_DEVICE_nGnRnE protection bits
> > > > > > > > on a kernel-linear-map range.
> > > > > > > 
> > > > > > > That sounds like a really terrible idea. Why is this necessary and how
> > > > > > > does it interact with things like load_unaligned_zeropad()?
> > > > > > 
> > > > > > This is necessary because once the memory controller has walled off the
> > > > > > new memory region the CPU must not access it under any circumstances or
> > > > > > it'll cause the CPU to lock up (I think technically it'll hit an SError
> > > > > > but in practice that just means it'll freeze, as far as I can tell).
> > > > > > 
> > > > > > Probably doesn't interact well at all with load_unaligned_zeropad().
> > > > > > 
> > > > > > > I think you should unmap the memory from the linear map and memremap()
> > > > > > > it instead.
> > > > > > 
> > > > > > Given that the memory can never be accessed by the CPU after the memory
> > > > > > controller locks it down, I don't think we'll even need memremap(). The
> > > > > > only thing we really need is the sg_table we hand out via the DMA BUFs
> > > > > > so that they can be used by device drivers to program their DMA engines
> > > > > > internally.
> > > > > > 
> > > > > > Looking through some of the architecture code around this, shouldn't we
> > > > > > simply be using set_memory_encrypted() and set_memory_decrypted() for
> > > > > > this? While they might've been created for slightly other use-cases,
> > > > > > they seem to be doing exactly what we want (i.e. remove the page range
> > > > > > from the linear mapping and flushing it, or restoring the valid bit and
> > > > > > standard permissions, respectively).
> > > > > 
> > > > > Ah... I guess we can't do it because we're not in a realm world and so
> > > > > the early checks in __set_memory_enc_dec() would return early and turn
> > > > > it into a no-op.
> > > > > 
> > > > > How about if I extract a common helper and provide set_memory_p() and
> > > > > set_memory_np() in terms of those. Those are available on x86 and
> > > > > PowerPC as well, so fairly standard. I suppose at that point we're
> > > > > closer to set_memory_valid().
> > > > 
> > > > Why not just call set_direct_map_invalid_noflush() +
> > > > flush_tlb_kernel_range() for each page? We already have APIs for this.
> > > 
> > > Having a "standard" helper with a fixed and documented purposed seemed
> > > like a preferable approach for this particular case. We also may want to
> > > make the driver that uses this buildable as a module, in which case we'd
> > > need to export these rather low-level APIs. And then there's also the
> > > fact that we typically call this on a rather large region of memory
> > > (usually something like 512 MiB), so doing it page-by-page is rather
> > > suboptimal.
> > > 
> > > > The big challenge I see with any linear map manipulation, however, is
> > > > that it will rely on can_set_direct_map() which likely means you need to
> > > > give up some performance and/or security to make this work. Does memory
> > > > become inaccesible dynamically at runtime? If not, the best bet would
> > > > be to describe it as a carveout in the DT and mark it as "no-map" so
> > > > we avoid mapping it in the first place.
> > > 
> > > VPR exists in two modes: static and resizable. For static VPR we do
> > > exactly that: describe it as carveout in DT with no-map and deal with it
> > > accordingly in the driver. Resizable VPR is for device that have small
> > > amounts of RAM. Content-protected video playback will in the worst case
> > > consume around 1.8 GiB of RAM, so we want to be able to reuse for other
> > > purposes when VPR is unused on those devices. In that case, the memory
> > > is also described as a reserved-memory region in DT, but it is marked as
> > > reusable so that it can be managed by CMA.
> > > 
> > > The resize operation is fairly slow to begin with because we need to
> > > stall the GPU and put it into reset before the operation, then take it
> > > out of reset and resume it afterwards.
> > > 
> > > What kind of performance impact do you expect?
> > 
> > You'll need to measure it, but we've seen reports of double-digit
> > percentage regressions in performance and power. As I said, the problem
> > is that you need to split the linear map to 4k page at runtime to unmap
> > the dynamic carveout, but that isn't something that can be done on most
> > CPUs. Therefore you end up having to use page-granular mappings for the
> > entire thing, similarly to how 'rodata_full' drives can_set_direct_map()
> > and the perf/power hit affects everything.
> 
> The VPR has fairly large alignment restrictions (1 MiB) and we do unmap
> in fairly large chunks (512 MiB currently, but we can change that if it
> is helpful) because we really want to avoid resizing operations, so the
> tradeoff is between frequency of resize vs. potential memory wasted.
> 
> Does that change anything with regards to performance?

Turns out that the system we need this for is very likely going to end
up using 4 KiB pages anyway because it doesn't have a whole lot of RAM
(which is the whole reason we want the VPR to be resizable in the first
place). So it sounds like set_direct_map_*() is a good way forward.

Still, wouldn't it potentially be much faster to unmap entire 2 MiB
blocks at a time if we know the driver guarantees the alignment? Maybe
the changes to add num_pages as an argument to set_direct_map_*() that
Mike mentioned would already be an improvement because it avoids those
gratuitous calls to can_set_direct_map().

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]