| Message ID | 20240724060224.3071065-34-sughosh.ganu@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Make LMB memory map global and persistent | expand |
Hi Sughosh, On Wed, 24 Jul 2024 at 00:05, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > Instead of a randomly selected address, use an LMB allocated one for > reading the file into memory. With the LMB map now being persistent > and global, the address used for reading the file might be already > allocated as non-overwritable, resulting in a failure. Get a valid > address from LMB and then read the file to that address. > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > --- > Changes since rfc: None > > test/boot/cedit.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > No, we should not start putting lmb into tests like this. Regards, Simon
On Fri, 26 Jul 2024 at 05:02, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Wed, 24 Jul 2024 at 00:05, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > Instead of a randomly selected address, use an LMB allocated one for > > reading the file into memory. With the LMB map now being persistent > > and global, the address used for reading the file might be already > > allocated as non-overwritable, resulting in a failure. Get a valid > > address from LMB and then read the file to that address. > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > --- > > Changes since rfc: None > > > > test/boot/cedit.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > No, we should not start putting lmb into tests like this. The test fails if the address is not allocated through an lmb api. Although I can check why. -sughosh > > Regards, > Simon
Hi Sughosh, On Mon, 29 Jul 2024 at 02:53, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > On Fri, 26 Jul 2024 at 05:02, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Sughosh, > > > > On Wed, 24 Jul 2024 at 00:05, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > Instead of a randomly selected address, use an LMB allocated one for > > > reading the file into memory. With the LMB map now being persistent > > > and global, the address used for reading the file might be already > > > allocated as non-overwritable, resulting in a failure. Get a valid > > > address from LMB and then read the file to that address. > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > --- > > > Changes since rfc: None > > > > > > test/boot/cedit.c | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > > No, we should not start putting lmb into tests like this. > > The test fails if the address is not allocated through an lmb api. > Although I can check why. Neither am I, but you did put the tcg in the same region so perhaps that is an issue? Sandbox is designed so that low memory regions can be used in tests. I pointed you to the memory-map docs. Regards, SImon
On Mon, 29 Jul 2024 at 20:58, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Mon, 29 Jul 2024 at 02:53, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > On Fri, 26 Jul 2024 at 05:02, Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi Sughosh, > > > > > > On Wed, 24 Jul 2024 at 00:05, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > Instead of a randomly selected address, use an LMB allocated one for > > > > reading the file into memory. With the LMB map now being persistent > > > > and global, the address used for reading the file might be already > > > > allocated as non-overwritable, resulting in a failure. Get a valid > > > > address from LMB and then read the file to that address. > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > --- > > > > Changes since rfc: None > > > > > > > > test/boot/cedit.c | 6 +++++- > > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > > > > > No, we should not start putting lmb into tests like this. > > > > The test fails if the address is not allocated through an lmb api. > > Although I can check why. > > Neither am I, but you did put the tcg in the same region so perhaps > that is an issue? I now remember the issue. The cedit_fdt() function uses a random address(0x1000) and tries to load the settings.dtb file to the address. The load command uses the lmb_alloc_addr() API function to reserve and use this address for loading the file. This worked earlier because of the local nature of the lmb memory map. But it does not work anymore, as this clashes with some existing reservation resulting in the test failure. So this change is very much needed. -sughosh > > Sandbox is designed so that low memory regions can be used in tests. I > pointed you to the memory-map docs. > > Regards, > SImon
Hi Sughosh, On Wed, 31 Jul 2024 at 01:26, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > On Mon, 29 Jul 2024 at 20:58, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Sughosh, > > > > On Mon, 29 Jul 2024 at 02:53, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > On Fri, 26 Jul 2024 at 05:02, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > Hi Sughosh, > > > > > > > > On Wed, 24 Jul 2024 at 00:05, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > Instead of a randomly selected address, use an LMB allocated one for > > > > > reading the file into memory. With the LMB map now being persistent > > > > > and global, the address used for reading the file might be already > > > > > allocated as non-overwritable, resulting in a failure. Get a valid > > > > > address from LMB and then read the file to that address. > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > > --- > > > > > Changes since rfc: None > > > > > > > > > > test/boot/cedit.c | 6 +++++- > > > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > > > > > > > > No, we should not start putting lmb into tests like this. > > > > > > The test fails if the address is not allocated through an lmb api. > > > Although I can check why. > > > > Neither am I, but you did put the tcg in the same region so perhaps > > that is an issue? > > I now remember the issue. The cedit_fdt() function uses a random > address(0x1000) and tries to load the settings.dtb file to the > address. The load command uses the lmb_alloc_addr() API function to > reserve and use this address for loading the file. This worked earlier > because of the local nature of the lmb memory map. But it does not > work anymore, as this clashes with some existing reservation resulting > in the test failure. So this change is very much needed. OK I see. Do you know what is clashing? I wonder if we should have strings associated with lmb records, to make this easier, if there are going to be so many? > > -sughosh > > > > > Sandbox is designed so that low memory regions can be used in tests. I > > pointed you to the memory-map docs. Regards, Simon
On Wed, 31 Jul 2024 at 20:08, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Wed, 31 Jul 2024 at 01:26, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > On Mon, 29 Jul 2024 at 20:58, Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi Sughosh, > > > > > > On Mon, 29 Jul 2024 at 02:53, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > On Fri, 26 Jul 2024 at 05:02, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > Hi Sughosh, > > > > > > > > > > On Wed, 24 Jul 2024 at 00:05, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > > > Instead of a randomly selected address, use an LMB allocated one for > > > > > > reading the file into memory. With the LMB map now being persistent > > > > > > and global, the address used for reading the file might be already > > > > > > allocated as non-overwritable, resulting in a failure. Get a valid > > > > > > address from LMB and then read the file to that address. > > > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > > > --- > > > > > > Changes since rfc: None > > > > > > > > > > > > test/boot/cedit.c | 6 +++++- > > > > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > > No, we should not start putting lmb into tests like this. > > > > > > > > The test fails if the address is not allocated through an lmb api. > > > > Although I can check why. > > > > > > Neither am I, but you did put the tcg in the same region so perhaps > > > that is an issue? > > > > I now remember the issue. The cedit_fdt() function uses a random > > address(0x1000) and tries to load the settings.dtb file to the > > address. The load command uses the lmb_alloc_addr() API function to > > reserve and use this address for loading the file. This worked earlier > > because of the local nature of the lmb memory map. But it does not > > work anymore, as this clashes with some existing reservation resulting > > in the test failure. So this change is very much needed. > > OK I see. Do you know what is clashing? I wonder if we should have > strings associated with lmb records, to make this easier, if there are > going to be so many? Regarding your point about making use of strings for lmb records, I have something similar on my todo list. I plan to use the strings for the lmb flags associated with a memory region. The current flag values shown as part of the bdinfo command are not very helpful from the ease of understanding pov. -sughosh > > > > > > -sughosh > > > > > > > > Sandbox is designed so that low memory regions can be used in tests. I > > > pointed you to the memory-map docs. > > Regards, > Simon
diff --git a/test/boot/cedit.c b/test/boot/cedit.c index fd19da0a0c..6078b7cc0f 100644 --- a/test/boot/cedit.c +++ b/test/boot/cedit.c @@ -7,6 +7,7 @@ #include <cedit.h> #include <env.h> #include <expo.h> +#include <lmb.h> #include <mapmem.h> #include <dm/ofnode.h> #include <test/ut.h> @@ -61,7 +62,7 @@ static int cedit_fdt(struct unit_test_state *uts) struct video_priv *vid_priv; extern struct expo *cur_exp; struct scene_obj_menu *menu; - ulong addr = 0x1000; + ulong addr; struct ofprop prop; struct scene *scn; oftree tree; @@ -86,6 +87,8 @@ static int cedit_fdt(struct unit_test_state *uts) str = abuf_data(&tline->buf); strcpy(str, "my-machine"); + addr = lmb_alloc(1024, 1024, LMB_NONE); + ut_asserteq(!!addr, !0); ut_assertok(run_command("cedit write_fdt hostfs - settings.dtb", 0)); ut_assertok(run_commandf("load hostfs - %lx settings.dtb", addr)); ut_assert_nextlinen("1024 bytes read"); @@ -94,6 +97,7 @@ static int cedit_fdt(struct unit_test_state *uts) tree = oftree_from_fdt(fdt); node = ofnode_find_subnode(oftree_root(tree), CEDIT_NODE_NAME); ut_assert(ofnode_valid(node)); + lmb_free(addr, 1024, LMB_NONE); ut_asserteq(ID_CPU_SPEED_2, ofnode_read_u32_default(node, "cpu-speed", 0));
Instead of a randomly selected address, use an LMB allocated one for reading the file into memory. With the LMB map now being persistent and global, the address used for reading the file might be already allocated as non-overwritable, resulting in a failure. Get a valid address from LMB and then read the file to that address. Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> --- Changes since rfc: None test/boot/cedit.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)