On Wed, 11 Sept 2024 at 16:53, Michal Simek <michal.si...@amd.com> wrote: > > > > On 9/11/24 13:20, Sughosh Ganu wrote: > > On Tue, 10 Sept 2024 at 21:14, Prasad Kummari <prasad.kumm...@amd.com> > > wrote: > >> > >> Added LMB API to prevent SF command from overwriting reserved > >> memory areas. The current SPI code does not use LMB APIs for > >> loading data into memory addresses. To resolve this, LMB APIs > >> were added to check the load address of an SF command and ensure it > >> does not overwrite reserved memory addresses. Similar checks are > >> used in TFTP, serial load, and boot code to prevent overwriting > >> reserved memory. > >> > >> Signed-off-by: Prasad Kummari <prasad.kumm...@amd.com> > >> --- > >> > >> Changes in V3: > >> - Removed lmb_init_and_reserve() as part of latest LMB series. > >> - Error message moved to one place. > >> - lmb_alloc_addr() is not required because the given memory address is > >> being checked to ensure it is free or not. > >> > >> Changes in V2: > >> - Rebased the code changes on top of the next branch. > >> > >> UT: > >> Tested on Versal NET board > >> > >> Versal NET> fdt print /reserved-memory > >> reserved-memory { > >> ranges; > >> #size-cells = <0x00000002>; > >> #address-cells = <0x00000002>; > >> tf-a { > >> reg = <0x00000000 0x70000000 0x00000000 0x00050000>; > >> no-map; > >> }; > >> }; > >> Versal NET> sf read 0x70000000 0x0 0x40 > >> device 0 offset 0x0, size 0x40 > >> ERROR: trying to overwrite reserved memory... > >> > >> Versal NET> sf write 0x70000000 0x0 0x40 > >> device 0 offset 0x0, size 0x40 > >> ERROR: trying to overwrite reserved memory... > >> > >> relocaddr = 0x000000007febc000 > >> > >> Versal NET> sf read 0x000000007febc000 0x0 0x40 > >> device 0 offset 0x0, size 0x40 > >> ERROR: trying to overwrite reserved memory... > >> > >> Versal NET> sf write 0x000000007febc000 0x0 0x40 > >> device 0 offset 0x0, size 0x40 > >> ERROR: trying to overwrite reserved memory... > >> > >> cmd/sf.c | 36 +++++++++++++++++++++++++++++++++++- > >> 1 file changed, 35 insertions(+), 1 deletion(-) > >> > >> diff --git a/cmd/sf.c b/cmd/sf.c > >> index f43a2e08b3..7bb8bcfce2 100644 > >> --- a/cmd/sf.c > >> +++ b/cmd/sf.c > >> @@ -10,6 +10,7 @@ > >> #include <div64.h> > >> #include <dm.h> > >> #include <log.h> > >> +#include <lmb.h> > >> #include <malloc.h> > >> #include <mapmem.h> > >> #include <spi.h> > >> @@ -272,6 +273,31 @@ static int spi_flash_update(struct spi_flash *flash, > >> u32 offset, > >> return 0; > >> } > >> > >> +#ifdef CONFIG_LMB > >> +static int do_spi_read_lmb_check(ulong start_addr, loff_t len) > >> +{ > >> + phys_size_t max_size; > >> + ulong end_addr; > >> + > >> + lmb_dump_all(); > >> + > >> + max_size = lmb_get_free_size(start_addr); > >> + if (!max_size) { > >> + return CMD_RET_FAILURE; > >> + } > >> + > >> + end_addr = start_addr + max_size; > >> + if (!end_addr) > >> + end_addr = ULONG_MAX; > >> + > >> + if ((start_addr + len) > end_addr) { > >> + return CMD_RET_FAILURE; > >> + } > > > > This is one way to get the load address, yes. But please do note that > > with this method, if the region of memory has already been marked as > > reserved, the call to lmb_get_free_size() will fail, i.e. return a > > value of 0. Whereas if you call lmb_alloc_addr(), and if the memory > > region is marked reserved with the flags set to LMB_NONE, that call > > will not fail, as we can re-reserve memory marked as LMB_NONE. Hence a > > better approach would be to use the logic used in the fs module > > function that I had pointed to in my earlier email. > > > > Also, like I mentioned earlier, it will be better to refactor the > > functionality, especially if the plan is to introduce this check for > > loading stuff through other interfaces like nand, mmc etc. You can add > > a function in the lmb.c like lmb_read_check() which does that. > > Is this something what you are going to work on?
Can that not be done as part of this work? It is about having a single function in the lmb.c file, which simply does a check for lmb_alloc_addr(). It's not very complicated tbh. -sughosh