From: Denis Mukhin <dmukhin@ford.com>
Remove the open-coded domain ID 0 and replace it with a call to
get_initial_domain_id().
Signed-off-by: Denis Mukhin <dmukhin@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) )
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() )
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());
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);
#endif
if ( domid < DOMID_FIRST_RESERVED )
--
2.34.1+Juergen for late-hwdom bit
Hi,
On Mon Jun 23, 2025 at 8:28 PM CEST, dmkhn wrote:
> From: Denis Mukhin <dmukhin@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 <dmukhin@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
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@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 <dmukhin@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
>
On 23.06.2025 20:28, dmkhn@proton.me wrote:
> --- 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() )
> 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());
> ASSERT(dom0 != NULL);
For both changes above you're introducing a subtle (largely theoretical)
behavioral change: In shim mode, this assertion, if we somehow made it
here, would suddenly not trigger anymore. Similarly for the earlier
change the return path may wrongly be taken then.
> @@ -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);
This imo is the worst of the changes. get_initial_domain_id() can return
non-zero. In that case we still would (in principle) want to re-start
from 1 here.
Jan
On Tue, Jun 24, 2025 at 08:07:55AM +0200, Jan Beulich wrote:
> On 23.06.2025 20:28, dmkhn@proton.me wrote:
> > --- 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() )
> > 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());
> > ASSERT(dom0 != NULL);
>
> For both changes above you're introducing a subtle (largely theoretical)
> behavioral change: In shim mode, this assertion, if we somehow made it
> here, would suddenly not trigger anymore. Similarly for the earlier
> change the return path may wrongly be taken then.
Thanks.
>
> > @@ -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);
>
> This imo is the worst of the changes. get_initial_domain_id() can return
> non-zero. In that case we still would (in principle) want to re-start
> from 1 here.
Thanks.
I will postpone this patch until the split of dom0 into control/hardware
settles.
>
> Jan
© 2016 - 2025 Red Hat, Inc.