On Mon, 16 Sept 2024 at 19:47, Vaishnav Achath <vaishna...@ti.com> wrote:
>
> Hi Sughosh,
>
> On 16/09/24 16:40, Sughosh Ganu wrote:
> > 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
> >
>
> I tested with your patches on our failing platforms and it is working,
> Thanks, In the logs, initially a mmc load of remoteproc firmware happens
> and then Kernel/DTB load through TFTP which was failing, now it is fixed
> with your patches:
Thanks much for testing the patches! I have put the patches through
CI. Will post them on the list once the CI completes.
-sughosh
>
> https://gist.github.com/vachath/bb7c8c7b5a38a58298026b831db58424
>
> Thanks and Regards,
> Vaishnav
>
> >>
> >>>>
> >>>>>>
> >>>>>>
> >>>>>>> + 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 */
