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