Re: [PATCH v1 02/18] introduction of generalized boot info

[Julien Grall <julien@xxxxxxx>]

Hi Daniel,

On 06/07/2022 22:04, Daniel P. Smith wrote:
> The x86 and Arm architectures represent in memory the general boot information
> and boot modules differently despite having commonality. The x86
> representations are bound to the multiboot v1 structures while the Arm
> representations are a slightly generalized meta-data container for the boot
> material. The multiboot structure does not lend itself well to being expanded
> to accommodate additional metadata, both general and boot module specific. The
> Arm structures are not bound to an external specification and thus are able to
> be expanded for solutions such as dom0less.
>
> This commit introduces a set of structures patterned off the Arm structures to
> represent the boot information in a manner that captures common data. The
> structures provide an arch field to allow arch specific expansions to the
> structures. The intended goal of these new common structures is to enable
> commonality between the different architectures.  Specifically to enable
> dom0less and hyperlaunch to have a common representation of boot-time
> constructed domains.
>
> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> Reviewed-by: Christopher Clark <christopher.clark@xxxxxxxxxx>
> ---
>   xen/arch/x86/include/asm/bootinfo.h | 48 +++++++++++++++++++++++++
>   xen/include/xen/bootinfo.h          | 54 +++++++++++++++++++++++++++++
>   2 files changed, 102 insertions(+)
>   create mode 100644 xen/arch/x86/include/asm/bootinfo.h
>   create mode 100644 xen/include/xen/bootinfo.h
>
> diff --git a/xen/arch/x86/include/asm/bootinfo.h 
> b/xen/arch/x86/include/asm/bootinfo.h
> new file mode 100644
> index 0000000000..b0754a3ed0
> --- /dev/null
> +++ b/xen/arch/x86/include/asm/bootinfo.h
> @@ -0,0 +1,48 @@
> +#ifndef __ARCH_X86_BOOTINFO_H__
> +#define __ARCH_X86_BOOTINFO_H__
> +
> +/* unused for x86 */
> +struct arch_bootstring { };
> +
> +struct __packed arch_bootmodule {
> +#define BOOTMOD_FLAG_X86_RELOCATED      1U << 0
> +    uint32_t flags;
> +    uint32_t headroom;
> +};
> +
> +struct __packed arch_boot_info {
> +    uint32_t flags;
> +#define BOOTINFO_FLAG_X86_MEMLIMITS    1U << 0
> +#define BOOTINFO_FLAG_X86_BOOTDEV      1U << 1
> +#define BOOTINFO_FLAG_X86_CMDLINE      1U << 2
> +#define BOOTINFO_FLAG_X86_MODULES      1U << 3
> +#define BOOTINFO_FLAG_X86_AOUT_SYMS    1U << 4
> +#define BOOTINFO_FLAG_X86_ELF_SYMS     1U << 5
> +#define BOOTINFO_FLAG_X86_MEMMAP       1U << 6
> +#define BOOTINFO_FLAG_X86_DRIVES       1U << 7
> +#define BOOTINFO_FLAG_X86_BIOSCONFIG   1U << 8
> +#define BOOTINFO_FLAG_X86_LOADERNAME   1U << 9
> +#define BOOTINFO_FLAG_X86_APM          1U << 10
> +
> +    bool xen_guest;
> +
> +    char *boot_loader_name;
> +    char *kextra;
> +
> +    uint32_t mem_lower;
> +    uint32_t mem_upper;
> +
> +    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
NIT: Missing emacs magics.

> diff --git a/xen/include/xen/bootinfo.h b/xen/include/xen/bootinfo.h
> new file mode 100644
> index 0000000000..42b53a3ca6
> --- /dev/null
> +++ b/xen/include/xen/bootinfo.h
> @@ -0,0 +1,54 @@
> +#ifndef __XEN_BOOTINFO_H__
> +#define __XEN_BOOTINFO_H__
> +
> +#include <xen/mm.h>
> +#include <xen/types.h>
> +
> +#include <asm/bootinfo.h>
> +
> +typedef enum {
> +    BOOTMOD_UNKNOWN,
> +    BOOTMOD_XEN,
> +    BOOTMOD_FDT,
> +    BOOTMOD_KERNEL,
> +    BOOTMOD_RAMDISK,
> +    BOOTMOD_XSM,
> +    BOOTMOD_UCODE,
> +    BOOTMOD_GUEST_DTB,
> +}  bootmodule_kind;
> +
> +typedef enum {
> +    BOOTSTR_EMPTY,
> +    BOOTSTR_STRING,
> +    BOOTSTR_CMDLINE,
> +} bootstring_kind;
> +
> +#define BOOTMOD_MAX_STRING 1024
> +struct __packed boot_string {
As you use __packed, the fields...

> +    bootstring_kind kind;
> +    struct arch_bootstring *arch;
... may not be naturally aligned anymore. Here it will depend on the 
size of bootstring_kind (this is an enum and it don't think C guarantees 
the size). This...
> +
> +    char bytes[BOOTMOD_MAX_STRING];
> +    size_t len;
> +};
> +
> +struct __packed boot_module {
> +    bootmodule_kind kind;
> +    paddr_t start;
> +    mfn_t mfn;
> +    size_t size;
> +
> +    struct arch_bootmodule *arch;
> +    struct boot_string string;
> +};
> +
> +struct __packed boot_info {
> +    char *cmdline;
> +
> +    uint32_t nr_mods;
> +    struct boot_module *mods;
... more obvious on this one because on 64-bit arch, there will be no 
32-bit padding. So 'mods' will be 32-bit aligned even if the value 64-bit.
This is going to be a problem on any architecture that forbid unaligned 
access (or let the software decide).
In this case, I don't think any structures you defined warrant to be 
__packed.
> +
> +    struct arch_boot_info *arch;
> +};
> +
> +#endif

NIT: Missing emacs magics.

--
Julien Grall