Re: [PATCH v1 3/3] xen: introduce CONFIG_HAS_DOMAIN_TYPE

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.04.2026 16:21, Oleksii Kurochko wrote:
> As domain type is part of common code now there is no any reason
> to have architecture-specific set_domain_type() functions so
> it is dropped.
> 
> The CONFIG_ARM_64 guard in xen/arch/arm/domain_build.c is intentionally
> left unchanged: it is ARM-specific code, and ARM_64 selects
> HAS_DOMAIN_TYPE, so the semantics are identical.

Still I think it would better be updated, for consistency's sake.

> x86 with CONFIG_64BIT=y shouldn't use is_{32,64}bit_domain() as
> x86 doesn't have support of CONFIG_HAS_DOMAIN_TYPE. For x86_32
> it is fine to follow generic is_{32,64}_bit_domain() implementations.

As Michal has also alluded to, the use of x86_32 here needs some further
qualification, as Xen hasn't been possible to be built for that target
for many years. Quite possibly you mean guest type there, but then it's
unclear why you would restrict the explanation to 32-bit guests.

> Generally it probably would be better to introduce CONFIG_32BIT
> and use it to define is_{32,64}bit_domain() as if one day
> CONFIG_128BIT will be introduced is_{32,64}bit_domain() will be
> defined incorrectly but considering that CONFIG_64BIT is maximum
> supported bitness it is fine to define in this way.

Many things will need adjustment for a future CONFIG_128BIT. Imo this
kind of statement doesn't belong in a patch description; it could be a
post-commit-message remark.

> --- a/xen/arch/Kconfig
> +++ b/xen/arch/Kconfig
> @@ -1,6 +1,9 @@
>  config 64BIT
>       bool
>  
> +config HAS_DOMAIN_TYPE
> +     bool
> +
>  config PHYS_ADDR_T_32
>       bool

Why here rather than where the bulk of the other HAS_* are?

> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1178,7 +1178,7 @@ int __init make_cpus_node(const struct domain *d, 
> struct kernel_info *kinfo)
>      /* Keep the compiler happy with -Og */
>      bool clock_valid = false;
>      uint64_t mpidr_aff;
> -    void *fdt = kinfo;
> +    void *fdt = kinfo->fdt;
>  
>      dt_dprintk("Create cpus node\n");
>  
> @@ -1774,13 +1774,13 @@ int __init construct_domain(struct domain *d, struct 
> kernel_info *kinfo)
>  
>  #ifdef CONFIG_ARM_64
>      /* if aarch32 mode is not supported at EL1 do not allow 32-bit domain */
> -    if ( !(cpu_has_el1_32) && kinfo->arch.type == DOMAIN_32BIT )
> +    if ( !(cpu_has_el1_32) && kinfo->type == DOMAIN_32BIT )

I'm not an Arm maintainer; if I was, I'd ask for the stray parentheses to be
dropped on this occasion.

> @@ -1896,7 +1896,7 @@ int __init construct_hwdom(struct kernel_info *kinfo,
>  
>  #ifdef CONFIG_ARM_64
>      /* type must be set before allocate_memory */
> -    d->arch.type = kinfo->arch.type;
> +    d->type = kinfo->type;
>  #endif

I wonder why this doesn't use set_domain_type() (see also below).

> --- a/xen/common/device-tree/dom0less-build.c
> +++ b/xen/common/device-tree/dom0less-build.c
> @@ -809,7 +809,10 @@ static int __init construct_domU(struct kernel_info 
> *kinfo,
>      if ( rc < 0 )
>          return rc;
>  
> -    set_domain_type(d, kinfo);
> +#ifdef CONFIG_HAS_DOMAIN_TYPE
> +    /* type must be set before allocate memory */

Nit: Comment style.

> +    d->type = kinfo->type;
> +#endif

Imo it would be preferable to retain set_domain_type(), to avoid #ifdef-ary
here and in construct_hwdom(). Of course its implementation would need to
move. Couldn't it be ...

> --- a/xen/include/xen/dom0less-build.h
> +++ b/xen/include/xen/dom0less-build.h
> @@ -57,8 +57,6 @@ int init_vuart(struct domain *d, struct kernel_info *kinfo,
>  int make_intc_domU_node(struct kernel_info *kinfo);
>  int make_arch_nodes(struct kernel_info *kinfo);
>  
> -void set_domain_type(struct domain *d, struct kernel_info *kinfo);

... an inline function here?

> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -668,6 +668,10 @@ struct domain
>      struct page_info *pending_scrub;
>      unsigned int pending_scrub_order;
>      unsigned int pending_scrub_index;
> +
> +#ifdef CONFIG_HAS_DOMAIN_TYPE
> +    enum domain_type type;
> +#endif
>  } __aligned(PAGE_SIZE);

I'm not quite happy with all new fields getting put at the bottom, when
better options may exist. If the enum was a packed one, it could go next
to domain_id (where 16 bits of padding presently exist). The five *_pages
fields also have a padding field following them (unless MEM_SHARING !=
MEM_PAGING).

Jan