On Mon, 16 Sept 2024 at 16:07, Vaishnav Achath <vaishna...@ti.com> wrote: > > Hi Sughosh, > > On 16/09/24 14:53, Sughosh Ganu wrote: > > On Mon, 16 Sept 2024 at 14:22, Vaishnav Achath <vaishna...@ti.com> wrote: > >> > >> Hi Sughosh, > >> > >> On 16/09/24 12:13, Sughosh Ganu wrote: > >>> On Mon, 16 Sept 2024 at 11:47, Vaishnav Achath <vaishna...@ti.com> wrote: > >>>> > >>>> Hi Prasad, > >>>> > >>>> On 13/09/24 13:02, Prasad Kummari 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 V4: > >>>>> - Removed do_spi_read_lmb_check(). > >>>>> - Added the lmb_read_check() function in lmb.c, making it reusable for > >>>>> NAND, MMC, etc. > >>>>> - Addressed review comments. > >>>>> > >>>>> 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. > >>>>> > >>>>> 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... > >>>>> > >>>>> 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... > >>>>> Versal NET> sf erase 0x0 0x1000000;mw.b 0x8000 aabbccdd 0x1000000;sf > >>>>> write 0x8000 0x0 0x1000000;mw.b 0x8008000 0x0 0x1000000;sf read > >>>>> 0x8008000 0x0 0x1000000;cmp.b 0x8000 0x8008000 0x01000000 > >>>>> SF: 16777216 bytes @ 0x0 Erased: OK > >>>>> device 0 offset 0x0, size 0x1000000 > >>>>> SF: 16777216 bytes @ 0x0 Written: OK > >>>>> device 0 offset 0x0, size 0x1000000 > >>>>> SF: 16777216 bytes @ 0x0 Read: OK > >>>>> Total of 16777216 byte(s) were the same > >>>>> > >>>>> cmd/sf.c | 8 ++++++++ > >>>>> include/lmb.h | 5 +++++ > >>>>> 2 files changed, 13 insertions(+) > >>>>> > >>>>> diff --git a/cmd/sf.c b/cmd/sf.c > >>>>> index f43a2e08b3..08e364e191 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> > >>>>> @@ -317,6 +318,13 @@ static int do_spi_flash_read_write(int argc, char > >>>>> *const argv[]) > >>>>> strncmp(argv[0], "write", 5) == 0) { > >>>>> int read; > >>>>> > >>>>> + if (CONFIG_IS_ENABLED(LMB)) { > >>>>> + if (lmb_read_check(addr, len)) { > >>>> > >>>> Even though the function is named lmb_read_check(), it performs an alloc > >>>> which is never freed, thus it makes it difficult for other callers to > >>>> use the same region for other purposes (some callers use > >>>> lmb_get_free_size() ), as mentioned in the commit message the check is > >>>> only to prevent sf from overwriting reserved region, but it looks like > >>>> this patch makes the load region also as reserved, is this necessary? > >>> > >>> Like I mentioned in my other reply, using a check for lmb_alloc_addr() > >>> allows for memory re-use, which is the behaviour that a large number > >>> of use cases rely on -- if you go through the test scripts, it is > >>> assumed that memory re-use is allowed. That there is no need to > >>> explicitly free up memory, and that has been how the LMB memory has > >>> been used historically. So it is allowed to use some address to load > >>> an image to that address, and then use the same address to load a > >>> different image. The LMB rework series does keep this behaviour > >>> consistent. So it would be better to change the behaviour of the tftp > >>> command to use the same API. I was planning on working on this > >>> cleanup. If you want, you can take it up. > >>> > >> > >> Please let me know if you are planning to work on this in the coming few > >> days, otherwise I can pick it up as we have platforms failing due to this. > > > > I can take this up. Will keep you on Cc so that you can test the > > patches on your boards. > > > > Thanks, I will test and report the results once you post the patches.
I have pushed a couple of patches to my github branch [1]. Can you please try these on your platforms and check if this fixes the issues that you see. I will also set up a board with network access on my end and try it out. Thanks. -sughosh [1] - https://github.com/sughoshg/u-boot/tree/tftp_wget_lmb_changes > > >> > >>>> > >>>> > >>>>> + printf("ERROR: trying to overwrite > >>>>> reserved memory...\n"); > >>>>> + return CMD_RET_FAILURE; > >>>>> + } > >>>>> + } > >>>>> + > >>>>> read = strncmp(argv[0], "read", 4) == 0; > >>>>> if (read) > >>>>> ret = spi_flash_read(flash, offset, len, buf); > >>>>> diff --git a/include/lmb.h b/include/lmb.h > >>>>> index fc2daaa7bf..aee2f9fcda 100644 > >>>>> --- a/include/lmb.h > >>>>> +++ b/include/lmb.h > >>>>> @@ -111,6 +111,11 @@ struct lmb *lmb_get(void); > >>>>> int lmb_push(struct lmb *store); > >>>>> void lmb_pop(struct lmb *store); > >>>>> > >>>>> +static inline int lmb_read_check(phys_addr_t addr, phys_size_t len) > >>>>> +{ > >>>>> + return lmb_alloc_addr(addr, len) == addr ? 0 : -1; > >>>> > >>>> who frees this? can we free this right after checking? > >>> > >>> It is not required to explicitly free up this memory, as it is not an > >>> actual allocation per-se. Why were these functions called alloc > >>> something, I am not sure. But the point is, if you change the tftp > >>> command code to use this API instead, and then use it after a previous > >>> load, it would not fail. > >>> > >>> -sughosh > >>> > >> > >> Agreed, but the commit message says to "ensure it does not overwrite > >> reserved memory addresses" but the implementation marks the region as > >> reserved in the global memory map and is visible in lmb_dump_all() , > >> which is not expected, is there really a need to mark the region as > >> reserved. > > > > What can be overwritten, and what cannot be can now be determined from > > the flags. If you check the LMB memory map from the bdinfo command, > > you will see that the regions which cannot be overwritten are now > > being marked with the "no-overwrite" flag. The other LMB memory which > > can be re-used to load multiple different images is being marked with > > the "none" flag. One issue with all this is that currently there is no > > document which explains all these concepts. I will work on adding such > > a document. Thanks. > > > > Yes, documenting the behavior will clear the confusion. > > Thanks and Regards, > Vaishnav > > > -sughosh > > > >> > >> Thanks and Regards, > >> Vaishnav > >> > >>>> > >>>> Thanks and Regards, > >>>> Vaishnav > >>>> > >>>>> +} > >>>>> + > >>>>> #endif /* __KERNEL__ */ > >>>>> > >>>>> #endif /* _LINUX_LMB_H */