| Message ID | 20240830214730.1621-2-dpsmith@apertussolutions.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Boot modules for Hyperlaunch | expand |
I haven't read the entire series yet, but here's my .02 so far On Fri Aug 30, 2024 at 10:46 PM BST, Daniel P. Smith wrote: > From: Christopher Clark <christopher.w.clark@gmail.com> > > An initial step towards a non-multiboot internal representation of boot > modules for common code, starting with x86 setup and converting the fields > that are accessed for the startup calculations. > > Introduce a new header, <xen/asm/bootinfo.h>, and populate it with a new > boot_info structure initially containing a count of the number of boot > modules. > > No functional change intended. > > Signed-off-by: Christopher Clark <christopher.w.clark@gmail.com> > Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> > --- > xen/arch/x86/include/asm/bootinfo.h | 25 +++++++++++++ > xen/arch/x86/setup.c | 58 +++++++++++++++++------------ > 2 files changed, 59 insertions(+), 24 deletions(-) > create mode 100644 xen/arch/x86/include/asm/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 000000000000..e850f80d26a7 > --- /dev/null > +++ b/xen/arch/x86/include/asm/bootinfo.h > @@ -0,0 +1,25 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * Copyright (c) 2024 Christopher Clark <christopher.w.clark@gmail.com> > + * Copyright (c) 2024 Apertus Solutions, LLC > + * Author: Daniel P. Smith <dpsmith@apertussolutions.com> > + */ > + > +#ifndef __XEN_X86_BOOTINFO_H__ > +#define __XEN_X86_BOOTINFO_H__ > + This struct would benefit from a comment stating what it's for and how it's meant to be used. At a glance it seems like it's meant to be serve as a boot-protocol agnostic representation of boot-parameters, used as a generic means of information handover. Which would imply multiboot_info is parsed onto it when booting from multiboot and is synthesised from scratch in other cases (e.g: direct EFI?). > +struct boot_info { > + unsigned int nr_mods; It's imo better to treat this as an ABI. That would allow using this layer as a boot protocol in itself (which I'm guessing is the objective? I haven't gotten that far in the series). If so, this would need to be a fixed-width uintN_t. Same with other fields in follow-up patches. > +}; > + > +#endif > + > +/* > + * Local variables: > + * mode: C > + * c-file-style: "BSD" > + * c-basic-offset: 4 > + * tab-width: 4 > + * indent-tabs-mode: nil > + * End: > + */ > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c > index eee20bb1753c..dd94ee2e736b 100644 > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -32,6 +32,7 @@ > #include <compat/xen.h> > #endif > #include <xen/bitops.h> > +#include <asm/bootinfo.h> > #include <asm/smp.h> > #include <asm/processor.h> > #include <asm/mpspec.h> > @@ -276,7 +277,16 @@ static int __init cf_check parse_acpi_param(const char *s) > custom_param("acpi", parse_acpi_param); > > static const module_t *__initdata initial_images; > -static unsigned int __initdata nr_initial_images; > +static struct boot_info __initdata *boot_info; > + > +static void __init multiboot_to_bootinfo(multiboot_info_t *mbi) If this function returned boot_info instead and the caller made the assignment then it would be possible to unit-test/fuzz it. It also fits a bit more nicely with the usual implications of that function name pattern, I think. > +{ > + static struct boot_info __initdata info; > + > + info.nr_mods = mbi->mods_count; Shouldn't this be gated on MBI_MODULES being set? info.nr_mods = (mbi->flags & MBI_MODULES) ? mbi->mods_count : 0; > + > + boot_info = &info; > +} > > unsigned long __init initial_images_nrpages(nodeid_t node) > { > @@ -285,7 +295,7 @@ unsigned long __init initial_images_nrpages(nodeid_t node) > unsigned long nr; > unsigned int i; > > - for ( nr = i = 0; i < nr_initial_images; ++i ) > + for ( nr = i = 0; i < boot_info->nr_mods; ++i ) > { > unsigned long start = initial_images[i].mod_start; > unsigned long end = start + PFN_UP(initial_images[i].mod_end); > @@ -301,7 +311,7 @@ void __init discard_initial_images(void) > { > unsigned int i; > > - for ( i = 0; i < nr_initial_images; ++i ) > + for ( i = 0; i < boot_info->nr_mods; ++i ) > { > uint64_t start = (uint64_t)initial_images[i].mod_start << PAGE_SHIFT; > > @@ -309,7 +319,7 @@ void __init discard_initial_images(void) > start + PAGE_ALIGN(initial_images[i].mod_end)); > } > > - nr_initial_images = 0; > + boot_info->nr_mods = 0; Out of curiosity, why is this required? > initial_images = NULL; > } > > @@ -1034,9 +1044,10 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) > mod = __va(mbi->mods_addr); > } > > + multiboot_to_bootinfo(mbi); > + > loader = (mbi->flags & MBI_LOADERNAME) ? __va(mbi->boot_loader_name) > : "unknown"; > - Stray newline removal? > /* Parse the command-line options. */ > if ( mbi->flags & MBI_CMDLINE ) > cmdline = cmdline_cook(__va(mbi->cmdline), loader); > @@ -1141,18 +1152,18 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) > bootsym(boot_edd_info_nr)); > > /* Check that we have at least one Multiboot module. */ > - if ( !(mbi->flags & MBI_MODULES) || (mbi->mods_count == 0) ) > + if ( !(mbi->flags & MBI_MODULES) || (boot_info->nr_mods == 0) ) With MBI_MODULES accounted for during conversion, the first part of the conditional can be ellided and you could simply do: if ( !boot_info->nr_mods ) panic(...) Also, could we move this to multiboot_to_bootinfo()? It'd contain these sorts of boot argument checks to a much more self contained function and help check at the point of assignment, preventing misuse. > panic("dom0 kernel not specified. Check bootloader configuration\n"); > > /* Check that we don't have a silly number of modules. */ > - if ( mbi->mods_count > sizeof(module_map) * 8 ) > + if ( boot_info->nr_mods > sizeof(module_map) * 8 ) Like above, this check would be much more neatly contained where boot_info is created, imo. > { > - mbi->mods_count = sizeof(module_map) * 8; > + boot_info->nr_mods = sizeof(module_map) * 8; > printk("Excessive multiboot modules - using the first %u only\n", Does the comment need adjusting too to make it more general? As in s/multiboot/boot. > - mbi->mods_count); > + boot_info->nr_mods); > } > > - bitmap_fill(module_map, mbi->mods_count); > + bitmap_fill(module_map, boot_info->nr_mods); > __clear_bit(0, module_map); /* Dom0 kernel is always first */ > > if ( pvh_boot ) Cheers, Alejandro
On 30/08/2024 10:46 pm, Daniel P. Smith wrote: > From: Christopher Clark <christopher.w.clark@gmail.com> > > An initial step towards a non-multiboot internal representation of boot > modules for common code, starting with x86 setup and converting the fields > that are accessed for the startup calculations. > > Introduce a new header, <xen/asm/bootinfo.h>, and populate it with a new Just <asm/bootinfo.h>, which matches the code. > diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h > new file mode 100644 > index 000000000000..e850f80d26a7 > --- /dev/null > +++ b/xen/arch/x86/include/asm/bootinfo.h > @@ -0,0 +1,25 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * Copyright (c) 2024 Christopher Clark <christopher.w.clark@gmail.com> > + * Copyright (c) 2024 Apertus Solutions, LLC > + * Author: Daniel P. Smith <dpsmith@apertussolutions.com> > + */ > + > +#ifndef __XEN_X86_BOOTINFO_H__ > +#define __XEN_X86_BOOTINFO_H__ > + There ought to be a short description of what boot_info is, even if it's only "Xen's local representation of information provided by the bootloader/environment." > +struct boot_info { > + unsigned int nr_mods; For the sake of 3 letters, please can this be nr_modules. I've run sed over the top of the v5 branch and it doesn't change line wrapping anywhere, but it is a legibility improvement IMO. > +}; > + > +#endif #endif /* __XEN_X86_BOOTINFO_H__ */ It very quickly get to not being in the same few lines as the #ifndef. > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c > index eee20bb1753c..dd94ee2e736b 100644 > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -276,7 +277,16 @@ static int __init cf_check parse_acpi_param(const char *s) > custom_param("acpi", parse_acpi_param); > > static const module_t *__initdata initial_images; > -static unsigned int __initdata nr_initial_images; > +static struct boot_info __initdata *boot_info; > + > +static void __init multiboot_to_bootinfo(multiboot_info_t *mbi) > +{ > + static struct boot_info __initdata info; > + > + info.nr_mods = mbi->mods_count; > + > + boot_info = &info; > +} Having a global pointer set only to this private structure is weird. Even this: static struct boot_info __initdata boot_info[1]; lets you keep -> notation, but removes one level of indirection. > @@ -1034,9 +1044,10 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) > mod = __va(mbi->mods_addr); > } > > + multiboot_to_bootinfo(mbi); > + > loader = (mbi->flags & MBI_LOADERNAME) ? __va(mbi->boot_loader_name) > : "unknown"; > - Stray line removal. (should be in patch 2 to minimise churn.) ~Andrew
On 30/08/2024 10:46 pm, Daniel P. Smith wrote: > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c > index eee20bb1753c..dd94ee2e736b 100644 > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -1034,9 +1044,10 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) > mod = __va(mbi->mods_addr); > } > > + multiboot_to_bootinfo(mbi); Actually, peeking ahead to the end of the series, we've got this: void __start_xen(unsigned long mbi_p) { ... multiboot_info_t *mbi; module_t *mod; ... if ( pvh_boot ) { ASSERT(mbi_p == 0); pvh_init(&mbi, &mod); } else { mbi = __va(mbi_p); mod = __va(mbi->mods_addr); } multiboot_to_bootinfo(mbi, mod); which are the sum total of the mbi and mod pointers. Worse, pvh_init() is transforming the PVH into into MB1 info, just to be transformed immediately to BI. I expect this is work for the end of the series (I can't think of a nice way to disentangle it earlier), but could we end up with something more like: if ( pvh_boot ) { ASSERT(mbi_p == 0); pvh_fill_boot_info(); } else { multiboot_info_t *mbi = __va(mbi_p); multiboot_fill_boot_info(mbi, __va(mbi->mods_addr)); } ? Or perhaps even just pass mbi_p in, and have multiboot_fill_boot_info() do the __va()'s itself. If so, we probably want to make a naming and possibly prototype difference in this patch. ~Andrew
On 02.09.2024 15:47, Alejandro Vallejo wrote: > On Fri Aug 30, 2024 at 10:46 PM BST, Daniel P. Smith wrote: >> @@ -309,7 +319,7 @@ void __init discard_initial_images(void) >> start + PAGE_ALIGN(initial_images[i].mod_end)); >> } >> >> - nr_initial_images = 0; >> + boot_info->nr_mods = 0; > > Out of curiosity, why is this required? Together with ... >> initial_images = NULL; ... this - to prevent undue access attempts later on. IOW just to be on the safe side, aiui. Jan
On 30.08.2024 23:46, Daniel P. Smith wrote: > @@ -276,7 +277,16 @@ static int __init cf_check parse_acpi_param(const char *s) > custom_param("acpi", parse_acpi_param); > > static const module_t *__initdata initial_images; > -static unsigned int __initdata nr_initial_images; > +static struct boot_info __initdata *boot_info; > + > +static void __init multiboot_to_bootinfo(multiboot_info_t *mbi) Pointer-to-const please. Jan
On 9/2/24 09:47, Alejandro Vallejo wrote: > I haven't read the entire series yet, but here's my .02 so far > > On Fri Aug 30, 2024 at 10:46 PM BST, Daniel P. Smith wrote: >> From: Christopher Clark <christopher.w.clark@gmail.com> >> >> An initial step towards a non-multiboot internal representation of boot >> modules for common code, starting with x86 setup and converting the fields >> that are accessed for the startup calculations. >> >> Introduce a new header, <xen/asm/bootinfo.h>, and populate it with a new >> boot_info structure initially containing a count of the number of boot >> modules. >> >> No functional change intended. >> >> Signed-off-by: Christopher Clark <christopher.w.clark@gmail.com> >> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> >> --- >> xen/arch/x86/include/asm/bootinfo.h | 25 +++++++++++++ >> xen/arch/x86/setup.c | 58 +++++++++++++++++------------ >> 2 files changed, 59 insertions(+), 24 deletions(-) >> create mode 100644 xen/arch/x86/include/asm/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 000000000000..e850f80d26a7 >> --- /dev/null >> +++ b/xen/arch/x86/include/asm/bootinfo.h >> @@ -0,0 +1,25 @@ >> +/* SPDX-License-Identifier: GPL-2.0-or-later */ >> +/* >> + * Copyright (c) 2024 Christopher Clark <christopher.w.clark@gmail.com> >> + * Copyright (c) 2024 Apertus Solutions, LLC >> + * Author: Daniel P. Smith <dpsmith@apertussolutions.com> >> + */ >> + >> +#ifndef __XEN_X86_BOOTINFO_H__ >> +#define __XEN_X86_BOOTINFO_H__ >> + > > This struct would benefit from a comment stating what it's for and how it's > meant to be used. At a glance it seems like it's meant to be serve as a > boot-protocol agnostic representation of boot-parameters, used as a generic > means of information handover. Which would imply multiboot_info is parsed onto > it when booting from multiboot and is synthesised from scratch in other cases > (e.g: direct EFI?). Yes, some inline documentation can be added. >> +struct boot_info { >> + unsigned int nr_mods; > > It's imo better to treat this as an ABI. That would allow using this layer as a > boot protocol in itself (which I'm guessing is the objective? I haven't gotten > that far in the series). If so, this would need to be a fixed-width uintN_t. > > Same with other fields in follow-up patches. The intent is to provide a clean internal abstraction around the boot material provided to Xen and not as an external boot protocol. The follow-on series to come will build upon this to introduce a representation of domains to be constructed at boot. A side goal with later introduced fields is to use proper xen types for the fields, eg. paddr_t, and mfn_t. >> +}; >> + >> +#endif >> + >> +/* >> + * Local variables: >> + * mode: C >> + * c-file-style: "BSD" >> + * c-basic-offset: 4 >> + * tab-width: 4 >> + * indent-tabs-mode: nil >> + * End: >> + */ >> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c >> index eee20bb1753c..dd94ee2e736b 100644 >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -32,6 +32,7 @@ >> #include <compat/xen.h> >> #endif >> #include <xen/bitops.h> >> +#include <asm/bootinfo.h> >> #include <asm/smp.h> >> #include <asm/processor.h> >> #include <asm/mpspec.h> >> @@ -276,7 +277,16 @@ static int __init cf_check parse_acpi_param(const char *s) >> custom_param("acpi", parse_acpi_param); >> >> static const module_t *__initdata initial_images; >> -static unsigned int __initdata nr_initial_images; >> +static struct boot_info __initdata *boot_info; >> + >> +static void __init multiboot_to_bootinfo(multiboot_info_t *mbi) > > If this function returned boot_info instead and the caller made the > assignment then it would be possible to unit-test/fuzz it. > > It also fits a bit more nicely with the usual implications of that function > name pattern, I think. As the larger capability continues to be developed, it is becoming necessary to get access to the reference from other areas of the code. Currently, I just move boot_info outside the function and exported the reference. I still have as an open question if the declaration should be static and access is obtained through an accessor function(s). I'm open to suggestions here. >> +{ >> + static struct boot_info __initdata info; >> + >> + info.nr_mods = mbi->mods_count; > > Shouldn't this be gated on MBI_MODULES being set? > > info.nr_mods = (mbi->flags & MBI_MODULES) ? mbi->mods_count : 0; Yes. >> + >> + boot_info = &info; >> +} >> >> unsigned long __init initial_images_nrpages(nodeid_t node) >> { >> @@ -285,7 +295,7 @@ unsigned long __init initial_images_nrpages(nodeid_t node) >> unsigned long nr; >> unsigned int i; >> >> - for ( nr = i = 0; i < nr_initial_images; ++i ) >> + for ( nr = i = 0; i < boot_info->nr_mods; ++i ) >> { >> unsigned long start = initial_images[i].mod_start; >> unsigned long end = start + PFN_UP(initial_images[i].mod_end); >> @@ -301,7 +311,7 @@ void __init discard_initial_images(void) >> { >> unsigned int i; >> >> - for ( i = 0; i < nr_initial_images; ++i ) >> + for ( i = 0; i < boot_info->nr_mods; ++i ) >> { >> uint64_t start = (uint64_t)initial_images[i].mod_start << PAGE_SHIFT; >> >> @@ -309,7 +319,7 @@ void __init discard_initial_images(void) >> start + PAGE_ALIGN(initial_images[i].mod_end)); >> } >> >> - nr_initial_images = 0; >> + boot_info->nr_mods = 0; > > Out of curiosity, why is this required? At this point, the boot modules have been "consumed" and are no longer safe to access. This ensures that if for some reason any module access code gets invoked that attempts to walk the modules, this will ensure it will result in no access. >> initial_images = NULL; >> } >> >> @@ -1034,9 +1044,10 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) >> mod = __va(mbi->mods_addr); >> } >> >> + multiboot_to_bootinfo(mbi); >> + >> loader = (mbi->flags & MBI_LOADERNAME) ? __va(mbi->boot_loader_name) >> : "unknown"; >> - > > Stray newline removal? > >> /* Parse the command-line options. */ >> if ( mbi->flags & MBI_CMDLINE ) >> cmdline = cmdline_cook(__va(mbi->cmdline), loader); >> @@ -1141,18 +1152,18 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) >> bootsym(boot_edd_info_nr)); >> >> /* Check that we have at least one Multiboot module. */ >> - if ( !(mbi->flags & MBI_MODULES) || (mbi->mods_count == 0) ) >> + if ( !(mbi->flags & MBI_MODULES) || (boot_info->nr_mods == 0) ) > > With MBI_MODULES accounted for during conversion, the first part of the > conditional can be ellided and you could simply do: > > if ( !boot_info->nr_mods ) > panic(...) This eventually happens, the goal in this revision was to slowly unhook the mbi usage as it is pervasive and so the reviewers can ensure we are correctly replacing all of its usage. With that said, I can look to see if it is reasonable to add its usage here. > Also, could we move this to multiboot_to_bootinfo()? It'd contain these sorts > of boot argument checks to a much more self contained function and help check > at the point of assignment, preventing misuse. These checks could be moved up into that function, but then you would lose the ability to print messages since the conversion function is invoked before serial is initialized. >> panic("dom0 kernel not specified. Check bootloader configuration\n"); >> >> /* Check that we don't have a silly number of modules. */ > >> - if ( mbi->mods_count > sizeof(module_map) * 8 ) >> + if ( boot_info->nr_mods > sizeof(module_map) * 8 ) > > Like above, this check would be much more neatly contained where boot_info > is created, imo. Again, you would lose the ability to print the error message. >> { >> - mbi->mods_count = sizeof(module_map) * 8; >> + boot_info->nr_mods = sizeof(module_map) * 8; >> printk("Excessive multiboot modules - using the first %u only\n", > > Does the comment need adjusting too to make it more general? As in > s/multiboot/boot. I don't see why the message couldn't be generalized. >> - mbi->mods_count); >> + boot_info->nr_mods); >> } >> >> - bitmap_fill(module_map, mbi->mods_count); >> + bitmap_fill(module_map, boot_info->nr_mods); >> __clear_bit(0, module_map); /* Dom0 kernel is always first */ >> >> if ( pvh_boot ) > > Cheers, > Alejandro Thanks for the review. v/r dps
On 9/3/24 18:24, Andrew Cooper wrote: > On 30/08/2024 10:46 pm, Daniel P. Smith wrote: >> From: Christopher Clark <christopher.w.clark@gmail.com> >> >> An initial step towards a non-multiboot internal representation of boot >> modules for common code, starting with x86 setup and converting the fields >> that are accessed for the startup calculations. >> >> Introduce a new header, <xen/asm/bootinfo.h>, and populate it with a new > > Just <asm/bootinfo.h>, which matches the code. Ack. >> diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h >> new file mode 100644 >> index 000000000000..e850f80d26a7 >> --- /dev/null >> +++ b/xen/arch/x86/include/asm/bootinfo.h >> @@ -0,0 +1,25 @@ >> +/* SPDX-License-Identifier: GPL-2.0-or-later */ >> +/* >> + * Copyright (c) 2024 Christopher Clark <christopher.w.clark@gmail.com> >> + * Copyright (c) 2024 Apertus Solutions, LLC >> + * Author: Daniel P. Smith <dpsmith@apertussolutions.com> >> + */ >> + >> +#ifndef __XEN_X86_BOOTINFO_H__ >> +#define __XEN_X86_BOOTINFO_H__ >> + > > There ought to be a short description of what boot_info is, even if it's > only "Xen's local representation of information provided by the > bootloader/environment." Yes, will be adding a description. >> +struct boot_info { >> + unsigned int nr_mods; > > For the sake of 3 letters, please can this be nr_modules. I've run sed > over the top of the v5 branch and it doesn't change line wrapping > anywhere, but it is a legibility improvement IMO. I'm okay with that. >> +}; >> + >> +#endif > > #endif /* __XEN_X86_BOOTINFO_H__ */ > > It very quickly get to not being in the same few lines as the #ifndef. Ack. >> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c >> index eee20bb1753c..dd94ee2e736b 100644 >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -276,7 +277,16 @@ static int __init cf_check parse_acpi_param(const char *s) >> custom_param("acpi", parse_acpi_param); >> >> static const module_t *__initdata initial_images; >> -static unsigned int __initdata nr_initial_images; >> +static struct boot_info __initdata *boot_info; >> + >> +static void __init multiboot_to_bootinfo(multiboot_info_t *mbi) >> +{ >> + static struct boot_info __initdata info; >> + >> + info.nr_mods = mbi->mods_count; >> + >> + boot_info = &info; >> +} > > Having a global pointer set only to this private structure is weird. > Even this: > > static struct boot_info __initdata boot_info[1]; > > lets you keep -> notation, but removes one level of indirection. Further work has pushed this into being a global, at least in the yet-to-be public work. The question is whether the allocation should just move out to the unit level or use a level of in direction with an accessor function. >> @@ -1034,9 +1044,10 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) >> mod = __va(mbi->mods_addr); >> } >> >> + multiboot_to_bootinfo(mbi); >> + >> loader = (mbi->flags & MBI_LOADERNAME) ? __va(mbi->boot_loader_name) >> : "unknown"; >> - > > Stray line removal. (should be in patch 2 to minimise churn.) Ack. > ~Andrew Thanks! v/r, dps
On 9/3/24 18:35, Andrew Cooper wrote: > On 30/08/2024 10:46 pm, Daniel P. Smith wrote: >> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c >> index eee20bb1753c..dd94ee2e736b 100644 >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -1034,9 +1044,10 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) >> mod = __va(mbi->mods_addr); >> } >> >> + multiboot_to_bootinfo(mbi); > > Actually, peeking ahead to the end of the series, we've got this: > > void __start_xen(unsigned long mbi_p) > { > ... > multiboot_info_t *mbi; > module_t *mod; > ... > > if ( pvh_boot ) > { > ASSERT(mbi_p == 0); > pvh_init(&mbi, &mod); > } > else > { > mbi = __va(mbi_p); > mod = __va(mbi->mods_addr); > } > > multiboot_to_bootinfo(mbi, mod); > > > which are the sum total of the mbi and mod pointers. Worse, pvh_init() > is transforming the PVH into into MB1 info, just to be transformed > immediately to BI. > > I expect this is work for the end of the series (I can't think of a nice > way to disentangle it earlier), but could we end up with something more > like: > > if ( pvh_boot ) > { > ASSERT(mbi_p == 0); > pvh_fill_boot_info(); > } > else > { > multiboot_info_t *mbi = __va(mbi_p); > > multiboot_fill_boot_info(mbi, __va(mbi->mods_addr)); > } > > ? > > Or perhaps even just pass mbi_p in, and have multiboot_fill_boot_info() > do the __va()'s itself. > > If so, we probably want to make a naming and possibly prototype > difference in this patch. Let me combine this with some of Alejandro's comments. What if I were to reshape it to look like this, /* * This level of indirection may not be desired, dropping it is not an * issue. I am proposing it because there is going to be a need to * access the instance from distant unit files. */ struct boot_info __init *get_boot_info(void) { static struct boot_info __initdata info; return &info; } static struct boot_info __init *multiboot_fill_boot_info( unsigned long mbi_p) { struct boot_info *bi = get_boot_info(); multiboot_info_t *mbi = __va(mbi_p); module_t mods = __va(mbi->mods_addr); ... return bi; } , then in the PVH boot code struct boot_info __init *pvh_fill_boot_info(void) { struct boot_info *bi = get_boot_info(); ... return bi; } , and then something similar for efi. What does everyone think? v/r, dps
V/r, Daniel P. Smith Apertus Solutions, LLC On 9/4/24 02:31, Jan Beulich wrote: > On 30.08.2024 23:46, Daniel P. Smith wrote: >> @@ -276,7 +277,16 @@ static int __init cf_check parse_acpi_param(const char *s) >> custom_param("acpi", parse_acpi_param); >> >> static const module_t *__initdata initial_images; >> -static unsigned int __initdata nr_initial_images; >> +static struct boot_info __initdata *boot_info; >> + >> +static void __init multiboot_to_bootinfo(multiboot_info_t *mbi) > > Pointer-to-const please. Provided it does not change based on other feedback, you are correct that it should be const. v/r, dps
diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h new file mode 100644 index 000000000000..e850f80d26a7 --- /dev/null +++ b/xen/arch/x86/include/asm/bootinfo.h @@ -0,0 +1,25 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Copyright (c) 2024 Christopher Clark <christopher.w.clark@gmail.com> + * Copyright (c) 2024 Apertus Solutions, LLC + * Author: Daniel P. Smith <dpsmith@apertussolutions.com> + */ + +#ifndef __XEN_X86_BOOTINFO_H__ +#define __XEN_X86_BOOTINFO_H__ + +struct boot_info { + unsigned int nr_mods; +}; + +#endif + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index eee20bb1753c..dd94ee2e736b 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -32,6 +32,7 @@ #include <compat/xen.h> #endif #include <xen/bitops.h> +#include <asm/bootinfo.h> #include <asm/smp.h> #include <asm/processor.h> #include <asm/mpspec.h> @@ -276,7 +277,16 @@ static int __init cf_check parse_acpi_param(const char *s) custom_param("acpi", parse_acpi_param); static const module_t *__initdata initial_images; -static unsigned int __initdata nr_initial_images; +static struct boot_info __initdata *boot_info; + +static void __init multiboot_to_bootinfo(multiboot_info_t *mbi) +{ + static struct boot_info __initdata info; + + info.nr_mods = mbi->mods_count; + + boot_info = &info; +} unsigned long __init initial_images_nrpages(nodeid_t node) { @@ -285,7 +295,7 @@ unsigned long __init initial_images_nrpages(nodeid_t node) unsigned long nr; unsigned int i; - for ( nr = i = 0; i < nr_initial_images; ++i ) + for ( nr = i = 0; i < boot_info->nr_mods; ++i ) { unsigned long start = initial_images[i].mod_start; unsigned long end = start + PFN_UP(initial_images[i].mod_end); @@ -301,7 +311,7 @@ void __init discard_initial_images(void) { unsigned int i; - for ( i = 0; i < nr_initial_images; ++i ) + for ( i = 0; i < boot_info->nr_mods; ++i ) { uint64_t start = (uint64_t)initial_images[i].mod_start << PAGE_SHIFT; @@ -309,7 +319,7 @@ void __init discard_initial_images(void) start + PAGE_ALIGN(initial_images[i].mod_end)); } - nr_initial_images = 0; + boot_info->nr_mods = 0; initial_images = NULL; } @@ -1034,9 +1044,10 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) mod = __va(mbi->mods_addr); } + multiboot_to_bootinfo(mbi); + loader = (mbi->flags & MBI_LOADERNAME) ? __va(mbi->boot_loader_name) : "unknown"; - /* Parse the command-line options. */ if ( mbi->flags & MBI_CMDLINE ) cmdline = cmdline_cook(__va(mbi->cmdline), loader); @@ -1141,18 +1152,18 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) bootsym(boot_edd_info_nr)); /* Check that we have at least one Multiboot module. */ - if ( !(mbi->flags & MBI_MODULES) || (mbi->mods_count == 0) ) + if ( !(mbi->flags & MBI_MODULES) || (boot_info->nr_mods == 0) ) panic("dom0 kernel not specified. Check bootloader configuration\n"); /* Check that we don't have a silly number of modules. */ - if ( mbi->mods_count > sizeof(module_map) * 8 ) + if ( boot_info->nr_mods > sizeof(module_map) * 8 ) { - mbi->mods_count = sizeof(module_map) * 8; + boot_info->nr_mods = sizeof(module_map) * 8; printk("Excessive multiboot modules - using the first %u only\n", - mbi->mods_count); + boot_info->nr_mods); } - bitmap_fill(module_map, mbi->mods_count); + bitmap_fill(module_map, boot_info->nr_mods); __clear_bit(0, module_map); /* Dom0 kernel is always first */ if ( pvh_boot ) @@ -1325,9 +1336,8 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) kexec_reserve_area(); initial_images = mod; - nr_initial_images = mbi->mods_count; - for ( i = 0; !efi_enabled(EFI_LOADER) && i < mbi->mods_count; i++ ) + for ( i = 0; !efi_enabled(EFI_LOADER) && i < boot_info->nr_mods; i++ ) { if ( mod[i].mod_start & (PAGE_SIZE - 1) ) panic("Bootloader didn't honor module alignment request\n"); @@ -1351,8 +1361,8 @@ void asmlinkage __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[mbi->mods_count].mod_start = virt_to_mfn(_stext); - mod[mbi->mods_count].mod_end = __2M_rwdata_end - _stext; + mod[boot_info->nr_mods].mod_start = virt_to_mfn(_stext); + mod[boot_info->nr_mods].mod_end = __2M_rwdata_end - _stext; } modules_headroom = bzimage_headroom(bootstrap_map(mod), mod->mod_end); @@ -1412,7 +1422,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) { /* Don't overlap with modules. */ end = consider_modules(s, e, reloc_size + mask, - mod, mbi->mods_count, -1); + mod, boot_info->nr_mods, -1); end &= ~mask; } else @@ -1433,7 +1443,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) } /* Is the region suitable for relocating the multiboot modules? */ - for ( j = mbi->mods_count - 1; j >= 0; j-- ) + for ( j = boot_info->nr_mods - 1; j >= 0; j-- ) { /* * 'headroom' is a guess for the decompressed size and @@ -1448,7 +1458,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) /* Don't overlap with other modules (or Xen itself). */ end = consider_modules(s, e, size, mod, - mbi->mods_count + relocated, j); + boot_info->nr_mods + relocated, j); if ( highmem_start && end > highmem_start ) continue; @@ -1475,7 +1485,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) { /* Don't overlap with modules (or Xen itself). */ e = consider_modules(s, e, PAGE_ALIGN(kexec_crash_area.size), mod, - mbi->mods_count + relocated, -1); + boot_info->nr_mods + relocated, -1); if ( s >= e ) break; if ( e > kexec_crash_area_limit ) @@ -1490,7 +1500,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) if ( modules_headroom && !mod->reserved ) panic("Not enough memory to relocate the dom0 kernel image\n"); - for ( i = 0; i < mbi->mods_count; ++i ) + for ( i = 0; i < boot_info->nr_mods; ++i ) { uint64_t s = (uint64_t)mod[i].mod_start << PAGE_SHIFT; @@ -1570,7 +1580,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) ASSERT(j); } map_e = boot_e820.map[j].addr + boot_e820.map[j].size; - for ( j = 0; j < mbi->mods_count; ++j ) + for ( j = 0; j < boot_info->nr_mods; ++j ) { uint64_t end = pfn_to_paddr(mod[j].mod_start) + mod[j].mod_end; @@ -1645,7 +1655,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) } } - for ( i = 0; i < mbi->mods_count; ++i ) + for ( i = 0; i < boot_info->nr_mods; ++i ) { set_pdx_range(mod[i].mod_start, mod[i].mod_start + PFN_UP(mod[i].mod_end)); @@ -2032,8 +2042,8 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) cpu_has_nx ? XENLOG_INFO : XENLOG_WARNING "Warning: ", cpu_has_nx ? "" : "not "); - initrdidx = find_first_bit(module_map, mbi->mods_count); - if ( bitmap_weight(module_map, mbi->mods_count) > 1 ) + initrdidx = find_first_bit(module_map, boot_info->nr_mods); + if ( bitmap_weight(module_map, boot_info->nr_mods) > 1 ) printk(XENLOG_WARNING "Multiple initrd candidates, picking module #%u\n", initrdidx); @@ -2043,7 +2053,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) * above our heap. The second module, if present, is an initrd ramdisk. */ dom0 = create_dom0(mod, modules_headroom, - initrdidx < mbi->mods_count ? mod + initrdidx : NULL, + initrdidx < boot_info->nr_mods ? mod + initrdidx : NULL, kextra, loader); if ( !dom0 ) panic("Could not set up DOM0 guest OS\n");