On Thu, 12 Dec 2024 at 13:58, Patrice CHOTARD
<patrice.chot...@foss.st.com> wrote:
>
>
>
> 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,
With earlier lmb code, it was a function of how the API's were being
used. Specifically the amount of RAM memory that would get added. It
was very much possible to allocate memory above ram_top [1], something
which is not so now.
> 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.
>
> For information, it has impact on all STM32MP platforms (at least 6 boards).
So this is indeed the case of all of the memory region above ram_top
being marked as no-map in the DT. I do have a ST DK2 board with me,
but I do not see a hang on that board. Not sure why. Can we not put a
check in the mmu configuration function to cover the case of memory
above ram_top. Something like this.
diff --git a/arch/arm/mach-stm32mp/stm32mp1/cpu.c
b/arch/arm/mach-stm32mp/stm32mp1/cpu.c
index 62cc98910a7..55ac62679ff 100644
--- a/arch/arm/mach-stm32mp/stm32mp1/cpu.c
+++ b/arch/arm/mach-stm32mp/stm32mp1/cpu.c
@@ -77,8 +77,13 @@ void dram_bank_mmu_setup(int bank)
for (i = start >> MMU_SECTION_SHIFT;
i < (start >> MMU_SECTION_SHIFT) + (size >> MMU_SECTION_SHIFT);
i++) {
+ phys_addr_t addr;
+
+ addr = i << MMU_SECTION_SHIFT;
option = DCACHE_DEFAULT_OPTION;
- if (use_lmb && lmb_is_reserved_flags(i <<
MMU_SECTION_SHIFT, LMB_NOMAP))
+ if (use_lmb && (lmb_is_reserved_flags(i << MMU_SECTION_SHIFT,
+ LMB_NOMAP) ||
+ addr >= gd->ram_top))
option = 0; /* INVALID ENTRY in TLB */
set_section_dcache(i, option);
}
-sughosh
[1] - https://gist.github.com/sughoshg/1a03f1af93bb75db10a4a1df3265ebf0
>
> 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
