xen.efi PE does not boot when loaded from shim or some patched
downstream grub2.

What happens is the bootloader would honour the MEM_DISCARDABLE
flag of the .reloc section meaning it would not load its content
into memory.

But Xen is parsing the .reloc section content twice at boot:
* https://elixir.bootlin.com/xen/v4.20.1/source/xen/common/efi/boot.c#L1362
* https://elixir.bootlin.com/xen/v4.20.1/source/xen/arch/x86/efi/efi-boot.h#L237

Therefore it would crash with the following message:
"Unsupported relocation type" as reported there:

* https://github.com/QubesOS/qubes-issues/issues/8206#issuecomment-2619048838
* https://lore.kernel.org/xen-devel/7e039262-1f54-46e1-8f70-ac3f03607d5a@suse.com/T/#me122b9e6c27cd98db917da2c9f67e74a2c6ad7a5

This commit adds a small C host tool named keeprelocs
that is called after xen.efi is produced by the build system
in order to remove this bit from its .reloc section header.

Signed-off-by: Yann Sionneau <yann.sionneau@vates.tech>
---
 xen/Makefile           |   5 +-
 xen/arch/x86/Makefile  |   1 +
 xen/tools/Makefile     |   3 ++
 xen/tools/keeprelocs.c | 119 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 127 insertions(+), 1 deletion(-)
 create mode 100644 xen/tools/keeprelocs.c

