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

[PATCH v2 0/8] 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 second 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.

Since last time there are a couple of new patches:
        libxl: use uint64_t not unsigned long long for addresses
        tools: arm: increase size of region set aside for guest grant
        table
        
I have also factored out some renaming and code motion from "support up
to (almost) 1TB of guest RAM" into two new precursor patches:
        tools: arm: refactor code to setup guest p2m and fill it with RAM
        tools: arm: prepare for multiple banks of guest RAM

Other changes are noted in the individual patch changelogs.

Ian.

[PATCH v2 1/8] tools: libxl: use uint64_t not unsigned long long for addresses

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v2: New patch
---
 tools/libxl/libxl_arm.c |   19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index 4f0f0e2..215ef9e 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -256,11 +256,10 @@ static int make_psci_node(libxl__gc *gc, void *fdt)
 }
 
 static int make_memory_node(libxl__gc *gc, void *fdt,
-                            unsigned long long base,
-                            unsigned long long size)
+                            uint64_t base, uint64_t size)
 {
     int res;
-    const char *name = GCSPRINTF("memory@%08llx", base);
+    const char *name = GCSPRINTF("memory@%"PRIx64, base);
 
     res = fdt_begin_node(fdt, name);
     if (res) return res;
@@ -269,7 +268,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);
@@ -279,13 +278,11 @@ static int make_memory_node(libxl__gc *gc, void *fdt,
 }
 
 static int make_intc_node(libxl__gc *gc, void *fdt,
-                          unsigned long long gicd_base,
-                          unsigned long long gicd_size,
-                          unsigned long long gicc_base,
-                          unsigned long long gicc_size)
+                          uint64_t gicd_base, uint64_t gicd_size,
+                          uint64_t gicc_base, uint64_t gicc_size)
 {
     int res;
-    const char *name = GCSPRINTF("interrupt-controller@%08llx", gicd_base);
+    const char *name = GCSPRINTF("interrupt-controller@%"PRIx64, gicd_base);
 
     res = fdt_begin_node(fdt, name);
     if (res) return res;
@@ -307,8 +304,8 @@ static int make_intc_node(libxl__gc *gc, void *fdt,
 
     res = fdt_property_regs(gc, fdt, ROOT_ADDRESS_CELLS, ROOT_SIZE_CELLS,
                             2,
-                            (uint64_t)gicd_base, (uint64_t)gicd_size,
-                            (uint64_t)gicc_base, (uint64_t)gicc_size);
+                            gicd_base, gicd_size,
+                            gicc_base, gicc_size);
     if (res) return res;
 
     res = fdt_property_cell(fdt, "linux,phandle", PHANDLE_GIC);
-- 
1.7.10.4

Re: [PATCH v2 1/8] tools: libxl: use uint64_t not unsigned long long for addresses

[Julien Grall]

Hi Ian,

On 25/04/14 12:22, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>

> ---
> v2: New patch
> ---
>   tools/libxl/libxl_arm.c |   19 ++++++++-----------
>   1 file changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> index 4f0f0e2..215ef9e 100644
> --- a/tools/libxl/libxl_arm.c
> +++ b/tools/libxl/libxl_arm.c
> @@ -256,11 +256,10 @@ static int make_psci_node(libxl__gc *gc, void *fdt)
>   }
>
>   static int make_memory_node(libxl__gc *gc, void *fdt,
> -                            unsigned long long base,
> -                            unsigned long long size)
> +                            uint64_t base, uint64_t size)
>   {
>       int res;
> -    const char *name = GCSPRINTF("memory@%08llx", base);
> +    const char *name = GCSPRINTF("memory@%"PRIx64, base);
>
>       res = fdt_begin_node(fdt, name);
>       if (res) return res;
> @@ -269,7 +268,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);
> @@ -279,13 +278,11 @@ static int make_memory_node(libxl__gc *gc, void *fdt,
>   }
>
>   static int make_intc_node(libxl__gc *gc, void *fdt,
> -                          unsigned long long gicd_base,
> -                          unsigned long long gicd_size,
> -                          unsigned long long gicc_base,
> -                          unsigned long long gicc_size)
> +                          uint64_t gicd_base, uint64_t gicd_size,
> +                          uint64_t gicc_base, uint64_t gicc_size)
>   {
>       int res;
> -    const char *name = GCSPRINTF("interrupt-controller@%08llx", gicd_base);
> +    const char *name = GCSPRINTF("interrupt-controller@%"PRIx64, gicd_base);
>
>       res = fdt_begin_node(fdt, name);
>       if (res) return res;
> @@ -307,8 +304,8 @@ static int make_intc_node(libxl__gc *gc, void *fdt,
>
>       res = fdt_property_regs(gc, fdt, ROOT_ADDRESS_CELLS, ROOT_SIZE_CELLS,
>                               2,
> -                            (uint64_t)gicd_base, (uint64_t)gicd_size,
> -                            (uint64_t)gicc_base, (uint64_t)gicc_size);
> +                            gicd_base, gicd_size,
> +                            gicc_base, gicc_size);
>       if (res) return res;
>
>       res = fdt_property_cell(fdt, "linux,phandle", PHANDLE_GIC);
>

-- 
Julien Grall

Re: [PATCH v2 1/8] tools: libxl: use uint64_t not unsigned long long for addresses

[Ian Jackson]

Ian Campbell writes ("[PATCH v2 1/8] tools: libxl: use uint64_t not unsigned long long for addresses"):
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

If we are going to be doing this, why not introduce a type alias or
two ?

Ian.

Re: [PATCH v2 1/8] tools: libxl: use uint64_t not unsigned long long for addresses

[Ian Campbell]

On Wed, 2014-04-30 at 16:12 +0100, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH v2 1/8] tools: libxl: use uint64_t not unsigned long long for addresses"):
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> 
> If we are going to be doing this, why not introduce a type alias or
> two ?

