| Message ID | 20220531024127.23669-2-dpsmith@apertussolutions.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introducing a common representation of boot info | expand |
Hi Daniel, > On 31 May 2022, at 03:41, Daniel P. Smith <dpsmith@apertussolutions.com> 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> > --- > 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..57b14e22c9 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 64 > + 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. This seems to be a spurious change. With that fixed, for the arm part: Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> Cheers Bertrand
Hi, On 31/05/2022 03:41, Daniel P. Smith wrote: > diff --git a/xen/arch/Kconfig b/xen/arch/Kconfig > index f16eb0df43..57b14e22c9 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 64 OOI, any reason to limit the size? > + 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 There are only a handful number of use of MAX_MODULES in Arm. So I would prefer if we replace all the use with CONFIG_NR_BOOTMODS. Cheers,
On 5/31/22 05:07, Bertrand Marquis wrote: > Hi Daniel, Greetings Bertrand. >> On 31 May 2022, at 03:41, Daniel P. Smith <dpsmith@apertussolutions.com> 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> >> --- >> 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..57b14e22c9 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 64 >> + 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. > > This seems to be a spurious change. I have been trying to clean up trailing white space when I see it nearby. I can drop this one if that is desired. > With that fixed, for the arm part: > Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> Thank you, will add it in when I respin it for submission. v/r, dps
Hi Daniel, > On 31 May 2022, at 11:45, Daniel P. Smith <dpsmith@apertussolutions.com> wrote: > > On 5/31/22 05:07, Bertrand Marquis wrote: >> Hi Daniel, > > Greetings Bertrand. > >>> On 31 May 2022, at 03:41, Daniel P. Smith <dpsmith@apertussolutions.com> 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> >>> --- >>> 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..57b14e22c9 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 64 >>> + 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. >> >> This seems to be a spurious change. > > I have been trying to clean up trailing white space when I see it > nearby. I can drop this one if that is desired. If this is wanted this is ok, just mention it in the commit message so that we know it was on purpose. > >> With that fixed, for the arm part: >> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> > > Thank you, will add it in when I respin it for submission. Cheers Bertrand > > v/r, > dps
On 5/31/22 05:25, Julien Grall wrote: > Hi, > > On 31/05/2022 03:41, Daniel P. Smith wrote: >> diff --git a/xen/arch/Kconfig b/xen/arch/Kconfig >> index f16eb0df43..57b14e22c9 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 64 > > OOI, any reason to limit the size? I modelled this entirely after NR_CPUS, which applied a limit, and it seemed reasonable to me at the time. I choose 64 since it was double currently what Arm had set for MAX_MODULES. As such, I have no hard reason for there to be a limit. It can easily be removed or adjusted to whatever the reviewers feel would be appropriate. >> + 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 > > There are only a handful number of use of MAX_MODULES in Arm. So I would > prefer if we replace all the use with CONFIG_NR_BOOTMODS. No problem, as I stated above, I mimicked what was done for NR_CPUS which did a similar #define for MAX_CPUS. v/r, dps
On 5/31/22 06:49, Bertrand Marquis wrote: > Hi Daniel, > >> On 31 May 2022, at 11:45, Daniel P. Smith <dpsmith@apertussolutions.com> wrote: >> >> On 5/31/22 05:07, Bertrand Marquis wrote: >>> Hi Daniel, >> >> Greetings Bertrand. >> >>>> On 31 May 2022, at 03:41, Daniel P. Smith <dpsmith@apertussolutions.com> 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> >>>> --- >>>> 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..57b14e22c9 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 64 >>>> + 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. >>> >>> This seems to be a spurious change. >> >> I have been trying to clean up trailing white space when I see it >> nearby. I can drop this one if that is desired. > > If this is wanted this is ok, just mention it in the commit message so that we know it was on purpose. Understood, I will update the commit message on this one and the others where there is done. >> >>> With that fixed, for the arm part: >>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> >> >> Thank you, will add it in when I respin it for submission. > > Cheers > Bertrand > >> >> v/r, >> dps > v/r, dps
On Tue, May 31, 2022 at 06:45:52AM -0400, Daniel P. Smith wrote: > On 5/31/22 05:07, Bertrand Marquis wrote: > > Hi Daniel, > > Greetings Bertrand. > > >> On 31 May 2022, at 03:41, Daniel P. Smith <dpsmith@apertussolutions.com> 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> > >> --- > >> 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..57b14e22c9 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 64 > >> + 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. > > > > This seems to be a spurious change. > > I have been trying to clean up trailing white space when I see it > nearby. I can drop this one if that is desired. IMO it's best if such white space removal is only done when already modifying the line, or else it makes it harder to track changes when using `git blame` for example (not likely in this case since it's a multi line comment). Thanks, Roger.
On 31.05.2022 15:52, Roger Pau Monné wrote: > On Tue, May 31, 2022 at 06:45:52AM -0400, Daniel P. Smith wrote: >> On 5/31/22 05:07, Bertrand Marquis wrote: >>> Hi Daniel, >> >> Greetings Bertrand. >> >>>> On 31 May 2022, at 03:41, Daniel P. Smith <dpsmith@apertussolutions.com> 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> >>>> --- >>>> 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..57b14e22c9 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 64 >>>> + 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. >>> >>> This seems to be a spurious change. >> >> I have been trying to clean up trailing white space when I see it >> nearby. I can drop this one if that is desired. > > IMO it's best if such white space removal is only done when already > modifying the line, or else it makes it harder to track changes when > using `git blame` for example (not likely in this case since it's a > multi line comment). +1 Jan
> On 31 May 2022, at 14:52, Roger Pau Monne <roger.pau@citrix.com> wrote: > > On Tue, May 31, 2022 at 06:45:52AM -0400, Daniel P. Smith wrote: >> On 5/31/22 05:07, Bertrand Marquis wrote: >>> Hi Daniel, >> >> Greetings Bertrand. >> >>>> On 31 May 2022, at 03:41, Daniel P. Smith <dpsmith@apertussolutions.com> 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> >>>> --- >>>> 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..57b14e22c9 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 64 >>>> + 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. >>> >>> This seems to be a spurious change. >> >> I have been trying to clean up trailing white space when I see it >> nearby. I can drop this one if that is desired. > > IMO it's best if such white space removal is only done when already > modifying the line, or else it makes it harder to track changes when > using `git blame` for example (not likely in this case since it's a > multi line comment). The down side of this is that you can’t use “automatically remove trailing whitespace on save” features of some editors. Without such automation, I introduce loads of trailing whitespace. With such automation, I end up removing random trailing whitespace as I happen to touch files. I’ve always done this by just adding “While here, remove some trailing whitespace” to the commit message, and there haven’t been any complaints. If we actually care about trailing whitespace, then I think we should accept random fix-ups as files are touched. OTOH if we want to avoid random fix-ups, we should remove the aversion to trailing whitespace. -George
On Wed, Jun 01, 2022 at 07:40:12AM +0000, George Dunlap wrote: > > > > On 31 May 2022, at 14:52, Roger Pau Monne <roger.pau@citrix.com> wrote: > > > > On Tue, May 31, 2022 at 06:45:52AM -0400, Daniel P. Smith wrote: > >> On 5/31/22 05:07, Bertrand Marquis wrote: > >>> Hi Daniel, > >> > >> Greetings Bertrand. > >> > >>>> On 31 May 2022, at 03:41, Daniel P. Smith <dpsmith@apertussolutions.com> 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> > >>>> --- > >>>> 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..57b14e22c9 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 64 > >>>> + 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. > >>> > >>> This seems to be a spurious change. > >> > >> I have been trying to clean up trailing white space when I see it > >> nearby. I can drop this one if that is desired. > > > > IMO it's best if such white space removal is only done when already > > modifying the line, or else it makes it harder to track changes when > > using `git blame` for example (not likely in this case since it's a > > multi line comment). > > The down side of this is that you can’t use “automatically remove trailing whitespace on save” features of some editors. > > Without such automation, I introduce loads of trailing whitespace. With such automation, I end up removing random trailing whitespace as I happen to touch files. I’ve always done this by just adding “While here, remove some trailing whitespace” to the commit message, and there haven’t been any complaints. FWIW, I have an editor feature that highlights trailing whitespace, but doesn't remove it. As said, I find it cumbersome to have to go through more jumps while using `git blame` or similar, just because of unrelated cleanups. IMO I don't think it's good practice to wholesale replace all trailing whitespace from a file as part of an unrelated change. If people do think this is fine I'm OK with it, but it's not my preference. Just raised this because such changes make it harder to use `git blame` IMO (have to remember to use -w, but that won't help people using the web interface for example). Thanks, Roger.
On 31/05/2022 11:53, Daniel P. Smith wrote: > On 5/31/22 05:25, Julien Grall wrote: >> Hi, >> >> On 31/05/2022 03:41, Daniel P. Smith wrote: >>> diff --git a/xen/arch/Kconfig b/xen/arch/Kconfig >>> index f16eb0df43..57b14e22c9 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 64 >> >> OOI, any reason to limit the size? > > I modelled this entirely after NR_CPUS, which applied a limit The limit for NR_CPUS makes sense because there are scalability issues after that (although 4095 seems quite high) and/or the HW impose a limit. > , and it > seemed reasonable to me at the time. I choose 64 since it was double > currently what Arm had set for MAX_MODULES. As such, I have no hard > reason for there to be a limit. It can easily be removed or adjusted to > whatever the reviewers feel would be appropriate. Ok. In which case I would drop the limit beause it also prevent a users to create more 64 dom0less domUs (actually a bit less because some modules are used by Xen). I don't think there are a strong reason to prevent that, right? >>> + 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 >> >> There are only a handful number of use of MAX_MODULES in Arm. So I would >> prefer if we replace all the use with CONFIG_NR_BOOTMODS. > > No problem, as I stated above, I mimicked what was done for NR_CPUS > which did a similar #define for MAX_CPUS. Thanks! Cheers,
On 01.06.2022 19:35, Julien Grall wrote: > > > On 31/05/2022 11:53, Daniel P. Smith wrote: >> On 5/31/22 05:25, Julien Grall wrote: >>> Hi, >>> >>> On 31/05/2022 03:41, Daniel P. Smith wrote: >>>> diff --git a/xen/arch/Kconfig b/xen/arch/Kconfig >>>> index f16eb0df43..57b14e22c9 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 64 >>> >>> OOI, any reason to limit the size? >> >> I modelled this entirely after NR_CPUS, which applied a limit > > The limit for NR_CPUS makes sense because there are scalability issues > after that (although 4095 seems quite high) and/or the HW impose a limit. The 4095 is actually a software limit (due to how spinlocks are implemented). >> , and it >> seemed reasonable to me at the time. I choose 64 since it was double >> currently what Arm had set for MAX_MODULES. As such, I have no hard >> reason for there to be a limit. It can easily be removed or adjusted to >> whatever the reviewers feel would be appropriate. > > Ok. In which case I would drop the limit beause it also prevent a users > to create more 64 dom0less domUs (actually a bit less because some > modules are used by Xen). I don't think there are a strong reason to > prevent that, right? At least as per the kconfig language doc the upper bound is not optional, so if a range is specified (which I think it should be, to enforce the lower limit of 1) and upper bound is needed. To address your concern with dom0less - 32768 maybe? Jan
On 31.05.2022 04:41, Daniel P. Smith wrote: > --- 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]; Did this build successfully for you? Looks like the trailing S is missing ... > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -1017,9 +1017,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); > } You'll want to accompany this by a BUILD_BUG_ON() checking that the Kconfig value doesn't exceed the capacity of module_map. Without that it could be quite hard to notice that a bump of the upper bound in the Kconfig file would result in an issue here. Jan
On 01/06/2022 10:06, Roger Pau Monné wrote: > On Wed, Jun 01, 2022 at 07:40:12AM +0000, George Dunlap wrote: >> The down side of this is that you can’t use “automatically remove trailing whitespace on save” features of some editors. >> >> Without such automation, I introduce loads of trailing whitespace. With such automation, I end up removing random trailing whitespace as I happen to touch files. I’ve always done this by just adding “While here, remove some trailing whitespace” to the commit message, and there haven’t been any complaints. > > FWIW, I have an editor feature that highlights trailing whitespace, > but doesn't remove it. > > As said, I find it cumbersome to have to go through more jumps while > using `git blame` or similar, just because of unrelated cleanups. > > IMO I don't think it's good practice to wholesale replace all trailing > whitespace from a file as part of an unrelated change. +1 Cheers,
Hi, On 02/06/2022 09:49, Jan Beulich wrote: > On 01.06.2022 19:35, Julien Grall wrote: >> >> >> On 31/05/2022 11:53, Daniel P. Smith wrote: >>> On 5/31/22 05:25, Julien Grall wrote: >>>> Hi, >>>> >>>> On 31/05/2022 03:41, Daniel P. Smith wrote: >>>>> diff --git a/xen/arch/Kconfig b/xen/arch/Kconfig >>>>> index f16eb0df43..57b14e22c9 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 64 >>>> >>>> OOI, any reason to limit the size? >>> >>> I modelled this entirely after NR_CPUS, which applied a limit >> >> The limit for NR_CPUS makes sense because there are scalability issues >> after that (although 4095 seems quite high) and/or the HW impose a limit. > > The 4095 is actually a software limit (due to how spinlocks are > implemented). > >>> , and it >>> seemed reasonable to me at the time. I choose 64 since it was double >>> currently what Arm had set for MAX_MODULES. As such, I have no hard >>> reason for there to be a limit. It can easily be removed or adjusted to >>> whatever the reviewers feel would be appropriate. >> >> Ok. In which case I would drop the limit beause it also prevent a users >> to create more 64 dom0less domUs (actually a bit less because some >> modules are used by Xen). I don't think there are a strong reason to >> prevent that, right? > > At least as per the kconfig language doc the upper bound is not > optional, so if a range is specified (which I think it should be, > to enforce the lower limit of 1) and upper bound is needed. To > address your concern with dom0less - 32768 maybe? I am fine with that. Cheers,
diff --git a/xen/arch/Kconfig b/xen/arch/Kconfig index f16eb0df43..57b14e22c9 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 64 + 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 ed67b50c9d..3f5e602e00 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -1017,9 +1017,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); }
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> --- 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(-)