diff --git a/xen/Makefile b/xen/Makefile
index 8fc4e042ff..7dc9cd7e05 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -299,10 +299,13 @@ export XEN_HAS_CHECKPOLICY := $(call success,$(CHECKPOLICY) -h 2>&1 | grep -q xe
 # ===========================================================================
 # Rules shared between *config targets and build targets
 
-PHONY += tools_fixdep
+PHONY += tools_fixdep tools_keeprelocs
 tools_fixdep:
 	$(Q)$(MAKE) $(build)=tools tools/fixdep
 
+tools_keeprelocs:
+	$(Q)$(MAKE) $(build)=tools tools/keeprelocs
+
 PHONY += outputmakefile
 # Before starting out-of-tree build, make sure the source tree is clean.
 # outputmakefile generates a Makefile in the output directory, if using a
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index ce724a9daa..9a47002fae 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -236,6 +236,7 @@ endif
 	$(NM) -pa --format=sysv $@ \
 		| $(objtree)/tools/symbols --all-symbols --xensyms --sysv --sort \
 		> $@.map
+	$(objtree)/tools/keeprelocs -q -i $@
 ifeq ($(CONFIG_DEBUG_INFO),y)
 	$(if $(filter --strip-debug,$(EFI_LDFLAGS)),:$(space))$(OBJCOPY) -O elf64-x86-64 $@ $@.elf
 endif
diff --git a/xen/tools/Makefile b/xen/tools/Makefile
index a5078b7cb8..4fd917b398 100644
--- a/xen/tools/Makefile
+++ b/xen/tools/Makefile
@@ -1,2 +1,5 @@
 hostprogs-always-y += symbols
 hostprogs-always-y += fixdep
+hostprogs-always-$(XEN_BUILD_PE) += keeprelocs
+# next line is to allow including include/efi/pe.h
+HOSTCFLAGS_keeprelocs.o := -I ../include
\ No newline at end of file
diff --git a/xen/tools/keeprelocs.c b/xen/tools/keeprelocs.c
new file mode 100644
index 0000000000..c169ddba1a
--- /dev/null
+++ b/xen/tools/keeprelocs.c
@@ -0,0 +1,119 @@
+#include <stdio.h>
+#include <fcntl.h>
+#include <errno.h>
+#include <string.h>
+#include <sys/mman.h>
+#include <sys/stat.h>
+#include <stdint.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <efi/pe.h>
+
+#undef DEBUG
+
+static void print_usage(const char *name) {
+	printf("%s: [-q] [-h] -i xen.efi\n", name);
+}
+
+int main(int argc, char **argv)
+{
+	char *filename = NULL;
+	int fd;
+	char *mem;
+	struct stat st;
+	off_t len;
+	int ret;
+	struct mz_hdr *mz;
+	struct pe_hdr *pe;
+	int opt;
+	const char *prog_name = argv[0];
+	int quiet = 0;
+
+	while ((opt = getopt(argc, argv, ":i:qh")) != -1)
+	{
+		switch (opt) {
+		case 'i':
+			filename = optarg;
+			break;
+		case 'q':
+			quiet = 1;
+			break;
+		case 'h':
+			print_usage(prog_name);
+			return 0;
+			break;
+		case '?':
+		default:
+			print_usage(prog_name);
+			return -1;
+		}
+	}
+
+
+	if (!filename) {
+		printf("Error: you must provide a `-i xen.efi` argument\n");
+		return -1;
+	}
+
+	fd = open(filename, O_RDWR);
+	if (fd < 0) {
+		printf("Could not open file %s: %s\n", filename, strerror(errno));
+		return -1;
+	}
+
+	ret = fstat(fd, &st);
+	if (ret < 0) {
+		perror("Error while getting PE file length");
+		return -1;
+	}
+
+	len = st.st_size;
+	mem = mmap(NULL, len, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
+
+	if (mem == MAP_FAILED) {
+		perror("Failed to mmap PE file");
+		return -1;
+	}
+
+	mz = (struct mz_hdr *)mem;
+	if (mz->magic != MZ_MAGIC) { // "MZ"
+		printf("file has incorrect MZ header 0x%02x instead of 0x5a4d\n", mz->magic);
+		return -1;
+	}
+
+	pe = (struct pe_hdr *)(mem + mz->peaddr);
+	if (strncmp((char *)&pe->magic, "PE\0\0", 4)) {
+		printf("file has incorrect PE header magic %08x instead of 0x00004550\n", pe->magic);
+		return -1;
+	}
+
+	if (pe->opt_hdr_size == 0) {
+		printf("file has empty OptionalHeader\n");
+		return -1;
+	}
+
+	struct section_header *section = (struct section_header *)((uint8_t *)pe + sizeof(*pe) + pe->opt_hdr_size);
+	for (unsigned int section_id = 0; section_id < pe->sections; section_id++, section++)
+	{
+#ifdef DEBUG
+		printf("section %s\n", section->Name);
+#endif
+		if (strncmp(section->name, ".reloc", strlen(".reloc")))
+			continue;
+
+		if (!quiet)
+			printf(".reloc section characteristics: %08x\n", section->flags);
+		if (section->flags & IMAGE_SCN_MEM_DISCARDABLE) {
+			if (!quiet)
+				printf("MEM_DISCARDABLE flag found! Dropping it.\n");
+			section->flags &= ~(IMAGE_SCN_MEM_DISCARDABLE);
+		}
+	}
+
+	munmap(mem, len);
+	close(fd);
+
+	if (!quiet)
+		printf("Ok!\n");
+	return 0;
+}
-- 
2.43.0



Yann Sionneau | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

On Wed, Jul 23, 2025 at 01:56:33PM +0000, Yann Sionneau wrote:
> diff --git a/xen/Makefile b/xen/Makefile
> index 8fc4e042ff..7dc9cd7e05 100644
> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -299,10 +299,13 @@ export XEN_HAS_CHECKPOLICY := $(call success,$(CHECKPOLICY) -h 2>&1 | grep -q xe
>  # ===========================================================================
>  # Rules shared between *config targets and build targets
>  
> -PHONY += tools_fixdep
> +PHONY += tools_fixdep tools_keeprelocs
>  tools_fixdep:
>  	$(Q)$(MAKE) $(build)=tools tools/fixdep
>  
> +tools_keeprelocs:
> +	$(Q)$(MAKE) $(build)=tools tools/keeprelocs

You don't need this new rule, and nothing calls it. `tools_fixdep` is
special as it is used by the build system and very early one.

There's the command `$(Q)$(MAKE) $(build)=tools` which will build
this new binary for you, it is the first call in the rule `$(TARGET)`.
So beside editing xen/tools/Makefile, there's nothing else to do to have
a build of this new binary.

> diff --git a/xen/tools/Makefile b/xen/tools/Makefile
> index a5078b7cb8..4fd917b398 100644
> --- a/xen/tools/Makefile
> +++ b/xen/tools/Makefile
> @@ -1,2 +1,5 @@
>  hostprogs-always-y += symbols
>  hostprogs-always-y += fixdep
> +hostprogs-always-$(XEN_BUILD_PE) += keeprelocs
> +# next line is to allow including include/efi/pe.h
> +HOSTCFLAGS_keeprelocs.o := -I ../include

`../include` doesn't exist, and `..` might not exist or might be outside
the build directory.

In the directory "xen/" (and the subdirectories) all the commands are
executed from "xen/". Also, "xen/" directory can be built out-of-tree.
In both case, `..` isn't the directory you are looking for.

You can try out-of-tree build with:
    make -C xen O=path/to/build-tree
Or:
    cd path/to/build-tree; make -f path/to/xen.git/xen/Makefile

Anyway, for the build to work, you want:

    HOSTCFLAGS_keeprelocs.o := -I $(srctree)/include

Thanks,

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

On 23.07.2025 15:56, Yann Sionneau wrote:
> xen.efi PE does not boot when loaded from shim or some patched
> downstream grub2.
> 
> What happens is the bootloader would honour the MEM_DISCARDABLE
> flag of the .reloc section meaning it would not load its content
> into memory.
> 
> But Xen is parsing the .reloc section content twice at boot:
> * https://elixir.bootlin.com/xen/v4.20.1/source/xen/common/efi/boot.c#L1362
> * https://elixir.bootlin.com/xen/v4.20.1/source/xen/arch/x86/efi/efi-boot.h#L237
> 
> Therefore it would crash with the following message:
> "Unsupported relocation type" as reported there:
> 
> * https://github.com/QubesOS/qubes-issues/issues/8206#issuecomment-2619048838
> * https://lore.kernel.org/xen-devel/7e039262-1f54-46e1-8f70-ac3f03607d5a@suse.com/T/#me122b9e6c27cd98db917da2c9f67e74a2c6ad7a5
> 
> This commit adds a small C host tool named keeprelocs
> that is called after xen.efi is produced by the build system
> in order to remove this bit from its .reloc section header.

As indicated on Matrix, giving this tool such a specific name doesn't
lend it to use for further editing later on.

Also such an entirely new tool imo wants to use Xen style, not Linux(?)
one. Unless of course it is taken from somewhere, but nothing is being
said along these line.

> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -299,10 +299,13 @@ export XEN_HAS_CHECKPOLICY := $(call success,$(CHECKPOLICY) -h 2>&1 | grep -q xe
>  # ===========================================================================
>  # Rules shared between *config targets and build targets
>  
> -PHONY += tools_fixdep
> +PHONY += tools_fixdep tools_keeprelocs
>  tools_fixdep:
>  	$(Q)$(MAKE) $(build)=tools tools/fixdep
>  
> +tools_keeprelocs:
> +	$(Q)$(MAKE) $(build)=tools tools/keeprelocs

Hmm, recursing twice into the tools/ subdir isn't quite nice. But
perhaps tolerable for the moment.

> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -236,6 +236,7 @@ endif
>  	$(NM) -pa --format=sysv $@ \
>  		| $(objtree)/tools/symbols --all-symbols --xensyms --sysv --sort \
>  		> $@.map
> +	$(objtree)/tools/keeprelocs -q -i $@

Consider the build being interrupted precisely before this command is
executed, and the target file not being removed. A subsequent build would
find it up-to-date, when the earlier build didn't really finish.

> --- a/xen/tools/Makefile
> +++ b/xen/tools/Makefile
> @@ -1,2 +1,5 @@
>  hostprogs-always-y += symbols
>  hostprogs-always-y += fixdep
> +hostprogs-always-$(XEN_BUILD_PE) += keeprelocs
> +# next line is to allow including include/efi/pe.h
> +HOSTCFLAGS_keeprelocs.o := -I ../include
> \ No newline at end of file

Please don't omit newlines at the ends of files.

> --- /dev/null
> +++ b/xen/tools/keeprelocs.c
> @@ -0,0 +1,119 @@
> +#include <stdio.h>
> +#include <fcntl.h>
> +#include <errno.h>
> +#include <string.h>
> +#include <sys/mman.h>
> +#include <sys/stat.h>
> +#include <stdint.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <efi/pe.h>
> +
> +#undef DEBUG
> +
> +static void print_usage(const char *name) {
> +	printf("%s: [-q] [-h] -i xen.efi\n", name);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +	char *filename = NULL;
> +	int fd;
> +	char *mem;
> +	struct stat st;
> +	off_t len;
> +	int ret;
> +	struct mz_hdr *mz;
> +	struct pe_hdr *pe;
> +	int opt;
> +	const char *prog_name = argv[0];
> +	int quiet = 0;

bool?

> +	while ((opt = getopt(argc, argv, ":i:qh")) != -1)
> +	{
> +		switch (opt) {
> +		case 'i':
> +			filename = optarg;
> +			break;

Is there a particular reason why -i needs to be used to specify the file name?
Can't the file name be the first (and only) non-option argument, as is commonly
done elsewhere?

> +		case 'q':
> +			quiet = 1;
> +			break;
> +		case 'h':
> +			print_usage(prog_name);
> +			return 0;
> +			break;

"break" after "return"?

> +		case '?':

Why is this not the same as 'h'?

> +		default:
> +			print_usage(prog_name);
> +			return -1;
> +		}
> +	}
> +
> +
> +	if (!filename) {
> +		printf("Error: you must provide a `-i xen.efi` argument\n");
> +		return -1;
> +	}
> +
> +	fd = open(filename, O_RDWR);
> +	if (fd < 0) {
> +		printf("Could not open file %s: %s\n", filename, strerror(errno));
> +		return -1;
> +	}
> +
> +	ret = fstat(fd, &st);
> +	if (ret < 0) {
> +		perror("Error while getting PE file length");
> +		return -1;
> +	}
> +
> +	len = st.st_size;
> +	mem = mmap(NULL, len, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
> +
> +	if (mem == MAP_FAILED) {
> +		perror("Failed to mmap PE file");
> +		return -1;
> +	}
> +
> +	mz = (struct mz_hdr *)mem;

If mem was void * you wouldn't need a cast here.

> +	if (mz->magic != MZ_MAGIC) { // "MZ"
> +		printf("file has incorrect MZ header 0x%02x instead of 0x5a4d\n", mz->magic);
> +		return -1;
> +	}
> +
> +	pe = (struct pe_hdr *)(mem + mz->peaddr);

Nor here.

> +	if (strncmp((char *)&pe->magic, "PE\0\0", 4)) {

I don't think strncmp() can be used here, as it'll stop at the first '\0'.

> +		printf("file has incorrect PE header magic %08x instead of 0x00004550\n", pe->magic);

0x%08x

> +		return -1;
> +	}
> +
> +	if (pe->opt_hdr_size == 0) {
> +		printf("file has empty OptionalHeader\n");
> +		return -1;
> +	}

Code further down doesn't really require this check, as it looks. IOW
either this check wants dropping, or it wants to be more strict than
just checking for zero.

> +	struct section_header *section = (struct section_header *)((uint8_t *)pe + sizeof(*pe) + pe->opt_hdr_size);

"(uint8_t *)pe + sizeof(*pe)" can be easier had as "(uint8_t *)(pe + 1)".
But that won't be sufficient to get the line under 80 chars.

> +	for (unsigned int section_id = 0; section_id < pe->sections; section_id++, section++)
> +	{
> +#ifdef DEBUG
> +		printf("section %s\n", section->Name);
> +#endif

This probably is just a leftover?

> +		if (strncmp(section->name, ".reloc", strlen(".reloc")))

strncmp(..., strlen(...)) is slightly odd. In the specific case, strcmp()
will do I think. Otherwise use memcmp()?

Jan

On 7/23/25 16:18, Jan Beulich wrote:
> On 23.07.2025 15:56, Yann Sionneau wrote:
>> xen.efi PE does not boot when loaded from shim or some patched
>> downstream grub2.
>>
>> What happens is the bootloader would honour the MEM_DISCARDABLE
>> flag of the .reloc section meaning it would not load its content
>> into memory.
>>
>> But Xen is parsing the .reloc section content twice at boot:
>> * https://elixir.bootlin.com/xen/v4.20.1/source/xen/common/efi/boot.c#L1362
>> * https://elixir.bootlin.com/xen/v4.20.1/source/xen/arch/x86/efi/efi-boot.h#L237
>>
>> Therefore it would crash with the following message:
>> "Unsupported relocation type" as reported there:
>>
>> * https://github.com/QubesOS/qubes-issues/issues/8206#issuecomment-2619048838
>> * https://lore.kernel.org/xen-devel/7e039262-1f54-46e1-8f70-ac3f03607d5a@suse.com/T/#me122b9e6c27cd98db917da2c9f67e74a2c6ad7a5
>>
>> This commit adds a small C host tool named keeprelocs
>> that is called after xen.efi is produced by the build system
>> in order to remove this bit from its .reloc section header.
> 
> As indicated on Matrix, giving this tool such a specific name doesn't
> lend it to use for further editing later on.

What would you like to call it?

> 
> Also such an entirely new tool imo wants to use Xen style, not Linux(?)
> one. Unless of course it is taken from somewhere, but nothing is being
> said along these line.

Ah, sorry I didn't know about the coding style, I'll reformat it then.
Is there a correct .clang-format file somewhere or a checkpatch.pl 
equivalent?

> 
>> --- a/xen/Makefile
>> +++ b/xen/Makefile
>> @@ -299,10 +299,13 @@ export XEN_HAS_CHECKPOLICY := $(call success,$(CHECKPOLICY) -h 2>&1 | grep -q xe
>>   # ===========================================================================
>>   # Rules shared between *config targets and build targets
>>   
>> -PHONY += tools_fixdep
>> +PHONY += tools_fixdep tools_keeprelocs
>>   tools_fixdep:
>>   	$(Q)$(MAKE) $(build)=tools tools/fixdep
>>   
>> +tools_keeprelocs:
>> +	$(Q)$(MAKE) $(build)=tools tools/keeprelocs
> 
> Hmm, recursing twice into the tools/ subdir isn't quite nice. But
> perhaps tolerable for the moment.
> 
>> --- a/xen/arch/x86/Makefile
>> +++ b/xen/arch/x86/Makefile
>> @@ -236,6 +236,7 @@ endif
>>   	$(NM) -pa --format=sysv $@ \
>>   		| $(objtree)/tools/symbols --all-symbols --xensyms --sysv --sort \
>>   		> $@.map
>> +	$(objtree)/tools/keeprelocs -q -i $@
> 
> Consider the build being interrupted precisely before this command is
> executed, and the target file not being removed. A subsequent build would
> find it up-to-date, when the earlier build didn't really finish.

Indeed you're right! Thanks!
Note that currently it's already problematic: if you interrupt the build 
and relaunch it, you could end up never generating the xen.efi.map nor 
the xen.efi.elf nor run the check-endbr.sh script.
But yeah, let's do better for my patch, as it impacts the final binary 
generation.

> 
>> --- a/xen/tools/Makefile
>> +++ b/xen/tools/Makefile
>> @@ -1,2 +1,5 @@
>>   hostprogs-always-y += symbols
>>   hostprogs-always-y += fixdep
>> +hostprogs-always-$(XEN_BUILD_PE) += keeprelocs
>> +# next line is to allow including include/efi/pe.h
>> +HOSTCFLAGS_keeprelocs.o := -I ../include
>> \ No newline at end of file
> 
> Please don't omit newlines at the ends of files.

Ok
> 
>> --- /dev/null
>> +++ b/xen/tools/keeprelocs.c
>> @@ -0,0 +1,119 @@
>> +#include <stdio.h>
>> +#include <fcntl.h>
>> +#include <errno.h>
>> +#include <string.h>
>> +#include <sys/mman.h>
>> +#include <sys/stat.h>
>> +#include <stdint.h>
>> +#include <stdlib.h>
>> +#include <unistd.h>
>> +#include <efi/pe.h>
>> +
>> +#undef DEBUG
>> +
>> +static void print_usage(const char *name) {
>> +	printf("%s: [-q] [-h] -i xen.efi\n", name);
>> +}
>> +
>> +int main(int argc, char **argv)
>> +{
>> +	char *filename = NULL;
>> +	int fd;
>> +	char *mem;
>> +	struct stat st;
>> +	off_t len;
>> +	int ret;
>> +	struct mz_hdr *mz;
>> +	struct pe_hdr *pe;
>> +	int opt;
>> +	const char *prog_name = argv[0];
>> +	int quiet = 0;
> 
> bool?

Ok
I can use bool from stdbool.h
> 
>> +	while ((opt = getopt(argc, argv, ":i:qh")) != -1)
>> +	{
>> +		switch (opt) {
>> +		case 'i':
>> +			filename = optarg;
>> +			break;
> 
> Is there a particular reason why -i needs to be used to specify the file name?
> Can't the file name be the first (and only) non-option argument, as is commonly
> done elsewhere?
Ok, let's do that
> 
>> +		case 'q':
>> +			quiet = 1;
>> +			break;
>> +		case 'h':
>> +			print_usage(prog_name);
>> +			return 0;
>> +			break;
> 
> "break" after "return"?
This needs to go.
> 
>> +		case '?':
> 
> Why is this not the same as 'h'?
One returns 0 because help is asked for so it's not an error.
The other one is when using non-existing argument which is an error.
> 
>> +		default:
>> +			print_usage(prog_name);
>> +			return -1;
>> +		}
>> +	}
>> +
>> +
>> +	if (!filename) {
>> +		printf("Error: you must provide a `-i xen.efi` argument\n");
>> +		return -1;
>> +	}
>> +
>> +	fd = open(filename, O_RDWR);
>> +	if (fd < 0) {
>> +		printf("Could not open file %s: %s\n", filename, strerror(errno));
>> +		return -1;
>> +	}
>> +
>> +	ret = fstat(fd, &st);
>> +	if (ret < 0) {
>> +		perror("Error while getting PE file length");
>> +		return -1;
>> +	}
>> +
>> +	len = st.st_size;
>> +	mem = mmap(NULL, len, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
>> +
>> +	if (mem == MAP_FAILED) {
>> +		perror("Failed to mmap PE file");
>> +		return -1;
>> +	}
>> +
>> +	mz = (struct mz_hdr *)mem;
> 
> If mem was void * you wouldn't need a cast here.
Ok
> 
>> +	if (mz->magic != MZ_MAGIC) { // "MZ"
>> +		printf("file has incorrect MZ header 0x%02x instead of 0x5a4d\n", mz->magic);
>> +		return -1;
>> +	}
>> +
>> +	pe = (struct pe_hdr *)(mem + mz->peaddr);
> 
> Nor here.
Ok
> 
>> +	if (strncmp((char *)&pe->magic, "PE\0\0", 4)) {
> 
> I don't think strncmp() can be used here, as it'll stop at the first '\0'.
Ah yes, you're right.
> 
>> +		printf("file has incorrect PE header magic %08x instead of 0x00004550\n", pe->magic);
> 
> 0x%08x
Ok
> 
>> +		return -1;
>> +	}
>> +
>> +	if (pe->opt_hdr_size == 0) {
>> +		printf("file has empty OptionalHeader\n");
>> +		return -1;
>> +	}
> 
> Code further down doesn't really require this check, as it looks. IOW
> either this check wants dropping, or it wants to be more strict than
> just checking for zero.

Based on 
https://learn.microsoft.com/en-us/windows/win32/debug/pe-format#coff-file-header-object-and-image
My understanding is that SizeOfOptionalHeader member can be 0, for 
object files.
But we don't want an object file here, we want an image file.
However, the optional header is required for image files (thus the != 0 
check):

"Every image file has an optional header that provides information to 
the loader."

But, we really don't know its size, moreover it's even different for 
PE32 vs PE32+.

> 
>> +	struct section_header *section = (struct section_header *)((uint8_t *)pe + sizeof(*pe) + pe->opt_hdr_size);
> 
> "(uint8_t *)pe + sizeof(*pe)" can be easier had as "(uint8_t *)(pe + 1)".
> But that won't be sufficient to get the line under 80 chars.
Yes I can indeed use pointer arithmetics like pe + 1 to go past the size 
of the pe struct, I just thought it was more readable/obvious in the pe 
+ sizeof(*pe) form.
> 
>> +	for (unsigned int section_id = 0; section_id < pe->sections; section_id++, section++)
>> +	{
>> +#ifdef DEBUG
>> +		printf("section %s\n", section->Name);
>> +#endif
> 
> This probably is just a leftover?
It can be removed yes
> 
>> +		if (strncmp(section->name, ".reloc", strlen(".reloc")))
> 
> strncmp(..., strlen(...)) is slightly odd. In the specific case, strcmp()
> will do I think. Otherwise use memcmp()?
I was trying to protect against section->name maybe not being \0 
terminated (in a broken PE file for instance).
I can switch to using
memcmp(section->name, ".reloc", strlen(".reloc")+1)

> 
> Jan

Thanks for the review :)


Regards,
-- 


Yann Sionneau | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

On 23.07.2025 17:39, Yann Sionneau wrote:
> On 7/23/25 16:18, Jan Beulich wrote:
>> On 23.07.2025 15:56, Yann Sionneau wrote:
>>> xen.efi PE does not boot when loaded from shim or some patched
>>> downstream grub2.
>>>
>>> What happens is the bootloader would honour the MEM_DISCARDABLE
>>> flag of the .reloc section meaning it would not load its content
>>> into memory.
>>>
>>> But Xen is parsing the .reloc section content twice at boot:
>>> * https://elixir.bootlin.com/xen/v4.20.1/source/xen/common/efi/boot.c#L1362
>>> * https://elixir.bootlin.com/xen/v4.20.1/source/xen/arch/x86/efi/efi-boot.h#L237
>>>
>>> Therefore it would crash with the following message:
>>> "Unsupported relocation type" as reported there:
>>>
>>> * https://github.com/QubesOS/qubes-issues/issues/8206#issuecomment-2619048838
>>> * https://lore.kernel.org/xen-devel/7e039262-1f54-46e1-8f70-ac3f03607d5a@suse.com/T/#me122b9e6c27cd98db917da2c9f67e74a2c6ad7a5
>>>
>>> This commit adds a small C host tool named keeprelocs
>>> that is called after xen.efi is produced by the build system
>>> in order to remove this bit from its .reloc section header.
>>
>> As indicated on Matrix, giving this tool such a specific name doesn't
>> lend it to use for further editing later on.
> 
> What would you like to call it?

peedit or editpe or some such? And then of course have it have a command
line option indicating to remove the one flag from the one section.

Thinking of it, binutils having elfedit, it may be an option to actually
have peedit there, in sufficiently generalized form.

>> Also such an entirely new tool imo wants to use Xen style, not Linux(?)
>> one. Unless of course it is taken from somewhere, but nothing is being
>> said along these line.
> 
> Ah, sorry I didn't know about the coding style, I'll reformat it then.
> Is there a correct .clang-format file somewhere or a checkpatch.pl 
> equivalent?

Sadly not. All we have is ./CODING_STYLE and a lot of unwritten rules.

>>> +		case 'q':
>>> +			quiet = 1;
>>> +			break;
>>> +		case 'h':
>>> +			print_usage(prog_name);
>>> +			return 0;
>>> +			break;
>>
>> "break" after "return"?
> This needs to go.
>>
>>> +		case '?':
>>
>> Why is this not the same as 'h'?
> One returns 0 because help is asked for so it's not an error.
> The other one is when using non-existing argument which is an error.

But a user passing -? deserves to be shown help output, just like you
do for -h?

>>> +	if (pe->opt_hdr_size == 0) {
>>> +		printf("file has empty OptionalHeader\n");
>>> +		return -1;
>>> +	}
>>
>> Code further down doesn't really require this check, as it looks. IOW
>> either this check wants dropping, or it wants to be more strict than
>> just checking for zero.
> 
> Based on 
> https://learn.microsoft.com/en-us/windows/win32/debug/pe-format#coff-file-header-object-and-image
> My understanding is that SizeOfOptionalHeader member can be 0, for 
> object files.
> But we don't want an object file here, we want an image file.
> However, the optional header is required for image files (thus the != 0 
> check):
> 
> "Every image file has an optional header that provides information to 
> the loader."
> 
> But, we really don't know its size, moreover it's even different for 
> PE32 vs PE32+.

Yet surely we know 1 is still too little, for example?

Jan

On 7/23/25 17:54, Jan Beulich wrote:
> On 23.07.2025 17:39, Yann Sionneau wrote:
>> On 7/23/25 16:18, Jan Beulich wrote:
>>> On 23.07.2025 15:56, Yann Sionneau wrote:
>>>> xen.efi PE does not boot when loaded from shim or some patched
>>>> downstream grub2.
>>>>
>>>> What happens is the bootloader would honour the MEM_DISCARDABLE
>>>> flag of the .reloc section meaning it would not load its content
>>>> into memory.
>>>>
>>>> But Xen is parsing the .reloc section content twice at boot:
>>>> * https://elixir.bootlin.com/xen/v4.20.1/source/xen/common/efi/boot.c#L1362
>>>> * https://elixir.bootlin.com/xen/v4.20.1/source/xen/arch/x86/efi/efi-boot.h#L237
>>>>
>>>> Therefore it would crash with the following message:
>>>> "Unsupported relocation type" as reported there:
>>>>
>>>> * https://github.com/QubesOS/qubes-issues/issues/8206#issuecomment-2619048838
>>>> * https://lore.kernel.org/xen-devel/7e039262-1f54-46e1-8f70-ac3f03607d5a@suse.com/T/#me122b9e6c27cd98db917da2c9f67e74a2c6ad7a5
>>>>
>>>> This commit adds a small C host tool named keeprelocs
>>>> that is called after xen.efi is produced by the build system
>>>> in order to remove this bit from its .reloc section header.
>>>
>>> As indicated on Matrix, giving this tool such a specific name doesn't
>>> lend it to use for further editing later on.
>>
>> What would you like to call it?
> 
> peedit or editpe or some such? And then of course have it have a command
> line option indicating to remove the one flag from the one section.

Honestly, I don't think peedit is a nice name, also, both peedit and 
editpe already exist.

> 
> Thinking of it, binutils having elfedit, it may be an option to actually
> have peedit there, in sufficiently generalized form.

yes why not, but I don't have it in my todo list to make a generalized 
PE header editor. I think this carries me too far away from the original 
bugfix.

> 
>>> Also such an entirely new tool imo wants to use Xen style, not Linux(?)
>>> one. Unless of course it is taken from somewhere, but nothing is being
>>> said along these line.
>>
>> Ah, sorry I didn't know about the coding style, I'll reformat it then.
>> Is there a correct .clang-format file somewhere or a checkpatch.pl
>> equivalent?
> 
> Sadly not. All we have is ./CODING_STYLE and a lot of unwritten rules.
> 
>>>> +		case 'q':
>>>> +			quiet = 1;
>>>> +			break;
>>>> +		case 'h':
>>>> +			print_usage(prog_name);
>>>> +			return 0;
>>>> +			break;
>>>
>>> "break" after "return"?
>> This needs to go.
>>>
>>>> +		case '?':
>>>
>>> Why is this not the same as 'h'?
>> One returns 0 because help is asked for so it's not an error.
>> The other one is when using non-existing argument which is an error.
> 
> But a user passing -? deserves to be shown help output, just like you
> do for -h?
> 
>>>> +	if (pe->opt_hdr_size == 0) {
>>>> +		printf("file has empty OptionalHeader\n");
>>>> +		return -1;
>>>> +	}
>>>
>>> Code further down doesn't really require this check, as it looks. IOW
>>> either this check wants dropping, or it wants to be more strict than
>>> just checking for zero.
>>
>> Based on
>> https://learn.microsoft.com/en-us/windows/win32/debug/pe-format#coff-file-header-object-and-image
>> My understanding is that SizeOfOptionalHeader member can be 0, for
>> object files.
>> But we don't want an object file here, we want an image file.
>> However, the optional header is required for image files (thus the != 0
>> check):
>>
>> "Every image file has an optional header that provides information to
>> the loader."
>>
>> But, we really don't know its size, moreover it's even different for
>> PE32 vs PE32+.
> 
> Yet surely we know 1 is still too little, for example?
> 
> Jan
> 


-- 


Yann Sionneau | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

On 23/07/2025 2:56 pm, Yann Sionneau wrote:
> xen.efi PE does not boot when loaded from shim or some patched
> downstream grub2.
>
> What happens is the bootloader would honour the MEM_DISCARDABLE
> flag of the .reloc section meaning it would not load its content
> into memory.
>
> But Xen is parsing the .reloc section content twice at boot:
> * https://elixir.bootlin.com/xen/v4.20.1/source/xen/common/efi/boot.c#L1362
> * https://elixir.bootlin.com/xen/v4.20.1/source/xen/arch/x86/efi/efi-boot.h#L237
>
> Therefore it would crash with the following message:
> "Unsupported relocation type" as reported there:
>
> * https://github.com/QubesOS/qubes-issues/issues/8206#issuecomment-2619048838
> * https://lore.kernel.org/xen-devel/7e039262-1f54-46e1-8f70-ac3f03607d5a@suse.com/T/#me122b9e6c27cd98db917da2c9f67e74a2c6ad7a5
>
> This commit adds a small C host tool named keeprelocs
> that is called after xen.efi is produced by the build system
> in order to remove this bit from its .reloc section header.
>
> Signed-off-by: Yann Sionneau <yann.sionneau@vates.tech>
> ---
>  xen/Makefile           |   5 +-
>  xen/arch/x86/Makefile  |   1 +
>  xen/tools/Makefile     |   3 ++
>  xen/tools/keeprelocs.c | 119 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 127 insertions(+), 1 deletion(-)
>  create mode 100644 xen/tools/keeprelocs.c

I'm sick and tired of the hoops we have to jump through for broken
tooling.  This is now rewriting the PE+ metadata because apparently the
linker can't do it correctly.

Either fix the linker (or the way we drive it/etc), or we're doing away
with PE+ emulation entirely and writing the MZ/PE headers by hand like
literally every other kernel does.

This is a level of technical debt I consider unreasonable to take.

Also, note that your analysis points at a common code file, yet this is
an x86-specific bodge.

~Andrew

On 23.07.2025 16:13, Andrew Cooper wrote:
> On 23/07/2025 2:56 pm, Yann Sionneau wrote:
>> xen.efi PE does not boot when loaded from shim or some patched
>> downstream grub2.
>>
>> What happens is the bootloader would honour the MEM_DISCARDABLE
>> flag of the .reloc section meaning it would not load its content
>> into memory.
>>
>> But Xen is parsing the .reloc section content twice at boot:
>> * https://elixir.bootlin.com/xen/v4.20.1/source/xen/common/efi/boot.c#L1362
>> * https://elixir.bootlin.com/xen/v4.20.1/source/xen/arch/x86/efi/efi-boot.h#L237
>>
>> Therefore it would crash with the following message:
>> "Unsupported relocation type" as reported there:
>>
>> * https://github.com/QubesOS/qubes-issues/issues/8206#issuecomment-2619048838
>> * https://lore.kernel.org/xen-devel/7e039262-1f54-46e1-8f70-ac3f03607d5a@suse.com/T/#me122b9e6c27cd98db917da2c9f67e74a2c6ad7a5
>>
>> This commit adds a small C host tool named keeprelocs
>> that is called after xen.efi is produced by the build system
>> in order to remove this bit from its .reloc section header.
>>
>> Signed-off-by: Yann Sionneau <yann.sionneau@vates.tech>
>> ---
>>  xen/Makefile           |   5 +-
>>  xen/arch/x86/Makefile  |   1 +
>>  xen/tools/Makefile     |   3 ++
>>  xen/tools/keeprelocs.c | 119 +++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 127 insertions(+), 1 deletion(-)
>>  create mode 100644 xen/tools/keeprelocs.c
> 
> I'm sick and tired of the hoops we have to jump through for broken
> tooling.  This is now rewriting the PE+ metadata because apparently the
> linker can't do it correctly.

The linker is doing it correctly. It is us (and very likely just us) who
have special needs here.

> Either fix the linker (or the way we drive it/etc), or we're doing away
> with PE+ emulation entirely and writing the MZ/PE headers by hand like
> literally every other kernel does.

"Fixing" the linker was suggested, but with my binutils maintainer hat on
I'm inclined to not accept a Xen-only option into the linker.

Jan

On 23/07/2025 3:21 pm, Jan Beulich wrote:
> On 23.07.2025 16:13, Andrew Cooper wrote:
>> On 23/07/2025 2:56 pm, Yann Sionneau wrote:
>>> xen.efi PE does not boot when loaded from shim or some patched
>>> downstream grub2.
>>>
>>> What happens is the bootloader would honour the MEM_DISCARDABLE
>>> flag of the .reloc section meaning it would not load its content
>>> into memory.
>>>
>>> But Xen is parsing the .reloc section content twice at boot:
>>> * https://elixir.bootlin.com/xen/v4.20.1/source/xen/common/efi/boot.c#L1362
>>> * https://elixir.bootlin.com/xen/v4.20.1/source/xen/arch/x86/efi/efi-boot.h#L237
>>>
>>> Therefore it would crash with the following message:
>>> "Unsupported relocation type" as reported there:
>>>
>>> * https://github.com/QubesOS/qubes-issues/issues/8206#issuecomment-2619048838
>>> * https://lore.kernel.org/xen-devel/7e039262-1f54-46e1-8f70-ac3f03607d5a@suse.com/T/#me122b9e6c27cd98db917da2c9f67e74a2c6ad7a5
>>>
>>> This commit adds a small C host tool named keeprelocs
>>> that is called after xen.efi is produced by the build system
>>> in order to remove this bit from its .reloc section header.
>>>
>>> Signed-off-by: Yann Sionneau <yann.sionneau@vates.tech>
>>> ---
>>>  xen/Makefile           |   5 +-
>>>  xen/arch/x86/Makefile  |   1 +
>>>  xen/tools/Makefile     |   3 ++
>>>  xen/tools/keeprelocs.c | 119 +++++++++++++++++++++++++++++++++++++++++
>>>  4 files changed, 127 insertions(+), 1 deletion(-)
>>>  create mode 100644 xen/tools/keeprelocs.c
>> I'm sick and tired of the hoops we have to jump through for broken
>> tooling.  This is now rewriting the PE+ metadata because apparently the
>> linker can't do it correctly.
> The linker is doing it correctly. It is us (and very likely just us) who
> have special needs here.
>
>> Either fix the linker (or the way we drive it/etc), or we're doing away
>> with PE+ emulation entirely and writing the MZ/PE headers by hand like
>> literally every other kernel does.
> "Fixing" the linker was suggested, but with my binutils maintainer hat on
> I'm inclined to not accept a Xen-only option into the linker.

Either Xen is doing something wrong, and should be doing it differently,
or Xen is doing something right and the tooling is
wrong/incomplete/whatever.

As a related question, is anyone other than Xen using PE+ emulation in
anger?  Every other kernel/OS level tool I'm aware of writes the MZ/PE
header by hand, and frankly, the list of bugs we've found in PE+
emulation would strongly suggest that noone else is using it.

~Andrew

On 23.07.2025 16:38, Andrew Cooper wrote:
> On 23/07/2025 3:21 pm, Jan Beulich wrote:
>> On 23.07.2025 16:13, Andrew Cooper wrote:
>>> On 23/07/2025 2:56 pm, Yann Sionneau wrote:
>>>> xen.efi PE does not boot when loaded from shim or some patched
>>>> downstream grub2.
>>>>
>>>> What happens is the bootloader would honour the MEM_DISCARDABLE
>>>> flag of the .reloc section meaning it would not load its content
>>>> into memory.
>>>>
>>>> But Xen is parsing the .reloc section content twice at boot:
>>>> * https://elixir.bootlin.com/xen/v4.20.1/source/xen/common/efi/boot.c#L1362
>>>> * https://elixir.bootlin.com/xen/v4.20.1/source/xen/arch/x86/efi/efi-boot.h#L237
>>>>
>>>> Therefore it would crash with the following message:
>>>> "Unsupported relocation type" as reported there:
>>>>
>>>> * https://github.com/QubesOS/qubes-issues/issues/8206#issuecomment-2619048838
>>>> * https://lore.kernel.org/xen-devel/7e039262-1f54-46e1-8f70-ac3f03607d5a@suse.com/T/#me122b9e6c27cd98db917da2c9f67e74a2c6ad7a5
>>>>
>>>> This commit adds a small C host tool named keeprelocs
>>>> that is called after xen.efi is produced by the build system
>>>> in order to remove this bit from its .reloc section header.
>>>>
>>>> Signed-off-by: Yann Sionneau <yann.sionneau@vates.tech>
>>>> ---
>>>>  xen/Makefile           |   5 +-
>>>>  xen/arch/x86/Makefile  |   1 +
>>>>  xen/tools/Makefile     |   3 ++
>>>>  xen/tools/keeprelocs.c | 119 +++++++++++++++++++++++++++++++++++++++++
>>>>  4 files changed, 127 insertions(+), 1 deletion(-)
>>>>  create mode 100644 xen/tools/keeprelocs.c
>>> I'm sick and tired of the hoops we have to jump through for broken
>>> tooling.  This is now rewriting the PE+ metadata because apparently the
>>> linker can't do it correctly.
>> The linker is doing it correctly. It is us (and very likely just us) who
>> have special needs here.
>>
>>> Either fix the linker (or the way we drive it/etc), or we're doing away
>>> with PE+ emulation entirely and writing the MZ/PE headers by hand like
>>> literally every other kernel does.
>> "Fixing" the linker was suggested, but with my binutils maintainer hat on
>> I'm inclined to not accept a Xen-only option into the linker.
> 
> Either Xen is doing something wrong, and should be doing it differently,

Well, no-one else would require access to its own executable's .reloc
section. Of course we can do things differently (like duplicate .reloc
into .init.data), but why have the same data in two places?

> or Xen is doing something right and the tooling is
> wrong/incomplete/whatever.
> 
> As a related question, is anyone other than Xen using PE+ emulation in
> anger?  Every other kernel/OS level tool I'm aware of writes the MZ/PE
> header by hand, and frankly, the list of bugs we've found in PE+
> emulation would strongly suggest that noone else is using it.

Both Cygwin and MinGW definitely are using it. And in Windows it's the
PE loader which reads the .reloc section. The executables themselves
wouldn't access the section, at least not normally.

One problem is that Cygwin and MinGW are lacking proper maintainers in
binutils. Hence changes are being made and approved by people (including
me) who may overlook certain aspects. Another problem is that testsuite
coverage is far more scarce for PE than it is for ELF. And then,
specifically for us, we're further making use of an entirely untested
(upstream) code path, linking ELF objects into PE binaries.

Jan

On 23.07.2025 16:45, Jan Beulich wrote:
> On 23.07.2025 16:38, Andrew Cooper wrote:
>> On 23/07/2025 3:21 pm, Jan Beulich wrote:
>>> On 23.07.2025 16:13, Andrew Cooper wrote:
>>>> On 23/07/2025 2:56 pm, Yann Sionneau wrote:
>>>>> xen.efi PE does not boot when loaded from shim or some patched
>>>>> downstream grub2.
>>>>>
>>>>> What happens is the bootloader would honour the MEM_DISCARDABLE
>>>>> flag of the .reloc section meaning it would not load its content
>>>>> into memory.
>>>>>
>>>>> But Xen is parsing the .reloc section content twice at boot:
>>>>> * https://elixir.bootlin.com/xen/v4.20.1/source/xen/common/efi/boot.c#L1362
>>>>> * https://elixir.bootlin.com/xen/v4.20.1/source/xen/arch/x86/efi/efi-boot.h#L237
>>>>>
>>>>> Therefore it would crash with the following message:
>>>>> "Unsupported relocation type" as reported there:
>>>>>
>>>>> * https://github.com/QubesOS/qubes-issues/issues/8206#issuecomment-2619048838
>>>>> * https://lore.kernel.org/xen-devel/7e039262-1f54-46e1-8f70-ac3f03607d5a@suse.com/T/#me122b9e6c27cd98db917da2c9f67e74a2c6ad7a5
>>>>>
>>>>> This commit adds a small C host tool named keeprelocs
>>>>> that is called after xen.efi is produced by the build system
>>>>> in order to remove this bit from its .reloc section header.
>>>>>
>>>>> Signed-off-by: Yann Sionneau <yann.sionneau@vates.tech>
>>>>> ---
>>>>>  xen/Makefile           |   5 +-
>>>>>  xen/arch/x86/Makefile  |   1 +
>>>>>  xen/tools/Makefile     |   3 ++
>>>>>  xen/tools/keeprelocs.c | 119 +++++++++++++++++++++++++++++++++++++++++
>>>>>  4 files changed, 127 insertions(+), 1 deletion(-)
>>>>>  create mode 100644 xen/tools/keeprelocs.c
>>>> I'm sick and tired of the hoops we have to jump through for broken
>>>> tooling.  This is now rewriting the PE+ metadata because apparently the
>>>> linker can't do it correctly.
>>> The linker is doing it correctly. It is us (and very likely just us) who
>>> have special needs here.
>>>
>>>> Either fix the linker (or the way we drive it/etc), or we're doing away
>>>> with PE+ emulation entirely and writing the MZ/PE headers by hand like
>>>> literally every other kernel does.
>>> "Fixing" the linker was suggested, but with my binutils maintainer hat on
>>> I'm inclined to not accept a Xen-only option into the linker.
>>
>> Either Xen is doing something wrong, and should be doing it differently,
> 
> Well, no-one else would require access to its own executable's .reloc
> section. Of course we can do things differently (like duplicate .reloc
> into .init.data), but why have the same data in two places?
> 
>> or Xen is doing something right and the tooling is
>> wrong/incomplete/whatever.

One more aspect, for completeness: Xen's need to self-relocate stems from
the fact that we combine OS loader and OS into a single binary. The original
EFI concept was to have an OS loader as EFI binary, which would then load
the OS kernel binary in whatever format it likes. That way, the OS loader
can run at wherever EFI loads it, and the OS kernel can be loaded right at
its designated address.

Jan