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