Good point. I'll do that.

Ian.

Re: [PATCH v2 1/8] tools: libxl: use uint64_t not unsigned long long for addresses

[Ian Campbell]

On Wed, 2014-04-30 at 16:49 +0100, Ian Campbell wrote:
> On Wed, 2014-04-30 at 16:12 +0100, Ian Jackson wrote:
> > Ian Campbell writes ("[PATCH v2 1/8] tools: libxl: use uint64_t not unsigned long long for addresses"):
> > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > If we are going to be doing this, why not introduce a type alias or
> > two ?
> 
> Good point. I'll do that.

Actually, when I came to look at this I realised that because these
numbers are being fed into device tree it is much simpler if they are of
a fixed size rather than semantic type because you need to declare
upfront the number of 32-bit cells each address and size value in the
tree takes and it's much simpler to just declare up from "everything is
2 cells" and then use uint64_t throughout.

Ian.

[PATCH v2 2/8] 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

v2:
 - Use GUEST_RAM_SIZE instead of GUEST_RAM_END
 - Refactor the ramlimit into one place.
---
 tools/libxc/xc_dom_arm.c      |    9 +++++++++
 xen/include/public/arch-arm.h |    3 ++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
index 60ac51a..46dfc36 100644
--- a/tools/libxc/xc_dom_arm.c
+++ b/tools/libxc/xc_dom_arm.c
@@ -272,6 +272,15 @@ int arch_setup_meminit(struct xc_dom_image *dom)
         return -1;
     }
 
