Re: [PATCH v4 05/13] x86/hyperlaunch: Add helpers to locate multiboot modules

[Alejandro Vallejo <agarciav@xxxxxxx>]

On Fri Apr 18, 2025 at 11:30 PM BST, dmkhn wrote:
> On Thu, Apr 17, 2025 at 01:48:27PM +0100, Alejandro Vallejo wrote:
>> 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 <jason.andryuk@xxxxxxx>
>> Signed-off-by: Alejandro Vallejo <agarciav@xxxxxxx>
>> ---
>> v4:
>>   * Remove stray reg prop parser in libfdt-xen.h.
>>   * Remove fdt32_as_uX accessors.
>>   * Brough fdt_prop_as_u32() accesor from later patches.
>>     * So it can be used in place of a hardcoded fdt32_as_u32().
>>   * Limited MAX_NR_BOOTMODS to INT_MAX.
>>   * Preserved BOOTMOD_XEN on module append logic.
>>   * Add missing bounds check to module-index parsed in multiboot module 
>> helper.
>>   * Converted idx variable to uint32_t for better bounds checking.
>>   * Braces from switch statement to conform to coding style.
>>   * Added missing XENLOG_X.
>>   * Print address_cells and size_cells on error parsing reg properties.
>>   * Added (transient) missing declaration for extern helper.
>>     * becomes static on the next patch.
>> ---
>>  xen/common/domain-builder/fdt.c     | 162 ++++++++++++++++++++++++++++
>>  xen/common/domain-builder/fdt.h     |   2 +
>>  xen/include/xen/domain-builder.h    |   3 +
>>  xen/include/xen/libfdt/libfdt-xen.h |  11 ++
>>  4 files changed, 178 insertions(+)
>> 
>> diff --git a/xen/common/domain-builder/fdt.c 
>> b/xen/common/domain-builder/fdt.c
>> index b5ff8220da..d73536fed6 100644
>> --- a/xen/common/domain-builder/fdt.c
>> +++ b/xen/common/domain-builder/fdt.c
>> @@ -13,6 +13,168 @@
>> 
>>  #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,
>
> I would do s/read_fdt_prop_as_reg/fdt_prop_as_reg/ similar to 
> fdt_prop_as_u32()
> below.

Yes, that sounds better.

>
>> +                                       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(XENLOG_ERR "  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(XENLOG_ERR "  unsupported address_cells=%d\n", 
>> address_cells);
>> +        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(XENLOG_ERR "  unsupported size_cells=%d\n", size_cells);
>> +        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;
>> +    uint32_t idx;
>> +
>> +    if ( fdt_node_check_compatible(fdt, node, "multiboot,module") )
>> +    {
>> +        printk(XENLOG_ERR "  bad module. multiboot,module not found");
>> +        return -ENODATA;
>> +    }
>> +
>> +    /* Location given as a `module-index` property. */
>> +    if ( (prop = fdt_get_property(fdt, node, "module-index", NULL)) )
>> +    {
>> +        if ( fdt_get_property(fdt, node, "reg", NULL) )
>> +        {
>> +            printk(XENLOG_ERR "  found both reg and module-index for 
>> module\n");
>> +            return -EINVAL;
>> +        }
>> +        if ( (ret = fdt_prop_as_u32(prop, &idx)) )
>> +        {
>> +            printk(XENLOG_ERR "  bad module-index prop\n");
>> +            return ret;
>> +        }
>> +        if ( idx >= MAX_NR_BOOTMODS )
>> +        {
>> +            printk(XENLOG_ERR "  module-index overflow. %s=%u\n",
>> +                   STR(MAX_NR_BOOTMODS), MAX_NR_BOOTMODS);
>> +            return -EINVAL;
>> +        }
>> +
>> +        return idx;
>> +    }
>> +
>> +    /* Otherwise location given as a `reg` property. */
>> +    if ( !(prop = fdt_get_property(fdt, node, "reg", NULL)) )
>> +    {
>> +        printk(XENLOG_ERR "  no location for multiboot,module\n");
>> +        return -EINVAL;
>> +    }
>> +    if ( fdt_get_property(fdt, node, "module-index", NULL) )
>> +    {
>> +        printk(XENLOG_ERR "  found both reg and module-index for module\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    ret = read_fdt_prop_as_reg(prop, address_cells, size_cells, &addr, 
>> &size);
>> +    if ( ret < 0 )
>> +    {
>> +        printk(XENLOG_ERR "  failed reading reg for multiboot,module\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    idx = bi->nr_modules;
>> +    if ( idx > MAX_NR_BOOTMODS )
>> +    {
>> +        /*
>> +         * MAX_NR_BOOTMODS must fit in 31 bits so it's representable in the
>> +         * positive side of an int; for the return value.
>> +         */
>> +        BUILD_BUG_ON(MAX_NR_BOOTMODS > (uint64_t)INT_MAX);
>> +        printk(XENLOG_ERR "  idx=%u exceeds len=%u\n", idx, 
>> MAX_NR_BOOTMODS);
>> +        return -EINVAL;
>> +    }
>> +
>> +    /*
>> +     * Append new module to the existing list
>> +     *
>> +     * Note that bi->nr_modules points to Xen itself, so we must shift it 
>> first
>> +     */
>> +    bi->nr_modules++;
>> +    bi->mods[bi->nr_modules] = bi->mods[idx];
>> +    bi->mods[idx] = (struct boot_module){
>> +        .start = addr,
>> +        .size = size,
>> +    };
>> +
>> +    printk(XENLOG_INFO "  module[%u]: 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/common/domain-builder/fdt.h 
>> b/xen/common/domain-builder/fdt.h
>> index 955aead497..8c98a256eb 100644
>> --- a/xen/common/domain-builder/fdt.h
>> +++ b/xen/common/domain-builder/fdt.h
>> @@ -2,6 +2,8 @@
>>  #ifndef __XEN_DOMAIN_BUILDER_FDT_H__
>>  #define __XEN_DOMAIN_BUILDER_FDT_H__
>> 
>> +#include <xen/libfdt/libfdt-xen.h>
>> +
>>  struct boot_info;
>> 
>>  /* hyperlaunch fdt is required to be module 0 */
>> diff --git a/xen/include/xen/domain-builder.h 
>> b/xen/include/xen/domain-builder.h
>> index ac2b84775d..ace6b6875b 100644
>> --- a/xen/include/xen/domain-builder.h
>> +++ b/xen/include/xen/domain-builder.h
>> @@ -5,5 +5,8 @@
>>  struct boot_info;
>> 
>>  void builder_init(struct boot_info *bi);
>> +int fdt_read_multiboot_module(const void *fdt, int node,
>> +                              int address_cells, int size_cells,
>> +                              struct boot_info *bi)
>> 
>>  #endif /* __XEN_DOMAIN_BUILDER_H__ */
>> diff --git a/xen/include/xen/libfdt/libfdt-xen.h 
>> b/xen/include/xen/libfdt/libfdt-xen.h
>> index a5340bc9f4..deafb25d98 100644
>> --- a/xen/include/xen/libfdt/libfdt-xen.h
>> +++ b/xen/include/xen/libfdt/libfdt-xen.h
>> @@ -12,6 +12,17 @@
>>  #define LIBFDT_XEN_H
>> 
>>  #include <xen/libfdt/libfdt.h>
>> +#include <xen/errno.h>
>> +
>> +static inline int __init fdt_prop_as_u32(
>> +    const struct fdt_property *prop, uint32_t *val)
>> +{
>> +    if ( !prop || fdt32_to_cpu(prop->len) < sizeof(u32) )
>> +        return -EINVAL;
>> +
>> +    *val = fdt32_to_cpu(*(const fdt32_t *)prop->data);
>> +    return 0;
>> +}
>
> My understanding is domain builder establishes its own shims around libfdt so
> libfdt is kept unmodified and it is easier to pick up libfdt updates.
>
> So, IMO, this function should reside in xen/common/domain-builder/fdt.c
>
> Thoughts?

Ugh. Too much code motion is not good for your head. I did mean to move
them all (I mentioned it in an earlier response to Jan, I think). Will
do for good this time.

Cheers,
Alejandro