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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 4/20/26 10:22 AM, Jan Beulich wrote:
> 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.
I can update that. Just tried to keep changes smaller.

> > 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.
I suggested to rephrase that part to: "Since x86_32 Xen no longer 
builds, the fallback is currently only relevant for arm32."
> > 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.
I will drop that part of the message.

> > --- 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?
Because it is a little arch-specific now as not all arch-s support it.

I can move it to xen/common/Kconfig.

> > --- 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.
They could be dropped. Should be then it mentioned in commit message?

> > @@ -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).
I assume that domain_build.c could be used for both dom0 and dom0less 
but set_domain_type() was used for dom0less. But generally I think it 
shouldn't be a problem to re-use it in construct_hwdom().
If it is fine to ...


> > --- 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?
... move set_domain_type() to <xen/fdt-domain-build.h> then it would be 
fine for me to have the following implementation:
static void inline set_domain_type(struct domain *d, struct kernel_info 
*kinfo)
{
#ifdef CONFIG_HAS_DOMAIN
    /* type must be set before allocate memory */
    d->arch.type = kinfo->arch.type;
#endif
}

and re-use it everywhere it is needed.

> > --- 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).
Just to be sure that I understand correctly what you meant:

enum __attribute__((packed)) domain_type {
    DOMAIN_32BIT,
    DOMAIN_64BIT,
};

struct domain
{
    domid_t          domain_id;

#ifdef CONFIG_HAS_DOMAIN_TYPE
    enum domain_type type;
#endif
 ....

It is what you suggested?

I thought that it make sense only for struct and unions to remove 
padding between members.
Maybe do you mean just declare type member as uint16_t or even uint8_t, 
place it after domain_id and and keep the enum as symbolic constants?
Thanks.

~ Oleksii