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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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.
Thanks.

~ Oleksii