Re: [PATCH v2 1/2] x86: Add support for building a multiboot2 PE binary

[Ross Lagerwall <ross.lagerwall@citrix.com>]

On Mon, Apr 8, 2024 at 11:25 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 28.03.2024 16:11, Ross Lagerwall wrote:
> > In addition to building xen.efi and xen.gz, build xen-mbi.exe. The
> > latter is a PE binary that can be used with a multiboot2 loader that
> > supports loading PE binaries.
>
> I have to admit I find .exe a strange extension outside of the Windows
> world. Would it be an option to have no extension at all (xen-mbi), or
> use xen.mbi?

Sure, I have no strong preference on the name. I'll change it to
xen-mbi.

>
> > Using this option allows the binary to be signed and verified by Shim.
> > This means the same xen-mbi.exe binary can then be used for BIOS boot,
> > UEFI Boot and UEFI boot with Secure Boot verification (all with the
> > convenience of GRUB2 as a bootloader).
>
> With which "UEFI boot" really means "chainloader" from grub? That isn't
> required though, is it? I.e. "UEFI boot" ought to work also without
> involving grub?

There are a few comments here related to terminology and purpose so let
me try and clarify them in one place...

These are the existing methods of booting Xen on x86 that I know of:

* UEFI firmware boots xen.efi directly. This can be used with
  Secure Boot enabled.

* UEFI GRUB2 chainloads xen.efi by calling the firmware's
  LoadImage/StartImage. This can be used with Secure Boot enabled.

* BIOS GRUB2 boots xen.gz via multiboot1 protocol.

* BIOS GRUB2 boots xen.gz via multiboot2 protocol.

* UEFI GRUB2 boots xen.gz via multiboot2 protocol. This is much the
  same as the previous ,ethod but it does use a different entry point.
  This cannot be used with Secure Boot because Shim can only verify PE
  binaries.

These methods _do not_ work:

* BIOS GRUB2 boots xen.efi via multiboot2 protocol.

