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

[Penny Zheng <Penny.Zheng@arm.com>]

Hi Julien 

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Wednesday, August 11, 2021 9:48 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>; nd <nd@arm.com>
> 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@arm.com>
> > ---
> >   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.

> Cheers,
> 
> --

Cheers,

--
> Julien Grall