Re: [PATCH v2 08/15] x86/hyperlaunch: locate dom0 kernel with hyperlaunch

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2024-12-26 11:57, Daniel P. Smith wrote:
> Look for a subnode of type `multiboot,kernel` within a domain node. If found,
> process the reg property for the MB1 module index. If the bootargs property is
> present and there was not an MB1 string, then use the command line from the
> device tree definition.
>
> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> Reviewed-by: Jason Andryuk <jason.andryuk@xxxxxxx>
> ---
> Changes since v1:
> - moved low-level fdt handlers to libfdt-xen.h
> - coding style changes
> - moved default to "unknown" up to a local declaration
> - moved the general fdt parsing code out to libfdt
> - reworked device tree property parsing for module index
>    - reworked parsers to take index as parameter
>    - parsers now return success or error value
> - added check if kernel was already located, warn and continue
> ---
> diff --git a/xen/arch/x86/domain-builder/fdt.c 
> b/xen/arch/x86/domain-builder/fdt.c
> index 5793bdc9fd47..bcaee50689a6 100644
> --- a/xen/arch/x86/domain-builder/fdt.c
> +++ b/xen/arch/x86/domain-builder/fdt.c
> @@ -13,6 +13,114 @@
>   
>   #include "fdt.h"
>   
> +static int __init hl_module_index(void *fdt, int node, uint32_t *idx)
> +{
> +    int ret = 0;
> +    const struct fdt_property *prop =
> +        fdt_get_property(fdt, node, "module-index", &ret);
> +
> +    /* FDT error or bad idx pointer, translate to -EINVAL */
> +    if ( ret < 0 || idx == NULL )
> +        return -EINVAL;
> +
> +    fdt_cell_as_u32((fdt32_t *)prop->data, idx);
fdt_prop_as_u32() provides a few more checks and would not require the 
cast here.
> +
> +    if ( *idx > MAX_NR_BOOTMODS )
> +        return -ERANGE;
> +
> +    return 0;
> +}
> +
> +static int __init dom0less_module_index(
> +    void *fdt, int node, int size_size, int address_size, uint32_t *idx)
> +{
> +    uint64_t size = ~0UL, addr = ~0UL;
> +    int ret =
> +        fdt_get_reg_prop(fdt, node, address_size, size_size, &addr, &size, 1);
> +
> +    /* FDT error or bad idx pointer, translate to -EINVAL */
> +    if ( ret < 0 || idx == NULL )
> +        return -EINVAL;
> +
> +    /* Convention is that zero size indicates address is an index */
> +    if ( size != 0 )
> +        return -EOPNOTSUPP;
We wanted reg = <addr size> to be converted into a new boot module.

i.e. if the user is using grub - they should use `module-index` to 
specify binaries.  If the binaries are loaded in memory, they must use 
`reg`.
> +
> +    if ( addr > MAX_NR_BOOTMODS )
> +        return -ERANGE;
> +
> +    /*
> +     * MAX_NR_BOOTMODS cannot exceed the max for MB1, represented by a u32,
> +     * thus the cast down to a u32 will be safe due to the prior check.
> +     */
> +    *idx = (uint32_t)addr;
> +
> +    return 0;
> +}
> +
> +static int __init process_domain_node(
> +    struct boot_info *bi, void *fdt, int dom_node)
> +{
> +    int node;
> +    struct boot_domain *bd = &bi->domains[bi->nr_domains];
> +    const char *name = fdt_get_name(fdt, dom_node, NULL) ?: "unknown";
> +
> +    fdt_for_each_subnode(node, fdt, dom_node)
> +    {
> +        if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 )
> +        {
> +            unsigned int idx;
> +            int ret = 0;
> +
> +            if ( bd->kernel )
> +            {
> +                printk(XENLOG_ERR "Duplicate kernel module for domain %s)\n",
> +                       name);
> +                continue;
> +            }
> +
> +            /* Try hyperlaunch property, fall back to dom0less property. */
> +            if ( hl_module_index(fdt, node, &idx) < 0 )
> +            {
> +                int address_size = fdt_address_cells(fdt, dom_node);
> +                int size_size = fdt_size_cells(fdt, dom_node);
> +
> +                if ( address_size < 0 || size_size < 0 )
> +                    ret = -EINVAL;
> +                else
> +                    ret = dom0less_module_index(
> +                            fdt, node, size_size, address_size, &idx);
> +            }
> +
> +            if ( ret < 0 )
> +            {
> +                printk("  failed processing kernel module for domain %s)\n",
> +                       name);
> +                return ret;
> +            }
> +
> +            if ( idx > bi->nr_modules )
> +            {
> +                printk("  invalid kernel module index for domain node (%d)\n",
> +                       bi->nr_domains);
> +                return -EINVAL;
> +            }
The earlier MAX_NR_BOOTMODS seem superfluous since this is the one that 
matters.
> +
> +            printk("  kernel: boot module %d\n", idx);
> +            bi->mods[idx].type = BOOTMOD_KERNEL;
> +            bd->kernel = &bi->mods[idx];
> +        }
> +    }
> +
> +    if ( !bd->kernel )
> +    {
> +        printk(XENLOG_ERR "ERR: no kernel assigned to domain\n");
> +        return -EFAULT;
> +    }
> +
> +    return 0;
> +}
> +
>   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 f5b89cb54b29..0be4ac771bc4 100644
> --- a/xen/arch/x86/domain-builder/fdt.h
> +++ b/xen/arch/x86/domain-builder/fdt.h
> @@ -10,6 +12,7 @@
>   #define HYPERLAUNCH_MODULE_IDX 0
>   
>   #ifdef CONFIG_DOMAIN_BUILDER
> +
This newline should move to a more appropriate patch.

