[PATCH 0/3] Really revert xen/arm: Allocate memory for dom0 from the bottom with the 1:1 Workaround

[PATCH 0/3] Really revert xen/arm: Allocate memory for dom0 from the bottom with the 1:1 Workaround

[Ian Campbell]

This required a bit of yak shaving in the first two patches. Tested on
arm32 (w/ >4GB) and arm64.

Ian.

[PATCH 1/3] xen: arm: refactor struct kernel_info

[Ian Campbell]

The kernel_order and kernel_image fields are ELF specific.

Also add a few comments about what things are.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/kernel.c |   14 +++++++-------
 xen/arch/arm/kernel.h |   14 ++++++++------
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index ae86772..d1fd307 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -295,7 +295,7 @@ static void kernel_elf_load(struct kernel_info *info)
     elf_load_binary(&info->elf.elf);
 
     printk("Free temporary kernel buffer\n");
-    free_xenheap_pages(info->kernel_img, info->kernel_order);
+    free_xenheap_pages(info->elf.kernel_img, info->elf.kernel_order);
 }
 
 static int kernel_try_elf_prepare(struct kernel_info *info,
@@ -305,14 +305,14 @@ static int kernel_try_elf_prepare(struct kernel_info *info,
 
     memset(&info->elf.elf, 0, sizeof(info->elf.elf));
 
-    info->kernel_order = get_order_from_bytes(size);
-    info->kernel_img = alloc_xenheap_pages(info->kernel_order, 0);
-    if ( info->kernel_img == NULL )
+    info->elf.kernel_order = get_order_from_bytes(size);
+    info->elf.kernel_img = alloc_xenheap_pages(info->elf.kernel_order, 0);
+    if ( info->elf.kernel_img == NULL )
         panic("Cannot allocate temporary buffer for kernel");
 
-    copy_from_paddr(info->kernel_img, addr, size);
+    copy_from_paddr(info->elf.kernel_img, addr, size);
 
-    if ( (rc = elf_init(&info->elf.elf, info->kernel_img, size )) != 0 )
+    if ( (rc = elf_init(&info->elf.elf, info->elf.kernel_img, size )) != 0 )
         goto err;
 #ifdef VERBOSE
     elf_set_verbose(&info->elf.elf);
@@ -351,7 +351,7 @@ err:
         printk("Xen: ELF kernel broken: %s\n",
                elf_check_broken(&info->elf.elf));
 
-    free_xenheap_pages(info->kernel_img, info->kernel_order);
+    free_xenheap_pages(info->elf.kernel_img, info->elf.kernel_order);
     return rc;
 }
 
diff --git a/xen/arch/arm/kernel.h b/xen/arch/arm/kernel.h
index ad2956b..2c27c64 100644
--- a/xen/arch/arm/kernel.h
+++ b/xen/arch/arm/kernel.h
@@ -18,14 +18,16 @@ struct kernel_info {
     paddr_t unassigned_mem; /* RAM not (yet) assigned to a bank */
     struct dt_mem_info mem;
 
-    paddr_t dtb_paddr;
+    /* kernel entry point */
     paddr_t entry;
 
+    /* boot blob load addresses */
+    paddr_t dtb_paddr;
     paddr_t initrd_paddr;
 
-    void *kernel_img;
-    unsigned kernel_order;
-
+    /* loader to use for this kernel */
+    void (*load)(struct kernel_info *info);
+    /* loader specific state */
     union {
         struct {
             paddr_t kernel_addr;
@@ -36,10 +38,10 @@ struct kernel_info {
         struct {
             struct elf_binary elf;
             struct elf_dom_parms parms;
+            unsigned kernel_order;
+            void *kernel_img;
         } elf;
     };
-
-    void (*load)(struct kernel_info *info);
 };
 
 int kernel_prepare(struct kernel_info *info);
-- 
1.7.10.4

Re: [PATCH 1/3] xen: arm: refactor struct kernel_info

[Julien Grall]

Hi Ian,

On 04/04/2014 01:56 PM, Ian Campbell wrote:
> The kernel_order and kernel_image fields are ELF specific.
> 
> Also add a few comments about what things are.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

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

Regards,

-- 
Julien Grall

[PATCH 2/3] xen: arm: probe the kernel to determine the guest type earlier

[Ian Campbell]

We need to know if the kernel is 32- or 64- bit capable sooner so that we know
what sort of domain we are building when allocating memory to it (so we can
place appropriate limits when allocating RAM to the guest). At the moment
kernel_prepare() decides this but it needs the memory to have already been
allocated.

Therefore split the probing functionality of kernel_prepare() and call it much
earlier. The remainder of kernel_prepare() deals with where to place the
kernel in RAM which can be deferred until kernel_load() so do so.

Document the input and output of kernel_probe() and _load().

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/domain_build.c |   10 ++---
 xen/arch/arm/kernel.c       |   95 ++++++++++++++++++++++++-------------------
 xen/arch/arm/kernel.h       |   26 +++++++++++-
 3 files changed, 83 insertions(+), 48 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 502db84..4d6b26b 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1002,9 +1002,7 @@ int construct_dom0(struct domain *d)
 
     kinfo.unassigned_mem = dom0_mem;
 
-    allocate_memory(d, &kinfo);
-
-    rc = kernel_prepare(&kinfo);
+    rc = kernel_probe(&kinfo);
     if ( rc < 0 )
         return rc;
 
@@ -1012,6 +1010,8 @@ int construct_dom0(struct domain *d)
     d->arch.type = kinfo.type;
 #endif
 
+    allocate_memory(d, &kinfo);
+
     rc = prepare_dtb(d, &kinfo);
     if ( rc < 0 )
         return rc;
@@ -1024,8 +1024,8 @@ int construct_dom0(struct domain *d)
     p2m_restore_state(v);
 
     /*
-     * kernel_load will determine the placement of the initrd & fdt in
-     * RAM, so call it first.
+     * kernel_load will determine the placement of the kernel as well
+     * as the initrd & fdt in RAM, so call it first.
      */
     kernel_load(&kinfo);
     /* initrd_load will fix up the fdt, so call it before dtb_load */
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index d1fd307..a7a3e0c 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -116,13 +116,47 @@ static void place_modules(struct kernel_info *info,
     info->initrd_paddr = info->dtb_paddr + dtb_len;
 }
 
+static paddr_t kernel_zimage_place(struct kernel_info *info)
+{
+    paddr_t load_addr;
+
+#ifdef CONFIG_ARM_64
+    if ( info->type == DOMAIN_64BIT )
+        return info->mem.bank[0].start + info->zimage.text_offset;
+#endif
+
+    /*
+     * If start is zero, the zImage is position independent, in this
+     * case Documentation/arm/Booting recommends loading below 128MiB
+     * and above 32MiB. Load it as high as possible within these
+     * constraints, while also avoiding the DTB.
+     */
+    if (info->zimage.start == 0)
+    {
+        paddr_t load_end;
+
+        load_end = info->mem.bank[0].start + info->mem.bank[0].size;
+        load_end = MIN(info->mem.bank[0].start + MB(128), load_end);
+
+        load_addr = load_end - info->zimage.len;
+        /* Align to 2MB */
+        load_addr &= ~((2 << 20) - 1);
+    }
+    else
+        load_addr = info->zimage.start;
+
+    return load_addr;
+}
+
 static void kernel_zimage_load(struct kernel_info *info)
 {
-    paddr_t load_addr = info->zimage.load_addr;
+    paddr_t load_addr = kernel_zimage_place(info);
     paddr_t paddr = info->zimage.kernel_addr;
     paddr_t len = info->zimage.len;
     unsigned long offs;
 
+    info->entry = load_addr;
+
     place_modules(info, load_addr, load_addr + len);
 
     printk("Loading zImage from %"PRIpaddr" to %"PRIpaddr"-%"PRIpaddr"\n",
@@ -154,10 +188,10 @@ static void kernel_zimage_load(struct kernel_info *info)
 
 #ifdef CONFIG_ARM_64
 /*
- * Check if the image is a 64-bit zImage and setup kernel_info
+ * Check if the image is a 64-bit Image.
  */
-static int kernel_try_zimage64_prepare(struct kernel_info *info,
-                                     paddr_t addr, paddr_t size)
+static int kernel_zimage64_probe(struct kernel_info *info,
+                                 paddr_t addr, paddr_t size)
 {
     /* linux/Documentation/arm64/booting.txt */
     struct {
@@ -196,12 +230,9 @@ static int kernel_try_zimage64_prepare(struct kernel_info *info,
         return -EINVAL;
 
     info->zimage.kernel_addr = addr;
-
-    info->zimage.load_addr = info->mem.bank[0].start
-        + zimage.text_offset;
     info->zimage.len = end - start;
+    info->zimage.text_offset = zimage.text_offset;
 
-    info->entry = info->zimage.load_addr;
     info->load = kernel_zimage_load;
 
     info->type = DOMAIN_64BIT;
@@ -213,8 +244,8 @@ static int kernel_try_zimage64_prepare(struct kernel_info *info,
 /*
  * Check if the image is a 32-bit zImage and setup kernel_info
  */
-static int kernel_try_zimage32_prepare(struct kernel_info *info,
-                                     paddr_t addr, paddr_t size)
+static int kernel_zimage32_probe(struct kernel_info *info,
+                                 paddr_t addr, paddr_t size)
 {
     uint32_t zimage[ZIMAGE32_HEADER_LEN/4];
     uint32_t start, end;
@@ -250,28 +281,9 @@ static int kernel_try_zimage32_prepare(struct kernel_info *info,
 
     info->zimage.kernel_addr = addr;
 
-    /*
-     * If start is zero, the zImage is position independent, in this
-     * case Documentation/arm/Booting recommends loading below 128MiB
-     * and above 32MiB. Load it as high as possible within these
-     * constraints, while also avoiding the DTB.
-     */
-    if (start == 0)
-    {
-        paddr_t load_end;
-
-        load_end = info->mem.bank[0].start + info->mem.bank[0].size;
-        load_end = MIN(info->mem.bank[0].start + MB(128), load_end);
-
-        info->zimage.load_addr = load_end - end;
-        /* Align to 2MB */
-        info->zimage.load_addr &= ~((2 << 20) - 1);
-    }
-    else
-        info->zimage.load_addr = start;
+    info->zimage.start = start;
     info->zimage.len = end - start;
 
-    info->entry = info->zimage.load_addr;
     info->load = kernel_zimage_load;
 
 #ifdef CONFIG_ARM_64
@@ -283,6 +295,12 @@ static int kernel_try_zimage32_prepare(struct kernel_info *info,
 
 static void kernel_elf_load(struct kernel_info *info)
 {
+    /*
+     * TODO: can the ELF header be used to find the physical address
+     * to load the image to?  Instead of assuming virt == phys.
+     */
+    info->entry = info->elf.parms.virt_entry;
+
     place_modules(info,
                   info->elf.parms.virt_kstart,
                   info->elf.parms.virt_kend);
@@ -298,8 +316,8 @@ static void kernel_elf_load(struct kernel_info *info)
     free_xenheap_pages(info->elf.kernel_img, info->elf.kernel_order);
 }
 
-static int kernel_try_elf_prepare(struct kernel_info *info,
-                                  paddr_t addr, paddr_t size)
+static int kernel_elf_probe(struct kernel_info *info,
+                            paddr_t addr, paddr_t size)
 {
     int rc;
 
@@ -334,11 +352,6 @@ static int kernel_try_elf_prepare(struct kernel_info *info,
     }
 #endif
 
-    /*
-     * TODO: can the ELF header be used to find the physical address
-     * to load the image to?  Instead of assuming virt == phys.
-     */
-    info->entry = info->elf.parms.virt_entry;
     info->load = kernel_elf_load;
 
     if ( elf_check_broken(&info->elf.elf) )
@@ -355,7 +368,7 @@ err:
     return rc;
 }
 
-int kernel_prepare(struct kernel_info *info)
+int kernel_probe(struct kernel_info *info)
 {
     int rc;
 
@@ -373,12 +386,12 @@ int kernel_prepare(struct kernel_info *info)
     printk("Loading kernel from boot module %d\n", MOD_KERNEL);
 
 #ifdef CONFIG_ARM_64
-    rc = kernel_try_zimage64_prepare(info, start, size);
+    rc = kernel_zimage64_probe(info, start, size);
     if (rc < 0)
 #endif
-        rc = kernel_try_zimage32_prepare(info, start, size);
+        rc = kernel_zimage32_probe(info, start, size);
     if (rc < 0)
-        rc = kernel_try_elf_prepare(info, start, size);
+        rc = kernel_elf_probe(info, start, size);
 
     return rc;
 }
diff --git a/xen/arch/arm/kernel.h b/xen/arch/arm/kernel.h
index 2c27c64..2f3d2d4 100644
--- a/xen/arch/arm/kernel.h
+++ b/xen/arch/arm/kernel.h
@@ -31,8 +31,11 @@ struct kernel_info {
     union {
         struct {
             paddr_t kernel_addr;
-            paddr_t load_addr;
             paddr_t len;
+#ifdef CONFIG_ARM_64
+            paddr_t text_offset; /* 64-bit Image only */
+#endif
+            paddr_t start; /* 32-bit zImage only */
         } zimage;
 
         struct {
@@ -44,7 +47,26 @@ struct kernel_info {
     };
 };
 
-int kernel_prepare(struct kernel_info *info);
+/*
+ * Probe the kernel to detemine its type and select a loader.
+ *
+ * Sets in info:
+ *  ->type
+ *  ->load hook, and sets loader specific variables ->{zimage,elf}
+ */
+int kernel_probe(struct kernel_info *info);
+/*
+ * Loads the kernel into guest RAM.
+ *
+ * Expects to be set in info when called:
+ *  ->mem
+ *  ->fdt
+ *
+ * Sets in info:
+ *  ->entry
+ *  ->dtb_paddr
+ *  ->initrd_paddr
+ */
 void kernel_load(struct kernel_info *info);
 
 #endif /* #ifdef __ARCH_ARM_KERNEL_H__ */
-- 
1.7.10.4

Re: [PATCH 2/3] xen: arm: probe the kernel to determine the guest type earlier

[Julien Grall]

Hi Ian,

On 04/04/2014 01:56 PM, Ian Campbell wrote:
> +static paddr_t kernel_zimage_place(struct kernel_info *info)
> +{
> +    paddr_t load_addr;
> +
> +#ifdef CONFIG_ARM_64
> +    if ( info->type == DOMAIN_64BIT )
> +        return info->mem.bank[0].start + info->zimage.text_offset;
> +#endif
> +
> +    /*
> +     * If start is zero, the zImage is position independent, in this
> +     * case Documentation/arm/Booting recommends loading below 128MiB
> +     * and above 32MiB. Load it as high as possible within these
> +     * constraints, while also avoiding the DTB.
> +     */
> +    if (info->zimage.start == 0)

if  ( ... )

[..]

> -int kernel_prepare(struct kernel_info *info);
> +/*
> + * Probe the kernel to detemine its type and select a loader.
> + *
> + * Sets in info:
> + *  ->type
> + *  ->load hook, and sets loader specific variables ->{zimage,elf}
> + */
> +int kernel_probe(struct kernel_info *info);

I would add a newline here.

With theses 2 minor changes, the code looks good to me:

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

Regards,

-- 
Julien Grall

[PATCH 3/3] Revert "xen/arm: Allocate memory for dom0 from the bottom with the 1:1 Workaround"

[Ian Campbell]

This reverts commit 6c21cb36e263de2db8716b477157a5b6cd531e1e.

The Linux = issue which this works around was fixed in v3.13 via f52bb722547f
"ARM: mm: Correct virt_to_phys patching for 64 bit physical addresses".

This is the second attempt to revert this. Now that we have fixed
allocate_memory_11 to allocate accessible memory on 32-bit this is safe to do.
This is not quite a straight revert since we need to ensure that for 32-bit
domain 0 we do not allocate dom0's memory above 4GB where the domain cannot
access it without paging (which is disabled at start of day) and LPAE (which
the kernel may not support) enabled.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/domain_build.c |   16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 4d6b26b..e7cc2c9 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -69,19 +69,21 @@ static void allocate_memory_11(struct domain *d, struct kernel_info *kinfo)
 {
     paddr_t start;
     paddr_t size;
-    struct page_info *pg = NULL;
+    struct page_info *pg;
     unsigned int order = get_order_from_bytes(dom0_mem);
     int res;
     paddr_t spfn;
-    unsigned int bits;
 
-    for ( bits = PAGE_SHIFT + 1; bits < PADDR_BITS; bits++ )
+    if ( is_32bit_domain(d) )
     {
-        pg = alloc_domheap_pages(d, order, MEMF_bits(bits));
-        if ( pg != NULL )
-            break;
+        printk("32 bit domain\n");
+        pg = alloc_domheap_pages(d, order, MEMF_bits(32));
+    }
+    else
+    {
+        printk("64 bit domain\n");
+        pg = alloc_domheap_pages(d, order, 0);
     }
-
     if ( !pg )
         panic("Failed to allocate contiguous memory for dom0");
 
-- 
1.7.10.4

Re: [PATCH 3/3] Revert "xen/arm: Allocate memory for dom0 from the bottom with the 1:1 Workaround"

[Julien Grall]

Hi Ian,

On 04/04/2014 01:56 PM, Ian Campbell wrote:
> This reverts commit 6c21cb36e263de2db8716b477157a5b6cd531e1e.
> 
> The Linux = issue which this works around was fixed in v3.13 via f52bb722547f
> "ARM: mm: Correct virt_to_phys patching for 64 bit physical addresses".
> 
> This is the second attempt to revert this. Now that we have fixed
> allocate_memory_11 to allocate accessible memory on 32-bit this is safe to do.
> This is not quite a straight revert since we need to ensure that for 32-bit
> domain 0 we do not allocate dom0's memory above 4GB where the domain cannot
> access it without paging (which is disabled at start of day) and LPAE (which
> the kernel may not support) enabled.

When multiple banks will be supported, I guess it will be safe to
allocate above 32bits (if dom0 has more than 4G) of RAM.

Anyway, it's not the goal of this patch :).

>  xen/arch/arm/domain_build.c |   16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 4d6b26b..e7cc2c9 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -69,19 +69,21 @@ static void allocate_memory_11(struct domain *d, struct kernel_info *kinfo)
>  {
>      paddr_t start;
>      paddr_t size;
> -    struct page_info *pg = NULL;
> +    struct page_info *pg;
>      unsigned int order = get_order_from_bytes(dom0_mem);
>      int res;
>      paddr_t spfn;
> -    unsigned int bits;
>  
> -    for ( bits = PAGE_SHIFT + 1; bits < PADDR_BITS; bits++ )
> +    if ( is_32bit_domain(d) )
>      {
> -        pg = alloc_domheap_pages(d, order, MEMF_bits(bits));
> -        if ( pg != NULL )
> -            break;
> +        printk("32 bit domain\n");

[..]

> +        printk("64 bit domain\n");

Can you be more explicit on both of theses messages? Nothing specify we
are allocate memory before them.

Regards,

-- 
Julien Grall

Re: [PATCH 3/3] Revert "xen/arm: Allocate memory for dom0 from the bottom with the 1:1 Workaround"

[Ian Campbell]

On Fri, 2014-04-04 at 15:48 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 04/04/2014 01:56 PM, Ian Campbell wrote:
> > This reverts commit 6c21cb36e263de2db8716b477157a5b6cd531e1e.
> > 
> > The Linux = issue which this works around was fixed in v3.13 via f52bb722547f
> > "ARM: mm: Correct virt_to_phys patching for 64 bit physical addresses".
> > 
> > This is the second attempt to revert this. Now that we have fixed
> > allocate_memory_11 to allocate accessible memory on 32-bit this is safe to do.
> > This is not quite a straight revert since we need to ensure that for 32-bit
> > domain 0 we do not allocate dom0's memory above 4GB where the domain cannot
> > access it without paging (which is disabled at start of day) and LPAE (which
> > the kernel may not support) enabled.
> 
> When multiple banks will be supported, I guess it will be safe to
> allocate above 32bits (if dom0 has more than 4G) of RAM.

Yes. We should aim for as much of dom0's memory as possible to be below
4GB, but if the user asked for more than that we may as well put it
above 4GB and hope that the kernel supports LPAE.

> Anyway, it's not the goal of this patch :).

Yes.

> >  xen/arch/arm/domain_build.c |   16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 4d6b26b..e7cc2c9 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -69,19 +69,21 @@ static void allocate_memory_11(struct domain *d, struct kernel_info *kinfo)
> >  {
> >      paddr_t start;
> >      paddr_t size;
> > -    struct page_info *pg = NULL;
> > +    struct page_info *pg;
> >      unsigned int order = get_order_from_bytes(dom0_mem);
> >      int res;
> >      paddr_t spfn;
> > -    unsigned int bits;
> >  
> > -    for ( bits = PAGE_SHIFT + 1; bits < PADDR_BITS; bits++ )
> > +    if ( is_32bit_domain(d) )
> >      {
> > -        pg = alloc_domheap_pages(d, order, MEMF_bits(bits));
> > -        if ( pg != NULL )
> > -            break;
> > +        printk("32 bit domain\n");
> 
> [..]
> 
> > +        printk("64 bit domain\n");
> 
> Can you be more explicit on both of theses messages? Nothing specify we
> are allocate memory before them.

ops, those were debugging which I shuld have removed.

Re: [PATCH 3/3] Revert "xen/arm: Allocate memory for dom0 from the bottom with the 1:1 Workaround"

[Julien Grall]

On 04/04/2014 03:52 PM, Ian Campbell wrote:
> On Fri, 2014-04-04 at 15:48 +0100, Julien Grall wrote:
>> Can you be more explicit on both of theses messages? Nothing specify we
>> are allocate memory before them.
> 
> ops, those were debugging which I shuld have removed.

With this change:

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

-- 
Julien Grall

Re: [PATCH 0/3] Really revert xen/arm: Allocate memory for dom0 from the bottom with the 1:1 Workaround

[Ian Campbell]

On Fri, 2014-04-04 at 13:56 +0100, Ian Campbell wrote:
> This required a bit of yak shaving in the first two patches. Tested on
> arm32 (w/ >4GB) and arm64.

Applied with Julien's comments + acks.