Re: [PATCH v8 1/3] xen/domain: unify domain ID allocation

[dmkhn@xxxxxxxxx]

On Wed, May 21, 2025 at 10:02:52AM +0200, Jan Beulich wrote:
> On 21.05.2025 02:00, dmkhn@xxxxxxxxx wrote:
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -66,6 +66,11 @@ DEFINE_RCU_READ_LOCK(domlist_read_lock);
> >  static struct domain *domain_hash[DOMAIN_HASH_SIZE];
> >  struct domain *domain_list;
> >
> > +/* Non-system domain ID allocator. */
> > +static DEFINE_SPINLOCK(domid_lock);
> > +static DECLARE_BITMAP(domid_bitmap, DOMID_FIRST_RESERVED);
> > +static domid_t domid_last;
> 
> Please move this into the narrowest possible scope. No reason to expose
> it to the entire CU.

Will do.

> 
> > @@ -2405,6 +2412,46 @@ domid_t get_initial_domain_id(void)
> >      return hardware_domid;
> >  }
> >
> > +domid_t domid_alloc(domid_t domid)
> > +{
> > +    spin_lock(&domid_lock);
> > +
> > +    if ( domid < DOMID_FIRST_RESERVED )
> > +    {
> > +        if ( __test_and_set_bit(domid, domid_bitmap) )
> > +            domid = DOMID_INVALID;
> > +    }
> > +    else
> > +    {
> > +        domid = find_next_zero_bit(domid_bitmap, DOMID_FIRST_RESERVED,
> > +                                   domid_last);
> > +
> > +        if ( domid == DOMID_FIRST_RESERVED )
> > +            domid = find_next_zero_bit(domid_bitmap, DOMID_FIRST_RESERVED, 
> > 0);
> 
> Despite what the last sentence of the description says, there is a subtle
> behavioral change here: The original code would never return what was
> called "rover" there. If the most recently created domain went away, you
> would return its ID again here if no other ID is free (which is easier to
> run into with patch 3 in place, and MAX_DOMID set to a pretty low value).
> (Noticing only later: This could even occur without wrapping, as the last
> argument passed is just domid_last, without adding in 1.)

Thanks for spotting the problem!

> 
> I agree it's debatable whether using the sole available ID instead of
> failing wouldn't be better(?) behavior in this specific case, but such
> definitely can't go silently.
> 
> Furthermore you once again are potentially returning ID 0 here (after
> wrapping), when the original code specifically avoided that (irrespective
> of there actually being a Dom0, that is; i.e. covering both the dom0less
> case and the late-hwdom one).
> 
> To be frank, being at v8 I find it problematic that there still are
> (unmentioned and potentially unwanted) behavioral changes here. This
> kind of supports my earlier raised question of whether we actually want
> this sort of a change.

I appreciate patience and the feedback for the series!

> 
> Jan