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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 2025-04-15 08:05, 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.
It's Rule 8.4, and it is indeed blocking. To double check, a scan on a 
push containing this patch should trigger the failure.
You may transitively add an inline deviation comment or just a deviation 
with a configuration (I can help if needed), justified by the subsequent 
addition of static.
> > 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
--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253