Re: [PATCH v2 2/6] xen/arm: dom0less hwdom construction

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

On 31/03/2025 23:43, Jason Andryuk wrote:
> When creating a hardware domain, have the dom0less code call
> construct_hwdom() which is shared with the dom0 code.  The hardware
> domain requires building that best matches the dom0 build path.  Re-use
> it to keep them in sync.
> 
> The device tree node of the dom0less config is now passed into
> construct_hwdom().  dom0 uses /chosen for process_shm while a hwdom will
> use the value from its dom0less device tree node.
> 
> Signed-off-by: Jason Andryuk <jason.andryuk@xxxxxxx>
> ---
>  xen/arch/arm/dom0less-build.c           | 57 ++++++++++++++-----------
>  xen/arch/arm/domain_build.c             | 10 +++--
>  xen/arch/arm/include/asm/domain_build.h |  3 +-
>  3 files changed, 42 insertions(+), 28 deletions(-)
> 
> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> index 573b0d25ae..e25d7bd468 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -911,36 +911,45 @@ static int __init construct_domU(struct domain *d,
>      /* type must be set before allocate memory */
>      d->arch.type = kinfo.type;
>  #endif
> -    if ( !dt_find_property(node, "xen,static-mem", NULL) )
> -        allocate_memory(d, &kinfo);
> -    else 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;
> -
> -    /*
> -     * Base address and irq number are needed when creating vpl011 device
> -     * tree node in prepare_dtb_domU, so initialization on related variables
> -     * shall be done first.
> -     */
> -    if ( kinfo.vpl011 )
> +    if ( is_hardware_domain(d) )
>      {
> -        rc = domain_vpl011_init(d, NULL);
> +        rc = construct_hwdom(&kinfo, node);
>          if ( rc < 0 )
>              return rc;
>      }
> +    else
> +    {
> +        if ( !dt_find_property(node, "xen,static-mem", NULL) )
> +            allocate_memory(d, &kinfo);
> +        else if ( !is_domain_direct_mapped(d) )
> +            allocate_static_memory(d, &kinfo, node);
> +        else
> +            assign_static_memory_11(d, &kinfo, node);
>  
> -    rc = prepare_dtb_domU(d, &kinfo);
> -    if ( rc < 0 )
> -        return rc;
> +        rc = process_shm(d, &kinfo, node);
> +        if ( rc < 0 )
> +            return rc;
>  
> -    rc = construct_domain(d, &kinfo);
> -    if ( rc < 0 )
> -        return rc;
> +        /*
> +         * Base address and irq number are needed when creating vpl011 device
> +         * tree node in prepare_dtb_domU, so initialization on related 
> variables
> +         * shall be done first.
> +         */
> +        if ( kinfo.vpl011 )
> +        {
> +            rc = domain_vpl011_init(d, NULL);
> +            if ( rc < 0 )
> +                return rc;
> +        }
> +
> +        rc = prepare_dtb_domU(d, &kinfo);
> +        if ( rc < 0 )
> +            return rc;
> +
> +        rc = construct_domain(d, &kinfo);
> +        if ( rc < 0 )
> +            return rc;
> +    }
>  
>      domain_vcpu_affinity(d, node);
>  
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 051c48329a..b3a396c2fc 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2305,10 +2305,11 @@ static int __init construct_dom0(struct domain *d)
>      if ( rc < 0 )
>          return rc;
>  
> -    return construct_hwdom(&kinfo);
> +    return construct_hwdom(&kinfo, NULL);
>  }
>  
> -int __init construct_hwdom(struct kernel_info *kinfo)
> +int __init construct_hwdom(struct kernel_info *kinfo,
> +                           const struct dt_device_node *node)
>  {
>      struct domain *d = kinfo->d;
>      int rc;
> @@ -2325,7 +2326,10 @@ int __init construct_hwdom(struct kernel_info *kinfo)
>      else
>          allocate_memory(d, kinfo);
>  
> -    rc = process_shm_chosen(d, kinfo);
> +    if ( node )
> +        rc = process_shm(d, kinfo, node);
> +    else
> +        rc = process_shm_chosen(d, kinfo);
I think we can avoid this if/else. I see no reason to keep process_shm_chosen.
We could check in process_shm, whether node is NULL and then grab /chosen node.
I'll try to send a patch shortly.

Rest LGTM.

~Michal