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
