Re: [RFC PATCH 1/4] kconfig: allow configuration of maximum modules

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx> 
> >>>> 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@xxxxxxxxxxxxxxxxxxxx>
> >>>> ---
> >>>> 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.