[PATCH] ARM64: mmu: fix mmu_early_enable VA->PA mapping

[PATCH] ARM64: mmu: fix mmu_early_enable VA->PA mapping

[Lior Weintraub]

From 1b8f4ee9e29e722bbb0b7d0f7fed0ae213ef8637 Mon Sep 17 00:00:00 2001
From: Lior Weintraub <liorw@pliops.com>
Date: Mon, 18 Dec 2023 14:01:16 +0200
Subject: [PATCH] ARM64: mmu: fix mmu_early_enable VA->PA mapping

Fix the mmu_early_enable function to correctly map 40bits of virtual address into physical address with a 1:1 mapping.
It uses the init_range function to sets 2 table entries on TTB level0 and then fill level1 with the correct 1:1 mapping.

Signed-off-by: Lior Weintraub <liorw@pliops.com>
---
 arch/arm/cpu/mmu_64.c            | 17 ++++++++++++++++-
 arch/arm/cpu/mmu_64.h            | 19 +++++++++++++++++--
 arch/arm/include/asm/pgtable64.h |  1 +
 3 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/arch/arm/cpu/mmu_64.c b/arch/arm/cpu/mmu_64.c
index c6ea63e655..fe5babbfd9 100644
--- a/arch/arm/cpu/mmu_64.c
+++ b/arch/arm/cpu/mmu_64.c
@@ -294,6 +294,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) {
+		early_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;
@@ -308,7 +321,9 @@ void mmu_early_enable(unsigned long membase, unsigned long memsize)
 
 	memset((void *)ttb, 0, GRANULE_SIZE);
 
-	early_remap_range(0, 1UL << (BITS_PER_VA - 1), MAP_UNCACHED);
+	// Assume maximum BITS_PER_PA set to 40 bits.
+	// Set 1:1 mapping of VA->PA. So to cover the full 1TB range we need 2 tables.
+	init_range(0, 2*L0_XLAT_SIZE);
 	early_remap_range(membase, memsize - OPTEE_SIZE, MAP_CACHED);
 	early_remap_range(membase + memsize - OPTEE_SIZE, OPTEE_SIZE, MAP_FAULT);
 	early_remap_range(PAGE_ALIGN_DOWN((uintptr_t)_stext), PAGE_ALIGN(_etext - _stext), MAP_CACHED);
diff --git a/arch/arm/cpu/mmu_64.h b/arch/arm/cpu/mmu_64.h
index e4d81dace4..e3959e4407 100644
--- a/arch/arm/cpu/mmu_64.h
+++ b/arch/arm/cpu/mmu_64.h
@@ -105,12 +105,27 @@ static inline uint64_t level2mask(int level)
 	return mask;
 }
 
+/**
+ * @brief Returns the TCR (Translation Control Register) value
+ * 
+ * @param el - Exception Level
+ * @param va_bits - Virtual Address bits
+ * @return uint64_t TCR
+ */
 static inline uint64_t calc_tcr(int el, int va_bits)
 {
-	u64 ips;
-	u64 tcr;
+	u64 ips; // Intermediate Physical Address Size
+	u64 tcr; // Translation Control Register
 
+#if (BITS_PER_PA == 40)
 	ips = 2;
+#elif (BITS_PER_PA == 36)
+	ips = 1;
+#elif (BITS_PER_PA == 32)
+	ips = 0;
+#else
+#error "Unsupported"
+#endif
 
 	if (el == 1)
 		tcr = (ips << 32) | TCR_EPD1_DISABLE;
diff --git a/arch/arm/include/asm/pgtable64.h b/arch/arm/include/asm/pgtable64.h
index 21dac30cfe..b88ffe6be5 100644
--- a/arch/arm/include/asm/pgtable64.h
+++ b/arch/arm/include/asm/pgtable64.h
@@ -8,6 +8,7 @@
 
 #define VA_START                   0x0
 #define BITS_PER_VA                48
+#define BITS_PER_PA                40 // Use 40 Physical address bits
 
 /* Granule size of 4KB is being used */
 #define GRANULE_SIZE_SHIFT         12
-- 
2.40.0

[PATCH] fixup! ARM64: mmu: fix mmu_early_enable VA->PA mapping

[Ahmad Fatoum]

Fix warnings and reformat:

  - fix whitespaces in accordance with kernel coding style
  - mark locally-used function static to fix warning
  - remove cast to fix -Wint-conversion warning
  - Use C-style comments as used elsewhere

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 arch/arm/cpu/mmu_64.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/arch/arm/cpu/mmu_64.c b/arch/arm/cpu/mmu_64.c
index e855ae4fa7d4..a392789ab696 100644
--- a/arch/arm/cpu/mmu_64.c
+++ b/arch/arm/cpu/mmu_64.c
@@ -294,12 +294,13 @@ void dma_flush_range(void *ptr, size_t size)
 	v8_flush_dcache_range(start, end);
 }
 
