[PATCH] armv8: mmu: don't switch to emergency tlb when adding a dynamic mapping

[Pierre-Clément Tosi]

Hi Caleb,

On Wed, Jan 08, 2025 at 04:19:07PM +0100, Caleb Connolly wrote:
> Hi Marc,
> 
> Thanks for your comments.
> 
> On 08/01/2025 16:05, Marc Zyngier wrote:
> > On Wed, 08 Jan 2025 14:22:24 +0000,
> > Caleb Connolly <caleb.connolly at linaro.org> wrote:
> >>
> >> This seems to cause crashes on a bunch of Qualcomm platforms. It's safer
> >> to just update the live table and flush it.
> > 
> > You may want to provide a bit more information, because that's not
> > much to go on, really.
> 
> Best I have is "the board hangs and then resets when trying to switch
> pagetables", I didn't manage to narrow it down exactly, but since it
> seems to work to update the tables without switching that feels like a
> better approach at least for now.
> 
> In hindsight, this mmu_map_region() function is not great in a lot of
> ways, I'm still getting my head around the MMU and I expect this will
> keep being improved.

Yes, [1] should not have used map_range() on live PTs as it assumes that

1. the PTs it modifies aren't live so doesn't perform Break Before Make;
2. the MMU is off so doesn't perform cache maintenance (on AArch64, CPU caches
   can't be enabled without also enabling the MMU)

This patch attempts to (somehow) address 2. but 1. is still an issue anyway.

FYI, see the discussion around [2] which is somewhat related. I suppose at least
adding documentation to clearly state the assumptions map_range() makes would be
helpful, in the short term.

> > 
> >>
> >> Signed-off-by: Caleb Connolly <caleb.connolly at linaro.org>
> >> ---
> >>  arch/arm/cpu/armv8/cache_v8.c | 11 ++---------
> >>  arch/arm/include/asm/system.h |  3 +--
> >>  drivers/soc/qcom/cmd-db.c     |  2 +-
> >>  3 files changed, 4 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c
> >> index e6be6359c5d9..43051d156122 100644
> >> --- a/arch/arm/cpu/armv8/cache_v8.c
> >> +++ b/arch/arm/cpu/armv8/cache_v8.c
> >> @@ -338,9 +338,9 @@ static void map_range(u64 virt, u64 phys, u64 size, int level,
> >>  		size -= next_size;
> >>  	}
> >>  }
> >>  
> >> -void mmu_map_region(phys_addr_t addr, u64 size, bool emergency)
> >> +void mmu_map_region(phys_addr_t addr, u64 size)
> >>  {
> >>  	u64 va_bits;
> >>  	int level = 0;
> >>  	u64 attrs = PTE_BLOCK_MEMTYPE(MT_NORMAL) | PTE_BLOCK_INNER_SHARE;
> >> @@ -350,19 +350,12 @@ void mmu_map_region(phys_addr_t addr, u64 size, bool emergency)
> >>  	get_tcr(NULL, &va_bits);
> >>  	if (va_bits < 39)
> >>  		level = 1;
> >>  
> >> -	if (emergency)
> >> -		map_range(addr, addr, size, level,
> >> -			  (u64 *)gd->arch.tlb_emerg, attrs);
> >> -
> >> -	/* Switch pagetables while we update the primary one */
> >> -	__asm_switch_ttbr(gd->arch.tlb_emerg);
> >> -
> >>  	map_range(addr, addr, size, level,
> >>  		  (u64 *)gd->arch.tlb_addr, attrs);
> >>  
> >> -	__asm_switch_ttbr(gd->arch.tlb_addr);
> >> +	flush_dcache_range(gd->arch.tlb_addr, gd->arch.tlb_size);
> > 
> > Why would you invalidate anything when *mapping* something? By
> > definition, if there was nothing mapped before, there is nothing to
> > invalidate (hint: the architecture forbids negative caching).
> 
> This isn't flushing the region or TLB, it's flushing the translation
> table we just modified.
> 
> I'm wondering if this tlb_addr variable is misnamed, I'm reading ARM
> docs which suggest the TLB is some cache inside(?) the MMU, but this
> tlb_addr in U-Boot actually points to the translation table in memory if
> my understanding is correct?

You're correct that tlb_addr has nothing to do with TLBs. IIRC, it's the
contiguous region (with a size fixed at boot time, distinct from the malloc
region) from which memory holding page tables is allocated.

> 
> Kind regards,
> > 
> > 	M.
> > 
> 
> -- 
> // Caleb (they/them)
> 

[1]: https://lore.kernel.org/u-boot/20240809-b4-snapdragon-improvements-v1-8-7c353f3e8f74@linaro.org/
[2]: https://lore.kernel.org/u-boot/43haokus4jdxguk4arig5tsqcgq2wbezwpbj7oti6pdkvrfual@wa7vz2iypcv5/

HTH,

Pierre