| Message ID | 20170828165430.6139-1-xypron.glpk@gmx.de |
|---|---|
| State | Accepted |
| Commit | 51c533fdce75fbdec7de28a05a675a553d209bcb |
| Headers | show |
| Series | [U-Boot,1/1] efi_loader: bootefi hello should use loadaddr | expand |
On 28.08.17 18:54, Heinrich Schuchardt wrote: > Command 'bootefi hello' currently uses CONFIG_SYS_LOAD_ADDR > as loading address. > > qemu machines have by default 128 MiB RAM. > CONFIG_SYS_LOAD_ADDR for x86 is 0x20000000 (512 MiB). > This causes 'bootefi hello' to fail. > > We should use the environment variable loadaddr if available. > It defaults to 0x1000000 (16 MiB) on qemu_x86. > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > cmd/bootefi.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c > index 47771f87cc..a3768158a2 100644 > --- a/cmd/bootefi.c > +++ b/cmd/bootefi.c > @@ -300,7 +300,11 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > if (!strcmp(argv[1], "hello")) { > ulong size = __efi_hello_world_end - __efi_hello_world_begin; > > - addr = CONFIG_SYS_LOAD_ADDR; > + saddr = env_get("loadaddr"); > + if (saddr) > + addr = simple_strtoul(saddr, NULL, 16); > + else > + addr = CONFIG_SYS_LOAD_ADDR; I'm not terribly happy about that logic. Ideally I would want to have it load to *one* explicit address, not multiple ones. Maybe we could just always memalign() a region? Alternatively if we can not use memalign, I would rather drop the CONFIG_SYS_LOAD_ADDR branch and *only* rely on loadaddr. Alex
On 08/29/2017 01:30 AM, Alexander Graf wrote: > > > On 28.08.17 18:54, Heinrich Schuchardt wrote: >> Command 'bootefi hello' currently uses CONFIG_SYS_LOAD_ADDR >> as loading address. >> >> qemu machines have by default 128 MiB RAM. >> CONFIG_SYS_LOAD_ADDR for x86 is 0x20000000 (512 MiB). >> This causes 'bootefi hello' to fail. >> >> We should use the environment variable loadaddr if available. >> It defaults to 0x1000000 (16 MiB) on qemu_x86. >> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >> --- >> cmd/bootefi.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/cmd/bootefi.c b/cmd/bootefi.c >> index 47771f87cc..a3768158a2 100644 >> --- a/cmd/bootefi.c >> +++ b/cmd/bootefi.c >> @@ -300,7 +300,11 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, >> int argc, char * const argv[]) >> if (!strcmp(argv[1], "hello")) { >> ulong size = __efi_hello_world_end - __efi_hello_world_begin; >> - addr = CONFIG_SYS_LOAD_ADDR; >> + saddr = env_get("loadaddr"); >> + if (saddr) >> + addr = simple_strtoul(saddr, NULL, 16); >> + else >> + addr = CONFIG_SYS_LOAD_ADDR; > > I'm not terribly happy about that logic. Ideally I would want to have it > load to *one* explicit address, not multiple ones. > > Maybe we could just always memalign() a region? > > Alternatively if we can not use memalign, I would rather drop the > CONFIG_SYS_LOAD_ADDR branch and *only* rely on loadaddr. > > > Alex > A user might have deleted loadaddr from the environment. If we do not fallback to CONFIG_SYS_LOAD_ADDR we would have to create an error message saying that we cannot do without loadaddr. Shall I adjust the patch in this way? Best regards Heinrich
On 08/29/2017 07:56 AM, Heinrich Schuchardt wrote: > On 08/29/2017 01:30 AM, Alexander Graf wrote: >> >> On 28.08.17 18:54, Heinrich Schuchardt wrote: >>> Command 'bootefi hello' currently uses CONFIG_SYS_LOAD_ADDR >>> as loading address. >>> >>> qemu machines have by default 128 MiB RAM. >>> CONFIG_SYS_LOAD_ADDR for x86 is 0x20000000 (512 MiB). >>> This causes 'bootefi hello' to fail. >>> >>> We should use the environment variable loadaddr if available. >>> It defaults to 0x1000000 (16 MiB) on qemu_x86. >>> >>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >>> --- >>> cmd/bootefi.c | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c >>> index 47771f87cc..a3768158a2 100644 >>> --- a/cmd/bootefi.c >>> +++ b/cmd/bootefi.c >>> @@ -300,7 +300,11 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, >>> int argc, char * const argv[]) >>> if (!strcmp(argv[1], "hello")) { >>> ulong size = __efi_hello_world_end - __efi_hello_world_begin; >>> - addr = CONFIG_SYS_LOAD_ADDR; >>> + saddr = env_get("loadaddr"); >>> + if (saddr) >>> + addr = simple_strtoul(saddr, NULL, 16); >>> + else >>> + addr = CONFIG_SYS_LOAD_ADDR; >> I'm not terribly happy about that logic. Ideally I would want to have it >> load to *one* explicit address, not multiple ones. >> >> Maybe we could just always memalign() a region? >> >> Alternatively if we can not use memalign, I would rather drop the >> CONFIG_SYS_LOAD_ADDR branch and *only* rely on loadaddr. >> >> >> Alex >> > A user might have deleted loadaddr from the environment. > If we do not fallback to CONFIG_SYS_LOAD_ADDR we would have to create an > error message saying that we cannot do without loadaddr. > > Shall I adjust the patch in this way? No, let's do it the same way as booti / bootz. Just use the global load_addr variable. Alex
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 47771f87cc..a3768158a2 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -300,7 +300,11 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) if (!strcmp(argv[1], "hello")) { ulong size = __efi_hello_world_end - __efi_hello_world_begin; - addr = CONFIG_SYS_LOAD_ADDR; + saddr = env_get("loadaddr"); + if (saddr) + addr = simple_strtoul(saddr, NULL, 16); + else + addr = CONFIG_SYS_LOAD_ADDR; memcpy((char *)addr, __efi_hello_world_begin, size); } else #endif
Command 'bootefi hello' currently uses CONFIG_SYS_LOAD_ADDR as loading address. qemu machines have by default 128 MiB RAM. CONFIG_SYS_LOAD_ADDR for x86 is 0x20000000 (512 MiB). This causes 'bootefi hello' to fail. We should use the environment variable loadaddr if available. It defaults to 0x1000000 (16 MiB) on qemu_x86. Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> --- cmd/bootefi.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)