[PATCH 0/4] xen: arm: support up to (almost) 1TB of guest RAM

[PATCH 0/4] xen: arm: support up to (almost) 1TB of guest RAM

[Ian Campbell]

This series rejigs the guest physical address space to allow for up to
1019GB of RAM, with up to 3GB below the 4GB boundary.

The first patch here (an error check) should be backported to 4.4 but
the rest are not suitable IMHO since they change the guest layout.
Although we reserve the right to do so I think we should avoid such
changes in stable branches.

Ian.

[PATCH 1/4] tools: arm: report an error if the guest RAM is too large

[Ian Campbell]

Due to the layout of the guest physical address space we cannot support more
than 768M of RAM before overrunning the area set aside for the grant table. Due
to the presence of the magic pages at the end of the RAM region guests are
actually limited to 767M.

Catch this case during domain build and fail gracefully instead of obscurely
later on.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
This is the only patch in this series which I consider to be suitable for
backporting to Xen 4.4
---
 tools/libxc/xc_dom_arm.c      |    8 ++++++++
 xen/include/public/arch-arm.h |    3 ++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
index 3330f12..c085b4a 100644
--- a/tools/libxc/xc_dom_arm.c
+++ b/tools/libxc/xc_dom_arm.c
@@ -262,6 +262,14 @@ int arch_setup_meminit(struct xc_dom_image *dom)
     const uint64_t modsize = dtb_size + ramdisk_size;
     const uint64_t ram128mb = rambase + (128<<20);
 
+    if ( ramend - 1 > GUEST_RAM_END - NR_MAGIC_PAGES*XC_PAGE_SIZE )
+    {
+        DOMPRINTF("%s: ram size is too large for guest address space: "
+                  "%"PRIx64" > %"PRIx64,
+                  __FUNCTION__, ramend, GUEST_RAM_END);
+        return -1;
+    }
+
     rc = set_mode(dom->xch, dom->guest_domid, dom->guest_type);
     if ( rc )
         return rc;
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 7496556..dc11040 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -369,7 +369,8 @@ typedef uint64_t xen_callback_t;
 #define GUEST_GICC_BASE   0x2c002000ULL
 #define GUEST_GICC_SIZE   0x100ULL
 
-#define GUEST_RAM_BASE    0x80000000ULL
+#define GUEST_RAM_BASE    0x80000000ULL /* 768M at 2GB*/
+#define GUEST_RAM_END     0xafffffffULL
 
 #define GUEST_GNTTAB_BASE 0xb0000000ULL
 #define GUEST_GNTTAB_SIZE 0x00020000ULL
-- 
1.7.10.4

Re: [PATCH 1/4] tools: arm: report an error if the guest RAM is too large

[Julien Grall]

Hi Ian,

On 04/08/2014 03:19 PM, Ian Campbell wrote:
> Due to the layout of the guest physical address space we cannot support more
> than 768M of RAM before overrunning the area set aside for the grant table. Due
> to the presence of the magic pages at the end of the RAM region guests are
> actually limited to 767M.
> 
> Catch this case during domain build and fail gracefully instead of obscurely
> later on.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> This is the only patch in this series which I consider to be suitable for
> backporting to Xen 4.4

Sounds good backport this patch. I'm wondering if patch #2 can also be
backported.

> ---
>  tools/libxc/xc_dom_arm.c      |    8 ++++++++
>  xen/include/public/arch-arm.h |    3 ++-
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
> index 3330f12..c085b4a 100644
> --- a/tools/libxc/xc_dom_arm.c
> +++ b/tools/libxc/xc_dom_arm.c
> @@ -262,6 +262,14 @@ int arch_setup_meminit(struct xc_dom_image *dom)
>      const uint64_t modsize = dtb_size + ramdisk_size;
>      const uint64_t ram128mb = rambase + (128<<20);
>  
> +    if ( ramend - 1 > GUEST_RAM_END - NR_MAGIC_PAGES*XC_PAGE_SIZE )

Can you add parenthesis for more readability?

Regards,

-- 
Julien Grall

Re: [PATCH 1/4] tools: arm: report an error if the guest RAM is too large

[Julien Grall]

Hi Ian,

On 04/08/2014 03:19 PM, Ian Campbell wrote:
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index 7496556..dc11040 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -369,7 +369,8 @@ typedef uint64_t xen_callback_t;
>  #define GUEST_GICC_BASE   0x2c002000ULL
>  #define GUEST_GICC_SIZE   0x100ULL
>  
> -#define GUEST_RAM_BASE    0x80000000ULL
> +#define GUEST_RAM_BASE    0x80000000ULL /* 768M at 2GB*/
> +#define GUEST_RAM_END     0xafffffffULL

I didn't catch this on the first read, every other pairs of define use
_BASE AND _SIZE. Can you be consistent here?

Regards,

-- 
Julien Grall

Re: [PATCH 1/4] tools: arm: report an error if the guest RAM is too large

[Ian Campbell]

On Tue, 2014-04-08 at 15:55 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 04/08/2014 03:19 PM, Ian Campbell wrote:
> > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> > index 7496556..dc11040 100644
> > --- a/xen/include/public/arch-arm.h
> > +++ b/xen/include/public/arch-arm.h
> > @@ -369,7 +369,8 @@ typedef uint64_t xen_callback_t;
> >  #define GUEST_GICC_BASE   0x2c002000ULL
> >  #define GUEST_GICC_SIZE   0x100ULL
> >  
> > -#define GUEST_RAM_BASE    0x80000000ULL
> > +#define GUEST_RAM_BASE    0x80000000ULL /* 768M at 2GB*/
> > +#define GUEST_RAM_END     0xafffffffULL
> 
> I didn't catch this on the first read, every other pairs of define use
> _BASE AND _SIZE. Can you be consistent here?

END is what the user of this particular #define needs.

But in any case SIZE gets added later on.

Ian.

