Re: [PATCH v1 2/3] xen/dom0less: refactor architecture-specific DomU construction

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 5/14/25 1:58 AM, Stefano Stabellini wrote:

> On Tue, 13 May 2025, Oleksii Kurochko wrote:
> > Refactor construct_domU() to improve architecture separation and reduce
> > reliance on ARM-specific logic in common code:
> > - Drop set_domain_type() from generic code. This function is specific
> >   to ARM and serves no purpose on other architectures like RISC-V,
> >   which lack the arch.type field in kernel_info.
> > - Introduce arch_construct_domU() to encapsulate architecture-specific
> >   DomU construction steps.
> > - Implement arch_construct_domU() for ARM. This includes:
> >   - Setting the domain type for CONFIG_ARM64.
> >   - Handling static memory allocation if xen,static-mem is present in
> >     the device tree.
> >   - Processing static shared memory.
> > - Move call of make_resv_memory_node() to Arm's make_arch_nodes() as
> >   this call is specific to CONFIG_STATIC_SHM which is ARM specific,
> >   at least, now.
> >
> > This cleanup avoids empty stubs on other architectures and moves
> > ARM-specific logic into arch code where it belongs.
> >
> > Also, don't loose  a return value of functions called in
> > Arm's make_arch_nodes().
> >
> > Suggested-by: Michal Orzel <michal.orzel@xxxxxxx>
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > ---
> >  xen/arch/arm/dom0less-build.c            | 42 +++++++++++++++++-------
> >  xen/common/device-tree/dom0less-build.c  | 30 ++---------------
> >  xen/include/asm-generic/dom0less-build.h |  3 +-
> >  3 files changed, 36 insertions(+), 39 deletions(-)
> >
> > diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> > index a49764f0ad..592173268f 100644
> > --- a/xen/arch/arm/dom0less-build.c
> > +++ b/xen/arch/arm/dom0less-build.c
> > @@ -220,9 +220,14 @@ int __init make_arch_nodes(struct kernel_info *kinfo)
> >  {
> >      int ret;
> >  
> > +    ret = make_resv_memory_node(kinfo, GUEST_ROOT_ADDRESS_CELLS,
> > +                                GUEST_ROOT_SIZE_CELLS);
> > +    if ( ret )
> > +        return ret;
> > +
> >      ret = make_psci_node(kinfo->fdt);
> >      if ( ret )
> > -        return -EINVAL;
> > +        return ret;
> >  
> >      if ( kinfo->arch.vpl011 )
> >      {
> > @@ -230,26 +235,41 @@ int __init make_arch_nodes(struct kernel_info *kinfo)
> >          ret = make_vpl011_uart_node(kinfo);
> >  #endif
> >          if ( ret )
> > -            return -EINVAL;
> > +            return ret;
> >      }
> >  
> >      return 0;
> >  }
> >  
> > -/* TODO: make arch.type generic ? */
> > -#ifdef CONFIG_ARM_64
> > -void __init set_domain_type(struct domain *d, struct kernel_info *kinfo)
> > +int __init arch_construct_domU(struct kernel_info *kinfo,
> > +                               const struct dt_device_node *node)
> >  {
> > +    int rc = 0;
> > +    struct domain *d = kinfo->d;
> > +
> > +#ifdef CONFIG_ARM_64
> >      /* type must be set before allocate memory */
> >      d->arch.type = kinfo->arch.type;
> > -}
> > -#else
> > -void __init set_domain_type(struct domain *d, struct kernel_info *kinfo)
> > -{
> > -    /* Nothing to do */
> > -}
> >  #endif
> >       
> NIT: I think it would be nicer to do 
>
> if ( is_hardware_domain(d) )
>     return rc;
>
> but it is also OK as below
>
> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>     
Thanks.

It would be really nicer, I'll update  that in the next one version.

