Re: [PATCH v10 3/3] xen/domain: use get_initial_domain_id() instead of open-coded 0

[dmkhn@xxxxxxxxx]

On Thu, Jul 17, 2025 at 12:59:15PM +0200, Alejandro Vallejo wrote:
> +Juergen for late-hwdom bit
> 
> Hi,
> 
> On Mon Jun 23, 2025 at 8:28 PM CEST, dmkhn wrote:
> > From: Denis Mukhin <dmukhin@xxxxxxxx>
> >
> > Remove the open-coded domain ID 0 and replace it with a call to
> > get_initial_domain_id().
> 
> Ideally we'd be better off replacing it where applicable with  is
> hardware_domain(), or is_control_domain(), or a ORd version of both depending
> on what the hardcoded 0 means to do.

I agree.

I think I will postpone this change until the design of dom0 split into
control/hardware settles.

P.S. Corrected the list of Cc for mail to be sent.

> 
> >
> > Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> > ---
> > Changes since v9:
> > - new patch
> > ---
> >  xen/arch/arm/domain_build.c | 4 ++--
> >  xen/common/domain.c         | 6 +++---
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index b59b56636920..b525d78c608f 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -2074,9 +2074,9 @@ void __init create_dom0(void)
> >      if ( !llc_coloring_enabled )
> >          flags |= CDF_directmap;
> >
> > -    domid = domid_alloc(0);
> > +    domid = domid_alloc(get_initial_domain_id());
> >      if ( domid == DOMID_INVALID )
> > -        panic("Error allocating domain ID 0\n");
> > +        panic("Error allocating domain ID %d\n", get_initial_domain_id());
> >
> >      dom0 = domain_create(domid, &dom0_cfg, flags);
> >      if ( IS_ERR(dom0) )
> 
> On arm this is just another level of indirection. It might work as a marker to
> know where dom0 is hardcoded, though. So sounds good to me.
> 
> > diff --git a/xen/common/domain.c b/xen/common/domain.c
> > index be022c720b13..27575b4610e3 100644
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -492,7 +492,7 @@ static int late_hwdom_init(struct domain *d)
> >      struct domain *dom0;
> >      int rv;
> >
> > -    if ( d != hardware_domain || d->domain_id == 0 )
> > +    if ( d != hardware_domain || d->domain_id == get_initial_domain_id() )
> 
> This is tricky. get_initial_domain_id() is only non-zero in shim-mode. And in
> that mode there's no control nor hardware domain (hence why the initial domain
> id is not zero in that case).
> 
> >          return 0;
> >
> >      rv = xsm_init_hardware_domain(XSM_HOOK, d);
> > @@ -501,7 +501,7 @@ static int late_hwdom_init(struct domain *d)
> >
> >      printk("Initialising hardware domain %d\n", hardware_domid);
> >
> > -    dom0 = rcu_lock_domain_by_id(0);
> > +    dom0 = rcu_lock_domain_by_id(get_initial_domain_id());
> 
> Hmmm. More generally this is probably trying to find the previous hardware
> domain. Which the caller already has a pointer to.
> 
> Maybe this would work?
> 
> ```
>       -static int late_hwdom_init(struct domain *d)
>       +static int late_hwdom_init(struct domain *d, struct domain *old_hwdom)
>        {
>        #ifdef CONFIG_LATE_HWDOM
>            struct domain *dom0;
>            int rv;
> 
>       -    if ( d != hardware_domain || d->domain_id == 0 )
>       +    if ( d != hardware_domain || !old_hwdom )
>                return 0;
> 
>            rv = xsm_init_hardware_domain(XSM_HOOK, d);
>       @@ -493,8 +493,6 @@ static int late_hwdom_init(struct domain *d)
> 
>            printk("Initialising hardware domain %d\n", hardware_domid);
> 
>       -    dom0 = rcu_lock_domain_by_id(0);
>       -    ASSERT(dom0 != NULL);
>            /*
>             * Hardware resource ranges for domain 0 have been set up from
>             * various sources intended to restrict the hardware domain's
>       @@ -512,11 +510,9 @@ static int late_hwdom_init(struct domain *d)
>        #ifdef CONFIG_X86
>            rangeset_swap(d->arch.ioport_caps, dom0->arch.ioport_caps);
>            setup_io_bitmap(d);
>       -    setup_io_bitmap(dom0);
>       +    setup_io_bitmap(old_hwdom);
>        #endif
> 
>       -    rcu_unlock_domain(dom0);
>       -
>            iommu_hwdom_init(d);
> 
>            return rv;
>       @@ -967,7 +963,7 @@ struct domain *domain_create(domid_t domid,
>            if ( (err = sched_init_domain(d, config->cpupool_id)) != 0 )
>                goto fail;
> 
>       -    if ( (err = late_hwdom_init(d)) != 0 )
>       +    if ( (err = late_hwdom_init(d, old_hwdom)) != 0 )
>                goto fail;
> ```
> 
> Juergen, do you have any thoughts about this?
> 
> >      ASSERT(dom0 != NULL);
> >      /*
> >       * Hardware resource ranges for domain 0 have been set up from
> > @@ -2479,7 +2479,7 @@ domid_t domid_alloc(domid_t domid)
> >          if ( domid == DOMID_FIRST_RESERVED )
> >              domid = find_next_zero_bit(domid_bitmap,
> >                                         DOMID_FIRST_RESERVED,
> > -                                       1);
> > +                                       get_initial_domain_id() + 1);
> 
> IMO, this should be either 1 (for defence in depth against using zero) or 0.
> There's nothing special with any other initial domain ids.
> 
> >  #endif
> >
> >          if ( domid < DOMID_FIRST_RESERVED )
> 
> Cheers,
> Alejandro
>