[PATCH] lmb: prohibit allocations above ram_top even from same bank

[PATCH] lmb: prohibit allocations above ram_top even from same bank

[Sughosh Ganu]

There are platforms which set the value of ram_top based on certain
restrictions that the platform might have in accessing memory above
ram_top, even when the memory region is in the same DRAM bank. So,
even though the LMB allocator works as expected, when trying to
allocate memory above ram_top, prohibit this by marking all memory
above ram_top as reserved, even if the said memory region is from the
same bank.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 lib/lmb.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/lmb.c b/lib/lmb.c
index 14b9b8466ff..be168dfb8dc 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -615,6 +615,7 @@ static __maybe_unused void lmb_reserve_common_spl(void)
 void lmb_add_memory(void)
 {
 	int i;
+	phys_addr_t bank_end;
 	phys_size_t size;
 	u64 ram_top = gd->ram_top;
 	struct bd_info *bd = gd->bd;
@@ -628,6 +629,8 @@ void lmb_add_memory(void)
 
 	for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {
 		size = bd->bi_dram[i].size;
+		bank_end = bd->bi_dram[i].start + size;
+
 		if (size) {
 			lmb_add(bd->bi_dram[i].start, size);
 
@@ -639,6 +642,9 @@ void lmb_add_memory(void)
 			if (bd->bi_dram[i].start >= ram_top)
 				lmb_reserve_flags(bd->bi_dram[i].start, size,
 						  LMB_NOOVERWRITE);
+			else if (bank_end > ram_top)
+				lmb_reserve_flags(ram_top, bank_end - ram_top,
+						  LMB_NOOVERWRITE);
 		}
 	}
 }
-- 
2.34.1

Re: [PATCH] lmb: prohibit allocations above ram_top even from same bank

[Andreas Schwab]

On Dez 02 2024, Sughosh Ganu wrote:

> There are platforms which set the value of ram_top based on certain
> restrictions that the platform might have in accessing memory above
> ram_top, even when the memory region is in the same DRAM bank. So,
> even though the LMB allocator works as expected, when trying to
> allocate memory above ram_top, prohibit this by marking all memory
> above ram_top as reserved, even if the said memory region is from the
> same bank.

This fixes the boot failure on the VisionFive2 board as reported in
<mvm8qtnv310.fsf@suse.de>.

Tested-by: Andreas Schwab <schwab@suse.de>

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] lmb: prohibit allocations above ram_top even from same bank

[Sughosh Ganu]

On Mon, 2 Dec 2024 at 19:16, E Shattow <lucent@gmail.com> wrote:
>
> Hi Sughosh, Andreas, et. al
>
> On Mon, Dec 2, 2024 at 3:58 AM Andreas Schwab <schwab@suse.de> wrote:
> >
> > On Dez 02 2024, Sughosh Ganu wrote:
> >
> > > There are platforms which set the value of ram_top based on certain
> > > restrictions that the platform might have in accessing memory above
> > > ram_top, even when the memory region is in the same DRAM bank. So,
> > > even though the LMB allocator works as expected, when trying to
> > > allocate memory above ram_top, prohibit this by marking all memory
> > > above ram_top as reserved, even if the said memory region is from the
> > > same bank.
> >
> > This fixes the boot failure on the VisionFive2 board as reported in
> > <mvm8qtnv310.fsf@suse.de>.
> >
> > Tested-by: Andreas Schwab <schwab@suse.de>
> >
> > --
> > Andreas Schwab, SUSE Labs, schwab@suse.de
> > GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
> > "And now for something completely different."
>
> Echoing what Andreas finds in testing, this avoids the platform issue
> with dw_mmc driver and the visionfive2 board support on 4GB Star64,
> 4GB Mars CM Lite, and 8GB Mars CM Lite WiFi.
>
> > diff --git a/lib/lmb.c b/lib/lmb.c
> > index 14b9b8466ff..be168dfb8dc 100644
> > --- a/lib/lmb.c
> > +++ b/lib/lmb.c
> > @@ -615,6 +615,7 @@ static __maybe_unused void lmb_reserve_common_spl(void)
> >  void lmb_add_memory(void)
> >  {
> >   int i;
> > + phys_addr_t bank_end;
> >   phys_size_t size;
> >   u64 ram_top = gd->ram_top;
> >   struct bd_info *bd = gd->bd;
> > @@ -628,6 +629,8 @@ void lmb_add_memory(void)
> >
> >   for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {
> >       size = bd->bi_dram[i].size;
> > +     bank_end = bd->bi_dram[i].start + size;
> > +
>
> Delete newline? Unless existing style is wrong here.

It's not about style, but the above two lines are more of an
initialisation within the block, which is the reason for a newline
here.

>
> >       if (size) {
> >           lmb_add(bd->bi_dram[i].start, size);
> >
> > @@ -639,6 +642,9 @@ void lmb_add_memory(void)
> >           if (bd->bi_dram[i].start >= ram_top)
> >               lmb_reserve_flags(bd->bi_dram[i].start, size,
> >                         LMB_NOOVERWRITE);
> > +         else if (bank_end > ram_top)
> > +             lmb_reserve_flags(ram_top, bank_end - ram_top,
> > +                       LMB_NOOVERWRITE);
>
> Please guard this somehow that it will be enabled per each platform
> that needs it, and/or give warning when conditions this specific code
> path is a workaround for are reached that are different than before
> the patch. Any similar platform-specific memory addressing bug/quirk
> having previously avoided a trigger will silently continue to work
> here instead of getting documented; we should at least uncover these
> assumptions.

Instead of putting some check here, and complicating the code, I would
say that the board maintainer would know why the ram_top has not been
put at the top of DRAM memory -- if possible, the ram_top should be
the highest possible DRAM address. So this is the responsibility of
the board maintainer instead. Ideally, for a 64 bit machine, like the
one where you hit this issue, there is no reason for the board to not
use the highest possible DRAM address as ram_top, unless there is some
issue, like accessing memory address above a certain value. So, this
would not be an issue if the board is using the highest possible value
of memory as ram_top, especially when it comes to a single DRAM bank.

>
> In the theoretical situation where we are to just fix the visionfive2
> mmc platform specific issue in the dw_mmc driver and not need this
> workaround code anymore, there's not visibility for us to easily
> diagnose if a similar assumption or previously un-triggered platform
> quirk exists elsewhere. There's no warning, and no easy on/off config
> option to test with and without. The memory rework commit 22f2c9ed9f53
> is here a useful tool to break and bring attention to assumptions in
> code and documentation.

Fixing the mmc driver issue may or may not be possible. And if this is
indeed a fixable issue, then, after fixing the mmc driver, the
subsequent change should also remove this restriction on the ram_top
value, which will automatically take care of this restriction.

-sughosh

>
> >       }
> >   }
> >  }
>
> CONFIG_EFI_LOADER_BOUNCE_BUFFER=y added to visionfive2 config in
> 6c791b6646c1 does not make sense to me as when I tested it did not
> avoid the platform issue. Restricting the memory allocations is an
> effective workaround to the visionfive2 platform issue. I leave that
> to the experts if a revert is appropriate given that we have an
> effective workaround now.
>
> Thank you Sughosh! This is (meanwhile until more comments and review)
> a functional workaround.
>
> Tested-by: E Shattow <lucent@gmail.com>

Re: [PATCH] lmb: prohibit allocations above ram_top even from same bank

[Tom Rini]

On Mon, 02 Dec 2024 12:36:24 +0530, Sughosh Ganu wrote:

> There are platforms which set the value of ram_top based on certain
> restrictions that the platform might have in accessing memory above
> ram_top, even when the memory region is in the same DRAM bank. So,
> even though the LMB allocator works as expected, when trying to
> allocate memory above ram_top, prohibit this by marking all memory
> above ram_top as reserved, even if the said memory region is from the
> same bank.
> 
> [...]

Applied to u-boot/master, thanks!

-- 
Tom

Re: [PATCH] lmb: prohibit allocations above ram_top even from same bank

[Patrice CHOTARD]

On 12/7/24 16:57, Tom Rini wrote:
> On Mon, 02 Dec 2024 12:36:24 +0530, Sughosh Ganu wrote:
> 
>> There are platforms which set the value of ram_top based on certain
>> restrictions that the platform might have in accessing memory above
>> ram_top, even when the memory region is in the same DRAM bank. So,
>> even though the LMB allocator works as expected, when trying to
>> allocate memory above ram_top, prohibit this by marking all memory
>> above ram_top as reserved, even if the said memory region is from the
>> same bank.
>>
>> [...]
> 
> Applied to u-boot/master, thanks!
> 
Hello

This patch is breaking the boot on STM32MP135F-DK.

On this platform, we got an area above gd->ram_top, 
this area, reserved for OPTEE, is tagged with LMB_NOMAP in boot_fdt_add_mem_rsv_regions().

Since this commit 1a48b0be93d4 ("lmb: prohibit allocations above ram_top even from same bank"),
this area is no more tagged as LMB_NOMAP, because it's previously been 
tagged with LMB_NOOVERWRITE in lmb_add_memory().

By not being tagged LMB_NOMAP, the MMU configuration is impacted and leads to a panic.

I suggest to revert this patch.

Patrice

Re: [PATCH] lmb: prohibit allocations above ram_top even from same bank

[Sughosh Ganu]

On Wed, 11 Dec 2024 at 21:50, Patrice CHOTARD
<patrice.chotard@foss.st.com> wrote:
>
>
>
> On 12/7/24 16:57, Tom Rini wrote:
> > On Mon, 02 Dec 2024 12:36:24 +0530, Sughosh Ganu wrote:
> >
> >> There are platforms which set the value of ram_top based on certain
> >> restrictions that the platform might have in accessing memory above
> >> ram_top, even when the memory region is in the same DRAM bank. So,
> >> even though the LMB allocator works as expected, when trying to
> >> allocate memory above ram_top, prohibit this by marking all memory
> >> above ram_top as reserved, even if the said memory region is from the
> >> same bank.
> >>
> >> [...]
> >
> > Applied to u-boot/master, thanks!
> >
> Hello
>
> This patch is breaking the boot on STM32MP135F-DK.
>
> On this platform, we got an area above gd->ram_top,
> this area, reserved for OPTEE, is tagged with LMB_NOMAP in boot_fdt_add_mem_rsv_regions().
>
> Since this commit 1a48b0be93d4 ("lmb: prohibit allocations above ram_top even from same bank"),
> this area is no more tagged as LMB_NOMAP, because it's previously been
> tagged with LMB_NOOVERWRITE in lmb_add_memory().
>
> By not being tagged LMB_NOMAP, the MMU configuration is impacted and leads to a panic.
>
> I suggest to revert this patch.

I don't think that this patch should be reverted. If the said platform
has a reserved memory region above ram_top, I would suggest to either
a) move the ram_top on this platform so that the op-tee region gets
marked as no-map in the lmb memory map, or b) do not use the lmb
memory map to configure the MMU -- can the MMU configuration logic not
read the DT to get which regions are to be marked as no-map?

As far as the lmb module is concerned, it is being told through this
commit to not consider memory above ram_top for allocations, which is
not an incorrect thing imo.

-sughosh

>
> Patrice