+    if ( ramsize > GUEST_RAM_SIZE - NR_MAGIC_PAGES*XC_PAGE_SIZE )
+    {
+        DOMPRINTF("%s: ram size is too large for guest address space: "
+                  "%"PRIx64" > %"PRIx64,
+                  __FUNCTION__, ramsize,
+                  GUEST_RAM_SIZE - NR_MAGIC_PAGES*XC_PAGE_SIZE);
+        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..dd53c94 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 @ 2GB */
+#define GUEST_RAM_SIZE    0x30000000ULL
 
 #define GUEST_GNTTAB_BASE 0xb0000000ULL
 #define GUEST_GNTTAB_SIZE 0x00020000ULL
-- 
1.7.10.4

Re: [PATCH v2 2/8] tools: arm: report an error if the guest RAM is too large

[Julien Grall]

Hi Ian,

On 25/04/14 12:22, 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

I agree with you. Without this the guest can crash with weird error.

For this patch:

Acked-by: Julien Grall <julien.grall@linaro.org>


-- 
Julien Grall

Re: [PATCH v2 2/8] tools: arm: report an error if the guest RAM is too large

[Ian Campbell]

On Fri, 2014-04-25 at 12:50 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 25/04/14 12:22, 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
> 
> I agree with you. Without this the guest can crash with weird error.

I've found that it just hangs silently, which is even more frustrating.

> 
> For this patch:
> 
> Acked-by: Julien Grall <julien.grall@linaro.org>

Thanks.

Re: [PATCH v2 2/8] tools: arm: report an error if the guest RAM is too large

[Julien Grall]

On 25/04/14 12:51, Ian Campbell wrote:
> On Fri, 2014-04-25 at 12:50 +0100, Julien Grall wrote:
>> Hi Ian,
>>
>> On 25/04/14 12:22, 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
>>
>> I agree with you. Without this the guest can crash with weird error.
> 
> I've found that it just hangs silently, which is even more frustrating.

When I try to create a domain with 800M of RAM I get some error from libxl:
libxl: notice: libxl_numa.c:494:libxl__get_numa_candidate: NUMA placement failed, performance might be affected
libxl: error: libxl_device.c:934:device_backend_callback: unable to add device with path /local/domain/0/backend/vbd/5/51712
libxl: error: libxl_create.c:1062:domcreate_launch_dm: unable to add disk devices
libxl: error: libxl_device.c:934:device_backend_callback: unable to remove device with path /local/domain/0/backend/vbd/5/51712
libxl: error: libxl.c:1470:devices_destroy_cb: libxl__devices_destroy failed for 5


-- 
Julien Grall

Re: [PATCH v2 2/8] tools: arm: report an error if the guest RAM is too large

[Ian Campbell]

On Fri, 2014-04-25 at 13:01 +0100, Julien Grall wrote:
> 
> On 25/04/14 12:51, Ian Campbell wrote:
> > On Fri, 2014-04-25 at 12:50 +0100, Julien Grall wrote:
> >> Hi Ian,
> >>
> >> On 25/04/14 12:22, 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
> >>
> >> I agree with you. Without this the guest can crash with weird error.
> > 
> > I've found that it just hangs silently, which is even more frustrating.
> 
> When I try to create a domain with 800M of RAM I get some error from libxl:

With 768M I get a hang.

> libxl: notice: libxl_numa.c:494:libxl__get_numa_candidate: NUMA placement failed, performance might be affected
> libxl: error: libxl_device.c:934:device_backend_callback: unable to add device with path /local/domain/0/backend/vbd/5/51712
> libxl: error: libxl_create.c:1062:domcreate_launch_dm: unable to add disk devices
> libxl: error: libxl_device.c:934:device_backend_callback: unable to remove device with path /local/domain/0/backend/vbd/5/51712
> libxl: error: libxl.c:1470:devices_destroy_cb: libxl__devices_destroy failed for 5
> 
> 

[PATCH v2 3/8] tools: 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>
Acked-by: Julien Grall <julien.grall@linaro.org>
--
v2: remove spurious w/s change
---
 tools/libxc/xc_dom_arm.c      |   12 ++++++------
 xen/include/public/arch-arm.h |    2 ++
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
index 46dfc36..5760bb1 100644
--- a/tools/libxc/xc_dom_arm.c
+++ b/tools/libxc/xc_dom_arm.c
@@ -58,12 +58,13 @@ 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 +72,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);
@@ -272,12 +273,11 @@ int arch_setup_meminit(struct xc_dom_image *dom)
         return -1;
     }
 
-    if ( ramsize > GUEST_RAM_SIZE - NR_MAGIC_PAGES*XC_PAGE_SIZE )
+    if ( ramsize > GUEST_RAM_SIZE )
     {
         DOMPRINTF("%s: ram size is too large for guest address space: "
                   "%"PRIx64" > %"PRIx64,
-                  __FUNCTION__, ramsize,
-                  GUEST_RAM_SIZE - NR_MAGIC_PAGES*XC_PAGE_SIZE);
+                  __FUNCTION__, ramsize, GUEST_RAM_SIZE);
         return -1;
     }
 
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index dd53c94..a94d16b 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 v2 3/8] tools: arm: move magic pfns out of guest RAM region

[Julien Grall]

On 25/04/14 12:22, Ian Campbell wrote:
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index dd53c94..a94d16b 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
> +

I'm wondering if we need to move NR_MAGIC_PAGES or add a comment in 
arch-arm.h if someone wants to bump the number page magic page.

Or perhaps a BUG_ON in libxc to check at compilation time the hole is 
large enought to hold all special pages.

-- 
Julien Grall

Re: [PATCH v2 3/8] tools: arm: move magic pfns out of guest RAM region

[Ian Campbell]

On Fri, 2014-04-25 at 13:09 +0100, Julien Grall wrote:
> 
> On 25/04/14 12:22, Ian Campbell wrote:
> > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> > index dd53c94..a94d16b 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
> > +
> 
> I'm wondering if we need to move NR_MAGIC_PAGES or add a comment in 
> arch-arm.h if someone wants to bump the number page magic page.
> 
> Or perhaps a BUG_ON in libxc to check at compilation time the hole is 
> large enought to hold all special pages.

I'll add GUEST_MAGIC_SIZE and a (BUILD_)BUG_ON to check against it.

Ian.

Re: [PATCH v2 3/8] tools: arm: move magic pfns out of guest RAM region

[Ian Campbell]