-void init_range(void *virt_addr, size_t size)
+static void init_range(void *virt_addr, size_t size)
 {
 	uint64_t *ttb = get_ttb();
 	uint64_t addr = (uint64_t)virt_addr;
-	while(size) {
-		early_remap_range((void *)addr, L0_XLAT_SIZE, MAP_UNCACHED);
+
+	while (size) {
+		early_remap_range(addr, L0_XLAT_SIZE, MAP_UNCACHED);
 		split_block(ttb,0);
 		size -= L0_XLAT_SIZE;
 		addr += L0_XLAT_SIZE;
@@ -324,9 +325,10 @@ void mmu_early_enable(unsigned long membase, unsigned long memsize)
 
 	memset((void *)ttb, 0, GRANULE_SIZE);
 
-	// Assume maximum BITS_PER_PA set to 40 bits.
-	// Set 1:1 mapping of VA->PA. So to cover the full 1TB range we need 2 tables.
-	init_range(0, 2*L0_XLAT_SIZE);
+	/* Assuming maximum BITS_PER_PA set to 40 bits, set 1:1 mapping
+	 * of VA->PA. To cover the full 1TiB range we need 2 tables.
+	 */
+	init_range(0, 2 * L0_XLAT_SIZE);
 	early_remap_range(membase, memsize - OPTEE_SIZE, MAP_CACHED);
 	early_remap_range(membase + memsize - OPTEE_SIZE, OPTEE_SIZE, MAP_FAULT);
 	early_remap_range(PAGE_ALIGN_DOWN((uintptr_t)_stext), PAGE_ALIGN(_etext - _stext), MAP_CACHED);
-- 
2.39.2

Re: [PATCH] ARM64: mmu: fix mmu_early_enable VA->PA mapping

[Ahmad Fatoum]

On 18.12.23 13:05, Lior Weintraub wrote:
> From 1b8f4ee9e29e722bbb0b7d0f7fed0ae213ef8637 Mon Sep 17 00:00:00 2001
> From: Lior Weintraub <liorw@pliops.com>
> Date: Mon, 18 Dec 2023 14:01:16 +0200
> Subject: [PATCH] ARM64: mmu: fix mmu_early_enable VA->PA mapping
> 
> Fix the mmu_early_enable function to correctly map 40bits of virtual address into physical address with a 1:1 mapping.
> It uses the init_range function to sets 2 table entries on TTB level0 and then fill level1 with the correct 1:1 mapping.
> 
> Signed-off-by: Lior Weintraub <liorw@pliops.com>

Tested-by: Ahmad Fatoum <a.fatoum@pengutronix.de> # Qemu ARM64 Virt

I ran into some warnings when building, for which I sent out a patch
just now. I think Sascha can squash them if there are no further
comments.

Cheers,
Ahmad

> ---
>  arch/arm/cpu/mmu_64.c            | 17 ++++++++++++++++-
>  arch/arm/cpu/mmu_64.h            | 19 +++++++++++++++++--
>  arch/arm/include/asm/pgtable64.h |  1 +
>  3 files changed, 34 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/cpu/mmu_64.c b/arch/arm/cpu/mmu_64.c
> index c6ea63e655..fe5babbfd9 100644
> --- a/arch/arm/cpu/mmu_64.c
> +++ b/arch/arm/cpu/mmu_64.c
> @@ -294,6 +294,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) {
> +		early_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;
> @@ -308,7 +321,9 @@ void mmu_early_enable(unsigned long membase, unsigned long memsize)
>  
>  	memset((void *)ttb, 0, GRANULE_SIZE);
>  
> -	early_remap_range(0, 1UL << (BITS_PER_VA - 1), MAP_UNCACHED);
> +	// Assume maximum BITS_PER_PA set to 40 bits.
> +	// Set 1:1 mapping of VA->PA. So to cover the full 1TB range we need 2 tables.
> +	init_range(0, 2*L0_XLAT_SIZE);
>  	early_remap_range(membase, memsize - OPTEE_SIZE, MAP_CACHED);
>  	early_remap_range(membase + memsize - OPTEE_SIZE, OPTEE_SIZE, MAP_FAULT);
>  	early_remap_range(PAGE_ALIGN_DOWN((uintptr_t)_stext), PAGE_ALIGN(_etext - _stext), MAP_CACHED);
> diff --git a/arch/arm/cpu/mmu_64.h b/arch/arm/cpu/mmu_64.h
> index e4d81dace4..e3959e4407 100644
> --- a/arch/arm/cpu/mmu_64.h
> +++ b/arch/arm/cpu/mmu_64.h
> @@ -105,12 +105,27 @@ static inline uint64_t level2mask(int level)
>  	return mask;
>  }
>  
> +/**
> + * @brief Returns the TCR (Translation Control Register) value
> + * 
> + * @param el - Exception Level
> + * @param va_bits - Virtual Address bits
> + * @return uint64_t TCR
> + */
>  static inline uint64_t calc_tcr(int el, int va_bits)
>  {
> -	u64 ips;
> -	u64 tcr;
> +	u64 ips; // Intermediate Physical Address Size
> +	u64 tcr; // Translation Control Register
>  
> +#if (BITS_PER_PA == 40)
>  	ips = 2;
> +#elif (BITS_PER_PA == 36)
> +	ips = 1;
> +#elif (BITS_PER_PA == 32)
> +	ips = 0;
> +#else
> +#error "Unsupported"
> +#endif
>  
>  	if (el == 1)
>  		tcr = (ips << 32) | TCR_EPD1_DISABLE;
> diff --git a/arch/arm/include/asm/pgtable64.h b/arch/arm/include/asm/pgtable64.h
> index 21dac30cfe..b88ffe6be5 100644
> --- a/arch/arm/include/asm/pgtable64.h
> +++ b/arch/arm/include/asm/pgtable64.h
> @@ -8,6 +8,7 @@
>  
>  #define VA_START                   0x0
>  #define BITS_PER_VA                48
> +#define BITS_PER_PA                40 // Use 40 Physical address bits
>  
>  /* Granule size of 4KB is being used */
>  #define GRANULE_SIZE_SHIFT         12

-- 
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] ARM64: mmu: fix mmu_early_enable VA->PA mapping

[Lior Weintraub]

Thanks Ahmad,
I think we can make the init_range function clearer:

void init_range(size_t total_level0_tables)
{
	uint64_t *ttb = get_ttb();
	uint64_t addr = 0;
	while(total_level0_tables--) {
		early_remap_range((void *)addr, L0_XLAT_SIZE, MAP_UNCACHED);
		split_block(ttb,0);
		addr += L0_XLAT_SIZE;
		ttb++;
	}
}

Let me know if you want me to prepare a new patch or you can include it in your warning fix patch.
Cheers,
Lior.

> -----Original Message-----
> From: Ahmad Fatoum <a.fatoum@pengutronix.de>
> Sent: Monday, December 18, 2023 2:43 PM
> To: Lior Weintraub <liorw@pliops.com>; barebox@lists.infradead.org
> Subject: Re: [PATCH] ARM64: mmu: fix mmu_early_enable VA->PA mapping
> 
> CAUTION: External Sender
> 
> On 18.12.23 13:05, Lior Weintraub wrote:
> > From 1b8f4ee9e29e722bbb0b7d0f7fed0ae213ef8637 Mon Sep 17
> 00:00:00 2001
> > From: Lior Weintraub <liorw@pliops.com>
> > Date: Mon, 18 Dec 2023 14:01:16 +0200
> > Subject: [PATCH] ARM64: mmu: fix mmu_early_enable VA->PA mapping
> >
> > Fix the mmu_early_enable function to correctly map 40bits of virtual address
> into physical address with a 1:1 mapping.
> > It uses the init_range function to sets 2 table entries on TTB level0 and then
> fill level1 with the correct 1:1 mapping.
> >
> > Signed-off-by: Lior Weintraub <liorw@pliops.com>
> 
> Tested-by: Ahmad Fatoum <a.fatoum@pengutronix.de> # Qemu ARM64 Virt
> 
> I ran into some warnings when building, for which I sent out a patch
> just now. I think Sascha can squash them if there are no further
> comments.
> 
> Cheers,
> Ahmad
> 
> > ---
> >  arch/arm/cpu/mmu_64.c            | 17 ++++++++++++++++-
> >  arch/arm/cpu/mmu_64.h            | 19 +++++++++++++++++--
> >  arch/arm/include/asm/pgtable64.h |  1 +
> >  3 files changed, 34 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm/cpu/mmu_64.c b/arch/arm/cpu/mmu_64.c
> > index c6ea63e655..fe5babbfd9 100644
> > --- a/arch/arm/cpu/mmu_64.c
> > +++ b/arch/arm/cpu/mmu_64.c
> > @@ -294,6 +294,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) {
> > +             early_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;
> > @@ -308,7 +321,9 @@ void mmu_early_enable(unsigned long membase,
> unsigned long memsize)
> >
> >       memset((void *)ttb, 0, GRANULE_SIZE);
> >
> > -     early_remap_range(0, 1UL << (BITS_PER_VA - 1), MAP_UNCACHED);
> > +     // Assume maximum BITS_PER_PA set to 40 bits.
> > +     // Set 1:1 mapping of VA->PA. So to cover the full 1TB range we need 2
> tables.
> > +     init_range(0, 2*L0_XLAT_SIZE);
> >       early_remap_range(membase, memsize - OPTEE_SIZE, MAP_CACHED);
> >       early_remap_range(membase + memsize - OPTEE_SIZE, OPTEE_SIZE,
> MAP_FAULT);
> >       early_remap_range(PAGE_ALIGN_DOWN((uintptr_t)_stext),
> PAGE_ALIGN(_etext - _stext), MAP_CACHED);
> > diff --git a/arch/arm/cpu/mmu_64.h b/arch/arm/cpu/mmu_64.h
> > index e4d81dace4..e3959e4407 100644
> > --- a/arch/arm/cpu/mmu_64.h
> > +++ b/arch/arm/cpu/mmu_64.h
> > @@ -105,12 +105,27 @@ static inline uint64_t level2mask(int level)
> >       return mask;
> >  }
> >
> > +/**
> > + * @brief Returns the TCR (Translation Control Register) value
> > + *
> > + * @param el - Exception Level
> > + * @param va_bits - Virtual Address bits
> > + * @return uint64_t TCR
> > + */
> >  static inline uint64_t calc_tcr(int el, int va_bits)
> >  {
> > -     u64 ips;
> > -     u64 tcr;
> > +     u64 ips; // Intermediate Physical Address Size
> > +     u64 tcr; // Translation Control Register
> >
> > +#if (BITS_PER_PA == 40)
> >       ips = 2;
> > +#elif (BITS_PER_PA == 36)
> > +     ips = 1;
> > +#elif (BITS_PER_PA == 32)
> > +     ips = 0;
> > +#else
> > +#error "Unsupported"
> > +#endif
> >
> >       if (el == 1)
> >               tcr = (ips << 32) | TCR_EPD1_DISABLE;
> > diff --git a/arch/arm/include/asm/pgtable64.h
> b/arch/arm/include/asm/pgtable64.h
> > index 21dac30cfe..b88ffe6be5 100644
> > --- a/arch/arm/include/asm/pgtable64.h
> > +++ b/arch/arm/include/asm/pgtable64.h
> > @@ -8,6 +8,7 @@
> >
> >  #define VA_START                   0x0
> >  #define BITS_PER_VA                48
> > +#define BITS_PER_PA                40 // Use 40 Physical address bits
> >
> >  /* Granule size of 4KB is being used */
> >  #define GRANULE_SIZE_SHIFT         12
> 
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://secure-
> web.cisco.com/1liV2eL7d3n7h9kx42p3X8Qq6NBttedq1RKB0Q_cBwnnB04E0
> FnT3ymWJy_tqsUoa8azOvJ9JTfqnc2FfDLTRXSREvCCFZ3lsufbobWiPBANyqld4
> I9K_0gGnupCTpXNidBqwEB8gTvQWpyyZw0rpJK27LxohsK69X8_v4VeWmRF
> RF1OhudU5oAvG5oDz6ZnvYDZmOkP7ywfArbvx4rnTlpqhiBODxTHm3P9WBR
> 0jIl_SaG6zk62bdDRXvumPQV3RlAJEchnQ7HyHskx7byyRGuPQTrzNNtZ_s3-
> Sqq7-mMQ/http%3A%2F%2Fwww.pengutronix.de%2F  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 

ARM: mmu_early_enable

[Lior Weintraub]

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.

Is that a misuse by our side or a potential issue in barebox?
If this is an issue with barebox maybe the solution can be:
Get putc_ctx from console.c and if not zero let mmu_early_enable set this region also (with size 0x1000) as a device type.

Cheers,
Lior.

Re: ARM: mmu_early_enable

[Sascha Hauer]

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://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: ARM: mmu_early_enable

[Lior Weintraub]

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 |

RE: ARM: mmu_early_enable

[Lior Weintraub]

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 |

RE: ARM: mmu_early_enable

[Lior Weintraub]

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

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.

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 |

Re: ARM: mmu_early_enable

[Ahmad Fatoum]

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 |

RE: ARM: mmu_early_enable

[Lior Weintraub]

Hi Ahmad,

The suggested patch would cost an extra 4KB for the second level1 table.
If we allow CONFIG option to set the total PA bits then the extra 4KB tables would double on each additional bit.
So currently PA bits is hard-coded to 40 which covers 1TB space and as a result needs 2 level1 tables.
Setting PA bits to 41 would require a total of 4 level1 tables to cover the 2TB range (so additional 3 tables = 12KB).

I don't know much about other architectures and cannot tell is that CONFIG option is needed or not.
In terms of clean code, I think that if you accept the idea of this patch than it will be cleaner to add a global define of PA_BITS even if there is currently no option to change it on CONFIG.
Just so the 2 pieces of code that assume PA 40 bits could relate to this define.

Cheers,
Lior.

> -----Original Message-----
> From: Ahmad Fatoum <a.fatoum@pengutronix.de>
> Sent: Thursday, September 7, 2023 11:31 AM
> To: Lior Weintraub <liorw@pliops.com>; Sascha Hauer
> <s.hauer@pengutronix.de>
> Cc: barebox@lists.infradead.org
> Subject: Re: ARM: mmu_early_enable
> 
> CAUTION: External Sender
> 
> 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://secure-
> web.cisco.com/1rHhZRaIMgLKBdYtAto-eJv-
> UVrRgCr70rxOmbuyWwvRnouTeJgTuxP_8toYZ2QUHYtscVBHs3UJC9WziVAE
> PJNQ2al3TLzj35iVExhjvjGXQg0zWWCYLcKwXbrdHhKJzQRIIc2rzwW1waZUHs
> YQejj6gEft75EDIxwt0Vqlu_KaA2_ocuxFdmkz4Nc1yBB336Srtlc1HA8NWxuVa
> V9tvMC6Ey8F6Z2_adwYpShKeFxa42FJSo2uuRfmmjpv7bj87vOsK_0h_67u5s
> PFiKY0jX1LoatvDUrlcHTZKdQ5h9gnnJnIQpfgAHrwRz2vZPZMU/http%3A%2F
> %2Fwww.pengutronix.de%2F  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 

[PATCH] [ARM64][MMU] Fix mmu_early_enable VA->PA mapping

[Lior Weintraub]

Hi,

The below patch fixes the mmu_early_enable function to correctly map 40bits of virtual address into physical address with a 1:1 mapping.
It uses the init_range function to sets 2 table entries on TTB level0 and then fill level1 with the correct 1:1 mapping.

This patch was merged from an older Barebox version into the most resent master (Commit: 975acf1bafba2366eb40c5e8d8cb732b53f27aa1).
Since it wasn't tested on the most recent master branch (lack of resources) I would appreciate if someone can test it on a 64bit ARM v8 platform.

IMHO, the old implementation is wrong because:
1. It tries to map the full range of VA (48bits) with 1:1 mapping but there are only maximum of 40 PA bits.
    As a result, there is a wraparound that causes wrong mapping.
2. TTB Level0 cannot have a block descriptor. Only table descriptor.
    According "Learn the architecture - AArch64 memory management", Figure 6-1: Translation table format:
"Each entry is 64 bits and the bottom two bits determine the type of entry.
Notice that some of the table entries are only valid at specific levels. The maximum number of
levels of tables is four, which is why there is no table descriptor for level 3 (or the fourth level),
tables. Similarly, there are no Block descriptors or Page descriptors for level 0. Because level 0
entry covers a large region of virtual address space, it does not make sense to allow blocks."
 
Cheers,
Lior.

From a98fa2bad05721fd4c3ceae4f63eedd90c29c244 Mon Sep 17 00:00:00 2001
From: Lior Weintraub <liorw@pliops.com>
Date: Thu, 14 Dec 2023 17:05:04 +0200
Subject: [PATCH] [ARM64][MMU] Fix mmu_early_enable VA->PA mapping

---
 arch/arm/cpu/mmu_64.c            | 17 ++++++++++++++++-
 arch/arm/cpu/mmu_64.h            | 19 +++++++++++++++++--
 arch/arm/include/asm/pgtable64.h |  1 +
 3 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/arch/arm/cpu/mmu_64.c b/arch/arm/cpu/mmu_64.c
index c6ea63e655..f35c1b5937 100644
--- a/arch/arm/cpu/mmu_64.c
+++ b/arch/arm/cpu/mmu_64.c
@@ -294,6 +294,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;
@@ -308,7 +321,9 @@ void mmu_early_enable(unsigned long membase, unsigned long memsize)
 
 	memset((void *)ttb, 0, GRANULE_SIZE);
 
-	early_remap_range(0, 1UL << (BITS_PER_VA - 1), MAP_UNCACHED);
+	// Assume maximum BITS_PER_PA set to 40 bits.
+	// Set 1:1 mapping of VA->PA. So to cover the full 1TB range we need 2 tables.
+	init_range(0, 2*L0_XLAT_SIZE);
 	early_remap_range(membase, memsize - OPTEE_SIZE, MAP_CACHED);
 	early_remap_range(membase + memsize - OPTEE_SIZE, OPTEE_SIZE, MAP_FAULT);
 	early_remap_range(PAGE_ALIGN_DOWN((uintptr_t)_stext), PAGE_ALIGN(_etext - _stext), MAP_CACHED);
diff --git a/arch/arm/cpu/mmu_64.h b/arch/arm/cpu/mmu_64.h
index e4d81dace4..51d8ad10a2 100644
--- a/arch/arm/cpu/mmu_64.h
+++ b/arch/arm/cpu/mmu_64.h
@@ -105,12 +105,27 @@ static inline uint64_t level2mask(int level)
 	return mask;
 }
 
+/**
+ * @brief Returns the TCR (Translation Control Register) value
+ * 
+ * @param el - Exception Level
+ * @param va_bits - Virtual Address bits
+ * @return uint64_t TCR
+ */
 static inline uint64_t calc_tcr(int el, int va_bits)
 {
-	u64 ips;
-	u64 tcr;
+	u64 ips; // Intermediate Physical Address Size
+	u64 tcr; // Translation Control Register
 
+#if (BITS_PER_PA == 40)
 	ips = 2;
+#elif (BITS_PER_PA == 36)
+	ips = 1;
+#elif (BITS_PER_PA == 32)
+	ips = 0;
+#else
+#error "Unsupported"
+#endif
 
 	if (el == 1)
 		tcr = (ips << 32) | TCR_EPD1_DISABLE;
diff --git a/arch/arm/include/asm/pgtable64.h b/arch/arm/include/asm/pgtable64.h
index 21dac30cfe..b88ffe6be5 100644
--- a/arch/arm/include/asm/pgtable64.h
+++ b/arch/arm/include/asm/pgtable64.h
@@ -8,6 +8,7 @@
 
 #define VA_START                   0x0
 #define BITS_PER_VA                48
+#define BITS_PER_PA                40 // Use 40 Physical address bits
 
 /* Granule size of 4KB is being used */
 #define GRANULE_SIZE_SHIFT         12
-- 
2.40.0


> -----Original Message-----
> From: Lior Weintraub
> Sent: Thursday, September 7, 2023 1:08 PM
> To: Ahmad Fatoum <a.fatoum@pengutronix.de>; Sascha Hauer
> <s.hauer@pengutronix.de>
> Cc: barebox@lists.infradead.org
> Subject: RE: ARM: mmu_early_enable
> 
> Hi Ahmad,
> 
> The suggested patch would cost an extra 4KB for the second level1 table.
> If we allow CONFIG option to set the total PA bits then the extra 4KB tables
> would double on each additional bit.
> So currently PA bits is hard-coded to 40 which covers 1TB space and as a result
> needs 2 level1 tables.
> Setting PA bits to 41 would require a total of 4 level1 tables to cover the 2TB
> range (so additional 3 tables = 12KB).
> 
> I don't know much about other architectures and cannot tell is that CONFIG
> option is needed or not.
> In terms of clean code, I think that if you accept the idea of this patch than it
> will be cleaner to add a global define of PA_BITS even if there is currently no
> option to change it on CONFIG.
> Just so the 2 pieces of code that assume PA 40 bits could relate to this define.
> 
> Cheers,
> Lior.
> 
> > -----Original Message-----
> > From: Ahmad Fatoum <a.fatoum@pengutronix.de>
> > Sent: Thursday, September 7, 2023 11:31 AM
> > To: Lior Weintraub <liorw@pliops.com>; Sascha Hauer
> > <s.hauer@pengutronix.de>
> > Cc: barebox@lists.infradead.org
> > Subject: Re: ARM: mmu_early_enable
> >
> > CAUTION: External Sender
> >
> > 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://secure-
> > web.cisco.com/1rHhZRaIMgLKBdYtAto-eJv-
> >
> UVrRgCr70rxOmbuyWwvRnouTeJgTuxP_8toYZ2QUHYtscVBHs3UJC9WziVAE
> >
> PJNQ2al3TLzj35iVExhjvjGXQg0zWWCYLcKwXbrdHhKJzQRIIc2rzwW1waZUHs
> >
> YQejj6gEft75EDIxwt0Vqlu_KaA2_ocuxFdmkz4Nc1yBB336Srtlc1HA8NWxuVa
> >
> V9tvMC6Ey8F6Z2_adwYpShKeFxa42FJSo2uuRfmmjpv7bj87vOsK_0h_67u5s
> >
> PFiKY0jX1LoatvDUrlcHTZKdQ5h9gnnJnIQpfgAHrwRz2vZPZMU/http%3A%2F
> > %2Fwww.pengutronix.de%2F  |
> > 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> > Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> >

Re: [PATCH] [ARM64][MMU] Fix mmu_early_enable VA->PA mapping

[Sascha Hauer]

On Thu, Dec 14, 2023 at 04:04:31PM +0000, Lior Weintraub wrote:
> Hi,
> 
> The below patch fixes the mmu_early_enable function to correctly map 40bits of virtual address into physical address with a 1:1 mapping.
> It uses the init_range function to sets 2 table entries on TTB level0 and then fill level1 with the correct 1:1 mapping.
> 
> This patch was merged from an older Barebox version into the most resent master (Commit: 975acf1bafba2366eb40c5e8d8cb732b53f27aa1).
> Since it wasn't tested on the most recent master branch (lack of resources) I would appreciate if someone can test it on a 64bit ARM v8 platform.
> 
> IMHO, the old implementation is wrong because:
> 1. It tries to map the full range of VA (48bits) with 1:1 mapping but there are only maximum of 40 PA bits.
>     As a result, there is a wraparound that causes wrong mapping.
> 2. TTB Level0 cannot have a block descriptor. Only table descriptor.
>     According "Learn the architecture - AArch64 memory management", Figure 6-1: Translation table format:
> "Each entry is 64 bits and the bottom two bits determine the type of entry.
> Notice that some of the table entries are only valid at specific levels. The maximum number of
> levels of tables is four, which is why there is no table descriptor for level 3 (or the fourth level),
> tables. Similarly, there are no Block descriptors or Page descriptors for level 0. Because level 0
> entry covers a large region of virtual address space, it does not make sense to allow blocks."
>  
> Cheers,
> Lior.
> 
> From a98fa2bad05721fd4c3ceae4f63eedd90c29c244 Mon Sep 17 00:00:00 2001
> From: Lior Weintraub <liorw@pliops.com>
> Date: Thu, 14 Dec 2023 17:05:04 +0200
> Subject: [PATCH] [ARM64][MMU] Fix mmu_early_enable VA->PA mapping

Applied, thanks

Sascha

> 
> ---
>  arch/arm/cpu/mmu_64.c            | 17 ++++++++++++++++-
>  arch/arm/cpu/mmu_64.h            | 19 +++++++++++++++++--
>  arch/arm/include/asm/pgtable64.h |  1 +
>  3 files changed, 34 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/cpu/mmu_64.c b/arch/arm/cpu/mmu_64.c
> index c6ea63e655..f35c1b5937 100644
> --- a/arch/arm/cpu/mmu_64.c
> +++ b/arch/arm/cpu/mmu_64.c
> @@ -294,6 +294,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;
> @@ -308,7 +321,9 @@ void mmu_early_enable(unsigned long membase, unsigned long memsize)
>  
>  	memset((void *)ttb, 0, GRANULE_SIZE);
>  
> -	early_remap_range(0, 1UL << (BITS_PER_VA - 1), MAP_UNCACHED);
> +	// Assume maximum BITS_PER_PA set to 40 bits.
> +	// Set 1:1 mapping of VA->PA. So to cover the full 1TB range we need 2 tables.
> +	init_range(0, 2*L0_XLAT_SIZE);
>  	early_remap_range(membase, memsize - OPTEE_SIZE, MAP_CACHED);
>  	early_remap_range(membase + memsize - OPTEE_SIZE, OPTEE_SIZE, MAP_FAULT);
>  	early_remap_range(PAGE_ALIGN_DOWN((uintptr_t)_stext), PAGE_ALIGN(_etext - _stext), MAP_CACHED);
> diff --git a/arch/arm/cpu/mmu_64.h b/arch/arm/cpu/mmu_64.h
> index e4d81dace4..51d8ad10a2 100644
> --- a/arch/arm/cpu/mmu_64.h
> +++ b/arch/arm/cpu/mmu_64.h
> @@ -105,12 +105,27 @@ static inline uint64_t level2mask(int level)
>  	return mask;
>  }
>  
> +/**
> + * @brief Returns the TCR (Translation Control Register) value
> + * 
> + * @param el - Exception Level
> + * @param va_bits - Virtual Address bits
> + * @return uint64_t TCR
> + */
>  static inline uint64_t calc_tcr(int el, int va_bits)
>  {
> -	u64 ips;
> -	u64 tcr;
> +	u64 ips; // Intermediate Physical Address Size
> +	u64 tcr; // Translation Control Register
>  
> +#if (BITS_PER_PA == 40)
>  	ips = 2;
> +#elif (BITS_PER_PA == 36)
> +	ips = 1;
> +#elif (BITS_PER_PA == 32)
> +	ips = 0;
> +#else
> +#error "Unsupported"
> +#endif
>  
>  	if (el == 1)
>  		tcr = (ips << 32) | TCR_EPD1_DISABLE;
> diff --git a/arch/arm/include/asm/pgtable64.h b/arch/arm/include/asm/pgtable64.h
> index 21dac30cfe..b88ffe6be5 100644
> --- a/arch/arm/include/asm/pgtable64.h
> +++ b/arch/arm/include/asm/pgtable64.h
> @@ -8,6 +8,7 @@
>  
>  #define VA_START                   0x0
>  #define BITS_PER_VA                48
> +#define BITS_PER_PA                40 // Use 40 Physical address bits
>  
>  /* Granule size of 4KB is being used */
>  #define GRANULE_SIZE_SHIFT         12
> -- 
> 2.40.0
> 
> 
> > -----Original Message-----
> > From: Lior Weintraub
> > Sent: Thursday, September 7, 2023 1:08 PM
> > To: Ahmad Fatoum <a.fatoum@pengutronix.de>; Sascha Hauer
> > <s.hauer@pengutronix.de>
> > Cc: barebox@lists.infradead.org
> > Subject: RE: ARM: mmu_early_enable
> > 
> > Hi Ahmad,
> > 
> > The suggested patch would cost an extra 4KB for the second level1 table.
> > If we allow CONFIG option to set the total PA bits then the extra 4KB tables
> > would double on each additional bit.
> > So currently PA bits is hard-coded to 40 which covers 1TB space and as a result
> > needs 2 level1 tables.
> > Setting PA bits to 41 would require a total of 4 level1 tables to cover the 2TB
> > range (so additional 3 tables = 12KB).
> > 
> > I don't know much about other architectures and cannot tell is that CONFIG
> > option is needed or not.
> > In terms of clean code, I think that if you accept the idea of this patch than it
> > will be cleaner to add a global define of PA_BITS even if there is currently no
> > option to change it on CONFIG.
> > Just so the 2 pieces of code that assume PA 40 bits could relate to this define.
> > 
> > Cheers,
> > Lior.
> > 
> > > -----Original Message-----
> > > From: Ahmad Fatoum <a.fatoum@pengutronix.de>
> > > Sent: Thursday, September 7, 2023 11:31 AM
> > > To: Lior Weintraub <liorw@pliops.com>; Sascha Hauer
> > > <s.hauer@pengutronix.de>
> > > Cc: barebox@lists.infradead.org
> > > Subject: Re: ARM: mmu_early_enable
> > >
> > > CAUTION: External Sender
> > >
> > > 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://secure-
> > > web.cisco.com/1rHhZRaIMgLKBdYtAto-eJv-
> > >
> > UVrRgCr70rxOmbuyWwvRnouTeJgTuxP_8toYZ2QUHYtscVBHs3UJC9WziVAE
> > >
> > PJNQ2al3TLzj35iVExhjvjGXQg0zWWCYLcKwXbrdHhKJzQRIIc2rzwW1waZUHs
> > >
> > YQejj6gEft75EDIxwt0Vqlu_KaA2_ocuxFdmkz4Nc1yBB336Srtlc1HA8NWxuVa
> > >
> > V9tvMC6Ey8F6Z2_adwYpShKeFxa42FJSo2uuRfmmjpv7bj87vOsK_0h_67u5s
> > >
> > PFiKY0jX1LoatvDUrlcHTZKdQ5h9gnnJnIQpfgAHrwRz2vZPZMU/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 |

Re: [PATCH] [ARM64][MMU] Fix mmu_early_enable VA->PA mapping

[Sascha Hauer]

Hi Lior,

On Thu, Dec 14, 2023 at 04:04:31PM +0000, Lior Weintraub wrote:
> Hi,
> 
> The below patch fixes the mmu_early_enable function to correctly map 40bits of virtual address into physical address with a 1:1 mapping.
> It uses the init_range function to sets 2 table entries on TTB level0 and then fill level1 with the correct 1:1 mapping.
> 
> This patch was merged from an older Barebox version into the most resent master (Commit: 975acf1bafba2366eb40c5e8d8cb732b53f27aa1).
> Since it wasn't tested on the most recent master branch (lack of resources) I would appreciate if someone can test it on a 64bit ARM v8 platform.
> 
> IMHO, the old implementation is wrong because:
> 1. It tries to map the full range of VA (48bits) with 1:1 mapping but there are only maximum of 40 PA bits.
>     As a result, there is a wraparound that causes wrong mapping.
> 2. TTB Level0 cannot have a block descriptor. Only table descriptor.
>     According "Learn the architecture - AArch64 memory management", Figure 6-1: Translation table format:
> "Each entry is 64 bits and the bottom two bits determine the type of entry.
> Notice that some of the table entries are only valid at specific levels. The maximum number of
> levels of tables is four, which is why there is no table descriptor for level 3 (or the fourth level),
> tables. Similarly, there are no Block descriptors or Page descriptors for level 0. Because level 0
> entry covers a large region of virtual address space, it does not make sense to allow blocks."
>  
> Cheers,
> Lior.
> 
> From a98fa2bad05721fd4c3ceae4f63eedd90c29c244 Mon Sep 17 00:00:00 2001
> From: Lior Weintraub <liorw@pliops.com>
> Date: Thu, 14 Dec 2023 17:05:04 +0200
> Subject: [PATCH] [ARM64][MMU] Fix mmu_early_enable VA->PA mapping
> 
> ---
>  arch/arm/cpu/mmu_64.c            | 17 ++++++++++++++++-
>  arch/arm/cpu/mmu_64.h            | 19 +++++++++++++++++--
>  arch/arm/include/asm/pgtable64.h |  1 +
>  3 files changed, 34 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/cpu/mmu_64.c b/arch/arm/cpu/mmu_64.c
> index c6ea63e655..f35c1b5937 100644
> --- a/arch/arm/cpu/mmu_64.c
> +++ b/arch/arm/cpu/mmu_64.c
> @@ -294,6 +294,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);

This should be early_remap_range(). remap_range() is not safe to be
called at this early stage and breaks running with qemu:

https://github.com/barebox/barebox/actions/runs/7246849041/job/19739568067

When sending a correct version, could you please add your signed-off-by
to the patch?
Also, please send the patch as a new thread, not in response to another
mail.

Sascha

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