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 <dmuk...@ford.com>
> >
> > 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 <dmuk...@ford.com>
> > ---
> > 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
>
