Re: [PATCH v1 01/18] kconfig: allow configuration of maximum modules

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

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.