Re: [PATCH v3 11/24] xen/domain: move get_initial_domain_id() to arch-independent header

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.01.2025 02:58, Denis Mukhin via B4 Relay wrote:
> From: Denis Mukhin <dmukhin@xxxxxxxx>
> 
> Honor 'hardware_domid=' parameter across all architectures

In which way is it not honored right now (besides depending on
LATE_HWDOM=y)? It is, for example, not supposed to be the case that
in a late-hwdom configuration all IDs below hardware_domid are
unavailable for new domains to be created.

> and update
> max_init_domid correctly so that toolstack and, subsequently, console driver
> could iterate across known domains more efficiently.
> 
> Also, move max_init_domid to arch-independent location.
> 
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>


> @@ -2387,15 +2388,15 @@ void __init create_dom0(void)
>      if ( !llc_coloring_enabled )
>          flags |= CDF_directmap;
>  
> -    dom0 = domain_create(0, &dom0_cfg, flags);
> +    dom0 = domain_create(domid, &dom0_cfg, CDF_privileged | CDF_directmap);

Why the move from "flags" to "CDF_privileged | CDF_directmap"?

>      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));
>  
>      if ( llc_coloring_enabled && (rc = dom0_set_llc_colors(dom0)) )
>          panic("Error initializing LLC coloring for domain 0 (rc = %d)\n", 
> rc);
>  
>      if ( alloc_dom0_vcpu0(dom0) == NULL )
> -        panic("Error creating domain 0 vcpu0\n");
> +        panic("Error creating domain %d vcpu0\n", domid);

If already you alter this, please switch to %pd.

> @@ -65,6 +68,9 @@ DEFINE_RCU_READ_LOCK(domlist_read_lock);
>  static struct domain *domain_hash[DOMAIN_HASH_SIZE];
>  struct domain *domain_list;
>  
> +/* Last known non-system domain ID. */
> +domid_t __read_mostly max_init_domid;

I'm afraid comment and variable name conflict with one another. And
really with its present purpose it ought to be __ro_after_init, I
think.

> @@ -2261,6 +2267,15 @@ int continue_hypercall_on_cpu(
>      return 0;
>  }
>  
> +domid_t get_initial_domain_id(void)
> +{
> +#ifdef CONFIG_PV_SHIM
> +    if ( pv_shim )
> +        return pv_shim_get_initial_domain_id();
> +#endif

Aiui the #ifdef is necessary for non-x86? Would be nice to avoid that, yet
then I'm not meaning to ask you to do a lot of further rearrangements.

> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -415,10 +415,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> u_domctl)
>      case XEN_DOMCTL_createdomain:
>      {
>          domid_t        dom;
> -        static domid_t rover = 0;
>  
>          dom = op->domain;
> -        if ( (dom > 0) && (dom < DOMID_FIRST_RESERVED) )
> +        if ( (dom > max_init_domid) && (dom < DOMID_FIRST_RESERVED) )
>          {
>              ret = -EEXIST;
>              if ( !is_free_domid(dom) )
> @@ -426,19 +425,19 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> u_domctl)
>          }
>          else
>          {
> -            for ( dom = rover + 1; dom != rover; dom++ )
> +            for ( dom = max_init_domid + 1; dom != max_init_domid; dom++ )

The "dom != max_init_domid" is dead code now if I'm no t mistaken, due to ...

>              {
>                  if ( dom == DOMID_FIRST_RESERVED )
> -                    dom = 1;
> +                    dom = max_init_domid + 1;

... this. Thus the loop will become infinite if all permissible domain IDs
are in use. Yet then ...

>                  if ( is_free_domid(dom) )
>                      break;
>              }
>  
>              ret = -ENOMEM;
> -            if ( dom == rover )
> +            if ( dom == max_init_domid )
>                  break;
>  
> -            rover = dom;
> +            max_init_domid = dom;
>          }

... why would all of this code need changing? (If it does, I think that
wants to be in a separate patch, with appropriate description.)

> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -568,6 +568,14 @@ static long xenver_varbuf_op(int cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>      return sz;
>  }
>  
> +static int __init cf_check globals_init(void)
> +{
> +    max_init_domid = get_initial_domain_id();
> +
> +    return 0;
> +}
> +__initcall(globals_init);

Imo this wants to live in the CU defining the variable.

Jan