Re: [PATCH] lmb: prohibit allocations above ram_top even from same bank

[Patrice CHOTARD]

On 12/11/24 17:27, Sughosh Ganu wrote:
> On Wed, 11 Dec 2024 at 21:50, Patrice CHOTARD
> <patrice.chotard@foss.st.com> wrote:
>>
>>
>>
>> On 12/7/24 16:57, Tom Rini wrote:
>>> On Mon, 02 Dec 2024 12:36:24 +0530, Sughosh Ganu wrote:
>>>
>>>> There are platforms which set the value of ram_top based on certain
>>>> restrictions that the platform might have in accessing memory above
>>>> ram_top, even when the memory region is in the same DRAM bank. So,
>>>> even though the LMB allocator works as expected, when trying to
>>>> allocate memory above ram_top, prohibit this by marking all memory
>>>> above ram_top as reserved, even if the said memory region is from the
>>>> same bank.
>>>>
>>>> [...]
>>>
>>> Applied to u-boot/master, thanks!
>>>
>> Hello
>>
>> This patch is breaking the boot on STM32MP135F-DK.
>>
>> On this platform, we got an area above gd->ram_top,
>> this area, reserved for OPTEE, is tagged with LMB_NOMAP in boot_fdt_add_mem_rsv_regions().
>>
>> Since this commit 1a48b0be93d4 ("lmb: prohibit allocations above ram_top even from same bank"),
>> this area is no more tagged as LMB_NOMAP, because it's previously been
>> tagged with LMB_NOOVERWRITE in lmb_add_memory().
>>
>> By not being tagged LMB_NOMAP, the MMU configuration is impacted and leads to a panic.
>>
>> I suggest to revert this patch.
> 
> I don't think that this patch should be reverted. If the said platform
> has a reserved memory region above ram_top, I would suggest to either
> a) move the ram_top on this platform so that the op-tee region gets
> marked as no-map in the lmb memory map, or b) do not use the lmb

In my explanation above, i indicated that before this commit, 
this area was marked as LMB_NOMAP in the lmb memory map by boot_fdt_add_mem_rsv_regions().
this is exactly what you described in the possible solution "a)".

But now with this commit, as lmb_add_memory() is called in lmb_init() the area above ram_top is marked LMB_NOOVERWRITE.
Then later, boot_fdt_add_mem_rsv_regions() is executed, but the area above ram_top can't be marked as
LMB_NOMAP as previously because it's already marked LMB_NOOVERWRITE. 


> memory map to configure the MMU -- can the MMU configuration logic not
> read the DT to get which regions are to be marked as no-map?
> 
> As far as the lmb module is concerned, it is being told through this
> commit to not consider memory above ram_top for allocations, which is
> not an incorrect thing imo.

That's the case, we don't consider memory above ram_top for allocations,
we only marked it with LMB_NOMAP.

Thanks
Patrice

> 
> -sughosh
> 
>>
>> Patrice

Re: [PATCH] lmb: prohibit allocations above ram_top even from same bank

[Sughosh Ganu]

On Wed, 11 Dec 2024 at 22:20, Patrice CHOTARD
<patrice.chotard@foss.st.com> wrote:
>
>
>
> On 12/11/24 17:27, Sughosh Ganu wrote:
> > On Wed, 11 Dec 2024 at 21:50, Patrice CHOTARD
> > <patrice.chotard@foss.st.com> wrote:
> >>
> >>
> >>
> >> On 12/7/24 16:57, Tom Rini wrote:
> >>> On Mon, 02 Dec 2024 12:36:24 +0530, Sughosh Ganu wrote:
> >>>
> >>>> There are platforms which set the value of ram_top based on certain
> >>>> restrictions that the platform might have in accessing memory above
> >>>> ram_top, even when the memory region is in the same DRAM bank. So,
> >>>> even though the LMB allocator works as expected, when trying to
> >>>> allocate memory above ram_top, prohibit this by marking all memory
> >>>> above ram_top as reserved, even if the said memory region is from the
> >>>> same bank.
> >>>>
> >>>> [...]
> >>>
> >>> Applied to u-boot/master, thanks!
> >>>
> >> Hello
> >>
> >> This patch is breaking the boot on STM32MP135F-DK.
> >>
> >> On this platform, we got an area above gd->ram_top,
> >> this area, reserved for OPTEE, is tagged with LMB_NOMAP in boot_fdt_add_mem_rsv_regions().
> >>
> >> Since this commit 1a48b0be93d4 ("lmb: prohibit allocations above ram_top even from same bank"),
> >> this area is no more tagged as LMB_NOMAP, because it's previously been
> >> tagged with LMB_NOOVERWRITE in lmb_add_memory().
> >>
> >> By not being tagged LMB_NOMAP, the MMU configuration is impacted and leads to a panic.
> >>
> >> I suggest to revert this patch.
> >
> > I don't think that this patch should be reverted. If the said platform
> > has a reserved memory region above ram_top, I would suggest to either
> > a) move the ram_top on this platform so that the op-tee region gets
> > marked as no-map in the lmb memory map, or b) do not use the lmb
>
> In my explanation above, i indicated that before this commit,
> this area was marked as LMB_NOMAP in the lmb memory map by boot_fdt_add_mem_rsv_regions().
> this is exactly what you described in the possible solution "a)".
>
> But now with this commit, as lmb_add_memory() is called in lmb_init() the area above ram_top is marked LMB_NOOVERWRITE.
> Then later, boot_fdt_add_mem_rsv_regions() is executed, but the area above ram_top can't be marked as
> LMB_NOMAP as previously because it's already marked LMB_NOOVERWRITE.

This has been done to ensure that memory above ram_top is not taken
into consideration when it comes to U-Boot. The reason why memory
above ram_top also needs to be added is to ensure that this memory
also gets passed on to the OS when booting with EFI. If it has to be
considered by U-Boot, the value of ram_top needs to be adjusted
accordingly. Is that not possible on the platform? If not, the only
other solution is to obtain this memory region from the DT, and then
configure the MMU.

>
>
> > memory map to configure the MMU -- can the MMU configuration logic not
> > read the DT to get which regions are to be marked as no-map?
> >
> > As far as the lmb module is concerned, it is being told through this
> > commit to not consider memory above ram_top for allocations, which is
> > not an incorrect thing imo.
>
> That's the case, we don't consider memory above ram_top for allocations,
> we only marked it with LMB_NOMAP.

That was because the lmb scope was local. That meant a platform could
add any size that it wanted, and then use that map for whatever it
fancied. The use of lmb for "allocating" addresses for io-va addresses
by the apple iommu is another such case.

-sughosh

>
> Thanks
> Patrice
>
> >
> > -sughosh
> >
> >>
> >> Patrice

Re: [PATCH] lmb: prohibit allocations above ram_top even from same bank

[Patrice CHOTARD]

On 12/11/24 19:16, Sughosh Ganu wrote:
> On Wed, 11 Dec 2024 at 22:20, Patrice CHOTARD
> <patrice.chotard@foss.st.com> wrote:
>>
>>
>>
>> On 12/11/24 17:27, Sughosh Ganu wrote:
>>> On Wed, 11 Dec 2024 at 21:50, Patrice CHOTARD
>>> <patrice.chotard@foss.st.com> wrote:
>>>>
>>>>
>>>>
>>>> On 12/7/24 16:57, Tom Rini wrote:
>>>>> On Mon, 02 Dec 2024 12:36:24 +0530, Sughosh Ganu wrote:
>>>>>
>>>>>> There are platforms which set the value of ram_top based on certain
>>>>>> restrictions that the platform might have in accessing memory above
>>>>>> ram_top, even when the memory region is in the same DRAM bank. So,
>>>>>> even though the LMB allocator works as expected, when trying to
>>>>>> allocate memory above ram_top, prohibit this by marking all memory
>>>>>> above ram_top as reserved, even if the said memory region is from the
>>>>>> same bank.
>>>>>>
>>>>>> [...]
>>>>>
>>>>> Applied to u-boot/master, thanks!
>>>>>
>>>> Hello
>>>>
>>>> This patch is breaking the boot on STM32MP135F-DK.
>>>>
>>>> On this platform, we got an area above gd->ram_top,
>>>> this area, reserved for OPTEE, is tagged with LMB_NOMAP in boot_fdt_add_mem_rsv_regions().
>>>>
>>>> Since this commit 1a48b0be93d4 ("lmb: prohibit allocations above ram_top even from same bank"),
>>>> this area is no more tagged as LMB_NOMAP, because it's previously been
>>>> tagged with LMB_NOOVERWRITE in lmb_add_memory().
>>>>
>>>> By not being tagged LMB_NOMAP, the MMU configuration is impacted and leads to a panic.
>>>>
>>>> I suggest to revert this patch.
>>>
>>> I don't think that this patch should be reverted. If the said platform
>>> has a reserved memory region above ram_top, I would suggest to either
>>> a) move the ram_top on this platform so that the op-tee region gets
>>> marked as no-map in the lmb memory map, or b) do not use the lmb
>>
>> In my explanation above, i indicated that before this commit,
>> this area was marked as LMB_NOMAP in the lmb memory map by boot_fdt_add_mem_rsv_regions().
>> this is exactly what you described in the possible solution "a)".
>>
>> But now with this commit, as lmb_add_memory() is called in lmb_init() the area above ram_top is marked LMB_NOOVERWRITE.
>> Then later, boot_fdt_add_mem_rsv_regions() is executed, but the area above ram_top can't be marked as
>> LMB_NOMAP as previously because it's already marked LMB_NOOVERWRITE.
> 
> This has been done to ensure that memory above ram_top is not taken
> into consideration when it comes to U-Boot. The reason why memory

It was already the case before this commit, ram_top was designed to
indicate to U-Boot the top address of available RAM,
see include/asm-generic/global_data.h :

	/**
	 * @ram_top: top address of RAM used by U-Boot
	 */
	phys_addr_t ram_top;

> above ram_top also needs to be added is to ensure that this memory
> also gets passed on to the OS when booting with EFI. If it has to be
> considered by U-Boot, the value of ram_top needs to be adjusted
> accordingly. Is that not possible on the platform? If not, the only
> other solution is to obtain this memory region from the DT, and then
> configure the MMU.

Currently, ram_top is adjusted on STM32MP platforms, 
for example in stm32mp135f-dk.dts :

	reserved-memory {
		#address-cells = <1>;
		#size-cells = <1>;
		ranges;

		optee@dd000000 {
			reg = <0xdd000000 0x3000000>;
			no-map;
		};
	};

          0xE000 0000  ********************  
                       *                  *  
                       *       OPTEE      *  
                       *    (LMB_NOMAP)   *  
                       *                  *  
ram_top = 0xDD00 0000  ********************  
                       *                  *
                       *                  *
                       *                  *
                       *                  *
                       *                  *
                       *                  *
                       *                  *
                       *                  *
                       *                  *
                       *                  *
          0xC000 0000  ********************

On STM32MP platforms, we already obtain all memory regions from DT with 
property "no-map" and we marked them LMB_NOMAP.

Later we parse the LMB regions, all of these region marked LMB_NOMAP are 
used to configure our MMU accordingly.
So, again, we are doing things as you suggested.

