Re: [PATCH V2] arm64/mm: Create level specific section mappings in map_range()

[Mark Rutland <mark.rutland@arm.com>]

On Mon, Mar 10, 2025 at 03:53:33PM +0100, Ard Biesheuvel wrote:
> On Mon, 10 Mar 2025 at 15:42, Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > On Mon, Mar 10, 2025 at 01:48:53PM +0000, Ryan Roberts wrote:
> > > On 10/03/2025 10:55, Mark Rutland wrote:
> > IIUC the problem is that for a sufficiently large (and aligned) extent
> > of physical memory, we might try to create a block mapping larger than
> > the HW supports?
> 
> Yes. However, this code is only called to map the kernel, the DTB and
> the ID map code and data pages, and so it seems unlikely that we'll
> hit this bug in practice.
> 
> Nonetheless, it is a shortcoming that we should address to avoid
> future surprises.
> 
> > With that in mind, my earlier comment about LPA2 and blocks at level-0
> > is particularly relevant...
> 
> Indeed - at the higher levels, there is quite some variation between
> page sizes, LPA2 vs !LPA2 etc in which level is the highest at which
> block mappings are supported.
> 
> But given the context where this code is used, I'd prefer to simply
> map everything down to level 2 or higher (for 64-bit descriptors)
> rather than add elaborate rules for all these combinations that are
> never exercised in practice. The net result should be identical.

That sounds much better to me!

> > Though TBH, maybe it makes more sense to split this up into separate
> > levels (as with the code in arch/arm64/mm/mmu.c) and handle each level
> > explicitly with a separate function. That way each level's check can be
> > handled within that level's function, and we can use the right types,
> > etc. It feels like that might be better for D128 in future?
> 
> I have a slight inclination towards having a separate routine for
> D128, but I can easily be persuaded otherwise.

FWIW, I don't have strong feelings either way w.r.t. sharing the code
with D128. If it's cleaner for that to be separate, sure, but I'd have
to see the code.

My concern here was the the current recursive nature of the PI
map_range() function makes it hard to have level-specific behaviour, and
means that it's structurally different to most other code in the kernel,
e.g. the alloc_init_pXX() functions in arch/arm64/mm/mmu.c, where we
have separate functions for each of the levels that handle the
level-specific details.

I think that structure of separate functions per level is clearer, even
if it's a bit more code.

Mark.