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

[Penny Zheng <penny.zheng@xxxxxxx>]

Hi Michal

On 2023/9/11 17:40, Michal Orzel wrote:
> 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.
yes, the latter would be sufficient. I'll fix it in next version.


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

Right now, bootinfo.reserved_mem is not only containing reserved regions 
described in host FDT /reserved-memory, but also the reserved ones for 
domain only, like in xen,static-mem = <xxx>.
So simply with non-zero bootinfo.reserved_mem.nr_banks, we couldn't tell
whether we had a /reserved-memory node in host FDT.

I agree that kinfo is not a good place to store whether host FDT has a
/reserved-memory node. Maybe bootinfo.has_resv_memory_node?

> ~Michal