This commit now forbids to mark OPTEE memory region with LMB_NOMAP as 
indicated in DT.

For information, it has impact on all STM32MP platforms (at least 6 boards).

Patrice



> 
>>
>>
>>> memory map to configure the MMU -- can the MMU configuration logic not
>>> read the DT to get which regions are to be marked as no-map?
>>>
>>> As far as the lmb module is concerned, it is being told through this
>>> commit to not consider memory above ram_top for allocations, which is
>>> not an incorrect thing imo.
>>
>> That's the case, we don't consider memory above ram_top for allocations,
>> we only marked it with LMB_NOMAP.
> 
> That was because the lmb scope was local. That meant a platform could
> add any size that it wanted, and then use that map for whatever it
> fancied. The use of lmb for "allocating" addresses for io-va addresses
> by the apple iommu is another such case.
> 
> -sughosh
> 
>>
>> Thanks
>> Patrice
>>
>>>
>>> -sughosh
>>>
>>>>
>>>> Patrice

Re: [PATCH] lmb: prohibit allocations above ram_top even from same bank

[Mark Kettenis]

> Date: Thu, 12 Dec 2024 09:23:52 +0100
> From: Patrice CHOTARD <patrice.chotard@foss.st.com>
> 
> On 12/11/24 19:16, Sughosh Ganu wrote:
> > On Wed, 11 Dec 2024 at 22:20, Patrice CHOTARD
> > <patrice.chotard@foss.st.com> wrote:
> >>
> >>
> >>
> >> On 12/11/24 17:27, Sughosh Ganu wrote:
> >>> On Wed, 11 Dec 2024 at 21:50, Patrice CHOTARD
> >>> <patrice.chotard@foss.st.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 12/7/24 16:57, Tom Rini wrote:
> >>>>> On Mon, 02 Dec 2024 12:36:24 +0530, Sughosh Ganu wrote:
> >>>>>
> >>>>>> There are platforms which set the value of ram_top based on certain
> >>>>>> restrictions that the platform might have in accessing memory above
> >>>>>> ram_top, even when the memory region is in the same DRAM bank. So,
> >>>>>> even though the LMB allocator works as expected, when trying to
> >>>>>> allocate memory above ram_top, prohibit this by marking all memory
> >>>>>> above ram_top as reserved, even if the said memory region is from the
> >>>>>> same bank.
> >>>>>>
> >>>>>> [...]
> >>>>>
> >>>>> Applied to u-boot/master, thanks!
> >>>>>
> >>>> Hello
> >>>>
> >>>> This patch is breaking the boot on STM32MP135F-DK.
> >>>>
> >>>> On this platform, we got an area above gd->ram_top,
> >>>> this area, reserved for OPTEE, is tagged with LMB_NOMAP in boot_fdt_add_mem_rsv_regions().
> >>>>
> >>>> Since this commit 1a48b0be93d4 ("lmb: prohibit allocations above ram_top even from same bank"),
> >>>> this area is no more tagged as LMB_NOMAP, because it's previously been
> >>>> tagged with LMB_NOOVERWRITE in lmb_add_memory().
> >>>>
> >>>> By not being tagged LMB_NOMAP, the MMU configuration is impacted and leads to a panic.
> >>>>
> >>>> I suggest to revert this patch.
> >>>
> >>> I don't think that this patch should be reverted. If the said platform
> >>> has a reserved memory region above ram_top, I would suggest to either
> >>> a) move the ram_top on this platform so that the op-tee region gets
> >>> marked as no-map in the lmb memory map, or b) do not use the lmb
> >>
> >> In my explanation above, i indicated that before this commit,
> >> this area was marked as LMB_NOMAP in the lmb memory map by boot_fdt_add_mem_rsv_regions().
> >> this is exactly what you described in the possible solution "a)".
> >>
> >> But now with this commit, as lmb_add_memory() is called in lmb_init() the area above ram_top is marked LMB_NOOVERWRITE.
> >> Then later, boot_fdt_add_mem_rsv_regions() is executed, but the area above ram_top can't be marked as
> >> LMB_NOMAP as previously because it's already marked LMB_NOOVERWRITE.
> > 
> > This has been done to ensure that memory above ram_top is not taken
> > into consideration when it comes to U-Boot. The reason why memory
> 
> It was already the case before this commit, ram_top was designed to
> indicate to U-Boot the top address of available RAM,
> see include/asm-generic/global_data.h :
> 
> 	/**
> 	 * @ram_top: top address of RAM used by U-Boot
> 	 */
> 	phys_addr_t ram_top;
> 
> > above ram_top also needs to be added is to ensure that this memory
> > also gets passed on to the OS when booting with EFI. If it has to be
> > considered by U-Boot, the value of ram_top needs to be adjusted
> > accordingly. Is that not possible on the platform? If not, the only
> > other solution is to obtain this memory region from the DT, and then
> > configure the MMU.
> 
> Currently, ram_top is adjusted on STM32MP platforms, 
> for example in stm32mp135f-dk.dts :
> 
> 	reserved-memory {
> 		#address-cells = <1>;
> 		#size-cells = <1>;
> 		ranges;
> 
> 		optee@dd000000 {
> 			reg = <0xdd000000 0x3000000>;
> 			no-map;
> 		};
> 	};
> 
>           0xE000 0000  ********************  
>                        *                  *  
>                        *       OPTEE      *  
>                        *    (LMB_NOMAP)   *  
>                        *                  *  
> ram_top = 0xDD00 0000  ********************  
>                        *                  *
>                        *                  *
>                        *                  *
>                        *                  *
>                        *                  *
>                        *                  *
>                        *                  *
>                        *                  *
>                        *                  *
>                        *                  *
>           0xC000 0000  ********************
> 
> On STM32MP platforms, we already obtain all memory regions from DT with 
> property "no-map" and we marked them LMB_NOMAP.
> 
> Later we parse the LMB regions, all of these region marked LMB_NOMAP are 
> used to configure our MMU accordingly.
> So, again, we are doing things as you suggested.
> 
> This commit now forbids to mark OPTEE memory region with LMB_NOMAP as 
> indicated in DT.

And that's what needs to be fixed I think.  It should be allowed to
add this flag to an already existing region regardless of whether the
LMB_NOOVERWRITE flag is set (and split the region if necessary).

