Re: [PATCH v4 09/10] xen/arm: fix duplicate /reserved-memory node in Dom0

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Penny,

On 11/09/2023 06:04, Penny Zheng wrote:
> 
> 
> In case there is a /reserved-memory node already present in the host dtb,
> current Xen codes would create yet another /reserved-memory node specially
s/codes/code/

> for the static shm in Dom0 Device Tree.
> 
> Xen will use write_properties() to copy the reserved memory nodes from host 
> dtb
> to Dom0 FDT, so we want to insert the shm node along with the copying.
> And avoiding duplication, we add a checking before make_resv_memory_node().
> 
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> 
> ---
> v3 -> v4:
> new commit
> ---
>  xen/arch/arm/domain_build.c       | 31 ++++++++++++++++++++++++++++---
>  xen/arch/arm/include/asm/kernel.h |  2 ++
>  2 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 02d903be78..dad234e4b5 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1401,6 +1401,10 @@ static int __init handle_linux_pci_domain(struct 
> kernel_info *kinfo,
>      return fdt_property_cell(kinfo->fdt, "linux,pci-domain", segment);
>  }
> 
> +static int __init make_shm_memory_node(const struct domain *d,
> +                                       void *fdt,
> +                                       int addrcells, int sizecells,
> +                                       const struct kernel_info *kinfo);
>  static int __init write_properties(struct domain *d, struct kernel_info 
> *kinfo,
>                                     const struct dt_device_node *node)
>  {
> @@ -1549,6 +1553,23 @@ static int __init write_properties(struct domain *d, 
> struct kernel_info *kinfo,
>          }
>      }
> 
> +    if ( dt_node_path_is_equal(node, "/reserved-memory") )
> +    {
> +        kinfo->resv_mem = true;
kinfo structure is used to store per-domain parameters and presence of reserved 
memory in host dtb
does not fit into this. Moreover, there is no need to introduce yet another 
flag for that given
that in other parts of the code in Xen we use bootinfo.reserved_mem.nr_banks to 
check if there are
reserved regions.

> +
> +        /* shared memory provided. */
> +        if ( kinfo->shminfo.nr_banks != 0 )
> +        {
> +            uint32_t addrcells = dt_n_addr_cells(node),
> +                     sizecells = dt_n_size_cells(node);
> +
> +            res = make_shm_memory_node(d, kinfo->fdt,
> +                                       addrcells, sizecells, kinfo);
I haven't yet looked at previous patches but does it make sense to request 
passing both kinfo->fdt and kinfo?
IMO, the latter would be sufficient. This would apply to other functions you 
modified.

> +            if ( res )
> +                return res;
> +        }
> +    }
> +
>      return 0;
>  }
> 
> @@ -2856,9 +2877,13 @@ static int __init handle_node(struct domain *d, struct 
> kernel_info *kinfo,
>                  return res;
>          }
> 
> -        res = make_resv_memory_node(d, kinfo->fdt, addrcells, sizecells, 
> kinfo);
> -        if ( res )
> -            return res;
> +        /* Avoid duplicate /reserved-memory nodes in Device Tree */
> +        if ( !kinfo->resv_mem )
No need for a new flag as mentioned above. Just before this line of code there 
is a check:
if ( bootinfo.reserved_mem.nr_banks > 0 )
{
    res = make_memory_node(d, kinfo->fdt, addrcells, sizecells,
                            &bootinfo.reserved_mem);
    if ( res )
        return res;
}
so you can just append the following:
else
{
    res = make_resv_memory_node(d, kinfo->fdt, addrcells, sizecells, kinfo);
    if ( res )
        return res;
}
to achieve the same without a need for a new flag.

~Michal