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

Commit Message

Alejandro Vallejo April 8, 2025, 4:07 p.m. UTC

Hyperlaunch mandates either a reg or module-index DT prop on nodes that
contain `multiboot,module" under their "compatible" prop. This patch
introduces a helper to generically find such index, appending the module
to the list of modules if it wasn't already (i.e: because it's given via
the "reg" prop).

Signed-off-by: Jason Andryuk <[e-mail]>
Signed-off-by: Alejandro Vallejo <[e-mail]>
---
v3:
    * New on v3.
    * Subsumes much of the dup code between kernel/initrd patches.
    * Changes previous behaviour in v2 to look into "reg" and
      "module-index" props, rather than just index.
    * Use addr_cells/size_cells rather than size_size
---
 xen/arch/x86/domain-builder/fdt.c   | 142 ++++++++++++++++++++++++++++
 xen/arch/x86/domain-builder/fdt.h   |   2 +
 xen/include/xen/libfdt/libfdt-xen.h |  57 +++++++++++
 3 files changed, 201 insertions(+)

Comments

Jan Beulich April 10, 2025, 10:42 a.m. UTC | #1

On 08.04.2025 18:07, Alejandro Vallejo wrote:
> --- a/xen/arch/x86/domain-builder/fdt.c
> +++ b/xen/arch/x86/domain-builder/fdt.c
> @@ -13,6 +13,148 @@
>  
>  #include "fdt.h"
>  
> +/*
> + * Unpacks a "reg" property into its address and size constituents.
> + *
> + * @param prop          Pointer to an FDT "reg" property.
> + * @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 p_addr[out]   Address encoded in the property.
> + * @param p_size[out]   Size encoded in the property.
> + * @returns             -EINVAL on malformed property, 0 otherwise.
> + */
> +static int __init read_fdt_prop_as_reg(const struct fdt_property *prop,
> +                                       int address_cells, int size_cells,
> +                                       uint64_t *p_addr, uint64_t *p_size)
> +{
> +    const fdt32_t *cell = (const fdt32_t *)prop->data;
> +    uint64_t addr, size;
> +
> +    if ( fdt32_to_cpu(prop->len) !=
> +         (address_cells + size_cells) * sizeof(*cell) )
> +    {
> +        printk("  Cannot read reg %lu+%lu from prop len %u\n",
> +            address_cells * sizeof(*cell), size_cells * sizeof(*cell),
> +            fdt32_to_cpu(prop->len));
> +        return -EINVAL;
> +    }
> +
> +    switch ( address_cells ) {

Nit: Brace on its own line please.

> +    case 1:
> +        addr = fdt32_to_cpu(*cell);
> +        break;
> +    case 2:
> +        addr = fdt64_to_cpu(*(const fdt64_t *)cell);
> +        break;
> +    default:
> +        printk("  unsupported sized address_cells\n");

Depending on how likely this or ...

> +        return -EINVAL;
> +    }
> +
> +    cell += address_cells;
> +    switch ( size_cells ) {
> +    case 1:
> +        size = fdt32_to_cpu(*cell);
> +        break;
> +    case 2:
> +        size = fdt64_to_cpu(*(const fdt64_t *)cell);
> +        break;
> +    default:
> +        printk("  unsupported sized size_cells\n");

... this path is to be hit, perhaps also log the bogus size? Then again, this
being passed in, isn't it an internal error if the wrong size makes it here?
I.e. rather use ASSERT_UNREACHABLE()?

> +        return -EINVAL;
> +    }
> +
> +    *p_addr = addr;
> +    *p_size = size;
> +
> +    return 0;
> +}

The function as a whole looks somewhat similar to fdt_get_reg_prop(). What's
the deal?

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

> +{
> +    const struct fdt_property *prop;
> +    uint64_t addr, size;
> +    int ret;
> +    int idx;
> +
> +    ASSERT(!fdt_node_check_compatible(fdt, node, "multiboot,module"));
> +
> +    /* Location given as a `module-index` property. */
> +    prop = fdt_get_property(fdt, node, "module-index", NULL);
> +
> +    if ( prop )
> +    {
> +        if ( fdt_get_property(fdt, node, "reg", NULL) )
> +        {
> +            printk("  Location of multiboot,module defined multiple times\n");
> +            return -EINVAL;
> +        }
> +        return fdt_cell_as_u32((const fdt32_t *)prop->data);

No concerns here of there being less than 4 bytes of data?

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

> +    if ( idx > MAX_NR_BOOTMODS )
> +    {
> +        /*
> +         * MAX_NR_BOOTMODS cannot exceed the max for MB1, represented by 32bits,
> +         * thus the cast down to a u32 will be safe due to the prior check.
> +         */
> +        BUILD_BUG_ON(MAX_NR_BOOTMODS >= (uint64_t)UINT32_MAX);

Because of idx being a signed quantity, isn't INT_MAX the required upper
bound? The latest then the somewhat odd cast should also be possible to
drop.

> +        printk("  idx %d exceeds maximum boot modules\n", idx);

Perhaps include STR(MAX_NR_BOOTMODS) as well?

> --- a/xen/include/xen/libfdt/libfdt-xen.h
> +++ b/xen/include/xen/libfdt/libfdt-xen.h
> @@ -13,6 +13,63 @@
>  
>  #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?

> +}

Marking such relatively generic inline functions __init is also somewhat
risky. 

> +/*
> + * Property: reg
> + *
> + * Defined in Section 2.3.6 of the Device Tree Specification is the "reg"
> + * standard property. The property is a prop-encoded-array that is encoded as
> + * an arbitrary number of (address, size) pairs.  We only extract a single
> + * pair since that is what is used in practice.
> + */
> +static inline int __init fdt_get_reg_prop(
> +    const void *fdt, int node, unsigned int addr_cells, unsigned int size_cells,
> +    uint64_t *addr, uint64_t *size)
> +{
> +    int ret;
> +    const struct fdt_property *prop;
> +    fdt32_t *cell;
> +
> +    /* FDT spec max size is 4 (128bit int), but largest arch int size is 64 */
> +    if ( size_cells > 2 || addr_cells > 2 )
> +        return -EINVAL;
> +
> +    prop = fdt_get_property(fdt, node, "reg", &ret);
> +    if ( !prop || ret < sizeof(u32) )

No uses of u32 et al in new code please. Question anyway is whether this isn't
meant to be sizeof(*cell) like you have it ...

> +        return ret < 0 ? ret : -EINVAL;
> +
> +    if ( fdt32_to_cpu(prop->len) !=
> +	 ((size_cells + addr_cells) * sizeof(*cell)) )

... here. Or maybe it's to be sizeof(prop->len)?

Also nit: Hard tab slipped in.

> +        return -EINVAL;
> +
> +    cell = (fdt32_t *)prop->data;
> +
> +    /* read address field */
> +    if ( addr_cells == 1 )
> +        *addr = fdt_cell_as_u32(cell);
> +    else
> +        *addr = fdt_cell_as_u64(cell);
> +
> +    cell += addr_cells;
> +
> +    /* read size field */
> +    if ( size_cells == 1 )
> +        *size = fdt_cell_as_u32(cell);
> +    else
> +        *size = fdt_cell_as_u64(cell);
> +
> +    return 0;
> +}

Does this really want/need to be an inline function?

Jan

Alejandro Vallejo April 14, 2025, 1:37 p.m. UTC | #2

On Thu Apr 10, 2025 at 11:42 AM BST, Jan Beulich wrote:
> On 08.04.2025 18:07, Alejandro Vallejo wrote:
>> --- a/xen/arch/x86/domain-builder/fdt.c
>> +++ b/xen/arch/x86/domain-builder/fdt.c
>> @@ -13,6 +13,148 @@
>>  
>>  #include "fdt.h"
>>  
>> +/*
>> + * Unpacks a "reg" property into its address and size constituents.
>> + *
>> + * @param prop          Pointer to an FDT "reg" property.
>> + * @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 p_addr[out]   Address encoded in the property.
>> + * @param p_size[out]   Size encoded in the property.
>> + * @returns             -EINVAL on malformed property, 0 otherwise.
>> + */
>> +static int __init read_fdt_prop_as_reg(const struct fdt_property *prop,
>> +                                       int address_cells, int size_cells,
>> +                                       uint64_t *p_addr, uint64_t *p_size)
>> +{
>> +    const fdt32_t *cell = (const fdt32_t *)prop->data;
>> +    uint64_t addr, size;
>> +
>> +    if ( fdt32_to_cpu(prop->len) !=
>> +         (address_cells + size_cells) * sizeof(*cell) )
>> +    {
>> +        printk("  Cannot read reg %lu+%lu from prop len %u\n",
>> +            address_cells * sizeof(*cell), size_cells * sizeof(*cell),
>> +            fdt32_to_cpu(prop->len));
>> +        return -EINVAL;
>> +    }
>> +
>> +    switch ( address_cells ) {
>
> Nit: Brace on its own line please.

Sure

>
>> +    case 1:
>> +        addr = fdt32_to_cpu(*cell);
>> +        break;
>> +    case 2:
>> +        addr = fdt64_to_cpu(*(const fdt64_t *)cell);
>> +        break;
>> +    default:
>> +        printk("  unsupported sized address_cells\n");
>
> Depending on how likely this or ...
>
>> +        return -EINVAL;
>> +    }
>> +
>> +    cell += address_cells;
>> +    switch ( size_cells ) {
>> +    case 1:
>> +        size = fdt32_to_cpu(*cell);
>> +        break;
>> +    case 2:
>> +        size = fdt64_to_cpu(*(const fdt64_t *)cell);
>> +        break;
>> +    default:
>> +        printk("  unsupported sized size_cells\n");
>
> ... this path is to be hit, perhaps also log the bogus size? Then again, this
> being passed in, isn't it an internal error if the wrong size makes it here?
> I.e. rather use ASSERT_UNREACHABLE()?

*_cells are DTB properties, so it's more of an input error.

Ack to log the sizes, will do.

>
>> +        return -EINVAL;
>> +    }
>> +
>> +    *p_addr = addr;
>> +    *p_size = size;
>> +
>> +    return 0;
>> +}
>
> The function as a whole looks somewhat similar to fdt_get_reg_prop(). What's
> the deal?

The latter shouldn't be there. It's leftover from code motion and a
merge.

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

>
>> +{
>> +    const struct fdt_property *prop;
>> +    uint64_t addr, size;
>> +    int ret;
>> +    int idx;
>> +
>> +    ASSERT(!fdt_node_check_compatible(fdt, node, "multiboot,module"));
>> +
>> +    /* Location given as a `module-index` property. */
>> +    prop = fdt_get_property(fdt, node, "module-index", NULL);
>> +
>> +    if ( prop )
>> +    {
>> +        if ( fdt_get_property(fdt, node, "reg", NULL) )
>> +        {
>> +            printk("  Location of multiboot,module defined multiple times\n");
>> +            return -EINVAL;
>> +        }
>> +        return fdt_cell_as_u32((const fdt32_t *)prop->data);
>
> No concerns here of there being less than 4 bytes of data?

v4 moves the property accessors earlier so this is a safe access.

>
>> +    }
>> +
>> +    /* 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 ( idx > MAX_NR_BOOTMODS )
>> +    {
>> +        /*
>> +         * MAX_NR_BOOTMODS cannot exceed the max for MB1, represented by 32bits,
>> +         * thus the cast down to a u32 will be safe due to the prior check.
>> +         */
>> +        BUILD_BUG_ON(MAX_NR_BOOTMODS >= (uint64_t)UINT32_MAX);
>
> Because of idx being a signed quantity, isn't INT_MAX the required upper
> bound? The latest then the somewhat odd cast should also be possible to
> drop.

It is, yes. Having a theoretical limit of 2**31-1 rather than 2**32-1 doesn't worry
me in the slightest.

>
>> +        printk("  idx %d exceeds maximum boot modules\n", idx);
>
> Perhaps include STR(MAX_NR_BOOTMODS) as well?
I'll print ARRAY_SIZE(bi->mods) instead. Otherwise it will be very
confusing.

>
>> --- 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?

>
>> +/*
>> + * Property: reg
>> + *
>> + * Defined in Section 2.3.6 of the Device Tree Specification is the "reg"
>> + * standard property. The property is a prop-encoded-array that is encoded as
>> + * an arbitrary number of (address, size) pairs.  We only extract a single
>> + * pair since that is what is used in practice.
>> + */
>> +static inline int __init fdt_get_reg_prop(
>> +    const void *fdt, int node, unsigned int addr_cells, unsigned int size_cells,
>> +    uint64_t *addr, uint64_t *size)
>> +{
>> +    int ret;
>> +    const struct fdt_property *prop;
>> +    fdt32_t *cell;
>> +
>> +    /* FDT spec max size is 4 (128bit int), but largest arch int size is 64 */
>> +    if ( size_cells > 2 || addr_cells > 2 )
>> +        return -EINVAL;
>> +
>> +    prop = fdt_get_property(fdt, node, "reg", &ret);
>> +    if ( !prop || ret < sizeof(u32) )
>
> No uses of u32 et al in new code please. Question anyway is whether this isn't
> meant to be sizeof(*cell) like you have it ...
>
>> +        return ret < 0 ? ret : -EINVAL;
>> +
>> +    if ( fdt32_to_cpu(prop->len) !=
>> +	 ((size_cells + addr_cells) * sizeof(*cell)) )
>
> ... here. Or maybe it's to be sizeof(prop->len)?
>
> Also nit: Hard tab slipped in.
>
>> +        return -EINVAL;
>> +
>> +    cell = (fdt32_t *)prop->data;
>> +
>> +    /* read address field */
>> +    if ( addr_cells == 1 )
>> +        *addr = fdt_cell_as_u32(cell);
>> +    else
>> +        *addr = fdt_cell_as_u64(cell);
>> +
>> +    cell += addr_cells;
>> +
>> +    /* read size field */
>> +    if ( size_cells == 1 )
>> +        *size = fdt_cell_as_u32(cell);
>> +    else
>> +        *size = fdt_cell_as_u64(cell);
>> +
>> +    return 0;
>> +}
>
> Does this really want/need to be an inline function?
>
> Jan

This function is gone in v4.

Cheers,
Alejandro

Jan Beulich April 14, 2025, 3:05 p.m. UTC | #3

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

Alejandro Vallejo April 14, 2025, 6:01 p.m. UTC | #4

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?

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.

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

Cheers,
Alejandro

Nicola Vetrini April 14, 2025, 7:09 p.m. UTC | #5

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

Jan Beulich April 15, 2025, 6:05 a.m. UTC | #6

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

Alejandro Vallejo April 15, 2025, 11:30 a.m. UTC | #7

On Tue Apr 15, 2025 at 7:05 AM BST, 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.
>
>> 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

There's no specific need for it on hyperlaunch AFAIK. Fixing
consider_modules to not require Xen being on the list of modules is easy
enough on both arm and x86 (it's a matter of passing the boot_info in
full rather than array + size), but I fear there may be more instances of
such checks.

I'll let it be for the time being and take a mental note to untangle
it later on. For this I'll simply ensure the append logic maintains Xen
at the back, as a sentinel of sorts for the module list, and document
that behaviour in the boot_info itself.

Cheers,
Alejandro

Nicola Vetrini April 16, 2025, 4:55 p.m. UTC | #8

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

Alejandro Vallejo April 17, 2025, 11:50 a.m. UTC | #9

On Wed Apr 16, 2025 at 5:55 PM BST, Nicola Vetrini wrote:
> 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:
>>>>>> 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.

Thanks for the context!

If I'm going to add something and remove it later might as well add the
missing declaration and remove it when static-ifying the function.
Particularly because I don't see a suitable x in SAFE-x-safe to use. (1
or 13 may work, but they hardly reflect what's being done.)

My bad for (mis)assuming transient states merely required bisectability
rather than full MISRA compliance. 

Cheers,
Alejandro

Patch

diff --git a/xen/arch/x86/domain-builder/fdt.c b/xen/arch/x86/domain-builder/fdt.c
index 4c5b7747f5..9ebc8fd0e4 100644
--- a/xen/arch/x86/domain-builder/fdt.c
+++ b/xen/arch/x86/domain-builder/fdt.c
@@ -13,6 +13,148 @@ 
 
 #include "fdt.h"
 
+/*
+ * Unpacks a "reg" property into its address and size constituents.
+ *
+ * @param prop          Pointer to an FDT "reg" property.
+ * @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 p_addr[out]   Address encoded in the property.
+ * @param p_size[out]   Size encoded in the property.
+ * @returns             -EINVAL on malformed property, 0 otherwise.
+ */
+static int __init read_fdt_prop_as_reg(const struct fdt_property *prop,
+                                       int address_cells, int size_cells,
+                                       uint64_t *p_addr, uint64_t *p_size)
+{
+    const fdt32_t *cell = (const fdt32_t *)prop->data;
+    uint64_t addr, size;
+
+    if ( fdt32_to_cpu(prop->len) !=
+         (address_cells + size_cells) * sizeof(*cell) )
+    {
+        printk("  Cannot read reg %lu+%lu from prop len %u\n",
+            address_cells * sizeof(*cell), size_cells * sizeof(*cell),
+            fdt32_to_cpu(prop->len));
+        return -EINVAL;
+    }
+
+    switch ( address_cells ) {
+    case 1:
+        addr = fdt32_to_cpu(*cell);
+        break;
+    case 2:
+        addr = fdt64_to_cpu(*(const fdt64_t *)cell);
+        break;
+    default:
+        printk("  unsupported sized address_cells\n");
+        return -EINVAL;
+    }
+
+    cell += address_cells;
+    switch ( size_cells ) {
+    case 1:
+        size = fdt32_to_cpu(*cell);
+        break;
+    case 2:
+        size = fdt64_to_cpu(*(const fdt64_t *)cell);
+        break;
+    default:
+        printk("  unsupported sized size_cells\n");
+        return -EINVAL;
+    }
+
+    *p_addr = addr;
+    *p_size = size;
+
+    return 0;
+}
+
+/*
+ * 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)
+{
+    const struct fdt_property *prop;
+    uint64_t addr, size;
+    int ret;
+    int idx;
+
+    ASSERT(!fdt_node_check_compatible(fdt, node, "multiboot,module"));
+
+    /* Location given as a `module-index` property. */
+    prop = fdt_get_property(fdt, node, "module-index", NULL);
+
+    if ( prop )
+    {
+        if ( fdt_get_property(fdt, node, "reg", NULL) )
+        {
+            printk("  Location of multiboot,module defined multiple times\n");
+            return -EINVAL;
+        }
+        return fdt_cell_as_u32((const fdt32_t *)prop->data);
+    }
+
+    /* 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;
+    if ( idx > MAX_NR_BOOTMODS )
+    {
+        /*
+         * MAX_NR_BOOTMODS cannot exceed the max for MB1, represented by 32bits,
+         * thus the cast down to a u32 will be safe due to the prior check.
+         */
+        BUILD_BUG_ON(MAX_NR_BOOTMODS >= (uint64_t)UINT32_MAX);
+        printk("  idx %d exceeds maximum boot modules\n", idx);
+        return -EINVAL;
+    }
+
+    /* Append new module to the existing list */
+
+    bi->nr_modules = idx;
+    bi->mods[idx].start = addr;
+    bi->mods[idx].size = size;
+    printk("  module[%d]: addr %lx size %lx\n", idx, addr, size);
+
+    return idx;
+}
+
 static int __init find_hyperlaunch_node(const void *fdt)
 {
     int hv_node = fdt_path_offset(fdt, "/chosen/hypervisor");
diff --git a/xen/arch/x86/domain-builder/fdt.h b/xen/arch/x86/domain-builder/fdt.h
index 1849656571..e8769dc51c 100644
--- a/xen/arch/x86/domain-builder/fdt.h
+++ b/xen/arch/x86/domain-builder/fdt.h
@@ -3,6 +3,8 @@ 
 #define __XEN_X86_FDT_H__
 
 #include <xen/init.h>
+#include <xen/libfdt/libfdt.h>
+#include <xen/libfdt/libfdt-xen.h>
 
 struct boot_info;
 
diff --git a/xen/include/xen/libfdt/libfdt-xen.h b/xen/include/xen/libfdt/libfdt-xen.h
index a5340bc9f4..2259c09a6a 100644
--- a/xen/include/xen/libfdt/libfdt-xen.h
+++ b/xen/include/xen/libfdt/libfdt-xen.h
@@ -13,6 +13,63 @@ 
 
 #include <xen/libfdt/libfdt.h>
 
+static inline int __init fdt_cell_as_u32(const fdt32_t *cell)
+{
+    return fdt32_to_cpu(*cell);
+}
+
+static inline uint64_t  __init fdt_cell_as_u64(const fdt32_t *cell)
+{
+    return ((uint64_t)fdt32_to_cpu(cell[0]) << 32) | fdt32_to_cpu(cell[1]);
+}
+
+/*
+ * Property: reg
+ *
+ * Defined in Section 2.3.6 of the Device Tree Specification is the "reg"
+ * standard property. The property is a prop-encoded-array that is encoded as
+ * an arbitrary number of (address, size) pairs.  We only extract a single
+ * pair since that is what is used in practice.
+ */
+static inline int __init fdt_get_reg_prop(
+    const void *fdt, int node, unsigned int addr_cells, unsigned int size_cells,
+    uint64_t *addr, uint64_t *size)
+{
+    int ret;
+    const struct fdt_property *prop;
+    fdt32_t *cell;
+
+    /* FDT spec max size is 4 (128bit int), but largest arch int size is 64 */
+    if ( size_cells > 2 || addr_cells > 2 )
+        return -EINVAL;
+
+    prop = fdt_get_property(fdt, node, "reg", &ret);
+    if ( !prop || ret < sizeof(u32) )
+        return ret < 0 ? ret : -EINVAL;
+
+    if ( fdt32_to_cpu(prop->len) !=
+	 ((size_cells + addr_cells) * sizeof(*cell)) )
+        return -EINVAL;
+
+    cell = (fdt32_t *)prop->data;
+
+    /* read address field */
+    if ( addr_cells == 1 )
+        *addr = fdt_cell_as_u32(cell);
+    else
+        *addr = fdt_cell_as_u64(cell);
+
+    cell += addr_cells;
+
+    /* read size field */
+    if ( size_cells == 1 )
+        *size = fdt_cell_as_u32(cell);
+    else
+        *size = fdt_cell_as_u64(cell);
+
+    return 0;
+}
+
 static inline int fdt_get_mem_rsv_paddr(const void *fdt, int n,
                                         paddr_t *address,
                                         paddr_t *size)