I wonder if it is enough to adjust the

                        if (flags == LMB_NONE) {

in lib/lmb.c:lmb_add_region_flags() into

                        if (flags == LMB_NONE || (flags & LMB_NOOVERWRITE)) {

to fix your issue.

Currently the code in boot/image-fdt.c:boot_fdt_add_mem_rsv_regions()
always adds LMB_NOOVERWRITE, which supports the view that LMB_NOMAP
adds restrictions on top of that.

> For information, it has impact on all STM32MP platforms (at least 6 boards).
> 
> Patrice
> 
> 
> 
> > 
> >>
> >>
> >>> memory map to configure the MMU -- can the MMU configuration logic not
> >>> read the DT to get which regions are to be marked as no-map?
> >>>
> >>> As far as the lmb module is concerned, it is being told through this
> >>> commit to not consider memory above ram_top for allocations, which is
> >>> not an incorrect thing imo.
> >>
> >> That's the case, we don't consider memory above ram_top for allocations,
> >> we only marked it with LMB_NOMAP.
> > 
> > That was because the lmb scope was local. That meant a platform could
> > add any size that it wanted, and then use that map for whatever it
> > fancied. The use of lmb for "allocating" addresses for io-va addresses
> > by the apple iommu is another such case.
> > 
> > -sughosh
> > 
> >>
> >> Thanks
> >> Patrice
> >>
> >>>
> >>> -sughosh
> >>>
> >>>>
> >>>> Patrice
> 

Re: [PATCH] lmb: prohibit allocations above ram_top even from same bank

[Sughosh Ganu]

On Thu, 12 Dec 2024 at 18:03, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>
> > Date: Thu, 12 Dec 2024 09:23:52 +0100
> > From: Patrice CHOTARD <patrice.chotard@foss.st.com>
> >
> > On 12/11/24 19:16, Sughosh Ganu wrote:
> > > On Wed, 11 Dec 2024 at 22:20, Patrice CHOTARD
> > > <patrice.chotard@foss.st.com> wrote:
> > >>
> > >>
> > >>
> > >> On 12/11/24 17:27, Sughosh Ganu wrote:
> > >>> On Wed, 11 Dec 2024 at 21:50, Patrice CHOTARD
> > >>> <patrice.chotard@foss.st.com> wrote:
> > >>>>
> > >>>>
> > >>>>
> > >>>> On 12/7/24 16:57, Tom Rini wrote:
> > >>>>> On Mon, 02 Dec 2024 12:36:24 +0530, Sughosh Ganu wrote:
> > >>>>>
> > >>>>>> There are platforms which set the value of ram_top based on certain
> > >>>>>> restrictions that the platform might have in accessing memory above
> > >>>>>> ram_top, even when the memory region is in the same DRAM bank. So,
> > >>>>>> even though the LMB allocator works as expected, when trying to
> > >>>>>> allocate memory above ram_top, prohibit this by marking all memory
> > >>>>>> above ram_top as reserved, even if the said memory region is from the
> > >>>>>> same bank.
> > >>>>>>
> > >>>>>> [...]
> > >>>>>
> > >>>>> Applied to u-boot/master, thanks!
> > >>>>>
> > >>>> Hello
> > >>>>
> > >>>> This patch is breaking the boot on STM32MP135F-DK.
> > >>>>
> > >>>> On this platform, we got an area above gd->ram_top,
> > >>>> this area, reserved for OPTEE, is tagged with LMB_NOMAP in boot_fdt_add_mem_rsv_regions().
> > >>>>
> > >>>> Since this commit 1a48b0be93d4 ("lmb: prohibit allocations above ram_top even from same bank"),
> > >>>> this area is no more tagged as LMB_NOMAP, because it's previously been
> > >>>> tagged with LMB_NOOVERWRITE in lmb_add_memory().
> > >>>>
> > >>>> By not being tagged LMB_NOMAP, the MMU configuration is impacted and leads to a panic.
> > >>>>
> > >>>> I suggest to revert this patch.
> > >>>
> > >>> I don't think that this patch should be reverted. If the said platform
> > >>> has a reserved memory region above ram_top, I would suggest to either
> > >>> a) move the ram_top on this platform so that the op-tee region gets
> > >>> marked as no-map in the lmb memory map, or b) do not use the lmb
> > >>
> > >> In my explanation above, i indicated that before this commit,
> > >> this area was marked as LMB_NOMAP in the lmb memory map by boot_fdt_add_mem_rsv_regions().
> > >> this is exactly what you described in the possible solution "a)".
> > >>
> > >> But now with this commit, as lmb_add_memory() is called in lmb_init() the area above ram_top is marked LMB_NOOVERWRITE.
> > >> Then later, boot_fdt_add_mem_rsv_regions() is executed, but the area above ram_top can't be marked as
> > >> LMB_NOMAP as previously because it's already marked LMB_NOOVERWRITE.
> > >
> > > This has been done to ensure that memory above ram_top is not taken
> > > into consideration when it comes to U-Boot. The reason why memory
> >
> > It was already the case before this commit, ram_top was designed to
> > indicate to U-Boot the top address of available RAM,
> > see include/asm-generic/global_data.h :
> >
> >       /**
> >        * @ram_top: top address of RAM used by U-Boot
> >        */
> >       phys_addr_t ram_top;
> >
> > > above ram_top also needs to be added is to ensure that this memory
> > > also gets passed on to the OS when booting with EFI. If it has to be
> > > considered by U-Boot, the value of ram_top needs to be adjusted
> > > accordingly. Is that not possible on the platform? If not, the only
> > > other solution is to obtain this memory region from the DT, and then
> > > configure the MMU.
> >
> > Currently, ram_top is adjusted on STM32MP platforms,
> > for example in stm32mp135f-dk.dts :
> >
> >       reserved-memory {
> >               #address-cells = <1>;
> >               #size-cells = <1>;
> >               ranges;
> >
> >               optee@dd000000 {
> >                       reg = <0xdd000000 0x3000000>;
> >                       no-map;
> >               };
> >       };
> >
> >           0xE000 0000  ********************
> >                        *                  *
> >                        *       OPTEE      *
> >                        *    (LMB_NOMAP)   *
> >                        *                  *
> > ram_top = 0xDD00 0000  ********************
> >                        *                  *
> >                        *                  *
> >                        *                  *
> >                        *                  *
> >                        *                  *
> >                        *                  *
> >                        *                  *
> >                        *                  *
> >                        *                  *
> >                        *                  *
> >           0xC000 0000  ********************
> >
> > On STM32MP platforms, we already obtain all memory regions from DT with
> > property "no-map" and we marked them LMB_NOMAP.
> >
> > Later we parse the LMB regions, all of these region marked LMB_NOMAP are
> > used to configure our MMU accordingly.
> > So, again, we are doing things as you suggested.
> >
> > This commit now forbids to mark OPTEE memory region with LMB_NOMAP as
> > indicated in DT.
>
> And that's what needs to be fixed I think.  It should be allowed to
> add this flag to an already existing region regardless of whether the
> LMB_NOOVERWRITE flag is set (and split the region if necessary).
>
> I wonder if it is enough to adjust the
>
>                         if (flags == LMB_NONE) {
>
> in lib/lmb.c:lmb_add_region_flags() into
>
>                         if (flags == LMB_NONE || (flags & LMB_NOOVERWRITE)) {
>
> to fix your issue.

I was thinking about this as a possible solution. To have an API which
allows adding flags. But then it is a slippery slope, where a more
restrictive attribute might get replaced by a less restrictive one.
Another option is to mark the region above ram_top as (LMB_NOOVERWRITE
| LMB_NOMAP). But I think instead of tinkering with the lmb code, it
would be much more prudent if the platform can handle this. In any
case, for the regions of memory below ram_top, this should not be an
issue. This is a corner case where some platforms need to configure
the memory region above ram_top. The platform can handle this
scenario.

-sughosh

>
> Currently the code in boot/image-fdt.c:boot_fdt_add_mem_rsv_regions()
> always adds LMB_NOOVERWRITE, which supports the view that LMB_NOMAP
> adds restrictions on top of that.
>
> > For information, it has impact on all STM32MP platforms (at least 6 boards).
> >
> > Patrice
> >
> >
> >
> > >
> > >>
> > >>
> > >>> memory map to configure the MMU -- can the MMU configuration logic not
> > >>> read the DT to get which regions are to be marked as no-map?
> > >>>
> > >>> As far as the lmb module is concerned, it is being told through this
> > >>> commit to not consider memory above ram_top for allocations, which is
> > >>> not an incorrect thing imo.
> > >>
> > >> That's the case, we don't consider memory above ram_top for allocations,
> > >> we only marked it with LMB_NOMAP.
> > >
> > > That was because the lmb scope was local. That meant a platform could
> > > add any size that it wanted, and then use that map for whatever it
> > > fancied. The use of lmb for "allocating" addresses for io-va addresses
> > > by the apple iommu is another such case.
> > >
> > > -sughosh
> > >
> > >>
> > >> Thanks
> > >> Patrice
> > >>
> > >>>
> > >>> -sughosh
> > >>>
> > >>>>
> > >>>> Patrice
> >

Re: [PATCH] lmb: prohibit allocations above ram_top even from same bank

[Mark Kettenis]

> From: Sughosh Ganu <sughosh.ganu@linaro.org>
> Date: Thu, 12 Dec 2024 19:02:44 +0530

Hi Sughosh,

> On Thu, 12 Dec 2024 at 18:03, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> >
> > > Date: Thu, 12 Dec 2024 09:23:52 +0100
> > > From: Patrice CHOTARD <patrice.chotard@foss.st.com>
> > >
> > > On 12/11/24 19:16, Sughosh Ganu wrote:
> > > > On Wed, 11 Dec 2024 at 22:20, Patrice CHOTARD
> > > > <patrice.chotard@foss.st.com> wrote:
> > > >>
> > > >>
> > > >>
> > > >> On 12/11/24 17:27, Sughosh Ganu wrote:
> > > >>> On Wed, 11 Dec 2024 at 21:50, Patrice CHOTARD
> > > >>> <patrice.chotard@foss.st.com> wrote:
> > > >>>>
> > > >>>>
> > > >>>>
> > > >>>> On 12/7/24 16:57, Tom Rini wrote:
> > > >>>>> On Mon, 02 Dec 2024 12:36:24 +0530, Sughosh Ganu wrote:
> > > >>>>>
> > > >>>>>> There are platforms which set the value of ram_top based on certain
> > > >>>>>> restrictions that the platform might have in accessing memory above
> > > >>>>>> ram_top, even when the memory region is in the same DRAM bank. So,
> > > >>>>>> even though the LMB allocator works as expected, when trying to
> > > >>>>>> allocate memory above ram_top, prohibit this by marking all memory
> > > >>>>>> above ram_top as reserved, even if the said memory region is from the
> > > >>>>>> same bank.
> > > >>>>>>
> > > >>>>>> [...]
> > > >>>>>
> > > >>>>> Applied to u-boot/master, thanks!
> > > >>>>>
> > > >>>> Hello
> > > >>>>
> > > >>>> This patch is breaking the boot on STM32MP135F-DK.
> > > >>>>
> > > >>>> On this platform, we got an area above gd->ram_top,
> > > >>>> this area, reserved for OPTEE, is tagged with LMB_NOMAP in boot_fdt_add_mem_rsv_regions().
> > > >>>>
> > > >>>> Since this commit 1a48b0be93d4 ("lmb: prohibit allocations above ram_top even from same bank"),
> > > >>>> this area is no more tagged as LMB_NOMAP, because it's previously been
> > > >>>> tagged with LMB_NOOVERWRITE in lmb_add_memory().
> > > >>>>
> > > >>>> By not being tagged LMB_NOMAP, the MMU configuration is impacted and leads to a panic.
> > > >>>>
> > > >>>> I suggest to revert this patch.
> > > >>>
> > > >>> I don't think that this patch should be reverted. If the said platform
> > > >>> has a reserved memory region above ram_top, I would suggest to either
> > > >>> a) move the ram_top on this platform so that the op-tee region gets
> > > >>> marked as no-map in the lmb memory map, or b) do not use the lmb
> > > >>
> > > >> In my explanation above, i indicated that before this commit,
> > > >> this area was marked as LMB_NOMAP in the lmb memory map by boot_fdt_add_mem_rsv_regions().
> > > >> this is exactly what you described in the possible solution "a)".
> > > >>
> > > >> But now with this commit, as lmb_add_memory() is called in lmb_init() the area above ram_top is marked LMB_NOOVERWRITE.
> > > >> Then later, boot_fdt_add_mem_rsv_regions() is executed, but the area above ram_top can't be marked as
> > > >> LMB_NOMAP as previously because it's already marked LMB_NOOVERWRITE.
> > > >
> > > > This has been done to ensure that memory above ram_top is not taken
> > > > into consideration when it comes to U-Boot. The reason why memory
> > >
> > > It was already the case before this commit, ram_top was designed to
> > > indicate to U-Boot the top address of available RAM,
> > > see include/asm-generic/global_data.h :
> > >
> > >       /**
> > >        * @ram_top: top address of RAM used by U-Boot
> > >        */
> > >       phys_addr_t ram_top;
> > >
> > > > above ram_top also needs to be added is to ensure that this memory
> > > > also gets passed on to the OS when booting with EFI. If it has to be
> > > > considered by U-Boot, the value of ram_top needs to be adjusted
> > > > accordingly. Is that not possible on the platform? If not, the only
> > > > other solution is to obtain this memory region from the DT, and then
> > > > configure the MMU.
> > >
> > > Currently, ram_top is adjusted on STM32MP platforms,
> > > for example in stm32mp135f-dk.dts :
> > >
> > >       reserved-memory {
> > >               #address-cells = <1>;
> > >               #size-cells = <1>;
> > >               ranges;
> > >
> > >               optee@dd000000 {
> > >                       reg = <0xdd000000 0x3000000>;
> > >                       no-map;
> > >               };
> > >       };
> > >
> > >           0xE000 0000  ********************
> > >                        *                  *
> > >                        *       OPTEE      *
> > >                        *    (LMB_NOMAP)   *
> > >                        *                  *
> > > ram_top = 0xDD00 0000  ********************
> > >                        *                  *
> > >                        *                  *
> > >                        *                  *
> > >                        *                  *
> > >                        *                  *
> > >                        *                  *
> > >                        *                  *
> > >                        *                  *
> > >                        *                  *
> > >                        *                  *
> > >           0xC000 0000  ********************
> > >
> > > On STM32MP platforms, we already obtain all memory regions from DT with
> > > property "no-map" and we marked them LMB_NOMAP.
> > >
> > > Later we parse the LMB regions, all of these region marked LMB_NOMAP are
> > > used to configure our MMU accordingly.
> > > So, again, we are doing things as you suggested.
> > >
> > > This commit now forbids to mark OPTEE memory region with LMB_NOMAP as
> > > indicated in DT.
> >
> > And that's what needs to be fixed I think.  It should be allowed to
> > add this flag to an already existing region regardless of whether the
> > LMB_NOOVERWRITE flag is set (and split the region if necessary).
> >
> > I wonder if it is enough to adjust the
> >
> >                         if (flags == LMB_NONE) {
> >
> > in lib/lmb.c:lmb_add_region_flags() into
> >
> >                         if (flags == LMB_NONE || (flags & LMB_NOOVERWRITE)) {
> >
> > to fix your issue.
> 
> I was thinking about this as a possible solution. To have an API which
> allows adding flags. But then it is a slippery slope, where a more
> restrictive attribute might get replaced by a less restrictive one.
> Another option is to mark the region above ram_top as (LMB_NOOVERWRITE
> | LMB_NOMAP).

I suppose that would work in the current codebase.  But it would make
generating the EFI memory map from the lmb memory map more difficult.
These "no-map" entries need to end up in the EFI memory map as
EFI_RESERVED_MEMORY_TYPE whereas the "usable" part above ram_top needs
to end up as EFI_BOOT_SERVICES_DATA.

> But I think instead of tinkering with the lmb code, it
> would be much more prudent if the platform can handle this. In any
> case, for the regions of memory below ram_top, this should not be an
> issue. This is a corner case where some platforms need to configure
> the memory region above ram_top. The platform can handle this
> scenario.

But are we sure the STM32 is the only platform where this problem
arises?  We're rapidly approach the 2025.1 release date and it would
be bad if several boards end up in a broken state.

Also, my point about generating the EFI memory map from the lmb memory
map still holds.  We need the LMB_NOMAP flag correctly represented in
the lmb memory map to do that.

> > Currently the code in boot/image-fdt.c:boot_fdt_add_mem_rsv_regions()
> > always adds LMB_NOOVERWRITE, which supports the view that LMB_NOMAP
> > adds restrictions on top of that.
> >
> > > For information, it has impact on all STM32MP platforms (at least 6 boards).
> > >
> > > Patrice
> > >
> > >
> > >
> > > >
> > > >>
> > > >>
> > > >>> memory map to configure the MMU -- can the MMU configuration logic not
> > > >>> read the DT to get which regions are to be marked as no-map?
> > > >>>
> > > >>> As far as the lmb module is concerned, it is being told through this
> > > >>> commit to not consider memory above ram_top for allocations, which is
> > > >>> not an incorrect thing imo.
> > > >>
> > > >> That's the case, we don't consider memory above ram_top for allocations,
> > > >> we only marked it with LMB_NOMAP.
> > > >
> > > > That was because the lmb scope was local. That meant a platform could
> > > > add any size that it wanted, and then use that map for whatever it
> > > > fancied. The use of lmb for "allocating" addresses for io-va addresses
> > > > by the apple iommu is another such case.
> > > >
> > > > -sughosh
> > > >
> > > >>
> > > >> Thanks
> > > >> Patrice
> > > >>
> > > >>>
> > > >>> -sughosh
> > > >>>
> > > >>>>
> > > >>>> Patrice
> > >
> 

Re: [PATCH] lmb: prohibit allocations above ram_top even from same bank

[Sughosh Ganu]

On Thu, 12 Dec 2024 at 20:08, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>
> > From: Sughosh Ganu <sughosh.ganu@linaro.org>
> > Date: Thu, 12 Dec 2024 19:02:44 +0530
>
> Hi Sughosh,
>
> > On Thu, 12 Dec 2024 at 18:03, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > >
> > > > Date: Thu, 12 Dec 2024 09:23:52 +0100
> > > > From: Patrice CHOTARD <patrice.chotard@foss.st.com>
> > > >
> > > > On 12/11/24 19:16, Sughosh Ganu wrote:
> > > > > On Wed, 11 Dec 2024 at 22:20, Patrice CHOTARD
> > > > > <patrice.chotard@foss.st.com> wrote:
> > > > >>
> > > > >>
> > > > >>
> > > > >> On 12/11/24 17:27, Sughosh Ganu wrote:
> > > > >>> On Wed, 11 Dec 2024 at 21:50, Patrice CHOTARD
> > > > >>> <patrice.chotard@foss.st.com> wrote:
> > > > >>>>
> > > > >>>>
> > > > >>>>
> > > > >>>> On 12/7/24 16:57, Tom Rini wrote:
> > > > >>>>> On Mon, 02 Dec 2024 12:36:24 +0530, Sughosh Ganu wrote:
> > > > >>>>>
> > > > >>>>>> There are platforms which set the value of ram_top based on certain
> > > > >>>>>> restrictions that the platform might have in accessing memory above
> > > > >>>>>> ram_top, even when the memory region is in the same DRAM bank. So,
> > > > >>>>>> even though the LMB allocator works as expected, when trying to
> > > > >>>>>> allocate memory above ram_top, prohibit this by marking all memory
> > > > >>>>>> above ram_top as reserved, even if the said memory region is from the
> > > > >>>>>> same bank.
> > > > >>>>>>
> > > > >>>>>> [...]
> > > > >>>>>
> > > > >>>>> Applied to u-boot/master, thanks!
> > > > >>>>>
> > > > >>>> Hello
> > > > >>>>
> > > > >>>> This patch is breaking the boot on STM32MP135F-DK.
> > > > >>>>
> > > > >>>> On this platform, we got an area above gd->ram_top,
> > > > >>>> this area, reserved for OPTEE, is tagged with LMB_NOMAP in boot_fdt_add_mem_rsv_regions().
> > > > >>>>
> > > > >>>> Since this commit 1a48b0be93d4 ("lmb: prohibit allocations above ram_top even from same bank"),
> > > > >>>> this area is no more tagged as LMB_NOMAP, because it's previously been
> > > > >>>> tagged with LMB_NOOVERWRITE in lmb_add_memory().
> > > > >>>>
> > > > >>>> By not being tagged LMB_NOMAP, the MMU configuration is impacted and leads to a panic.
> > > > >>>>
> > > > >>>> I suggest to revert this patch.
> > > > >>>
> > > > >>> I don't think that this patch should be reverted. If the said platform
> > > > >>> has a reserved memory region above ram_top, I would suggest to either
> > > > >>> a) move the ram_top on this platform so that the op-tee region gets
> > > > >>> marked as no-map in the lmb memory map, or b) do not use the lmb
> > > > >>
> > > > >> In my explanation above, i indicated that before this commit,
> > > > >> this area was marked as LMB_NOMAP in the lmb memory map by boot_fdt_add_mem_rsv_regions().
> > > > >> this is exactly what you described in the possible solution "a)".
> > > > >>
> > > > >> But now with this commit, as lmb_add_memory() is called in lmb_init() the area above ram_top is marked LMB_NOOVERWRITE.
> > > > >> Then later, boot_fdt_add_mem_rsv_regions() is executed, but the area above ram_top can't be marked as
> > > > >> LMB_NOMAP as previously because it's already marked LMB_NOOVERWRITE.
> > > > >
> > > > > This has been done to ensure that memory above ram_top is not taken
> > > > > into consideration when it comes to U-Boot. The reason why memory
> > > >
> > > > It was already the case before this commit, ram_top was designed to
> > > > indicate to U-Boot the top address of available RAM,
> > > > see include/asm-generic/global_data.h :
> > > >
> > > >       /**
> > > >        * @ram_top: top address of RAM used by U-Boot
> > > >        */
> > > >       phys_addr_t ram_top;
> > > >
> > > > > above ram_top also needs to be added is to ensure that this memory
> > > > > also gets passed on to the OS when booting with EFI. If it has to be
> > > > > considered by U-Boot, the value of ram_top needs to be adjusted
> > > > > accordingly. Is that not possible on the platform? If not, the only
> > > > > other solution is to obtain this memory region from the DT, and then
> > > > > configure the MMU.
> > > >
> > > > Currently, ram_top is adjusted on STM32MP platforms,
> > > > for example in stm32mp135f-dk.dts :
> > > >
> > > >       reserved-memory {
> > > >               #address-cells = <1>;
> > > >               #size-cells = <1>;
> > > >               ranges;
> > > >
> > > >               optee@dd000000 {
> > > >                       reg = <0xdd000000 0x3000000>;
> > > >                       no-map;
> > > >               };
> > > >       };
> > > >
> > > >           0xE000 0000  ********************
> > > >                        *                  *
> > > >                        *       OPTEE      *
> > > >                        *    (LMB_NOMAP)   *
> > > >                        *                  *
> > > > ram_top = 0xDD00 0000  ********************
> > > >                        *                  *
> > > >                        *                  *
> > > >                        *                  *
> > > >                        *                  *
> > > >                        *                  *
> > > >                        *                  *
> > > >                        *                  *
> > > >                        *                  *
> > > >                        *                  *
> > > >                        *                  *
> > > >           0xC000 0000  ********************
> > > >
> > > > On STM32MP platforms, we already obtain all memory regions from DT with
> > > > property "no-map" and we marked them LMB_NOMAP.
> > > >
> > > > Later we parse the LMB regions, all of these region marked LMB_NOMAP are
> > > > used to configure our MMU accordingly.
> > > > So, again, we are doing things as you suggested.
> > > >
> > > > This commit now forbids to mark OPTEE memory region with LMB_NOMAP as
> > > > indicated in DT.
> > >
> > > And that's what needs to be fixed I think.  It should be allowed to
> > > add this flag to an already existing region regardless of whether the
> > > LMB_NOOVERWRITE flag is set (and split the region if necessary).
> > >
> > > I wonder if it is enough to adjust the
> > >
> > >                         if (flags == LMB_NONE) {
> > >
> > > in lib/lmb.c:lmb_add_region_flags() into
> > >
> > >                         if (flags == LMB_NONE || (flags & LMB_NOOVERWRITE)) {
> > >
> > > to fix your issue.
> >
> > I was thinking about this as a possible solution. To have an API which
> > allows adding flags. But then it is a slippery slope, where a more
> > restrictive attribute might get replaced by a less restrictive one.
> > Another option is to mark the region above ram_top as (LMB_NOOVERWRITE
> > | LMB_NOMAP).
>
> I suppose that would work in the current codebase.  But it would make
> generating the EFI memory map from the lmb memory map more difficult.
> These "no-map" entries need to end up in the EFI memory map as
> EFI_RESERVED_MEMORY_TYPE whereas the "usable" part above ram_top needs
> to end up as EFI_BOOT_SERVICES_DATA.

The current settings are the same as what was done in the 2024.10
release, whereby the EFI memory map had all memory above ram_top being
set as boot services data. If the regions set as no-map in the DT need
to be set as reserved memory type, that logic will have to be added.
But that was not being done earlier as well. I can take this up in the
future, but I need to understand how the kernel uses the memory map
passed on by the uefi firmware.

>
> > But I think instead of tinkering with the lmb code, it
> > would be much more prudent if the platform can handle this. In any
> > case, for the regions of memory below ram_top, this should not be an
> > issue. This is a corner case where some platforms need to configure
> > the memory region above ram_top. The platform can handle this
> > scenario.
>
> But are we sure the STM32 is the only platform where this problem
> arises?  We're rapidly approach the 2025.1 release date and it would
> be bad if several boards end up in a broken state.

There might be other platforms which have reserved,no-map memory
regions above ram_top, but fwiw, it is only the ST platforms which are
setting up the mmu based on the LMB_NOMAP flag.

>
> Also, my point about generating the EFI memory map from the lmb memory
> map still holds.  We need the LMB_NOMAP flag correctly represented in
> the lmb memory map to do that.

Okay, that support will have to be added. But like I said this is not
introduced by the recent changes, as the earlier efi memory map too
had all memory above ram_top set as boot services data.

-sughosh

>
> > > Currently the code in boot/image-fdt.c:boot_fdt_add_mem_rsv_regions()
> > > always adds LMB_NOOVERWRITE, which supports the view that LMB_NOMAP
> > > adds restrictions on top of that.
> > >
> > > > For information, it has impact on all STM32MP platforms (at least 6 boards).
> > > >
> > > > Patrice
> > > >
> > > >
> > > >
> > > > >
> > > > >>
> > > > >>
> > > > >>> memory map to configure the MMU -- can the MMU configuration logic not
> > > > >>> read the DT to get which regions are to be marked as no-map?
> > > > >>>
> > > > >>> As far as the lmb module is concerned, it is being told through this
> > > > >>> commit to not consider memory above ram_top for allocations, which is
> > > > >>> not an incorrect thing imo.
> > > > >>
> > > > >> That's the case, we don't consider memory above ram_top for allocations,
> > > > >> we only marked it with LMB_NOMAP.
> > > > >
> > > > > That was because the lmb scope was local. That meant a platform could
> > > > > add any size that it wanted, and then use that map for whatever it
> > > > > fancied. The use of lmb for "allocating" addresses for io-va addresses
> > > > > by the apple iommu is another such case.
> > > > >
> > > > > -sughosh
> > > > >
> > > > >>
> > > > >> Thanks
> > > > >> Patrice
> > > > >>
> > > > >>>
> > > > >>> -sughosh
> > > > >>>
> > > > >>>>
> > > > >>>> Patrice
> > > >
> >

Re: [PATCH] lmb: prohibit allocations above ram_top even from same bank

[Mark Kettenis]

> From: Sughosh Ganu <sughosh.ganu@linaro.org>
> Date: Thu, 12 Dec 2024 20:24:18 +0530

Hi Sughosh,

> On Thu, 12 Dec 2024 at 20:08, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> >
> > > From: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > Date: Thu, 12 Dec 2024 19:02:44 +0530
> >
> > Hi Sughosh,
> >
> > > On Thu, 12 Dec 2024 at 18:03, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > > >
> > > > > Date: Thu, 12 Dec 2024 09:23:52 +0100
> > > > > From: Patrice CHOTARD <patrice.chotard@foss.st.com>
> > > > >
> > > > > On 12/11/24 19:16, Sughosh Ganu wrote:
> > > > > > On Wed, 11 Dec 2024 at 22:20, Patrice CHOTARD
> > > > > > <patrice.chotard@foss.st.com> wrote:
> > > > > >>
> > > > > >>
> > > > > >>
> > > > > >> On 12/11/24 17:27, Sughosh Ganu wrote:
> > > > > >>> On Wed, 11 Dec 2024 at 21:50, Patrice CHOTARD
> > > > > >>> <patrice.chotard@foss.st.com> wrote:
> > > > > >>>>
> > > > > >>>>
> > > > > >>>>
> > > > > >>>> On 12/7/24 16:57, Tom Rini wrote:
> > > > > >>>>> On Mon, 02 Dec 2024 12:36:24 +0530, Sughosh Ganu wrote:
> > > > > >>>>>
> > > > > >>>>>> There are platforms which set the value of ram_top based on certain
> > > > > >>>>>> restrictions that the platform might have in accessing memory above
> > > > > >>>>>> ram_top, even when the memory region is in the same DRAM bank. So,
> > > > > >>>>>> even though the LMB allocator works as expected, when trying to
> > > > > >>>>>> allocate memory above ram_top, prohibit this by marking all memory
> > > > > >>>>>> above ram_top as reserved, even if the said memory region is from the
> > > > > >>>>>> same bank.
> > > > > >>>>>>
> > > > > >>>>>> [...]
> > > > > >>>>>
> > > > > >>>>> Applied to u-boot/master, thanks!
> > > > > >>>>>
> > > > > >>>> Hello
> > > > > >>>>
> > > > > >>>> This patch is breaking the boot on STM32MP135F-DK.
> > > > > >>>>
> > > > > >>>> On this platform, we got an area above gd->ram_top,
> > > > > >>>> this area, reserved for OPTEE, is tagged with LMB_NOMAP in boot_fdt_add_mem_rsv_regions().
> > > > > >>>>
> > > > > >>>> Since this commit 1a48b0be93d4 ("lmb: prohibit allocations above ram_top even from same bank"),
> > > > > >>>> this area is no more tagged as LMB_NOMAP, because it's previously been
> > > > > >>>> tagged with LMB_NOOVERWRITE in lmb_add_memory().
> > > > > >>>>
> > > > > >>>> By not being tagged LMB_NOMAP, the MMU configuration is impacted and leads to a panic.
> > > > > >>>>
> > > > > >>>> I suggest to revert this patch.
> > > > > >>>
> > > > > >>> I don't think that this patch should be reverted. If the said platform
> > > > > >>> has a reserved memory region above ram_top, I would suggest to either
> > > > > >>> a) move the ram_top on this platform so that the op-tee region gets
> > > > > >>> marked as no-map in the lmb memory map, or b) do not use the lmb
> > > > > >>
> > > > > >> In my explanation above, i indicated that before this commit,
> > > > > >> this area was marked as LMB_NOMAP in the lmb memory map by boot_fdt_add_mem_rsv_regions().
> > > > > >> this is exactly what you described in the possible solution "a)".
> > > > > >>
> > > > > >> But now with this commit, as lmb_add_memory() is called in lmb_init() the area above ram_top is marked LMB_NOOVERWRITE.
> > > > > >> Then later, boot_fdt_add_mem_rsv_regions() is executed, but the area above ram_top can't be marked as
> > > > > >> LMB_NOMAP as previously because it's already marked LMB_NOOVERWRITE.
> > > > > >
> > > > > > This has been done to ensure that memory above ram_top is not taken
> > > > > > into consideration when it comes to U-Boot. The reason why memory
> > > > >
> > > > > It was already the case before this commit, ram_top was designed to
> > > > > indicate to U-Boot the top address of available RAM,
> > > > > see include/asm-generic/global_data.h :
> > > > >
> > > > >       /**
> > > > >        * @ram_top: top address of RAM used by U-Boot
> > > > >        */
> > > > >       phys_addr_t ram_top;
> > > > >
> > > > > > above ram_top also needs to be added is to ensure that this memory
> > > > > > also gets passed on to the OS when booting with EFI. If it has to be
> > > > > > considered by U-Boot, the value of ram_top needs to be adjusted
> > > > > > accordingly. Is that not possible on the platform? If not, the only
> > > > > > other solution is to obtain this memory region from the DT, and then
> > > > > > configure the MMU.
> > > > >
> > > > > Currently, ram_top is adjusted on STM32MP platforms,
> > > > > for example in stm32mp135f-dk.dts :
> > > > >
> > > > >       reserved-memory {
> > > > >               #address-cells = <1>;
> > > > >               #size-cells = <1>;
> > > > >               ranges;
> > > > >
> > > > >               optee@dd000000 {
> > > > >                       reg = <0xdd000000 0x3000000>;
> > > > >                       no-map;
> > > > >               };
> > > > >       };
> > > > >
> > > > >           0xE000 0000  ********************
> > > > >                        *                  *
> > > > >                        *       OPTEE      *
> > > > >                        *    (LMB_NOMAP)   *
> > > > >                        *                  *
> > > > > ram_top = 0xDD00 0000  ********************
> > > > >                        *                  *
> > > > >                        *                  *
> > > > >                        *                  *
> > > > >                        *                  *
> > > > >                        *                  *
> > > > >                        *                  *
> > > > >                        *                  *
> > > > >                        *                  *
> > > > >                        *                  *
> > > > >                        *                  *
> > > > >           0xC000 0000  ********************
> > > > >
> > > > > On STM32MP platforms, we already obtain all memory regions from DT with
> > > > > property "no-map" and we marked them LMB_NOMAP.
> > > > >
> > > > > Later we parse the LMB regions, all of these region marked LMB_NOMAP are
> > > > > used to configure our MMU accordingly.
> > > > > So, again, we are doing things as you suggested.
> > > > >
> > > > > This commit now forbids to mark OPTEE memory region with LMB_NOMAP as
> > > > > indicated in DT.
> > > >
> > > > And that's what needs to be fixed I think.  It should be allowed to
> > > > add this flag to an already existing region regardless of whether the
> > > > LMB_NOOVERWRITE flag is set (and split the region if necessary).
> > > >
> > > > I wonder if it is enough to adjust the
> > > >
> > > >                         if (flags == LMB_NONE) {
> > > >
> > > > in lib/lmb.c:lmb_add_region_flags() into
> > > >
> > > >                         if (flags == LMB_NONE || (flags & LMB_NOOVERWRITE)) {
> > > >
> > > > to fix your issue.
> > >
> > > I was thinking about this as a possible solution. To have an API which
> > > allows adding flags. But then it is a slippery slope, where a more
> > > restrictive attribute might get replaced by a less restrictive one.
> > > Another option is to mark the region above ram_top as (LMB_NOOVERWRITE
> > > | LMB_NOMAP).
> >
> > I suppose that would work in the current codebase.  But it would make
> > generating the EFI memory map from the lmb memory map more difficult.
> > These "no-map" entries need to end up in the EFI memory map as
> > EFI_RESERVED_MEMORY_TYPE whereas the "usable" part above ram_top needs
> > to end up as EFI_BOOT_SERVICES_DATA.
> 
> The current settings are the same as what was done in the 2024.10
> release, whereby the EFI memory map had all memory above ram_top being
> set as boot services data. If the regions set as no-map in the DT need
> to be set as reserved memory type, that logic will have to be added.

There is logic to do that already.  The efi_loader code in U-Boot
makes another pass over the reserved regions in the device tree and
adjust the EFI memory map.

> But that was not being done earlier as well. I can take this up in the
> future, but I need to understand how the kernel uses the memory map
> passed on by the uefi firmware.

It isn't really the kernel, but the EFI application that matters here.
Typically that EFI application is a bootloader of some sorts.  Many
OSes have their own, Linux has several.  But it can also be the EFI
stub of a Linux kernel.

Typical usage is that the EFI application will only use memory marked
as EFI_CONVENTIONAL_MEMORY up until it calls ExitBootServices().  The
OpenBSD bootloader for example walks the EFI memory map to find a
suitable location to load the OpenBSD kernel.

After ExitBootServices() has been called it will also use memory
marked as EFI_BOOT_SERVICES_DATA (and potentially some other memory
types that U-Boot doesn't use for now).

> > > But I think instead of tinkering with the lmb code, it
> > > would be much more prudent if the platform can handle this. In any
> > > case, for the regions of memory below ram_top, this should not be an
> > > issue. This is a corner case where some platforms need to configure
> > > the memory region above ram_top. The platform can handle this
> > > scenario.
> >
> > But are we sure the STM32 is the only platform where this problem
> > arises?  We're rapidly approach the 2025.1 release date and it would
> > be bad if several boards end up in a broken state.
> 
> There might be other platforms which have reserved,no-map memory
> regions above ram_top, but fwiw, it is only the ST platforms which are
> setting up the mmu based on the LMB_NOMAP flag.

You're right.  Only STM32 uses the lmb_is_reserved_flags() API at this
moment.  So it is likely that that is indeed the only affected platform.

> > Also, my point about generating the EFI memory map from the lmb memory
> > map still holds.  We need the LMB_NOMAP flag correctly represented in
> > the lmb memory map to do that.
> 
> Okay, that support will have to be added. But like I said this is not
> introduced by the recent changes, as the earlier efi memory map too
> had all memory above ram_top set as boot services data.
> 
> -sughosh
> 
> >
> > > > Currently the code in boot/image-fdt.c:boot_fdt_add_mem_rsv_regions()
> > > > always adds LMB_NOOVERWRITE, which supports the view that LMB_NOMAP
> > > > adds restrictions on top of that.
> > > >
> > > > > For information, it has impact on all STM32MP platforms (at least 6 boards).
> > > > >
> > > > > Patrice
> > > > >
> > > > >
> > > > >
> > > > > >
> > > > > >>
> > > > > >>
> > > > > >>> memory map to configure the MMU -- can the MMU configuration logic not
> > > > > >>> read the DT to get which regions are to be marked as no-map?
> > > > > >>>
> > > > > >>> As far as the lmb module is concerned, it is being told through this
> > > > > >>> commit to not consider memory above ram_top for allocations, which is
> > > > > >>> not an incorrect thing imo.
> > > > > >>
> > > > > >> That's the case, we don't consider memory above ram_top for allocations,
> > > > > >> we only marked it with LMB_NOMAP.
> > > > > >
> > > > > > That was because the lmb scope was local. That meant a platform could
> > > > > > add any size that it wanted, and then use that map for whatever it
> > > > > > fancied. The use of lmb for "allocating" addresses for io-va addresses
> > > > > > by the apple iommu is another such case.
> > > > > >
> > > > > > -sughosh
> > > > > >
> > > > > >>
> > > > > >> Thanks
> > > > > >> Patrice
> > > > > >>
> > > > > >>>
> > > > > >>> -sughosh
> > > > > >>>
> > > > > >>>>
> > > > > >>>> Patrice
> > > > >
> > >
> 

Re: [PATCH] lmb: prohibit allocations above ram_top even from same bank

[Sughosh Ganu]

On Thu, 12 Dec 2024 at 13:58, Patrice CHOTARD
<patrice.chotard@foss.st.com> wrote:
>
>
>
> On 12/11/24 19:16, Sughosh Ganu wrote:
> > On Wed, 11 Dec 2024 at 22:20, Patrice CHOTARD
> > <patrice.chotard@foss.st.com> wrote:
> >>
> >>
> >>
> >> On 12/11/24 17:27, Sughosh Ganu wrote:
> >>> On Wed, 11 Dec 2024 at 21:50, Patrice CHOTARD
> >>> <patrice.chotard@foss.st.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 12/7/24 16:57, Tom Rini wrote:
> >>>>> On Mon, 02 Dec 2024 12:36:24 +0530, Sughosh Ganu wrote:
> >>>>>
> >>>>>> There are platforms which set the value of ram_top based on certain
> >>>>>> restrictions that the platform might have in accessing memory above
> >>>>>> ram_top, even when the memory region is in the same DRAM bank. So,
> >>>>>> even though the LMB allocator works as expected, when trying to
> >>>>>> allocate memory above ram_top, prohibit this by marking all memory
> >>>>>> above ram_top as reserved, even if the said memory region is from the
> >>>>>> same bank.
> >>>>>>
> >>>>>> [...]
> >>>>>
> >>>>> Applied to u-boot/master, thanks!
> >>>>>
> >>>> Hello
> >>>>
> >>>> This patch is breaking the boot on STM32MP135F-DK.
> >>>>
> >>>> On this platform, we got an area above gd->ram_top,
> >>>> this area, reserved for OPTEE, is tagged with LMB_NOMAP in boot_fdt_add_mem_rsv_regions().
> >>>>
> >>>> Since this commit 1a48b0be93d4 ("lmb: prohibit allocations above ram_top even from same bank"),
> >>>> this area is no more tagged as LMB_NOMAP, because it's previously been
> >>>> tagged with LMB_NOOVERWRITE in lmb_add_memory().
> >>>>
> >>>> By not being tagged LMB_NOMAP, the MMU configuration is impacted and leads to a panic.
> >>>>
> >>>> I suggest to revert this patch.
> >>>
> >>> I don't think that this patch should be reverted. If the said platform
> >>> has a reserved memory region above ram_top, I would suggest to either
> >>> a) move the ram_top on this platform so that the op-tee region gets
> >>> marked as no-map in the lmb memory map, or b) do not use the lmb
> >>
> >> In my explanation above, i indicated that before this commit,
> >> this area was marked as LMB_NOMAP in the lmb memory map by boot_fdt_add_mem_rsv_regions().
> >> this is exactly what you described in the possible solution "a)".
> >>
> >> But now with this commit, as lmb_add_memory() is called in lmb_init() the area above ram_top is marked LMB_NOOVERWRITE.
> >> Then later, boot_fdt_add_mem_rsv_regions() is executed, but the area above ram_top can't be marked as
> >> LMB_NOMAP as previously because it's already marked LMB_NOOVERWRITE.
> >
> > This has been done to ensure that memory above ram_top is not taken
> > into consideration when it comes to U-Boot. The reason why memory
>
> It was already the case before this commit, ram_top was designed to
> indicate to U-Boot the top address of available RAM,

With earlier lmb code, it was a function of how the API's were being
used. Specifically the amount of RAM memory that would get added. It
was very much possible to allocate memory above ram_top [1], something
which is not so now.

> see include/asm-generic/global_data.h :
>
>         /**
>          * @ram_top: top address of RAM used by U-Boot
>          */
>         phys_addr_t ram_top;
>
> > above ram_top also needs to be added is to ensure that this memory
> > also gets passed on to the OS when booting with EFI. If it has to be
> > considered by U-Boot, the value of ram_top needs to be adjusted
> > accordingly. Is that not possible on the platform? If not, the only
> > other solution is to obtain this memory region from the DT, and then
> > configure the MMU.
>
> Currently, ram_top is adjusted on STM32MP platforms,
> for example in stm32mp135f-dk.dts :
>
>         reserved-memory {
>                 #address-cells = <1>;
>                 #size-cells = <1>;
>                 ranges;
>
>                 optee@dd000000 {
>                         reg = <0xdd000000 0x3000000>;
>                         no-map;
>                 };
>         };
>
>           0xE000 0000  ********************
>                        *                  *
>                        *       OPTEE      *
>                        *    (LMB_NOMAP)   *
>                        *                  *
> ram_top = 0xDD00 0000  ********************
>                        *                  *
>                        *                  *
>                        *                  *
>                        *                  *
>                        *                  *
>                        *                  *
>                        *                  *
>                        *                  *
>                        *                  *
>                        *                  *
>           0xC000 0000  ********************
>
> On STM32MP platforms, we already obtain all memory regions from DT with
> property "no-map" and we marked them LMB_NOMAP.
>
> Later we parse the LMB regions, all of these region marked LMB_NOMAP are
> used to configure our MMU accordingly.
> So, again, we are doing things as you suggested.
>
> This commit now forbids to mark OPTEE memory region with LMB_NOMAP as
> indicated in DT.
>
> For information, it has impact on all STM32MP platforms (at least 6 boards).

So this is indeed the case of all of the memory region above ram_top
being marked as no-map in the DT. I do have a ST DK2 board with me,
but I do not see a hang on that board. Not sure why. Can we not put a
check in the mmu configuration function to cover the case of memory
above ram_top. Something like this.

diff --git a/arch/arm/mach-stm32mp/stm32mp1/cpu.c
b/arch/arm/mach-stm32mp/stm32mp1/cpu.c
index 62cc98910a7..55ac62679ff 100644
--- a/arch/arm/mach-stm32mp/stm32mp1/cpu.c
+++ b/arch/arm/mach-stm32mp/stm32mp1/cpu.c
@@ -77,8 +77,13 @@ void dram_bank_mmu_setup(int bank)
        for (i = start >> MMU_SECTION_SHIFT;
             i < (start >> MMU_SECTION_SHIFT) + (size >> MMU_SECTION_SHIFT);
             i++) {
+               phys_addr_t addr;
+
+               addr = i << MMU_SECTION_SHIFT;
                option = DCACHE_DEFAULT_OPTION;
-               if (use_lmb && lmb_is_reserved_flags(i <<
MMU_SECTION_SHIFT, LMB_NOMAP))
+               if (use_lmb && (lmb_is_reserved_flags(i << MMU_SECTION_SHIFT,
+                                                     LMB_NOMAP) ||
+                               addr >= gd->ram_top))
                        option = 0; /* INVALID ENTRY in TLB */
                set_section_dcache(i, option);
        }

-sughosh

[1] - https://gist.github.com/sughoshg/1a03f1af93bb75db10a4a1df3265ebf0

>
> Patrice
>
>
>
> >
> >>
> >>
> >>> memory map to configure the MMU -- can the MMU configuration logic not
> >>> read the DT to get which regions are to be marked as no-map?
> >>>
> >>> As far as the lmb module is concerned, it is being told through this
> >>> commit to not consider memory above ram_top for allocations, which is
> >>> not an incorrect thing imo.
> >>
> >> That's the case, we don't consider memory above ram_top for allocations,
> >> we only marked it with LMB_NOMAP.
> >
> > That was because the lmb scope was local. That meant a platform could
> > add any size that it wanted, and then use that map for whatever it
> > fancied. The use of lmb for "allocating" addresses for io-va addresses
> > by the apple iommu is another such case.
> >
> > -sughosh
> >
> >>
> >> Thanks
> >> Patrice
> >>
> >>>
> >>> -sughosh
> >>>
> >>>>
> >>>> Patrice

Re: [PATCH] lmb: prohibit allocations above ram_top even from same bank

[Tom Rini]

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

On Wed, Dec 11, 2024 at 05:18:28PM +0100, Patrice CHOTARD wrote:

> 
> 
> On 12/7/24 16:57, Tom Rini wrote:
> > On Mon, 02 Dec 2024 12:36:24 +0530, Sughosh Ganu wrote:
> > 
> >> There are platforms which set the value of ram_top based on certain
> >> restrictions that the platform might have in accessing memory above
> >> ram_top, even when the memory region is in the same DRAM bank. So,
> >> even though the LMB allocator works as expected, when trying to
> >> allocate memory above ram_top, prohibit this by marking all memory
> >> above ram_top as reserved, even if the said memory region is from the
> >> same bank.
> >>
> >> [...]
> > 
> > Applied to u-boot/master, thanks!
> > 
> Hello
> 
> This patch is breaking the boot on STM32MP135F-DK.
> 
> On this platform, we got an area above gd->ram_top, 
> this area, reserved for OPTEE, is tagged with LMB_NOMAP in boot_fdt_add_mem_rsv_regions().
> 
> Since this commit 1a48b0be93d4 ("lmb: prohibit allocations above ram_top even from same bank"),
> this area is no more tagged as LMB_NOMAP, because it's previously been 
> tagged with LMB_NOOVERWRITE in lmb_add_memory().
> 
> By not being tagged LMB_NOMAP, the MMU configuration is impacted and leads to a panic.
> 
> I suggest to revert this patch.

Hi Patrice. Can you please re-test current top of tree? I believe Sam's
series should resolve this problem as well. Thanks!

-- 
Tom

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

Re: [PATCH] lmb: prohibit allocations above ram_top even from same bank

[Sughosh Ganu]

On Fri, 13 Dec 2024 at 00:48, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Dec 11, 2024 at 05:18:28PM +0100, Patrice CHOTARD wrote:
>
> >
> >
> > On 12/7/24 16:57, Tom Rini wrote:
> > > On Mon, 02 Dec 2024 12:36:24 +0530, Sughosh Ganu wrote:
> > >
> > >> There are platforms which set the value of ram_top based on certain
> > >> restrictions that the platform might have in accessing memory above
> > >> ram_top, even when the memory region is in the same DRAM bank. So,
> > >> even though the LMB allocator works as expected, when trying to
> > >> allocate memory above ram_top, prohibit this by marking all memory
> > >> above ram_top as reserved, even if the said memory region is from the
> > >> same bank.
> > >>
> > >> [...]
> > >
> > > Applied to u-boot/master, thanks!
> > >
> > Hello
> >
> > This patch is breaking the boot on STM32MP135F-DK.
> >
> > On this platform, we got an area above gd->ram_top,
> > this area, reserved for OPTEE, is tagged with LMB_NOMAP in boot_fdt_add_mem_rsv_regions().
> >
> > Since this commit 1a48b0be93d4 ("lmb: prohibit allocations above ram_top even from same bank"),
> > this area is no more tagged as LMB_NOMAP, because it's previously been
> > tagged with LMB_NOOVERWRITE in lmb_add_memory().
> >
> > By not being tagged LMB_NOMAP, the MMU configuration is impacted and leads to a panic.
> >
> > I suggest to revert this patch.
>
> Hi Patrice. Can you please re-test current top of tree? I believe Sam's
> series should resolve this problem as well. Thanks!

Tom, that won't happen. This is a separate issue from what is being
fixed by Sam's series.

Patrice, please let me know if the hang is being fixed by the change
that I had suggested earlier in this thread. For some reason I don't
see a hang on the DK2 board despite the board having a similar memory
map of memory being reserved for the op-tee image above ram_top with
no-map property.

-sughosh

>
> --
> Tom

Re: [PATCH] lmb: prohibit allocations above ram_top even from same bank

[Patrice CHOTARD]

On 12/12/24 20:50, Sughosh Ganu wrote:
> On Fri, 13 Dec 2024 at 00:48, Tom Rini <trini@konsulko.com> wrote:
>>
>> On Wed, Dec 11, 2024 at 05:18:28PM +0100, Patrice CHOTARD wrote:
>>
>>>
>>>
>>> On 12/7/24 16:57, Tom Rini wrote:
>>>> On Mon, 02 Dec 2024 12:36:24 +0530, Sughosh Ganu wrote:
>>>>
>>>>> There are platforms which set the value of ram_top based on certain
>>>>> restrictions that the platform might have in accessing memory above
>>>>> ram_top, even when the memory region is in the same DRAM bank. So,
>>>>> even though the LMB allocator works as expected, when trying to
>>>>> allocate memory above ram_top, prohibit this by marking all memory
>>>>> above ram_top as reserved, even if the said memory region is from the
>>>>> same bank.
>>>>>
>>>>> [...]
>>>>
>>>> Applied to u-boot/master, thanks!
>>>>
>>> Hello
>>>
>>> This patch is breaking the boot on STM32MP135F-DK.
>>>
>>> On this platform, we got an area above gd->ram_top,
>>> this area, reserved for OPTEE, is tagged with LMB_NOMAP in boot_fdt_add_mem_rsv_regions().
>>>
>>> Since this commit 1a48b0be93d4 ("lmb: prohibit allocations above ram_top even from same bank"),
>>> this area is no more tagged as LMB_NOMAP, because it's previously been
>>> tagged with LMB_NOOVERWRITE in lmb_add_memory().
>>>
>>> By not being tagged LMB_NOMAP, the MMU configuration is impacted and leads to a panic.
>>>
>>> I suggest to revert this patch.
>>
>> Hi Patrice. Can you please re-test current top of tree? I believe Sam's
>> series should resolve this problem as well. Thanks!
> 
> Tom, that won't happen. This is a separate issue from what is being
> fixed by Sam's series.

Hi All

I confirm, Sam's series didn't help here as Sughosh said.

> 
> Patrice, please let me know if the hang is being fixed by the change
> that I had suggested earlier in this thread. For some reason I don't
> see a hang on the DK2 board despite the board having a similar memory
> map of memory being reserved for the op-tee image above ram_top with
> no-map property.

On my side i didn't see the issue on STM32MP157C-DK2 board, but 
i got it systematically on STM2MP135F-DK.

Your suggested patch is fixing the hang observed, i will send a patch.

Thanks all ;-)

