[PATCH] arm64: Fix map_range() not splitting mapped blocks

[PATCH] arm64: Fix map_range() not splitting mapped blocks

[Pierre-Clément Tosi]

The implementation of map_range() creates the requested mapping by
walking the page tables, iterating over multiple PTEs and/or descending
into existing table mappings as needed. When doing so, it assumes any
pre-existing valid PTE to be a table mapping. This assumption is wrong
if the platform code attempts to successively map two overlapping ranges
where the latter intersects a block mapping created for the former.

As a result, map_range() treats the existing block mapping as a table
mapping and descends into it i.e. starts interpreting the
previously-mapped range as an array of PTEs, writing to them and
potentially even descending further (extra fun with MMIO ranges!).

Instead, pass any valid non-table mapping to split_block(), which
ensures that it actually was a block mapping (calls panic() otherwise)
before splitting it.

Fixes: 41e2787f5ec4 ("arm64: Reduce add_map() complexity")
Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
---
 arch/arm/cpu/armv8/cache_v8.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c
index 697334086f..57d06f0575 100644
--- a/arch/arm/cpu/armv8/cache_v8.c
+++ b/arch/arm/cpu/armv8/cache_v8.c
@@ -326,6 +326,8 @@ static void map_range(u64 virt, u64 phys, u64 size, int level,
 		/* Going one level down */
 		if (pte_type(&table[i]) == PTE_TYPE_FAULT)
 			set_pte_table(&table[i], create_table());
+		else if (pte_type(&table[i]) != PTE_TYPE_TABLE)
+			split_block(&table[i], level);
 
 		next_table = (u64 *)(table[i] & GENMASK_ULL(47, PAGE_SHIFT));
 		next_size = min(map_size - (virt & (map_size - 1)), size);
-- 
2.44.0.291.gc1ea87d7ee-goog


-- 
Pierre

Re: [PATCH] arm64: Fix map_range() not splitting mapped blocks

[Fabio Estevam]

Hi Pierre,

On Mon, Mar 18, 2024 at 4:35 PM Pierre-Clément Tosi <ptosi@google.com> wrote:
>
> The implementation of map_range() creates the requested mapping by
> walking the page tables, iterating over multiple PTEs and/or descending
> into existing table mappings as needed. When doing so, it assumes any
> pre-existing valid PTE to be a table mapping. This assumption is wrong
> if the platform code attempts to successively map two overlapping ranges
> where the latter intersects a block mapping created for the former.
>
> As a result, map_range() treats the existing block mapping as a table
> mapping and descends into it i.e. starts interpreting the
> previously-mapped range as an array of PTEs, writing to them and
> potentially even descending further (extra fun with MMIO ranges!).
>
> Instead, pass any valid non-table mapping to split_block(), which
> ensures that it actually was a block mapping (calls panic() otherwise)
> before splitting it.
>
> Fixes: 41e2787f5ec4 ("arm64: Reduce add_map() complexity")
> Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>

This fixes the boot regression on colibri-imx8x.

Thanks a lot for your fix!

Tested-by: Fabio Estevam <festevam@gmail.com>

Re: [PATCH] arm64: Fix map_range() not splitting mapped blocks

[Marc Zyngier]

On Mon, 18 Mar 2024 19:35:49 +0000,
Pierre-Clément Tosi <ptosi@google.com> wrote:
> 
> The implementation of map_range() creates the requested mapping by
> walking the page tables, iterating over multiple PTEs and/or descending
> into existing table mappings as needed. When doing so, it assumes any
> pre-existing valid PTE to be a table mapping. This assumption is wrong
> if the platform code attempts to successively map two overlapping ranges
> where the latter intersects a block mapping created for the former.
> 
> As a result, map_range() treats the existing block mapping as a table
> mapping and descends into it i.e. starts interpreting the
> previously-mapped range as an array of PTEs, writing to them and
> potentially even descending further (extra fun with MMIO ranges!).
> 
> Instead, pass any valid non-table mapping to split_block(), which
> ensures that it actually was a block mapping (calls panic() otherwise)
> before splitting it.
> 
> Fixes: 41e2787f5ec4 ("arm64: Reduce add_map() complexity")
> Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
> ---
>  arch/arm/cpu/armv8/cache_v8.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c
> index 697334086f..57d06f0575 100644
> --- a/arch/arm/cpu/armv8/cache_v8.c
> +++ b/arch/arm/cpu/armv8/cache_v8.c
> @@ -326,6 +326,8 @@ static void map_range(u64 virt, u64 phys, u64 size, int level,
>  		/* Going one level down */
>  		if (pte_type(&table[i]) == PTE_TYPE_FAULT)
>  			set_pte_table(&table[i], create_table());
> +		else if (pte_type(&table[i]) != PTE_TYPE_TABLE)
> +			split_block(&table[i], level);
>  
>  		next_table = (u64 *)(table[i] & GENMASK_ULL(47, PAGE_SHIFT));
>  		next_size = min(map_size - (virt & (map_size - 1)), size);

This seems pretty reasonable, thanks for looking into this. However, I
can't help but notice that this is done without any BBM, and no TLBI
either.

Are we guaranteed that the updated page tables are not live at the
point of update?

Thanks,

	M.

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

Re: [PATCH] arm64: Fix map_range() not splitting mapped blocks

[Pierre-Clément Tosi]

Hi Marc,

On Tue, Mar 19, 2024 at 09:43:03AM +0000, Marc Zyngier wrote:
> This seems pretty reasonable, thanks for looking into this. However, I
> can't help but notice that this is done without any BBM, and no TLBI
> either.
> 
> Are we guaranteed that the updated page tables are not live at the
> point of update?

This means gd->arch.tlb_addr pointing to the live PTs during setup_pgtables().

In arch/arm/cpu/armv8, setup_all_pgtables() runs with SCTLR_ELx.M unset.

In arch/arm/cpu/armv8/fsl-layerscape, setup_pgtables() is called twice:

 - early_mmu_setup() calls it with SCTLR_ELx.M unset;
 - final_mmu_setup() overwrites gd->arch.tlb_addr before calling it iff
   CFG_SYS_MEM_RESERVE_SECURE is defined i.e. if CONFIG_SYS_SOC="fsl-layerscape"
   so that <asm/arch-fsl-layerscape/config.h> gets auto-included through
   <include/config.h>.

So can CONFIG_FSL_LAYERSCAPE be set while CONFIG_SYS_SOC != "fsl-layerscape"?

I suppose Fabio and Stefano can answer this and/or help with ensuring that
setup_pgtables() is never called on live PTs.

Thanks,

-- 
Pierre

Re: [PATCH] arm64: Fix map_range() not splitting mapped blocks

[Fabio Estevam]

Hi Pierre,

On Tue, Mar 19, 2024 at 8:39 AM Pierre-Clément Tosi <ptosi@google.com> wrote:

> This means gd->arch.tlb_addr pointing to the live PTs during setup_pgtables().
>
> In arch/arm/cpu/armv8, setup_all_pgtables() runs with SCTLR_ELx.M unset.
>
> In arch/arm/cpu/armv8/fsl-layerscape, setup_pgtables() is called twice:
>
>  - early_mmu_setup() calls it with SCTLR_ELx.M unset;
>  - final_mmu_setup() overwrites gd->arch.tlb_addr before calling it iff
>    CFG_SYS_MEM_RESERVE_SECURE is defined i.e. if CONFIG_SYS_SOC="fsl-layerscape"
>    so that <asm/arch-fsl-layerscape/config.h> gets auto-included through
>    <include/config.h>.
>
> So can CONFIG_FSL_LAYERSCAPE be set while CONFIG_SYS_SOC != "fsl-layerscape"?

No, this cannot happen.

Only the following Layerscape SoCs select CONFIG_FSL_LAYERSCAPE
in arch/arm/cpu/armv8/fsl-layerscape/Kconfig:
LS1012A, LS1028A, LS1043A, LS1046A, LS1088A, LS2080A, LX2162A and LX2160A

I saw the original boot problem with the i.MX8QX.

The i.MX8QX is part of the i.MX family, not the Layerscape family.

> I suppose Fabio and Stefano can answer this and/or help with ensuring that
> setup_pgtables() is never called on live PTs.

Let me know if you need any clarification.

Thanks,

Fabio Estevam

Re: [PATCH] arm64: Fix map_range() not splitting mapped blocks

[Pierre-Clément Tosi]

Hi Fabio,

On Tue, Mar 19, 2024 at 09:13:12AM -0300, Fabio Estevam wrote:
> Hi Pierre,
> 
> On Tue, Mar 19, 2024 at 8:39 AM Pierre-Clément Tosi <ptosi@google.com> wrote:
> 
> > This means gd->arch.tlb_addr pointing to the live PTs during setup_pgtables().
> >
> > In arch/arm/cpu/armv8, setup_all_pgtables() runs with SCTLR_ELx.M unset.
> >
> > In arch/arm/cpu/armv8/fsl-layerscape, setup_pgtables() is called twice:
> >
> >  - early_mmu_setup() calls it with SCTLR_ELx.M unset;
> >  - final_mmu_setup() overwrites gd->arch.tlb_addr before calling it iff
> >    CFG_SYS_MEM_RESERVE_SECURE is defined i.e. if CONFIG_SYS_SOC="fsl-layerscape"
> >    so that <asm/arch-fsl-layerscape/config.h> gets auto-included through
> >    <include/config.h>.
> >
> > So can CONFIG_FSL_LAYERSCAPE be set while CONFIG_SYS_SOC != "fsl-layerscape"?
> 
> No, this cannot happen.

Thanks for confirming.

For clarity, it might then make sense to drop that #ifdef in final_mmu_setup().

> Only the following Layerscape SoCs select CONFIG_FSL_LAYERSCAPE
> in arch/arm/cpu/armv8/fsl-layerscape/Kconfig:
> LS1012A, LS1028A, LS1043A, LS1046A, LS1088A, LS2080A, LX2162A and LX2160A
> 
> I saw the original boot problem with the i.MX8QX.
> 
> The i.MX8QX is part of the i.MX family, not the Layerscape family.

Sure.

To be clear, the concern here was that split_block() doesn't perform what the
CPU architecture requires when modifying page tables that the MMU is using and
the question therefore was: can setup_pgtables() be called on such live PTs?

For most AArch64 U-Boot ports (including the i.MX family), the answer is trivial
because they use the arch code i.e. setup_all_pgtables(). However, as
fsl-layerscape re-implements mmu_setup(), it had to be looked at separately,
hence my question, which you answered above.

HTH,

-- 
Pierre

Re: [PATCH] arm64: Fix map_range() not splitting mapped blocks

[Fabio Estevam]

Hi Tom,

On Tue, Mar 19, 2024 at 9:39 AM Pierre-Clément Tosi <ptosi@google.com> wrote:

> For most AArch64 U-Boot ports (including the i.MX family), the answer is trivial
> because they use the arch code i.e. setup_all_pgtables(). However, as
> fsl-layerscape re-implements mmu_setup(), it had to be looked at separately,
> hence my question, which you answered above.

As Pierre's explanation addresses Marc's concern,
do you think this can go to 2024.01 to fix the boot regression on imx8qxp/8qm?

Thanks

Re: [PATCH] arm64: Fix map_range() not splitting mapped blocks

[Fabio Estevam]

On Fri, Mar 22, 2024 at 4:31 PM Fabio Estevam <festevam@gmail.com> wrote:

> As Pierre's explanation addresses Marc's concern,
> do you think this can go to 2024.01 to fix the boot regression on imx8qxp/8qm?

I meant 2024.04, sorry.

Re: [PATCH] arm64: Fix map_range() not splitting mapped blocks

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 379 bytes --]

On Fri, Mar 22, 2024 at 04:33:03PM -0300, Fabio Estevam wrote:
> On Fri, Mar 22, 2024 at 4:31 PM Fabio Estevam <festevam@gmail.com> wrote:
> 
> > As Pierre's explanation addresses Marc's concern,
> > do you think this can go to 2024.01 to fix the boot regression on imx8qxp/8qm?
> 
> I meant 2024.04, sorry.

How much testing has this seen outside of imx?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] arm64: Fix map_range() not splitting mapped blocks

