Re: ARM: mmu_early_enable

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello Lior,

On 21.08.23 16:29, Lior Weintraub wrote:
> Hi Sascha,
> 
> After further digging into the mmu_early_enable function I found that level 0 is set with block type (PTE_TYPE_BLOCK) but AFAIK level 0 must have a table type.
> Only level 1-3 can be either table or block but not level 0.
> I think that until now this wasn't an issue because all (I assume) implementations\porting that enabled the use of MMU also gave the barebox a starting address that is within the first 512GB range.

That's most certainly the case. Most implementations even place barebox
in the first 4G to be on the safe side in regard to DMA limitations of
the devices.

> As a result, the current implementation replaced the first entry of level 0 to point into level 1 table.
> In our case, where the memory used is above the first 512GB we got an exception. 
> 
> I suggest that the initial setting of flat 1:1 mapping of all 40 bits will have:
> 2 Entries of Level 0 which point to level1 tables.
> 512 Entries of level 1 each of which define a block of 1GB to map the first 512GB.
> 512 Entries of level 1 each of which define a block of 1GB to map the second 512GB.

How much extra memory would that cost us for the early TTB?

> The reason 40 bits is used (1TB) is because this is what currently barebox set by default for the physical address (see function calc_tcr on mmu_64.h where ips is set to 2).
> The below patch suggestion assumes this will always be the case.
> We can make it more generic by introducing a new settings that will set the total physical address bits.
> We then can derive from this configuration how to set the IPS value and how many tables of level 0 needs to be initialized.
> Let me know what you think.

At least a CONFIG option that can be selected by architectures requiring it
would be apt, I think.

Cheers,
Ahmad

