[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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

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
+     * in /reserved-memory node, the other refers to domain on static
+     * 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 
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.


Julien Grall



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.