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

[Alejandro Vallejo <alejandro.garciavallejo@xxxxxxx>]

+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.

>
> 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