[Hiago De Franco]

On Mon, Mar 18, 2024 at 04:46:55PM -0300, Fabio Estevam wrote:
> Hi Pierre,
> 
> On Mon, Mar 18, 2024 at 4:35 PM Pierre-Clément Tosi <ptosi@google.com> wrote:
> >
> > The implementation of map_range() creates the requested mapping by
> > walking the page tables, iterating over multiple PTEs and/or descending
> > into existing table mappings as needed. When doing so, it assumes any
> > pre-existing valid PTE to be a table mapping. This assumption is wrong
> > if the platform code attempts to successively map two overlapping ranges
> > where the latter intersects a block mapping created for the former.
> >
> > As a result, map_range() treats the existing block mapping as a table
> > mapping and descends into it i.e. starts interpreting the
> > previously-mapped range as an array of PTEs, writing to them and
> > potentially even descending further (extra fun with MMIO ranges!).
> >
> > Instead, pass any valid non-table mapping to split_block(), which
> > ensures that it actually was a block mapping (calls panic() otherwise)
> > before splitting it.
> >
> > Fixes: 41e2787f5ec4 ("arm64: Reduce add_map() complexity")
> > Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
> 
> This fixes the boot regression on colibri-imx8x.
> 
> Thanks a lot for your fix!
> 
> Tested-by: Fabio Estevam <festevam@gmail.com>

