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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> @@ -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.)

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.

Jan