Patrice


> 
> -sughosh
> 
>>
>> --
>> Tom

Re: [PATCH] lmb: prohibit allocations above ram_top even from same bank

[Sughosh Ganu]

On Fri, 13 Dec 2024 at 14:44, Patrice CHOTARD
<patrice.chotard@foss.st.com> wrote:
>
>
>
> On 12/12/24 20:50, Sughosh Ganu wrote:
> > On Fri, 13 Dec 2024 at 00:48, Tom Rini <trini@konsulko.com> wrote:
> >>
> >> On Wed, Dec 11, 2024 at 05:18:28PM +0100, Patrice CHOTARD wrote:
> >>
> >>>
> >>>
> >>> On 12/7/24 16:57, Tom Rini wrote:
> >>>> On Mon, 02 Dec 2024 12:36:24 +0530, Sughosh Ganu wrote:
> >>>>
> >>>>> There are platforms which set the value of ram_top based on certain
> >>>>> restrictions that the platform might have in accessing memory above
> >>>>> ram_top, even when the memory region is in the same DRAM bank. So,
> >>>>> even though the LMB allocator works as expected, when trying to
> >>>>> allocate memory above ram_top, prohibit this by marking all memory
> >>>>> above ram_top as reserved, even if the said memory region is from the
> >>>>> same bank.
> >>>>>
> >>>>> [...]
> >>>>
> >>>> Applied to u-boot/master, thanks!
> >>>>
> >>> Hello
> >>>
> >>> This patch is breaking the boot on STM32MP135F-DK.
> >>>
> >>> On this platform, we got an area above gd->ram_top,
> >>> this area, reserved for OPTEE, is tagged with LMB_NOMAP in boot_fdt_add_mem_rsv_regions().
> >>>
> >>> Since this commit 1a48b0be93d4 ("lmb: prohibit allocations above ram_top even from same bank"),
> >>> this area is no more tagged as LMB_NOMAP, because it's previously been
> >>> tagged with LMB_NOOVERWRITE in lmb_add_memory().
> >>>
> >>> By not being tagged LMB_NOMAP, the MMU configuration is impacted and leads to a panic.
> >>>
> >>> I suggest to revert this patch.
> >>
> >> Hi Patrice. Can you please re-test current top of tree? I believe Sam's
> >> series should resolve this problem as well. Thanks!
> >
> > Tom, that won't happen. This is a separate issue from what is being
> > fixed by Sam's series.
>
> Hi All
>
> I confirm, Sam's series didn't help here as Sughosh said.
>
> >
> > Patrice, please let me know if the hang is being fixed by the change
> > that I had suggested earlier in this thread. For some reason I don't
> > see a hang on the DK2 board despite the board having a similar memory
> > map of memory being reserved for the op-tee image above ram_top with
> > no-map property.
>
> On my side i didn't see the issue on STM32MP157C-DK2 board, but
> i got it systematically on STM2MP135F-DK.
>
> Your suggested patch is fixing the hang observed, i will send a patch.

