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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

>>> +    /* 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.

>>> --- a/xen/include/xen/libfdt/libfdt-xen.h
>>> +++ b/xen/include/xen/libfdt/libfdt-xen.h
>>> @@ -13,6 +13,63 @@kkk
>>>  
>>>  #include <xen/libfdt/libfdt.h>
>>>  
>>> +static inline int __init fdt_cell_as_u32(const fdt32_t *cell)
>>
>> Why plain int here, but ...
>>
>>> +{
>>> +    return fdt32_to_cpu(*cell);
>>> +}
>>> +
>>> +static inline uint64_t  __init fdt_cell_as_u64(const fdt32_t *cell)
>>
>> ... a fixed-width and unsigned type here? Question is whether the former
>> helper is really warranted.
>>
>> Also nit: Stray double blank.
>>
>>> +{
>>> +    return ((uint64_t)fdt32_to_cpu(cell[0]) << 32) | fdt32_to_cpu(cell[1]);
>>
>> That is - uniformly big endian?
> 
> These helpers are disappearing, so it doesn't matter. This is basically
> an open coded:
> 
>   fdt64_to_cpu(*(const fdt64_t *)fdt32)
> 
> And, yes. DTBs are standardised as having big-endian properties, for
> better or worse :/
> 
>>
>>> +}
>>
>> Marking such relatively generic inline functions __init is also somewhat
>> risky. 
> 
> They were originally in domain-builder/fdt.c and moved here as a result
> of a request to have them on libfdt. libfdt proved to be somewhat
> annoying because it would be hard to distinguish accessors for the
> flattened and the unflattened tree.
> 
> I'd personally have them in domain-builder instead, where they are used.
> Should they be needed somewhere else, we can always fator them out
> somewhere else.
> 
> Thoughts?

As long as they're needed only by domain-builder, it's probably fine to have
them just there.

Jan