[U-Boot] [PATCH 1/1] efi_loader: fix building crt0 on arm
Before the patch an undefined constant EFI_SUBSYSTEM was used in the crt0 code. The current version of binutils does not swallow the error.
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=888403
The necessary constant IMAGE_SUBSYSTEM_EFI_APPLICATION is already defined in pe.h. So let's factor out asm-generic/pe.h for the image subsystem constants and use it in our assembler code.
IMAGE_SUBSYSTEM_SAL_RUNTIME_DRIVER does not exist in the specification let's use IMAGE_SUBSYSTEM_EFI_ROM instead.
The include pe.h is only used in code maintained by Alex so let him be the maintainer here too.
Reported-by: Andre Przywara andre.przywara@arm.com Cc: Alexander Graf agraf@suse.de Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- MAINTAINERS | 2 ++ arch/arm/lib/crt0_aarch64_efi.S | 4 +++- arch/arm/lib/crt0_arm_efi.S | 4 +++- include/asm-generic/pe.h | 21 +++++++++++++++++++++ include/pe.h | 8 ++------ lib/efi_loader/efi_image_loader.c | 2 +- 6 files changed, 32 insertions(+), 9 deletions(-) create mode 100644 include/asm-generic/pe.h
diff --git a/MAINTAINERS b/MAINTAINERS index 0aecc18a6c..879b41c97e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -291,6 +291,8 @@ S: Maintained T: git git://github.com/agraf/u-boot.git F: doc/README.iscsi F: include/efi* +F: include/pe.h +F: include/asm-generic/pe.h F: lib/efi*/ F: test/py/tests/test_efi* F: cmd/bootefi.c diff --git a/arch/arm/lib/crt0_aarch64_efi.S b/arch/arm/lib/crt0_aarch64_efi.S index 52056469be..9b0e894f8a 100644 --- a/arch/arm/lib/crt0_aarch64_efi.S +++ b/arch/arm/lib/crt0_aarch64_efi.S @@ -8,6 +8,8 @@ * This file is taken and modified from the gnu-efi project. */
+#include <asm-generic/pe.h> + .section .text.head
/* @@ -62,7 +64,7 @@ extra_header_fields: */ .long _start - ImageBase /* SizeOfHeaders */ .long 0 /* CheckSum */ - .short EFI_SUBSYSTEM /* Subsystem */ + .short IMAGE_SUBSYSTEM_EFI_APPLICATION /* Subsystem */ .short 0 /* DllCharacteristics */ .quad 0 /* SizeOfStackReserve */ .quad 0 /* SizeOfStackCommit */ diff --git a/arch/arm/lib/crt0_arm_efi.S b/arch/arm/lib/crt0_arm_efi.S index 967c885982..af55bba4ba 100644 --- a/arch/arm/lib/crt0_arm_efi.S +++ b/arch/arm/lib/crt0_arm_efi.S @@ -8,6 +8,8 @@ * This file is taken and modified from the gnu-efi project. */
+#include <asm-generic/pe.h> + .section .text.head
/* @@ -64,7 +66,7 @@ extra_header_fields: */ .long _start - image_base /* SizeOfHeaders */ .long 0 /* CheckSum */ - .short EFI_SUBSYSTEM /* Subsystem */ + .short IMAGE_SUBSYSTEM_EFI_APPLICATION /* Subsystem */ .short 0 /* DllCharacteristics */ .long 0 /* SizeOfStackReserve */ .long 0 /* SizeOfStackCommit */ diff --git a/include/asm-generic/pe.h b/include/asm-generic/pe.h new file mode 100644 index 0000000000..d1683f238a --- /dev/null +++ b/include/asm-generic/pe.h @@ -0,0 +1,21 @@ +/* + * Portable Executable and Common Object Constants + * + * Copyright (c) 2018 Heinrich Schuchardt + * + * based on the "Microsoft Portable Executable and Common Object File Format + * Specification", revision 11, 2017-01-23 + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#ifndef _ASM_PE_H +#define _ASM_PE_H + +/* Subsystem type */ +#define IMAGE_SUBSYSTEM_EFI_APPLICATION 10 +#define IMAGE_SUBSYSTEM_EFI_BOOT_SERVICE_DRIVER 11 +#define IMAGE_SUBSYSTEM_EFI_RUNTIME_DRIVER 12 +#define IMAGE_SUBSYSTEM_EFI_ROM 13 + +#endif /* _ASM_PE_H */ diff --git a/include/pe.h b/include/pe.h index 4ef3e92efa..c3a19cef76 100644 --- a/include/pe.h +++ b/include/pe.h @@ -11,6 +11,8 @@ #ifndef _PE_H #define _PE_H
+#include <asm-generic/pe.h> + typedef struct _IMAGE_DOS_HEADER { uint16_t e_magic; /* 00: MZ Header signature */ uint16_t e_cblp; /* 02: Bytes on last page of file */ @@ -62,12 +64,6 @@ typedef struct _IMAGE_DATA_DIRECTORY {
#define IMAGE_NUMBEROF_DIRECTORY_ENTRIES 16
-/* PE32+ Subsystem type for EFI images */ -#define IMAGE_SUBSYSTEM_EFI_APPLICATION 10 -#define IMAGE_SUBSYSTEM_EFI_BOOT_SERVICE_DRIVER 11 -#define IMAGE_SUBSYSTEM_EFI_RUNTIME_DRIVER 12 -#define IMAGE_SUBSYSTEM_SAL_RUNTIME_DRIVER 13 - typedef struct _IMAGE_OPTIONAL_HEADER64 { uint16_t Magic; /* 0x20b */ uint8_t MajorLinkerVersion; diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c index 9d2214b481..cac64ba9fe 100644 --- a/lib/efi_loader/efi_image_loader.c +++ b/lib/efi_loader/efi_image_loader.c @@ -94,7 +94,7 @@ static void efi_set_code_and_data_type( loaded_image_info->image_data_type = EFI_BOOT_SERVICES_DATA; break; case IMAGE_SUBSYSTEM_EFI_RUNTIME_DRIVER: - case IMAGE_SUBSYSTEM_SAL_RUNTIME_DRIVER: + case IMAGE_SUBSYSTEM_EFI_ROM: loaded_image_info->image_code_type = EFI_RUNTIME_SERVICES_CODE; loaded_image_info->image_data_type = EFI_RUNTIME_SERVICES_DATA; break;
On 2018-01-31, Heinrich Schuchardt wrote:
Before the patch an undefined constant EFI_SUBSYSTEM was used in the crt0 code. The current version of binutils does not swallow the error.
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=888403
The necessary constant IMAGE_SUBSYSTEM_EFI_APPLICATION is already defined in pe.h. So let's factor out asm-generic/pe.h for the image subsystem constants and use it in our assembler code.
IMAGE_SUBSYSTEM_SAL_RUNTIME_DRIVER does not exist in the specification let's use IMAGE_SUBSYSTEM_EFI_ROM instead.
The include pe.h is only used in code maintained by Alex so let him be the maintainer here too.
Reported-by: Andre Przywara andre.przywara@arm.com Cc: Alexander Graf agraf@suse.de Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
MAINTAINERS | 2 ++ arch/arm/lib/crt0_aarch64_efi.S | 4 +++- arch/arm/lib/crt0_arm_efi.S | 4 +++- include/asm-generic/pe.h | 21 +++++++++++++++++++++ include/pe.h | 8 ++------ lib/efi_loader/efi_image_loader.c | 2 +- 6 files changed, 32 insertions(+), 9 deletions(-) create mode 100644 include/asm-generic/pe.h
I tested that it at least fixed the build failure with v2018.03-rc1, though I haven't tested the resulting images actually boot.
Also haven't been able to successfully backported it to v2018.01...
With those caveats:
Tested-by: Vagrant Cascadian vagrant@debian.org
live well, vagrant
diff --git a/MAINTAINERS b/MAINTAINERS index 0aecc18a6c..879b41c97e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -291,6 +291,8 @@ S: Maintained T: git git://github.com/agraf/u-boot.git F: doc/README.iscsi F: include/efi* +F: include/pe.h +F: include/asm-generic/pe.h F: lib/efi*/ F: test/py/tests/test_efi* F: cmd/bootefi.c diff --git a/arch/arm/lib/crt0_aarch64_efi.S b/arch/arm/lib/crt0_aarch64_efi.S index 52056469be..9b0e894f8a 100644 --- a/arch/arm/lib/crt0_aarch64_efi.S +++ b/arch/arm/lib/crt0_aarch64_efi.S @@ -8,6 +8,8 @@
- This file is taken and modified from the gnu-efi project.
*/
+#include <asm-generic/pe.h>
.section .text.head
/*
@@ -62,7 +64,7 @@ extra_header_fields: */ .long _start - ImageBase /* SizeOfHeaders */ .long 0 /* CheckSum */
- .short EFI_SUBSYSTEM /* Subsystem */
- .short IMAGE_SUBSYSTEM_EFI_APPLICATION /* Subsystem */ .short 0 /* DllCharacteristics */ .quad 0 /* SizeOfStackReserve */ .quad 0 /* SizeOfStackCommit */
diff --git a/arch/arm/lib/crt0_arm_efi.S b/arch/arm/lib/crt0_arm_efi.S index 967c885982..af55bba4ba 100644 --- a/arch/arm/lib/crt0_arm_efi.S +++ b/arch/arm/lib/crt0_arm_efi.S @@ -8,6 +8,8 @@
- This file is taken and modified from the gnu-efi project.
*/
+#include <asm-generic/pe.h>
.section .text.head
/*
@@ -64,7 +66,7 @@ extra_header_fields: */ .long _start - image_base /* SizeOfHeaders */ .long 0 /* CheckSum */
- .short EFI_SUBSYSTEM /* Subsystem */
- .short IMAGE_SUBSYSTEM_EFI_APPLICATION /* Subsystem */ .short 0 /* DllCharacteristics */ .long 0 /* SizeOfStackReserve */ .long 0 /* SizeOfStackCommit */
diff --git a/include/asm-generic/pe.h b/include/asm-generic/pe.h new file mode 100644 index 0000000000..d1683f238a --- /dev/null +++ b/include/asm-generic/pe.h @@ -0,0 +1,21 @@ +/*
- Portable Executable and Common Object Constants
- Copyright (c) 2018 Heinrich Schuchardt
- based on the "Microsoft Portable Executable and Common Object File Format
- Specification", revision 11, 2017-01-23
- SPDX-License-Identifier: GPL-2.0+
- */
+#ifndef _ASM_PE_H +#define _ASM_PE_H
+/* Subsystem type */ +#define IMAGE_SUBSYSTEM_EFI_APPLICATION 10 +#define IMAGE_SUBSYSTEM_EFI_BOOT_SERVICE_DRIVER 11 +#define IMAGE_SUBSYSTEM_EFI_RUNTIME_DRIVER 12 +#define IMAGE_SUBSYSTEM_EFI_ROM 13
+#endif /* _ASM_PE_H */ diff --git a/include/pe.h b/include/pe.h index 4ef3e92efa..c3a19cef76 100644 --- a/include/pe.h +++ b/include/pe.h @@ -11,6 +11,8 @@ #ifndef _PE_H #define _PE_H
+#include <asm-generic/pe.h>
typedef struct _IMAGE_DOS_HEADER { uint16_t e_magic; /* 00: MZ Header signature */ uint16_t e_cblp; /* 02: Bytes on last page of file */ @@ -62,12 +64,6 @@ typedef struct _IMAGE_DATA_DIRECTORY {
#define IMAGE_NUMBEROF_DIRECTORY_ENTRIES 16
-/* PE32+ Subsystem type for EFI images */ -#define IMAGE_SUBSYSTEM_EFI_APPLICATION 10 -#define IMAGE_SUBSYSTEM_EFI_BOOT_SERVICE_DRIVER 11 -#define IMAGE_SUBSYSTEM_EFI_RUNTIME_DRIVER 12 -#define IMAGE_SUBSYSTEM_SAL_RUNTIME_DRIVER 13
typedef struct _IMAGE_OPTIONAL_HEADER64 { uint16_t Magic; /* 0x20b */ uint8_t MajorLinkerVersion; diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c index 9d2214b481..cac64ba9fe 100644 --- a/lib/efi_loader/efi_image_loader.c +++ b/lib/efi_loader/efi_image_loader.c @@ -94,7 +94,7 @@ static void efi_set_code_and_data_type( loaded_image_info->image_data_type = EFI_BOOT_SERVICES_DATA; break; case IMAGE_SUBSYSTEM_EFI_RUNTIME_DRIVER:
- case IMAGE_SUBSYSTEM_SAL_RUNTIME_DRIVER:
- case IMAGE_SUBSYSTEM_EFI_ROM: loaded_image_info->image_code_type = EFI_RUNTIME_SERVICES_CODE; loaded_image_info->image_data_type = EFI_RUNTIME_SERVICES_DATA; break;
Hi Heinrich,
On 31/01/18 18:45, Heinrich Schuchardt wrote:
Before the patch an undefined constant EFI_SUBSYSTEM was used in the crt0 code. The current version of binutils does not swallow the error.
Ah, indeed it was the switch to binutils 2.30 in my case that made the difference, not the GCC version. Thanks for pointing that out!
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=888403
The necessary constant IMAGE_SUBSYSTEM_EFI_APPLICATION is already defined in pe.h. So let's factor out asm-generic/pe.h for the image subsystem constants and use it in our assembler code.
IMAGE_SUBSYSTEM_SAL_RUNTIME_DRIVER does not exist in the specification let's use IMAGE_SUBSYSTEM_EFI_ROM instead.
The include pe.h is only used in code maintained by Alex so let him be the maintainer here too.
Thanks very much for the quick handling! The patch looks reasonable, the constants are correct. Just wondering how this worked before? EFI_SUBSYSTEM is clearly not defined anywhere, so does it really get silently translated to 0? Amazing...
So indeed that fixes it for me. Quickly boot tested on the SoPine64 with "bootefi hello".
Reviewed-by: Andre Przywara andre.przywara@arm.com Tested-by: Andre Przywara andre.przywara@arm.com
Cheers, Andre.
Reported-by: Andre Przywara andre.przywara@arm.com Cc: Alexander Graf agraf@suse.de Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
MAINTAINERS | 2 ++ arch/arm/lib/crt0_aarch64_efi.S | 4 +++- arch/arm/lib/crt0_arm_efi.S | 4 +++- include/asm-generic/pe.h | 21 +++++++++++++++++++++ include/pe.h | 8 ++------ lib/efi_loader/efi_image_loader.c | 2 +- 6 files changed, 32 insertions(+), 9 deletions(-) create mode 100644 include/asm-generic/pe.h
diff --git a/MAINTAINERS b/MAINTAINERS index 0aecc18a6c..879b41c97e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -291,6 +291,8 @@ S: Maintained T: git git://github.com/agraf/u-boot.git F: doc/README.iscsi F: include/efi* +F: include/pe.h +F: include/asm-generic/pe.h F: lib/efi*/ F: test/py/tests/test_efi* F: cmd/bootefi.c diff --git a/arch/arm/lib/crt0_aarch64_efi.S b/arch/arm/lib/crt0_aarch64_efi.S index 52056469be..9b0e894f8a 100644 --- a/arch/arm/lib/crt0_aarch64_efi.S +++ b/arch/arm/lib/crt0_aarch64_efi.S @@ -8,6 +8,8 @@
- This file is taken and modified from the gnu-efi project.
*/
+#include <asm-generic/pe.h>
.section .text.head
/*
@@ -62,7 +64,7 @@ extra_header_fields: */ .long _start - ImageBase /* SizeOfHeaders */ .long 0 /* CheckSum */
- .short EFI_SUBSYSTEM /* Subsystem */
- .short IMAGE_SUBSYSTEM_EFI_APPLICATION /* Subsystem */ .short 0 /* DllCharacteristics */ .quad 0 /* SizeOfStackReserve */ .quad 0 /* SizeOfStackCommit */
diff --git a/arch/arm/lib/crt0_arm_efi.S b/arch/arm/lib/crt0_arm_efi.S index 967c885982..af55bba4ba 100644 --- a/arch/arm/lib/crt0_arm_efi.S +++ b/arch/arm/lib/crt0_arm_efi.S @@ -8,6 +8,8 @@
- This file is taken and modified from the gnu-efi project.
*/
+#include <asm-generic/pe.h>
.section .text.head
/*
@@ -64,7 +66,7 @@ extra_header_fields: */ .long _start - image_base /* SizeOfHeaders */ .long 0 /* CheckSum */
- .short EFI_SUBSYSTEM /* Subsystem */
- .short IMAGE_SUBSYSTEM_EFI_APPLICATION /* Subsystem */ .short 0 /* DllCharacteristics */ .long 0 /* SizeOfStackReserve */ .long 0 /* SizeOfStackCommit */
diff --git a/include/asm-generic/pe.h b/include/asm-generic/pe.h new file mode 100644 index 0000000000..d1683f238a --- /dev/null +++ b/include/asm-generic/pe.h @@ -0,0 +1,21 @@ +/*
- Portable Executable and Common Object Constants
- Copyright (c) 2018 Heinrich Schuchardt
- based on the "Microsoft Portable Executable and Common Object File Format
- Specification", revision 11, 2017-01-23
- SPDX-License-Identifier: GPL-2.0+
- */
+#ifndef _ASM_PE_H +#define _ASM_PE_H
+/* Subsystem type */ +#define IMAGE_SUBSYSTEM_EFI_APPLICATION 10 +#define IMAGE_SUBSYSTEM_EFI_BOOT_SERVICE_DRIVER 11 +#define IMAGE_SUBSYSTEM_EFI_RUNTIME_DRIVER 12 +#define IMAGE_SUBSYSTEM_EFI_ROM 13
+#endif /* _ASM_PE_H */ diff --git a/include/pe.h b/include/pe.h index 4ef3e92efa..c3a19cef76 100644 --- a/include/pe.h +++ b/include/pe.h @@ -11,6 +11,8 @@ #ifndef _PE_H #define _PE_H
+#include <asm-generic/pe.h>
typedef struct _IMAGE_DOS_HEADER { uint16_t e_magic; /* 00: MZ Header signature */ uint16_t e_cblp; /* 02: Bytes on last page of file */ @@ -62,12 +64,6 @@ typedef struct _IMAGE_DATA_DIRECTORY {
#define IMAGE_NUMBEROF_DIRECTORY_ENTRIES 16
-/* PE32+ Subsystem type for EFI images */ -#define IMAGE_SUBSYSTEM_EFI_APPLICATION 10 -#define IMAGE_SUBSYSTEM_EFI_BOOT_SERVICE_DRIVER 11 -#define IMAGE_SUBSYSTEM_EFI_RUNTIME_DRIVER 12 -#define IMAGE_SUBSYSTEM_SAL_RUNTIME_DRIVER 13
typedef struct _IMAGE_OPTIONAL_HEADER64 { uint16_t Magic; /* 0x20b */ uint8_t MajorLinkerVersion; diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c index 9d2214b481..cac64ba9fe 100644 --- a/lib/efi_loader/efi_image_loader.c +++ b/lib/efi_loader/efi_image_loader.c @@ -94,7 +94,7 @@ static void efi_set_code_and_data_type( loaded_image_info->image_data_type = EFI_BOOT_SERVICES_DATA; break; case IMAGE_SUBSYSTEM_EFI_RUNTIME_DRIVER:
- case IMAGE_SUBSYSTEM_SAL_RUNTIME_DRIVER:
- case IMAGE_SUBSYSTEM_EFI_ROM: loaded_image_info->image_code_type = EFI_RUNTIME_SERVICES_CODE; loaded_image_info->image_data_type = EFI_RUNTIME_SERVICES_DATA; break;
participants (3)
-
Andre Przywara -
Heinrich Schuchardt -
Vagrant Cascadian