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

[Jan Beulich <jbeulich@suse.com>]

On 23.11.2024 19:20, 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.

Why specifically MB1?

> --- a/xen/arch/x86/domain_builder/core.c
> +++ b/xen/arch/x86/domain_builder/core.c
> @@ -56,6 +56,18 @@ void __init builder_init(struct boot_info *bi)
>  
>          printk(XENLOG_INFO "  Number of domains: %d\n", bi->nr_domains);
>      }
> +    else
> +    {
> +        int i;

Plain int when ...

> +        /* Find first unknown boot module to use as Dom0 kernel */
> +        printk("Falling back to using first boot module as dom0\n");
> +        i = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
> +        bi->mods[i].type = BOOTMOD_KERNEL;
> +        bi->domains[0].kernel = &bi->mods[i];
> +        bi->nr_domains = 1;
> +    }

... it's used as array index (and there's no check for the function return
value being negative)?

> --- a/xen/arch/x86/domain_builder/fdt.c
> +++ b/xen/arch/x86/domain_builder/fdt.c
> @@ -14,6 +14,122 @@
>  
>  #include "fdt.h"
>  
> +static inline int __init fdt_get_prop_as_reg(

What does "reg" stand for here?

> +    const void *fdt, int node, const char *name, unsigned int ssize,
> +    unsigned int asize, uint64_t *size, uint64_t *addr)
> +{
> +    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 ( ssize > 2 || asize > 2 )
> +        return -EINVAL;
> +
> +    prop = fdt_get_property(fdt, node, name, &ret);
> +    if ( !prop || ret < sizeof(u32) )
> +        return ret < 0 ? ret : -EINVAL;
> +
> +    /* read address field */
> +    cell = (fdt32_t *)prop->data;
> +
> +    if ( asize == 1 )
> +    {
> +        uint32_t val;
> +        fdt_cell_as_u32(cell, &val);
> +        *addr = (uint64_t)val;

No need for a cast here nor ...

> +    }
> +    else
> +        fdt_cell_as_u64(cell, addr);
> +
> +    /* read size field */
> +    cell += asize;
> +
> +    if ( ssize == 1 )
> +    {
> +        uint32_t val;
> +        fdt_cell_as_u32(cell, &val);
> +        *size = (uint64_t)val;

... here?

> +    }
> +    else
> +        fdt_cell_as_u64(cell, size);
> +
> +    return 0;
> +}

This whole function reads very much like a library one. Does it really need
adding here, rather than to the FDT library code we already have? In any
event there's nothing x86-specific about it, afaics.

> +static int __init dom0less_module_node(
> +    void *fdt, int node, int size_size, int address_size)

Three times plain int, when ...

> +{
> +    uint64_t size, addr;
> +    int ret = fdt_get_prop_as_reg(fdt, node, "reg", size_size, address_size,

... two get converted to unsigned int in the course of the function call
here?

> +                                  &size, &addr);
> +    /* An FDT error value may have been returned, translate to -EINVAL */
> +    if ( ret < 0 )
> +        return -EINVAL;
> +
> +    if ( size != 0 )
> +        return -EOPNOTSUPP;

Not knowing much about DT: What does 0 represent here?

> +    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.
> +     */
> +    return (int)addr;

Comment and cast contradict one another. DYM u32 (really: uint32_t), or plain
int? If you mean to return a plain int (for the sake of the -errno values
further up), MAX_NR_BOOTMODS needs to stay below 2**31.

> +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);
> +    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 )
> +    {
> +        printk("  failed processing #address or #size for domain %s)\n",
> +               name == NULL ? "unknown" : name);
> +        return -EINVAL;
> +    }
> +
> +    fdt_for_each_subnode(node, fdt, dom_node)
> +    {
> +        if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 )
> +        {
> +            int idx = dom0less_module_node(fdt, node, size_size, address_size);
> +            if ( idx < 0 )
> +            {
> +                printk("  failed processing kernel module for domain %s)\n",
> +                       name == NULL ? "unknown" : name);
> +                return idx;
> +            }
> +
> +            if ( idx > bi->nr_modules )
> +            {
> +                printk("  invalid kernel module index for domain node (%d)\n",
> +                       bi->nr_domains);
> +                return -EINVAL;
> +            }
> +
> +            printk("  kernel: boot module %d\n", idx);
> +            bi->mods[idx].type = BOOTMOD_KERNEL;
> +            bd->kernel = &bi->mods[idx];
> +        }
> +    }

What if you find two?

> --- a/xen/arch/x86/domain_builder/fdt.h
> +++ b/xen/arch/x86/domain_builder/fdt.h
> @@ -3,6 +3,7 @@
>  #define __XEN_X86_FDT_H__
>  
>  #include <xen/init.h>
> +#include <xen/libfdt/libfdt.h>
>  
>  #include <asm/bootinfo.h>
>  
> @@ -10,6 +11,22 @@
>  #define HYPERLAUNCH_MODULE_IDX 0
>  
>  #ifdef CONFIG_DOMAIN_BUILDER
> +
> +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]);
> +
> +    return 0;
> +}

Basic library routines again?

Jan