On Fri, 2014-04-25 at 13:22 +0100, Ian Campbell wrote:
> On Fri, 2014-04-25 at 13:09 +0100, Julien Grall wrote:
> > 
> > On 25/04/14 12:22, Ian Campbell wrote:
> > > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> > > index dd53c94..a94d16b 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
> > > +
> > 
> > I'm wondering if we need to move NR_MAGIC_PAGES or add a comment in 
> > arch-arm.h if someone wants to bump the number page magic page.
> > 
> > Or perhaps a BUG_ON in libxc to check at compilation time the hole is 
> > large enought to hold all special pages.
> 
> I'll add GUEST_MAGIC_SIZE and a (BUILD_)BUG_ON to check against it.

Doing that in this patch would require a full repost since it has knock
on effects on other patches. So instead I have tacked on the end:

8<---------------

>From f38f5e3e6e5be151e7734ffb114bbee3ce251c81 Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@citrix.com>
Date: Fri, 25 Apr 2014 14:07:45 +0100
Subject: [PATCH v3 9/8] tools: arm: make the size of the magic page region explicit

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

diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
index 7216d2a..c2f6d27 100644
--- a/tools/libxc/xc_dom_arm.c
+++ b/tools/libxc/xc_dom_arm.c
@@ -62,6 +62,8 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
     const xen_pfn_t base = GUEST_MAGIC_BASE >> PAGE_SHIFT;
     xen_pfn_t p2m[NR_MAGIC_PAGES];
 
+    XC_BUILD_BUG_ON(NR_MAGIC_PAGES > GUEST_MAGIC_SIZE >> XC_PAGE_SHIFT);
+
     DOMPRINTF_CALLED(dom->xch);
 
     for (i = 0; i < NR_MAGIC_PAGES; i++)
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 061d59d..31ba44d 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -376,6 +376,7 @@ typedef uint64_t xen_callback_t;
 #define GUEST_GNTTAB_SIZE 0x01000000ULL
 
 #define GUEST_MAGIC_BASE  0x39000000ULL
+#define GUEST_MAGIC_SIZE  0x01000000ULL
 
 #define GUEST_RAM0_BASE   0x40000000ULL /* 3GB of low RAM @ 1GB */
 #define GUEST_RAM0_SIZE   0xc0000000ULL
-- 
1.7.10.4

[PATCH v2 4/8] tools: 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 a94d16b..4149d6f 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 @ 2GB */
-#define GUEST_RAM_SIZE    0x30000000ULL
-
-#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_SIZE    0xc0000000ULL
 
 /* Interrupts */
 #define GUEST_TIMER_VIRT_PPI    27
-- 
1.7.10.4

Re: [PATCH v2 4/8] tools: arm: rearrange guest physical address space to increase max RAM

[Julien Grall]

Hi Ian,

On 25/04/14 12:22, 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.

I guess I will have to rework the layout when device assignment will be 
added.

In any case:

Acked-by: Julien Grall <julien.grall@linaro.org>


> 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 a94d16b..4149d6f 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 @ 2GB */
> -#define GUEST_RAM_SIZE    0x30000000ULL
> -
> -#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_SIZE    0xc0000000ULL
>
>   /* Interrupts */
>   #define GUEST_TIMER_VIRT_PPI    27
>

-- 
Julien Grall

[PATCH v2 5/8] tools: arm: prepare for multiple banks of guest RAM

[Ian Campbell]

Prepare for adding more banks of guest RAM by renaming a bunch of variables
and defines as RAM0 etc.

Also in preparation switch to using GUEST_RAM0_BASE explicitly instead of
implicitly via dom->rambase_pfn (while asserting that they must be the same).
This makes the multiple bank case cleaner (although it looks a bit odd for
now).

Lastly for now ramsize (total size) and ram0size (size of first bank) are the
same, but use the appropriate one for each context.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v2: New patch
---
 tools/libxc/xc_dom_arm.c      |   25 +++++++++++++++----------
 xen/include/public/arch-arm.h |    8 ++++++--
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
index 5760bb1..8775ca4 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>
@@ -253,9 +254,11 @@ int arch_setup_meminit(struct xc_dom_image *dom)
     uint64_t modbase;
 
     /* Convenient */
-    const uint64_t rambase = dom->rambase_pfn << XC_PAGE_SHIFT;
     const uint64_t ramsize = dom->total_pages << XC_PAGE_SHIFT;
-    const uint64_t ramend = rambase + ramsize;
+
+    const uint64_t ram0size = ramsize;
+    const uint64_t ram0end = GUEST_RAM0_BASE + ram0size;
+
     const uint64_t kernbase = dom->kernel_seg.vstart;
     const uint64_t kernend = ROUNDUP(dom->kernel_seg.vend, 21/*2MB*/);
     const uint64_t kernsize = kernend - kernbase;
@@ -264,20 +267,22 @@ int arch_setup_meminit(struct xc_dom_image *dom)
     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);
