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

[Jan Beulich <jbeulich@suse.com>]

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@gmail.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>

In principle:
Acked-by: Jan Beulich <jbeulich@suse.com>

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?

> +{
> +#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.

Jan