[PATCH 0/3] xen: arm: fixes for initrd and dtb placement

[PATCH 0/3] xen: arm: fixes for initrd and dtb placement

[Ian Campbell]

The first patch here should address the regression introduced by
314c9815e2f5 "tools: implement initial ramdisk support for ARM." and
should be backported at the same time.

The second two patches make the dom0 placement algorithm the same as the
libxc one.

Ian.

[PATCH 1/3] tools: arm: improve placement of initial modules.

[Ian Campbell]

314c9815e2f5 "tools: implement initial ramdisk support for ARM." broke starting
guests with <= 128 MB ram by placing the boot modules (dtb and initrd)
immediately after the kernel in this case, running the risk of them being
overwritten. Instead place the modules at the end of RAM, as the hypervisor
does for dom0.

The hypervisor also falls back to placing things before the kernel as a last
resort before failing, so add that here too.

Tested with the Debian installer initrd and guests of 96MB, 128MB, 256MB and
1GB. All work, also tested with 64MB but the installer doesn't run with so
little RAM (but our placement of the initrd is correct).

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
This should be added to 4.4.1 along with 314c9815e2f5.
---
 tools/libxc/xc_dom_arm.c |   29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
index f051515..60ac51a 100644
--- a/tools/libxc/xc_dom_arm.c
+++ b/tools/libxc/xc_dom_arm.c
@@ -253,8 +253,11 @@ int arch_setup_meminit(struct xc_dom_image *dom)
 
     /* 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 = dom->total_pages << XC_PAGE_SHIFT;
+    const uint64_t ramend = rambase + ramsize;
+    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;
     const uint64_t dtb_size = dom->devicetree_blob ?
         ROUNDUP(dom->devicetree_size, XC_PAGE_SHIFT) : 0;
     const uint64_t ramdisk_size = dom->ramdisk_blob ?
@@ -262,6 +265,13 @@ 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 ( modsize + kernsize > ramsize )
+    {
+        DOMPRINTF("%s: Not enough memory for the kernel+dtb+initrd",
+                  __FUNCTION__);
+        return -1;
+    }
+
     rc = set_mode(dom->xch, dom->guest_domid, dom->guest_type);
     if ( rc )
         return rc;
@@ -290,17 +300,20 @@ int arch_setup_meminit(struct xc_dom_image *dom)
             0, 0, &dom->p2m_host[i]);
     }
 
-
     /*
-     * Place boot modules at 128MB into 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.
+     * We try to place dtb+initrd at 128MB or if we have less RAM
+     * as high as possible. If there is no space then fallback to
+     * just before the kernel.
+     *
+     * If changing this then consider
+     * xen/arch/arm/kernel.c:place_modules as well.
      */
     if ( ramend >= ram128mb + modsize && kernend < ram128mb )
         modbase = ram128mb;
-    else if ( ramend >= kernend + modsize )
-        modbase = kernend;
+    else if ( ramend - modsize > kernend )
+        modbase = ramend - modsize;
+    else if (kernbase - rambase > modsize )
+        modbase = kernbase - modsize;
     else
         return -1;
 
-- 
1.7.10.4

[PATCH 2/3] xen: arm: use same variables as userspace in dom0 builder place_modules()

[Ian Campbell]

The placement algorithm should be effectively the same and using different
variable names makes my head hurt when I try to compare.

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

diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index ae86772..bc625a4 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -65,25 +65,25 @@ void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len)
 }
 
 static void place_modules(struct kernel_info *info,
-                         paddr_t kernel_start,
-                         paddr_t kernel_end)
+                          paddr_t kernbase, paddr_t kernend)
 {
     /* Align DTB and initrd size to 2Mb. Linux only requires 4 byte alignment */
     const paddr_t initrd_len =
         ROUNDUP(early_info.modules.module[MOD_INITRD].size, MB(2));
     const paddr_t dtb_len = ROUNDUP(fdt_totalsize(info->fdt), MB(2));
-    const paddr_t total = initrd_len + dtb_len;
+    const paddr_t modsize = initrd_len + dtb_len;
 
     /* Convenient */
-    const paddr_t mem_start = info->mem.bank[0].start;
-    const paddr_t mem_size = info->mem.bank[0].size;
-    const paddr_t mem_end = mem_start + mem_size;
-    const paddr_t kernel_size = kernel_end - kernel_start;
+    const paddr_t rambase = info->mem.bank[0].start;
+    const paddr_t ramsize = info->mem.bank[0].size;
+    const paddr_t ramend = rambase + ramsize;
+    const paddr_t kernsize = kernend - kernbase;
+    const paddr_t ram128mb = rambase + MB(128);
 
-    paddr_t addr;
+    paddr_t modbase;
 
-    if ( total + kernel_size > mem_size )
-        panic("Not enough memory in the first bank for the dtb+initrd");
+    if ( modsize + kernsize > ramsize )
+        panic("Not enough memory in the first bank for the kernel+dtb+initrd");
 
     /*
      * DTB must be loaded such that it does not conflict with the
@@ -100,19 +100,19 @@ static void place_modules(struct kernel_info *info,
      * just after the kernel, if there is room, otherwise just before.
      */
 
-    if ( kernel_end < MIN(mem_start + MB(128), mem_end - total) )
-        addr = MIN(mem_start + MB(128), mem_end - total);
-    else if ( mem_end - ROUNDUP(kernel_end, MB(2)) >= total )
-        addr = ROUNDUP(kernel_end, MB(2));
-    else if ( kernel_start - mem_start >= total )
-        addr = kernel_start - total;
+    if ( kernend < MIN(ram128mb, ramend - modsize) )
+        modbase = MIN(ram128mb, ramend - modsize);
+    else if ( ramend - ROUNDUP(kernend, MB(2)) >= modsize )
+        modbase = ROUNDUP(kernend, MB(2));
+    else if ( kernbase - rambase >= modsize )
+        modbase = kernbase - modsize;
     else
     {
         panic("Unable to find suitable location for dtb+initrd");
         return;
     }
 
-    info->dtb_paddr = addr;
+    info->dtb_paddr = modbase;
     info->initrd_paddr = info->dtb_paddr + dtb_len;
 }
 
-- 
1.7.10.4

[PATCH 3/3] xen: arm: rework dom0 initrd and dtb placement

[Ian Campbell]

This now uses the same decision tree as libxc (which is much easier to test).

The main change is to explictly handle the placement at 128MB or end of RAM as
two cases, rather than combining with MIN. The effect is the same but the code
is clearer.

Secondly the attempt to place the mopules right after the kernel is removed,
since it is redundant with the case where placing them at the end of RAM ends
up abutting the kernel.

