| Message ID | 20241022120032.196311-6-sjg@chromium.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | efi: Add a test for EFI bootmeth | expand |
Hi Simon, On Tue, 22 Oct 2024 at 15:00, Simon Glass <sjg@chromium.org> wrote: > > When the --native flag is given, pretend to be running the host > architecture rather than sandbox. > > Add an 'efidebug filename' command to report it. Heinrich does this allow you to continue your testing in native archs? > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > Changes in v8: > - Add new patch to report host default-filename in native mode > > boot/bootmeth_efi.c | 25 ++------------------ > boot/efi_fname.c | 57 +++++++++++++++++++++++++++++++++------------ > cmd/efidebug.c | 25 ++++++++++++++++++++ > include/efi.h | 25 ++++++++++++++++++++ > 4 files changed, 94 insertions(+), 38 deletions(-) > > diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c > index 371b36d550b..f836aa655f5 100644 > --- a/boot/bootmeth_efi.c > +++ b/boot/bootmeth_efi.c > @@ -25,32 +25,11 @@ > > #define EFI_DIRNAME "/EFI/BOOT/" > > -static int get_efi_pxe_arch(void) > -{ > - /* http://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xml */ > - if (IS_ENABLED(CONFIG_ARM64)) > - return 0xb; > - else if (IS_ENABLED(CONFIG_ARM)) > - return 0xa; > - else if (IS_ENABLED(CONFIG_X86_64)) > - return 0x6; > - else if (IS_ENABLED(CONFIG_X86)) > - return 0x7; > - else if (IS_ENABLED(CONFIG_ARCH_RV32I)) > - return 0x19; > - else if (IS_ENABLED(CONFIG_ARCH_RV64I)) > - return 0x1b; > - else if (IS_ENABLED(CONFIG_SANDBOX)) > - return 0; /* not used */ > - > - return -EINVAL; > -} > - > static int get_efi_pxe_vci(char *str, int max_len) > { > int ret; > > - ret = get_efi_pxe_arch(); > + ret = efi_get_pxe_arch(); > if (ret < 0) > return ret; > > @@ -239,7 +218,7 @@ static int distro_efi_read_bootflow_net(struct bootflow *bflow) > ret = get_efi_pxe_vci(str, sizeof(str)); > if (ret) > return log_msg_ret("vci", ret); > - ret = get_efi_pxe_arch(); > + ret = efi_get_pxe_arch(); > if (ret < 0) > return log_msg_ret("arc", ret); > arch = ret; > diff --git a/boot/efi_fname.c b/boot/efi_fname.c > index a6b11383bba..790f9e2fa36 100644 > --- a/boot/efi_fname.c > +++ b/boot/efi_fname.c > @@ -9,29 +9,34 @@ > */ > > #include <efi.h> > +#include <errno.h> > #include <host_arch.h> > > -#ifdef CONFIG_SANDBOX > - > #if HOST_ARCH == HOST_ARCH_X86_64 > -#define BOOTEFI_NAME "BOOTX64.EFI" > +#define HOST_BOOTEFI_NAME "BOOTX64.EFI" > +#define HOST_PXE_ARCH 0x6 > #elif HOST_ARCH == HOST_ARCH_X86 > -#define BOOTEFI_NAME "BOOTIA32.EFI" > +#define HOST_BOOTEFI_NAME "BOOTIA32.EFI" > +#define HOST_PXE_ARCH 0x7 > #elif HOST_ARCH == HOST_ARCH_AARCH64 > -#define BOOTEFI_NAME "BOOTAA64.EFI" > +#define HOST_BOOTEFI_NAME "BOOTAA64.EFI" > +#define HOST_PXE_ARCH 0xb > #elif HOST_ARCH == HOST_ARCH_ARM > -#define BOOTEFI_NAME "BOOTARM.EFI" > +#define HOST_BOOTEFI_NAME "BOOTARM.EFI" > +#define HOST_PXE_ARCH 0xa > #elif HOST_ARCH == HOST_ARCH_RISCV32 > -#define BOOTEFI_NAME "BOOTRISCV32.EFI" > +#define HOST_BOOTEFI_NAME "BOOTRISCV32.EFI" > +#define HOST_PXE_ARCH 0x19 > #elif HOST_ARCH == HOST_ARCH_RISCV64 > -#define BOOTEFI_NAME "BOOTRISCV64.EFI" > +#define HOST_BOOTEFI_NAME "BOOTRISCV64.EFI" > +#define HOST_PXE_ARCH 0x1b > #else > -#error Unsupported UEFI architecture > +#error Unsupported Host architecture > #endif > > -#else > - > -#if defined(CONFIG_ARM64) > +#if defined(CONFIG_SANDBOX) > +#define BOOTEFI_NAME "BOOTSBOX.EFI" > +#elif defined(CONFIG_ARM64) > #define BOOTEFI_NAME "BOOTAA64.EFI" > #elif defined(CONFIG_ARM) > #define BOOTEFI_NAME "BOOTARM.EFI" > @@ -47,9 +52,31 @@ > #error Unsupported UEFI architecture > #endif > > -#endif > - > const char *efi_get_basename(void) > { > - return BOOTEFI_NAME; > + return efi_use_host_arch() ? HOST_BOOTEFI_NAME : BOOTEFI_NAME; > +} > + > +int efi_get_pxe_arch(void) > +{ > + if (efi_use_host_arch()) > + return HOST_PXE_ARCH; > + > + /* http://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xml */ > + if (IS_ENABLED(CONFIG_ARM64)) > + return 0xb; > + else if (IS_ENABLED(CONFIG_ARM)) > + return 0xa; > + else if (IS_ENABLED(CONFIG_X86_64)) > + return 0x6; > + else if (IS_ENABLED(CONFIG_X86)) > + return 0x7; > + else if (IS_ENABLED(CONFIG_ARCH_RV32I)) > + return 0x19; > + else if (IS_ENABLED(CONFIG_ARCH_RV64I)) > + return 0x1b; > + else if (IS_ENABLED(CONFIG_SANDBOX)) > + return 0; /* not used */ > + > + return -EINVAL; > } > diff --git a/cmd/efidebug.c b/cmd/efidebug.c > index e040fe75fa1..02f1e080e88 100644 > --- a/cmd/efidebug.c > +++ b/cmd/efidebug.c > @@ -511,6 +511,27 @@ static int do_efi_show_images(struct cmd_tbl *cmdtp, int flag, > return CMD_RET_SUCCESS; > } > > +/** > + * do_efi_show_defaults() - show UEFI default filename and PXE architecture > + * > + * @cmdtp: Command table > + * @flag: Command flag > + * @argc: Number of arguments > + * @argv: Argument array > + * Return: CMD_RET_SUCCESS on success, CMD_RET_RET_FAILURE on failure > + * > + * Implement efidebug "defaults" sub-command. > + * Shows the default EFI filename and PXE architecture > + */ > +static int do_efi_show_defaults(struct cmd_tbl *cmdtp, int flag, > + int argc, char *const argv[]) > +{ > + printf("Default boot path: EFI\\BOOT\\%s\n", efi_get_basename()); > + printf("PXE arch: 0x%02x\n", efi_get_pxe_arch()); > + > + return CMD_RET_SUCCESS; > +} > + > static const char * const efi_mem_type_string[] = { > [EFI_RESERVED_MEMORY_TYPE] = "RESERVED", > [EFI_LOADER_CODE] = "LOADER CODE", > @@ -1561,6 +1582,8 @@ static struct cmd_tbl cmd_efidebug_sub[] = { > "", ""), > U_BOOT_CMD_MKENT(dh, CONFIG_SYS_MAXARGS, 1, do_efi_show_handles, > "", ""), > + U_BOOT_CMD_MKENT(defaults, CONFIG_SYS_MAXARGS, 1, do_efi_show_defaults, > + "", ""), > U_BOOT_CMD_MKENT(images, CONFIG_SYS_MAXARGS, 1, do_efi_show_images, > "", ""), > U_BOOT_CMD_MKENT(memmap, CONFIG_SYS_MAXARGS, 1, do_efi_show_memmap, > @@ -1653,6 +1676,8 @@ U_BOOT_LONGHELP(efidebug, > " - show UEFI drivers\n" > "efidebug dh\n" > " - show UEFI handles\n" > + "efidebug defaults\n" > + " - show default EFI filename and PXE architecture\n" > "efidebug images\n" > " - show loaded images\n" > "efidebug memmap\n" > diff --git a/include/efi.h b/include/efi.h > index 1b8093bd4d3..70bb47e742f 100644 > --- a/include/efi.h > +++ b/include/efi.h > @@ -678,4 +678,29 @@ void efi_show_tables(struct efi_system_table *systab); > */ > const char *efi_get_basename(void); > > +#ifdef CONFIG_SANDBOX > +#include <asm/state.h> > +#endif > + > +static inline bool efi_use_host_arch(void) > +{ > +#ifdef CONFIG_SANDBOX #if IS_ENABLED(CONFIG_SANDBOX) is preferred no ? > + struct sandbox_state *state = state_get_current(); > + > + return state->native; > +#else > + return false; > +#endif > +} > + > +/** > + * efi_get_pxe_arch() - Get the architecture value for PXE > + * > + * See: > + * http://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xml > + * > + * Return: Architecture value > + */ > +int efi_get_pxe_arch(void); > + > #endif /* _LINUX_EFI_H */ > -- > 2.43.0 > Thanks /Ilias
Hi Ilias, Heinrich, On Fri, 25 Oct 2024 at 11:57, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Simon, > > > On Tue, 22 Oct 2024 at 15:00, Simon Glass <sjg@chromium.org> wrote: > > > > When the --native flag is given, pretend to be running the host > > architecture rather than sandbox. > > > > Add an 'efidebug filename' command to report it. > > Heinrich does this allow you to continue your testing in native archs? Yes, Heinrich seems OK with it. Separately from this patch, at present something is broken / missing on x86_64 (with recent QEMU / Tianocore), so I cannot run the tests with qemu-x86_64 today. It was apparently running OK before the last EDK II and QEMU updates. Also sandbox only works with the EFI Self-Certification Test (SCT) when natively compiled on aarch64 or riscv64[1][2]. There might be something missing on sandbox x86_64, which gives a segfault when running InstallX64.efi I am running these: ii ovmf 2024.02-2 all UEFI firmware for 64-bit x86 virtual machines ii qemu-system-x86 1:8.2.2+ds-0ubuntu1.2 amd64 QEMU full system emulation binaries (x86) So far I have not tried going back to older versions. > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > --- > > > > Changes in v8: > > - Add new patch to report host default-filename in native mode > > > > boot/bootmeth_efi.c | 25 ++------------------ > > boot/efi_fname.c | 57 +++++++++++++++++++++++++++++++++------------ > > cmd/efidebug.c | 25 ++++++++++++++++++++ > > include/efi.h | 25 ++++++++++++++++++++ > > 4 files changed, 94 insertions(+), 38 deletions(-) > > [..] > > diff --git a/include/efi.h b/include/efi.h > > index 1b8093bd4d3..70bb47e742f 100644 > > --- a/include/efi.h > > +++ b/include/efi.h > > @@ -678,4 +678,29 @@ void efi_show_tables(struct efi_system_table *systab); > > */ > > const char *efi_get_basename(void); > > > > +#ifdef CONFIG_SANDBOX > > +#include <asm/state.h> > > +#endif > > + > > +static inline bool efi_use_host_arch(void) > > +{ > > +#ifdef CONFIG_SANDBOX > > #if IS_ENABLED(CONFIG_SANDBOX) is preferred no ? That is more for use with if(), which I cannot use here. > > > + struct sandbox_state *state = state_get_current(); > > + > > + return state->native; > > +#else > > + return false; > > +#endif > > +} > > + [..] Regards, Simon [1] https://github.com/U-Boot-EFI/u-boot-sct-results [2] https://github.com/xypron/sct_release_test/tree/main
On 10/27/24 18:16, Simon Glass wrote: > Hi Ilias, Heinrich, > > On Fri, 25 Oct 2024 at 11:57, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: >> >> Hi Simon, >> >> >> On Tue, 22 Oct 2024 at 15:00, Simon Glass <sjg@chromium.org> wrote: >>> >>> When the --native flag is given, pretend to be running the host >>> architecture rather than sandbox. >>> >>> Add an 'efidebug filename' command to report it. >> >> Heinrich does this allow you to continue your testing in native archs? > > Yes, Heinrich seems OK with it. I highlighted to Simon, that it is important to me that the sandbox continues to support booting standard compliant images via shim, GRUB, just into the Linux EFI stub. Introducing a sandbox specific EFI binary name or PXE value provides no value. But at least the current suggestion does not do harm except forcing sandbox users to remember another command line parameter. Tests on QEMU or real hardware should use the architecture specific filename for the EFI binary and this is not what the test provided by patch 7/8 does. Best regards Heinrich > > Separately from this patch, at present something is broken / missing > on x86_64 (with recent QEMU / Tianocore), so I cannot run the tests > with qemu-x86_64 today. It was apparently running OK before the last > EDK II and QEMU updates. > > Also sandbox only works with the EFI Self-Certification Test (SCT) > when natively compiled on aarch64 or riscv64[1][2]. There might be > something missing on sandbox x86_64, which gives a segfault when > running InstallX64.efi > > I am running these: > ii ovmf 2024.02-2 all UEFI firmware for 64-bit > x86 virtual machines > ii qemu-system-x86 1:8.2.2+ds-0ubuntu1.2 amd64 QEMU full > system emulation binaries (x86) > > So far I have not tried going back to older versions. > >> >>> >>> Signed-off-by: Simon Glass <sjg@chromium.org> >>> --- >>> >>> Changes in v8: >>> - Add new patch to report host default-filename in native mode >>> >>> boot/bootmeth_efi.c | 25 ++------------------ >>> boot/efi_fname.c | 57 +++++++++++++++++++++++++++++++++------------ >>> cmd/efidebug.c | 25 ++++++++++++++++++++ >>> include/efi.h | 25 ++++++++++++++++++++ >>> 4 files changed, 94 insertions(+), 38 deletions(-) >>> > > [..] > >>> diff --git a/include/efi.h b/include/efi.h >>> index 1b8093bd4d3..70bb47e742f 100644 >>> --- a/include/efi.h >>> +++ b/include/efi.h >>> @@ -678,4 +678,29 @@ void efi_show_tables(struct efi_system_table *systab); >>> */ >>> const char *efi_get_basename(void); >>> >>> +#ifdef CONFIG_SANDBOX >>> +#include <asm/state.h> >>> +#endif >>> + >>> +static inline bool efi_use_host_arch(void) >>> +{ >>> +#ifdef CONFIG_SANDBOX >> >> #if IS_ENABLED(CONFIG_SANDBOX) is preferred no ? > > That is more for use with if(), which I cannot use here. > >> >>> + struct sandbox_state *state = state_get_current(); >>> + >>> + return state->native; >>> +#else >>> + return false; >>> +#endif >>> +} >>> + > [..] > > Regards, > Simon > > [1] https://github.com/U-Boot-EFI/u-boot-sct-results > [2] https://github.com/xypron/sct_release_test/tree/main
On 10/25/24 11:56, Ilias Apalodimas wrote: > Hi Simon, > > > On Tue, 22 Oct 2024 at 15:00, Simon Glass <sjg@chromium.org> wrote: >> >> When the --native flag is given, pretend to be running the host >> architecture rather than sandbox. >> >> Add an 'efidebug filename' command to report it. > > Heinrich does this allow you to continue your testing in native archs? > >> >> Signed-off-by: Simon Glass <sjg@chromium.org> >> --- >> >> Changes in v8: >> - Add new patch to report host default-filename in native mode >> >> boot/bootmeth_efi.c | 25 ++------------------ >> boot/efi_fname.c | 57 +++++++++++++++++++++++++++++++++------------ >> cmd/efidebug.c | 25 ++++++++++++++++++++ >> include/efi.h | 25 ++++++++++++++++++++ >> 4 files changed, 94 insertions(+), 38 deletions(-) >> >> diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c >> index 371b36d550b..f836aa655f5 100644 >> --- a/boot/bootmeth_efi.c >> +++ b/boot/bootmeth_efi.c >> @@ -25,32 +25,11 @@ >> >> #define EFI_DIRNAME "/EFI/BOOT/" >> >> -static int get_efi_pxe_arch(void) >> -{ >> - /* http://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xml */ >> - if (IS_ENABLED(CONFIG_ARM64)) >> - return 0xb; >> - else if (IS_ENABLED(CONFIG_ARM)) >> - return 0xa; >> - else if (IS_ENABLED(CONFIG_X86_64)) >> - return 0x6; >> - else if (IS_ENABLED(CONFIG_X86)) >> - return 0x7; >> - else if (IS_ENABLED(CONFIG_ARCH_RV32I)) >> - return 0x19; >> - else if (IS_ENABLED(CONFIG_ARCH_RV64I)) >> - return 0x1b; >> - else if (IS_ENABLED(CONFIG_SANDBOX)) >> - return 0; /* not used */ >> - >> - return -EINVAL; >> -} >> - >> static int get_efi_pxe_vci(char *str, int max_len) >> { >> int ret; >> >> - ret = get_efi_pxe_arch(); >> + ret = efi_get_pxe_arch(); >> if (ret < 0) >> return ret; >> >> @@ -239,7 +218,7 @@ static int distro_efi_read_bootflow_net(struct bootflow *bflow) >> ret = get_efi_pxe_vci(str, sizeof(str)); >> if (ret) >> return log_msg_ret("vci", ret); >> - ret = get_efi_pxe_arch(); >> + ret = efi_get_pxe_arch(); >> if (ret < 0) >> return log_msg_ret("arc", ret); >> arch = ret; >> diff --git a/boot/efi_fname.c b/boot/efi_fname.c >> index a6b11383bba..790f9e2fa36 100644 >> --- a/boot/efi_fname.c >> +++ b/boot/efi_fname.c >> @@ -9,29 +9,34 @@ >> */ >> >> #include <efi.h> >> +#include <errno.h> >> #include <host_arch.h> >> >> -#ifdef CONFIG_SANDBOX >> - >> #if HOST_ARCH == HOST_ARCH_X86_64 >> -#define BOOTEFI_NAME "BOOTX64.EFI" >> +#define HOST_BOOTEFI_NAME "BOOTX64.EFI" >> +#define HOST_PXE_ARCH 0x6 >> #elif HOST_ARCH == HOST_ARCH_X86 >> -#define BOOTEFI_NAME "BOOTIA32.EFI" >> +#define HOST_BOOTEFI_NAME "BOOTIA32.EFI" >> +#define HOST_PXE_ARCH 0x7 >> #elif HOST_ARCH == HOST_ARCH_AARCH64 >> -#define BOOTEFI_NAME "BOOTAA64.EFI" >> +#define HOST_BOOTEFI_NAME "BOOTAA64.EFI" >> +#define HOST_PXE_ARCH 0xb >> #elif HOST_ARCH == HOST_ARCH_ARM >> -#define BOOTEFI_NAME "BOOTARM.EFI" >> +#define HOST_BOOTEFI_NAME "BOOTARM.EFI" >> +#define HOST_PXE_ARCH 0xa >> #elif HOST_ARCH == HOST_ARCH_RISCV32 >> -#define BOOTEFI_NAME "BOOTRISCV32.EFI" >> +#define HOST_BOOTEFI_NAME "BOOTRISCV32.EFI" >> +#define HOST_PXE_ARCH 0x19 >> #elif HOST_ARCH == HOST_ARCH_RISCV64 >> -#define BOOTEFI_NAME "BOOTRISCV64.EFI" >> +#define HOST_BOOTEFI_NAME "BOOTRISCV64.EFI" >> +#define HOST_PXE_ARCH 0x1b >> #else >> -#error Unsupported UEFI architecture >> +#error Unsupported Host architecture >> #endif >> >> -#else >> - >> -#if defined(CONFIG_ARM64) >> +#if defined(CONFIG_SANDBOX) >> +#define BOOTEFI_NAME "BOOTSBOX.EFI" >> +#elif defined(CONFIG_ARM64) >> #define BOOTEFI_NAME "BOOTAA64.EFI" >> #elif defined(CONFIG_ARM) >> #define BOOTEFI_NAME "BOOTARM.EFI" >> @@ -47,9 +52,31 @@ >> #error Unsupported UEFI architecture >> #endif>> >> -#endif >> - >> const char *efi_get_basename(void) >> { >> - return BOOTEFI_NAME; >> + return efi_use_host_arch() ? HOST_BOOTEFI_NAME : BOOTEFI_NAME; >> +} >> + >> +int efi_get_pxe_arch(void) >> +{ >> + if (efi_use_host_arch()) >> + return HOST_PXE_ARCH; >> + >> + /* http://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xml */ >> + if (IS_ENABLED(CONFIG_ARM64)) >> + return 0xb; >> + else if (IS_ENABLED(CONFIG_ARM)) >> + return 0xa; >> + else if (IS_ENABLED(CONFIG_X86_64)) >> + return 0x6; >> + else if (IS_ENABLED(CONFIG_X86)) >> + return 0x7; >> + else if (IS_ENABLED(CONFIG_ARCH_RV32I)) >> + return 0x19; >> + else if (IS_ENABLED(CONFIG_ARCH_RV64I)) >> + return 0x1b; >> + else if (IS_ENABLED(CONFIG_SANDBOX)) Please, add a warning here: log_warn("You are using the sandbox in non-compliant mode\n"); The riscv64 sandbox can only run riscv64 binaries The arm64 sandbox can only run arm64 binary. We cannot expect a DHCP server to do the expected with the value 0. So why should we introduce this? >> + return 0; /* not used */ The following comment would fit better: /* This breaks PXE booting on the sandbox.*/ Best regards Heinrich >> + >> + return -EINVAL; >> } >> diff --git a/cmd/efidebug.c b/cmd/efidebug.c >> index e040fe75fa1..02f1e080e88 100644 >> --- a/cmd/efidebug.c >> +++ b/cmd/efidebug.c >> @@ -511,6 +511,27 @@ static int do_efi_show_images(struct cmd_tbl *cmdtp, int flag, >> return CMD_RET_SUCCESS; >> } >> >> +/** >> + * do_efi_show_defaults() - show UEFI default filename and PXE architecture >> + * >> + * @cmdtp: Command table >> + * @flag: Command flag >> + * @argc: Number of arguments >> + * @argv: Argument array >> + * Return: CMD_RET_SUCCESS on success, CMD_RET_RET_FAILURE on failure >> + * >> + * Implement efidebug "defaults" sub-command. >> + * Shows the default EFI filename and PXE architecture >> + */ >> +static int do_efi_show_defaults(struct cmd_tbl *cmdtp, int flag, >> + int argc, char *const argv[]) >> +{ >> + printf("Default boot path: EFI\\BOOT\\%s\n", efi_get_basename()); >> + printf("PXE arch: 0x%02x\n", efi_get_pxe_arch()); >> + >> + return CMD_RET_SUCCESS; >> +} >> + >> static const char * const efi_mem_type_string[] = { >> [EFI_RESERVED_MEMORY_TYPE] = "RESERVED", >> [EFI_LOADER_CODE] = "LOADER CODE", >> @@ -1561,6 +1582,8 @@ static struct cmd_tbl cmd_efidebug_sub[] = { >> "", ""), >> U_BOOT_CMD_MKENT(dh, CONFIG_SYS_MAXARGS, 1, do_efi_show_handles, >> "", ""), >> + U_BOOT_CMD_MKENT(defaults, CONFIG_SYS_MAXARGS, 1, do_efi_show_defaults, >> + "", ""), >> U_BOOT_CMD_MKENT(images, CONFIG_SYS_MAXARGS, 1, do_efi_show_images, >> "", ""), >> U_BOOT_CMD_MKENT(memmap, CONFIG_SYS_MAXARGS, 1, do_efi_show_memmap, >> @@ -1653,6 +1676,8 @@ U_BOOT_LONGHELP(efidebug, >> " - show UEFI drivers\n" >> "efidebug dh\n" >> " - show UEFI handles\n" >> + "efidebug defaults\n" >> + " - show default EFI filename and PXE architecture\n" >> "efidebug images\n" >> " - show loaded images\n" >> "efidebug memmap\n" >> diff --git a/include/efi.h b/include/efi.h >> index 1b8093bd4d3..70bb47e742f 100644 >> --- a/include/efi.h >> +++ b/include/efi.h >> @@ -678,4 +678,29 @@ void efi_show_tables(struct efi_system_table *systab); >> */ >> const char *efi_get_basename(void); >> >> +#ifdef CONFIG_SANDBOX >> +#include <asm/state.h> >> +#endif >> + >> +static inline bool efi_use_host_arch(void) >> +{ >> +#ifdef CONFIG_SANDBOX > > #if IS_ENABLED(CONFIG_SANDBOX) is preferred no ? > >> + struct sandbox_state *state = state_get_current(); >> + >> + return state->native; >> +#else >> + return false; >> +#endif >> +} >> + >> +/** >> + * efi_get_pxe_arch() - Get the architecture value for PXE >> + * >> + * See: >> + * http://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xml >> + * >> + * Return: Architecture value >> + */ >> +int efi_get_pxe_arch(void); >> + >> #endif /* _LINUX_EFI_H */ >> -- >> 2.43.0 >> > > Thanks > /Ilias
Hi Heinrich, On Mon, 28 Oct 2024 at 20:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 10/25/24 11:56, Ilias Apalodimas wrote: > > Hi Simon, > > > > > > On Tue, 22 Oct 2024 at 15:00, Simon Glass <sjg@chromium.org> wrote: > >> > >> When the --native flag is given, pretend to be running the host > >> architecture rather than sandbox. > >> > >> Add an 'efidebug filename' command to report it. > > > > Heinrich does this allow you to continue your testing in native archs? > > > >> > >> Signed-off-by: Simon Glass <sjg@chromium.org> > >> --- > >> > >> Changes in v8: > >> - Add new patch to report host default-filename in native mode > >> > >> boot/bootmeth_efi.c | 25 ++------------------ > >> boot/efi_fname.c | 57 +++++++++++++++++++++++++++++++++------------ > >> cmd/efidebug.c | 25 ++++++++++++++++++++ > >> include/efi.h | 25 ++++++++++++++++++++ > >> 4 files changed, 94 insertions(+), 38 deletions(-) > >> > >> diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c > >> index 371b36d550b..f836aa655f5 100644 > >> --- a/boot/bootmeth_efi.c > >> +++ b/boot/bootmeth_efi.c > >> @@ -25,32 +25,11 @@ > >> > >> #define EFI_DIRNAME "/EFI/BOOT/" > >> > >> -static int get_efi_pxe_arch(void) > >> -{ > >> - /* http://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xml */ > >> - if (IS_ENABLED(CONFIG_ARM64)) > >> - return 0xb; > >> - else if (IS_ENABLED(CONFIG_ARM)) > >> - return 0xa; > >> - else if (IS_ENABLED(CONFIG_X86_64)) > >> - return 0x6; > >> - else if (IS_ENABLED(CONFIG_X86)) > >> - return 0x7; > >> - else if (IS_ENABLED(CONFIG_ARCH_RV32I)) > >> - return 0x19; > >> - else if (IS_ENABLED(CONFIG_ARCH_RV64I)) > >> - return 0x1b; > >> - else if (IS_ENABLED(CONFIG_SANDBOX)) > >> - return 0; /* not used */ > >> - > >> - return -EINVAL; > >> -} > >> - > >> static int get_efi_pxe_vci(char *str, int max_len) > >> { > >> int ret; > >> > >> - ret = get_efi_pxe_arch(); > >> + ret = efi_get_pxe_arch(); > >> if (ret < 0) > >> return ret; > >> > >> @@ -239,7 +218,7 @@ static int distro_efi_read_bootflow_net(struct bootflow *bflow) > >> ret = get_efi_pxe_vci(str, sizeof(str)); > >> if (ret) > >> return log_msg_ret("vci", ret); > >> - ret = get_efi_pxe_arch(); > >> + ret = efi_get_pxe_arch(); > >> if (ret < 0) > >> return log_msg_ret("arc", ret); > >> arch = ret; > >> diff --git a/boot/efi_fname.c b/boot/efi_fname.c > >> index a6b11383bba..790f9e2fa36 100644 > >> --- a/boot/efi_fname.c > >> +++ b/boot/efi_fname.c > >> @@ -9,29 +9,34 @@ > >> */ > >> > >> #include <efi.h> > >> +#include <errno.h> > >> #include <host_arch.h> > >> > >> -#ifdef CONFIG_SANDBOX > >> - > >> #if HOST_ARCH == HOST_ARCH_X86_64 > >> -#define BOOTEFI_NAME "BOOTX64.EFI" > >> +#define HOST_BOOTEFI_NAME "BOOTX64.EFI" > >> +#define HOST_PXE_ARCH 0x6 > >> #elif HOST_ARCH == HOST_ARCH_X86 > >> -#define BOOTEFI_NAME "BOOTIA32.EFI" > >> +#define HOST_BOOTEFI_NAME "BOOTIA32.EFI" > >> +#define HOST_PXE_ARCH 0x7 > >> #elif HOST_ARCH == HOST_ARCH_AARCH64 > >> -#define BOOTEFI_NAME "BOOTAA64.EFI" > >> +#define HOST_BOOTEFI_NAME "BOOTAA64.EFI" > >> +#define HOST_PXE_ARCH 0xb > >> #elif HOST_ARCH == HOST_ARCH_ARM > >> -#define BOOTEFI_NAME "BOOTARM.EFI" > >> +#define HOST_BOOTEFI_NAME "BOOTARM.EFI" > >> +#define HOST_PXE_ARCH 0xa > >> #elif HOST_ARCH == HOST_ARCH_RISCV32 > >> -#define BOOTEFI_NAME "BOOTRISCV32.EFI" > >> +#define HOST_BOOTEFI_NAME "BOOTRISCV32.EFI" > >> +#define HOST_PXE_ARCH 0x19 > >> #elif HOST_ARCH == HOST_ARCH_RISCV64 > >> -#define BOOTEFI_NAME "BOOTRISCV64.EFI" > >> +#define HOST_BOOTEFI_NAME "BOOTRISCV64.EFI" > >> +#define HOST_PXE_ARCH 0x1b > >> #else > >> -#error Unsupported UEFI architecture > >> +#error Unsupported Host architecture > >> #endif > >> > >> -#else > >> - > >> -#if defined(CONFIG_ARM64) > >> +#if defined(CONFIG_SANDBOX) > >> +#define BOOTEFI_NAME "BOOTSBOX.EFI" > >> +#elif defined(CONFIG_ARM64) > >> #define BOOTEFI_NAME "BOOTAA64.EFI" > >> #elif defined(CONFIG_ARM) > >> #define BOOTEFI_NAME "BOOTARM.EFI" > >> @@ -47,9 +52,31 @@ > >> #error Unsupported UEFI architecture > >> #endif>> > >> -#endif > >> - > >> const char *efi_get_basename(void) > >> { > >> - return BOOTEFI_NAME; > >> + return efi_use_host_arch() ? HOST_BOOTEFI_NAME : BOOTEFI_NAME; > >> +} > >> + > >> +int efi_get_pxe_arch(void) > >> +{ > >> + if (efi_use_host_arch()) > >> + return HOST_PXE_ARCH; > >> + > >> + /* http://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xml */ > >> + if (IS_ENABLED(CONFIG_ARM64)) > >> + return 0xb; > >> + else if (IS_ENABLED(CONFIG_ARM)) > >> + return 0xa; > >> + else if (IS_ENABLED(CONFIG_X86_64)) > >> + return 0x6; > >> + else if (IS_ENABLED(CONFIG_X86)) > >> + return 0x7; > >> + else if (IS_ENABLED(CONFIG_ARCH_RV32I)) > >> + return 0x19; > >> + else if (IS_ENABLED(CONFIG_ARCH_RV64I)) > >> + return 0x1b; > >> + else if (IS_ENABLED(CONFIG_SANDBOX)) > > Please, add a warning here: > > log_warn("You are using the sandbox in non-compliant mode\n"); > > The riscv64 sandbox can only run riscv64 binaries > The arm64 sandbox can only run arm64 binary. > > We cannot expect a DHCP server to do the expected with the value 0. > > So why should we introduce this? > > >> + return 0; /* not used */ > > The following comment would fit better: > > /* This breaks PXE booting on the sandbox.*/ This line is already in the code. I am just moving it to a new file. PXE booting is not supported on sandbox. Neither is EFI booting, until this series is applied. If you can see your way to drop your other objections on this series, I don't mind removing this line and just returning the error code. Regards, Simon
On Tue, Oct 29, 2024 at 04:45:54PM +0100, Simon Glass wrote: > Hi Heinrich, > > On Mon, 28 Oct 2024 at 20:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > On 10/25/24 11:56, Ilias Apalodimas wrote: > > > Hi Simon, > > > > > > > > > On Tue, 22 Oct 2024 at 15:00, Simon Glass <sjg@chromium.org> wrote: > > >> > > >> When the --native flag is given, pretend to be running the host > > >> architecture rather than sandbox. > > >> > > >> Add an 'efidebug filename' command to report it. > > > > > > Heinrich does this allow you to continue your testing in native archs? > > > > > >> > > >> Signed-off-by: Simon Glass <sjg@chromium.org> > > >> --- > > >> > > >> Changes in v8: > > >> - Add new patch to report host default-filename in native mode > > >> > > >> boot/bootmeth_efi.c | 25 ++------------------ > > >> boot/efi_fname.c | 57 +++++++++++++++++++++++++++++++++------------ > > >> cmd/efidebug.c | 25 ++++++++++++++++++++ > > >> include/efi.h | 25 ++++++++++++++++++++ > > >> 4 files changed, 94 insertions(+), 38 deletions(-) > > >> > > >> diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c > > >> index 371b36d550b..f836aa655f5 100644 > > >> --- a/boot/bootmeth_efi.c > > >> +++ b/boot/bootmeth_efi.c > > >> @@ -25,32 +25,11 @@ > > >> > > >> #define EFI_DIRNAME "/EFI/BOOT/" > > >> > > >> -static int get_efi_pxe_arch(void) > > >> -{ > > >> - /* http://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xml */ > > >> - if (IS_ENABLED(CONFIG_ARM64)) > > >> - return 0xb; > > >> - else if (IS_ENABLED(CONFIG_ARM)) > > >> - return 0xa; > > >> - else if (IS_ENABLED(CONFIG_X86_64)) > > >> - return 0x6; > > >> - else if (IS_ENABLED(CONFIG_X86)) > > >> - return 0x7; > > >> - else if (IS_ENABLED(CONFIG_ARCH_RV32I)) > > >> - return 0x19; > > >> - else if (IS_ENABLED(CONFIG_ARCH_RV64I)) > > >> - return 0x1b; > > >> - else if (IS_ENABLED(CONFIG_SANDBOX)) > > >> - return 0; /* not used */ > > >> - > > >> - return -EINVAL; > > >> -} > > >> - > > >> static int get_efi_pxe_vci(char *str, int max_len) > > >> { > > >> int ret; > > >> > > >> - ret = get_efi_pxe_arch(); > > >> + ret = efi_get_pxe_arch(); > > >> if (ret < 0) > > >> return ret; > > >> > > >> @@ -239,7 +218,7 @@ static int distro_efi_read_bootflow_net(struct bootflow *bflow) > > >> ret = get_efi_pxe_vci(str, sizeof(str)); > > >> if (ret) > > >> return log_msg_ret("vci", ret); > > >> - ret = get_efi_pxe_arch(); > > >> + ret = efi_get_pxe_arch(); > > >> if (ret < 0) > > >> return log_msg_ret("arc", ret); > > >> arch = ret; > > >> diff --git a/boot/efi_fname.c b/boot/efi_fname.c > > >> index a6b11383bba..790f9e2fa36 100644 > > >> --- a/boot/efi_fname.c > > >> +++ b/boot/efi_fname.c > > >> @@ -9,29 +9,34 @@ > > >> */ > > >> > > >> #include <efi.h> > > >> +#include <errno.h> > > >> #include <host_arch.h> > > >> > > >> -#ifdef CONFIG_SANDBOX > > >> - > > >> #if HOST_ARCH == HOST_ARCH_X86_64 > > >> -#define BOOTEFI_NAME "BOOTX64.EFI" > > >> +#define HOST_BOOTEFI_NAME "BOOTX64.EFI" > > >> +#define HOST_PXE_ARCH 0x6 > > >> #elif HOST_ARCH == HOST_ARCH_X86 > > >> -#define BOOTEFI_NAME "BOOTIA32.EFI" > > >> +#define HOST_BOOTEFI_NAME "BOOTIA32.EFI" > > >> +#define HOST_PXE_ARCH 0x7 > > >> #elif HOST_ARCH == HOST_ARCH_AARCH64 > > >> -#define BOOTEFI_NAME "BOOTAA64.EFI" > > >> +#define HOST_BOOTEFI_NAME "BOOTAA64.EFI" > > >> +#define HOST_PXE_ARCH 0xb > > >> #elif HOST_ARCH == HOST_ARCH_ARM > > >> -#define BOOTEFI_NAME "BOOTARM.EFI" > > >> +#define HOST_BOOTEFI_NAME "BOOTARM.EFI" > > >> +#define HOST_PXE_ARCH 0xa > > >> #elif HOST_ARCH == HOST_ARCH_RISCV32 > > >> -#define BOOTEFI_NAME "BOOTRISCV32.EFI" > > >> +#define HOST_BOOTEFI_NAME "BOOTRISCV32.EFI" > > >> +#define HOST_PXE_ARCH 0x19 > > >> #elif HOST_ARCH == HOST_ARCH_RISCV64 > > >> -#define BOOTEFI_NAME "BOOTRISCV64.EFI" > > >> +#define HOST_BOOTEFI_NAME "BOOTRISCV64.EFI" > > >> +#define HOST_PXE_ARCH 0x1b > > >> #else > > >> -#error Unsupported UEFI architecture > > >> +#error Unsupported Host architecture > > >> #endif > > >> > > >> -#else > > >> - > > >> -#if defined(CONFIG_ARM64) > > >> +#if defined(CONFIG_SANDBOX) > > >> +#define BOOTEFI_NAME "BOOTSBOX.EFI" > > >> +#elif defined(CONFIG_ARM64) > > >> #define BOOTEFI_NAME "BOOTAA64.EFI" > > >> #elif defined(CONFIG_ARM) > > >> #define BOOTEFI_NAME "BOOTARM.EFI" > > >> @@ -47,9 +52,31 @@ > > >> #error Unsupported UEFI architecture > > >> #endif>> > > >> -#endif > > >> - > > >> const char *efi_get_basename(void) > > >> { > > >> - return BOOTEFI_NAME; > > >> + return efi_use_host_arch() ? HOST_BOOTEFI_NAME : BOOTEFI_NAME; > > >> +} > > >> + > > >> +int efi_get_pxe_arch(void) > > >> +{ > > >> + if (efi_use_host_arch()) > > >> + return HOST_PXE_ARCH; > > >> + > > >> + /* http://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xml */ > > >> + if (IS_ENABLED(CONFIG_ARM64)) > > >> + return 0xb; > > >> + else if (IS_ENABLED(CONFIG_ARM)) > > >> + return 0xa; > > >> + else if (IS_ENABLED(CONFIG_X86_64)) > > >> + return 0x6; > > >> + else if (IS_ENABLED(CONFIG_X86)) > > >> + return 0x7; > > >> + else if (IS_ENABLED(CONFIG_ARCH_RV32I)) > > >> + return 0x19; > > >> + else if (IS_ENABLED(CONFIG_ARCH_RV64I)) > > >> + return 0x1b; > > >> + else if (IS_ENABLED(CONFIG_SANDBOX)) > > > > Please, add a warning here: > > > > log_warn("You are using the sandbox in non-compliant mode\n"); > > > > The riscv64 sandbox can only run riscv64 binaries > > The arm64 sandbox can only run arm64 binary. > > > > We cannot expect a DHCP server to do the expected with the value 0. > > > > So why should we introduce this? > > > > >> + return 0; /* not used */ > > > > The following comment would fit better: > > > > /* This breaks PXE booting on the sandbox.*/ > > This line is already in the code. I am just moving it to a new file. > PXE booting is not supported on sandbox. Neither is EFI booting, until > this series is applied. It sounds like EFI booting in sandbox is working and being tested and used (as a testing mechanism) right now. And the work and questions being raised here are around having your changes not break that existing use.
Hi Tom, On Tue, 29 Oct 2024 at 17:40, Tom Rini <trini@konsulko.com> wrote: > > On Tue, Oct 29, 2024 at 04:45:54PM +0100, Simon Glass wrote: > > Hi Heinrich, > > > > On Mon, 28 Oct 2024 at 20:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > On 10/25/24 11:56, Ilias Apalodimas wrote: > > > > Hi Simon, > > > > > > > > > > > > On Tue, 22 Oct 2024 at 15:00, Simon Glass <sjg@chromium.org> wrote: > > > >> > > > >> When the --native flag is given, pretend to be running the host > > > >> architecture rather than sandbox. > > > >> > > > >> Add an 'efidebug filename' command to report it. > > > > > > > > Heinrich does this allow you to continue your testing in native archs? > > > > > > > >> > > > >> Signed-off-by: Simon Glass <sjg@chromium.org> > > > >> --- > > > >> > > > >> Changes in v8: > > > >> - Add new patch to report host default-filename in native mode > > > >> > > > >> boot/bootmeth_efi.c | 25 ++------------------ > > > >> boot/efi_fname.c | 57 +++++++++++++++++++++++++++++++++------------ > > > >> cmd/efidebug.c | 25 ++++++++++++++++++++ > > > >> include/efi.h | 25 ++++++++++++++++++++ > > > >> 4 files changed, 94 insertions(+), 38 deletions(-) > > > >> > > > >> diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c > > > >> index 371b36d550b..f836aa655f5 100644 > > > >> --- a/boot/bootmeth_efi.c > > > >> +++ b/boot/bootmeth_efi.c > > > >> @@ -25,32 +25,11 @@ > > > >> > > > >> #define EFI_DIRNAME "/EFI/BOOT/" > > > >> > > > >> -static int get_efi_pxe_arch(void) > > > >> -{ > > > >> - /* http://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xml */ > > > >> - if (IS_ENABLED(CONFIG_ARM64)) > > > >> - return 0xb; > > > >> - else if (IS_ENABLED(CONFIG_ARM)) > > > >> - return 0xa; > > > >> - else if (IS_ENABLED(CONFIG_X86_64)) > > > >> - return 0x6; > > > >> - else if (IS_ENABLED(CONFIG_X86)) > > > >> - return 0x7; > > > >> - else if (IS_ENABLED(CONFIG_ARCH_RV32I)) > > > >> - return 0x19; > > > >> - else if (IS_ENABLED(CONFIG_ARCH_RV64I)) > > > >> - return 0x1b; > > > >> - else if (IS_ENABLED(CONFIG_SANDBOX)) > > > >> - return 0; /* not used */ > > > >> - > > > >> - return -EINVAL; > > > >> -} > > > >> - > > > >> static int get_efi_pxe_vci(char *str, int max_len) > > > >> { > > > >> int ret; > > > >> > > > >> - ret = get_efi_pxe_arch(); > > > >> + ret = efi_get_pxe_arch(); > > > >> if (ret < 0) > > > >> return ret; > > > >> > > > >> @@ -239,7 +218,7 @@ static int distro_efi_read_bootflow_net(struct bootflow *bflow) > > > >> ret = get_efi_pxe_vci(str, sizeof(str)); > > > >> if (ret) > > > >> return log_msg_ret("vci", ret); > > > >> - ret = get_efi_pxe_arch(); > > > >> + ret = efi_get_pxe_arch(); > > > >> if (ret < 0) > > > >> return log_msg_ret("arc", ret); > > > >> arch = ret; > > > >> diff --git a/boot/efi_fname.c b/boot/efi_fname.c > > > >> index a6b11383bba..790f9e2fa36 100644 > > > >> --- a/boot/efi_fname.c > > > >> +++ b/boot/efi_fname.c > > > >> @@ -9,29 +9,34 @@ > > > >> */ > > > >> > > > >> #include <efi.h> > > > >> +#include <errno.h> > > > >> #include <host_arch.h> > > > >> > > > >> -#ifdef CONFIG_SANDBOX > > > >> - > > > >> #if HOST_ARCH == HOST_ARCH_X86_64 > > > >> -#define BOOTEFI_NAME "BOOTX64.EFI" > > > >> +#define HOST_BOOTEFI_NAME "BOOTX64.EFI" > > > >> +#define HOST_PXE_ARCH 0x6 > > > >> #elif HOST_ARCH == HOST_ARCH_X86 > > > >> -#define BOOTEFI_NAME "BOOTIA32.EFI" > > > >> +#define HOST_BOOTEFI_NAME "BOOTIA32.EFI" > > > >> +#define HOST_PXE_ARCH 0x7 > > > >> #elif HOST_ARCH == HOST_ARCH_AARCH64 > > > >> -#define BOOTEFI_NAME "BOOTAA64.EFI" > > > >> +#define HOST_BOOTEFI_NAME "BOOTAA64.EFI" > > > >> +#define HOST_PXE_ARCH 0xb > > > >> #elif HOST_ARCH == HOST_ARCH_ARM > > > >> -#define BOOTEFI_NAME "BOOTARM.EFI" > > > >> +#define HOST_BOOTEFI_NAME "BOOTARM.EFI" > > > >> +#define HOST_PXE_ARCH 0xa > > > >> #elif HOST_ARCH == HOST_ARCH_RISCV32 > > > >> -#define BOOTEFI_NAME "BOOTRISCV32.EFI" > > > >> +#define HOST_BOOTEFI_NAME "BOOTRISCV32.EFI" > > > >> +#define HOST_PXE_ARCH 0x19 > > > >> #elif HOST_ARCH == HOST_ARCH_RISCV64 > > > >> -#define BOOTEFI_NAME "BOOTRISCV64.EFI" > > > >> +#define HOST_BOOTEFI_NAME "BOOTRISCV64.EFI" > > > >> +#define HOST_PXE_ARCH 0x1b > > > >> #else > > > >> -#error Unsupported UEFI architecture > > > >> +#error Unsupported Host architecture > > > >> #endif > > > >> > > > >> -#else > > > >> - > > > >> -#if defined(CONFIG_ARM64) > > > >> +#if defined(CONFIG_SANDBOX) > > > >> +#define BOOTEFI_NAME "BOOTSBOX.EFI" > > > >> +#elif defined(CONFIG_ARM64) > > > >> #define BOOTEFI_NAME "BOOTAA64.EFI" > > > >> #elif defined(CONFIG_ARM) > > > >> #define BOOTEFI_NAME "BOOTARM.EFI" > > > >> @@ -47,9 +52,31 @@ > > > >> #error Unsupported UEFI architecture > > > >> #endif>> > > > >> -#endif > > > >> - > > > >> const char *efi_get_basename(void) > > > >> { > > > >> - return BOOTEFI_NAME; > > > >> + return efi_use_host_arch() ? HOST_BOOTEFI_NAME : BOOTEFI_NAME; > > > >> +} > > > >> + > > > >> +int efi_get_pxe_arch(void) > > > >> +{ > > > >> + if (efi_use_host_arch()) > > > >> + return HOST_PXE_ARCH; > > > >> + > > > >> + /* http://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xml */ > > > >> + if (IS_ENABLED(CONFIG_ARM64)) > > > >> + return 0xb; > > > >> + else if (IS_ENABLED(CONFIG_ARM)) > > > >> + return 0xa; > > > >> + else if (IS_ENABLED(CONFIG_X86_64)) > > > >> + return 0x6; > > > >> + else if (IS_ENABLED(CONFIG_X86)) > > > >> + return 0x7; > > > >> + else if (IS_ENABLED(CONFIG_ARCH_RV32I)) > > > >> + return 0x19; > > > >> + else if (IS_ENABLED(CONFIG_ARCH_RV64I)) > > > >> + return 0x1b; > > > >> + else if (IS_ENABLED(CONFIG_SANDBOX)) > > > > > > Please, add a warning here: > > > > > > log_warn("You are using the sandbox in non-compliant mode\n"); > > > > > > The riscv64 sandbox can only run riscv64 binaries > > > The arm64 sandbox can only run arm64 binary. > > > > > > We cannot expect a DHCP server to do the expected with the value 0. > > > > > > So why should we introduce this? > > > > > > >> + return 0; /* not used */ > > > > > > The following comment would fit better: > > > > > > /* This breaks PXE booting on the sandbox.*/ > > > > This line is already in the code. I am just moving it to a new file. > > PXE booting is not supported on sandbox. Neither is EFI booting, until > > this series is applied. > > It sounds like EFI booting in sandbox is working and being tested and > used (as a testing mechanism) right now. And the work and questions > being raised here are around having your changes not break that existing > use. Well it would be, although at present that flow does not work, due to a problem either with EDK2 or QEMU. It may be fixed at some point. With this patch, the -N flag is needed to use this 'native' flow. The flag is not needed for CI, which is what I consider to be the normal 'sandbox' case. Regards, Simon
On Wed, Oct 30, 2024 at 08:44:30AM +0100, Simon Glass wrote: > Hi Tom, > > On Tue, 29 Oct 2024 at 17:40, Tom Rini <trini@konsulko.com> wrote: > > > > On Tue, Oct 29, 2024 at 04:45:54PM +0100, Simon Glass wrote: > > > Hi Heinrich, > > > > > > On Mon, 28 Oct 2024 at 20:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > > > On 10/25/24 11:56, Ilias Apalodimas wrote: > > > > > Hi Simon, > > > > > > > > > > > > > > > On Tue, 22 Oct 2024 at 15:00, Simon Glass <sjg@chromium.org> wrote: > > > > >> > > > > >> When the --native flag is given, pretend to be running the host > > > > >> architecture rather than sandbox. > > > > >> > > > > >> Add an 'efidebug filename' command to report it. > > > > > > > > > > Heinrich does this allow you to continue your testing in native archs? > > > > > > > > > >> > > > > >> Signed-off-by: Simon Glass <sjg@chromium.org> > > > > >> --- > > > > >> > > > > >> Changes in v8: > > > > >> - Add new patch to report host default-filename in native mode > > > > >> > > > > >> boot/bootmeth_efi.c | 25 ++------------------ > > > > >> boot/efi_fname.c | 57 +++++++++++++++++++++++++++++++++------------ > > > > >> cmd/efidebug.c | 25 ++++++++++++++++++++ > > > > >> include/efi.h | 25 ++++++++++++++++++++ > > > > >> 4 files changed, 94 insertions(+), 38 deletions(-) > > > > >> > > > > >> diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c > > > > >> index 371b36d550b..f836aa655f5 100644 > > > > >> --- a/boot/bootmeth_efi.c > > > > >> +++ b/boot/bootmeth_efi.c > > > > >> @@ -25,32 +25,11 @@ > > > > >> > > > > >> #define EFI_DIRNAME "/EFI/BOOT/" > > > > >> > > > > >> -static int get_efi_pxe_arch(void) > > > > >> -{ > > > > >> - /* http://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xml */ > > > > >> - if (IS_ENABLED(CONFIG_ARM64)) > > > > >> - return 0xb; > > > > >> - else if (IS_ENABLED(CONFIG_ARM)) > > > > >> - return 0xa; > > > > >> - else if (IS_ENABLED(CONFIG_X86_64)) > > > > >> - return 0x6; > > > > >> - else if (IS_ENABLED(CONFIG_X86)) > > > > >> - return 0x7; > > > > >> - else if (IS_ENABLED(CONFIG_ARCH_RV32I)) > > > > >> - return 0x19; > > > > >> - else if (IS_ENABLED(CONFIG_ARCH_RV64I)) > > > > >> - return 0x1b; > > > > >> - else if (IS_ENABLED(CONFIG_SANDBOX)) > > > > >> - return 0; /* not used */ > > > > >> - > > > > >> - return -EINVAL; > > > > >> -} > > > > >> - > > > > >> static int get_efi_pxe_vci(char *str, int max_len) > > > > >> { > > > > >> int ret; > > > > >> > > > > >> - ret = get_efi_pxe_arch(); > > > > >> + ret = efi_get_pxe_arch(); > > > > >> if (ret < 0) > > > > >> return ret; > > > > >> > > > > >> @@ -239,7 +218,7 @@ static int distro_efi_read_bootflow_net(struct bootflow *bflow) > > > > >> ret = get_efi_pxe_vci(str, sizeof(str)); > > > > >> if (ret) > > > > >> return log_msg_ret("vci", ret); > > > > >> - ret = get_efi_pxe_arch(); > > > > >> + ret = efi_get_pxe_arch(); > > > > >> if (ret < 0) > > > > >> return log_msg_ret("arc", ret); > > > > >> arch = ret; > > > > >> diff --git a/boot/efi_fname.c b/boot/efi_fname.c > > > > >> index a6b11383bba..790f9e2fa36 100644 > > > > >> --- a/boot/efi_fname.c > > > > >> +++ b/boot/efi_fname.c > > > > >> @@ -9,29 +9,34 @@ > > > > >> */ > > > > >> > > > > >> #include <efi.h> > > > > >> +#include <errno.h> > > > > >> #include <host_arch.h> > > > > >> > > > > >> -#ifdef CONFIG_SANDBOX > > > > >> - > > > > >> #if HOST_ARCH == HOST_ARCH_X86_64 > > > > >> -#define BOOTEFI_NAME "BOOTX64.EFI" > > > > >> +#define HOST_BOOTEFI_NAME "BOOTX64.EFI" > > > > >> +#define HOST_PXE_ARCH 0x6 > > > > >> #elif HOST_ARCH == HOST_ARCH_X86 > > > > >> -#define BOOTEFI_NAME "BOOTIA32.EFI" > > > > >> +#define HOST_BOOTEFI_NAME "BOOTIA32.EFI" > > > > >> +#define HOST_PXE_ARCH 0x7 > > > > >> #elif HOST_ARCH == HOST_ARCH_AARCH64 > > > > >> -#define BOOTEFI_NAME "BOOTAA64.EFI" > > > > >> +#define HOST_BOOTEFI_NAME "BOOTAA64.EFI" > > > > >> +#define HOST_PXE_ARCH 0xb > > > > >> #elif HOST_ARCH == HOST_ARCH_ARM > > > > >> -#define BOOTEFI_NAME "BOOTARM.EFI" > > > > >> +#define HOST_BOOTEFI_NAME "BOOTARM.EFI" > > > > >> +#define HOST_PXE_ARCH 0xa > > > > >> #elif HOST_ARCH == HOST_ARCH_RISCV32 > > > > >> -#define BOOTEFI_NAME "BOOTRISCV32.EFI" > > > > >> +#define HOST_BOOTEFI_NAME "BOOTRISCV32.EFI" > > > > >> +#define HOST_PXE_ARCH 0x19 > > > > >> #elif HOST_ARCH == HOST_ARCH_RISCV64 > > > > >> -#define BOOTEFI_NAME "BOOTRISCV64.EFI" > > > > >> +#define HOST_BOOTEFI_NAME "BOOTRISCV64.EFI" > > > > >> +#define HOST_PXE_ARCH 0x1b > > > > >> #else > > > > >> -#error Unsupported UEFI architecture > > > > >> +#error Unsupported Host architecture > > > > >> #endif > > > > >> > > > > >> -#else > > > > >> - > > > > >> -#if defined(CONFIG_ARM64) > > > > >> +#if defined(CONFIG_SANDBOX) > > > > >> +#define BOOTEFI_NAME "BOOTSBOX.EFI" > > > > >> +#elif defined(CONFIG_ARM64) > > > > >> #define BOOTEFI_NAME "BOOTAA64.EFI" > > > > >> #elif defined(CONFIG_ARM) > > > > >> #define BOOTEFI_NAME "BOOTARM.EFI" > > > > >> @@ -47,9 +52,31 @@ > > > > >> #error Unsupported UEFI architecture > > > > >> #endif>> > > > > >> -#endif > > > > >> - > > > > >> const char *efi_get_basename(void) > > > > >> { > > > > >> - return BOOTEFI_NAME; > > > > >> + return efi_use_host_arch() ? HOST_BOOTEFI_NAME : BOOTEFI_NAME; > > > > >> +} > > > > >> + > > > > >> +int efi_get_pxe_arch(void) > > > > >> +{ > > > > >> + if (efi_use_host_arch()) > > > > >> + return HOST_PXE_ARCH; > > > > >> + > > > > >> + /* http://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xml */ > > > > >> + if (IS_ENABLED(CONFIG_ARM64)) > > > > >> + return 0xb; > > > > >> + else if (IS_ENABLED(CONFIG_ARM)) > > > > >> + return 0xa; > > > > >> + else if (IS_ENABLED(CONFIG_X86_64)) > > > > >> + return 0x6; > > > > >> + else if (IS_ENABLED(CONFIG_X86)) > > > > >> + return 0x7; > > > > >> + else if (IS_ENABLED(CONFIG_ARCH_RV32I)) > > > > >> + return 0x19; > > > > >> + else if (IS_ENABLED(CONFIG_ARCH_RV64I)) > > > > >> + return 0x1b; > > > > >> + else if (IS_ENABLED(CONFIG_SANDBOX)) > > > > > > > > Please, add a warning here: > > > > > > > > log_warn("You are using the sandbox in non-compliant mode\n"); > > > > > > > > The riscv64 sandbox can only run riscv64 binaries > > > > The arm64 sandbox can only run arm64 binary. > > > > > > > > We cannot expect a DHCP server to do the expected with the value 0. > > > > > > > > So why should we introduce this? > > > > > > > > >> + return 0; /* not used */ > > > > > > > > The following comment would fit better: > > > > > > > > /* This breaks PXE booting on the sandbox.*/ > > > > > > This line is already in the code. I am just moving it to a new file. > > > PXE booting is not supported on sandbox. Neither is EFI booting, until > > > this series is applied. > > > > It sounds like EFI booting in sandbox is working and being tested and > > used (as a testing mechanism) right now. And the work and questions > > being raised here are around having your changes not break that existing > > use. > > Well it would be, although at present that flow does not work, due to > a problem either with EDK2 or QEMU. It may be fixed at some point. Since it sounds like Heinrich is testing shim+grub binaries currently, I'm not sure what you mean here. > With this patch, the -N flag is needed to use this 'native' flow. The > flag is not needed for CI, which is what I consider to be the normal > 'sandbox' case. Well, unfortunately your code has escaped in to the wild and is being used by other people now. For your still unclear what we need it for use case, you should be invoking the magic logic you need, not the other way around.
diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c index 371b36d550b..f836aa655f5 100644 --- a/boot/bootmeth_efi.c +++ b/boot/bootmeth_efi.c @@ -25,32 +25,11 @@ #define EFI_DIRNAME "/EFI/BOOT/" -static int get_efi_pxe_arch(void) -{ - /* http://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xml */ - if (IS_ENABLED(CONFIG_ARM64)) - return 0xb; - else if (IS_ENABLED(CONFIG_ARM)) - return 0xa; - else if (IS_ENABLED(CONFIG_X86_64)) - return 0x6; - else if (IS_ENABLED(CONFIG_X86)) - return 0x7; - else if (IS_ENABLED(CONFIG_ARCH_RV32I)) - return 0x19; - else if (IS_ENABLED(CONFIG_ARCH_RV64I)) - return 0x1b; - else if (IS_ENABLED(CONFIG_SANDBOX)) - return 0; /* not used */ - - return -EINVAL; -} - static int get_efi_pxe_vci(char *str, int max_len) { int ret; - ret = get_efi_pxe_arch(); + ret = efi_get_pxe_arch(); if (ret < 0) return ret; @@ -239,7 +218,7 @@ static int distro_efi_read_bootflow_net(struct bootflow *bflow) ret = get_efi_pxe_vci(str, sizeof(str)); if (ret) return log_msg_ret("vci", ret); - ret = get_efi_pxe_arch(); + ret = efi_get_pxe_arch(); if (ret < 0) return log_msg_ret("arc", ret); arch = ret; diff --git a/boot/efi_fname.c b/boot/efi_fname.c index a6b11383bba..790f9e2fa36 100644 --- a/boot/efi_fname.c +++ b/boot/efi_fname.c @@ -9,29 +9,34 @@ */ #include <efi.h> +#include <errno.h> #include <host_arch.h> -#ifdef CONFIG_SANDBOX - #if HOST_ARCH == HOST_ARCH_X86_64 -#define BOOTEFI_NAME "BOOTX64.EFI" +#define HOST_BOOTEFI_NAME "BOOTX64.EFI" +#define HOST_PXE_ARCH 0x6 #elif HOST_ARCH == HOST_ARCH_X86 -#define BOOTEFI_NAME "BOOTIA32.EFI" +#define HOST_BOOTEFI_NAME "BOOTIA32.EFI" +#define HOST_PXE_ARCH 0x7 #elif HOST_ARCH == HOST_ARCH_AARCH64 -#define BOOTEFI_NAME "BOOTAA64.EFI" +#define HOST_BOOTEFI_NAME "BOOTAA64.EFI" +#define HOST_PXE_ARCH 0xb #elif HOST_ARCH == HOST_ARCH_ARM -#define BOOTEFI_NAME "BOOTARM.EFI" +#define HOST_BOOTEFI_NAME "BOOTARM.EFI" +#define HOST_PXE_ARCH 0xa #elif HOST_ARCH == HOST_ARCH_RISCV32 -#define BOOTEFI_NAME "BOOTRISCV32.EFI" +#define HOST_BOOTEFI_NAME "BOOTRISCV32.EFI" +#define HOST_PXE_ARCH 0x19 #elif HOST_ARCH == HOST_ARCH_RISCV64 -#define BOOTEFI_NAME "BOOTRISCV64.EFI" +#define HOST_BOOTEFI_NAME "BOOTRISCV64.EFI" +#define HOST_PXE_ARCH 0x1b #else -#error Unsupported UEFI architecture +#error Unsupported Host architecture #endif -#else - -#if defined(CONFIG_ARM64) +#if defined(CONFIG_SANDBOX) +#define BOOTEFI_NAME "BOOTSBOX.EFI" +#elif defined(CONFIG_ARM64) #define BOOTEFI_NAME "BOOTAA64.EFI" #elif defined(CONFIG_ARM) #define BOOTEFI_NAME "BOOTARM.EFI" @@ -47,9 +52,31 @@ #error Unsupported UEFI architecture #endif -#endif - const char *efi_get_basename(void) { - return BOOTEFI_NAME; + return efi_use_host_arch() ? HOST_BOOTEFI_NAME : BOOTEFI_NAME; +} + +int efi_get_pxe_arch(void) +{ + if (efi_use_host_arch()) + return HOST_PXE_ARCH; + + /* http://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xml */ + if (IS_ENABLED(CONFIG_ARM64)) + return 0xb; + else if (IS_ENABLED(CONFIG_ARM)) + return 0xa; + else if (IS_ENABLED(CONFIG_X86_64)) + return 0x6; + else if (IS_ENABLED(CONFIG_X86)) + return 0x7; + else if (IS_ENABLED(CONFIG_ARCH_RV32I)) + return 0x19; + else if (IS_ENABLED(CONFIG_ARCH_RV64I)) + return 0x1b; + else if (IS_ENABLED(CONFIG_SANDBOX)) + return 0; /* not used */ + + return -EINVAL; } diff --git a/cmd/efidebug.c b/cmd/efidebug.c index e040fe75fa1..02f1e080e88 100644 --- a/cmd/efidebug.c +++ b/cmd/efidebug.c @@ -511,6 +511,27 @@ static int do_efi_show_images(struct cmd_tbl *cmdtp, int flag, return CMD_RET_SUCCESS; } +/** + * do_efi_show_defaults() - show UEFI default filename and PXE architecture + * + * @cmdtp: Command table + * @flag: Command flag + * @argc: Number of arguments + * @argv: Argument array + * Return: CMD_RET_SUCCESS on success, CMD_RET_RET_FAILURE on failure + * + * Implement efidebug "defaults" sub-command. + * Shows the default EFI filename and PXE architecture + */ +static int do_efi_show_defaults(struct cmd_tbl *cmdtp, int flag, + int argc, char *const argv[]) +{ + printf("Default boot path: EFI\\BOOT\\%s\n", efi_get_basename()); + printf("PXE arch: 0x%02x\n", efi_get_pxe_arch()); + + return CMD_RET_SUCCESS; +} + static const char * const efi_mem_type_string[] = { [EFI_RESERVED_MEMORY_TYPE] = "RESERVED", [EFI_LOADER_CODE] = "LOADER CODE", @@ -1561,6 +1582,8 @@ static struct cmd_tbl cmd_efidebug_sub[] = { "", ""), U_BOOT_CMD_MKENT(dh, CONFIG_SYS_MAXARGS, 1, do_efi_show_handles, "", ""), + U_BOOT_CMD_MKENT(defaults, CONFIG_SYS_MAXARGS, 1, do_efi_show_defaults, + "", ""), U_BOOT_CMD_MKENT(images, CONFIG_SYS_MAXARGS, 1, do_efi_show_images, "", ""), U_BOOT_CMD_MKENT(memmap, CONFIG_SYS_MAXARGS, 1, do_efi_show_memmap, @@ -1653,6 +1676,8 @@ U_BOOT_LONGHELP(efidebug, " - show UEFI drivers\n" "efidebug dh\n" " - show UEFI handles\n" + "efidebug defaults\n" + " - show default EFI filename and PXE architecture\n" "efidebug images\n" " - show loaded images\n" "efidebug memmap\n" diff --git a/include/efi.h b/include/efi.h index 1b8093bd4d3..70bb47e742f 100644 --- a/include/efi.h +++ b/include/efi.h @@ -678,4 +678,29 @@ void efi_show_tables(struct efi_system_table *systab); */ const char *efi_get_basename(void); +#ifdef CONFIG_SANDBOX +#include <asm/state.h> +#endif + +static inline bool efi_use_host_arch(void) +{ +#ifdef CONFIG_SANDBOX + struct sandbox_state *state = state_get_current(); + + return state->native; +#else + return false; +#endif +} + +/** + * efi_get_pxe_arch() - Get the architecture value for PXE + * + * See: + * http://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xml + * + * Return: Architecture value + */ +int efi_get_pxe_arch(void); + #endif /* _LINUX_EFI_H */
When the --native flag is given, pretend to be running the host architecture rather than sandbox. Add an 'efidebug filename' command to report it. Signed-off-by: Simon Glass <sjg@chromium.org> --- Changes in v8: - Add new patch to report host default-filename in native mode boot/bootmeth_efi.c | 25 ++------------------ boot/efi_fname.c | 57 +++++++++++++++++++++++++++++++++------------ cmd/efidebug.c | 25 ++++++++++++++++++++ include/efi.h | 25 ++++++++++++++++++++ 4 files changed, 94 insertions(+), 38 deletions(-)