Tested-by: Hiago De Franco <hiago.franco@toradex.com> # Toradex Verdin AM62

On Sat, Mar 23, 2024 at 10:34:54AM -0400, Tom Rini wrote:
> On Fri, Mar 22, 2024 at 04:33:03PM -0300, Fabio Estevam wrote:
> > On Fri, Mar 22, 2024 at 4:31 PM Fabio Estevam <festevam@gmail.com> wrote:
> > 
> > > As Pierre's explanation addresses Marc's concern,
> > > do you think this can go to 2024.01 to fix the boot regression on imx8qxp/8qm?
> > 
> > I meant 2024.04, sorry.
> 
> How much testing has this seen outside of imx?

Tom, I tested with the AM62 TI arm processor (Toradex Verdin AM62) and
it works fine.

U-Boot 2024.04-rc4-00001-g5db2e36c8e97 (Mar 25 2024 - 17:28:20 -0300)

SoC:   AM62X SR1.0 HS-FS
DRAM:  1 GiB
Core:  138 devices, 29 uclasses, devicetree: separate
MMC:   mmc@fa10000: 0, mmc@fa00000: 1
Loading Environment from MMC... OK
In:    serial@2800000
Out:   serial@2800000
Err:   serial@2800000
Model: Toradex 0074 Verdin AM62 Dual 1GB IT V1.1A
Serial#: 15133548
Carrier: Toradex Dahlia V1.1C, Serial# 10952631

