> Date: Thu, 12 Dec 2024 09:23:52 +0100
> From: Patrice CHOTARD <patrice.chot...@foss.st.com>
>
> On 12/11/24 19:16, Sughosh Ganu wrote:
> > On Wed, 11 Dec 2024 at 22:20, Patrice CHOTARD
> > <patrice.chot...@foss.st.com> wrote:
> >>
> >>
> >>
> >> On 12/11/24 17:27, Sughosh Ganu wrote:
> >>> On Wed, 11 Dec 2024 at 21:50, Patrice CHOTARD
> >>> <patrice.chot...@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
>
