| Message ID | 20220706210454.30096-2-dpsmith@apertussolutions.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Hyperlaunch | expand |
Hi Daniel, > -----Original Message----- > Subject: [PATCH v1 01/18] kconfig: allow configuration of maximum modules > > For x86 the number of allowable multiboot modules varies between the > different > entry points, non-efi boot, pvh boot, and efi boot. In the case of both Arm > and > x86 this value is fixed to values based on generalized assumptions. With > hyperlaunch for x86 and dom0less on Arm, use of static sizes results in large > allocations compiled into the hypervisor that will go unused by many use > cases. > > This commit introduces a Kconfig variable that is set with sane defaults based > on configuration selection. This variable is in turned used as the array size > for the cases where a static allocated array of boot modules is declared. > > Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> > Reviewed-by: Christopher Clark <christopher.clark@starlab.io> Reviewed-by: Henry Wang <Henry.Wang@arm.com> > --- > xen/arch/Kconfig | 12 ++++++++++++ > xen/arch/arm/include/asm/setup.h | 5 +++-- > xen/arch/x86/efi/efi-boot.h | 2 +- > xen/arch/x86/guest/xen/pvh-boot.c | 2 +- > xen/arch/x86/setup.c | 4 ++-- > 5 files changed, 19 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/Kconfig b/xen/arch/Kconfig > index f16eb0df43..24139057be 100644 > --- a/xen/arch/Kconfig > +++ b/xen/arch/Kconfig > @@ -17,3 +17,15 @@ config NR_CPUS > For CPU cores which support Simultaneous Multi-Threading or > similar > technologies, this the number of logical threads which Xen will > support. > + > +config NR_BOOTMODS > + int "Maximum number of boot modules that a loader can pass" > + range 1 32768 > + default "8" if X86 > + default "32" if ARM > + help > + Controls the build-time size of various arrays allocated for > + parsing the boot modules passed by a loader when starting Xen. > + > + This is of particular interest when using Xen's hypervisor domain > + capabilities such as dom0less. > diff --git a/xen/arch/arm/include/asm/setup.h > b/xen/arch/arm/include/asm/setup.h > index 2bb01ecfa8..312a3e4209 100644 > --- a/xen/arch/arm/include/asm/setup.h > +++ b/xen/arch/arm/include/asm/setup.h > @@ -10,7 +10,8 @@ > > #define NR_MEM_BANKS 256 > > -#define MAX_MODULES 32 /* Current maximum useful modules */ > +/* Current maximum useful modules */ > +#define MAX_MODULES CONFIG_NR_BOOTMODS > > typedef enum { > BOOTMOD_XEN, > @@ -38,7 +39,7 @@ struct meminfo { > * The domU flag is set for kernels and ramdisks of "xen,domain" nodes. > * The purpose of the domU flag is to avoid getting confused in > * kernel_probe, where we try to guess which is the dom0 kernel and > - * initrd to be compatible with all versions of the multiboot spec. > + * initrd to be compatible with all versions of the multiboot spec. Thanks for taking the chance to remove the space in the end of the sentence. Kind regards, Henry
Hi Daniel, On 06/07/2022 22:04, Daniel P. Smith wrote: > For x86 the number of allowable multiboot modules varies between the different > entry points, non-efi boot, pvh boot, and efi boot. In the case of both Arm and > x86 this value is fixed to values based on generalized assumptions. With > hyperlaunch for x86 and dom0less on Arm, use of static sizes results in large > allocations compiled into the hypervisor that will go unused by many use cases. > > This commit introduces a Kconfig variable that is set with sane defaults based > on configuration selection. This variable is in turned used as the array size > for the cases where a static allocated array of boot modules is declared. > > Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> > Reviewed-by: Christopher Clark <christopher.clark@starlab.io> I am not entirely sure where this reviewed-by is coming from. Is this from internal review? If yes, my recommendation would be to provide the reviewed-by on the mailing list. Ideally, the review should also be done in the open, but I understand some company wish to do a fully internal review first. At least from a committer perspective, this helps me to know whether the reviewed-by still apply. An example would be if you send a v2, I would not be able to know whether Christoffer still agreed on the change. > --- > xen/arch/Kconfig | 12 ++++++++++++ > xen/arch/arm/include/asm/setup.h | 5 +++-- > xen/arch/x86/efi/efi-boot.h | 2 +- > xen/arch/x86/guest/xen/pvh-boot.c | 2 +- > xen/arch/x86/setup.c | 4 ++-- > 5 files changed, 19 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/Kconfig b/xen/arch/Kconfig > index f16eb0df43..24139057be 100644 > --- a/xen/arch/Kconfig > +++ b/xen/arch/Kconfig > @@ -17,3 +17,15 @@ config NR_CPUS > For CPU cores which support Simultaneous Multi-Threading or similar > technologies, this the number of logical threads which Xen will > support. > + > +config NR_BOOTMODS > + int "Maximum number of boot modules that a loader can pass" > + range 1 32768 > + default "8" if X86 > + default "32" if ARM > + help > + Controls the build-time size of various arrays allocated for > + parsing the boot modules passed by a loader when starting Xen. > + > + This is of particular interest when using Xen's hypervisor domain > + capabilities such as dom0less. > diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h > index 2bb01ecfa8..312a3e4209 100644 > --- a/xen/arch/arm/include/asm/setup.h > +++ b/xen/arch/arm/include/asm/setup.h > @@ -10,7 +10,8 @@ > > #define NR_MEM_BANKS 256 > > -#define MAX_MODULES 32 /* Current maximum useful modules */ > +/* Current maximum useful modules */ > +#define MAX_MODULES CONFIG_NR_BOOTMODS > > typedef enum { > BOOTMOD_XEN, > @@ -38,7 +39,7 @@ struct meminfo { > * The domU flag is set for kernels and ramdisks of "xen,domain" nodes. > * The purpose of the domU flag is to avoid getting confused in > * kernel_probe, where we try to guess which is the dom0 kernel and > - * initrd to be compatible with all versions of the multiboot spec. > + * initrd to be compatible with all versions of the multiboot spec. In general, I much prefer if coding style changes are done separately because it helps the review (I don't have to stare at the line to figure out what changed). I am not going to force this here. However, the strict minimum is to mention the change in the commit message. > */ > #define BOOTMOD_MAX_CMDLINE 1024 > struct bootmodule { > diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h > index 6e65b569b0..4e1a799749 100644 > --- a/xen/arch/x86/efi/efi-boot.h > +++ b/xen/arch/x86/efi/efi-boot.h > @@ -18,7 +18,7 @@ static multiboot_info_t __initdata mbi = { > * The array size needs to be one larger than the number of modules we > * support - see __start_xen(). > */ > -static module_t __initdata mb_modules[5]; > +static module_t __initdata mb_modules[CONFIG_NR_BOOTMODS + 1]; Please explain in the commit message why the number of modules was bumped from 5 to 9. > > static void __init edd_put_string(u8 *dst, size_t n, const char *src) > { > diff --git a/xen/arch/x86/guest/xen/pvh-boot.c b/xen/arch/x86/guest/xen/pvh-boot.c > index 498625eae0..834b1ad16b 100644 > --- a/xen/arch/x86/guest/xen/pvh-boot.c > +++ b/xen/arch/x86/guest/xen/pvh-boot.c > @@ -32,7 +32,7 @@ bool __initdata pvh_boot; > uint32_t __initdata pvh_start_info_pa; > > static multiboot_info_t __initdata pvh_mbi; > -static module_t __initdata pvh_mbi_mods[8]; > +static module_t __initdata pvh_mbi_mods[CONFIG_NR_BOOTMOD + 1]; What's the +1 for? > static const char *__initdata pvh_loader = "PVH Directboot"; > > static void __init convert_pvh_info(multiboot_info_t **mbi, > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c > index f08b07b8de..2aa1e28c8f 100644 > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -1020,9 +1020,9 @@ void __init noreturn __start_xen(unsigned long mbi_p) > 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 ( mbi->mods_count > CONFIG_NR_BOOTMODS ) > { > - mbi->mods_count = sizeof(module_map) * 8; > + mbi->mods_count = CONFIG_NR_BOOTMODS; > printk("Excessive multiboot modules - using the first %u only\n", > mbi->mods_count); > } AFAIU, this check is to make sure that we will not overrun module_map in the next line: bitmap_fill(module_map, mbi->mods_count); The current definition of module_map will allow 64 modules. But you are allowing 32768. So I think you either want to keep the check or define module_map as: DECLARE_BITMAP(module_map, CONFIG_NR_BOOTMODS); Cheers,
On 06.07.2022 23:04, Daniel P. Smith wrote: > --- a/xen/arch/Kconfig > +++ b/xen/arch/Kconfig > @@ -17,3 +17,15 @@ config NR_CPUS > For CPU cores which support Simultaneous Multi-Threading or similar > technologies, this the number of logical threads which Xen will > support. > + > +config NR_BOOTMODS > + int "Maximum number of boot modules that a loader can pass" > + range 1 32768 > + default "8" if X86 > + default "32" if ARM Any reason for the larger default on Arm, irrespective of dom0less actually being in use? (I'm actually surprised I can't spot a Kconfig option controlling inclusion of dom0less. The default here imo isn't supposed to depend on the architecture, but on whether dom0less is supported. That way if another arch gained dom0less support, the higher default would apply to it without needing further adjustment.) > --- a/xen/arch/x86/efi/efi-boot.h > +++ b/xen/arch/x86/efi/efi-boot.h > @@ -18,7 +18,7 @@ static multiboot_info_t __initdata mbi = { > * The array size needs to be one larger than the number of modules we > * support - see __start_xen(). > */ > -static module_t __initdata mb_modules[5]; > +static module_t __initdata mb_modules[CONFIG_NR_BOOTMODS + 1]; If the build admin selected 1, I'm pretty sure about nothing would work. I think you want max(5, CONFIG_NR_BOOTMODS) or max(4, CONFIG_NR_BOOTMODS) + 1 here and ... > --- a/xen/arch/x86/guest/xen/pvh-boot.c > +++ b/xen/arch/x86/guest/xen/pvh-boot.c > @@ -32,7 +32,7 @@ bool __initdata pvh_boot; > uint32_t __initdata pvh_start_info_pa; > > static multiboot_info_t __initdata pvh_mbi; > -static module_t __initdata pvh_mbi_mods[8]; > +static module_t __initdata pvh_mbi_mods[CONFIG_NR_BOOTMOD + 1]; ... max(8, CONFIG_NR_BOOTMODS) here (albeit the 8 may have room for lowering - I don't recall why 8 was chosen rather than going with the minimum possible value covering all module kinds known at that time). Jan
On 7/15/22 15:16, Julien Grall wrote: > Hi Daniel, > > On 06/07/2022 22:04, Daniel P. Smith wrote: >> For x86 the number of allowable multiboot modules varies between the >> different >> entry points, non-efi boot, pvh boot, and efi boot. In the case of >> both Arm and >> x86 this value is fixed to values based on generalized assumptions. With >> hyperlaunch for x86 and dom0less on Arm, use of static sizes results >> in large >> allocations compiled into the hypervisor that will go unused by many >> use cases. >> >> This commit introduces a Kconfig variable that is set with sane >> defaults based >> on configuration selection. This variable is in turned used as the >> array size >> for the cases where a static allocated array of boot modules is declared. >> >> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> >> Reviewed-by: Christopher Clark <christopher.clark@starlab.io> > > I am not entirely sure where this reviewed-by is coming from. Is this > from internal review? Yes. > If yes, my recommendation would be to provide the reviewed-by on the > mailing list. Ideally, the review should also be done in the open, but I > understand some company wish to do a fully internal review first. Since this capability is being jointly developed by Christopher and I, with myself being the author of code, Christopher reviewed the code as the co-developer. He did so as a second pair of eyes for any obvious mistakes and to concur that the implementation was in line with the approach the two of us architected. Perhaps a SoB line might be more appropriate than an R-b line. > At least from a committer perspective, this helps me to know whether the > reviewed-by still apply. An example would be if you send a v2, I would > not be able to know whether Christoffer still agreed on the change. If an SoB line is more appropriate, then on the next version I can switch it >> --- >> xen/arch/Kconfig | 12 ++++++++++++ >> xen/arch/arm/include/asm/setup.h | 5 +++-- >> xen/arch/x86/efi/efi-boot.h | 2 +- >> xen/arch/x86/guest/xen/pvh-boot.c | 2 +- >> xen/arch/x86/setup.c | 4 ++-- >> 5 files changed, 19 insertions(+), 6 deletions(-) >> >> diff --git a/xen/arch/Kconfig b/xen/arch/Kconfig >> index f16eb0df43..24139057be 100644 >> --- a/xen/arch/Kconfig >> +++ b/xen/arch/Kconfig >> @@ -17,3 +17,15 @@ config NR_CPUS >> For CPU cores which support Simultaneous Multi-Threading or >> similar >> technologies, this the number of logical threads which Xen will >> support. >> + >> +config NR_BOOTMODS >> + int "Maximum number of boot modules that a loader can pass" >> + range 1 32768 >> + default "8" if X86 >> + default "32" if ARM >> + help >> + Controls the build-time size of various arrays allocated for >> + parsing the boot modules passed by a loader when starting Xen. >> + >> + This is of particular interest when using Xen's hypervisor domain >> + capabilities such as dom0less. >> diff --git a/xen/arch/arm/include/asm/setup.h >> b/xen/arch/arm/include/asm/setup.h >> index 2bb01ecfa8..312a3e4209 100644 >> --- a/xen/arch/arm/include/asm/setup.h >> +++ b/xen/arch/arm/include/asm/setup.h >> @@ -10,7 +10,8 @@ >> #define NR_MEM_BANKS 256 >> -#define MAX_MODULES 32 /* Current maximum useful modules */ >> +/* Current maximum useful modules */ >> +#define MAX_MODULES CONFIG_NR_BOOTMODS >> typedef enum { >> BOOTMOD_XEN, >> @@ -38,7 +39,7 @@ struct meminfo { >> * The domU flag is set for kernels and ramdisks of "xen,domain" nodes. >> * The purpose of the domU flag is to avoid getting confused in >> * kernel_probe, where we try to guess which is the dom0 kernel and >> - * initrd to be compatible with all versions of the multiboot spec. >> + * initrd to be compatible with all versions of the multiboot spec. > > In general, I much prefer if coding style changes are done separately > because it helps the review (I don't have to stare at the line to figure > out what changed). Actually, on a past review of another series I got dinged on this, and I did try to get most of them out of this series. This is just a straggler that I missed. I will clean up on next revision. > I am not going to force this here. However, the strict minimum is to > mention the change in the commit message. > >> */ >> #define BOOTMOD_MAX_CMDLINE 1024 >> struct bootmodule { >> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h >> index 6e65b569b0..4e1a799749 100644 >> --- a/xen/arch/x86/efi/efi-boot.h >> +++ b/xen/arch/x86/efi/efi-boot.h >> @@ -18,7 +18,7 @@ static multiboot_info_t __initdata mbi = { >> * The array size needs to be one larger than the number of modules we >> * support - see __start_xen(). >> */ >> -static module_t __initdata mb_modules[5]; >> +static module_t __initdata mb_modules[CONFIG_NR_BOOTMODS + 1]; > > Please explain in the commit message why the number of modules was > bumped from 5 to 9. The number of modules were inconsistent between the different entry points into __start_xen(). By switching to a Kconfig variable, whose default was set to the largest value used across the entry points, results in change for the locations using another value. See below for +1 explanation. >> static void __init edd_put_string(u8 *dst, size_t n, const char *src) >> { >> diff --git a/xen/arch/x86/guest/xen/pvh-boot.c >> b/xen/arch/x86/guest/xen/pvh-boot.c >> index 498625eae0..834b1ad16b 100644 >> --- a/xen/arch/x86/guest/xen/pvh-boot.c >> +++ b/xen/arch/x86/guest/xen/pvh-boot.c >> @@ -32,7 +32,7 @@ bool __initdata pvh_boot; >> uint32_t __initdata pvh_start_info_pa; >> static multiboot_info_t __initdata pvh_mbi; >> -static module_t __initdata pvh_mbi_mods[8]; >> +static module_t __initdata pvh_mbi_mods[CONFIG_NR_BOOTMOD + 1]; > > What's the +1 for? I should clarify in the commit message, but the value set in CONFIG_NR_BOOTMOD is the max modules that Xen would accept from a bootloader. Xen startup code expects to be able to append Xen itself as the array. The +1 allocates an additional entry to store Xen in the array should a bootloader actually pass CONFIG_NR_BOOTMOD modules to Xen. There is an existing comment floating in one of these locations that explained it. >> static const char *__initdata pvh_loader = "PVH Directboot"; >> static void __init convert_pvh_info(multiboot_info_t **mbi, >> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c >> index f08b07b8de..2aa1e28c8f 100644 >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -1020,9 +1020,9 @@ void __init noreturn __start_xen(unsigned long >> mbi_p) >> 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 ( mbi->mods_count > CONFIG_NR_BOOTMODS ) >> { >> - mbi->mods_count = sizeof(module_map) * 8; >> + mbi->mods_count = CONFIG_NR_BOOTMODS; >> printk("Excessive multiboot modules - using the first %u >> only\n", >> mbi->mods_count); >> } > > AFAIU, this check is to make sure that we will not overrun module_map in > the next line: > > bitmap_fill(module_map, mbi->mods_count); > > The current definition of module_map will allow 64 modules. But you are > allowing 32768. So I think you either want to keep the check or define > module_map as: > > DECLARE_BITMAP(module_map, CONFIG_NR_BOOTMODS); Yes, in the RFC I had it capped to 64 and lost track of this related changed when it was bumped to 32768 per the review discussion. Later in the series, module_map goes away. To ensure stability at this point I would be inclined to restore the 64 module clamp down check. Thoughts? v/r, dps
On 7/19/22 05:32, Jan Beulich wrote: > On 06.07.2022 23:04, Daniel P. Smith wrote: >> --- a/xen/arch/Kconfig >> +++ b/xen/arch/Kconfig >> @@ -17,3 +17,15 @@ config NR_CPUS >> For CPU cores which support Simultaneous Multi-Threading or similar >> technologies, this the number of logical threads which Xen will >> support. >> + >> +config NR_BOOTMODS >> + int "Maximum number of boot modules that a loader can pass" >> + range 1 32768 >> + default "8" if X86 >> + default "32" if ARM > > Any reason for the larger default on Arm, irrespective of dom0less > actually being in use? (I'm actually surprised I can't spot a Kconfig > option controlling inclusion of dom0less. The default here imo isn't > supposed to depend on the architecture, but on whether dom0less is > supported. That way if another arch gained dom0less support, the > higher default would apply to it without needing further adjustment.) Yes, multidomain construction is always on for Arm and the only configurable is a commandline parameter to enforce that dom0 is not created. As for the default, it was selected based on the largest value used in the locations replaced by the Kconfig variable. Since there was a significant difference between Arm and x86, I did not feel it was appropriate to reduce/increase either, since it drives multiple static array allocations for x86. I have no attachments to any specific value, so I will freely adjust to whatever conscience the community might come to. >> --- a/xen/arch/x86/efi/efi-boot.h >> +++ b/xen/arch/x86/efi/efi-boot.h >> @@ -18,7 +18,7 @@ static multiboot_info_t __initdata mbi = { >> * The array size needs to be one larger than the number of modules we >> * support - see __start_xen(). >> */ >> -static module_t __initdata mb_modules[5]; >> +static module_t __initdata mb_modules[CONFIG_NR_BOOTMODS + 1]; > > If the build admin selected 1, I'm pretty sure about nothing would work. > I think you want max(5, CONFIG_NR_BOOTMODS) or > max(4, CONFIG_NR_BOOTMODS) + 1 here and ... Actually, I reasoned this out and 1 is in fact a valid value. It would mean Xen + Dom0 Linux kernel with embedded initramfs with no externally loaded XSM policy and no boot time microcode patching. This is a working configuration, but open to debate if it is a desirable configuration. The question is whether it is desired to block someone from building such a configuration, or any number between 1 and 4. If the answer is yes, then why not just set the lower bound of the range in the Kconfig file instead of having to maintain a hard-coded lower bound in a max marco across multiple locations? >> --- a/xen/arch/x86/guest/xen/pvh-boot.c >> +++ b/xen/arch/x86/guest/xen/pvh-boot.c >> @@ -32,7 +32,7 @@ bool __initdata pvh_boot; >> uint32_t __initdata pvh_start_info_pa; >> >> static multiboot_info_t __initdata pvh_mbi; >> -static module_t __initdata pvh_mbi_mods[8]; >> +static module_t __initdata pvh_mbi_mods[CONFIG_NR_BOOTMOD + 1]; > > ... max(8, CONFIG_NR_BOOTMODS) here (albeit the 8 may have room for > lowering - I don't recall why 8 was chosen rather than going with > the minimum possible value covering all module kinds known at that > time). This is what drove the default for x86 in Kconfig to be 8. I thought it was excessive but assumed there was some reason for the value. And see my comment above whether it should be max({n},CONFIG_NR_BOOTMOD) vs range {n}..32768.
On 19.07.2022 19:02, Daniel P. Smith wrote: > On 7/19/22 05:32, Jan Beulich wrote: >> On 06.07.2022 23:04, Daniel P. Smith wrote: >>> --- a/xen/arch/x86/efi/efi-boot.h >>> +++ b/xen/arch/x86/efi/efi-boot.h >>> @@ -18,7 +18,7 @@ static multiboot_info_t __initdata mbi = { >>> * The array size needs to be one larger than the number of modules we >>> * support - see __start_xen(). >>> */ >>> -static module_t __initdata mb_modules[5]; >>> +static module_t __initdata mb_modules[CONFIG_NR_BOOTMODS + 1]; >> >> If the build admin selected 1, I'm pretty sure about nothing would work. >> I think you want max(5, CONFIG_NR_BOOTMODS) or >> max(4, CONFIG_NR_BOOTMODS) + 1 here and ... > > Actually, I reasoned this out and 1 is in fact a valid value. It would > mean Xen + Dom0 Linux kernel with embedded initramfs with no externally > loaded XSM policy and no boot time microcode patching. This is a working > configuration, but open to debate if it is a desirable configuration. > The question is whether it is desired to block someone from building > such a configuration, or any number between 1 and 4. If the answer is > yes, then why not just set the lower bound of the range in the Kconfig > file instead of having to maintain a hard-coded lower bound in a max > marco across multiple locations? While I'd be fine with the lower bounds being raised, I wouldn't be very happy with seeing those lower bounds becoming arch-specific. Jan
On 7/20/22 03:27, Jan Beulich wrote: > On 19.07.2022 19:02, Daniel P. Smith wrote: >> On 7/19/22 05:32, Jan Beulich wrote: >>> On 06.07.2022 23:04, Daniel P. Smith wrote: >>>> --- a/xen/arch/x86/efi/efi-boot.h >>>> +++ b/xen/arch/x86/efi/efi-boot.h >>>> @@ -18,7 +18,7 @@ static multiboot_info_t __initdata mbi = { >>>> * The array size needs to be one larger than the number of modules we >>>> * support - see __start_xen(). >>>> */ >>>> -static module_t __initdata mb_modules[5]; >>>> +static module_t __initdata mb_modules[CONFIG_NR_BOOTMODS + 1]; >>> >>> If the build admin selected 1, I'm pretty sure about nothing would work. >>> I think you want max(5, CONFIG_NR_BOOTMODS) or >>> max(4, CONFIG_NR_BOOTMODS) + 1 here and ... >> >> Actually, I reasoned this out and 1 is in fact a valid value. It would >> mean Xen + Dom0 Linux kernel with embedded initramfs with no externally >> loaded XSM policy and no boot time microcode patching. This is a working >> configuration, but open to debate if it is a desirable configuration. >> The question is whether it is desired to block someone from building >> such a configuration, or any number between 1 and 4. If the answer is >> yes, then why not just set the lower bound of the range in the Kconfig >> file instead of having to maintain a hard-coded lower bound in a max >> marco across multiple locations? > > While I'd be fine with the lower bounds being raised, I wouldn't be very > happy with seeing those lower bounds becoming arch-specific. Okay, and I am not sure how changing the range in Kconfig would make it arch-specific. I was not proposing making the existing range conditioned and having arch specific instances. There is one range, it will have a lower bound of 4 and the upper bound of 31768. v/r, dps
Hi Daniel, On 19/07/2022 17:36, Daniel P. Smith wrote: > > On 7/15/22 15:16, Julien Grall wrote: >> Hi Daniel, >> >> On 06/07/2022 22:04, Daniel P. Smith wrote: >>> For x86 the number of allowable multiboot modules varies between the >>> different >>> entry points, non-efi boot, pvh boot, and efi boot. In the case of >>> both Arm and >>> x86 this value is fixed to values based on generalized assumptions. With >>> hyperlaunch for x86 and dom0less on Arm, use of static sizes results >>> in large >>> allocations compiled into the hypervisor that will go unused by many >>> use cases. >>> >>> This commit introduces a Kconfig variable that is set with sane >>> defaults based >>> on configuration selection. This variable is in turned used as the >>> array size >>> for the cases where a static allocated array of boot modules is declared. >>> >>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> >>> Reviewed-by: Christopher Clark <christopher.clark@starlab.io> >> >> I am not entirely sure where this reviewed-by is coming from. Is this >> from internal review? > > Yes. > >> If yes, my recommendation would be to provide the reviewed-by on the >> mailing list. Ideally, the review should also be done in the open, but I >> understand some company wish to do a fully internal review first. > > Since this capability is being jointly developed by Christopher and I, > with myself being the author of code, Christopher reviewed the code as > the co-developer. He did so as a second pair of eyes for any obvious > mistakes and to concur that the implementation was in line with the > approach the two of us architected. Perhaps a SoB line might be more > appropriate than an R-b line. > >> At least from a committer perspective, this helps me to know whether the >> reviewed-by still apply. An example would be if you send a v2, I would >> not be able to know whether Christoffer still agreed on the change. > > If an SoB line is more appropriate, then on the next version I can > switch it Thanks for the explanation. To me "signed-off-by" means the person wrote some code (or sent the patches) code. So from above, it sounds more like Christoffer did a review. So I think it is more suitable for him to provide a reviewed-by. For follow-up, my preference would be Christoffer to provide the reviewed-by on the ML. If it is too much overhead, I would suggest to log the latest version Christoffer reviewed-by in the changelog. I usually do: Changes in vX: - Add Christoffer's reviewed-by Or if he will reviewing every version, just mention it in the cover letter. >> >> Please explain in the commit message why the number of modules was >> bumped from 5 to 9. > > The number of modules were inconsistent between the different entry > points into __start_xen(). By switching to a Kconfig variable, whose > default was set to the largest value used across the entry points, > results in change for the locations using another value. Ok. Can you add something like: "For x86, the number of modules is not consistent across the code base. Use the maximum"? > > See below for +1 explanation. > >>> static void __init edd_put_string(u8 *dst, size_t n, const char *src) >>> { >>> diff --git a/xen/arch/x86/guest/xen/pvh-boot.c >>> b/xen/arch/x86/guest/xen/pvh-boot.c >>> index 498625eae0..834b1ad16b 100644 >>> --- a/xen/arch/x86/guest/xen/pvh-boot.c >>> +++ b/xen/arch/x86/guest/xen/pvh-boot.c >>> @@ -32,7 +32,7 @@ bool __initdata pvh_boot; >>> uint32_t __initdata pvh_start_info_pa; >>> static multiboot_info_t __initdata pvh_mbi; >>> -static module_t __initdata pvh_mbi_mods[8]; >>> +static module_t __initdata pvh_mbi_mods[CONFIG_NR_BOOTMOD + 1]; >> >> What's the +1 for? > > I should clarify in the commit message, but the value set in > CONFIG_NR_BOOTMOD is the max modules that Xen would accept from a > bootloader. Xen startup code expects to be able to append Xen itself as > the array. The +1 allocates an additional entry to store Xen in the > array should a bootloader actually pass CONFIG_NR_BOOTMOD modules to > Xen. There is an existing comment floating in one of these locations > that explained it. This makes sense. So every use of CONFIG_NR_BOOTMOD would end up to require +1. Is that correct? If yes, then I think it would be better to require CONFIG_NR_BOOTMOD to be at minimum 1. This would reduce the risk to have different array size again. That said, this is x86 code, so the call is for the x86 maintainers. > >>> static const char *__initdata pvh_loader = "PVH Directboot"; >>> static void __init convert_pvh_info(multiboot_info_t **mbi, >>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c >>> index f08b07b8de..2aa1e28c8f 100644 >>> --- a/xen/arch/x86/setup.c >>> +++ b/xen/arch/x86/setup.c >>> @@ -1020,9 +1020,9 @@ void __init noreturn __start_xen(unsigned long >>> mbi_p) >>> 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 ( mbi->mods_count > CONFIG_NR_BOOTMODS ) >>> { >>> - mbi->mods_count = sizeof(module_map) * 8; >>> + mbi->mods_count = CONFIG_NR_BOOTMODS; >>> printk("Excessive multiboot modules - using the first %u >>> only\n", >>> mbi->mods_count); >>> } >> >> AFAIU, this check is to make sure that we will not overrun module_map in >> the next line: >> >> bitmap_fill(module_map, mbi->mods_count); >> >> The current definition of module_map will allow 64 modules. But you are >> allowing 32768. So I think you either want to keep the check or define >> module_map as: >> >> DECLARE_BITMAP(module_map, CONFIG_NR_BOOTMODS); > > Yes, in the RFC I had it capped to 64 and lost track of this related > changed when it was bumped to 32768 per the review discussion. Later in > the series, module_map goes away. To ensure stability at this point I > would be inclined to restore the 64 module clamp down check. Thoughts? I don't know what would a sensible value for x86. I will leave this question to the x86 maintainers. Cheers,
On 26.07.2022 20:07, Julien Grall wrote: > On 19/07/2022 17:36, Daniel P. Smith wrote: >> On 7/15/22 15:16, Julien Grall wrote: >>> On 06/07/2022 22:04, Daniel P. Smith wrote: >>>> index 498625eae0..834b1ad16b 100644 >>>> --- a/xen/arch/x86/guest/xen/pvh-boot.c >>>> +++ b/xen/arch/x86/guest/xen/pvh-boot.c >>>> @@ -32,7 +32,7 @@ bool __initdata pvh_boot; >>>> uint32_t __initdata pvh_start_info_pa; >>>> static multiboot_info_t __initdata pvh_mbi; >>>> -static module_t __initdata pvh_mbi_mods[8]; >>>> +static module_t __initdata pvh_mbi_mods[CONFIG_NR_BOOTMOD + 1]; >>> >>> What's the +1 for? >> >> I should clarify in the commit message, but the value set in >> CONFIG_NR_BOOTMOD is the max modules that Xen would accept from a >> bootloader. Xen startup code expects to be able to append Xen itself as >> the array. The +1 allocates an additional entry to store Xen in the >> array should a bootloader actually pass CONFIG_NR_BOOTMOD modules to >> Xen. There is an existing comment floating in one of these locations >> that explained it. > > This makes sense. So every use of CONFIG_NR_BOOTMOD would end up to > require +1. Is that correct? > > If yes, then I think it would be better to require CONFIG_NR_BOOTMOD to > be at minimum 1. This would reduce the risk to have different array size > again. That said, this is x86 code, so the call is for the x86 maintainers. I think the Kconfig setting should stand for "true" modules. Anywhere that x86 code internally uses one extra slot this should be expressed by an explicit "+ 1" imo. >>>> static const char *__initdata pvh_loader = "PVH Directboot"; >>>> static void __init convert_pvh_info(multiboot_info_t **mbi, >>>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c >>>> index f08b07b8de..2aa1e28c8f 100644 >>>> --- a/xen/arch/x86/setup.c >>>> +++ b/xen/arch/x86/setup.c >>>> @@ -1020,9 +1020,9 @@ void __init noreturn __start_xen(unsigned long >>>> mbi_p) >>>> 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 ( mbi->mods_count > CONFIG_NR_BOOTMODS ) >>>> { >>>> - mbi->mods_count = sizeof(module_map) * 8; >>>> + mbi->mods_count = CONFIG_NR_BOOTMODS; >>>> printk("Excessive multiboot modules - using the first %u >>>> only\n", >>>> mbi->mods_count); >>>> } >>> >>> AFAIU, this check is to make sure that we will not overrun module_map in >>> the next line: >>> >>> bitmap_fill(module_map, mbi->mods_count); >>> >>> The current definition of module_map will allow 64 modules. But you are >>> allowing 32768. So I think you either want to keep the check or define >>> module_map as: >>> >>> DECLARE_BITMAP(module_map, CONFIG_NR_BOOTMODS); >> >> Yes, in the RFC I had it capped to 64 and lost track of this related >> changed when it was bumped to 32768 per the review discussion. Later in >> the series, module_map goes away. To ensure stability at this point I >> would be inclined to restore the 64 module clamp down check. Thoughts? > > I don't know what would a sensible value for x86. I will leave this > question to the x86 maintainers. I guess I'd be fine either way, as long as the code is correct. Jan
diff --git a/xen/arch/Kconfig b/xen/arch/Kconfig index f16eb0df43..24139057be 100644 --- a/xen/arch/Kconfig +++ b/xen/arch/Kconfig @@ -17,3 +17,15 @@ config NR_CPUS For CPU cores which support Simultaneous Multi-Threading or similar technologies, this the number of logical threads which Xen will support. + +config NR_BOOTMODS + int "Maximum number of boot modules that a loader can pass" + range 1 32768 + default "8" if X86 + default "32" if ARM + help + Controls the build-time size of various arrays allocated for + parsing the boot modules passed by a loader when starting Xen. + + This is of particular interest when using Xen's hypervisor domain + capabilities such as dom0less. diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h index 2bb01ecfa8..312a3e4209 100644 --- a/xen/arch/arm/include/asm/setup.h +++ b/xen/arch/arm/include/asm/setup.h @@ -10,7 +10,8 @@ #define NR_MEM_BANKS 256 -#define MAX_MODULES 32 /* Current maximum useful modules */ +/* Current maximum useful modules */ +#define MAX_MODULES CONFIG_NR_BOOTMODS typedef enum { BOOTMOD_XEN, @@ -38,7 +39,7 @@ struct meminfo { * The domU flag is set for kernels and ramdisks of "xen,domain" nodes. * The purpose of the domU flag is to avoid getting confused in * kernel_probe, where we try to guess which is the dom0 kernel and - * initrd to be compatible with all versions of the multiboot spec. + * initrd to be compatible with all versions of the multiboot spec. */ #define BOOTMOD_MAX_CMDLINE 1024 struct bootmodule { diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index 6e65b569b0..4e1a799749 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -18,7 +18,7 @@ static multiboot_info_t __initdata mbi = { * The array size needs to be one larger than the number of modules we * support - see __start_xen(). */ -static module_t __initdata mb_modules[5]; +static module_t __initdata mb_modules[CONFIG_NR_BOOTMODS + 1]; static void __init edd_put_string(u8 *dst, size_t n, const char *src) { diff --git a/xen/arch/x86/guest/xen/pvh-boot.c b/xen/arch/x86/guest/xen/pvh-boot.c index 498625eae0..834b1ad16b 100644 --- a/xen/arch/x86/guest/xen/pvh-boot.c +++ b/xen/arch/x86/guest/xen/pvh-boot.c @@ -32,7 +32,7 @@ bool __initdata pvh_boot; uint32_t __initdata pvh_start_info_pa; static multiboot_info_t __initdata pvh_mbi; -static module_t __initdata pvh_mbi_mods[8]; +static module_t __initdata pvh_mbi_mods[CONFIG_NR_BOOTMOD + 1]; static const char *__initdata pvh_loader = "PVH Directboot"; static void __init convert_pvh_info(multiboot_info_t **mbi, diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index f08b07b8de..2aa1e28c8f 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -1020,9 +1020,9 @@ void __init noreturn __start_xen(unsigned long mbi_p) 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 ( mbi->mods_count > CONFIG_NR_BOOTMODS ) { - mbi->mods_count = sizeof(module_map) * 8; + mbi->mods_count = CONFIG_NR_BOOTMODS; printk("Excessive multiboot modules - using the first %u only\n", mbi->mods_count); }