| Message ID | 1397044276-30185-1-git-send-email-ian.campbell@citrix.com |
|---|---|
| State | Accepted |
| Commit | 6f4ff742a5caa411397fc38233f818e64a0c541c |
| Headers | show |
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,
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, >
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.
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.
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.
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;
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(-)