> 
> Cheers,
> Lior.
> 
> 
> diff --git a/arch/arm/cpu/mmu_64.c b/arch/arm/cpu/mmu_64.c
> index cdc4825422..28358dd7d9 100644
> --- a/arch/arm/cpu/mmu_64.c
> +++ b/arch/arm/cpu/mmu_64.c
> @@ -245,6 +245,19 @@ void dma_flush_range(void *ptr, size_t size)
>  	v8_flush_dcache_range(start, end);
>  }
>  
> +void init_range(void *virt_addr, size_t size)
> +{
> +    uint64_t *ttb = get_ttb();
> +    uint64_t addr = (uint64_t)virt_addr;
> +    while(size) {
> +        remap_range((void *)addr, L0_XLAT_SIZE, MAP_UNCACHED);
> +        split_block(ttb,0);
> +        size -= L0_XLAT_SIZE;
> +        addr += L0_XLAT_SIZE;
> +        ttb++;
> +    }
> +}
> +
>  void mmu_early_enable(unsigned long membase, unsigned long memsize)
>  {
>  	int el;
> @@ -257,7 +270,7 @@ void mmu_early_enable(unsigned long membase, unsigned long memsize)
>  
>  	memset((void *)ttb, 0, GRANULE_SIZE);
>  
> -	remap_range(0, 1UL << (BITS_PER_VA - 1), MAP_UNCACHED);
> +    init_range(0, 2*L0_XLAT_SIZE); // Setting 2 entries of 512GB to cover 1TB memory space
>  	remap_range((void *)membase, memsize - OPTEE_SIZE, MAP_CACHED);
>  	remap_range((void *)membase + memsize - OPTEE_SIZE, OPTEE_SIZE, MAP_FAULT);
>  
> 
>   
> 
> 
>> -----Original Message-----
>> From: Lior Weintraub
>> Sent: Thursday, August 17, 2023 4:50 PM
>> To: Sascha Hauer <s.hauer@pengutronix.de>
>> Cc: barebox@lists.infradead.org
>> Subject: RE: ARM: mmu_early_enable
>>
>> Hi Sascha,
>>
>> I read your answer again and realized my misunderstanding.
>> So essentially what you say is that all 48 bits of the address space would be set
>> as flat 1:1 un-cached and only the SRAM region will be set as cached.
>> In that case the UART at address 0xD0_0030_7000 and our ROM at address
>> 0xC0_0400_0000 should be OK to access.
>> We will further investigate and try to figure out what went wrong.
>>
>> Thanks,
>> Lior.
>>
>>
>>> -----Original Message-----
>>> From: Lior Weintraub
>>> Sent: Thursday, August 17, 2023 10:32 AM
>>> To: Sascha Hauer <s.hauer@pengutronix.de>
>>> Cc: barebox@lists.infradead.org
>>> Subject: RE: ARM: mmu_early_enable
>>>
>>> But the UART is set on a total different memory space.
>>> The SRAM where barebox runs and also given as membase and memsize is in
>>> the region of 0xC0_0000_0000
>>> The UART is in the region of 0xD0_0030_7000.
>>>
>>> According the tarmac log, it looks like access to this location caused the
>>> exception:
>>>     3505306 ns  ES  (000000c0000814ac:910003fd) O el3h:	mov	x29,
>>> 	sp 	(console_putc)
>>>                     R X29 (AARCH64) 000000c0 00377ac0
>>>     3505306 ns  ES  (000000c0000814b0:f9400460) O el3h:	ldr	x0,
>>> 	[x3,	#8] 	(console_putc)
>>>                     LD 000000c0000825d0  ........ ........ 000000d0 00307000
>>> S:c0000825d0
>>>                     R X0 (AARCH64) 000000d0 00307000
>>>     3505307 ns  ES  (000000c0000814b4:d63f0040) O el3h:	blr	x2
>>> 	(console_putc)
>>>                     R X30 (AARCH64) 000000c0 000814b8
>>>     3505371 ns  ES  (000000c000000b5c:b9000001) O el3h:	str	w1,
>>> 	[x0] 	(spider_serial_putc)
>>>                     EXC [0x200] Synchronous Current EL with SP_ELx
>>>                     R FAR_EL3 (AARCH64) 000000c0 00000b5c
>>>                     R ESR_EL3 (AARCH64) 8600000f
>>>                     R CPSR 200003cd
>>>                     R SPSR_EL3 (AARCH64) 200003cd
>>>                     R ELR_EL3 (AARCH64) 000000c0 00000b5c
>>>     3505443 ns  ES  (000000c004000a00:14000586) O el3h:	b
>>> 	c004002018	<exception_entry> 	(Vectors)
>>>                     EXC [0x200] Synchronous Current EL with SP_ELx
>>>                     R FAR_EL3 (AARCH64) 000000c0 04000a00
>>>                     R ESR_EL3 (AARCH64) 8600000e
>>>                     R CPSR 200003cd
>>>                     R SPSR_EL3 (AARCH64) 200003cd
>>>                     R ELR_EL3 (AARCH64) 000000c0 04000a00
>>>
>>> BTW, In addition to the UART, there seems to be another issue.
>>> The Vectors themselves are located in our ROM location which also resides
>> on
>>> a different memory area (0xC0_0400_0000 space).
>>> Once the UART access caused an exception, the jump to Vectors caused
>>> another exception so we are in a loop.
>>>
>>> Looks like a catch22 to me.
>>> On one hand the barebox wanted to start clean and disabled the MMU but
>> on
>>> the other hand the mmu_early_enable sets a partial MMU which causes
>>> exceptions.
>>> What do you think needs to be the best solution here?
>>>
>>> Cheers,
>>> Lior.
>>>
>>>> -----Original Message-----
>>>> From: Sascha Hauer <s.hauer@pengutronix.de>
>>>> Sent: Thursday, August 17, 2023 10:18 AM
>>>> To: Lior Weintraub <liorw@pliops.com>
>>>> Cc: barebox@lists.infradead.org
>>>> Subject: Re: ARM: mmu_early_enable
>>>>
>>>> CAUTION: External Sender
>>>>
>>>> On Thu, Aug 17, 2023 at 06:22:50AM +0000, Lior Weintraub wrote:
>>>>> Hi Sascha,
>>>>>
>>>>> I think I found an issue with the CONFIG_MMU feature.
>>>>> When the code under barebox_pbl_start calls mmu_early_enable, the
>>> MMU
>>>>> is set such that only the given SRAM is defined (membase, memsize).
>>>>> But then, if DEBUG_LL is in use and the function pr_debug is called we
>>>>> get an exception because the UART address is not included in the MMU.
>>>>
>>>> That shouldn't happen. See the code in mmu_early_enable():
>>>>
>>>>         early_remap_range(0, 1UL << (BITS_PER_VA - 1), MAP_UNCACHED);
>>>>         early_remap_range(membase, memsize - OPTEE_SIZE, MAP_CACHED);
>>>>         early_remap_range(membase + memsize - OPTEE_SIZE, OPTEE_SIZE,
>>>> MAP_FAULT);
>>>>
>>>> The first line maps the whole address space uncached in a flat 1:1
>>>> mapping. The second and third lines map the SDRAM (SRAM in your case)
>>>> cached.
>>>>
>>>> Your availabe memory is quite small (3MiB) and by skipping the
>>>> relocation your SRAM layout is not standard. Could it be that something
>>>> overwrites your page tables?
>>>>
>>>> Sascha
>>>>
>>>> --
>>>> Pengutronix e.K.                           |                             |
>>>> Steuerwalder Str. 21                       | http://secure-
>>>> web.cisco.com/1OEKFl2BnNpoLNUlGA--QcqTLOmehOhRYUZN-
>>>> THBCB91kVNePMy2om4tD5Nv-
>>>>
>>>
>> _isTZzlwD1lXGQLUMWmqHlBH9dEe0vcctRC7gn_a6v7IxQu5RV7VCRo5Rl7Tylx
>>>> vh1hfoYe3c1lCTbGAEE5kXKVZLdKBs7oNP9Xn4ml3gy7I78-
>>>>
>>>
>> c_QsTMlZ4xNZj06ORqpIvkGFgk72fNMsGjjLXZP6zTk2yEI2gjapDB8ClJ0mVtAl
>>>> oiP9YHbgjuY0qbUbZfQq-UuasUtCi2rRo0Pu2jKm7sqnlCFb16xbdfl-
>>>>
>>>
>> JN9oqUXAy8l3lHq0yGyfhYZnzWTxH/http%3A%2F%2Fwww.pengutronix.de%
>>>> 2F  |
>>>> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
>>>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 
> 
> 

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