Re: [PATCH V4 03/10] xen/arm: handle static memory in dt_unreserved_regions

[Julien Grall <julien@xxxxxxx>]

On 16/08/2021 07:00, Penny Zheng wrote:
> Hi Julien
Hi Penny,

> > -----Original Message-----
> > From: Julien Grall <julien@xxxxxxx>
> > Sent: Wednesday, August 11, 2021 9:48 PM
> > To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> > sstabellini@xxxxxxxxxx
> > Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Wei Chen
> > <Wei.Chen@xxxxxxx>; nd <nd@xxxxxxx>
> > Subject: Re: [PATCH V4 03/10] xen/arm: handle static memory in
> > dt_unreserved_regions
> >
> > Hi Penny,
> >
> > On 28/07/2021 11:27, Penny Zheng wrote:
> > > static memory regions overlap with memory nodes. The overlapping
> > > memory is reserved-memory and should be handled accordingly:
> > > dt_unreserved_regions should skip these regions the same way they are
> > > already skipping mem-reserved regions.
> > >
> > > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> > > ---
> > >    xen/arch/arm/setup.c | 47 ++++++++++++++++++++++++++++----------------
> > >    1 file changed, 30 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index
> > > 63a908e325..f569134317 100644
> > > --- a/xen/arch/arm/setup.c
> > > +++ b/xen/arch/arm/setup.c
> > > @@ -200,6 +200,13 @@ static void __init dt_unreserved_regions(paddr_t s,
> > paddr_t e,
> > >                                             int first)
> > >    {
> > >        int i, nr = fdt_num_mem_rsv(device_tree_flattened);
> > > +    /*
> > > +     * There are two types of reserved memory stored in bootinfo, one
> > defines
> > > +     * in /reserved-memory node, the other refers to domain on static
> > allocation
> > > +     * through "xen,static-mem" property.
> > > +     */
> > > +    int nr_rsv_type = 2, t = 0, prev_nr;
> > > +    struct meminfo *rsv_type[2] = {&bootinfo.reserved_mem,
> > > + &bootinfo.static_mem};
> > Looking at the rest of the series, it doesn't look like there is a real 
> > benefits to
> > have the static memory and reserved memory in separate arrays as they are
> > walked only a few times and they are both meant to be small. In fact, I think
> > this code is lot more difficult to read.
> >
> > So it would be best to merge the two arrays in one. We can add a flag in the
> > structure to differentiate between "static" and "reserved" memory.
> How about adding a "static" flag in "struct meminfo" to tell. See the below 
> example:
> "
> struct meminfo {
>      int nr_banks;
>      struct membank bank[NR_MEM_BANKS];
>      bool static;  /* whether memory is reserved as static memory. */
> };
> "
>
> And I will delete "struct meminfo static_mem" array, all "static" and 
> "reserved" memory
> will be stored in one "struct meminfo reserved_mem" array.
Did you intend to suggest to add the new member in struct membank rather 
than struct meminfo?
If not, then I don't understand how this would help have the static and 
reserved region in a single array and avoid the extra loop you added.
Cheers,

--
Julien Grall