On Thu, 12 Dec 2024 at 20:08, Mark Kettenis <mark.kette...@xs4all.nl> wrote: > > > From: Sughosh Ganu <sughosh.g...@linaro.org> > > Date: Thu, 12 Dec 2024 19:02:44 +0530 > > Hi Sughosh, > > > On Thu, 12 Dec 2024 at 18:03, Mark Kettenis <mark.kette...@xs4all.nl> wrote: > > > > > > > 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. > > > > 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 > > > > > >