Patrice, thanks much for testing and confirming the fix. I suspect
that the cpu's being used on the two boards might be different. Looks
like one implementation is speculatively prefetching data from the
op-tee memory region while other is not, which might explain why this
is not seen on all ST boards. Just a hunch though :)

-sughosh

>
> Thanks all ;-)
>
> Patrice
>
>
> >
> > -sughosh
> >
> >>
> >> --
> >> Tom

Re: [PATCH] lmb: prohibit allocations above ram_top even from same bank

[Patrice CHOTARD]

On 12/13/24 10:54, Sughosh Ganu wrote:
> On Fri, 13 Dec 2024 at 14:44, Patrice CHOTARD
> <patrice.chotard@foss.st.com> wrote:
>>
>>
>>
>> On 12/12/24 20:50, Sughosh Ganu wrote:
>>> On Fri, 13 Dec 2024 at 00:48, Tom Rini <trini@konsulko.com> wrote:
>>>>
>>>> On Wed, Dec 11, 2024 at 05:18:28PM +0100, Patrice CHOTARD wrote:
>>>>
>>>>>
>>>>>
>>>>> On 12/7/24 16:57, Tom Rini wrote:
>>>>>> On Mon, 02 Dec 2024 12:36:24 +0530, Sughosh Ganu wrote:
>>>>>>
>>>>>>> There are platforms which set the value of ram_top based on certain
>>>>>>> restrictions that the platform might have in accessing memory above
>>>>>>> ram_top, even when the memory region is in the same DRAM bank. So,
>>>>>>> even though the LMB allocator works as expected, when trying to
>>>>>>> allocate memory above ram_top, prohibit this by marking all memory
>>>>>>> above ram_top as reserved, even if the said memory region is from the
>>>>>>> same bank.
>>>>>>>
>>>>>>> [...]
>>>>>>
>>>>>> Applied to u-boot/master, thanks!
>>>>>>
>>>>> Hello
>>>>>
>>>>> This patch is breaking the boot on STM32MP135F-DK.
>>>>>
>>>>> On this platform, we got an area above gd->ram_top,
>>>>> this area, reserved for OPTEE, is tagged with LMB_NOMAP in boot_fdt_add_mem_rsv_regions().
>>>>>
>>>>> Since this commit 1a48b0be93d4 ("lmb: prohibit allocations above ram_top even from same bank"),
>>>>> this area is no more tagged as LMB_NOMAP, because it's previously been
>>>>> tagged with LMB_NOOVERWRITE in lmb_add_memory().
>>>>>
>>>>> By not being tagged LMB_NOMAP, the MMU configuration is impacted and leads to a panic.
>>>>>
>>>>> I suggest to revert this patch.
>>>>
>>>> Hi Patrice. Can you please re-test current top of tree? I believe Sam's
>>>> series should resolve this problem as well. Thanks!
>>>
>>> Tom, that won't happen. This is a separate issue from what is being
>>> fixed by Sam's series.
>>
>> Hi All
>>
>> I confirm, Sam's series didn't help here as Sughosh said.
>>
>>>
>>> Patrice, please let me know if the hang is being fixed by the change
>>> that I had suggested earlier in this thread. For some reason I don't
>>> see a hang on the DK2 board despite the board having a similar memory
>>> map of memory being reserved for the op-tee image above ram_top with
>>> no-map property.
>>
>> On my side i didn't see the issue on STM32MP157C-DK2 board, but
>> i got it systematically on STM2MP135F-DK.
>>
>> Your suggested patch is fixing the hang observed, i will send a patch.
> 
> Patrice, thanks much for testing and confirming the fix. I suspect
> that the cpu's being used on the two boards might be different. Looks
> like one implementation is speculatively prefetching data from the
> op-tee memory region while other is not, which might explain why this
> is not seen on all ST boards. Just a hunch though :)

That's exactly what we think too ;-)

> 
> -sughosh
> 
>>
>> Thanks all ;-)
>>
>> Patrice
>>
>>
>>>
>>> -sughosh
>>>
>>>>
>>>> --
>>>> Tom