* [U-Boot] [PATCH 1/1] efi_loader: fix building crt0 on arm
@ 2018-01-31 18:45 Heinrich Schuchardt
2018-01-31 19:51 ` [U-Boot] [U-Boot,1/1] " Vagrant Cascadian
2018-02-02 12:05 ` [U-Boot] [PATCH 1/1] " Andre Przywara
0 siblings, 2 replies; 3+ messages in thread
From: Heinrich Schuchardt @ 2018-01-31 18:45 UTC (permalink / raw)
To: u-boot
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;
--
2.15.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [U-Boot] [U-Boot,1/1] efi_loader: fix building crt0 on arm
2018-01-31 18:45 [U-Boot] [PATCH 1/1] efi_loader: fix building crt0 on arm Heinrich Schuchardt
@ 2018-01-31 19:51 ` Vagrant Cascadian
2018-02-02 12:05 ` [U-Boot] [PATCH 1/1] " Andre Przywara
1 sibling, 0 replies; 3+ messages in thread
From: Vagrant Cascadian @ 2018-01-31 19:51 UTC (permalink / raw)
To: u-boot
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;
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180131/6bc7b640/attachment.sig>
^ permalink raw reply [flat|nested] 3+ messages in thread
* [U-Boot] [PATCH 1/1] efi_loader: fix building crt0 on arm
2018-01-31 18:45 [U-Boot] [PATCH 1/1] efi_loader: fix building crt0 on arm Heinrich Schuchardt
2018-01-31 19:51 ` [U-Boot] [U-Boot,1/1] " Vagrant Cascadian
@ 2018-02-02 12:05 ` Andre Przywara
1 sibling, 0 replies; 3+ messages in thread
From: Andre Przywara @ 2018-02-02 12:05 UTC (permalink / raw)
To: u-boot
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;
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-02-02 12:05 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-01-31 18:45 [U-Boot] [PATCH 1/1] efi_loader: fix building crt0 on arm Heinrich Schuchardt
2018-01-31 19:51 ` [U-Boot] [U-Boot,1/1] " Vagrant Cascadian
2018-02-02 12:05 ` [U-Boot] [PATCH 1/1] " Andre Przywara
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox