Hi Casey,
On Mon, 4 May 2026 at 21:37, Casey Connolly <[email protected]> wrote:
>
> Currently set_one_region() implicitly assumes that we want to map a
> region and aggressively splits blocks into tables to do this, but when
> called with PTE_TYPE_FAULT to unmap a currently mapped region it may
> try to unnecessarily split blocks which doesn't make sense if the entire
> block should actually be unmapped. In the end it then has to walk every
> single page and create a bunch of empty tables.
>
> Introduce a check for this kind of behaviour and optimise with a fast
> path, if we're unmapping a region >= the size of this entry then we can
> just unmap the entire PTE and whatever it contains.
>
> This fixes some bogus empty tables being left behind when carving out
> reserved memory regions on Qualcomm, and should improve the performance
> of the break-before-make in mmu_change_region_attr().
>
> Signed-off-by: Casey Connolly <[email protected]>
> ---
> Changes in v2:
> - Update pte_type() to correctly check the PTE_TYPE_VALID bit
> - Explicitly unset both bits of PTE_TYPE_MASK to be extra safe
> - V1:
> https://lore.kernel.org/u-boot/[email protected]/
> ---
[...]
>
> + /*
> + * If we're trying to unmap a region then check if it's already
> unmapped or if it's bigger
> + * then the PTE we're looking at right now, in the first case we can
> do nothing and in the
> + * second case we just need to unmap this page/block.
> + * Otherwise we will needlessly create new tables until we have
> traversed every single page
> + * in the region.
> + */
> + if (attrs == PTE_TYPE_FAULT && (pte_type(pte) == PTE_TYPE_FAULT ||
> size >= levelsize)) {
You need an alignment check (with is_aligned) here instead of just the
size and levelsize. But then you have to measure if that makes a
meningful difference on the time it takes to run.
For example imagine you have two 2mb block mapping and the user
requests to unmap 2mb starting at 1mb offset. Something like that:
Mapped memory:
0x0-0x200000
0x0000000-0x4000000
and the user reqeusts to unmap 0x100000-0x300000
The size >= levelsize will succed in this case, but if I am reading
the code right you'll end up unmapping 0-0x200000
Cheers
/Ilias
> + *pte &= ~(PMD_ATTRMASK | PTE_TYPE_MASK);
> + return levelsize;
> + }
> +
> /* Can we can just modify the current level block PTE? */
> if (is_aligned(start, size, levelsize)) {
> if (flag) {
> *pte &= ~PMD_ATTRMASK;
> @@ -1081,8 +1093,12 @@ void mmu_change_region_attr(phys_addr_t addr, size_t
> siz, u64 attrs)
> flush_dcache_range(gd->arch.tlb_addr,
> gd->arch.tlb_addr + gd->arch.tlb_size);
> __asm_invalidate_tlb_all();
>
> + /* If we were unmapping a region then we're done! */
> + if (attrs == PTE_TYPE_FAULT)
> + return;
> +
> mmu_change_region_attr_nobreak(addr, siz, attrs);
> }
>
> int pgprot_set_attrs(phys_addr_t addr, size_t size, enum pgprot_attrs perm)
> --
> 2.53.0
>