I've added my Tested-by as well.

> 
> -- 
> Tom

Best Regards,

Hiago.

Re: [PATCH] arm64: Fix map_range() not splitting mapped blocks

[Fabio Estevam]

Hi Tom,

On Mon, Mar 25, 2024 at 5:37 PM Hiago De Franco <hiagofranco@gmail.com> wrote:

> > How much testing has this seen outside of imx?
>
> Tom, I tested with the AM62 TI arm processor (Toradex Verdin AM62) and
> it works fine.

Do you think this one can be applied to next?

Then we would have time for more testing until 2024.07.

Re: [PATCH] arm64: Fix map_range() not splitting mapped blocks

[Fabio Estevam]

Hi Tom,

On Sat, Mar 30, 2024 at 5:03 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Tom,
>
> On Mon, Mar 25, 2024 at 5:37 PM Hiago De Franco <hiagofranco@gmail.com> wrote:
>
> > > How much testing has this seen outside of imx?
> >
> > Tom, I tested with the AM62 TI arm processor (Toradex Verdin AM62) and
> > it works fine.
>
> Do you think this one can be applied to next?
>
> Then we would have time for more testing until 2024.07.

Can this go in now?

Thanks

Re: [PATCH] arm64: Fix map_range() not splitting mapped blocks

[Marc Zyngier]

