Re: [PATCH] arm64: Fix map_range() not splitting mapped blocks
On Mon, 18 Mar 2024 19:35:49 +, 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
Re: [PATCH] arm64: Fix map_range() not splitting mapped blocks
Hi Tom, On Sat, Mar 30, 2024 at 5:03 PM Fabio Estevam wrote: > > Hi Tom, > > On Mon, Mar 25, 2024 at 5:37 PM Hiago De Franco 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
Hi Tom, On Mon, Mar 25, 2024 at 5:37 PM Hiago De Franco 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
On Tue, 19 Mar 2024 12:39:26 +, Pierre-Clément Tosi 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 > > 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 gets auto-included through > > >. > > > > > > 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 M. -- Without deviation from the norm, progress is not possible.
Re: [PATCH] arm64: Fix map_range() not splitting mapped blocks
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 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
>
> This fixes the boot regression on colibri-imx8x.
>
> Thanks a lot for your fix!
>
> Tested-by: Fabio Estevam
Tested-by: Hiago De Franco # 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 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-1-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@fa1: 0, mmc@fa0: 1
Loading Environment from MMC... OK
In:serial@280
Out: serial@280
Err: serial@280
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
On Fri, Mar 22, 2024 at 04:33:03PM -0300, Fabio Estevam wrote: > On Fri, Mar 22, 2024 at 4:31 PM Fabio Estevam 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 signature.asc Description: PGP signature
Re: [PATCH] arm64: Fix map_range() not splitting mapped blocks
On Fri, Mar 22, 2024 at 4:31 PM Fabio Estevam 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
Hi Tom, On Tue, Mar 19, 2024 at 9:39 AM Pierre-Clément Tosi 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
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 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 gets auto-included through > >. > > > > 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
Hi Pierre, On Tue, Mar 19, 2024 at 8:39 AM Pierre-Clément Tosi 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 gets auto-included through >. > > 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
Hi Marc, On Tue, Mar 19, 2024 at 09:43:03AM +, 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 gets auto-included through . 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
On Mon, 18 Mar 2024 19:35:49 +,
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.
>
> 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
> ---
> 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([i]) == PTE_TYPE_FAULT)
> set_pte_table([i], create_table());
> + else if (pte_type([i]) != PTE_TYPE_TABLE)
> + split_block([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
Hi Pierre,
On Mon, Mar 18, 2024 at 4:35 PM 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.
>
> 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
This fixes the boot regression on colibri-imx8x.
Thanks a lot for your fix!
Tested-by: Fabio Estevam
[PATCH] arm64: Fix map_range() not splitting mapped blocks
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
---
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([i]) == PTE_TYPE_FAULT)
set_pte_table([i], create_table());
+ else if (pte_type([i]) != PTE_TYPE_TABLE)
+ split_block([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
