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