[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v4 3/4] efi: Enable booting unified hypervisor/kernel/initrd images
On Mon, Sep 14, 2020 at 07:50:12AM -0400, Trammell Hudson wrote: > 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 > (`.ramdisk`), and XSM config (`.xsm`), which are included after building > 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> Thanks, just have one comment and two style nits. > diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c > index 57df89cacb..4b1fbc9643 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(const EFI_LOADED_IMAGE *image, > + char *name, struct file *file, char *options); > 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); > @@ -623,6 +625,27 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, > CHAR16 *name, > return true; > } > > +static bool __init read_section(const EFI_LOADED_IMAGE *image, > + char *const name, struct file *file, > + char *options) > +{ > + /* skip the leading "." in the section name */ > + union string name_string = { .s = name + 1 }; > + > + file->ptr = (void *)pe_find_section(image->ImageBase, image->ImageSize, > + name, &file->size); > + if ( !file->ptr ) > + return false; > + > + file->need_to_free = false; > + > + s2w(&name_string); Don't you need to check that s2w succeed, so that name_string.w is not a random pointer from stack garbage? > + handle_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; > 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); Nit: extra space between * and function name. > diff --git a/xen/common/efi/pe.c b/xen/common/efi/pe.c > new file mode 100644 > index 0000000000..2986545d53 > --- /dev/null > +++ b/xen/common/efi/pe.c > @@ -0,0 +1,137 @@ > +/* > + * 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) > +{ > + const struct DosFileHeader *dos = (const void*)image; Nit: missing space between void and *. Thanks, Roger.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |