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

[Alejandro Vallejo <agarciav@xxxxxxx>]

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