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

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 11/25/24 18:45, Jason Andryuk wrote:
> 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?
Yah, that would be good.

> > +            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"?
Yep.

> > +            printk(XENLOG_WARNING "WARN: unsuported booting not using 
> > initial domid\n");
> "unsupported"
>
> Maybe "Booting without initial domid not supported"?
I am good with that phrasing.

> > +
> >       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?
Your approach would explicitly force a single fdt_get_string() call, 
logic would remain readable, all while not hoping the optimizer can 
catch it.
> (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?
Even if the call to get_initial_domain_id() is moved to builder_init(), 
I would not feel comfortable with assuming bd->domid is valid when 
getting here. So I would still have the check, but with panic instead. 
I'm open to making the change if the x86 maintainers are comfortable 
with making the assumption.
v/r,
dps