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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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