Re: [PATCH v5 08/12] x86/hyperlaunch: add domain id parsing to domain config

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2025-04-24 12:10, Alejandro Vallejo wrote:
> From: "Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>
>
> Introduce the ability to specify the desired domain id for the domain
> definition. The domain id will be populated in the domid property of the
> domain node in the device tree configuration.
>
> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Alejandro Vallejo <agarciav@xxxxxxx>
> ---
> diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
> index 144fcc75b5..5a5b3c8806 100644
> --- a/xen/common/domain-builder/fdt.c
> +++ b/xen/common/domain-builder/fdt.c
> @@ -188,12 +189,54 @@ static int __init fdt_read_multiboot_module(const void 
> *fdt, int node,
>   static int __init process_domain_node(
>       struct boot_info *bi, const void *fdt, int dom_node)
>   {
> -    int node;
> +    int node, property;
>       struct boot_domain *bd = &bi->domains[bi->nr_domains];
>       const char *name = fdt_get_name(fdt, dom_node, NULL) ?: "unknown";
>       int address_cells = fdt_address_cells(fdt, dom_node);
>       int size_cells = fdt_size_cells(fdt, dom_node);
>   
> +    fdt_for_each_property_offset(property, fdt, dom_node)
> +    {
> +        const struct fdt_property *prop;
> +        const char *prop_name;
> +        int name_len, rc;
> +
> +        prop = fdt_get_property_by_offset(fdt, property, NULL);
> +        if ( !prop )
> +            continue; /* silently skip */
> +
> +        prop_name = fdt_get_string(fdt, fdt32_to_cpu(prop->nameoff), 
> &name_len);
> +
Stray blank line.

> +        if ( !strncmp(prop_name, "domid", name_len) )
> +        {
> +            uint32_t val = DOMID_INVALID;
> +
> +            if ( (rc = fdt_prop_as_u32(prop, &val)) )
> +            {
> +                printk(XENLOG_ERR
> +                       "  failed processing domain id for domain %s\n", name);
> +                return rc;
> +            }
Maybe add a blank line here?

> +            if ( val >= DOMID_FIRST_RESERVED )
> +            {
> +                printk(XENLOG_ERR "  invalid domain id for domain %s\n", name);
> +                return -EINVAL;
> +            }
> +
> @@ -258,6 +301,13 @@ static int __init process_domain_node(
>           return -ENODATA;
>       }
>   
> +    if ( bd->domid == DOMID_INVALID )
> +        bd->domid = get_initial_domain_id();
> +    else if ( bd->domid != get_initial_domain_id() )
> +        printk(XENLOG_WARNING
> +               "warning: unsupported boot. d%d is not the initial domain.\n",
Maybe:
"warning: d%u is not the expected initial domid (%u)\n" ?

It's a strange message.  The domid property is added, but it's not 
expected to actually be used?
With the newlines addressed (and optionally the message change):

Reviewed-by: Jason Andryuk <jason.andryuk@xxxxxxx>

Thanks,
Jason