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

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 7/15/22 15:25, Julien Grall wrote:
> 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.

As a devoted vim user, begrudged ack. ( ^_^)

>> 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...

Ack.

>> +
>> +    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.

Ack, I was too focused on 32bit alignment for x86 bootstrap entry point
when I was laying out the structure, that was short-sighted on my part.
I will go back and rework to be 64bit aligned.

>> +
>> +    struct arch_boot_info *arch;
>> +};
>> +
>> +#endif
> 
> 
> NIT: Missing emacs magics.

Ack.