Re: [PATCH 1/4] tools: arm: report an error if the guest RAM is too large

[Julien Grall]

On 04/08/2014 04:06 PM, Ian Campbell wrote:
> On Tue, 2014-04-08 at 15:55 +0100, Julien Grall wrote:
>> Hi Ian,
>>
>> On 04/08/2014 03:19 PM, Ian Campbell wrote:
>>> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
>>> index 7496556..dc11040 100644
>>> --- a/xen/include/public/arch-arm.h
>>> +++ b/xen/include/public/arch-arm.h
>>> @@ -369,7 +369,8 @@ typedef uint64_t xen_callback_t;
>>>  #define GUEST_GICC_BASE   0x2c002000ULL
>>>  #define GUEST_GICC_SIZE   0x100ULL
>>>  
>>> -#define GUEST_RAM_BASE    0x80000000ULL
>>> +#define GUEST_RAM_BASE    0x80000000ULL /* 768M at 2GB*/
>>> +#define GUEST_RAM_END     0xafffffffULL
>>
>> I didn't catch this on the first read, every other pairs of define use
>> _BASE AND _SIZE. Can you be consistent here?
> 
> END is what the user of this particular #define needs.

You can deal with dom->total_pages. Anyway it was just to stay
consistent in the name...

> But in any case SIZE gets added later on.

Oh right, I didn't see it.

-- 
Julien Grall

[PATCH 2/4] xen: arm: move magic pfns out of guest RAM region

[Ian Campbell]

Because toolstacks (at least libxl) only allow RAM to be specified in 1M
increments these two pages were effectively costing 1M of guest RAM space.

Since these pages don't actually need to live in RAM just move them out.

With this a guest can now use the full 768M of the address space reserved
for RAM. (ok, not that impressive, but it simplifies things later)

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxc/xc_dom_arm.c      |   10 +++++-----
 xen/include/public/arch-arm.h |    2 ++
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
index c085b4a..36b1487 100644
--- a/tools/libxc/xc_dom_arm.c
+++ b/tools/libxc/xc_dom_arm.c
@@ -58,12 +58,12 @@ static int setup_pgtables_arm(struct xc_dom_image *dom)
 static int alloc_magic_pages(struct xc_dom_image *dom)
 {
     int rc, i;
+    const xen_pfn_t base = GUEST_MAGIC_BASE >> PAGE_SHIFT;
     xen_pfn_t p2m[NR_MAGIC_PAGES];
-
     DOMPRINTF_CALLED(dom->xch);
 
     for (i = 0; i < NR_MAGIC_PAGES; i++)
-        p2m[i] = dom->rambase_pfn + dom->total_pages + i;
+        p2m[i] = base + i;
 
     rc = xc_domain_populate_physmap_exact(
             dom->xch, dom->guest_domid, NR_MAGIC_PAGES,
@@ -71,8 +71,8 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
     if ( rc < 0 )
         return rc;
 
-    dom->console_pfn = dom->rambase_pfn + dom->total_pages + CONSOLE_PFN_OFFSET;
-    dom->xenstore_pfn = dom->rambase_pfn + dom->total_pages + XENSTORE_PFN_OFFSET;
+    dom->console_pfn = base + CONSOLE_PFN_OFFSET;
+    dom->xenstore_pfn = base + XENSTORE_PFN_OFFSET;
 
     xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_pfn);
     xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_pfn);
@@ -262,7 +262,7 @@ int arch_setup_meminit(struct xc_dom_image *dom)
     const uint64_t modsize = dtb_size + ramdisk_size;
     const uint64_t ram128mb = rambase + (128<<20);
 