+
+    assert(dom->rambase_pfn << XC_PAGE_SHIFT == GUEST_RAM0_BASE);
 
-    if ( modsize + kernsize > ramsize )
+    if ( modsize + kernsize > ram0size )
     {
         DOMPRINTF("%s: Not enough memory for the kernel+dtb+initrd",
                   __FUNCTION__);
         return -1;
     }
 
-    if ( ramsize > GUEST_RAM_SIZE )
+    if ( ramsize > GUEST_RAM_MAX )
     {
         DOMPRINTF("%s: ram size is too large for guest address space: "
                   "%"PRIx64" > %"PRIx64,
-                  __FUNCTION__, ramsize, GUEST_RAM_SIZE);
+                  __FUNCTION__, ramsize, GUEST_RAM_MAX);
         return -1;
     }
 
@@ -317,11 +322,11 @@ int arch_setup_meminit(struct xc_dom_image *dom)
      * If changing this then consider
      * xen/arch/arm/kernel.c:place_modules as well.
      */
-    if ( ramend >= ram128mb + modsize && kernend < ram128mb )
+    if ( ram0end >= ram128mb + modsize && kernend < ram128mb )
         modbase = ram128mb;
-    else if ( ramend - modsize > kernend )
-        modbase = ramend - modsize;
-    else if (kernbase - rambase > modsize )
+    else if ( ram0end - modsize > kernend )
+        modbase = ram0end - modsize;
+    else if (kernbase - GUEST_RAM0_BASE > modsize )
         modbase = kernbase - modsize;
     else
         return -1;
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 4149d6f..c4f4990 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -374,8 +374,12 @@ typedef uint64_t xen_callback_t;
 
 #define GUEST_MAGIC_BASE  0x39000000ULL
 
-#define GUEST_RAM_BASE    0x40000000ULL /* 3GB of RAM @ 1GB */
-#define GUEST_RAM_SIZE    0xc0000000ULL
+#define GUEST_RAM0_BASE   0x40000000ULL /* 3GB of RAM @ 1GB */
+#define GUEST_RAM0_SIZE   0xc0000000ULL
+
+#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)
 
 /* Interrupts */
 #define GUEST_TIMER_VIRT_PPI    27
-- 
1.7.10.4

Re: [PATCH v2 5/8] tools: arm: prepare for multiple banks of guest RAM

[Julien Grall]

Hi Ian,

On 25/04/14 12:22, Ian Campbell wrote:
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index 4149d6f..c4f4990 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -374,8 +374,12 @@ typedef uint64_t xen_callback_t;
>
>   #define GUEST_MAGIC_BASE  0x39000000ULL
>
> -#define GUEST_RAM_BASE    0x40000000ULL /* 3GB of RAM @ 1GB */
> -#define GUEST_RAM_SIZE    0xc0000000ULL
> +#define GUEST_RAM0_BASE   0x40000000ULL /* 3GB of RAM @ 1GB */
> +#define GUEST_RAM0_SIZE   0xc0000000ULL
> +
> +#define GUEST_RAM_BASE    GUEST_RAM0_BASE /* Lowest RAM address */

Is it necessary to define GUEST_RAM_BASE? I don't see any usage of this 
define in this patch series.

> +/* Largest amount of actual RAM, not including holes */
> +#define GUEST_RAM_MAX     (GUEST_RAM0_SIZE)
>
>   /* Interrupts */
>   #define GUEST_TIMER_VIRT_PPI    27

Regards,

-- 
Julien Grall

Re: [PATCH v2 5/8] tools: arm: prepare for multiple banks of guest RAM

[Ian Campbell]

On Fri, 2014-04-25 at 13:19 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 25/04/14 12:22, Ian Campbell wrote:
> > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> > index 4149d6f..c4f4990 100644
> > --- a/xen/include/public/arch-arm.h
> > +++ b/xen/include/public/arch-arm.h
> > @@ -374,8 +374,12 @@ typedef uint64_t xen_callback_t;
> >
> >   #define GUEST_MAGIC_BASE  0x39000000ULL
> >
> > -#define GUEST_RAM_BASE    0x40000000ULL /* 3GB of RAM @ 1GB */
> > -#define GUEST_RAM_SIZE    0xc0000000ULL
> > +#define GUEST_RAM0_BASE   0x40000000ULL /* 3GB of RAM @ 1GB */
> > +#define GUEST_RAM0_SIZE   0xc0000000ULL
> > +
> > +#define GUEST_RAM_BASE    GUEST_RAM0_BASE /* Lowest RAM address */
> 
> Is it necessary to define GUEST_RAM_BASE? I don't see any usage of this 
> define in this patch series.

