| Message ID | 20241218090007.692815-1-ilias.apalodimas@linaro.org |
|---|---|
| State | Accepted |
| Commit | 0b7f4c7cf3270103a9aa8ffe2c2fb43d09b4ced5 |
| Headers | show |
| Series | imx: Fix usable memory ranges for imx8m SOCs | expand |
On 18.12.24 10:00 AM, Ilias Apalodimas wrote: > commit e27bddff4b97 ("imx8m: Restrict usable memory to space below 4G boundary") > tried to adjust the usable memory limits on a 4GB boundary. I would be nice to also mention 74f88b72219e ("ARM: imx: imx8m: Fix board_get_usable_ram_top()") which actually introduces the value of 0xFFFF_FFFF. I guess it would even qualify for another Fixes tag. > > ram_top is described as 'top address of RAM used by U-Boot' and we want > to preserve that. This is defined as a phys_addr_t and unfortunately > its size differs across architectures. This has lead us to a weird > state where 32bit boards define it 'SZ_4GB - 1' and 64bit boards as > 'SZ_4GB' unless it was otherwise defined. > > With some recent LMB changes and specifically > commit 1a48b0be93d4 ("lmb: prohibit allocations above ram_top even from same bank") > the board fails to boot properly although the commit above is correct > since it's making sure that no memory above ram_top is usable -- but > added to our memory map so EFI can hand it over to the booted OS. > > The reason for that is that during the LMB init we add all usable memory > in lmb_add_memory(). In that function any memory above ram_top gets added > as 'reserved' for LMB. With the current values tha's set to 0xFFFF_FFFF > for this board. Later LMB is trying to protect the memory area U-Boot lives > in with lmb_reserve_common(). The latter fails though since it tries to > add U-Boot top (which is 0xFFFF_FFFF as well) to U-Boot 'bottom'. This call > will fail since 1 byte of that memory range is already marked as 'reserved'. > > Since we are close to the release, LMB seems to assume that the address > is rounded up and is the 'next address' and so does parsing and adding > memory ranges from DT files, bump the ram_top of the board by 1byte. > > In the long run we should change all of the above and have 32b and 64b > platforms define ram_top identically. Some more ideas for cleaning up the situation in the long run: * Get rid of board_get_usable_ram_top() and fix the drivers that require address space below SZ_4G. * Parse the 'dma-ranges' property in the SoC node of the devicetree which already tells us what address space is usable (at least for i.MX8M). Anyway, the change looks good to me for now, so: Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de> > > Add a Fixes tag although the commit is correct, so people can figure out > the broken scenarios in the future. > > Suggested-by: Sughosh Ganu <sughosh.ganu@linaro.org> > Fixes: commit 1a48b0be93d4 ("lmb: prohibit allocations above ram_top even from same bank") > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > arch/arm/mach-imx/imx8m/soc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/mach-imx/imx8m/soc.c b/arch/arm/mach-imx/imx8m/soc.c > index 9588b8b28bf2..85dc8b51a145 100644 > --- a/arch/arm/mach-imx/imx8m/soc.c > +++ b/arch/arm/mach-imx/imx8m/soc.c > @@ -362,7 +362,7 @@ phys_addr_t board_get_usable_ram_top(phys_size_t total_size) > * space below the 4G address boundary (which is 3GiB big), > * even when the effective available memory is bigger. > */ > - top_addr = clamp_val((u64)PHYS_SDRAM + gd->ram_size, 0, 0xffffffff); > + top_addr = clamp_val((u64)PHYS_SDRAM + gd->ram_size, 0, SZ_4G); > > /* > * rom_pointer[0] stores the TEE memory start address.
Thanks Frieder On Wed, 18 Dec 2024 at 11:17, Frieder Schrempf <frieder.schrempf@kontron.de> wrote: > > On 18.12.24 10:00 AM, Ilias Apalodimas wrote: > > commit e27bddff4b97 ("imx8m: Restrict usable memory to space below 4G boundary") > > tried to adjust the usable memory limits on a 4GB boundary. > > I would be nice to also mention 74f88b72219e ("ARM: imx: imx8m: Fix > board_get_usable_ram_top()") which actually introduces the value of > 0xFFFF_FFFF. I guess it would even qualify for another Fixes tag. Ah thanks, Tom can you amend it or want me to send a v2? > > > > > ram_top is described as 'top address of RAM used by U-Boot' and we want > > to preserve that. This is defined as a phys_addr_t and unfortunately > > its size differs across architectures. This has lead us to a weird > > state where 32bit boards define it 'SZ_4GB - 1' and 64bit boards as > > 'SZ_4GB' unless it was otherwise defined. > > > > With some recent LMB changes and specifically > > commit 1a48b0be93d4 ("lmb: prohibit allocations above ram_top even from same bank") > > the board fails to boot properly although the commit above is correct > > since it's making sure that no memory above ram_top is usable -- but > > added to our memory map so EFI can hand it over to the booted OS. > > > > The reason for that is that during the LMB init we add all usable memory > > in lmb_add_memory(). In that function any memory above ram_top gets added > > as 'reserved' for LMB. With the current values tha's set to 0xFFFF_FFFF > > for this board. Later LMB is trying to protect the memory area U-Boot lives > > in with lmb_reserve_common(). The latter fails though since it tries to > > add U-Boot top (which is 0xFFFF_FFFF as well) to U-Boot 'bottom'. This call > > will fail since 1 byte of that memory range is already marked as 'reserved'. > > > > Since we are close to the release, LMB seems to assume that the address > > is rounded up and is the 'next address' and so does parsing and adding > > memory ranges from DT files, bump the ram_top of the board by 1byte. > > > > In the long run we should change all of the above and have 32b and 64b > > platforms define ram_top identically. > > Some more ideas for cleaning up the situation in the long run: > > * Get rid of board_get_usable_ram_top() and fix the drivers that require > address space below SZ_4G. > > * Parse the 'dma-ranges' property in the SoC node of the devicetree > which already tells us what address space is usable (at least for > i.MX8M). > > Anyway, the change looks good to me for now, so: Yep both of these are valid and good ideas. > > Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de> Thanks /Ilias > > > > > Add a Fixes tag although the commit is correct, so people can figure out > > the broken scenarios in the future. > > > > Suggested-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > Fixes: commit 1a48b0be93d4 ("lmb: prohibit allocations above ram_top even from same bank") > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > --- > > arch/arm/mach-imx/imx8m/soc.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/arm/mach-imx/imx8m/soc.c b/arch/arm/mach-imx/imx8m/soc.c > > index 9588b8b28bf2..85dc8b51a145 100644 > > --- a/arch/arm/mach-imx/imx8m/soc.c > > +++ b/arch/arm/mach-imx/imx8m/soc.c > > @@ -362,7 +362,7 @@ phys_addr_t board_get_usable_ram_top(phys_size_t total_size) > > * space below the 4G address boundary (which is 3GiB big), > > * even when the effective available memory is bigger. > > */ > > - top_addr = clamp_val((u64)PHYS_SDRAM + gd->ram_size, 0, 0xffffffff); > > + top_addr = clamp_val((u64)PHYS_SDRAM + gd->ram_size, 0, SZ_4G); > > > > /* > > * rom_pointer[0] stores the TEE memory start address. >
On Wed, Dec 18, 2024 at 11:24:45AM +0200, Ilias Apalodimas wrote: > Thanks Frieder > > On Wed, 18 Dec 2024 at 11:17, Frieder Schrempf > <frieder.schrempf@kontron.de> wrote: > > > > On 18.12.24 10:00 AM, Ilias Apalodimas wrote: > > > commit e27bddff4b97 ("imx8m: Restrict usable memory to space below 4G boundary") > > > tried to adjust the usable memory limits on a 4GB boundary. > > > > I would be nice to also mention 74f88b72219e ("ARM: imx: imx8m: Fix > > board_get_usable_ram_top()") which actually introduces the value of > > 0xFFFF_FFFF. I guess it would even qualify for another Fixes tag. > > Ah thanks, Tom can you amend it or want me to send a v2? patchwork / b4 will pickup a Fixes tag, but if you want to further-reword things instead do a v2 please.
Hi Tom, On Wed, 18 Dec 2024 at 16:34, Tom Rini <trini@konsulko.com> wrote: > > On Wed, Dec 18, 2024 at 11:24:45AM +0200, Ilias Apalodimas wrote: > > Thanks Frieder > > > > On Wed, 18 Dec 2024 at 11:17, Frieder Schrempf > > <frieder.schrempf@kontron.de> wrote: > > > > > > On 18.12.24 10:00 AM, Ilias Apalodimas wrote: > > > > commit e27bddff4b97 ("imx8m: Restrict usable memory to space below 4G boundary") > > > > tried to adjust the usable memory limits on a 4GB boundary. > > > > > > I would be nice to also mention 74f88b72219e ("ARM: imx: imx8m: Fix > > > board_get_usable_ram_top()") which actually introduces the value of > > > 0xFFFF_FFFF. I guess it would even qualify for another Fixes tag. > > > > Ah thanks, Tom can you amend it or want me to send a v2? > > patchwork / b4 will pickup a Fixes tag, but if you want to > further-reword things instead do a v2 please. Great, I am fine with the current commit message unless someone has a better idea. Cheers /Ilias > > -- > Tom
On Wed, Dec 18, 2024 at 04:50:09PM +0200, Ilias Apalodimas wrote: > Hi Tom, > > On Wed, 18 Dec 2024 at 16:34, Tom Rini <trini@konsulko.com> wrote: > > > > On Wed, Dec 18, 2024 at 11:24:45AM +0200, Ilias Apalodimas wrote: > > > Thanks Frieder > > > > > > On Wed, 18 Dec 2024 at 11:17, Frieder Schrempf > > > <frieder.schrempf@kontron.de> wrote: > > > > > > > > On 18.12.24 10:00 AM, Ilias Apalodimas wrote: > > > > > commit e27bddff4b97 ("imx8m: Restrict usable memory to space below 4G boundary") > > > > > tried to adjust the usable memory limits on a 4GB boundary. > > > > > > > > I would be nice to also mention 74f88b72219e ("ARM: imx: imx8m: Fix > > > > board_get_usable_ram_top()") which actually introduces the value of > > > > 0xFFFF_FFFF. I guess it would even qualify for another Fixes tag. > > > > > > Ah thanks, Tom can you amend it or want me to send a v2? > > > > patchwork / b4 will pickup a Fixes tag, but if you want to > > further-reword things instead do a v2 please. > > Great, I am fine with the current commit message unless someone has a > better idea. Fixes: 74f88b72219e ("ARM: imx: imx8m: Fix board_get_usable_ram_top()")
> Date: Wed, 18 Dec 2024 10:17:54 +0100 > From: Frieder Schrempf <frieder.schrempf@kontron.de> Hi, > On 18.12.24 10:00 AM, Ilias Apalodimas wrote: > > In the long run we should change all of the above and have 32b and 64b > > platforms define ram_top identically. > > Some more ideas for cleaning up the situation in the long run: > > * Get rid of board_get_usable_ram_top() and fix the drivers that require > address space below SZ_4G. Note that in many cases there are hardware limitations that restrict access to memory above SZ_4G by these devices. So the only way to "fix" the drivers would be to add bounce buffer support to them. That typically isn't trivial and adds complexity, especially when dealing with things like DMA rings. And it will make things slower. These boards all seem to have plenty of memory below SZ_4G, so I think board_get_usable_ram_top() is actually a very reasonable solution for the problem. > * Parse the 'dma-ranges' property in the SoC node of the devicetree > which already tells us what address space is usable (at least for > i.MX8M). We do this in OpenBSD to determine a memory range that is "DMA-safe". It isn't entirely trivial though. Not all boards have the relevant 'dma-ranges' property in the SoC node, and some boards have a 'dma-ranges' on nodes further down the tree that are very restrictive and not really relevant for what U-Boot wants to do. So you'd probably need a Kconfig option to override this. Cheers, Mark
> Subject: [PATCH] imx: Fix usable memory ranges for imx8m SOCs > > commit e27bddff4b97 ("imx8m: Restrict usable memory to space > below 4G boundary") tried to adjust the usable memory limits on a > 4GB boundary. > > ram_top is described as 'top address of RAM used by U-Boot' and we > want to preserve that. This is defined as a phys_addr_t and > unfortunately its size differs across architectures. This has lead us to a > weird state where 32bit boards define it 'SZ_4GB - 1' and 64bit boards > as 'SZ_4GB' unless it was otherwise defined. > > With some recent LMB changes and specifically commit > 1a48b0be93d4 ("lmb: prohibit allocations above ram_top even from > same bank") the board fails to boot properly although the commit > above is correct since it's making sure that no memory above ram_top > is usable -- but added to our memory map so EFI can hand it over to > the booted OS. > > The reason for that is that during the LMB init we add all usable > memory in lmb_add_memory(). In that function any memory above > ram_top gets added as 'reserved' for LMB. With the current values > tha's set to 0xFFFF_FFFF for this board. Later LMB is trying to protect > the memory area U-Boot lives in with lmb_reserve_common(). The > latter fails though since it tries to add U-Boot top (which is > 0xFFFF_FFFF as well) to U-Boot 'bottom'. This call will fail since 1 byte > of that memory range is already marked as 'reserved'. > > Since we are close to the release, LMB seems to assume that the > address is rounded up and is the 'next address' and so does parsing and > adding memory ranges from DT files, bump the ram_top of the board > by 1byte. > > In the long run we should change all of the above and have 32b and > 64b platforms define ram_top identically. > > Add a Fixes tag although the commit is correct, so people can figure > out the broken scenarios in the future. > > Suggested-by: Sughosh Ganu <sughosh.ganu@linaro.org> > Fixes: commit 1a48b0be93d4 ("lmb: prohibit allocations above > ram_top even from same bank") > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> Reviewed-by: Peng Fan <peng.fan@nxp.com>
On Thu, 19 Dec 2024 at 04:23, Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > Date: Wed, 18 Dec 2024 10:17:54 +0100 > > From: Frieder Schrempf <frieder.schrempf@kontron.de> > > Hi, > > > On 18.12.24 10:00 AM, Ilias Apalodimas wrote: > > > In the long run we should change all of the above and have 32b and 64b > > > platforms define ram_top identically. > > > > Some more ideas for cleaning up the situation in the long run: > > > > * Get rid of board_get_usable_ram_top() and fix the drivers that require > > address space below SZ_4G. > > Note that in many cases there are hardware limitations that restrict > access to memory above SZ_4G by these devices. So the only way to > "fix" the drivers would be to add bounce buffer support to them. That > typically isn't trivial and adds complexity, especially when dealing > with things like DMA rings. And it will make things slower. > > These boards all seem to have plenty of memory below SZ_4G, so I think > board_get_usable_ram_top() is actually a very reasonable solution for > the problem. Up until now, most issues have cropped up because of the addition of RAM above ram_top to LMB's memory map. And I believe that is primarily down to different, often conflicting requirements of platforms. The addition of memory above ram_top is being done so that the memory above ram_top gets registered in the EFI memory map. But with all these issues that platforms are encountering, I think it will be best to have this addition above ram_top done from the EFI memory module. The LMB module will then be concerned only about memory till ram_top, which platforms are free to set to a correct value considering their limitations, or placement of images in RAM memory. I will be working on this change for the next release, and send the patches out once I have tested them on some of the boards that I have. As an aside, I believe, there needs to be some kind of an audit of the value of ram_top that is set by platforms. By default, ram_top is set to an address one higher than the actual highest used address. That is what is being done in the setup_dest_addr() function. The LMB function to reserve the U-Boot image region also works with this assumption. However, some platforms are not working with this rule. The imx8m is one such platform. Considering the fact that we have 32 bit systems, the ram_top should be changed to a u64 type so that we can have this consistency among platforms. -sughosh > > > * Parse the 'dma-ranges' property in the SoC node of the devicetree > > which already tells us what address space is usable (at least for > > i.MX8M). > > We do this in OpenBSD to determine a memory range that is "DMA-safe". > It isn't entirely trivial though. Not all boards have the relevant > 'dma-ranges' property in the SoC node, and some boards have a > 'dma-ranges' on nodes further down the tree that are very restrictive > and not really relevant for what U-Boot wants to do. So you'd > probably need a Kconfig option to override this. > > Cheers, > > Mark
On Wed, Dec 18, 2024 at 11:00:06AM +0200, Ilias Apalodimas wrote: > commit e27bddff4b97 ("imx8m: Restrict usable memory to space below 4G boundary") > tried to adjust the usable memory limits on a 4GB boundary. > > ram_top is described as 'top address of RAM used by U-Boot' and we want > to preserve that. This is defined as a phys_addr_t and unfortunately > its size differs across architectures. This has lead us to a weird > state where 32bit boards define it 'SZ_4GB - 1' and 64bit boards as > 'SZ_4GB' unless it was otherwise defined. > > With some recent LMB changes and specifically > commit 1a48b0be93d4 ("lmb: prohibit allocations above ram_top even from same bank") > the board fails to boot properly although the commit above is correct > since it's making sure that no memory above ram_top is usable -- but > added to our memory map so EFI can hand it over to the booted OS. > > The reason for that is that during the LMB init we add all usable memory > in lmb_add_memory(). In that function any memory above ram_top gets added > as 'reserved' for LMB. With the current values tha's set to 0xFFFF_FFFF > for this board. Later LMB is trying to protect the memory area U-Boot lives > in with lmb_reserve_common(). The latter fails though since it tries to > add U-Boot top (which is 0xFFFF_FFFF as well) to U-Boot 'bottom'. This call > will fail since 1 byte of that memory range is already marked as 'reserved'. > > Since we are close to the release, LMB seems to assume that the address > is rounded up and is the 'next address' and so does parsing and adding > memory ranges from DT files, bump the ram_top of the board by 1byte. > > In the long run we should change all of the above and have 32b and 64b > platforms define ram_top identically. > > Add a Fixes tag although the commit is correct, so people can figure out > the broken scenarios in the future. > > Suggested-by: Sughosh Ganu <sughosh.ganu@linaro.org> > Fixes: commit 1a48b0be93d4 ("lmb: prohibit allocations above ram_top even from same bank") > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> Reported-by: João Paulo Gonçalves <jpaulo.silvagoncalves@gmail.com> Closes: https://lore.kernel.org/all/20241216114231.qpfwug3zfqkxn3d5@joaog-nb.corp.toradex.com/ Thanks, Francesco
On Wed, 18 Dec 2024 11:00:06 +0200, Ilias Apalodimas wrote: > commit e27bddff4b97 ("imx8m: Restrict usable memory to space below 4G boundary") > tried to adjust the usable memory limits on a 4GB boundary. > > ram_top is described as 'top address of RAM used by U-Boot' and we want > to preserve that. This is defined as a phys_addr_t and unfortunately > its size differs across architectures. This has lead us to a weird > state where 32bit boards define it 'SZ_4GB - 1' and 64bit boards as > 'SZ_4GB' unless it was otherwise defined. > > [...] Applied to u-boot/master, thanks!
diff --git a/arch/arm/mach-imx/imx8m/soc.c b/arch/arm/mach-imx/imx8m/soc.c index 9588b8b28bf2..85dc8b51a145 100644 --- a/arch/arm/mach-imx/imx8m/soc.c +++ b/arch/arm/mach-imx/imx8m/soc.c @@ -362,7 +362,7 @@ phys_addr_t board_get_usable_ram_top(phys_size_t total_size) * space below the 4G address boundary (which is 3GiB big), * even when the effective available memory is bigger. */ - top_addr = clamp_val((u64)PHYS_SDRAM + gd->ram_size, 0, 0xffffffff); + top_addr = clamp_val((u64)PHYS_SDRAM + gd->ram_size, 0, SZ_4G); /* * rom_pointer[0] stores the TEE memory start address.
commit e27bddff4b97 ("imx8m: Restrict usable memory to space below 4G boundary") tried to adjust the usable memory limits on a 4GB boundary. ram_top is described as 'top address of RAM used by U-Boot' and we want to preserve that. This is defined as a phys_addr_t and unfortunately its size differs across architectures. This has lead us to a weird state where 32bit boards define it 'SZ_4GB - 1' and 64bit boards as 'SZ_4GB' unless it was otherwise defined. With some recent LMB changes and specifically commit 1a48b0be93d4 ("lmb: prohibit allocations above ram_top even from same bank") the board fails to boot properly although the commit above is correct since it's making sure that no memory above ram_top is usable -- but added to our memory map so EFI can hand it over to the booted OS. The reason for that is that during the LMB init we add all usable memory in lmb_add_memory(). In that function any memory above ram_top gets added as 'reserved' for LMB. With the current values tha's set to 0xFFFF_FFFF for this board. Later LMB is trying to protect the memory area U-Boot lives in with lmb_reserve_common(). The latter fails though since it tries to add U-Boot top (which is 0xFFFF_FFFF as well) to U-Boot 'bottom'. This call will fail since 1 byte of that memory range is already marked as 'reserved'. Since we are close to the release, LMB seems to assume that the address is rounded up and is the 'next address' and so does parsing and adding memory ranges from DT files, bump the ram_top of the board by 1byte. In the long run we should change all of the above and have 32b and 64b platforms define ram_top identically. Add a Fixes tag although the commit is correct, so people can figure out the broken scenarios in the future. Suggested-by: Sughosh Ganu <sughosh.ganu@linaro.org> Fixes: commit 1a48b0be93d4 ("lmb: prohibit allocations above ram_top even from same bank") Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- arch/arm/mach-imx/imx8m/soc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)