-    if ( ramend - 1 > GUEST_RAM_END - NR_MAGIC_PAGES*XC_PAGE_SIZE )
+    if ( ramend - 1 > GUEST_RAM_END )
     {
         DOMPRINTF("%s: ram size is too large for guest address space: "
                   "%"PRIx64" > %"PRIx64,
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index dc11040..b860da5 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -375,6 +375,8 @@ typedef uint64_t xen_callback_t;
 #define GUEST_GNTTAB_BASE 0xb0000000ULL
 #define GUEST_GNTTAB_SIZE 0x00020000ULL
 
+#define GUEST_MAGIC_BASE  0xc0000000ULL
+
 /* Interrupts */
 #define GUEST_TIMER_VIRT_PPI    27
 #define GUEST_TIMER_PHYS_S_PPI  29
-- 
1.7.10.4

Re: [PATCH 2/4] xen: arm: move magic pfns out of guest RAM region

[Julien Grall]

Hi Ian,

On 04/08/2014 03:19 PM, Ian Campbell wrote:
> Because toolstacks (at least libxl) only allow RAM to be specified in 1M
> increments these two pages were effectively costing 1M of guest RAM space.
> 
> Since these pages don't actually need to live in RAM just move them out.
> 
> With this a guest can now use the full 768M of the address space reserved
> for RAM. (ok, not that impressive, but it simplifies things later)
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  tools/libxc/xc_dom_arm.c      |   10 +++++-----
>  xen/include/public/arch-arm.h |    2 ++
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
> index c085b4a..36b1487 100644
> --- a/tools/libxc/xc_dom_arm.c
> +++ b/tools/libxc/xc_dom_arm.c
> @@ -58,12 +58,12 @@ static int setup_pgtables_arm(struct xc_dom_image *dom)
>  static int alloc_magic_pages(struct xc_dom_image *dom)
>  {
>      int rc, i;
> +    const xen_pfn_t base = GUEST_MAGIC_BASE >> PAGE_SHIFT;
>      xen_pfn_t p2m[NR_MAGIC_PAGES];
> -

Spurious change?

Apart this minor question:
Acked-by: Julien Grall <julien.grall@linaro.org>

-- 
Julien Grall

[PATCH 3/4] xen: arm: rearrange guest physical address space to increase max RAM

[Ian Campbell]

By switching things around we can manage to expose up to 3GB of RAM to guests.

I deliberately didn't place the RAM at address 0 to avoid coming to rely on
this, so the various peripherals, MMIO and magic pages etc all live in the
lower 1GB leaving the upper 3GB available for RAM.

It would likely have been possible to reduce the space used by the peripherals
etc and allow for 3.5 or 3.75GB but I decided to keep things simple and will
handle >3GB memory in a subsequent patch.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/include/public/arch-arm.h |   18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index b860da5..5840453 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -364,18 +364,18 @@ typedef uint64_t xen_callback_t;
  */
 
 /* Physical Address Space */
-#define GUEST_GICD_BASE   0x2c001000ULL
-#define GUEST_GICD_SIZE   0x1000ULL
-#define GUEST_GICC_BASE   0x2c002000ULL
-#define GUEST_GICC_SIZE   0x100ULL
+#define GUEST_GICD_BASE   0x03001000ULL
+#define GUEST_GICD_SIZE   0x00001000ULL
+#define GUEST_GICC_BASE   0x03002000ULL
+#define GUEST_GICC_SIZE   0x00000100ULL
 
-#define GUEST_RAM_BASE    0x80000000ULL /* 768M at 2GB*/
-#define GUEST_RAM_END     0xafffffffULL
-
-#define GUEST_GNTTAB_BASE 0xb0000000ULL
+#define GUEST_GNTTAB_BASE 0x38000000ULL
 #define GUEST_GNTTAB_SIZE 0x00020000ULL
 
-#define GUEST_MAGIC_BASE  0xc0000000ULL
+#define GUEST_MAGIC_BASE  0x39000000ULL
+
+#define GUEST_RAM_BASE    0x40000000ULL /* 3GB of RAM @ 1GB */
+#define GUEST_RAM_END     0xffffffffULL
 
 /* Interrupts */
 #define GUEST_TIMER_VIRT_PPI    27
-- 
1.7.10.4

Re: [PATCH 3/4] xen: arm: rearrange guest physical address space to increase max RAM

[Julien Grall]

Hi Ian,

On 04/08/2014 03:19 PM, Ian Campbell wrote:
> By switching things around we can manage to expose up to 3GB of RAM to guests.
> 
> I deliberately didn't place the RAM at address 0 to avoid coming to rely on
> this, so the various peripherals, MMIO and magic pages etc all live in the
> lower 1GB leaving the upper 3GB available for RAM.
> 
> It would likely have been possible to reduce the space used by the peripherals
> etc and allow for 3.5 or 3.75GB but I decided to keep things simple and will
> handle >3GB memory in a subsequent patch.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/include/public/arch-arm.h |   18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index b860da5..5840453 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -364,18 +364,18 @@ typedef uint64_t xen_callback_t;
>   */
>  
>  /* Physical Address Space */
> -#define GUEST_GICD_BASE   0x2c001000ULL
> -#define GUEST_GICD_SIZE   0x1000ULL
> -#define GUEST_GICC_BASE   0x2c002000ULL
> -#define GUEST_GICC_SIZE   0x100ULL
> +#define GUEST_GICD_BASE   0x03001000ULL
> +#define GUEST_GICD_SIZE   0x00001000ULL
> +#define GUEST_GICC_BASE   0x03002000ULL
> +#define GUEST_GICC_SIZE   0x00000100ULL
>  
> -#define GUEST_RAM_BASE    0x80000000ULL /* 768M at 2GB*/
> -#define GUEST_RAM_END     0xafffffffULL
> -
> -#define GUEST_GNTTAB_BASE 0xb0000000ULL
> +#define GUEST_GNTTAB_BASE 0x38000000ULL
>  #define GUEST_GNTTAB_SIZE 0x00020000ULL

Not related to this patch... while you are re-working the guest layout.
Can you comment where does come from the GNTTAB_SIZE...?

Also, can you make sure that the GNTTAB_SIZE is greater or equal to the
maximum number of frames (maybe by overriding max_nr_grant_frames)?

The current implementation on Linux only care about the number of frames
given by Xen, the size of the table in the DT is not used. So the range
may overlap to something else.

Regards,

-- 
Julien Grall

Re: [PATCH 3/4] xen: arm: rearrange guest physical address space to increase max RAM

[Ian Campbell]

On Tue, 2014-04-08 at 16:12 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 04/08/2014 03:19 PM, Ian Campbell wrote:
> > By switching things around we can manage to expose up to 3GB of RAM to guests.
> > 
> > I deliberately didn't place the RAM at address 0 to avoid coming to rely on
> > this, so the various peripherals, MMIO and magic pages etc all live in the
> > lower 1GB leaving the upper 3GB available for RAM.
> > 
> > It would likely have been possible to reduce the space used by the peripherals
> > etc and allow for 3.5 or 3.75GB but I decided to keep things simple and will
> > handle >3GB memory in a subsequent patch.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  xen/include/public/arch-arm.h |   18 +++++++++---------
> >  1 file changed, 9 insertions(+), 9 deletions(-)
> > 
> > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> > index b860da5..5840453 100644
> > --- a/xen/include/public/arch-arm.h
> > +++ b/xen/include/public/arch-arm.h
> > @@ -364,18 +364,18 @@ typedef uint64_t xen_callback_t;
> >   */
> >  
> >  /* Physical Address Space */
> > -#define GUEST_GICD_BASE   0x2c001000ULL
> > -#define GUEST_GICD_SIZE   0x1000ULL
> > -#define GUEST_GICC_BASE   0x2c002000ULL
> > -#define GUEST_GICC_SIZE   0x100ULL
> > +#define GUEST_GICD_BASE   0x03001000ULL
> > +#define GUEST_GICD_SIZE   0x00001000ULL
> > +#define GUEST_GICC_BASE   0x03002000ULL
> > +#define GUEST_GICC_SIZE   0x00000100ULL
> >  
> > -#define GUEST_RAM_BASE    0x80000000ULL /* 768M at 2GB*/
> > -#define GUEST_RAM_END     0xafffffffULL
> > -
> > -#define GUEST_GNTTAB_BASE 0xb0000000ULL
> > +#define GUEST_GNTTAB_BASE 0x38000000ULL
> >  #define GUEST_GNTTAB_SIZE 0x00020000ULL
> 
> Not related to this patch... while you are re-working the guest layout.
> Can you comment where does come from the GNTTAB_SIZE...?

Stefano added that one, I assume he made it up...

> Also, can you make sure that the GNTTAB_SIZE is greater or equal to the
> maximum number of frames (maybe by overriding max_nr_grant_frames)?

It turns out that the current size corresponds to
DEFAULT_MAX_NR_GRANT_FRAMES, which explains where it came from.

If you want to change this to reserve say 1MB of address space (which is
enough for 256 grant pages) or even more then please send a patch.

> The current implementation on Linux only care about the number of frames
> given by Xen, the size of the table in the DT is not used. So the range
> may overlap to something else.

That would be a guest bug, but nothing to do with this series.

Ian.

Re: [PATCH 3/4] xen: arm: rearrange guest physical address space to increase max RAM

[Julien Grall]

On 04/08/2014 04:22 PM, Ian Campbell wrote:
> On Tue, 2014-04-08 at 16:12 +0100, Julien Grall wrote:
>> Hi Ian,
>>
>> On 04/08/2014 03:19 PM, Ian Campbell wrote:
>>> By switching things around we can manage to expose up to 3GB of RAM to guests.
>>>
>>> I deliberately didn't place the RAM at address 0 to avoid coming to rely on
>>> this, so the various peripherals, MMIO and magic pages etc all live in the
>>> lower 1GB leaving the upper 3GB available for RAM.
>>>
>>> It would likely have been possible to reduce the space used by the peripherals
>>> etc and allow for 3.5 or 3.75GB but I decided to keep things simple and will
>>> handle >3GB memory in a subsequent patch.
>>>
>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>> ---
>>>  xen/include/public/arch-arm.h |   18 +++++++++---------
>>>  1 file changed, 9 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
>>> index b860da5..5840453 100644
>>> --- a/xen/include/public/arch-arm.h
>>> +++ b/xen/include/public/arch-arm.h
>>> @@ -364,18 +364,18 @@ typedef uint64_t xen_callback_t;
>>>   */
>>>  
>>>  /* Physical Address Space */
>>> -#define GUEST_GICD_BASE   0x2c001000ULL
>>> -#define GUEST_GICD_SIZE   0x1000ULL
>>> -#define GUEST_GICC_BASE   0x2c002000ULL
>>> -#define GUEST_GICC_SIZE   0x100ULL
>>> +#define GUEST_GICD_BASE   0x03001000ULL
>>> +#define GUEST_GICD_SIZE   0x00001000ULL
>>> +#define GUEST_GICC_BASE   0x03002000ULL
>>> +#define GUEST_GICC_SIZE   0x00000100ULL
>>>  
>>> -#define GUEST_RAM_BASE    0x80000000ULL /* 768M at 2GB*/
>>> -#define GUEST_RAM_END     0xafffffffULL
>>> -
>>> -#define GUEST_GNTTAB_BASE 0xb0000000ULL
>>> +#define GUEST_GNTTAB_BASE 0x38000000ULL
>>>  #define GUEST_GNTTAB_SIZE 0x00020000ULL
>>
>> Not related to this patch... while you are re-working the guest layout.
>> Can you comment where does come from the GNTTAB_SIZE...?
> 
> Stefano added that one, I assume he made it up...

I didn't find any documentation in the code about it.

>> Also, can you make sure that the GNTTAB_SIZE is greater or equal to the
>> maximum number of frames (maybe by overriding max_nr_grant_frames)?
> 
> It turns out that the current size corresponds to
> DEFAULT_MAX_NR_GRANT_FRAMES, which explains where it came from.

I know it... a comment in the code here would be great to avoid loosing
20mins every time we hit this define.

> If you want to change this to reserve say 1MB of address space (which is
> enough for 256 grant pages) or even more then please send a patch.
> 
>> The current implementation on Linux only care about the number of frames
>> given by Xen, the size of the table in the DT is not used. So the range
>> may overlap to something else.
> 
> That would be a guest bug, but nothing to do with this series.

It's not really a guest bug ... we have an hypercall which provides the
GNTTAB size (see gnttab_query_size).

It returns max_nr_grant_frames which can be modified by the Xen command
line.

-- 
Julien Grall

Re: [PATCH 3/4] xen: arm: rearrange guest physical address space to increase max RAM

[Ian Campbell]

On Tue, 2014-04-08 at 17:01 +0100, Julien Grall wrote:

> >> Also, can you make sure that the GNTTAB_SIZE is greater or equal to the
> >> maximum number of frames (maybe by overriding max_nr_grant_frames)?
> > 
> > It turns out that the current size corresponds to
> > DEFAULT_MAX_NR_GRANT_FRAMES, which explains where it came from.
> 
> I know it... a comment in the code here would be great to avoid loosing
> 20mins every time we hit this define.

Feel free to send a patch.

> > If you want to change this to reserve say 1MB of address space (which is
> > enough for 256 grant pages) or even more then please send a patch.
> > 
> >> The current implementation on Linux only care about the number of frames
> >> given by Xen, the size of the table in the DT is not used. So the range
> >> may overlap to something else.
> > 
> > That would be a guest bug, but nothing to do with this series.
> 
> It's not really a guest bug ... we have an hypercall which provides the
> GNTTAB size (see gnttab_query_size).
> 
> It returns max_nr_grant_frames which can be modified by the Xen command
> line.

The tools could query this and use it to size the region, or we could
just make the region be more than large enough. Patches welcome.

Ian.

[PATCH 4/4] xen: arm: support up to (almost) 1TB of guest RAM

[Ian Campbell]

This creates a second bank of RAM starting at 8GB and potentially extending to
the 1TB boundary, which is the limit imposed by our current use of a 3 level
p2m with 2 pages at level 0 (2^40 bits).

I've deliberately left a gap between the two banks just to exercise those code paths.

The second bank is 1016GB in size which plus the 3GB below 4GB is 1019GB
maximum guest RAM. At the point where the fact that this is slightly less than
a full TB starts to become an issue for people then we can switch to a 4 level
p2m, which would be needed to support guests larger than 1TB anyhow.

Tested on 32-bit with 1, 4 and 6GB guests. Anything more than ~3GB requires an
LPAE enabled kernel, or a 64-bit guest.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxc/xc_dom_arm.c      |   95 +++++++++++++++++++++++++++++------------
 tools/libxl/libxl_arm.c       |   27 +++++++++---
 xen/include/public/arch-arm.h |   13 +++++-
 3 files changed, 100 insertions(+), 35 deletions(-)

diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
index 36b1487..65464c7 100644
--- a/tools/libxc/xc_dom_arm.c
+++ b/tools/libxc/xc_dom_arm.c
@@ -18,6 +18,7 @@
  * Copyright (c) 2011, Citrix Systems
  */
 #include <inttypes.h>
+#include <assert.h>
 
 #include <xen/xen.h>
 #include <xen/io/protocols.h>
@@ -245,28 +246,73 @@ static int set_mode(xc_interface *xch, domid_t domid, char *guest_type)
     return rc;
 }
 
+static int populate_guest_memory(struct xc_dom_image *dom,
+                                 xen_pfn_t base_pfn, xen_pfn_t nr_pfns)
+{
+    int rc;
+    xen_pfn_t allocsz, pfn;
+
+    if (!nr_pfns)
+        return 0;
+
+    DOMPRINTF("%s: populating RAM @ %016"PRIx64"-%016"PRIx64" (%"PRId64"MB)",
+              __FUNCTION__,
+              (uint64_t)base_pfn << XC_PAGE_SHIFT,
+              (uint64_t)(base_pfn + nr_pfns) << XC_PAGE_SHIFT,
+              (uint64_t)nr_pfns >> (20-XC_PAGE_SHIFT));
+
+    for ( pfn = 0; pfn < nr_pfns; pfn++ )
+        dom->p2m_host[pfn] = base_pfn + pfn;
+
+    for ( pfn = rc = allocsz = 0; (pfn < nr_pfns) && !rc; pfn += allocsz )
+    {
+        allocsz = nr_pfns - pfn;
+        if ( allocsz > 1024*1024 )
+            allocsz = 1024*1024;
+
+        rc = xc_domain_populate_physmap_exact(
+            dom->xch, dom->guest_domid, allocsz,
+            0, 0, &dom->p2m_host[pfn]);
+    }
+
+    return rc;
+}
+
 int arch_setup_meminit(struct xc_dom_image *dom)
 {
     int rc;
-    xen_pfn_t pfn, allocsz, i;
+    xen_pfn_t pfn;
     uint64_t modbase;
 
     /* Convenient */
-    const uint64_t rambase = dom->rambase_pfn << XC_PAGE_SHIFT;
-    const uint64_t ramend = rambase + ( dom->total_pages << XC_PAGE_SHIFT );
+    const uint64_t ramsize = (uint64_t)dom->total_pages << XC_PAGE_SHIFT;
+
+    const uint64_t ram0size =
+        ramsize > GUEST_RAM0_SIZE ? GUEST_RAM0_SIZE : ramsize;
+    const uint64_t ram0end = GUEST_RAM0_BASE + ram0size;
+    const uint64_t ram1size =
+        ramsize > ram0size ? ramsize - ram0size : 0;
+    const uint64_t ram1end = GUEST_RAM1_BASE + ram1size;
+
+    const xen_pfn_t p2m_size = ram1size ?
+        (ram1end - GUEST_RAM0_BASE) >> XC_PAGE_SHIFT :
+        (ram0end - GUEST_RAM0_BASE) >> XC_PAGE_SHIFT;
+
     const uint64_t kernend = ROUNDUP(dom->kernel_seg.vend, 21/*2MB*/);
     const uint64_t dtb_size = dom->devicetree_blob ?
         ROUNDUP(dom->devicetree_size, XC_PAGE_SHIFT) : 0;
     const uint64_t ramdisk_size = dom->ramdisk_blob ?
         ROUNDUP(dom->ramdisk_size, XC_PAGE_SHIFT) : 0;
     const uint64_t modsize = dtb_size + ramdisk_size;
-    const uint64_t ram128mb = rambase + (128<<20);
+    const uint64_t ram128mb = GUEST_RAM0_BASE + (128<<20);
 
-    if ( ramend - 1 > GUEST_RAM_END )
+    assert(dom->rambase_pfn << XC_PAGE_SHIFT == GUEST_RAM0_BASE);
+
+    if ( ramsize > GUEST_RAM_MAX )
     {
         DOMPRINTF("%s: ram size is too large for guest address space: "
                   "%"PRIx64" > %"PRIx64,
-                  __FUNCTION__, ramend, GUEST_RAM_END);
+                  __FUNCTION__, ramsize, GUEST_RAM_MAX);
         return -1;
     }
 
@@ -276,38 +322,31 @@ int arch_setup_meminit(struct xc_dom_image *dom)
 
     dom->shadow_enabled = 1;
 
-    dom->p2m_host = xc_dom_malloc(dom, sizeof(xen_pfn_t) * dom->total_pages);
+    dom->p2m_host = xc_dom_malloc(dom, sizeof(xen_pfn_t) * p2m_size);
     if ( dom->p2m_host == NULL )
         return -EINVAL;
+    for ( pfn = 0; pfn < p2m_size; pfn++ )
+        dom->p2m_host[pfn] = INVALID_MFN;
 
-    /* setup initial p2m */
-    for ( pfn = 0; pfn < dom->total_pages; pfn++ )
-        dom->p2m_host[pfn] = pfn + dom->rambase_pfn;
-
-    /* allocate guest memory */
-    for ( i = rc = allocsz = 0;
-          (i < dom->total_pages) && !rc;
-          i += allocsz )
-    {
-        allocsz = dom->total_pages - i;
-        if ( allocsz > 1024*1024 )
-            allocsz = 1024*1024;
-
-        rc = xc_domain_populate_physmap_exact(
-            dom->xch, dom->guest_domid, allocsz,
-            0, 0, &dom->p2m_host[i]);
-    }
-
+    /* setup initial p2m and allocate guest memory */
+    if ((rc = populate_guest_memory(dom,
+                                    GUEST_RAM0_BASE >> XC_PAGE_SHIFT,
+                                    ram0size >> XC_PAGE_SHIFT)))
+        return rc;
+    if ((rc = populate_guest_memory(dom,
+                                    GUEST_RAM1_BASE >> XC_PAGE_SHIFT,
+                                    ram1size >> XC_PAGE_SHIFT)))
+        return rc;
 
     /*
-     * Place boot modules at 128MB into RAM if there is enough RAM and
+     * Place boot modules at 128MB into low RAM if there is enough RAM and
      * the kernel does not overlap. Otherwise place them immediately
      * after the kernel. If there is no space after the kernel then
      * there is insufficient RAM and we fail.
      */
-    if ( ramend >= ram128mb + modsize && kernend < ram128mb )
+    if ( ram0end >= ram128mb + modsize && kernend < ram128mb )
         modbase = ram128mb;
-    else if ( ramend >= kernend + modsize )
+    else if ( ram0end >= kernend + modsize )
         modbase = kernend;
     else
         return -1;
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index 4f0f0e2..6170454 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -255,9 +255,9 @@ static int make_psci_node(libxl__gc *gc, void *fdt)
     return 0;
 }
 
-static int make_memory_node(libxl__gc *gc, void *fdt,
-                            unsigned long long base,
-                            unsigned long long size)
+static int make_one_memory_node(libxl__gc *gc, void *fdt,
+                                unsigned long long base,
+                                unsigned long long size)
 {
     int res;
     const char *name = GCSPRINTF("memory@%08llx", base);
@@ -269,7 +269,7 @@ static int make_memory_node(libxl__gc *gc, void *fdt,
     if (res) return res;
 
     res = fdt_property_regs(gc, fdt, ROOT_ADDRESS_CELLS, ROOT_SIZE_CELLS,
-                            1, (uint64_t)base, (uint64_t)size);
+                            1, base, size);
     if (res) return res;
 
     res = fdt_end_node(fdt);
@@ -278,6 +278,24 @@ static int make_memory_node(libxl__gc *gc, void *fdt,
     return 0;
 }
 
+static int make_memory_node(libxl__gc *gc, void *fdt,
+                            unsigned long long size)
+{
+    int res;
+    /* This had better match libxc's arch_setup_meminit... */
+    const uint64_t size0 = size > GUEST_RAM0_SIZE ? GUEST_RAM0_SIZE : size;
+    const uint64_t size1 = size > GUEST_RAM0_SIZE ? size - size0 : 0;
+
+    res = make_one_memory_node(gc, fdt, GUEST_RAM0_BASE, size0);
+    if (res) return res;
+    if (size1) {
+        res = make_one_memory_node(gc, fdt, GUEST_RAM1_BASE, size1);
+        if (res) return res;
+    }
+
+    return 0;
+}
+
 static int make_intc_node(libxl__gc *gc, void *fdt,
                           unsigned long long gicd_base,
                           unsigned long long gicd_size,
@@ -493,7 +511,6 @@ next_resize:
         FDT( make_psci_node(gc, fdt) );
 
         FDT( make_memory_node(gc, fdt,
-                              dom->rambase_pfn << XC_PAGE_SHIFT,
                               info->target_memkb * 1024) );
         FDT( make_intc_node(gc, fdt,
                             GUEST_GICD_BASE, GUEST_GICD_SIZE,
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 5840453..d15a977 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -374,8 +374,17 @@ typedef uint64_t xen_callback_t;
 
 #define GUEST_MAGIC_BASE  0x39000000ULL
 
-#define GUEST_RAM_BASE    0x40000000ULL /* 3GB of RAM @ 1GB */
-#define GUEST_RAM_END     0xffffffffULL
+#define GUEST_RAM0_BASE   0x40000000ULL /* 3GB of low RAM @ 1GB */
+#define GUEST_RAM0_SIZE   0xc0000000ULL
+#define GUEST_RAM0_END    0xffffffffULL
+
+#define GUEST_RAM1_BASE   0x0200000000ULL /* 1016GB of RAM @ 8GB */
+#define GUEST_RAM1_SIZE   0xfe00000000ULL
+#define GUEST_RAM1_END    0xffffffffffULL
+
+#define GUEST_RAM_BASE    GUEST_RAM0_BASE /* Lowest RAM address */
+ /* Largest amount of actual RAM, not including holes */
+#define GUEST_RAM_MAX     (GUEST_RAM0_SIZE + GUEST_RAM1_SIZE)
 
 /* Interrupts */
 #define GUEST_TIMER_VIRT_PPI    27
-- 
1.7.10.4

Re: [PATCH 4/4] xen: arm: support up to (almost) 1TB of guest RAM

[Julien Grall]

Hi Ian,

On 04/08/2014 03:19 PM, Ian Campbell wrote:
> This creates a second bank of RAM starting at 8GB and potentially extending to
> the 1TB boundary, which is the limit imposed by our current use of a 3 level
> p2m with 2 pages at level 0 (2^40 bits).
> 
> I've deliberately left a gap between the two banks just to exercise those code paths.
> 
> The second bank is 1016GB in size which plus the 3GB below 4GB is 1019GB
> maximum guest RAM. At the point where the fact that this is slightly less than
> a full TB starts to become an issue for people then we can switch to a 4 level
> p2m, which would be needed to support guests larger than 1TB anyhow.
> 
> Tested on 32-bit with 1, 4 and 6GB guests. Anything more than ~3GB requires an
> LPAE enabled kernel, or a 64-bit guest.

What happen if you try to boot a non-LPAE guest with more than ~3GB?
Does it boot or crash?

> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  tools/libxc/xc_dom_arm.c      |   95 +++++++++++++++++++++++++++++------------
>  tools/libxl/libxl_arm.c       |   27 +++++++++---
>  xen/include/public/arch-arm.h |   13 +++++-
>  3 files changed, 100 insertions(+), 35 deletions(-)
> 
> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
> index 36b1487..65464c7 100644
> --- a/tools/libxc/xc_dom_arm.c
> +++ b/tools/libxc/xc_dom_arm.c
> @@ -18,6 +18,7 @@
>   * Copyright (c) 2011, Citrix Systems
>   */
>  #include <inttypes.h>
> +#include <assert.h>
>  
>  #include <xen/xen.h>
>  #include <xen/io/protocols.h>
> @@ -245,28 +246,73 @@ static int set_mode(xc_interface *xch, domid_t domid, char *guest_type)
>      return rc;
>  }
>  
> +static int populate_guest_memory(struct xc_dom_image *dom,
> +                                 xen_pfn_t base_pfn, xen_pfn_t nr_pfns)
> +{
> +    int rc;
> +    xen_pfn_t allocsz, pfn;
> +
> +    if (!nr_pfns)

if ( !nr_pfns )

> +        return 0;
> +
> +    DOMPRINTF("%s: populating RAM @ %016"PRIx64"-%016"PRIx64" (%"PRId64"MB)",
> +              __FUNCTION__,
> +              (uint64_t)base_pfn << XC_PAGE_SHIFT,
> +              (uint64_t)(base_pfn + nr_pfns) << XC_PAGE_SHIFT,
> +              (uint64_t)nr_pfns >> (20-XC_PAGE_SHIFT));
> +
> +    for ( pfn = 0; pfn < nr_pfns; pfn++ )
> +        dom->p2m_host[pfn] = base_pfn + pfn;
> +
> +    for ( pfn = rc = allocsz = 0; (pfn < nr_pfns) && !rc; pfn += allocsz )

It's very confusing to see pfn = rc = allocsz = 0 and each are not
related. Any reason to not initialized rc/allocsz earlier?

> +    {
> +        allocsz = nr_pfns - pfn;

In fact you are using allocsz here... so you don't need to initialize it.

> +        if ( allocsz > 1024*1024 )
> +            allocsz = 1024*1024;
> +
> +        rc = xc_domain_populate_physmap_exact(
> +            dom->xch, dom->guest_domid, allocsz,
> +            0, 0, &dom->p2m_host[pfn]);
> +    }
> +
> +    return rc;
> +}
> +
>  int arch_setup_meminit(struct xc_dom_image *dom)
>  {
>      int rc;
> -    xen_pfn_t pfn, allocsz, i;
> +    xen_pfn_t pfn;
>      uint64_t modbase;
>  
>      /* Convenient */
> -    const uint64_t rambase = dom->rambase_pfn << XC_PAGE_SHIFT;
> -    const uint64_t ramend = rambase + ( dom->total_pages << XC_PAGE_SHIFT );
> +    const uint64_t ramsize = (uint64_t)dom->total_pages << XC_PAGE_SHIFT;
> +
> +    const uint64_t ram0size =
> +        ramsize > GUEST_RAM0_SIZE ? GUEST_RAM0_SIZE : ramsize;
> +    const uint64_t ram0end = GUEST_RAM0_BASE + ram0size;
> +    const uint64_t ram1size =
> +        ramsize > ram0size ? ramsize - ram0size : 0;
> +    const uint64_t ram1end = GUEST_RAM1_BASE + ram1size;
> +
> +    const xen_pfn_t p2m_size = ram1size ?
> +        (ram1end - GUEST_RAM0_BASE) >> XC_PAGE_SHIFT :
> +        (ram0end - GUEST_RAM0_BASE) >> XC_PAGE_SHIFT;
> +
>      const uint64_t kernend = ROUNDUP(dom->kernel_seg.vend, 21/*2MB*/);
>      const uint64_t dtb_size = dom->devicetree_blob ?
>          ROUNDUP(dom->devicetree_size, XC_PAGE_SHIFT) : 0;
>      const uint64_t ramdisk_size = dom->ramdisk_blob ?
>          ROUNDUP(dom->ramdisk_size, XC_PAGE_SHIFT) : 0;
>      const uint64_t modsize = dtb_size + ramdisk_size;
> -    const uint64_t ram128mb = rambase + (128<<20);
> +    const uint64_t ram128mb = GUEST_RAM0_BASE + (128<<20);
>  
> -    if ( ramend - 1 > GUEST_RAM_END )
> +    assert(dom->rambase_pfn << XC_PAGE_SHIFT == GUEST_RAM0_BASE);

Can you add parenthesis here for readability?

[..]

> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> index 4f0f0e2..6170454 100644
> --- a/tools/libxl/libxl_arm.c
> +++ b/tools/libxl/libxl_arm.c
> @@ -255,9 +255,9 @@ static int make_psci_node(libxl__gc *gc, void *fdt)
>      return 0;
>  }
>  
> -static int make_memory_node(libxl__gc *gc, void *fdt,
> -                            unsigned long long base,
> -                            unsigned long long size)> 
> +static int make_one_memory_node(libxl__gc *gc, void *fdt,
> +                                unsigned long long base,
> +                                unsigned long long size)

I would use uint64_t rather unsigned long long. It's more clear the
base/size are 64 bits.

>  {
>      int res;
>      const char *name = GCSPRINTF("memory@%08llx", base);
> @@ -269,7 +269,7 @@ static int make_memory_node(libxl__gc *gc, void *fdt,
>      if (res) return res;
>  
>      res = fdt_property_regs(gc, fdt, ROOT_ADDRESS_CELLS, ROOT_SIZE_CELLS,
> -                            1, (uint64_t)base, (uint64_t)size);
> +                            1, base, size);
>      if (res) return res;
>  
>      res = fdt_end_node(fdt);
> @@ -278,6 +278,24 @@ static int make_memory_node(libxl__gc *gc, void *fdt,
>      return 0;
>  }
>  
> +static int make_memory_node(libxl__gc *gc, void *fdt,
> +                            unsigned long long size)

Same here.

> +{
> +    int res;
> +    /* This had better match libxc's arch_setup_meminit... */
> +    const uint64_t size0 = size > GUEST_RAM0_SIZE ? GUEST_RAM0_SIZE : size;
> +    const uint64_t size1 = size > GUEST_RAM0_SIZE ? size - size0 : 0;
> +
> +    res = make_one_memory_node(gc, fdt, GUEST_RAM0_BASE, size0);
> +    if (res) return res;
> +    if (size1) {
> +        res = make_one_memory_node(gc, fdt, GUEST_RAM1_BASE, size1);
> +        if (res) return res;
> +    }

Any reason to not gather the both bank in one memory node?

Regards,

-- 
Julien Grall

Re: [PATCH 4/4] xen: arm: support up to (almost) 1TB of guest RAM

[Ian Campbell]

On Tue, 2014-04-08 at 16:35 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 04/08/2014 03:19 PM, Ian Campbell wrote:
> > This creates a second bank of RAM starting at 8GB and potentially extending to
> > the 1TB boundary, which is the limit imposed by our current use of a 3 level
> > p2m with 2 pages at level 0 (2^40 bits).
> > 
> > I've deliberately left a gap between the two banks just to exercise those code paths.
> > 
> > The second bank is 1016GB in size which plus the 3GB below 4GB is 1019GB
> > maximum guest RAM. At the point where the fact that this is slightly less than
> > a full TB starts to become an issue for people then we can switch to a 4 level
> > p2m, which would be needed to support guests larger than 1TB anyhow.
> > 
> > Tested on 32-bit with 1, 4 and 6GB guests. Anything more than ~3GB requires an
> > LPAE enabled kernel, or a 64-bit guest.
> 
> What happen if you try to boot a non-LPAE guest with more than ~3GB?
> Does it boot or crash?

It logs a message in dmesg about either truncating or ignoring banks and
then boots with the lesser amount of ram. Logically it has too so you
can install on native using a distro kernel even on a big box and have
the installed choose the LPAE kernel for you.

> > +        return 0;
> > +
> > +    DOMPRINTF("%s: populating RAM @ %016"PRIx64"-%016"PRIx64" (%"PRId64"MB)",
> > +              __FUNCTION__,
> > +              (uint64_t)base_pfn << XC_PAGE_SHIFT,
> > +              (uint64_t)(base_pfn + nr_pfns) << XC_PAGE_SHIFT,
> > +              (uint64_t)nr_pfns >> (20-XC_PAGE_SHIFT));
> > +
> > +    for ( pfn = 0; pfn < nr_pfns; pfn++ )
> > +        dom->p2m_host[pfn] = base_pfn + pfn;
> > +
> > +    for ( pfn = rc = allocsz = 0; (pfn < nr_pfns) && !rc; pfn += allocsz )
> 
> It's very confusing to see pfn = rc = allocsz = 0 and each are not
> related. Any reason to not initialized rc/allocsz earlier?

This was mainly code motion, the original loop was the same.  I can try
and clean it up a bit though.

> > +    {
> > +        allocsz = nr_pfns - pfn;
> 
> In fact you are using allocsz here... so you don't need to initialize it.

I'll see if the compiler complains about the use in the for loop
increment.

> > +    assert(dom->rambase_pfn << XC_PAGE_SHIFT == GUEST_RAM0_BASE);
> 
> Can you add parenthesis here for readability?

OK.

> 
> [..]
> 
> > diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> > index 4f0f0e2..6170454 100644
> > --- a/tools/libxl/libxl_arm.c
> > +++ b/tools/libxl/libxl_arm.c
> > @@ -255,9 +255,9 @@ static int make_psci_node(libxl__gc *gc, void *fdt)
> >      return 0;
> >  }
> >  
> > -static int make_memory_node(libxl__gc *gc, void *fdt,
> > -                            unsigned long long base,
> > -                            unsigned long long size)> 
> > +static int make_one_memory_node(libxl__gc *gc, void *fdt,
> > +                                unsigned long long base,
> > +                                unsigned long long size)
> 
> I would use uint64_t rather unsigned long long. It's more clear the
> base/size are 64 bits.

I will add a patch to change all the unsigned long long's in this file,
of which there are several others.

> > +{
> > +    int res;
> > +    /* This had better match libxc's arch_setup_meminit... */
> > +    const uint64_t size0 = size > GUEST_RAM0_SIZE ? GUEST_RAM0_SIZE : size;
> > +    const uint64_t size1 = size > GUEST_RAM0_SIZE ? size - size0 : 0;
> > +
> > +    res = make_one_memory_node(gc, fdt, GUEST_RAM0_BASE, size0);
> > +    if (res) return res;
> > +    if (size1) {
> > +        res = make_one_memory_node(gc, fdt, GUEST_RAM1_BASE, size1);
> > +        if (res) return res;
> > +    }
> 
> Any reason to not gather the both bank in one memory node?

This is simpler to code and is equally valid DT.

Ian.