>   int has_hyperlaunch_fdt(struct boot_info *bi);
>   int walk_hyperlaunch_fdt(struct boot_info *bi);
>   #else
> diff --git a/xen/include/xen/libfdt/libfdt-xen.h 
> b/xen/include/xen/libfdt/libfdt-xen.h
> index a5340bc9f4e1..27d23df03af3 100644
> --- a/xen/include/xen/libfdt/libfdt-xen.h
> +++ b/xen/include/xen/libfdt/libfdt-xen.h
> @@ -13,6 +13,82 @@
>   
>   #include <xen/libfdt/libfdt.h>
>   
> +static inline int __init fdt_cell_as_u32(const fdt32_t *cell, uint32_t *val)
> +{
> +    *val = fdt32_to_cpu(*cell);
> +
> +    return 0;
> +}
> +
> +static inline int __init fdt_cell_as_u64(const fdt32_t *cell, uint64_t *val)
> +{
> +    *val = ((uint64_t)fdt32_to_cpu(cell[0]) << 32) |
> +           (uint64_t)fdt32_to_cpu(cell[1]);
A cast isn't needed on cell[1] since it's not shifted.

> +
> +    return 0;
> +}
> +
> +/*
> + * 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, length) pairs.
> + */
> +static inline int __init fdt_get_reg_prop(
> +    const void *fdt, int node, unsigned int asize, unsigned int ssize,
> +    uint64_t *addr, uint64_t *size, unsigned int pairs)
Your other function uses address_size and size_size compared to asize 
and ssize here.  While size_size is fun to write, I think addr_cells and 
size_cells are more accurate names.  It's the number of cells to parse 
for the respective values.  device_tree_get_reg() already exists - is 
that not usable?
> +{
> +    int ret;
> +    unsigned int i, count;
> +    const struct fdt_property *prop;
> +    fdt32_t *cell;
> +
> +    /* FDT spec max size is 4 (128bit int), but largest arch int size is 64 */
> +    if ( ssize > 2 || asize > 2 )
> +        return -EINVAL;
> +
> +    prop = fdt_get_property(fdt, node, "reg", &ret);
> +    if ( !prop || ret < sizeof(u32) )
> +        return ret < 0 ? ret : -EINVAL;
> +
> +    /* Get the number of (addr, size) pairs and clamp down. */
> +    count = fdt32_to_cpu(prop->len) / (ssize + asize);
Rounding down seems okay-ish.

> +    count = count < pairs ? count : pairs;
If the caller is asking for "pairs", should it just be an error if there 
aren't enough?
Regards,
Jason