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

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 06-May-26 09:44, Oleksii Kurochko wrote:
> 
> 
> On 5/4/26 2:21 PM, Jan Beulich wrote:
>> On 27.04.2026 17:34, 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.
>>>
>>> Change the guard around access of kinfo->type to CONFIG_HAS_DOMAIN_TYPE
>>> for consistency. Also, drop and add some parentheses to be aligned
>>> with the similar if() below.
>>>
>>> x86 with CONFIG_64BIT=y shouldn't use is_{32,64}bit_domain() as
>>> x86 doesn't have support of CONFIG_HAS_DOMAIN_TYPE. Since x86_32 Xen no
>>> longer builds, the fallback is currently only relevant for arm32.
>>>
>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
>>> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
>>
>> In principle:
>> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Thanks.
> 
>>
>> However, still a few remarks:
>>
>>> --- a/xen/include/xen/domain.h
>>> +++ b/xen/include/xen/domain.h
>>> @@ -13,6 +13,19 @@ struct guest_area {
>>>       void *map;
>>>   };
>>>   
>>> +#ifdef CONFIG_HAS_DOMAIN_TYPE
>>> +enum __packed domain_type {
>>> +    DOMAIN_32BIT,
>>> +    DOMAIN_64BIT,
>>> +};
>>> +#define is_32bit_domain(d) ((d)->type == DOMAIN_32BIT)
>>> +#define is_64bit_domain(d) ((d)->type == DOMAIN_64BIT)
>>> +#elif !defined(CONFIG_64BIT)
>>> +/* At the moment on 32-bit-only platforms all domains are 32-bit. */
>>> +#define is_32bit_domain(d) (true)
>>> +#define is_64bit_domain(d) (false)
>>
>> I think it would be nice if the excess parentheses were dropped from here.
>>
>>> --- a/xen/include/xen/fdt-domain-build.h
>>> +++ b/xen/include/xen/fdt-domain-build.h
>>> @@ -7,6 +7,7 @@
>>>   #include <xen/device_tree.h>
>>>   #include <xen/fdt-kernel.h>
>>>   #include <xen/mm.h>
>>> +#include <xen/sched.h>
>>>   #include <xen/types.h>
>>>   
>>>   struct domain;
>>> @@ -69,6 +70,14 @@ static inline uint32_t alloc_phandle(struct kernel_info 
>>> *kinfo)
>>>       return kinfo->next_phandle >= GUEST_PHANDLE_GIC ? 0 : 
>>> kinfo->next_phandle++;
>>>   }
>>>   
>>> +static inline void set_domain_type(struct domain *d, struct kernel_info 
>>> *kinfo)
>>
>> Pointer-to-const for the 2nd parameter?
> 
> I will apply this comment and comment above.
> 
>>
>>> +{
>>> +#ifdef CONFIG_HAS_DOMAIN_TYPE
>>> +    /* Type must be set before allocate memory */
>>
>> This comment would be more prominent if it lived outside of the #ifdef,
>> perhaps (read on) ahead of the function. I wonder though why it's only
>> a comment, and not e.g. an assertion. If an assertion was possible to
>> add, the comment would want to live next to it. Without an assertion
>> putting it ahead of the function may be better.
>>
>> Depending on how far to go, changes could be made while committing, or a
>> proper v5 may want submitting.
> 
> I think that instead of comment or just after comment the following 
> could be added:
>    ASSERT(!domain_tot_pages(d));
> 
> Jan, Michal, do you see any concern with that ASSERT() or I could add it 
> and keep your Ack-by and R-by.
I don't see any issues with it. You can send a v5 and I'll commit it later on.

~Michal