$ git grep GUEST_RAM_BASE
tools/libxl/libxl_dom.c:#ifdef GUEST_RAM_BASE
tools/libxl/libxl_dom.c:    if ( (ret = xc_dom_rambase_init(dom, GUEST_RAM_BASE

(this then ties back to the assert in arch_setup_meminit)

Ian.

Re: [PATCH v2 5/8] tools: arm: prepare for multiple banks of guest RAM

[Julien Grall]

On 25/04/14 13:23, Ian Campbell wrote:
> On Fri, 2014-04-25 at 13:19 +0100, Julien Grall wrote:
>> Hi Ian,
>>
>> On 25/04/14 12:22, Ian Campbell wrote:
>>> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
>>> index 4149d6f..c4f4990 100644
>>> --- a/xen/include/public/arch-arm.h
>>> +++ b/xen/include/public/arch-arm.h
>>> @@ -374,8 +374,12 @@ typedef uint64_t xen_callback_t;
>>>
>>>    #define GUEST_MAGIC_BASE  0x39000000ULL
>>>
>>> -#define GUEST_RAM_BASE    0x40000000ULL /* 3GB of RAM @ 1GB */
>>> -#define GUEST_RAM_SIZE    0xc0000000ULL
>>> +#define GUEST_RAM0_BASE   0x40000000ULL /* 3GB of RAM @ 1GB */
>>> +#define GUEST_RAM0_SIZE   0xc0000000ULL
>>> +
>>> +#define GUEST_RAM_BASE    GUEST_RAM0_BASE /* Lowest RAM address */
>>
>> Is it necessary to define GUEST_RAM_BASE? I don't see any usage of this
>> define in this patch series.
>
> $ git grep GUEST_RAM_BASE
> tools/libxl/libxl_dom.c:#ifdef GUEST_RAM_BASE
> tools/libxl/libxl_dom.c:    if ( (ret = xc_dom_rambase_init(dom, GUEST_RAM_BASE

I forgot this one. Sorry.

Acked-by: Julien Grall <julien.grall@linaro.org>


-- 
Julien Grall

[PATCH v2 6/8] tools: arm: refactor code to setup guest p2m and fill it with RAM

[Ian Campbell]

This will help when we have more guest RAM banks.

Mostly code motion of the p2m_host initialisation and allocation loop into the
new function populate_guest_memory, but in addition in the caller we now
initialise the p2m all the INVALID_MFN to handle any holes, although in this
patch we still fill in the entire allocated region.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v2: New patch
---
 tools/libxc/xc_dom_arm.c |   62 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 43 insertions(+), 19 deletions(-)

diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
index 8775ca4..61f9ba6 100644
--- a/tools/libxc/xc_dom_arm.c
+++ b/tools/libxc/xc_dom_arm.c
@@ -247,10 +247,42 @@ 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 */
@@ -259,6 +291,8 @@ int arch_setup_meminit(struct xc_dom_image *dom)
     const uint64_t ram0size = ramsize;
     const uint64_t ram0end = GUEST_RAM0_BASE + ram0size;
 
+    const xen_pfn_t p2m_size = (ram0end - GUEST_RAM0_BASE) >> XC_PAGE_SHIFT;
+
     const uint64_t kernbase = dom->kernel_seg.vstart;
     const uint64_t kernend = ROUNDUP(dom->kernel_seg.vend, 21/*2MB*/);
     const uint64_t kernsize = kernend - kernbase;
@@ -292,27 +326,17 @@ 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;
 
     /*
      * We try to place dtb+initrd at 128MB or if we have less RAM
-- 
1.7.10.4

Re: [PATCH v2 6/8] tools: arm: refactor code to setup guest p2m and fill it with RAM

[Julien Grall]

Hi Ian,

On 25/04/14 12:22, Ian Campbell wrote:
> +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 )

May I ask for a bit of clean up here?
	- allocsz doesn't need to be initialized. It will be unconditionally 
set at the beginning of the loop
	- rc can be set 0 at the beginning of the function.

> +    {
> +        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 */
> @@ -259,6 +291,8 @@ int arch_setup_meminit(struct xc_dom_image *dom)
>       const uint64_t ram0size = ramsize;
>       const uint64_t ram0end = GUEST_RAM0_BASE + ram0size;
>
> +    const xen_pfn_t p2m_size = (ram0end - GUEST_RAM0_BASE) >> XC_PAGE_SHIFT;
> +
>       const uint64_t kernbase = dom->kernel_seg.vstart;
>       const uint64_t kernend = ROUNDUP(dom->kernel_seg.vend, 21/*2MB*/);
>       const uint64_t kernsize = kernend - kernbase;
> @@ -292,27 +326,17 @@ 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;

With this solution, you will loop 262244 times for nothing (the hole 
between the 2 banks).

Also when the guess will have lots of RAM, it will be slow because we 
loop nearly twice the array (one here, the other in populate_guest_memory).

It think we can avoid looping twice by making the two banks contiguous 
in the memory (i.e starting the second bank at 4GB instead of 8GB).

Regards,

-- 
Julien Grall

Re: [PATCH v2 6/8] tools: arm: refactor code to setup guest p2m and fill it with RAM

[Ian Campbell]

On Fri, 2014-04-25 at 13:51 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 25/04/14 12:22, Ian Campbell wrote:
> > +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 )
> 
> May I ask for a bit of clean up here?

This is code motion. I deliberately don't want to change it for that
reason.

> > @@ -292,27 +326,17 @@ 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;
> 
> With this solution, you will loop 262244 times for nothing (the hole 
> between the 2 banks).

Yes, this is the simplest way to ensure that p2m_host is definitely
completely initialised, irrespective of the presence of any holes in the
address space.

> Also when the guess will have lots of RAM, it will be slow because we 
> loop nearly twice the array (one here, the other in populate_guest_memory).

This is dwarfed by all the other overheads of course, like actually
filling in the RAM on the second pass.

> It think we can avoid looping twice by making the two banks contiguous 
> in the memory (i.e starting the second bank at 4GB instead of 8GB).

As explained in the commit message I have deliberately left a hole so
that we can see that such configurations actually work.

Ian.

Re: [PATCH v2 6/8] tools: arm: refactor code to setup guest p2m and fill it with RAM

[Julien Grall]

On 25/04/14 13:59, Ian Campbell wrote:
  >> It think we can avoid looping twice by making the two banks contiguous
>> in the memory (i.e starting the second bank at 4GB instead of 8GB).
>
> As explained in the commit message I have deliberately left a hole so
> that we can see that such configurations actually work.

IHMO, the code path doesn't seem very complicate. Adding this overhead 
(the two loop + the 1GB hole) just for it seems pointless.

BTW, is there any issue to create one big bank rather than 2?

-- 
Julien Grall

Re: [PATCH v2 6/8] tools: arm: refactor code to setup guest p2m and fill it with RAM

[Ian Campbell]

On Fri, 2014-04-25 at 14:12 +0100, Julien Grall wrote:
> 
> On 25/04/14 13:59, Ian Campbell wrote:
>   >> It think we can avoid looping twice by making the two banks contiguous
> >> in the memory (i.e starting the second bank at 4GB instead of 8GB).
> >
> > As explained in the commit message I have deliberately left a hole so
> > that we can see that such configurations actually work.
> 
> IHMO, the code path doesn't seem very complicate. Adding this overhead 
> (the two loop + the 1GB hole) just for it seems pointless.

I disagree. It is always worth exercising these things, and this will
make sure we don't bake in assumptions about using a single contiguous
bank anywhere by mistake.

I think you are overestimating the overhead and underestimating the
benefit.0

> BTW, is there any issue to create one big bank rather than 2?

I expect it would work just fine, but I am not going to do that either
for the reasons already discussed.

Ian.

Re: [PATCH v2 6/8] tools: arm: refactor code to setup guest p2m and fill it with RAM

[Julien Grall]

On 25/04/14 14:22, Ian Campbell wrote:
> On Fri, 2014-04-25 at 14:12 +0100, Julien Grall wrote:
>>
>> On 25/04/14 13:59, Ian Campbell wrote:
>>    >> It think we can avoid looping twice by making the two banks contiguous
>>>> in the memory (i.e starting the second bank at 4GB instead of 8GB).
>>>
>>> As explained in the commit message I have deliberately left a hole so
>>> that we can see that such configurations actually work.
>>
>> IHMO, the code path doesn't seem very complicate. Adding this overhead
>> (the two loop + the 1GB hole) just for it seems pointless.
>
> I disagree. It is always worth exercising these things, and this will
> make sure we don't bake in assumptions about using a single contiguous
> bank anywhere by mistake.
>
> I think you are overestimating the overhead and underestimating the
> benefit.0
>
>> BTW, is there any issue to create one big bank rather than 2?
>
> I expect it would work just fine, but I am not going to do that either
> for the reasons already discussed.

I'm not entirely convince of this hole... but it might help to also 
avoid assumption during guest migration:

Acked-by: Julien Grall <julien.grall@linaro.org>

-- 
Julien Grall

[PATCH v2 7/8] tools: 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      |   16 +++++++++++++---
 tools/libxl/libxl_arm.c       |   22 +++++++++++++++++++---
 xen/include/public/arch-arm.h |    9 ++++++---
 3 files changed, 38 insertions(+), 9 deletions(-)

diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
index 61f9ba6..7216d2a 100644
--- a/tools/libxc/xc_dom_arm.c
+++ b/tools/libxc/xc_dom_arm.c
@@ -286,12 +286,18 @@ int arch_setup_meminit(struct xc_dom_image *dom)
     uint64_t modbase;
 
     /* Convenient */
-    const uint64_t ramsize = dom->total_pages << XC_PAGE_SHIFT;
+    const uint64_t ramsize = (uint64_t)dom->total_pages << XC_PAGE_SHIFT;
 
-    const uint64_t ram0size = ramsize;
+    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 = (ram0end - GUEST_RAM0_BASE) >> XC_PAGE_SHIFT;
+    const xen_pfn_t p2m_size = ram1size ?
+        (ram1end - GUEST_RAM0_BASE) >> XC_PAGE_SHIFT :
+        (ram0end - GUEST_RAM0_BASE) >> XC_PAGE_SHIFT;
 
     const uint64_t kernbase = dom->kernel_seg.vstart;
     const uint64_t kernend = ROUNDUP(dom->kernel_seg.vend, 21/*2MB*/);
@@ -337,6 +343,10 @@ int arch_setup_meminit(struct xc_dom_image *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;
 
     /*
      * We try to place dtb+initrd at 128MB or if we have less RAM
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index 215ef9e..1af6b4a 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -255,8 +255,8 @@ static int make_psci_node(libxl__gc *gc, void *fdt)
     return 0;
 }
 
-static int make_memory_node(libxl__gc *gc, void *fdt,
-                            uint64_t base, uint64_t size)
+static int make_one_memory_node(libxl__gc *gc, void *fdt,
+                                uint64_t base, uint64_t size)
 {
     int res;
     const char *name = GCSPRINTF("memory@%"PRIx64, base);
@@ -277,6 +277,23 @@ static int make_memory_node(libxl__gc *gc, void *fdt,
     return 0;
 }
 
+static int make_memory_node(libxl__gc *gc, void *fdt, uint64_t 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,
                           uint64_t gicd_base, uint64_t gicd_size,
                           uint64_t gicc_base, uint64_t gicc_size)
@@ -490,7 +507,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 c4f4990..3f457f1 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -374,12 +374,15 @@ typedef uint64_t xen_callback_t;
 
 #define GUEST_MAGIC_BASE  0x39000000ULL
 
-#define GUEST_RAM0_BASE   0x40000000ULL /* 3GB of RAM @ 1GB */
+#define GUEST_RAM0_BASE   0x40000000ULL /* 3GB of low RAM @ 1GB */
 #define GUEST_RAM0_SIZE   0xc0000000ULL
 
+#define GUEST_RAM1_BASE   0x0200000000ULL /* 1016GB of RAM @ 8GB */
+#define GUEST_RAM1_SIZE   0xfe00000000ULL
+
 #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)
+ /* 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 v2 7/8] tools: arm: support up to (almost) 1TB of guest RAM

[Julien Grall]

On 25/04/14 12:22, Ian Campbell wrote:
>   #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)
> + /* Largest amount of actual RAM, not including holes */

I think you've added a space by mistake here :).

With this minor change:

Acked-by: Julien Grall <julien.grall@linaro.org>

-- 
Julien Grall

[PATCH v2 8/8] tools: arm: increase size of region set aside for guest grant table

[Ian Campbell]

The current size is sufficient for the default maximum grant table size
(32-frames), but increase the reserved region to 16M/4096 pages to allow for
the use of the gnttab_max_nr_frames command line option.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v2: New patch
---
 xen/include/public/arch-arm.h |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 3f457f1..061d59d 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -369,8 +369,11 @@ typedef uint64_t xen_callback_t;
 #define GUEST_GICC_BASE   0x03002000ULL
 #define GUEST_GICC_SIZE   0x00000100ULL
 
+/* 16MB == 4096 pages reserved for guest to use as a region to map its
+ * grant table in.
+ */
 #define GUEST_GNTTAB_BASE 0x38000000ULL
-#define GUEST_GNTTAB_SIZE 0x00020000ULL
+#define GUEST_GNTTAB_SIZE 0x01000000ULL
 
 #define GUEST_MAGIC_BASE  0x39000000ULL
 
-- 
1.7.10.4

Re: [PATCH v2 8/8] tools: arm: increase size of region set aside for guest grant table

[Julien Grall]

On 25/04/14 12:22, Ian Campbell wrote:
> The current size is sufficient for the default maximum grant table size
> (32-frames), but increase the reserved region to 16M/4096 pages to allow for
> the use of the gnttab_max_nr_frames command line option.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Great! thanks for writing this patch.

Acked-by: Julien Grall <julien.grall@linaro.org>

-- 
Julien Grall