~ Oleksii
> > +    if ( !is_hardware_domain(d) )
> > +    {
> > +        if ( dt_find_property(node, "xen,static-mem", NULL) )
> > +        {
> > +            if ( !is_domain_direct_mapped(d) )
> > +                allocate_static_memory(d, kinfo, node);
> > +            else
> > +                assign_static_memory_11(d, kinfo, node);
> > +        }
> > +
> > +        rc = process_shm(d, kinfo, node);
> > +        if ( rc < 0 )
> > +            return rc;
> > +    }
> > +
> > +    return rc;
> > +}
> > +
> >  int __init init_vuart(struct domain *d, struct kernel_info *kinfo,
> >                        const struct dt_device_node *node)
> >  {
> > diff --git a/xen/common/device-tree/dom0less-build.c b/xen/common/device-tree/dom0less-build.c
> > index 2c56f13771..f6aabc2093 100644
> > --- a/xen/common/device-tree/dom0less-build.c
> > +++ b/xen/common/device-tree/dom0less-build.c
> > @@ -28,14 +28,6 @@
> >  #include <asm/dom0less-build.h>
> >  #include <asm/setup.h>
> >  
> > -#if __has_include(<asm/static-memory.h>)
> > -#   include <asm/static-memory.h>
> > -#endif
> > -
> > -#if __has_include(<asm/static-shmem.h>)
> > -#   include <asm/static-shmem.h>
> > -#endif
> > -
> >  #define XENSTORE_PFN_LATE_ALLOC UINT64_MAX
> >  
> >  static domid_t __initdata xs_domid = DOMID_INVALID;
> > @@ -507,12 +499,6 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
> >      if ( ret )
> >          goto err;
> >  
> > -#ifdef CONFIG_STATIC_SHM
> > -    ret = make_resv_memory_node(kinfo, addrcells, sizecells);
> > -    if ( ret )
> > -        goto err;
> > -#endif
> > -
> >      /*
> >       * domain_handle_dtb_bootmodule has to be called before the rest of
> >       * the device tree is generated because it depends on the value of
> > @@ -787,7 +773,9 @@ static int __init construct_domU(struct domain *d,
> >      if ( rc < 0 )
> >          return rc;
> >  
> > -    set_domain_type(d, &kinfo);
> > +    rc = arch_construct_domU(&kinfo, node);
> > +    if ( rc )
> > +        return rc;
> >  
> >      if ( is_hardware_domain(d) )
> >      {
> > @@ -799,18 +787,6 @@ static int __init construct_domU(struct domain *d,
> >      {
> >          if ( !dt_find_property(node, "xen,static-mem", NULL) )
> >              allocate_memory(d, &kinfo);
> > -#ifdef CONFIG_STATIC_MEMORY
> > -        else if ( !is_domain_direct_mapped(d) )
> > -            allocate_static_memory(d, &kinfo, node);
> > -        else
> > -            assign_static_memory_11(d, &kinfo, node);
> > -#endif
> > -
> > -#ifdef CONFIG_STATIC_SHM
> > -        rc = process_shm(d, &kinfo, node);
> > -        if ( rc < 0 )
> > -            return rc;
> > -#endif
> >  
> >          rc = init_vuart(d, &kinfo, node);
> >          if ( rc < 0 )
> > diff --git a/xen/include/asm-generic/dom0less-build.h b/xen/include/asm-generic/dom0less-build.h
> > index e0ad0429ec..78142e71ca 100644
> > --- a/xen/include/asm-generic/dom0less-build.h
> > +++ b/xen/include/asm-generic/dom0less-build.h
> > @@ -56,7 +56,8 @@ 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);
> > +int arch_construct_domU(struct kernel_info *kinfo,
> > +                        const struct dt_device_node *node);
> >  
> >  int init_intc_phandle(struct kernel_info *kinfo, const char *name,
> >                        const int node_next, const void *pfdt);
> > -- 
> > 2.49.0
> >
> >       
>