| Message ID | 20241202070624.1295497-1-sughosh.ganu@linaro.org |
|---|---|
| State | Accepted |
| Commit | 1a48b0be93d48f9ec5498297ec775fb572d3d150 |
| Headers | show |
| Series | lmb: prohibit allocations above ram_top even from same bank | expand |
On Dez 02 2024, 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. This fixes the boot failure on the VisionFive2 board as reported in <mvm8qtnv310.fsf@suse.de>. Tested-by: Andreas Schwab <schwab@suse.de>
On Mon, 2 Dec 2024 at 19:16, E Shattow <lucent@gmail.com> wrote: > > Hi Sughosh, Andreas, et. al > > On Mon, Dec 2, 2024 at 3:58 AM Andreas Schwab <schwab@suse.de> wrote: > > > > On Dez 02 2024, 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. > > > > This fixes the boot failure on the VisionFive2 board as reported in > > <mvm8qtnv310.fsf@suse.de>. > > > > Tested-by: Andreas Schwab <schwab@suse.de> > > > > -- > > Andreas Schwab, SUSE Labs, schwab@suse.de > > GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 > > "And now for something completely different." > > Echoing what Andreas finds in testing, this avoids the platform issue > with dw_mmc driver and the visionfive2 board support on 4GB Star64, > 4GB Mars CM Lite, and 8GB Mars CM Lite WiFi. > > > diff --git a/lib/lmb.c b/lib/lmb.c > > index 14b9b8466ff..be168dfb8dc 100644 > > --- a/lib/lmb.c > > +++ b/lib/lmb.c > > @@ -615,6 +615,7 @@ static __maybe_unused void lmb_reserve_common_spl(void) > > void lmb_add_memory(void) > > { > > int i; > > + phys_addr_t bank_end; > > phys_size_t size; > > u64 ram_top = gd->ram_top; > > struct bd_info *bd = gd->bd; > > @@ -628,6 +629,8 @@ void lmb_add_memory(void) > > > > for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) { > > size = bd->bi_dram[i].size; > > + bank_end = bd->bi_dram[i].start + size; > > + > > Delete newline? Unless existing style is wrong here. It's not about style, but the above two lines are more of an initialisation within the block, which is the reason for a newline here. > > > if (size) { > > lmb_add(bd->bi_dram[i].start, size); > > > > @@ -639,6 +642,9 @@ void lmb_add_memory(void) > > if (bd->bi_dram[i].start >= ram_top) > > lmb_reserve_flags(bd->bi_dram[i].start, size, > > LMB_NOOVERWRITE); > > + else if (bank_end > ram_top) > > + lmb_reserve_flags(ram_top, bank_end - ram_top, > > + LMB_NOOVERWRITE); > > Please guard this somehow that it will be enabled per each platform > that needs it, and/or give warning when conditions this specific code > path is a workaround for are reached that are different than before > the patch. Any similar platform-specific memory addressing bug/quirk > having previously avoided a trigger will silently continue to work > here instead of getting documented; we should at least uncover these > assumptions. Instead of putting some check here, and complicating the code, I would say that the board maintainer would know why the ram_top has not been put at the top of DRAM memory -- if possible, the ram_top should be the highest possible DRAM address. So this is the responsibility of the board maintainer instead. Ideally, for a 64 bit machine, like the one where you hit this issue, there is no reason for the board to not use the highest possible DRAM address as ram_top, unless there is some issue, like accessing memory address above a certain value. So, this would not be an issue if the board is using the highest possible value of memory as ram_top, especially when it comes to a single DRAM bank. > > In the theoretical situation where we are to just fix the visionfive2 > mmc platform specific issue in the dw_mmc driver and not need this > workaround code anymore, there's not visibility for us to easily > diagnose if a similar assumption or previously un-triggered platform > quirk exists elsewhere. There's no warning, and no easy on/off config > option to test with and without. The memory rework commit 22f2c9ed9f53 > is here a useful tool to break and bring attention to assumptions in > code and documentation. Fixing the mmc driver issue may or may not be possible. And if this is indeed a fixable issue, then, after fixing the mmc driver, the subsequent change should also remove this restriction on the ram_top value, which will automatically take care of this restriction. -sughosh > > > } > > } > > } > > CONFIG_EFI_LOADER_BOUNCE_BUFFER=y added to visionfive2 config in > 6c791b6646c1 does not make sense to me as when I tested it did not > avoid the platform issue. Restricting the memory allocations is an > effective workaround to the visionfive2 platform issue. I leave that > to the experts if a revert is appropriate given that we have an > effective workaround now. > > Thank you Sughosh! This is (meanwhile until more comments and review) > a functional workaround. > > Tested-by: E Shattow <lucent@gmail.com>
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!
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. Patrice
On Wed, 11 Dec 2024 at 21:50, Patrice CHOTARD <patrice.chotard@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 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. -sughosh > > Patrice
On 12/11/24 17:27, Sughosh Ganu wrote: > On Wed, 11 Dec 2024 at 21:50, Patrice CHOTARD > <patrice.chotard@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. > 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. Thanks Patrice > > -sughosh > >> >> Patrice
On Wed, 11 Dec 2024 at 22:20, Patrice CHOTARD <patrice.chotard@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@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 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. > > > > 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
diff --git a/lib/lmb.c b/lib/lmb.c index 14b9b8466ff..be168dfb8dc 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -615,6 +615,7 @@ static __maybe_unused void lmb_reserve_common_spl(void) void lmb_add_memory(void) { int i; + phys_addr_t bank_end; phys_size_t size; u64 ram_top = gd->ram_top; struct bd_info *bd = gd->bd; @@ -628,6 +629,8 @@ void lmb_add_memory(void) for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) { size = bd->bi_dram[i].size; + bank_end = bd->bi_dram[i].start + size; + if (size) { lmb_add(bd->bi_dram[i].start, size); @@ -639,6 +642,9 @@ void lmb_add_memory(void) if (bd->bi_dram[i].start >= ram_top) lmb_reserve_flags(bd->bi_dram[i].start, size, LMB_NOOVERWRITE); + else if (bank_end > ram_top) + lmb_reserve_flags(ram_top, bank_end - ram_top, + LMB_NOOVERWRITE); } } }
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. Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> --- lib/lmb.c | 6 ++++++ 1 file changed, 6 insertions(+)