Re: [PATCH 11/15] x86/hyperlaunch: add domain id parsing to domain config

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2024-11-23 13:20, Daniel P. Smith wrote:
> 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>
> ---
>   xen/arch/x86/domain_builder/fdt.c | 31 ++++++++++++++++++++++++++++++-
>   xen/arch/x86/domain_builder/fdt.h | 18 ++++++++++++++++++
>   xen/arch/x86/setup.c              |  3 ++-
>   3 files changed, 50 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/domain_builder/fdt.c 
> b/xen/arch/x86/domain_builder/fdt.c
> index bc8054a80ec1..3a6b4fbc09a9 100644
> --- a/xen/arch/x86/domain_builder/fdt.c
> +++ b/xen/arch/x86/domain_builder/fdt.c
> @@ -120,6 +121,28 @@ static int __init process_domain_node(
>           return -EINVAL;
>       }
>   
> +    fdt_for_each_property_offset(property, fdt, dom_node)
> +    {
> +        const struct fdt_property *prop;
> +
> +        prop = fdt_get_property_by_offset(fdt, property, NULL);
> +        if ( !prop )
> +            continue; /* silently skip */
> +
> +        if ( match_fdt_property(fdt, prop, "domid" ) )
> +        {
> +            uint32_t val = DOMID_INVALID;
> +            if ( fdt_prop_as_u32(prop, &val) != 0 )
> +            {
> +                printk("  failed processing domain id for domain %s\n",
> +                       name == NULL ? "unknown" : name);
> +                return -EINVAL;
> +            }
Bounds check against DOMID_FIRST_RESERVED?

> +            bd->domid = (domid_t)val;
> +            printk("  domid: %d\n", bd->domid);
> +        }
> +    }
> +
>       fdt_for_each_subnode(node, fdt, dom_node)
>       {
>           if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 )
> @@ -186,6 +209,12 @@ static int __init process_domain_node(
>           return -EFAULT;
>       }
>   
> +    if ( bd->domid == DOMID_INVALID )
> +        bd->domid = get_initial_domain_id();
> +    else
> +        if ( bd->domid != get_initial_domain_id() )
single line "else if"?

> +            printk(XENLOG_WARNING "WARN: unsuported booting not using initial 
> domid\n");
"unsupported"

Maybe "Booting without initial domid not supported"?

> +
>       return 0;
>   }
>   
> diff --git a/xen/arch/x86/domain_builder/fdt.h b/xen/arch/x86/domain_builder/fdt.h
> index ab2b43872e25..06ead05a2583 100644
> --- a/xen/arch/x86/domain_builder/fdt.h
> +++ b/xen/arch/x86/domain_builder/fdt.h
> @@ -27,6 +27,24 @@ static inline int __init fdt_cell_as_u64(const fdt32_t 
> *cell, uint64_t *val)
> +static inline bool __init match_fdt_property(
> +    const void *fdt, const struct fdt_property *prop, const char *s)
> +{
> +    int slen, len = strlen(s);
> +    const char *p = fdt_get_string(fdt, fdt32_to_cpu(prop->nameoff), &slen);
> +
> +    return p && (slen == len) && (memcmp(p, s, len) == 0);
match_fdt_property() gets called more in later patches.  I wonder if

    const char *p = fdt_get_string(fdt, fdt32_to_cpu(prop->nameoff), 
&slen);
should move into process_domain_node, and then the string is just 
compared?  Maybe it already gets optimized?
(Is there a way to disassemble .init.text with symbols?)

> +}
> +
>   static inline int __init fdt_cmdline_prop_size(const void *fdt, int offset)
>   {
>       int ret;
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index eaac87b02f78..317349b80ac6 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1020,7 +1020,8 @@ static struct domain *__init create_dom0(struct boot_info 
> *bi)
>           dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>   
>       /* Create initial domain.  Not d0 for pvshim. */
> -    bd->domid = get_initial_domain_id();
> +    if ( bd->domid == DOMID_INVALID )
> +        bd->domid = get_initial_domain_id();
This seems redundant with the earlier DOMID_INVALID check & setting.  Or 
does this handle the non-hyperlaunch case?  Maybe it should move to 
builder_init() for other non-hyperlaunch configuration?
>       d = domain_create(bd->domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged);
>       if ( IS_ERR(d) )
>           panic("Error creating d%u: %ld\n", bd->domid, PTR_ERR(d));