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

[Jan Beulich <jbeulich@xxxxxxxx>]

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