Xen project Mailing List

On Sat, Jul 8, 2023 at 12:15 PM Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:

On Sat, 1 Jul 2023, Christopher Clark wrote:
> Adjust the PV and PVH dom0 construction entry points to take boot module
> structures as parameters, and add further fields to the boot module
> structures to plumb the data needed to support this use. Populate these
> from the multiboot module data.
>
> This change removes multiboot from the PV and PVH dom0 construction logic.
>
> Introduce and use new inline accessor functions for navigating the boot
> module structures.
>
> The per-boot-module arrays are expanded from singletons to accommodate
> all modules, up to a static maximum of 64 modules including Xen that can
> be accepted from a bootloader to match the previous value from the
> module map check.
>
> The field that identifies the type of a boot module (kernel, ramdisk,
> etc) is introduced to the common boot module structure and declared as a
> non-enum integer type to allow the field to be of a known-size and so
> structure can be packed in a subsequent patch in the series, and it will
> then be reconciled with the equivalent Arm boot field type.
>
> The command line provided by multiboot for each boot module is added
> directly to the boot_module structure, which is appropriate for this
> logic just replacing multiboot.
>
> The maximum number of boot modules that a bootloader can provide in
> addition to the Xen hypervisor is preserved from prior logic with the
> module_map at 63.
>
> Signed-off-by: Christopher Clark <christopher.w.clark@xxxxxxxxx>
> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
>
> ---
> Changes since v1: patch is a subset of v1 series patches 2 and 3.
> - The module_map is kept for now since still in use.
> - Move the static inline functions into a separate dedicated header.
> - <mm-frame.h> and <compiler.h> replace prior inclusion of <mm.h>
> for simpler dependencies.
>
> xen/arch/x86/dom0_build.c | 10 +-
> xen/arch/x86/hvm/dom0_build.c | 43 +++---
> xen/arch/x86/include/asm/boot.h | 36 +++++
> xen/arch/x86/include/asm/bootinfo.h | 24 +++
> xen/arch/x86/include/asm/dom0_build.h | 13 +-
> xen/arch/x86/include/asm/setup.h | 4 +-
> xen/arch/x86/pv/dom0_build.c | 32 ++--
> xen/arch/x86/setup.c | 206 +++++++++++++++-----------
> xen/include/xen/bootinfo.h | 27 ++++
> 9 files changed, 254 insertions(+), 141 deletions(-)
>
> diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
> index 9f5300a3ef..42310202a2 100644
> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -4,6 +4,7 @@
> * Copyright (c) 2002-2005, K A Fraser
> */
>
> +#include <xen/bootinfo.h>
> #include <xen/init.h>
> #include <xen/iocap.h>
> #include <xen/libelf.h>
> @@ -562,9 +563,8 @@ int __init dom0_setup_permissions(struct domain *d)
> return rc;
> }
>
> -int __init construct_dom0(struct domain *d, const module_t *image,
> - unsigned long image_headroom, module_t *initrd,
> - char *cmdline)
> +int __init construct_dom0(struct domain *d, const struct boot_module *image,
> + struct boot_module *initrd, char *cmdline)
> {
> int rc;
>
> @@ -576,9 +576,9 @@ int __init construct_dom0(struct domain *d, const module_t *image,
> process_pending_softirqs();
>
> if ( is_hvm_domain(d) )
> - rc = dom0_construct_pvh(d, image, image_headroom, initrd, cmdline);
> + rc = dom0_construct_pvh(d, image, initrd, cmdline);
> else if ( is_pv_domain(d) )
> - rc = dom0_construct_pv(d, image, image_headroom, initrd, cmdline);
> + rc = dom0_construct_pv(d, image, initrd, cmdline);
> else
> panic("Cannot construct Dom0. No guest interface available\n");
>
> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
> index 56fe89632b..c094863bb8 100644
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -8,9 +8,9 @@
> */
>
> #include <xen/acpi.h>
> +#include <xen/bootinfo.h>
> #include <xen/init.h>
> #include <xen/libelf.h>
> -#include <xen/multiboot.h>
> #include <xen/pci.h>
> #include <xen/softirq.h>
>
> @@ -530,14 +530,13 @@ static paddr_t __init find_memory(
> return INVALID_PADDR;
> }
>
> -static int __init pvh_load_kernel(struct domain *d, const module_t *image,
> - unsigned long image_headroom,
> - module_t *initrd, void *image_base,
> - char *cmdline, paddr_t *entry,
> - paddr_t *start_info_addr)
> +static int __init pvh_load_kernel(
> + struct domain *d, const struct boot_module *image,
> + struct boot_module *initrd, void *image_base, char *cmdline, paddr_t *entry,
> + paddr_t *start_info_addr)
> {
> - void *image_start = image_base + image_headroom;
> - unsigned long image_len = image->mod_end;
> + void *image_start = image_base + image->arch->headroom;
> + unsigned long image_len = image->size;
> struct elf_binary elf;
> struct elf_dom_parms parms;
> paddr_t last_addr;
> @@ -546,7 +545,7 @@ static int __init pvh_load_kernel(struct domain *d, const module_t *image,
> struct vcpu *v = d->vcpu[0];
> int rc;
>
> - if ( (rc = bzimage_parse(image_base, &image_start, image_headroom,
> + if ( (rc = bzimage_parse(image_base, &image_start, image->arch->headroom,
> &image_len)) != 0 )
> {
> printk("Error trying to detect bz compressed kernel\n");
> @@ -594,7 +593,7 @@ static int __init pvh_load_kernel(struct domain *d, const module_t *image,
> * simplify it.
> */
> last_addr = find_memory(d, &elf, sizeof(start_info) +
> - (initrd ? ROUNDUP(initrd->mod_end, PAGE_SIZE) +
> + (initrd ? ROUNDUP(initrd->size, PAGE_SIZE) +
> sizeof(mod)
> : 0) +
> (cmdline ? ROUNDUP(strlen(cmdline) + 1,
> @@ -608,8 +607,8 @@ static int __init pvh_load_kernel(struct domain *d, const module_t *image,
>
> if ( initrd != NULL )
> {
> - rc = hvm_copy_to_guest_phys(last_addr, mfn_to_virt(initrd->mod_start),
> - initrd->mod_end, v);
> + rc = hvm_copy_to_guest_phys(last_addr, maddr_to_virt(initrd->start),
> + initrd->size, v);
> if ( rc )
> {
> printk("Unable to copy initrd to guest\n");
> @@ -617,11 +616,11 @@ static int __init pvh_load_kernel(struct domain *d, const module_t *image,
> }
>
> mod.paddr = last_addr;
> - mod.size = initrd->mod_end;
> - last_addr += ROUNDUP(initrd->mod_end, elf_64bit(&elf) ? 8 : 4);
> - if ( initrd->string )
> + mod.size = initrd->size;
> + last_addr += ROUNDUP(initrd->size, elf_64bit(&elf) ? 8 : 4);
> + if ( initrd->string.len )
> {
> - char *str = __va(initrd->string);
> + char *str = initrd->string.bytes;
> size_t len = strlen(str) + 1;
>
> rc = hvm_copy_to_guest_phys(last_addr, str, len, v);
> @@ -1176,10 +1175,9 @@ static void __hwdom_init pvh_setup_mmcfg(struct domain *d)
> }
> }
>
> -int __init dom0_construct_pvh(struct domain *d, const module_t *image,
> - unsigned long image_headroom,
> - module_t *initrd,
> - char *cmdline)
> +int __init dom0_construct_pvh(
> + struct domain *d, const struct boot_module *image,
> + struct boot_module *initrd, char *cmdline)
> {
> paddr_t entry, start_info;
> int rc;
> @@ -1209,9 +1207,8 @@ int __init dom0_construct_pvh(struct domain *d, const module_t *image,
> return rc;
> }
>
> - rc = pvh_load_kernel(d, image, image_headroom, initrd,
> - bootstrap_map_multiboot(image),
> - cmdline, &entry, &start_info);
> + rc = pvh_load_kernel(d, image, initrd, bootstrap_map(image), cmdline,
> + &entry, &start_info);
> if ( rc )
> {
> printk("Failed to load Dom0 kernel\n");
> diff --git a/xen/arch/x86/include/asm/boot.h b/xen/arch/x86/include/asm/boot.h
> index 10b17f12b2..bcf4f2e2e3 100644
> --- a/xen/arch/x86/include/asm/boot.h
> +++ b/xen/arch/x86/include/asm/boot.h
> @@ -19,6 +19,42 @@ static inline void *bootstrap_map_multiboot(const module_t *mod)
> return bootstrap_map(&bm);
> }
>
> +static inline unsigned long bootmodule_index(
> + const struct boot_info *info, bootmod_type_t bootmod_type,
> + unsigned long start)
> +{
> + for ( ; start < info->nr_mods; start++ )
> + if ( info->mods[start].bootmod_type == bootmod_type )
> + return start;
> +
> + return info->nr_mods + 1;
> +}
> +
> +static inline struct boot_module *bootmodule_next(
> + const struct boot_info *info, bootmod_type_t bootmod_type)
> +{
> + unsigned long i;
> +
> + for ( i = 0; i < info->nr_mods; i++ )
> + if ( info->mods[i].bootmod_type == bootmod_type )
> + return &info->mods[i];
> +
> + return NULL;
> +}
> +
> +static inline void bootmodule_update_start(struct boot_module *bm,
> + paddr_t new_start)
> +{
> + bm->start = new_start;
> + bm->mfn = maddr_to_mfn(new_start);
> +}
> +
> +static inline void bootmodule_update_mfn(struct boot_module *bm, mfn_t new_mfn)
> +{
> + bm->mfn = new_mfn;
> + bm->start = mfn_to_maddr(new_mfn);
> +}
> +
> #endif
>
> /*
> diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
> index a25054f372..30c27980e0 100644
> --- a/xen/arch/x86/include/asm/bootinfo.h
> +++ b/xen/arch/x86/include/asm/bootinfo.h
> @@ -2,9 +2,33 @@
> #define __ARCH_X86_BOOTINFO_H__
>
> struct arch_bootmodule {
> +#define BOOTMOD_FLAG_X86_RELOCATED 1U << 0
> + uint32_t flags;
> unsigned headroom;
> };
>
> +struct arch_boot_info {
> + uint32_t flags;
> +#define BOOTINFO_FLAG_X86_CMDLINE 1U << 2

Is this to indicate the presence of the Xen cmdline?

Yes; it replaces the previous use of the MBI_CMDLINE flag and is set whenever MBI_CMDLINE is set when passed from multiboot. This indicates that a command line has been passed via multiboot.

A comment in the conversion function confirms the intended use of the flag field:
/* The BOOTINFO_FLAG_X86_* flags are a 1-1 map to MBI_* */

> +#define BOOTINFO_FLAG_X86_MODULES 1U << 3
> +#define BOOTINFO_FLAG_X86_MEMMAP 1U << 6
> +#define BOOTINFO_FLAG_X86_LOADERNAME 1U << 9
> +
> + char *boot_loader_name;
> +
> + uint32_t mmap_length;
> + paddr_t mmap_addr;
> +};
> +
> +struct __packed mb_memmap {
> + uint32_t size;
> + uint32_t base_addr_low;
> + uint32_t base_addr_high;
> + uint32_t length_low;
> + uint32_t length_high;
> + uint32_t type;
> +};
> +
> #endif
>
> /*
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 3b623a4149..f9b04daebd 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -37,6 +37,7 @@
> #include <asm/processor.h>
> #include <asm/mpspec.h>
> #include <asm/apic.h>
> +#include <asm/boot.h>
> #include <asm/msi.h>
> #include <asm/desc.h>
> #include <asm/paging.h>
> @@ -59,6 +60,9 @@
> #include <asm/prot-key.h>
> #include <asm/pv/domain.h>
>
> +/* Max number of boot modules a bootloader can provide in addition to Xen */
> +#define MAX_NR_BOOTMODS 63

Call it MAX_MODULES ?
Like I wrote in the past, you already did the hard work of aligning the
interfaces, we might as well also use the same names.

On the general naming: BOOTMODS is more descriptive in that it indicates an association between the module and the context it is from: ie. boot, and is a module handed to the hypervisor by a bootloader (as opposed to say, a late-loaded module). The term BOOTMOD is also already used within the Arm source, see: BOOTMOD_MAX_CMDLINE.

On using the same name: following the feedback in the previous round of reviews this value doesn't include a count of the hypervisor itself; so it isn't the same thing and so I wouldn't use the same name for it.

[...]

> @@ -1357,12 +1382,14 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> * respective reserve_e820_ram() invocation below. No need to
> * query efi_boot_mem_unused() here, though.
> */
> - mod[boot_info->nr_mods].mod_start = virt_to_mfn(_stext);
> - mod[boot_info->nr_mods].mod_end = __2M_rwdata_end - _stext;
> + bootmodule_update_start(&boot_info->mods[boot_info->nr_mods],
> + virt_to_maddr(_stext));
> + boot_info->mods[boot_info->nr_mods].size = __2M_rwdata_end - _stext;
> }

The original code had the end address as "__2M_rwdata_end - _stext"
while now we have the size as "__2M_rwdata_end - _stext" which is not
the same?

(this was answered by Jan in a previous reply)

> boot_info->mods[0].arch->headroom =
> - bzimage_headroom(bootstrap_map_multiboot(mod), mod->mod_end);
> + bzimage_headroom(bootstrap_map(&boot_info->mods[0]),
> + boot_info->mods[0].size);
>
> bootstrap_map(NULL);
>

[...]

> diff --git a/xen/include/xen/bootinfo.h b/xen/include/xen/bootinfo.h
> index eb93cc3439..2f4284a91f 100644
> --- a/xen/include/xen/bootinfo.h
> +++ b/xen/include/xen/bootinfo.h
> @@ -2,23 +2,50 @@
> #define __XEN_BOOTINFO_H__
>
> #include <xen/types.h>
> +#include <xen/compiler.h>
> +#include <xen/mm-frame.h>
>
> #ifdef CONFIG_X86
> #include <asm/bootinfo.h>
> #else
> struct arch_bootmodule { };
> + struct arch_boot_info { };
> #endif
>
> +/* Boot module binary type / purpose */
> +#define BOOTMOD_UNKNOWN 0
> +#define BOOTMOD_XEN 1
> +#define BOOTMOD_FDT 2
> +#define BOOTMOD_KERNEL 3
> +#define BOOTMOD_RAMDISK 4
> +#define BOOTMOD_XSM 5
> +#define BOOTMOD_UCODE 6
> +#define BOOTMOD_GUEST_DTB 7
> +typedef unsigned int bootmod_type_t;
> +
> +#define BOOTMOD_STRING_MAX_LEN 1024

BOOTMOD_MAX_CMDLINE ?

The string associated with a boot module may not be a command line; it depends on the type of the module, hence BOOTMOD_MAX_STRING was what it was called in v1 of the hyperlaunch series, and I revised it to this since I think it's clearer.
ie. There's a purpose to the rename.

> +struct boot_string {

struct bootcmdline ?

It is similar to bootcmdline in that it stores the contents of the string provided by the bootloader to associate with a boot module, but the struct contents differ and the way that it is accessed is different too.

I think it replaces bootcmdline once the new structures are fully in use on Arm.

> + char bytes[BOOTMOD_STRING_MAX_LEN];

cmdline?

It may not be command line, depending on the module type, hence the interest in using a different term for it.

If the string is \0 terminated we don't need len?

This is for generalized strings associated with boot modules supplied by the bootloader, not just command lines, so additional consideration may be wanted. This is a defensive mechanism, attackers don't follow specifications and sometimes people cause bugs.

> + size_t len;
> +};
> +
> struct boot_module {
> + bootmod_type_t bootmod_type;

Why not use a good old enum?

The early x86 boot logic that runs in 32-bit mode populates the structures which are then accessed in the main hypervisor initialization logic in 64-bit mode, and we would like to have a single common definition for the structures in a header that is useable in both places, so that requires preparing for fixed-size types in packed structures; an enum isn't guaranteed to compile to
the same size in those two cases, so doesn't pack. To make transition for the Arm code easier I can make these definitions closer to the old enum though.

> paddr_t start;
> + mfn_t mfn;

I think mfn should be in arch_bootmodule

I think that's ok if it's not needed in non-x86 logic.

> size_t size;
>
> struct arch_bootmodule *arch;
> + struct boot_string string;
> };
>
> struct boot_info {
> + char *cmdline;

Is this for Xen cmdline?

Yes

While all the other cmdline are in the various
struct boot_string? Is there any benefit in using the BOOTMOD_XEN for it?
BOOTMOD_XEN is not used so far, so if you don't end up using it,
probably not, otherwise it could be considered.

ok - I agree that we haven't dropped any use of BOOTMOD_XEN so far but will keep it in mind.

thanks,

Christopher

> unsigned int nr_mods;
> struct boot_module *mods;
> +
> + struct arch_boot_info *arch;
> };
>
> #endif
> --
> 2.25.1
>
>

Re: [PATCH 04/10] x86 setup: porting dom0 construction logic to boot module structures