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

[Jan Beulich <jbeulich@xxxxxxxx>]

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