On Tue, 19 Mar 2024 12:39:26 +0000,
Pierre-Clément Tosi <ptosi@google.com> wrote:
> 
> Hi Fabio,
> 
> On Tue, Mar 19, 2024 at 09:13:12AM -0300, Fabio Estevam wrote:
> > Hi Pierre,
> > 
> > On Tue, Mar 19, 2024 at 8:39 AM Pierre-Clément Tosi <ptosi@google.com> wrote:
> > 
> > > This means gd->arch.tlb_addr pointing to the live PTs during setup_pgtables().
> > >
> > > In arch/arm/cpu/armv8, setup_all_pgtables() runs with SCTLR_ELx.M unset.
> > >
> > > In arch/arm/cpu/armv8/fsl-layerscape, setup_pgtables() is called twice:
> > >
> > >  - early_mmu_setup() calls it with SCTLR_ELx.M unset;
> > >  - final_mmu_setup() overwrites gd->arch.tlb_addr before calling it iff
> > >    CFG_SYS_MEM_RESERVE_SECURE is defined i.e. if CONFIG_SYS_SOC="fsl-layerscape"
> > >    so that <asm/arch-fsl-layerscape/config.h> gets auto-included through
> > >    <include/config.h>.
> > >
> > > So can CONFIG_FSL_LAYERSCAPE be set while CONFIG_SYS_SOC != "fsl-layerscape"?
> > 
> > No, this cannot happen.
> 
> Thanks for confirming.
> 
> For clarity, it might then make sense to drop that #ifdef in final_mmu_setup().
> 
> > Only the following Layerscape SoCs select CONFIG_FSL_LAYERSCAPE
> > in arch/arm/cpu/armv8/fsl-layerscape/Kconfig:
> > LS1012A, LS1028A, LS1043A, LS1046A, LS1088A, LS2080A, LX2162A and LX2160A
> > 
> > I saw the original boot problem with the i.MX8QX.
> > 
> > The i.MX8QX is part of the i.MX family, not the Layerscape family.
> 
> Sure.
> 
> To be clear, the concern here was that split_block() doesn't perform what the
> CPU architecture requires when modifying page tables that the MMU is using and
> the question therefore was: can setup_pgtables() be called on such live PTs?
> 
> For most AArch64 U-Boot ports (including the i.MX family), the answer is trivial
> because they use the arch code i.e. setup_all_pgtables(). However, as
> fsl-layerscape re-implements mmu_setup(), it had to be looked at separately,
> hence my question, which you answered above.

Thanks for the details.

With that,

Reviewed-by: Marc Zyngier <maz@kernel.org>

	M.

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

Re: [PATCH] arm64: Fix map_range() not splitting mapped blocks

[Tom Rini]

On Mon, 18 Mar 2024 19:35:49 +0000, Pierre-Clément Tosi wrote:

> The implementation of map_range() creates the requested mapping by
> walking the page tables, iterating over multiple PTEs and/or descending
> into existing table mappings as needed. When doing so, it assumes any
> pre-existing valid PTE to be a table mapping. This assumption is wrong
> if the platform code attempts to successively map two overlapping ranges
> where the latter intersects a block mapping created for the former.
> 
> [...]

Applied to u-boot/master, thanks!

-- 
Tom