[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 3/4] efi: Enable booting unified hypervisor/kernel/initrd images
On Mon, Sep 07, 2020 at 03:00:26PM -0400, Trammell Hudson wrote: > From: Trammell hudson <hudson@xxxxxxxx> > > This patch adds support for bundling the xen.efi hypervisor, the xen.cfg > configuration file, the Linux kernel and initrd, as well as the XSM, > and architectural specific files into a single "unified" EFI executable. > This allows an administrator to update the components independently > without requiring rebuilding xen, as well as to replace the components > in an existing image. > > The resulting EFI executable can be invoked directly from the UEFI Boot > Manager, removing the need to use a separate loader like grub as well > as removing dependencies on local filesystem access. And since it is > a single file, it can be signed and validated by UEFI Secure Boot without > requring the shim protocol. > > It is inspired by systemd-boot's unified kernel technique and borrows the > function to locate PE sections from systemd's LGPL'ed code. During EFI > boot, Xen looks at its own loaded image to locate the PE sections for > the Xen configuration (`.config`), dom0 kernel (`.kernel`), dom0 initrd > (`.initrd`), and XSM config (`.xsm`), which are included after building Could we name this kernel_payload or maybe just payload instead of initrd? That's a Linux specific concept. > xen.efi using objcopy to add named sections for each input file. > > For x86, the CPU ucode can be included in a section named `.ucode`, > which is loaded in the efi_arch_cfg_file_late() stage of the boot process. > > On ARM systems the Device Tree can be included in a section named > `.dtb`, which is loaded during the efi_arch_cfg_file_early() stage of > the boot process. > > Signed-off-by: Trammell hudson <hudson@xxxxxxxx> > --- > .gitignore | 1 + > docs/misc/efi.pandoc | 47 ++++++++++++ > xen/arch/arm/efi/efi-boot.h | 22 ++++-- > xen/arch/x86/efi/Makefile | 2 +- > xen/arch/x86/efi/efi-boot.h | 7 +- > xen/common/efi/boot.c | 72 +++++++++++++----- > xen/common/efi/efi.h | 3 + > xen/common/efi/pe.c | 141 ++++++++++++++++++++++++++++++++++++ > 8 files changed, 266 insertions(+), 29 deletions(-) > create mode 100644 xen/common/efi/pe.c > > diff --git a/.gitignore b/.gitignore > index 823f4743dc..1c79b9f90a 100644 > --- a/.gitignore > +++ b/.gitignore > @@ -296,6 +296,7 @@ xen/arch/x86/efi/mkreloc > xen/arch/*/xen.lds > xen/arch/*/asm-offsets.s > xen/arch/*/efi/boot.c > +xen/arch/*/efi/pe.c Please sort alphabetically like to rest of the entries in efi/. > xen/arch/*/efi/compat.c > xen/arch/*/efi/ebmalloc.c > xen/arch/*/efi/efi.h > diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc > index 23c1a2732d..c882b1f8f8 100644 > --- a/docs/misc/efi.pandoc > +++ b/docs/misc/efi.pandoc > @@ -116,3 +116,50 @@ Filenames must be specified relative to the location of > the EFI binary. > > Extra options to be passed to Xen can also be specified on the command line, > following a `--` separator option. > + > +## Unified Xen kernel image > + > +The "Unified" kernel image can be generated by adding additional > +sections to the Xen EFI executable with objcopy, similar to how > +[systemd-boot uses the stub to add them to the Linux > kernel](https://wiki.archlinux.org/index.php/systemd-boot#Preparing_a_unified_kernel_image) > + > +The sections for the xen configuration file, the dom0 kernel, dom0 initrd, > +XSM and CPU microcode should be added after the Xen `.pad` section, the > +ending address of which can be located with: > + > +``` > +objdump -h xen.efi \ > + | perl -ane '/\.pad/ && printf "0x%016x\n", hex($F[2]) + hex($F[3])' > +``` > + > +All the sections are optional (`.config`, `.kernel`, `.initrd`, `.xsm`, > +`.ucode` (x86) and `.dtb` (ARM)) and the order does not matter. > +The addresses do not need to be contiguous, although they should not > +be overlapping. > + > +``` > +objcopy \ > + --add-section .config=xen.cfg \ > + --change-section-vma .config=0xffff82d041000000 > + --add-section .ucode=ucode.bin \ > + --change-section-vma .ucode=0xffff82d041010000 \ > + --add-section .xsm=xsm.cfg \ > + --change-section-vma .xsm=0xffff82d041080000 \ > + --add-section .kernel=vmlinux \ > + --change-section-vma .kernel=0xffff82d041100000 \ > + --add-section .ramdisk=initrd.img \ This seems like a typo, you should use .initrd instead? > + --change-section-vma .initrd=0xffff82d042000000 \ Why do you need to adjust the VMA, is this not set to a suitable default value? How can users find a suitable VMA value? > + xen.efi \ > + xen.unified.efi > +``` > + > +The unified executable can be signed with sbsigntool to make > +it usable with UEFI secure boot: > + > +``` > +sbsign \ > + --key signing.key \ > + --cert cert.pem \ > + --output xen.signed.efi \ > + xen.unified.efi > +``` > diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h > index 6527cb0bdf..154e605fee 100644 > --- a/xen/arch/arm/efi/efi-boot.h > +++ b/xen/arch/arm/efi/efi-boot.h > @@ -375,27 +375,33 @@ static void __init noreturn > efi_arch_post_exit_boot(void) > efi_xen_start(fdt, fdt_totalsize(fdt)); > } > > -static void __init efi_arch_cfg_file_early(EFI_FILE_HANDLE dir_handle, char > *section) > +static void __init efi_arch_cfg_file_early(EFI_LOADED_IMAGE *image, > EFI_FILE_HANDLE dir_handle, char *section) image should be const. > { > union string name; > > /* > * The DTB must be processed before any other entries in the > configuration > - * file, as the DTB is updated as modules are loaded. > + * file, as the DTB is updated as modules are loaded. Prefer the one > + * stored as a PE section in a unified image, and fall back to a file > + * on disk if the section is not present. > */ > - name.s = get_value(&cfg, section, "dtb"); > - if ( name.s ) > + if ( !read_section(image, ".dtb", &dtbfile, NULL) ) > { > - split_string(name.s); > - read_file(dir_handle, s2w(&name), &dtbfile, NULL); > - efi_bs->FreePool(name.w); > + name.s = get_value(&cfg, section, "dtb"); > + if ( name.s ) > + { > + split_string(name.s); > + read_file(dir_handle, s2w(&name), &dtbfile, NULL); > + efi_bs->FreePool(name.w); > + } You seem to have used hard tabs instead of spaces. > } > + > fdt = fdt_increase_size(&dtbfile, cfg.size + EFI_PAGE_SIZE); > if ( !fdt ) > blexit(L"Unable to create new FDT"); > } > > -static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE dir_handle, char > *section) > +static void __init efi_arch_cfg_file_late(EFI_LOADED_IMAGE *image, > EFI_FILE_HANDLE dir_handle, char *section) > { > } > > diff --git a/xen/arch/x86/efi/Makefile b/xen/arch/x86/efi/Makefile > index 770438a029..e857c0f2cc 100644 > --- a/xen/arch/x86/efi/Makefile > +++ b/xen/arch/x86/efi/Makefile > @@ -8,7 +8,7 @@ cmd_objcopy_o_ihex = $(OBJCOPY) -I ihex -O binary $< $@ > > boot.init.o: buildid.o > > -EFIOBJ := boot.init.o ebmalloc.o compat.o runtime.o > +EFIOBJ := boot.init.o pe.init.o ebmalloc.o compat.o runtime.o > > $(call cc-option-add,cflags-stack-boundary,CC,-mpreferred-stack-boundary=4) > $(EFIOBJ): CFLAGS-stack-boundary := $(cflags-stack-boundary) > diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h > index 7188c9a551..ecc1bbd17d 100644 > --- a/xen/arch/x86/efi/efi-boot.h > +++ b/xen/arch/x86/efi/efi-boot.h > @@ -272,14 +272,17 @@ static void __init noreturn > efi_arch_post_exit_boot(void) > unreachable(); > } > > -static void __init efi_arch_cfg_file_early(EFI_FILE_HANDLE dir_handle, char > *section) > +static void __init efi_arch_cfg_file_early(EFI_LOADED_IMAGE *image, > EFI_FILE_HANDLE dir_handle, char *section) > { > } > > -static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE dir_handle, char > *section) > +static void __init efi_arch_cfg_file_late(EFI_LOADED_IMAGE *image, > EFI_FILE_HANDLE dir_handle, char *section) Same here, image could be const. > { > union string name; > > + if ( read_section(image, ".ucode", &ucode, NULL) ) > + return; > + > name.s = get_value(&cfg, section, "ucode"); > if ( !name.s ) > name.s = get_value(&cfg, "global", "ucode"); > diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c > index f5bdc4b1df..452b5f4362 100644 > --- a/xen/common/efi/boot.c > +++ b/xen/common/efi/boot.c > @@ -121,6 +121,8 @@ static CHAR16 *s2w(union string *str); > static char *w2s(const union string *str); > static bool read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, > struct file *file, char *options); > +static bool read_section(EFI_LOADED_IMAGE *image, > + char *name, struct file *file, char *options); Aling overflow parameters to the start parentheses please. > static size_t wstrlen(const CHAR16 * s); > static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz); > static bool match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2); > @@ -622,6 +624,26 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, > CHAR16 *name, > return true; > } > > +static bool __init read_section(EFI_LOADED_IMAGE *image, image can be const I think. > + char *const name, struct file *file, char > *optio/pe_find_sectionns) options should also be const IMO, but that would require a change to efi_arch_handle_module AFAICT. > +{ > + /* skip the leading "." in the section name */ > + union string name_string = { .s = name + 1 }; > + > + file->ptr = (void *)pe_find_section(image->ImageBase, image->ImageSize, This already returns a void *, so there's no need for the cast. > + name, &file->size); Same comment regarding alignment of overflow parameters. > + if ( !file->ptr ) > + return false; > + > + file->need_to_free = false; > + > + s2w(&name_string); > + display_file_info(name_string.w, file, options); > + efi_bs->FreePool(name_string.w); > + > + return true; > +} > + > static void __init pre_parse(const struct file *cfg) > { > char *ptr = cfg->ptr, *end = ptr + cfg->size; > @@ -1206,9 +1228,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE > *SystemTable) > /* Get the file system interface. */ > dir_handle = get_parent_handle(loaded_image, &file_name); > > - /* Read and parse the config file. */ > - if ( !cfg_file_name ) > + if ( read_section(loaded_image, ".config", &cfg, NULL) ) > + { > + PrintStr(L"Using unified config file\r\n"); > + } > + else if ( !cfg_file_name ) > { > + /* Read and parse the config file. */ > CHAR16 *tail; > > while ( (tail = point_tail(file_name)) != NULL ) > @@ -1257,29 +1283,39 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE > *SystemTable) > if ( !name.s ) > blexit(L"No Dom0 kernel image specified."); > > - efi_arch_cfg_file_early(dir_handle, section.s); > + efi_arch_cfg_file_early(loaded_image, dir_handle, section.s); > > option_str = split_string(name.s); > - read_file(dir_handle, s2w(&name), &kernel, option_str); > - efi_bs->FreePool(name.w); > - > - if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL, > - (void **)&shim_lock)) && > - (status = shim_lock->Verify(kernel.ptr, kernel.size)) != > EFI_SUCCESS ) > - PrintErrMesg(L"Dom0 kernel image could not be verified", status); > > - name.s = get_value(&cfg, section.s, "ramdisk"); > - if ( name.s ) > + if ( !read_section(loaded_image, ".kernel", &kernel, option_str) ) > { > - read_file(dir_handle, s2w(&name), &ramdisk, NULL); > + read_file(dir_handle, s2w(&name), &kernel, option_str); > efi_bs->FreePool(name.w); > + > + if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL, > + (void **)&shim_lock)) && > + (status = shim_lock->Verify(kernel.ptr, kernel.size)) != > EFI_SUCCESS ) > + PrintErrMesg(L"Dom0 kernel image could not be verified", > status); > } > > - name.s = get_value(&cfg, section.s, "xsm"); > - if ( name.s ) > + if ( !read_section(loaded_image, ".ramdisk", &ramdisk, NULL) ) In the commit message you mention .initrd, but here you seem to use .ramdisk instead as the section name for the kernel payload? > { > - read_file(dir_handle, s2w(&name), &xsm, NULL); > - efi_bs->FreePool(name.w); > + name.s = get_value(&cfg, section.s, "ramdisk"); > + if ( name.s ) > + { > + read_file(dir_handle, s2w(&name), &ramdisk, NULL); > + efi_bs->FreePool(name.w); > + } > + } > + > + if ( !read_section(loaded_image, ".xsm", &xsm, NULL) ) > + { > + name.s = get_value(&cfg, section.s, "xsm"); > + if ( name.s ) > + { > + read_file(dir_handle, s2w(&name), &xsm, NULL); > + efi_bs->FreePool(name.w); > + } > } > > /* > @@ -1315,7 +1351,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE > *SystemTable) > } > } > > - efi_arch_cfg_file_late(dir_handle, section.s); > + efi_arch_cfg_file_late(loaded_image, dir_handle, section.s); > > efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size)); > cfg.addr = 0; > diff --git a/xen/common/efi/efi.h b/xen/common/efi/efi.h > index 4845d84913..c9b741bf27 100644 > --- a/xen/common/efi/efi.h > +++ b/xen/common/efi/efi.h > @@ -47,3 +47,6 @@ const CHAR16 *wmemchr(const CHAR16 *s, CHAR16 c, UINTN n); > /* EFI boot allocator. */ > void *ebmalloc(size_t size); > void free_ebmalloc_unused_mem(void); > + > +const void * pe_find_section(const UINT8 *image_base, const size_t > image_size, > + const char *section_name, UINTN *size_out); > diff --git a/xen/common/efi/pe.c b/xen/common/efi/pe.c > new file mode 100644 > index 0000000000..097c6848a8 > --- /dev/null > +++ b/xen/common/efi/pe.c > @@ -0,0 +1,141 @@ > +/* > + * xen/common/efi/pe.c > + * > + * PE executable header parser. > + * > + * Derived from > https://github.com/systemd/systemd/blob/master/src/boot/efi/pe.c > + * commit 07d5ed536ec0a76b08229c7a80b910cb9acaf6b1 > + * > + * Copyright (C) 2015 Kay Sievers <kay@xxxxxxxx> > + * Copyright (C) 2020 Trammell Hudson <hudson@xxxxxxxx> > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU Lesser General Public License as published by > + * the Free Software Foundation; either version 2.1 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, but > + * WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + */ > + > + > +#include "efi.h" > + > +struct DosFileHeader { > + UINT8 Magic[2]; > + UINT16 LastSize; > + UINT16 nBlocks; > + UINT16 nReloc; > + UINT16 HdrSize; > + UINT16 MinAlloc; > + UINT16 MaxAlloc; > + UINT16 ss; > + UINT16 sp; > + UINT16 Checksum; > + UINT16 ip; > + UINT16 cs; > + UINT16 RelocPos; > + UINT16 nOverlay; > + UINT16 reserved[4]; > + UINT16 OEMId; > + UINT16 OEMInfo; > + UINT16 reserved2[10]; > + UINT32 ExeHeader; > +} __attribute__((packed)); > + > +#define PE_HEADER_MACHINE_ARM64 0xaa64 > +#define PE_HEADER_MACHINE_X64 0x8664 > +#define PE_HEADER_MACHINE_I386 0x014c > + > +struct PeFileHeader { > + UINT16 Machine; > + UINT16 NumberOfSections; > + UINT32 TimeDateStamp; > + UINT32 PointerToSymbolTable; > + UINT32 NumberOfSymbols; > + UINT16 SizeOfOptionalHeader; > + UINT16 Characteristics; > +} __attribute__((packed)); > + > +struct PeHeader { > + UINT8 Magic[4]; > + struct PeFileHeader FileHeader; > +} __attribute__((packed)); > + > +struct PeSectionHeader { > + UINT8 Name[8]; > + UINT32 VirtualSize; > + UINT32 VirtualAddress; > + UINT32 SizeOfRawData; > + UINT32 PointerToRawData; > + UINT32 PointerToRelocations; > + UINT32 PointerToLinenumbers; > + UINT16 NumberOfRelocations; > + UINT16 NumberOfLinenumbers; > + UINT32 Characteristics; > +} __attribute__((packed)); > + > +const void * __init pe_find_section(const CHAR8 * image, const UINTN > image_size, > + const char * section_name, UINTN * size_out) You seem to have changed this quite extensively, so the spaces between the * and the parameter name (or __init) should be removed. > +{ > + const struct DosFileHeader * dos = (const void*) image; > + const struct PeHeader * pe; > + const struct PeSectionHeader * sect; > + const UINTN name_len = strlen(section_name); > + UINTN offset = 0; > + > + if ( name_len > sizeof(sect->Name) ) > + return NULL; > + > + if ( image_size < sizeof(*dos) ) > + return NULL; > + if ( memcmp(dos->Magic, "MZ", 2) != 0 ) > + return NULL; Please join all the ifs into a single one. > + > + offset = dos->ExeHeader; > + pe = (const void *) &image[offset]; Extra space after the closing parentheses. > + > + offset += sizeof(*pe); > + if ( image_size < offset ) > + return NULL; > + > + if ( memcmp(pe->Magic, "PE\0\0", 4) != 0 ) > + return NULL; Same here, you can join the two ifs into a single one. > + > + /* PE32+ Subsystem type */ > +#if defined(__arm__) || defined (__aarch64__) > + if ( pe->FileHeader.Machine != PE_HEADER_MACHINE_ARM64 ) > + return NULL; > +#elif defined(__x86_64__) > + if ( pe->FileHeader.Machine != PE_HEADER_MACHINE_X64 ) > + return NULL; > +#else > + /* unknown architecture */ > + return NULL; > +#endif > + > + offset += pe->FileHeader.SizeOfOptionalHeader; > + > + for ( UINTN i = 0 ; i < pe->FileHeader.NumberOfSections ; i++ ) Please define i at the top of the function, and there's an extra space before the semicolon: for ( i = 0; i < ... > + { > + sect = (const void *) &image[offset]; > + if ( image_size < offset + sizeof(*sect) ) > + return NULL; > + > + if ( memcmp(sect->Name, section_name, name_len) != 0 || > + image_size < sect->VirtualSize + sect->VirtualAddress ) > + { > + offset += sizeof(*sect); > + continue; > + } > + > + if ( size_out ) > + *size_out = sect->VirtualSize; > + > + return &image[sect->VirtualAddress]; Oh, so the virtual address does indeed matter, and hence will need a bit more explanation in the document I think. Thanks, Roger.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |