|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v4 05/13] x86/hyperlaunch: Add helpers to locate multiboot modules
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.
> + 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?
>
> static inline int fdt_get_mem_rsv_paddr(const void *fdt, int n,
> paddr_t *address,
> --
> 2.43.0
>
>
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |