[PATCH] lmb: prohibit allocations above ram_top even from same bank
Sughosh Ganu
sughosh.ganu at linaro.org
Thu Dec 12 15:54:18 CET 2024
On Thu, 12 Dec 2024 at 20:08, Mark Kettenis <mark.kettenis at xs4all.nl> wrote:
>
> > 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.
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
> > > >
> >
More information about the U-Boot
mailing list