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

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

[Caleb Connolly]

This seems to cause crashes on a bunch of Qualcomm platforms. It's safer
to just update the live table and flush it.

Signed-off-by: Caleb Connolly <caleb.connolly@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);
 }
 
 static void add_map(struct mm_region *map)
 {
diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h
index 52f6c9b934d7..475ca97c5c82 100644
--- a/arch/arm/include/asm/system.h
+++ b/arch/arm/include/asm/system.h
@@ -283,11 +283,10 @@ void flush_l3_cache(void);
  * Will be mapped MT_NORMAL & PTE_BLOCK_INNER_SHARE.
  *
  * @start: Start address of the region
  * @size: Size of the region
- * @emerg: Also map the region in the emergency table
  */
-void mmu_map_region(phys_addr_t start, u64 size, bool emerg);
+void mmu_map_region(phys_addr_t start, u64 size);
 void mmu_change_region_attr(phys_addr_t start, size_t size, u64 attrs);
 
 /*
  * smc_call() - issue a secure monitor call
diff --git a/drivers/soc/qcom/cmd-db.c b/drivers/soc/qcom/cmd-db.c
index 67be18e89f4d..4c8bd9abda23 100644
--- a/drivers/soc/qcom/cmd-db.c
+++ b/drivers/soc/qcom/cmd-db.c
@@ -202,9 +202,9 @@ static int cmd_db_bind(struct udevice *dev)
 		return -ENOENT;
 	}
 
 	/* On SM8550/SM8650 and newer SoCs cmd-db might not be mapped */
-	mmu_map_region((phys_addr_t)base, (phys_size_t)size, false);
+	mmu_map_region((phys_addr_t)base, (phys_size_t)size);
 
 	cmd_db_header = base;
 	if (!cmd_db_magic_matches(cmd_db_header)) {
 		log_err("%s: Invalid Command DB Magic\n", __func__);
-- 
2.47.0

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

[Marc Zyngier]

On Wed, 08 Jan 2025 14:22:24 +0000,
Caleb Connolly <caleb.connolly@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.

> 
> Signed-off-by: Caleb Connolly <caleb.connolly@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).

	M.

-- 
Without deviation from the norm, progress is not possible.

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

[Caleb Connolly]

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@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.
> 
>>
>> Signed-off-by: Caleb Connolly <caleb.connolly@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?

Kind regards,
> 
> 	M.
> 

-- 
// Caleb (they/them)

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

[Marc Zyngier]

On Wed, 08 Jan 2025 15:19:07 +0000,
Caleb Connolly <caleb.connolly@linaro.org> 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@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.

I think you should completely characterise the issue before changing
things like this. My hunch is that the so-called "emergency" page
tables do not map the page tables you're modifying. Or something along
those lines. But you really want to get to the bottom of it.

> 
> 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.
> > 
> >>
> >> Signed-off-by: Caleb Connolly <caleb.connolly@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.

Ah, you're obviously right. I can't read today...

But it then begs the question: are the page tables configured to be
accessed by the page-table walker in a non-cacheable fashion? That'd
be rather silly, and the only reason why you'd need any form of CMO at
this point.

> 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?

That's my impression, but each time I look at this code, I feel a bit
sick...

	M.

-- 
Without deviation from the norm, progress is not possible.

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

[Caleb Connolly]

On 08/01/2025 16:42, Marc Zyngier wrote:
> On Wed, 08 Jan 2025 15:19:07 +0000,
> Caleb Connolly <caleb.connolly@linaro.org> 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@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.
> 
> I think you should completely characterise the issue before changing
> things like this. My hunch is that the so-called "emergency" page
> tables do not map the page tables you're modifying. Or something along
> those lines. But you really want to get to the bottom of it.

Well, I was the person who added this function and the only user is a
qualcomm driver which needed this because it accesses some reserved
memory which is sometimes not included in the memory map we get from a
previous bootloader stage.

I guess it happened to work on the board I was using at the time, but on
newer platforms with a different hypervisor or when running in EL2 this
code does not work correctly at all.

I can't really find much good documentation on the specific steps that
should be followed to do what I want to do here, but considering that
this patch works and the current version doesn't it seems like an
improvement at least.

And yes, I'd like to spend more time getting my head around this, I'm
sure it's something I'll return to.


> 
>>
>> 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.
>>>
>>>>
>>>> Signed-off-by: Caleb Connolly <caleb.connolly@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.
> 
> Ah, you're obviously right. I can't read today...
> 
> But it then begs the question: are the page tables configured to be
> accessed by the page-table walker in a non-cacheable fashion? That'd
> be rather silly, and the only reason why you'd need any form of CMO at
> this point.

you're probably right. Barebox has a broader arch_remap_range() function
which seems to be much more robustly implemented, definitely worth
taking things from there going forwards I think.
> 
>> 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?
> 
> That's my impression, but each time I look at this code, I feel a bit
> sick...

Yeah there are a whole lot of problems here and barebox has already
solved a lot of them (thanks Ahmad) so hopefully we can start aligning
with them and improve things on the U-Boot side...

In the meantime, this patch at least partially fixes a function that was
broken from it's inception, alternatively I can remove it and do some
ugly hacks in board code to ensure the region is in the memory map
before we create it.
> 
> 	M.
> 

-- 
// Caleb (they/them)

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

[Neil Armstrong]

On 08/01/2025 16:42, Marc Zyngier wrote:
> On Wed, 08 Jan 2025 15:19:07 +0000,
> Caleb Connolly <caleb.connolly@linaro.org> 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@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.
> 
> I think you should completely characterise the issue before changing
> things like this. My hunch is that the so-called "emergency" page
> tables do not map the page tables you're modifying. Or something along
> those lines. But you really want to get to the bottom of it.

I narrowed to the call to __asm_switch_ttbr() even with the same table
address (so no change), and I can't explain why this doesn't work.

> 
>>
>> 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.
>>>
>>>>
>>>> Signed-off-by: Caleb Connolly <caleb.connolly@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.
> 
> Ah, you're obviously right. I can't read today...
> 
> But it then begs the question: are the page tables configured to be
> accessed by the page-table walker in a non-cacheable fashion? That'd
> be rather silly, and the only reason why you'd need any form of CMO at
> this point.
> 
>> 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?
> 
> That's my impression, but each time I look at this code, I feel a bit
> sick...
> 
> 	M.
> 

Re: [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@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@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

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

[Ahmad Fatoum]

Hi Pierre,

On 08.01.25 16:52, Pierre-Clément Tosi wrote:
> 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@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;

Quoting ARM ARM (L.a) D8.17.1 "Using break-before-make when updating translation
table entries":

  If multiple execution threads use the same translation tables, then when a
  translation table entry is modified in one or more of the following ways,
  the architecture requires software to use a break-before-make sequence: [...]

So I am wondering: Does the break-before-make requirement even apply to a
single-threaded software like U-Boot?

Cheers,
Ahmad

> 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@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


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Marc Zyngier]

On Tue, 14 Jan 2025 08:38:48 +0000,
Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
> 
> Hi Pierre,
> 
> On 08.01.25 16:52, Pierre-Clément Tosi wrote:
> > 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@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;
> 
> Quoting ARM ARM (L.a) D8.17.1 "Using break-before-make when updating translation
> table entries":
> 
>   If multiple execution threads use the same translation tables, then when a
>   translation table entry is modified in one or more of the following ways,
>   the architecture requires software to use a break-before-make sequence: [...]
> 
> So I am wondering: Does the break-before-make requirement even apply to a
> single-threaded software like U-Boot?

Yes, this equally applies, as the page-table walker is a separate
agent in the system, and can *speculatively* walk page tables any time
it wants. These walks are allowed to be cached as TLBs, and this can
result in a single VA translating to multiple PAs.

Don't do that.

	M.

-- 
Without deviation from the norm, progress is not possible.