Re: [PATCH v9 5/6] x86/boot: introduce domid field to struct boot_domain

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

On 11/27/24 05:32, Jan Beulich wrote:
> On 15.11.2024 14:12, Daniel P. Smith wrote:
> > Add a domid field to struct boot_domain to hold the assigned domain id for the
> > domain. During initialization, ensure all instances of struct boot_domain have
> > the invalid domid to ensure that the domid must be set either by convention or
> > configuration.
> I'm missing the "why" part here - after all ...
Which is part of why I rolled these over into the dom0 device tree 
series, as it will provide more context to its purpose. This field is 
used to store the value parsed in from the device tree. In dom0 device 
tree series, this commit could be merged with the commit that introduces 
the device tree parsing for domid to provide better context for the 
introduction of the structure element.
> > --- a/xen/arch/x86/include/asm/bootdomain.h
> > +++ b/xen/arch/x86/include/asm/bootdomain.h
> > @@ -12,6 +12,8 @@ struct boot_module;
> >   struct domain;
> >   
> >   struct boot_domain {
> > +    domid_t domid;
> > +
> >       struct boot_module *kernel;
> >       struct boot_module *ramdisk;
> >   
> ... just out of context here there is struct domain *. I can only guess that
> the domain ID is needed for the time until the domain pointer was actually
> filled.
Correct, thus why it makes more sense to merge with the domid device 
tree parsing.
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -339,6 +339,9 @@ static struct boot_info *__init multiboot_fill_boot_info(
> >       /* Variable 'i' should be one entry past the last module. */
> >       bi->mods[i].type = BOOTMOD_XEN;
> >   
> > +    for ( i = 0; i < MAX_NR_BOOTDOMS; i++ )
> > +        bi->domains[i].domid = DOMID_INVALID;
> Generally I think ARRAY_SIZE() is better to use for loop boundaries. Yet
> then - why don't you statically initialize the array in xen_boot_info?
I indifferent with ARRAY_SIZE(), I will try and keep that in mind for 
future parts of the series. As for static init'ing, good point since we 
are already doing it with other fields, I can add this to it.
> > @@ -977,7 +980,6 @@ static struct domain *__init create_dom0(struct boot_info 
> > *bi)
> >       };
> >       struct boot_domain *bd = &bi->domains[0];
> >       struct domain *d;
> > -    domid_t domid;
> >   
> >       if ( opt_dom0_pvh )
> >       {
> > @@ -993,15 +995,15 @@ static struct domain *__init create_dom0(struct boot_info 
> > *bi)
> >           dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
> >   
> >       /* Create initial domain.  Not d0 for pvshim. */
> > -    domid = get_initial_domain_id();
> > -    d = domain_create(domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged);
> > +    bd->domid = get_initial_domain_id();
> > +    d = domain_create(bd->domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged);
> >       if ( IS_ERR(d) )
> > -        panic("Error creating d%u: %ld\n", domid, PTR_ERR(d));
> > +        panic("Error creating d%u: %ld\n", bd->domid, PTR_ERR(d));
> As to the comment at the top - this change alone certainly doesn't clarify
> the "why".
Agreed.

> >       init_dom0_cpuid_policy(d);
> >   
> >       if ( alloc_dom0_vcpu0(d) == NULL )
> > -        panic("Error creating d%uv0\n", domid);
> > +        panic("Error creating d%uv0\n", bd->domid);
> Imo this would better use d->domain_id. And while touching it, %u would also
> want swapping for %d.
hmm, I was actually considering s/d%u/%pd/ and just pass in d, but was 
certain if there was an explicit reason it wasn't used before. If I am 
going to change it, would %pd not be more desired here?
v/r,
dps