Re: [PATCH v3 08/16] x86/hyperlaunch: Add helpers to locate multiboot modules

[Alejandro Vallejo <agarciav@xxxxxxx>]

On Tue Apr 15, 2025 at 7:05 AM BST, Jan Beulich wrote:
> On 14.04.2025 20:01, Alejandro Vallejo wrote:
>> On Mon Apr 14, 2025 at 4:05 PM BST, Jan Beulich wrote:
>>> On 14.04.2025 15:37, Alejandro Vallejo wrote:
>>>> On Thu Apr 10, 2025 at 11:42 AM BST, Jan Beulich wrote:
>>>>> On 08.04.2025 18:07, Alejandro Vallejo wrote:
>>>>>> +/*
>>>>>> + * Locate a multiboot module given its node offset in the FDT.
>>>>>> + *
>>>>>> + * The module location may be given via either FDT property:
>>>>>> + *     * reg = <address, size>
>>>>>> + *         * Mutates `bi` to append the module.
>>>>>> + *     * module-index = <idx>
>>>>>> + *         * Leaves `bi` unchanged.
>>>>>> + *
>>>>>> + * @param fdt           Pointer to the full FDT.
>>>>>> + * @param node          Offset for the module node.
>>>>>> + * @param address_cells Number of 4-octet cells that make up an 
>>>>>> "address".
>>>>>> + * @param size_cells    Number of 4-octet cells that make up a "size".
>>>>>> + * @param bi[inout]     Xen's representation of the boot parameters.
>>>>>> + * @return              -EINVAL on malformed nodes, otherwise
>>>>>> + *                      index inside `bi->mods`
>>>>>> + */
>>>>>> +int __init fdt_read_multiboot_module(const void *fdt, int node,
>>>>>> +                                     int address_cells, int size_cells,
>>>>>> +                                     struct boot_info *bi)
>>>>>
>>>>> Functions without callers and non-static ones without declarations are
>>>>> disliked by Misra.
>>>>
>>>> Can't do much about it if I want them to stand alone in a single patch.
>>>> Otherwise the following ones become quite unwieldy to look at. All I can
>>>> say is that this function becomes static and with a caller on the next
>>>> patch.
>>>
>>> Which means you need to touch this again anyway. Perhaps we need a Misra
>>> deviation for __maybe_unused functions / data, in which case you could
>>> use that here and strip it along with making the function static. Cc-ing
>>> Bugseng folks.
>> 
>> It's a transient violation, sure. Do we care about transient MISRA
>> violations though? I understand the importance of bisectability, but
>> AUIU MISRA compliance matters to the extent that that the tip is
>> compliant rather than the intermediate steps?
>
> Thing is that quite a few rules are blocking now. I haven't checked whether
> the one here (already) is; if it isn't, we can't exclude it will be by the
> time this patch is committed. If then the next patch isn't committed
> together with it, we'd face a CI failure.
>
>> Another option would be to fold them this patch and the next together
>> after both get their R-by. As I said, I assumed you'd rather see them in
>> isolation for purposes of review.
>
> As it looks it's all plain code additions, so reviewability would merely
> mildly suffer from patch size. But afaict there would be no loss of clarity.
>
>>>>>> +    /* Otherwise location given as a `reg` property. */
>>>>>> +    prop = fdt_get_property(fdt, node, "reg", NULL);
>>>>>> +
>>>>>> +    if ( !prop )
>>>>>> +    {
>>>>>> +        printk("  No location for multiboot,module\n");
>>>>>> +        return -EINVAL;
>>>>>> +    }
>>>>>> +    if ( fdt_get_property(fdt, node, "module-index", NULL) )
>>>>>> +    {
>>>>>> +        printk("  Location of multiboot,module defined multiple 
>>>>>> times\n");
>>>>>> +        return -EINVAL;
>>>>>> +    }
>>>>>> +
>>>>>> +    ret = read_fdt_prop_as_reg(prop, address_cells, size_cells, &addr, 
>>>>>> &size);
>>>>>> +
>>>>>> +    if ( ret < 0 )
>>>>>> +    {
>>>>>> +        printk("  Failed reading reg for multiboot,module\n");
>>>>>> +        return -EINVAL;
>>>>>> +    }
>>>>>> +
>>>>>> +    idx = bi->nr_modules + 1;
>>>>>
>>>>> This at least looks like an off-by-one. If the addition of 1 is really
>>>>> intended, I think it needs commenting on.
>>>>
>>>> Seems to be, yes. The underlying array is a bit bizarre. It's sizes as
>>>> MAX_NR_BOOTMODS + 1, with the first one being the DTB itself. I guess
>>>> the intent was to take it into account, but bi->nr_modules is
>>>> initialised to the number of multiboot modules, so it SHOULD be already
>>>> taking it into account.
>>>>
>>>> Also, the logic for bounds checking seems... off (because of the + 1 I
>>>> mentioned before). Or at least confusing, so I've moved to using
>>>> ARRAY_SIZE(bi->mods) rather than explicitly comparing against
>>>> MAX_NR_BOOTMODS.
>>>>
>>>> The array is MAX_NR_BOOTMODS + 1 in length, so it's just more cognitive
>>>> load than I'm comfortable with.
>>>
>>> If I'm not mistaken the +1 is inherited from the modules array we had in
>>> the past, where we wanted 1 extra slot for Xen itself. Hence before you
>>> move to using ARRAY_SIZE() everywhere it needs to really be clear what
>>> the +1 here is used for.
>> 
>> Ew.  Ok, just looked at the code in multiboot_fill_boot_info and indeed
>> the arrangement is for all multiboot modules to be in front, and Xen to
>> be appended. But bi->nr_modules only lists multiboot modules, so
>> increasing that value is therefore not enough (or
>> next_boot_module_index() would fail).
>> 
>> I need to have a proper read on how this is all stitched together.  I
>> may simply swap BOOTMOD_XEN with the next entry on append. Though my
>> preference would be to _not_ have Xen as part of the module list to
>> begin with. Before boot_info that was probably a place as good as any,
>> but this would be much better off in a dedicated field.
>> 
>> I don't see much in terms of usage though. Why is it being added at all?
>
> For hyperlaunch I fear it's you who needs to answer this question. For
> pre-hyperlaunch it's (primarily?) for consider_modules(), iirc. See two
> of the three comments ahead of its non-recursive invocations.
>
> Jan

There's no specific need for it on hyperlaunch AFAIK. Fixing
consider_modules to not require Xen being on the list of modules is easy
enough on both arm and x86 (it's a matter of passing the boot_info in
full rather than array + size), but I fear there may be more instances of
such checks.

I'll let it be for the time being and take a mental note to untangle
it later on. For this I'll simply ensure the append logic maintains Xen
at the back, as a sentinel of sorts for the module list, and document
that behaviour in the boot_info itself.

Cheers,
Alejandro