[PATCH] lmb: prohibit allocations above ram_top even from same bank
Mark Kettenis
mark.kettenis at xs4all.nl
Thu Dec 12 15:38:48 CET 2024
> From: Sughosh Ganu <sughosh.ganu at 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 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).
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
> > >
>
More information about the U-Boot
mailing list