| Message ID | 20220706210454.30096-3-dpsmith@apertussolutions.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Hyperlaunch | expand |
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@apertussolutions.com> > Reviewed-by: Christopher Clark <christopher.clark@starlab.io> > --- > 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.
On 06.07.2022 23:04, Daniel P. Smith wrote: > --- /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 Such macro expansions need parenthesizing. > + uint32_t flags; > + uint32_t headroom; > +}; Since you're not following any external spec, on top of what Julien said about the __packed attribute I'd also like to point out that in many cases here there's no need to use fixed-width types. > +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; As the example of this, with just the header files being introduced here it is not really possible to figure what these fields are to be used for and hence whether they're legitimately represented here. > + char *boot_loader_name; > + char *kextra; const? Jan
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@apertussolutions.com> >> Reviewed-by: Christopher Clark <christopher.clark@starlab.io> >> --- >> 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.
On 7/19/22 09:11, Jan Beulich wrote: > On 06.07.2022 23:04, Daniel P. Smith wrote: >> --- /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 > > Such macro expansions need parenthesizing. Ack. >> + uint32_t flags; >> + uint32_t headroom; >> +}; > > Since you're not following any external spec, on top of what Julien > said about the __packed attribute I'd also like to point out that > in many cases here there's no need to use fixed-width types. Oh, I forgot to mention that in the reply to Julien. Yes, the __packed is needed to correctly cross the 32bit to 64bit bridge from the x86 bootstrap in patch 4. >> +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; > > As the example of this, with just the header files being introduced > here it is not really possible to figure what these fields are to > be used for and hence whether they're legitimately represented here. I can add a comment to clarify these are a mirror of the multiboot flags. These were mirrored to allow the multiboot flags to be direct copied and eased the replacement locations where an mb flag is checked. >> + char *boot_loader_name; >> + char *kextra; > > const? I want to say const will not work based on usage, but I will double-check.
On 21.07.2022 16:28, Daniel P. Smith wrote: > On 7/19/22 09:11, Jan Beulich wrote: >> On 06.07.2022 23:04, Daniel P. Smith wrote: >>> --- /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 >> >> Such macro expansions need parenthesizing. > > Ack. > >>> + uint32_t flags; >>> + uint32_t headroom; >>> +}; >> >> Since you're not following any external spec, on top of what Julien >> said about the __packed attribute I'd also like to point out that >> in many cases here there's no need to use fixed-width types. > > Oh, I forgot to mention that in the reply to Julien. Yes, the __packed > is needed to correctly cross the 32bit to 64bit bridge from the x86 > bootstrap in patch 4. I'm afraid I don't follow you here. I did briefly look at patch 4 (but that really also falls in the "wants to be split" category), but I can't see why a purely internally used struct may need packing. I'd appreciate if you could expand on that. >>> +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; >> >> As the example of this, with just the header files being introduced >> here it is not really possible to figure what these fields are to >> be used for and hence whether they're legitimately represented here. > > I can add a comment to clarify these are a mirror of the multiboot > flags. These were mirrored to allow the multiboot flags to be direct > copied and eased the replacement locations where an mb flag is checked. Multiboot flags? The context here is the "xen_guest" field. Jan
On 21.07.2022 16:28, Daniel P. Smith wrote: > On 7/19/22 09:11, Jan Beulich wrote: >> On 06.07.2022 23:04, Daniel P. Smith wrote: >>> --- /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 >> >> Such macro expansions need parenthesizing. > > Ack. > >>> + uint32_t flags; >>> + uint32_t headroom; >>> +}; >> >> Since you're not following any external spec, on top of what Julien >> said about the __packed attribute I'd also like to point out that >> in many cases here there's no need to use fixed-width types. > > Oh, I forgot to mention that in the reply to Julien. Yes, the __packed > is needed to correctly cross the 32bit to 64bit bridge from the x86 > bootstrap in patch 4. I'm afraid I don't follow you here. I did briefly look at patch 4 (but that really also falls in the "wants to be split" category), but I can't see why a purely internally used struct may need packing. I'd appreciate if you could expand on that. >>> +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; >> >> As the example of this, with just the header files being introduced >> here it is not really possible to figure what these fields are to >> be used for and hence whether they're legitimately represented here. > > I can add a comment to clarify these are a mirror of the multiboot > flags. These were mirrored to allow the multiboot flags to be direct > copied and eased the replacement locations where an mb flag is checked. Multiboot flags? The context here is the "xen_guest" field. Jan
On 7/21/22 12:00, Jan Beulich wrote: > On 21.07.2022 16:28, Daniel P. Smith wrote: >> On 7/19/22 09:11, Jan Beulich wrote: >>> On 06.07.2022 23:04, Daniel P. Smith wrote: >>>> --- /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 >>> >>> Such macro expansions need parenthesizing. >> >> Ack. >> >>>> + uint32_t flags; >>>> + uint32_t headroom; >>>> +}; >>> >>> Since you're not following any external spec, on top of what Julien >>> said about the __packed attribute I'd also like to point out that >>> in many cases here there's no need to use fixed-width types. >> >> Oh, I forgot to mention that in the reply to Julien. Yes, the __packed >> is needed to correctly cross the 32bit to 64bit bridge from the x86 >> bootstrap in patch 4. > > I'm afraid I don't follow you here. I did briefly look at patch 4 (but > that really also falls in the "wants to be split" category), but I > can't see why a purely internally used struct may need packing. I'd > appreciate if you could expand on that. Originally, patch 3 and patch 4 were a single patch, and obviously was way too large. To split them, I realized I could introduce a temporary conversion function that would allow the patch to be split into a post start_xen() patch (patch 3) and a pre start_xen() patch, (patch 4). For x86, pre start_xen() consists of 3 different entry points. These being the classic/traditional/old multiboot1/2 entry, EFI entry, and PVH entry (aka Xen Guest). The latter two are all internal, 64bit, but the former is located in arch/x86/boot and is compiled as 32bit. I tried different approaches to support using a single header between these two environments. Ultimately, IMHO, the cleanest approach is what is introduced in patch 4 as it enabled the use of Xen types in the structures and maintain a single structure that need to be passed around. To do this, a 32bit specific version of the structures were defined in arch/x86/boot/boot_info32.h that is populated under 32bit mode, then they can be fixed up after getting into start_xen() and in 64bit code. To ensure no unexpected insertion of padding, I focused on ensuring everything was 32bit aligned and packed. As Julien pointed out, I messed up with the use of enum as its size is not guaranteed as the enum list grows and I forgot to consider keeping pointers 64bit aligned. Does that help? >>>> +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; >>> >>> As the example of this, with just the header files being introduced >>> here it is not really possible to figure what these fields are to >>> be used for and hence whether they're legitimately represented here. >> >> I can add a comment to clarify these are a mirror of the multiboot >> flags. These were mirrored to allow the multiboot flags to be direct >> copied and eased the replacement locations where an mb flag is checked. > > Multiboot flags? The context here is the "xen_guest" field. Apologies, I thought you were referring to all the fields and I forgot to explain xen_guest. So to clarify, flags is to carry the MB flags passed up from the MB entry point and xen_guest is meant to carry the xen_guest bool passed up from the PVH/Xen Guest entry point. v/r, dps
On 22.07.2022 18:01, Daniel P. Smith wrote: > On 7/21/22 12:00, Jan Beulich wrote: >> On 21.07.2022 16:28, Daniel P. Smith wrote: >>> On 7/19/22 09:11, Jan Beulich wrote: >>>> On 06.07.2022 23:04, Daniel P. Smith wrote: >>>>> --- /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 >>>> >>>> Such macro expansions need parenthesizing. >>> >>> Ack. >>> >>>>> + uint32_t flags; >>>>> + uint32_t headroom; >>>>> +}; >>>> >>>> Since you're not following any external spec, on top of what Julien >>>> said about the __packed attribute I'd also like to point out that >>>> in many cases here there's no need to use fixed-width types. >>> >>> Oh, I forgot to mention that in the reply to Julien. Yes, the __packed >>> is needed to correctly cross the 32bit to 64bit bridge from the x86 >>> bootstrap in patch 4. >> >> I'm afraid I don't follow you here. I did briefly look at patch 4 (but >> that really also falls in the "wants to be split" category), but I >> can't see why a purely internally used struct may need packing. I'd >> appreciate if you could expand on that. > > Originally, patch 3 and patch 4 were a single patch, and obviously was > way too large. To split them, I realized I could introduce a temporary > conversion function that would allow the patch to be split into a post > start_xen() patch (patch 3) and a pre start_xen() patch, (patch 4). For > x86, pre start_xen() consists of 3 different entry points. These being > the classic/traditional/old multiboot1/2 entry, EFI entry, and PVH entry > (aka Xen Guest). The latter two are all internal, 64bit, but the former > is located in arch/x86/boot and is compiled as 32bit. I tried different > approaches to support using a single header between these two > environments. Ultimately, IMHO, the cleanest approach is what is > introduced in patch 4 as it enabled the use of Xen types in the > structures and maintain a single structure that need to be passed > around. To do this, a 32bit specific version of the structures were > defined in arch/x86/boot/boot_info32.h that is populated under 32bit > mode, then they can be fixed up after getting into start_xen() and in > 64bit code. To ensure no unexpected insertion of padding, I focused on > ensuring everything was 32bit aligned and packed. As Julien pointed out, > I messed up with the use of enum as its size is not guaranteed as the > enum list grows and I forgot to consider keeping pointers 64bit aligned. > > Does that help? It helps as background info, yes, but I continue to be unhappy with the new uses of the __packed attribute. >>>>> +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; >>>> >>>> As the example of this, with just the header files being introduced >>>> here it is not really possible to figure what these fields are to >>>> be used for and hence whether they're legitimately represented here. >>> >>> I can add a comment to clarify these are a mirror of the multiboot >>> flags. These were mirrored to allow the multiboot flags to be direct >>> copied and eased the replacement locations where an mb flag is checked. >> >> Multiboot flags? The context here is the "xen_guest" field. > > Apologies, I thought you were referring to all the fields and I forgot > to explain xen_guest. So to clarify, flags is to carry the MB flags > passed up from the MB entry point and xen_guest is meant to carry the > xen_guest bool passed up from the PVH/Xen Guest entry point. That was my guess, but then my request stands: The fields should be added to the struct at the time they're being made use of. Jan
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 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 { + bootstring_kind kind; + struct arch_bootstring *arch; + + 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; + + struct arch_boot_info *arch; +}; + +#endif