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

[Julien Grall <julien@xen.org>]

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