Re: [PATCH v3] xen/domain: unify domain ID allocation

[dmkhn@xxxxxxxxx]

On Thu, Apr 17, 2025 at 11:47:07AM +0200, Jan Beulich wrote:
> On 16.04.2025 08:15, dmkhn@xxxxxxxxx wrote:
> > From: Denis Mukhin <dmukhin@xxxxxxxx>
> >
> > Unify the logic of domain ID allocation, so that both the initial domain
> > creation and the usage by domctl use the same helper function across
> > architectures (Arm and x86).
> >
> > Wrap the domain ID allocation as an arch-independent function domid_alloc() 
> > in
> > common/domain.c.
> >
> > Allocation algorithm:
> > - If an explicit domain ID is provided, verify its availability and
> >   use it if ID is unused;
> > - Otherwise, perform an exhaustive search for the first available ID
> >   within the [0..DOMID_FIRST_RESERVED) range, excluding hardware_domid.
> >
> > Move the is_free_domid() helper closer to domid_alloc(). Simplify
> > is_free_domid() by removing the domain ID range check, as the ID is now
> > guaranteed to be within the valid range. Additionally, update the predicate 
> > to
> > return a bool value instead of an int.
> >
> > Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> 
> Please can you clarify whether this is intended to be no functional change
> (as far as one would be able to observe from the outside)? (It isn't, and
> when it isn't, the behavioral change needs justifying. Which I fear you
> won't be able to, in which case it needs undoing. Not using the first
> unused ID is a deliberate property of the present allocation scheme.)

Reverted the algorithm to the original one (v4).
Thanks for review!

> 
> > ---
> >  xen/arch/arm/dom0less-build.c | 19 ++++++++-------
> >  xen/arch/arm/domain_build.c   | 19 +++++++++++----
> >  xen/arch/x86/setup.c          |  8 +++++--
> >  xen/common/domain.c           | 45 +++++++++++++++++++++++++++++++++++
> >  xen/common/domctl.c           | 45 ++++-------------------------------
> >  xen/include/xen/domain.h      |  2 ++
> >  6 files changed, 81 insertions(+), 57 deletions(-)
> 
> This suggests it's not clearly an improvement. And I'm heavily inclined
> to ask (also considering the above) that this simply be dropped.
> 
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -2370,6 +2370,7 @@ void __init create_dom0(void)
> >          .grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version),
> >      };
> >      unsigned int flags = CDF_privileged;
> > +    domid_t domid;
> >      int rc;
> >
> >      /* The vGIC for DOM0 is exactly emulating the hardware GIC */
> > @@ -2394,19 +2395,27 @@ void __init create_dom0(void)
> >      if ( !llc_coloring_enabled )
> >          flags |= CDF_directmap;
> >
> > -    dom0 = domain_create(0, &dom0_cfg, flags);
> > +    rc = domid_alloc(get_initial_domain_id());
> > +    if ( rc < 0 )
> > +        panic("Error allocating domain ID %d (rc = %d)\n",
> > +              get_initial_domain_id(), rc);
> > +    domid = rc;
> > +
> > +    dom0 = domain_create(domid, &dom0_cfg, flags);
> >      if ( IS_ERR(dom0) )
> > -        panic("Error creating domain 0 (rc = %ld)\n", PTR_ERR(dom0));
> > +        panic("Error creating domain %d (rc = %ld)\n", domid, 
> > PTR_ERR(dom0));
> 
> Up to here using domid is okay. However, ...
> 
> >      if ( llc_coloring_enabled && (rc = dom0_set_llc_colors(dom0)) )
> > -        panic("Error initializing LLC coloring for domain 0 (rc = %d)\n", 
> > rc);
> > +        panic("Error initializing LLC coloring for domain %d (rc = %d)\n",
> > +              domid, rc);
> >
> >      if ( alloc_dom0_vcpu0(dom0) == NULL )
> > -        panic("Error creating domain 0 vcpu0\n");
> > +        panic("Error creating domain %d vcpu0\n", domid);
> >
> >      rc = construct_dom0(dom0);
> >      if ( rc )
> > -        panic("Could not set up DOM0 guest OS (rc = %d)\n", rc);
> > +        panic("Could not set up guest OS for domain %d (rc = %d)\n",
> > +              domid, rc);
> >  }
> 
> ... these all would better use %pd, when already being touched.
> 
> While touching all of these I think you also want to aim at making output
> match that %pd or %pv would result in, if they were usable at those places.
> 
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -1009,8 +1009,12 @@ static struct domain *__init create_dom0(struct 
> > boot_info *bi)
> >      if ( iommu_enabled )
> >          dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
> >
> > -    /* Create initial domain.  Not d0 for pvshim. */
> > -    bd->domid = get_initial_domain_id();
> > +    /* Allocate initial domain ID. Not d0 for pvshim. */
> > +    bd->domid = domid_alloc(get_initial_domain_id());
> 
> You're clipping the int return value to domid_t here, and thus ...
> 
> > +    if ( bd->domid < 0 )
> 
> ... this condition will be always false. I'm surprised the compiler didn't
> flag this for you.
> 
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -66,6 +66,51 @@ DEFINE_RCU_READ_LOCK(domlist_read_lock);
> >  static struct domain *domain_hash[DOMAIN_HASH_SIZE];
> >  struct domain *domain_list;
> >
> > +static inline bool is_free_domid(domid_t dom)
> > +{
> > +    struct domain *d = rcu_lock_domain_by_id(dom);
> > +
> > +    if ( d )
> > +        rcu_unlock_domain(d);
> > +
> > +    return !d;
> > +}
> > +
> > +/*
> > + * Allocate new domain ID based on the hint.
> > + *
> > + * If hint is outside of valid [0..DOMID_FIRST_RESERVED] range of IDs,
> 
> That's [0, DOMID_FIRST_RESERVED), to be unambiguous. In C array initializer
> notation it would be [0 ... DOMID_FIRST_RESERVED - 1].
> 
> > + * perform an exhaustive search of the first free domain ID excluding
> > + * hardware_domid.
> > + */
> > +int domid_alloc(int hint)
> 
> I would have thought that I did comment already on the parameter being plain
> int.
> 
> > +{
> > +    domid_t domid;
> > +
> > +    if ( hint >= 0 && hint < DOMID_FIRST_RESERVED )
> > +    {
> > +        if ( !is_free_domid(hint) )
> > +            return -EEXIST;
> > +
> > +        domid = hint;
> > +    }
> > +    else
> > +    {
> > +        for ( domid = 0; domid < DOMID_FIRST_RESERVED; domid++ )
> > +        {
> > +            if ( domid == hardware_domid )
> > +                continue;
> > +            if ( is_free_domid(domid) )
> > +                break;
> > +        }
> > +
> > +        if ( domid == DOMID_FIRST_RESERVED )
> > +            return -ENOMEM;
> 
> There's no memory allocation here, so why ENOMEM? ENOSPC may already be 
> slightly
> better.
> 
> Jan