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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.04.2026 13:53, Oleksii Kurochko wrote:
> On 4/20/26 10:22 AM, Jan Beulich wrote:
>> On 16.04.2026 16:21, Oleksii Kurochko wrote:
>>> --- 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.

Most HAS_* are there to deal with per-arch differences.

> I can move it to xen/common/Kconfig.

Please do.

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

For something this small I wouldn't insist. But recall that I'm not the one
to ack this part of the change.

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

Yes, just without open-coding of __packed.

> I thought that it make sense only for struct and unions to remove 
> padding between members.

That's the attribute's effect on struct / union, yes. The effect is
different for enum.

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

Preferably use the real type for the field.

Jan