* UEFI GRUB2 boots xen.efi via multiboot2 protocol (despite what is
  implied by https://xenbits.xen.org/docs/unstable/misc/efi.html).

* Shim chainloads xen.efi. AFAICT this may have worked some time in
  the past but it doesn't currently work in my testing.

* GRUB2 verifies xen.efi using Shim and then chainloads it using an
  internal PE loader. This functionality doesn't exist in upstream
  GRUB. There are some distro patches to implement this functionality
  but they did not work with Xen when I tested them (probably for the
  same reason as the previous method).

This patch series adds 2 new methods of booting Xen:

* BIOS GRUB2 boots xen-mbi via multiboot2 protocol.

* UEFI GRUB2 boots xen-mbi via multiboot2 protocol. This supports
  Secure Boot by making it possible to call back to Shim to verify
  Xen.

We want to add Secure Boot support to XenServer and to that end, the
boot methods added by this patch are preferable to the existing boot
methods for a few reasons:

* We want a similar user experience regardless of whether BIOS or UEFI
  is used.
* When using GRUB2/multiboot, the boot files (xen.gz, vmlinuz, initrd)
  can be stored outside of the ESP which is preferable since the ESP
  is less flexible and is often somewhat limited in size.
* Using GRUB2/multiboot makes it easier to edit the Xen/Dom0
  command-line while booting / recovering a host compared with the
  config files used by the direct EFI boot.
* Using GRUB2 makes it easier to choose different boot options rather
  than having to use the firmware boot menu which is often quite
  unpleasant. Yes, we could use a UEFI bootloader like rEFInd to help
  with this but that then goes back to the first point about user
  experience.
* For development it makes life easier to always have a single Xen
  binary that is used regardless of whether BIOS/UEFI is used.

The terminology used in the patch description was not particularly
precise. To clarify, "BIOS boot" means booting xen-mbi via the
multiboot2 protocol with a BIOS-booted bootloader (like GRUB2).
"(U)EFI boot" means booting xen-mbi via the multiboot2 protocol with
a UEFI bootloader (like GRUB2).

>
> > The new binary is created by modifying xen.efi:
> > * Relocations are stripped since they are not needed.
>
> Because of the changed entry point, aiui. What hasn't become clear to
> me is what difference in functionality there's going to be between
> booting this new image in "UEFI boot" mode vs using xen.efi. Clearly
> xen.efi's own (EFI-level) command line options won't be available. But
> it feels like there might be more.

Indeed, relocations are not needed because we're using the multiboot2
entry point which handles everything without the need for relocations.

Hopefully the long comment above addresses why this patch is useful.

>
> > * The image base address is set to 0 since it must necessarily be below
> >   4 GiB and the loader will relocate it anyway.
>
> While technically okay, what is the reason for this adjustment?

The multiboot2 spec generally uses 32 bit addresses for everything and
says:

"The bootloader must not load any part of the kernel, the modules, the
Multiboot2 information structure, etc. higher than 4 GiB - 1."

An image base address above 4 GiB causes trouble because multiboot2
wasn't designed for this.

>
> > * The PE entry point is set to the multiboot2 entry point rather than
> >   the normal EFI entry point. This is only relevant for BIOS boot since
> >   for EFI boot the entry point is specified via a multiboot2 tag.
>
> Hmm, I may then be confused about the terminology further up: What is
> "BIOS boot" then? So far I've taken that as the equivalent of xen.gz's
> way of being booted (i.e. grub without EFI underneath).

Hopefully the long comment above clarifies the terminology.

>
> > @@ -123,6 +124,19 @@ syms-warn-dup-$(CONFIG_ENFORCE_UNIQUE_SYMBOLS) := --error-dup
> >
> >  orphan-handling-$(call ld-option,--orphan-handling=warn) += --orphan-handling=warn
> >
> > +ifeq ($(XEN_BUILD_PE),y)
> > +$(TARGET)-mbi.exe: $(TARGET).efi $(obj)/efi/modify-mbi-exe
> > +     $(OBJCOPY) --remove-section=.reloc $< $@.tmp
> > +     $(obj)/efi/modify-mbi-exe $@.tmp
> > +     $(OBJCOPY) --set-start=0x$$($(NM) -pa $@.tmp | awk '/T start$$/{print $$1}') $@.tmp $@.tmp2
>
> I understand section removal is better done by objcopy. Changing the entry
> point could be done by the new tool, though (by passing it the designated
> address)?

Sure, I can do that if you would prefer.

>
> As to stripping .reloc - generally this needs accompanying by setting the
> "relocations stripped" flag in the COFF(?) header. Here, however, I take
> it this is not only not needed, but actually not wanted. This imo wants
> saying somewhere; the individual steps done here could do with brief
> comments anyway, imo.

Correct, it is not wanted. I can add some descriptive comments.

>
> > --- /dev/null
> > +++ b/xen/arch/x86/efi/modify-mbi-exe.c
> > @@ -0,0 +1,77 @@
> > +#include <stdio.h>
> > +#include <stdint.h>
> > +#include <unistd.h>
> > +#include <fcntl.h>
> > +
> > +struct mz_hdr {
> > +    uint16_t signature;
> > +#define MZ_SIGNATURE 0x5a4d
> > +    uint16_t last_page_size;
> > +    uint16_t page_count;
> > +    uint16_t relocation_count;
> > +    uint16_t header_paras;
> > +    uint16_t min_paras;
> > +    uint16_t max_paras;
> > +    uint16_t entry_ss;
> > +    uint16_t entry_sp;
> > +    uint16_t checksum;
> > +    uint16_t entry_ip;
> > +    uint16_t entry_cs;
> > +    uint16_t relocations;
> > +    uint16_t overlay;
> > +    uint8_t reserved[32];
> > +    uint32_t extended_header_base;
> > +};
> > +
> > +struct coff_hdr {
> > +    uint32_t signature;
> > +    uint16_t cpu;
> > +    uint16_t section_count;
> > +    int32_t timestamp;
> > +    uint32_t symbols_file_offset;
> > +    uint32_t symbol_count;
> > +    uint16_t opt_hdr_size;
> > +    uint16_t flags;
> > +};
>
> I can't spot any use of this struct.
>

Indeed, though this will actually be used if changing the entry point
is done with this tool so I'll probably keep it.

Thanks for your review,
Ross