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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 2025-04-14 17:05, 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.
There is already an exception for __maybe_unused on labels (Rule 2.6). 
In principle it could be easily extended to encompass unused functions 
(which are verified by another rule), with a suitable rationale.
> > > > +    /* 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
--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253