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};
 
     for ( i = first; i < nr ; i++ )
     {
@@ -219,26 +226,32 @@ static void __init dt_unreserved_regions(paddr_t s, paddr_t e,
         }
     }
 
-    /*
-     * i is the current bootmodule we are evaluating across all possible
-     * kinds.
-     *
-     * When retrieving the corresponding reserved-memory addresses
-     * below, we need to index the bootinfo.reserved_mem bank starting
-     * from 0, and only counting the reserved-memory modules. Hence,
-     * we need to use i - nr.
-     */
-    for ( ; i - nr < bootinfo.reserved_mem.nr_banks; i++ )
+    prev_nr = nr;
+    while ( t < nr_rsv_type )
     {
-        paddr_t r_s = bootinfo.reserved_mem.bank[i - nr].start;
-        paddr_t r_e = r_s + bootinfo.reserved_mem.bank[i - nr].size;
-
-        if ( s < r_e && r_s < e )
+        /*
+         * i is the current bootmodule we are evaluating across all possible
+         * kinds.
+         *
+         * When retrieving the corresponding reserved-memory addresses
+         * below, we need to index the reserved mem bank starting
+         * from 0, and only counting the reserved-memory modules. Hence,
+         * we need to use i - prev_nr.
+         */
+        i = i - prev_nr;
+        for ( ; i < rsv_type[t]->nr_banks; i++ )
         {
-            dt_unreserved_regions(r_e, e, cb, i + 1);
-            dt_unreserved_regions(s, r_s, cb, i + 1);
-            return;
+            paddr_t r_s = rsv_type[t]->bank[i].start;
+            paddr_t r_e = r_s + rsv_type[t]->bank[i].size;
+
+            if ( s < r_e && r_s < e )
+            {
+                dt_unreserved_regions(r_e, e, cb, i + 1);
+                dt_unreserved_regions(s, r_s, cb, i + 1);
+                return;
+            }
         }
+        prev_nr = rsv_type[t++]->nr_banks;
     }
 
     cb(s, e);
-- 
2.25.1

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.

Cheers,

-- 
Julien Grall

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

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

Hi Penny,

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

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