Also round the kernel size up to a 2MB boundary.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
I'm sure to regret playing with this yet again...
---
 xen/arch/arm/kernel.c |   22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index bc625a4..1102392 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -77,7 +77,7 @@ static void place_modules(struct kernel_info *info,
     const paddr_t rambase = info->mem.bank[0].start;
     const paddr_t ramsize = info->mem.bank[0].size;
     const paddr_t ramend = rambase + ramsize;
-    const paddr_t kernsize = kernend - kernbase;
+    const paddr_t kernsize = ROUNDUP(kernend, MB(2)) - kernbase;
     const paddr_t ram128mb = rambase + MB(128);
 
     paddr_t modbase;
@@ -95,16 +95,18 @@ static void place_modules(struct kernel_info *info,
      * If the bootloader provides an initrd, it will be loaded just
      * after the DTB.
      *
-     * We try to place dtb+initrd at 128MB, (or, if we have less RAM,
-     * as high as possible). If there is no space then fallback to
-     * just after the kernel, if there is room, otherwise just before.
+     * We try to place dtb+initrd at 128MB or if we have less RAM
+     * as high as possible. If there is no space then fallback to
+     * just before the kernel.
+     *
+     * If changing this then consider
+     * tools/libxc/xc_dom_arm.c:arch_setup_meminit as well.
      */
-
-    if ( kernend < MIN(ram128mb, ramend - modsize) )
-        modbase = MIN(ram128mb, ramend - modsize);
-    else if ( ramend - ROUNDUP(kernend, MB(2)) >= modsize )
-        modbase = ROUNDUP(kernend, MB(2));
-    else if ( kernbase - rambase >= modsize )
+    if ( ramend >= ram128mb + modsize && kernend < ram128mb )
+        modbase = ram128mb;
+    else if ( ramend - modsize > ROUNDUP(kernend, MB(2)) )
+        modbase = ramend - modsize;
+    else if ( kernbase - rambase > modsize )
         modbase = kernbase - modsize;
     else
     {
-- 
1.7.10.4

Re: [PATCH 1/3] tools: arm: improve placement of initial modules.

[Julien Grall]

Hi Ian,

On 04/09/2014 12:51 PM, Ian Campbell wrote:
> 314c9815e2f5 "tools: implement initial ramdisk support for ARM." broke starting
> guests with <= 128 MB ram by placing the boot modules (dtb and initrd)
> immediately after the kernel in this case, running the risk of them being
> overwritten. Instead place the modules at the end of RAM, as the hypervisor
> does for dom0.
> 
> The hypervisor also falls back to placing things before the kernel as a last

Did you mean "falls backs on placing"?

Other than that the patch looks good to me:

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

Regards,

-- 
Julien Grall

Re: [PATCH 2/3] xen: arm: use same variables as userspace in dom0 builder place_modules()

[Julien Grall]

On 04/09/2014 12:51 PM, Ian Campbell wrote:
>  static void place_modules(struct kernel_info *info,
> -                         paddr_t kernel_start,
> -                         paddr_t kernel_end)
> +                          paddr_t kernbase, paddr_t kernend)
>  {
>      /* Align DTB and initrd size to 2Mb. Linux only requires 4 byte alignment */
>      const paddr_t initrd_len =
>          ROUNDUP(early_info.modules.module[MOD_INITRD].size, MB(2));
>      const paddr_t dtb_len = ROUNDUP(fdt_totalsize(info->fdt), MB(2));
> -    const paddr_t total = initrd_len + dtb_len;
> +    const paddr_t modsize = initrd_len + dtb_len;
>  
>      /* Convenient */
> -    const paddr_t mem_start = info->mem.bank[0].start;
> -    const paddr_t mem_size = info->mem.bank[0].size;
> -    const paddr_t mem_end = mem_start + mem_size;
> -    const paddr_t kernel_size = kernel_end - kernel_start;
> +    const paddr_t rambase = info->mem.bank[0].start;
> +    const paddr_t ramsize = info->mem.bank[0].size;
> +    const paddr_t ramend = rambase + ramsize;
> +    const paddr_t kernsize = kernend - kernbase;
> +    const paddr_t ram128mb = rambase + MB(128);

Shall we use ram0base, ram0size, ram0end to show that we are using that
the first bank?

Regards,

-- 
Julien Grall

Re: [PATCH 1/3] tools: arm: improve placement of initial modules.

[Ian Campbell]

On Wed, 2014-04-09 at 13:43 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 04/09/2014 12:51 PM, Ian Campbell wrote:
> > 314c9815e2f5 "tools: implement initial ramdisk support for ARM." broke starting
> > guests with <= 128 MB ram by placing the boot modules (dtb and initrd)
> > immediately after the kernel in this case, running the risk of them being
> > overwritten. Instead place the modules at the end of RAM, as the hypervisor
> > does for dom0.
> > 
> > The hypervisor also falls back to placing things before the kernel as a last
> 
> Did you mean "falls backs on placing"?

"Falls back to X" is also correct, "on" might be but reads strangely to
me in this particular context.

> Other than that the patch looks good to me:
> 
> Acked-by: Julien Grall <julien.grall@linaro.org>

Thanks.

> Regards,
> 

Re: [PATCH 2/3] xen: arm: use same variables as userspace in dom0 builder place_modules()

[Ian Campbell]

On Wed, 2014-04-09 at 13:45 +0100, Julien Grall wrote:
> On 04/09/2014 12:51 PM, Ian Campbell wrote:
> >  static void place_modules(struct kernel_info *info,
> > -                         paddr_t kernel_start,
> > -                         paddr_t kernel_end)
> > +                          paddr_t kernbase, paddr_t kernend)
> >  {
> >      /* Align DTB and initrd size to 2Mb. Linux only requires 4 byte alignment */
> >      const paddr_t initrd_len =
> >          ROUNDUP(early_info.modules.module[MOD_INITRD].size, MB(2));
> >      const paddr_t dtb_len = ROUNDUP(fdt_totalsize(info->fdt), MB(2));
> > -    const paddr_t total = initrd_len + dtb_len;
> > +    const paddr_t modsize = initrd_len + dtb_len;
> >  
> >      /* Convenient */
> > -    const paddr_t mem_start = info->mem.bank[0].start;
> > -    const paddr_t mem_size = info->mem.bank[0].size;
> > -    const paddr_t mem_end = mem_start + mem_size;
> > -    const paddr_t kernel_size = kernel_end - kernel_start;
> > +    const paddr_t rambase = info->mem.bank[0].start;
> > +    const paddr_t ramsize = info->mem.bank[0].size;
> > +    const paddr_t ramend = rambase + ramsize;
> > +    const paddr_t kernsize = kernend - kernbase;
> > +    const paddr_t ram128mb = rambase + MB(128);
> 
> Shall we use ram0base, ram0size, ram0end to show that we are using that
> the first bank?

I was planning to do that in my 1TB RAM series, but this is different
code isn't it. Oops.

I'll change if there is some other reason to repost, but I don't think
it is worth reposting just for that.

Ian.

Re: [PATCH 3/3] xen: arm: rework dom0 initrd and dtb placement

[Julien Grall]

Hi Ian,

On 04/09/2014 12:51 PM, Ian Campbell wrote:
> This now uses the same decision tree as libxc (which is much easier to test).
> 
> The main change is to explictly handle the placement at 128MB or end of RAM as

s/explictly/explicitly/

> two cases, rather than combining with MIN. The effect is the same but the code
> is clearer.
> 
> Secondly the attempt to place the mopules right after the kernel is removed,

s/mopules/modules/

> since it is redundant with the case where placing them at the end of RAM ends
> up abutting the kernel.
> 
> Also round the kernel size up to a 2MB boundary.

A bit surprised that it was not done before :).

> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> I'm sure to regret playing with this yet again...
> ---
>  xen/arch/arm/kernel.c |   22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index bc625a4..1102392 100644
> --- a/xen/arch/arm/kernel.c
> +++ b/xen/arch/arm/kernel.c
> @@ -77,7 +77,7 @@ static void place_modules(struct kernel_info *info,
>      const paddr_t rambase = info->mem.bank[0].start;
>      const paddr_t ramsize = info->mem.bank[0].size;
>      const paddr_t ramend = rambase + ramsize;
> -    const paddr_t kernsize = kernend - kernbase;
> +    const paddr_t kernsize = ROUNDUP(kernend, MB(2)) - kernbase;

You are using ROUNDUP(kernend, MB(2)) in few places, why didn't you
roundup directly kernend?

-- 
Julien Grall

Re: [PATCH 3/3] xen: arm: rework dom0 initrd and dtb placement

[Ian Campbell]

On Wed, 2014-04-09 at 13:54 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 04/09/2014 12:51 PM, Ian Campbell wrote:
> > This now uses the same decision tree as libxc (which is much easier to test).
> > 
> > The main change is to explictly handle the placement at 128MB or end of RAM as
> 
> s/explictly/explicitly/
> s/mopules/modules/

Gah, fingers not working right today it seems.

> > since it is redundant with the case where placing them at the end of RAM ends
> > up abutting the kernel.
> > 
> > Also round the kernel size up to a 2MB boundary.
> 
> A bit surprised that it was not done before :).

Me too.

> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> > I'm sure to regret playing with this yet again...
> > ---
> >  xen/arch/arm/kernel.c |   22 ++++++++++++----------
> >  1 file changed, 12 insertions(+), 10 deletions(-)
> > 
> > diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> > index bc625a4..1102392 100644
> > --- a/xen/arch/arm/kernel.c
> > +++ b/xen/arch/arm/kernel.c
> > @@ -77,7 +77,7 @@ static void place_modules(struct kernel_info *info,
> >      const paddr_t rambase = info->mem.bank[0].start;
> >      const paddr_t ramsize = info->mem.bank[0].size;
> >      const paddr_t ramend = rambase + ramsize;
> > -    const paddr_t kernsize = kernend - kernbase;
> > +    const paddr_t kernsize = ROUNDUP(kernend, MB(2)) - kernbase;
> 
> You are using ROUNDUP(kernend, MB(2)) in few places, why didn't you
> roundup directly kernend?

It's passed as a paramter, and it's not possible to round it before
using it here (code before declarations), so I'd have to make kernsize
non-const and initialise it after the rounding. I didn't think it was
worth it.

Ian.

Re: [PATCH 3/3] xen: arm: rework dom0 initrd and dtb placement

[Julien Grall]

On 04/09/2014 01:57 PM, Ian Campbell wrote:
> On Wed, 2014-04-09 at 13:54 +0100, Julien Grall wrote:
>> Hi Ian,
>>
>> On 04/09/2014 12:51 PM, Ian Campbell wrote:
>>> This now uses the same decision tree as libxc (which is much easier to test).
>>>
>>> The main change is to explictly handle the placement at 128MB or end of RAM as
>>
>> s/explictly/explicitly/
>> s/mopules/modules/
> 
> Gah, fingers not working right today it seems.
> 
>>> since it is redundant with the case where placing them at the end of RAM ends
>>> up abutting the kernel.
>>>
>>> Also round the kernel size up to a 2MB boundary.
>>
>> A bit surprised that it was not done before :).
> 
> Me too.
> 
>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>> ---
>>> I'm sure to regret playing with this yet again...
>>> ---
>>>  xen/arch/arm/kernel.c |   22 ++++++++++++----------
>>>  1 file changed, 12 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
>>> index bc625a4..1102392 100644
>>> --- a/xen/arch/arm/kernel.c
>>> +++ b/xen/arch/arm/kernel.c
>>> @@ -77,7 +77,7 @@ static void place_modules(struct kernel_info *info,
>>>      const paddr_t rambase = info->mem.bank[0].start;
>>>      const paddr_t ramsize = info->mem.bank[0].size;
>>>      const paddr_t ramend = rambase + ramsize;
>>> -    const paddr_t kernsize = kernend - kernbase;
>>> +    const paddr_t kernsize = ROUNDUP(kernend, MB(2)) - kernbase;
>>
>> You are using ROUNDUP(kernend, MB(2)) in few places, why didn't you
>> roundup directly kernend?
> 
> It's passed as a paramter, and it's not possible to round it before
> using it here (code before declarations), so I'd have to make kernsize
> non-const and initialise it after the rounding. I didn't think it was
> worth it.

In this case I'm lost... You are mixing kernend and ROUNDUP(kernend, MB(2)).

As I understand, we don't need to round up on the second if expression
(see code below).

+    if ( ramend >= ram128mb + modsize && kernend < ram128mb )
+        modbase = ram128mb;
+    else if ( ramend - modsize > ROUNDUP(kernend, MB(2)) )
+        modbase = ramend - modsize;
+    else if ( kernbase - rambase > modsize )

Regards,

-- 
Julien Grall

Re: [PATCH 3/3] xen: arm: rework dom0 initrd and dtb placement

[Ian Campbell]

On Wed, 2014-04-09 at 14:09 +0100, Julien Grall wrote:
> On 04/09/2014 01:57 PM, Ian Campbell wrote:
> > On Wed, 2014-04-09 at 13:54 +0100, Julien Grall wrote:
> >> Hi Ian,
> >>
> >> On 04/09/2014 12:51 PM, Ian Campbell wrote:
> >>> This now uses the same decision tree as libxc (which is much easier to test).
> >>>
> >>> The main change is to explictly handle the placement at 128MB or end of RAM as
> >>
> >> s/explictly/explicitly/
> >> s/mopules/modules/
> > 
> > Gah, fingers not working right today it seems.
> > 
> >>> since it is redundant with the case where placing them at the end of RAM ends
> >>> up abutting the kernel.
> >>>
> >>> Also round the kernel size up to a 2MB boundary.
> >>
> >> A bit surprised that it was not done before :).
> > 
> > Me too.
> > 
> >>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> >>> ---
> >>> I'm sure to regret playing with this yet again...
> >>> ---
> >>>  xen/arch/arm/kernel.c |   22 ++++++++++++----------
> >>>  1 file changed, 12 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> >>> index bc625a4..1102392 100644
> >>> --- a/xen/arch/arm/kernel.c
> >>> +++ b/xen/arch/arm/kernel.c
> >>> @@ -77,7 +77,7 @@ static void place_modules(struct kernel_info *info,
> >>>      const paddr_t rambase = info->mem.bank[0].start;
> >>>      const paddr_t ramsize = info->mem.bank[0].size;
> >>>      const paddr_t ramend = rambase + ramsize;
> >>> -    const paddr_t kernsize = kernend - kernbase;
> >>> +    const paddr_t kernsize = ROUNDUP(kernend, MB(2)) - kernbase;
> >>
> >> You are using ROUNDUP(kernend, MB(2)) in few places, why didn't you
> >> roundup directly kernend?
> > 
> > It's passed as a paramter, and it's not possible to round it before
> > using it here (code before declarations), so I'd have to make kernsize
> > non-const and initialise it after the rounding. I didn't think it was
> > worth it.
> 
> In this case I'm lost... You are mixing kernend and ROUNDUP(kernend, MB(2)).
> 
> As I understand, we don't need to round up on the second if expression
> (see code below).

It ensures that the modules don't start until at least the 2M boundary
after the kernel's end. The userspace side does the same.

Whether that's really worthwhile I don't know.

> 
> +    if ( ramend >= ram128mb + modsize && kernend < ram128mb )
> +        modbase = ram128mb;
> +    else if ( ramend - modsize > ROUNDUP(kernend, MB(2)) )
> +        modbase = ramend - modsize;
> +    else if ( kernbase - rambase > modsize )
> 
> Regards,
> 

Re: [PATCH 3/3] xen: arm: rework dom0 initrd and dtb placement

[Julien Grall]

On 04/09/2014 02:15 PM, Ian Campbell wrote:
> On Wed, 2014-04-09 at 14:09 +0100, Julien Grall wrote:
>> On 04/09/2014 01:57 PM, Ian Campbell wrote:
>>> On Wed, 2014-04-09 at 13:54 +0100, Julien Grall wrote:
>>>> Hi Ian,
>>>>
>>>> On 04/09/2014 12:51 PM, Ian Campbell wrote:
>>>>> This now uses the same decision tree as libxc (which is much easier to test).
>>>>>
>>>>> The main change is to explictly handle the placement at 128MB or end of RAM as
>>>>
>>>> s/explictly/explicitly/
>>>> s/mopules/modules/
>>>
>>> Gah, fingers not working right today it seems.
>>>
>>>>> since it is redundant with the case where placing them at the end of RAM ends
>>>>> up abutting the kernel.
>>>>>
>>>>> Also round the kernel size up to a 2MB boundary.
>>>>
>>>> A bit surprised that it was not done before :).
>>>
>>> Me too.
>>>
>>>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>>>> ---
>>>>> I'm sure to regret playing with this yet again...
>>>>> ---
>>>>>  xen/arch/arm/kernel.c |   22 ++++++++++++----------
>>>>>  1 file changed, 12 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
>>>>> index bc625a4..1102392 100644
>>>>> --- a/xen/arch/arm/kernel.c
>>>>> +++ b/xen/arch/arm/kernel.c
>>>>> @@ -77,7 +77,7 @@ static void place_modules(struct kernel_info *info,
>>>>>      const paddr_t rambase = info->mem.bank[0].start;
>>>>>      const paddr_t ramsize = info->mem.bank[0].size;
>>>>>      const paddr_t ramend = rambase + ramsize;
>>>>> -    const paddr_t kernsize = kernend - kernbase;
>>>>> +    const paddr_t kernsize = ROUNDUP(kernend, MB(2)) - kernbase;
>>>>
>>>> You are using ROUNDUP(kernend, MB(2)) in few places, why didn't you
>>>> roundup directly kernend?
>>>
>>> It's passed as a paramter, and it's not possible to round it before
>>> using it here (code before declarations), so I'd have to make kernsize
>>> non-const and initialise it after the rounding. I didn't think it was
>>> worth it.
>>
>> In this case I'm lost... You are mixing kernend and ROUNDUP(kernend, MB(2)).
>>
>> As I understand, we don't need to round up on the second if expression
>> (see code below).
> 
> It ensures that the modules don't start until at least the 2M boundary
> after the kernel's end. The userspace side does the same.

The userspace is consistent with the usage of kernend. It's not the case
here. There is no reason to mix the usage of kernend with and without
ROUNDUP.

In another point, I don't think 2MB-aligned is enough to ensure the
kernel won't decompress on the device tree.

-- 
Julien Grall

Re: [PATCH 3/3] xen: arm: rework dom0 initrd and dtb placement

[Ian Campbell]

On Wed, 2014-04-09 at 14:24 +0100, Julien Grall wrote:
> On 04/09/2014 02:15 PM, Ian Campbell wrote:
> > On Wed, 2014-04-09 at 14:09 +0100, Julien Grall wrote:
> >> On 04/09/2014 01:57 PM, Ian Campbell wrote:
> >>> On Wed, 2014-04-09 at 13:54 +0100, Julien Grall wrote:
> >>>> Hi Ian,
> >>>>
> >>>> On 04/09/2014 12:51 PM, Ian Campbell wrote:
> >>>>> This now uses the same decision tree as libxc (which is much easier to test).
> >>>>>
> >>>>> The main change is to explictly handle the placement at 128MB or end of RAM as
> >>>>
> >>>> s/explictly/explicitly/
> >>>> s/mopules/modules/
> >>>
> >>> Gah, fingers not working right today it seems.
> >>>
> >>>>> since it is redundant with the case where placing them at the end of RAM ends
> >>>>> up abutting the kernel.
> >>>>>
> >>>>> Also round the kernel size up to a 2MB boundary.
> >>>>
> >>>> A bit surprised that it was not done before :).
> >>>
> >>> Me too.
> >>>
> >>>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> >>>>> ---
> >>>>> I'm sure to regret playing with this yet again...
> >>>>> ---
> >>>>>  xen/arch/arm/kernel.c |   22 ++++++++++++----------
> >>>>>  1 file changed, 12 insertions(+), 10 deletions(-)
> >>>>>
> >>>>> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> >>>>> index bc625a4..1102392 100644
> >>>>> --- a/xen/arch/arm/kernel.c
> >>>>> +++ b/xen/arch/arm/kernel.c
> >>>>> @@ -77,7 +77,7 @@ static void place_modules(struct kernel_info *info,
> >>>>>      const paddr_t rambase = info->mem.bank[0].start;
> >>>>>      const paddr_t ramsize = info->mem.bank[0].size;
> >>>>>      const paddr_t ramend = rambase + ramsize;
> >>>>> -    const paddr_t kernsize = kernend - kernbase;
> >>>>> +    const paddr_t kernsize = ROUNDUP(kernend, MB(2)) - kernbase;
> >>>>
> >>>> You are using ROUNDUP(kernend, MB(2)) in few places, why didn't you
> >>>> roundup directly kernend?
> >>>
> >>> It's passed as a paramter, and it's not possible to round it before
> >>> using it here (code before declarations), so I'd have to make kernsize
> >>> non-const and initialise it after the rounding. I didn't think it was
> >>> worth it.
> >>
> >> In this case I'm lost... You are mixing kernend and ROUNDUP(kernend, MB(2)).
> >>
> >> As I understand, we don't need to round up on the second if expression
> >> (see code below).
> > 
> > It ensures that the modules don't start until at least the 2M boundary
> > after the kernel's end. The userspace side does the same.
> 
> The userspace is consistent with the usage of kernend. It's not the case
> here. There is no reason to mix the usage of kernend with and without
> ROUNDUP.

Ah, you mean the lack of rounding in the first if:
        if ( ramend >= ram128mb + modsize && kernend < ram128mb )
?

Since ram128mb is 2MB aligned it doesn't really matter that kernend
isn't.

> In another point, I don't think 2MB-aligned is enough to ensure the
> kernel won't decompress on the device tree.

Yes, like I say I don't know if it actually worthwhile.

Ian.

Re: [PATCH 3/3] xen: arm: rework dom0 initrd and dtb placement

[Julien Grall]

On 04/09/2014 02:28 PM, Ian Campbell wrote:
> 
> Ah, you mean the lack of rounding in the first if:
>         if ( ramend >= ram128mb + modsize && kernend < ram128mb )
> ?
> 
> Since ram128mb is 2MB aligned it doesn't really matter that kernend
> isn't.

Oh right, I was lost with all the arithmetic.

>> In another point, I don't think 2MB-aligned is enough to ensure the
>> kernel won't decompress on the device tree.
> 
> Yes, like I say I don't know if it actually worthwhile.

Ok. Let's stick with this solution and see if someone platform will
complain:

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

-- 
Julien Grall

Re: [PATCH 2/3] xen: arm: use same variables as userspace in dom0 builder place_modules()

[Julien Grall]

On 04/09/2014 01:52 PM, Ian Campbell wrote:
> On Wed, 2014-04-09 at 13:45 +0100, Julien Grall wrote:
>> On 04/09/2014 12:51 PM, Ian Campbell wrote:
>>>  static void place_modules(struct kernel_info *info,
>>> -                         paddr_t kernel_start,
>>> -                         paddr_t kernel_end)
>>> +                          paddr_t kernbase, paddr_t kernend)
>>>  {
>>>      /* Align DTB and initrd size to 2Mb. Linux only requires 4 byte alignment */
>>>      const paddr_t initrd_len =
>>>          ROUNDUP(early_info.modules.module[MOD_INITRD].size, MB(2));
>>>      const paddr_t dtb_len = ROUNDUP(fdt_totalsize(info->fdt), MB(2));
>>> -    const paddr_t total = initrd_len + dtb_len;
>>> +    const paddr_t modsize = initrd_len + dtb_len;
>>>  
>>>      /* Convenient */
>>> -    const paddr_t mem_start = info->mem.bank[0].start;
>>> -    const paddr_t mem_size = info->mem.bank[0].size;
>>> -    const paddr_t mem_end = mem_start + mem_size;
>>> -    const paddr_t kernel_size = kernel_end - kernel_start;
>>> +    const paddr_t rambase = info->mem.bank[0].start;
>>> +    const paddr_t ramsize = info->mem.bank[0].size;
>>> +    const paddr_t ramend = rambase + ramsize;
>>> +    const paddr_t kernsize = kernend - kernbase;
>>> +    const paddr_t ram128mb = rambase + MB(128);
>>
>> Shall we use ram0base, ram0size, ram0end to show that we are using that
>> the first bank?
> 
> I was planning to do that in my 1TB RAM series, but this is different
> code isn't it. Oops.
> 
> I'll change if there is some other reason to repost, but I don't think
> it is worth reposting just for that.

Ok. With or without this change:

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

-- 
Julien Grall

Re: [PATCH 1/3] tools: arm: improve placement of initial modules.

[Ian Campbell]

On Wed, 2014-04-09 at 13:43 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 04/09/2014 12:51 PM, Ian Campbell wrote:
> > 314c9815e2f5 "tools: implement initial ramdisk support for ARM." broke starting
> > guests with <= 128 MB ram by placing the boot modules (dtb and initrd)
> > immediately after the kernel in this case, running the risk of them being
> > overwritten. Instead place the modules at the end of RAM, as the hypervisor
> > does for dom0.
> > 
> > The hypervisor also falls back to placing things before the kernel as a last
> 
> Did you mean "falls backs on placing"?
> 
> Other than that the patch looks good to me:
> 
> Acked-by: Julien Grall <julien.grall@linaro.org>

Applied. Ian, please add 6f4ff742a5caa411397fc38233f818e64a0c541c to be
backported along with 314c9815e2f5dc8a9fec11e0cf9b49b16ed0e96b.

Ian.

Re: [PATCH 1/3] tools: arm: improve placement of initial modules.

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 1/3] tools: arm: improve placement of initial modules."):
> Applied. Ian, please add 6f4ff742a5caa411397fc38233f818e64a0c541c to be
> backported along with 314c9815e2f5dc8a9fec11e0cf9b49b16ed0e96b.

Queued.

Thanks,
Ian.

Re: [PATCH 1/3] tools: arm: improve placement of initial modules.

[Ian Jackson]

Ian Jackson writes ("Re: [PATCH 1/3] tools: arm: improve placement of initial modules."):
> Ian Campbell writes ("Re: [PATCH 1/3] tools: arm: improve placement of initial modules."):
> > Applied. Ian, please add 6f4ff742a5caa411397fc38233f818e64a0c541c to be
> > backported along with 314c9815e2f5dc8a9fec11e0cf9b49b16ed0e96b.
> 
> Queued.

Backported to 4.4.

Ian.