[PATCH v2] tools: implement initial ramdisk support for ARM.

[PATCH v2] tools: implement initial ramdisk support for ARM.

[Ian Campbell]

The ramdisk is passed to the kernel as a property in the chosen node of the
device tree. This is somewhat tricky since in order to place the ramdisk and
dtb in ram we first need to know the size of the dtb. So we initially create a
DTB with placeholders for the ramdisk and finalise the value (which doesn't
change the size) once we know where everything is.

Rename libxl__arch_domain_configure to xl__arch_domain_init_hw_description to
better reflect its use and to be consistent with the new
libxl__arch_domain_finalise_hw_description.

The common xc_dom_build_image() function did not support explicit placement of
the ramdisk, instead passing 0 to xc_dom_alloc_segment, meaning "pick
somewhere". This change instead passes ramdisk_seg.vstart. If nothing has set
vstart then it will be zero because the entire dom struct is zeroed on
allocation in xc_dom_allocate(). Therefore there is no change to the behaviour
on x86. This is also consistent with how other segments (kernel, dtb) are
handled.

Furthermore if the ramdisk has been explicitly placed then xc_dom_build_image()
assumes that it is not to be decompressed (since that would muck up the sizings
used on placement).

With all that I'm able to boot a domain using the current Debian Jessie armhf
installer initrd and have it complete successfully.

The emacs magic in tools/libxc/xc_dom_arm.c was wrong, so I corrected it while
I was there.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@citrix.com>
---
v2: - moved comment into header and expanded/reworded a bit
    - none of the fdt_* calls in libxl__arch_domain_finalise_hw_description can
      fail, so simplify error handling using assert.

I'm considering proposing this for backport to 4.4 since lack of initrd support
is at the very least rather inconvenient for many use cases.

Also, should we consider (separate from this change) making libxc not
decompress the consider ramdisk by default? These days Linux at least can do
the decompress itself and supports more algorithmns, like xz.
---
 tools/libxc/xc_dom.h      |    8 ++++++
 tools/libxc/xc_dom_arm.c  |   65 ++++++++++++++++++++++++++++++++++----------
 tools/libxc/xc_dom_core.c |   13 ++++++---
 tools/libxl/libxl_arch.h  |   11 +++++---
 tools/libxl/libxl_arm.c   |   66 ++++++++++++++++++++++++++++++++++++++++-----
 tools/libxl/libxl_dom.c   |    8 ++++--
 tools/libxl/libxl_x86.c   |   13 ++++++---
 7 files changed, 152 insertions(+), 32 deletions(-)

diff --git a/tools/libxc/xc_dom.h b/tools/libxc/xc_dom.h
index 7099cee..313fd65 100644
--- a/tools/libxc/xc_dom.h
+++ b/tools/libxc/xc_dom.h
@@ -68,6 +68,14 @@ struct xc_dom_image {
 
     /* memory layout */
     struct xc_dom_seg kernel_seg;
+    /* If ramdisk_seg.vstart is non zero then the ramdisk will be
+     * loaded at that address, otherwise it will automatically placed.
+     *
+     * If automatic placement is used and the ramdisk is gzip
+     * compressed then it will be decompressed as it is loaded. If the
+     * ramdisk has been explicitly placed then it is loaded as is
+     * itherwise decompressing risks undoing the manual placement.
+     */
     struct xc_dom_seg ramdisk_seg;
     struct xc_dom_seg p2m_seg;
     struct xc_dom_seg pgtables_seg;
diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
index a40e04d..3330f12 100644
--- a/tools/libxc/xc_dom_arm.c
+++ b/tools/libxc/xc_dom_arm.c
@@ -249,6 +249,18 @@ int arch_setup_meminit(struct xc_dom_image *dom)
 {
     int rc;
     xen_pfn_t pfn, allocsz, i;
+    uint64_t modbase;
+
+    /* Convenient */
+    const uint64_t rambase = dom->rambase_pfn << XC_PAGE_SHIFT;
+    const uint64_t ramend = rambase + ( dom->total_pages << XC_PAGE_SHIFT );
+    const uint64_t kernend = ROUNDUP(dom->kernel_seg.vend, 21/*2MB*/);
+    const uint64_t dtb_size = dom->devicetree_blob ?
+        ROUNDUP(dom->devicetree_size, XC_PAGE_SHIFT) : 0;
+    const uint64_t ramdisk_size = dom->ramdisk_blob ?
+        ROUNDUP(dom->ramdisk_size, XC_PAGE_SHIFT) : 0;
+    const uint64_t modsize = dtb_size + ramdisk_size;
+    const uint64_t ram128mb = rambase + (128<<20);
 
     rc = set_mode(dom->xch, dom->guest_domid, dom->guest_type);
     if ( rc )
@@ -278,23 +290,49 @@ int arch_setup_meminit(struct xc_dom_image *dom)
             0, 0, &dom->p2m_host[i]);
     }
 
-    if ( dom->devicetree_blob )
+
+    /*
+     * 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.
+     */
+    if ( ramend >= ram128mb + modsize && kernend < ram128mb )
+        modbase = ram128mb;
+    else if ( ramend >= kernend + modsize )
+        modbase = kernend;
+    else
+        return -1;
+
+    DOMPRINTF("%s: placing boot modules at 0x%" PRIx64, __FUNCTION__, modbase);
+
+    /*
+     * Must map DTB *after* initrd, to satisfy order of calls to
+     * xc_dom_alloc_segment in xc_dom_build_image, which must map
+     * things at monotonolically increasing addresses.
+     */
+    if ( ramdisk_size )
     {
-        const uint64_t rambase = dom->rambase_pfn << XC_PAGE_SHIFT;
-        const uint64_t ramend = rambase + ( dom->total_pages << XC_PAGE_SHIFT );
-        const uint64_t dtbsize = ROUNDUP(dom->devicetree_size, XC_PAGE_SHIFT);
-
-        /* Place at 128MB if there is sufficient RAM */
-        if ( ramend >= rambase + 128*1024*1024 + dtbsize )
-            dom->devicetree_seg.vstart = rambase + 128*1024*1024;
-        else /* otherwise at top of RAM */
-            dom->devicetree_seg.vstart = ramend - dtbsize;
-
-        dom->devicetree_seg.vend =
-            dom->devicetree_seg.vstart + dom->devicetree_size;
+        dom->ramdisk_seg.vstart = modbase;
+        dom->ramdisk_seg.vend = modbase + ramdisk_size;
+
+        DOMPRINTF("%s: ramdisk: 0x%" PRIx64 " -> 0x%" PRIx64 "",
+                  __FUNCTION__,
+                  dom->ramdisk_seg.vstart, dom->ramdisk_seg.vend);
+
+        modbase += ramdisk_size;
+    }
+
+    if ( dtb_size )
+    {
+        dom->devicetree_seg.vstart = modbase;
+        dom->devicetree_seg.vend = modbase + dtb_size;
+
         DOMPRINTF("%s: devicetree: 0x%" PRIx64 " -> 0x%" PRIx64 "",
                   __FUNCTION__,
                   dom->devicetree_seg.vstart, dom->devicetree_seg.vend);
+
+        modbase += dtb_size;
     }
 
     return 0;
@@ -326,7 +364,6 @@ int xc_dom_feature_translated(struct xc_dom_image *dom)
  * mode: C
  * c-file-style: "BSD"
  * c-basic-offset: 4
- * tab-width: 4
  * indent-tabs-mode: nil
  * End:
  */
diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
index b9d1015..baa62a1 100644
--- a/tools/libxc/xc_dom_core.c
+++ b/tools/libxc/xc_dom_core.c
@@ -955,13 +955,20 @@ int xc_dom_build_image(struct xc_dom_image *dom)
         size_t unziplen, ramdisklen;
         void *ramdiskmap;
 
-        unziplen = xc_dom_check_gzip(dom->xch, dom->ramdisk_blob, dom->ramdisk_size);
-        if ( xc_dom_ramdisk_check_size(dom, unziplen) != 0 )
+        if ( !dom->ramdisk_seg.vstart )
+        {
+            unziplen = xc_dom_check_gzip(dom->xch,
+                                         dom->ramdisk_blob, dom->ramdisk_size);
+            if ( xc_dom_ramdisk_check_size(dom, unziplen) != 0 )
+                unziplen = 0;
+        }
+        else
             unziplen = 0;
 
         ramdisklen = unziplen ? unziplen : dom->ramdisk_size;
 
-        if ( xc_dom_alloc_segment(dom, &dom->ramdisk_seg, "ramdisk", 0,
+        if ( xc_dom_alloc_segment(dom, &dom->ramdisk_seg, "ramdisk",
+                                  dom->ramdisk_seg.vstart,
                                   ramdisklen) != 0 )
             goto err;
         ramdiskmap = xc_dom_seg_to_ptr(dom, &dom->ramdisk_seg);
diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
index aee0a91..d3bc136 100644
--- a/tools/libxl/libxl_arch.h
+++ b/tools/libxl/libxl_arch.h
@@ -19,7 +19,12 @@
 int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
                uint32_t domid);
 
-int libxl__arch_domain_configure(libxl__gc *gc,
-                                 libxl_domain_build_info *info,
-                                 struct xc_dom_image *dom);
+/* setup arch specific hardware description, i.e. DTB on ARM */
+int libxl__arch_domain_init_hw_description(libxl__gc *gc,
+                                           libxl_domain_build_info *info,
+                                           struct xc_dom_image *dom);
+/* finalize arch specific hardware description. */
+int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
+                                      libxl_domain_build_info *info,
+                                      struct xc_dom_image *dom);
 #endif
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index 0cfd0cf..4f0f0e2 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -2,6 +2,7 @@
 #include "libxl_arch.h"
 
 #include <xc_dom.h>
+#include <stdbool.h>
 #include <libfdt.h>
 #include <assert.h>
 
@@ -31,6 +32,9 @@ typedef be32 gic_interrupt[3];
 #define ROOT_ADDRESS_CELLS 2
 #define ROOT_SIZE_CELLS 2
 
+#define PROP_INITRD_START "linux,initrd-start"
+#define PROP_INITRD_END "linux,initrd-end"
+
 static void set_cell(be32 **cellp, int size, uint64_t val)
 {
     int cells = size;
@@ -155,7 +159,7 @@ static int make_root_properties(libxl__gc *gc,
     return 0;
 }
 
-static int make_chosen_node(libxl__gc *gc, void *fdt,
+static int make_chosen_node(libxl__gc *gc, void *fdt, bool ramdisk,
                             const libxl_domain_build_info *info)
 {
     int res;
@@ -169,6 +173,15 @@ static int make_chosen_node(libxl__gc *gc, void *fdt,
         if (res) return res;
     }
 
+    if (ramdisk) {
+        uint64_t dummy = 0;
+        LOG(DEBUG, "/chosen adding placeholder linux,initrd properties");
+        res = fdt_property(fdt, PROP_INITRD_START, &dummy, sizeof(dummy));
+        if (res) return res;
+        res = fdt_property(fdt, PROP_INITRD_END, &dummy, sizeof(dummy));
+        if (res) return res;
+    }
+
     res = fdt_end_node(fdt);
     if (res) return res;
 
@@ -412,9 +425,9 @@ out:
 
 #define FDT_MAX_SIZE (1<<20)
 
-int libxl__arch_domain_configure(libxl__gc *gc,
-                                 libxl_domain_build_info *info,
-                                 struct xc_dom_image *dom)
+int libxl__arch_domain_init_hw_description(libxl__gc *gc,
+                                           libxl_domain_build_info *info,
+                                           struct xc_dom_image *dom)
 {
     void *fdt = NULL;
     int rc, res;
@@ -475,7 +488,7 @@ next_resize:
         FDT( fdt_begin_node(fdt, "") );
 
         FDT( make_root_properties(gc, vers, fdt) );
-        FDT( make_chosen_node(gc, fdt, info) );
+        FDT( make_chosen_node(gc, fdt, !!dom->ramdisk_blob, info) );
         FDT( make_cpus_node(gc, fdt, info->max_vcpus, ainfo) );
         FDT( make_psci_node(gc, fdt) );
 
@@ -505,10 +518,49 @@ next_resize:
         goto out;
     }
 
-    debug_dump_fdt(gc, fdt);
-
     rc = 0;
 
 out:
     return rc;
 }
+
+int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
+                                               libxl_domain_build_info *info,
+                                               struct xc_dom_image *dom)
+{
+    void *fdt = dom->devicetree_blob;
+
+    const struct xc_dom_seg *ramdisk = dom->ramdisk_blob ?
+        &dom->ramdisk_seg : NULL;
+
+    if (ramdisk) {
+        int chosen, res;
+        uint64_t val;
+
+        /* Neither the fdt_path_offset() nor either of the
+         * fdt_setprop_inplace() calls can fail. If they do then
+         * make_chosen_node() (see above) has got something very
+         * wrong.
+         */
+        chosen = fdt_path_offset(fdt, "/chosen");
+        assert(chosen > 0);
+
+        LOG(DEBUG, "/chosen updating initrd properties to cover "
+            "%"PRIx64"-%"PRIx64,
+            ramdisk->vstart, ramdisk->vend);
+
+        val = cpu_to_fdt64(ramdisk->vstart);
+        res = fdt_setprop_inplace(fdt, chosen, PROP_INITRD_START,
+                                  &val, sizeof(val));
+        assert(!res);
+
+        val = cpu_to_fdt64(ramdisk->vend);
+        res = fdt_setprop_inplace(fdt, chosen,PROP_INITRD_END,
+                                  &val, sizeof(val));
+        assert(!res);
+    }
+
+    debug_dump_fdt(gc, fdt);
+
+    return 0;
+}
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 36e70b5..c3fa439 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -407,8 +407,8 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
         LOGE(ERROR, "xc_dom_parse_image failed");
         goto out;
     }
-    if ( (ret = libxl__arch_domain_configure(gc, info, dom)) != 0 ) {
-        LOGE(ERROR, "libxl__arch_domain_configure failed");
+    if ( (ret = libxl__arch_domain_init_hw_description(gc, info, dom)) != 0 ) {
+        LOGE(ERROR, "libxl__arch_domain_init_hw_description failed");
         goto out;
     }
     if ( (ret = xc_dom_mem_init(dom, info->target_memkb / 1024)) != 0 ) {
@@ -419,6 +419,10 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
         LOGE(ERROR, "xc_dom_boot_mem_init failed");
         goto out;
     }
+    if ( (ret = libxl__arch_domain_finalise_hw_description(gc, info, dom)) != 0 ) {
+        LOGE(ERROR, "libxl__arch_domain_finalise_hw_description failed");
+        goto out;
+    }
     if ( (ret = xc_dom_build_image(dom)) != 0 ) {
         LOGE(ERROR, "xc_dom_build_image failed");
         goto out;
diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index b11d036..7589060 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -311,9 +311,16 @@ int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
     return ret;
 }
 
-int libxl__arch_domain_configure(libxl__gc *gc,
-                                 libxl_domain_build_info *info,
-                                 struct xc_dom_image *dom)
+int libxl__arch_domain_init_hw_description(libxl__gc *gc,
+                                           libxl_domain_build_info *info,
+                                           struct xc_dom_image *dom)
+{
+    return 0;
+}
+
+int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
+                                               libxl_domain_build_info *info,
+                                               struct xc_dom_image *dom)
 {
     return 0;
 }
-- 
1.7.10.4

Re: [PATCH v2] tools: implement initial ramdisk support for ARM.

[Ian Jackson]

Ian Campbell writes ("[PATCH v2] tools: implement initial ramdisk support for ARM."):
> The ramdisk is passed to the kernel as a property in the chosen node
> of the device tree. This is somewhat tricky since in order to place
> the ramdisk and dtb in ram we first need to know the size of the
> dtb. So we initially create a DTB with placeholders for the ramdisk
> and finalise the value (which doesn't change the size) once we know
> where everything is.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

I'll leave you to push it (perhaps after giving time for others to
comment on v2).

> I'm considering proposing this for backport to 4.4 since lack of
> initrd support is at the very least rather inconvenient for many use
> cases.

I think this is a viable backport candidate.  Can you let me know when
it's in-tree and I'll add it to my list.

> Also, should we consider (separate from this change) making libxc not
> decompress the consider ramdisk by default? These days Linux at least can do
> the decompress itself and supports more algorithmns, like xz.

DYK whether Linux has ever not been able to decompress itself - and if
so when did this change ?  It would be annoying to break some old
installations.

Looking at the code, it might be a good idea to at least pass unknown
compression formats on to the kernel, rather than failing as we do
now.

Thanks,
Ian.

Re: [PATCH v2] tools: implement initial ramdisk support for ARM.

[Ian Campbell]

On Fri, 2014-04-04 at 16:00 +0100, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH v2] tools: implement initial ramdisk support for ARM."):
> > The ramdisk is passed to the kernel as a property in the chosen node
> > of the device tree. This is somewhat tricky since in order to place
> > the ramdisk and dtb in ram we first need to know the size of the
> > dtb. So we initially create a DTB with placeholders for the ramdisk
> > and finalise the value (which doesn't change the size) once we know
> > where everything is.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> I'll leave you to push it (perhaps after giving time for others to
> comment on v2).

Thanks, I'll leave it until next week.

> > I'm considering proposing this for backport to 4.4 since lack of
> > initrd support is at the very least rather inconvenient for many use
> > cases.
> 
> I think this is a viable backport candidate.  Can you let me know when
> it's in-tree and I'll add it to my list.

Sure. Or since I'm supposed to be doing backport stuff for ARM I could
pick this one up myself.

> > Also, should we consider (separate from this change) making libxc not
> > decompress the consider ramdisk by default? These days Linux at least can do
> > the decompress itself and supports more algorithmns, like xz.
> 
> DYK whether Linux has ever not been able to decompress itself - and if
> so when did this change ?  It would be annoying to break some old
> installations.

By "itself" ITYM "its ramdisk". The answer there is that I don't know.

If you did mean "itself" then the answer is that it is moot because all
of the kernel's self decompression stuff is in early real (16-bit) mode
which we skip when booting a PV kernel -- so we definitely do have to
decompress the kernel image. Hence the trickle of bzip2, xz, lzma
patches to Xen, libxc and stubdoms.

> Looking at the code, it might be a good idea to at least pass unknown
> compression formats on to the kernel, rather than failing as we do
> now.

Unless I'm misreading that's what we do.

If either xc_dom_check_gzip or xc_dom_ramdisk_check_size fail then we
set unziplen to 0 which indicates "do not decompress" to the remainder
of that code block.

Ian.

Re: [PATCH v2] tools: implement initial ramdisk support for ARM.

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH v2] tools: implement initial ramdisk support for ARM."):
> On Fri, 2014-04-04 at 16:00 +0100, Ian Jackson wrote:
> > DYK whether Linux has ever not been able to decompress itself - and if
> > so when did this change ?  It would be annoying to break some old
> > installations.
> 
> By "itself" ITYM "its ramdisk". The answer there is that I don't know.

Yes, that's what I meant.

> If you did mean "itself" then the answer is that it is moot because all
> of the kernel's self decompression stuff is in early real (16-bit) mode
> which we skip when booting a PV kernel -- so we definitely do have to
> decompress the kernel image. Hence the trickle of bzip2, xz, lzma
> patches to Xen, libxc and stubdoms.

Right, yes, I knew that.  By "itself" I meant "the kernel is
decompresssing the ramdisk (rather than expecting the loader to do
it)".

> > Looking at the code, it might be a good idea to at least pass unknown
> > compression formats on to the kernel, rather than failing as we do
> > now.
> 
> Unless I'm misreading that's what we do.
> 
> If either xc_dom_check_gzip or xc_dom_ramdisk_check_size fail then we
> set unziplen to 0 which indicates "do not decompress" to the remainder
> of that code block.

Oh yes, inded.

I foolishly assumed that since we had a function
xc_dom_probe_bzimage_kernel (which dispatches to a variety of
decompressors and aborted if it got a compressed image with a magic it
doesn't understand) that this function was used for ramdisks too (and
hence, wasn't quite accurately named).  But I see we have a different
ad hoc arrangement for ramdisks.

Hrm.  Oh well.  I guess this is good enough, then.  We won't fail
on new compression formats (if they're only used for the ramdisk) and
we will work with putative old kernels that don't know how to
uncompress ramdisks.

Ian.

Re: [PATCH v2] tools: implement initial ramdisk support for ARM.

[Ian Campbell]

On Fri, 2014-04-04 at 16:00 +0100, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH v2] tools: implement initial ramdisk support for ARM."):
> > The ramdisk is passed to the kernel as a property in the chosen node
> > of the device tree. This is somewhat tricky since in order to place
> > the ramdisk and dtb in ram we first need to know the size of the
> > dtb. So we initially create a DTB with placeholders for the ramdisk
> > and finalise the value (which doesn't change the size) once we know
> > where everything is.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> I'll leave you to push it (perhaps after giving time for others to
> comment on v2).
> 
> > I'm considering proposing this for backport to 4.4 since lack of
> > initrd support is at the very least rather inconvenient for many use
> > cases.
> 
> I think this is a viable backport candidate.  Can you let me know when
> it's in-tree and I'll add it to my list

It's in now, commit 314c9815e2f5dc8a9fec11e0cf9b49b16ed0e96b

Ian.

Re: [PATCH v2] tools: implement initial ramdisk support for ARM.

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH v2] tools: implement initial ramdisk support for ARM."):
> On Fri, 2014-04-04 at 16:00 +0100, Ian Jackson wrote:
> > I think this is a viable backport candidate.  Can you let me know when
> > it's in-tree and I'll add it to my list
> 
> It's in now, commit 314c9815e2f5dc8a9fec11e0cf9b49b16ed0e96b

Noted, thanks.

Ian.

Re: [PATCH v2] tools: implement initial ramdisk support for ARM.

[Julien Grall]

Hi Ian & Ian,

On 04/08/2014 05:53 PM, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH v2] tools: implement initial ramdisk support for ARM."):
>> On Fri, 2014-04-04 at 16:00 +0100, Ian Jackson wrote:
>>> I think this is a viable backport candidate.  Can you let me know when
>>> it's in-tree and I'll add it to my list
>>
>> It's in now, commit 314c9815e2f5dc8a9fec11e0cf9b49b16ed0e96b
> 
> Noted, thanks.

I'm unable to boot a guest with this patch on Xen 4.5. Revert it works
correctly. The guest is blocked without useful log.

My config:

kernel="/root/zImage"
memory=128
name="test"
vcpus=2
autoballon="off"
root="/dev/xvda"
extra="console=hvc0"
disk=[ 'phy:/dev/loop0,xvda,w' ]

-- 
Julien Grall

Re: [PATCH v2] tools: implement initial ramdisk support for ARM.

[Julien Grall]

On 04/08/2014 06:29 PM, Julien Grall wrote:
> Hi Ian & Ian,
> 
> On 04/08/2014 05:53 PM, Ian Jackson wrote:
>> Ian Campbell writes ("Re: [PATCH v2] tools: implement initial ramdisk support for ARM."):
>>> On Fri, 2014-04-04 at 16:00 +0100, Ian Jackson wrote:
>>>> I think this is a viable backport candidate.  Can you let me know when
>>>> it's in-tree and I'll add it to my list
>>>
>>> It's in now, commit 314c9815e2f5dc8a9fec11e0cf9b49b16ed0e96b
>>
>> Noted, thanks.
> 
> I'm unable to boot a guest with this patch on Xen 4.5. Revert it works
> correctly. The guest is blocked without useful log.
> 
> My config:
> 
> kernel="/root/zImage"
> memory=128
> name="test"
> vcpus=2
> autoballon="off"
> root="/dev/xvda"
> extra="console=hvc0"
> disk=[ 'phy:/dev/loop0,xvda,w' ]
> 

This small changes fix boot of the guest with RAM < 128Mb:

diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
index f051515..2228ba5 100644
--- a/tools/libxc/xc_dom_arm.c
+++ b/tools/libxc/xc_dom_arm.c
@@ -300,7 +300,7 @@ int arch_setup_meminit(struct xc_dom_image *dom)
     if ( ramend >= ram128mb + modsize && kernend < ram128mb )
         modbase = ram128mb;
     else if ( ramend >= kernend + modsize )
-        modbase = kernend;
+        modbase = ramend - modsize;
     else
         return -1;

I guess this is because the kernel is extracting on it. I think we
should follow the same "algorithm" as Xen (see place_modules) to decide
where the modules should be loaded.

Regards,

-- 
Julien Grall

Re: [PATCH v2] tools: implement initial ramdisk support for ARM.

[Ian Campbell]

On Tue, 2014-04-08 at 19:20 +0100, Julien Grall wrote:
> On 04/08/2014 06:29 PM, Julien Grall wrote:
> > Hi Ian & Ian,
> > 
> > On 04/08/2014 05:53 PM, Ian Jackson wrote:
> >> Ian Campbell writes ("Re: [PATCH v2] tools: implement initial ramdisk support for ARM."):
> >>> On Fri, 2014-04-04 at 16:00 +0100, Ian Jackson wrote:
> >>>> I think this is a viable backport candidate.  Can you let me know when
> >>>> it's in-tree and I'll add it to my list
> >>>
> >>> It's in now, commit 314c9815e2f5dc8a9fec11e0cf9b49b16ed0e96b
> >>
> >> Noted, thanks.
> > 
> > I'm unable to boot a guest with this patch on Xen 4.5. Revert it works
> > correctly. The guest is blocked without useful log.
> > 
> > My config:
> > 
> > kernel="/root/zImage"
> > memory=128
> > name="test"
> > vcpus=2
> > autoballon="off"
> > root="/dev/xvda"
> > extra="console=hvc0"
> > disk=[ 'phy:/dev/loop0,xvda,w' ]
> > 
> 
> This small changes fix boot of the guest with RAM < 128Mb:
> 
> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
> index f051515..2228ba5 100644
> --- a/tools/libxc/xc_dom_arm.c
> +++ b/tools/libxc/xc_dom_arm.c
> @@ -300,7 +300,7 @@ int arch_setup_meminit(struct xc_dom_image *dom)
>      if ( ramend >= ram128mb + modsize && kernend < ram128mb )
>          modbase = ram128mb;
>      else if ( ramend >= kernend + modsize )
> -        modbase = kernend;
> +        modbase = ramend - modsize;
>      else
>          return -1;
> 
> I guess this is because the kernel is extracting on it. I think we
> should follow the same "algorithm" as Xen (see place_modules) to decide
> where the modules should be loaded.

Yes, this fix is correct. The existing code is just bogus, it makes no
sense to place the modules exactly at the end of RAM since they will
spin over the end.

Can you resubmit with a changelog and an S-o-b please.

Ian.

Re: [PATCH v2] tools: implement initial ramdisk support for ARM.

[Julien Grall]

On 09/04/14 09:12, Ian Campbell wrote:
>> This small changes fix boot of the guest with RAM < 128Mb:
>>
>> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
>> index f051515..2228ba5 100644
>> --- a/tools/libxc/xc_dom_arm.c
>> +++ b/tools/libxc/xc_dom_arm.c
>> @@ -300,7 +300,7 @@ int arch_setup_meminit(struct xc_dom_image *dom)
>>       if ( ramend >= ram128mb + modsize && kernend < ram128mb )
>>           modbase = ram128mb;
>>       else if ( ramend >= kernend + modsize )
>> -        modbase = kernend;
>> +        modbase = ramend - modsize;
>>       else
>>           return -1;
>>
>> I guess this is because the kernel is extracting on it. I think we
>> should follow the same "algorithm" as Xen (see place_modules) to decide
>> where the modules should be loaded.
>
> Yes, this fix is correct. The existing code is just bogus, it makes no
> sense to place the modules exactly at the end of RAM since they will
> spin over the end.

I'm not sure to understand why you are talking about RAM... currently 
the modules are placed just after the kernel (which in this case is 
always before ramend).

> Can you resubmit with a changelog and an S-o-b please.

Sure.

Regards,

-- 
Julien Grall

Re: [PATCH v2] tools: implement initial ramdisk support for ARM.

[Ian Campbell]

On Wed, 2014-04-09 at 09:17 +0100, Julien Grall wrote:
> 
> On 09/04/14 09:12, Ian Campbell wrote:
> >> This small changes fix boot of the guest with RAM < 128Mb:
> >>
> >> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
> >> index f051515..2228ba5 100644
> >> --- a/tools/libxc/xc_dom_arm.c
> >> +++ b/tools/libxc/xc_dom_arm.c
> >> @@ -300,7 +300,7 @@ int arch_setup_meminit(struct xc_dom_image *dom)
> >>       if ( ramend >= ram128mb + modsize && kernend < ram128mb )
> >>           modbase = ram128mb;
> >>       else if ( ramend >= kernend + modsize )
> >> -        modbase = kernend;
> >> +        modbase = ramend - modsize;
> >>       else
> >>           return -1;
> >>
> >> I guess this is because the kernel is extracting on it. I think we
> >> should follow the same "algorithm" as Xen (see place_modules) to decide
> >> where the modules should be loaded.
> >
> > Yes, this fix is correct. The existing code is just bogus, it makes no
> > sense to place the modules exactly at the end of RAM since they will
> > spin over the end.
> 
> I'm not sure to understand why you are talking about RAM... currently 
> the modules are placed just after the kernel (which in this case is 
> always before ramend).

I misread the change.

I think the current code matches the dom0 place_modules call (modulo the
condition being expressed differently). Perhaps that case is also wrong?

> 
> > Can you resubmit with a changelog and an S-o-b please.
> 
> Sure.
> 
> Regards,
> 

Re: [PATCH v2] tools: implement initial ramdisk support for ARM.

[Julien Grall]

On 09/04/14 09:37, Ian Campbell wrote:
> On Wed, 2014-04-09 at 09:17 +0100, Julien Grall wrote:
>>
>> On 09/04/14 09:12, Ian Campbell wrote:
>>>> This small changes fix boot of the guest with RAM < 128Mb:
>>>>
>>>> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
>>>> index f051515..2228ba5 100644
>>>> --- a/tools/libxc/xc_dom_arm.c
>>>> +++ b/tools/libxc/xc_dom_arm.c
>>>> @@ -300,7 +300,7 @@ int arch_setup_meminit(struct xc_dom_image *dom)
>>>>        if ( ramend >= ram128mb + modsize && kernend < ram128mb )
>>>>            modbase = ram128mb;
>>>>        else if ( ramend >= kernend + modsize )
>>>> -        modbase = kernend;
>>>> +        modbase = ramend - modsize;
>>>>        else
>>>>            return -1;
>>>>
>>>> I guess this is because the kernel is extracting on it. I think we
>>>> should follow the same "algorithm" as Xen (see place_modules) to decide
>>>> where the modules should be loaded.
>>>
>>> Yes, this fix is correct. The existing code is just bogus, it makes no
>>> sense to place the modules exactly at the end of RAM since they will
>>> spin over the end.
>>
>> I'm not sure to understand why you are talking about RAM... currently
>> the modules are placed just after the kernel (which in this case is
>> always before ramend).
>
> I misread the change.
>
> I think the current code matches the dom0 place_modules call (modulo the
> condition being expressed differently). Perhaps that case is also wrong?

The 2 if of libxc  express the first if of dom0 place_modules, right?

If dom0 as less than 128MB of RAM, we are trying to load the modules as 
high as possible.

In case of the guest, we are loading them just after kernel. But ... the 
kernel will likely decompress on it.

-- 
Julien Grall

Re: [PATCH v2] tools: implement initial ramdisk support for ARM.

[Ian Campbell]

On Wed, 2014-04-09 at 10:10 +0100, Julien Grall wrote:
> 
> On 09/04/14 09:37, Ian Campbell wrote:
> > On Wed, 2014-04-09 at 09:17 +0100, Julien Grall wrote:
> >>
> >> On 09/04/14 09:12, Ian Campbell wrote:
> >>>> This small changes fix boot of the guest with RAM < 128Mb:
> >>>>
> >>>> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
> >>>> index f051515..2228ba5 100644
> >>>> --- a/tools/libxc/xc_dom_arm.c
> >>>> +++ b/tools/libxc/xc_dom_arm.c
> >>>> @@ -300,7 +300,7 @@ int arch_setup_meminit(struct xc_dom_image *dom)
> >>>>        if ( ramend >= ram128mb + modsize && kernend < ram128mb )
> >>>>            modbase = ram128mb;
> >>>>        else if ( ramend >= kernend + modsize )
> >>>> -        modbase = kernend;
> >>>> +        modbase = ramend - modsize;
> >>>>        else
> >>>>            return -1;
> >>>>
> >>>> I guess this is because the kernel is extracting on it. I think we
> >>>> should follow the same "algorithm" as Xen (see place_modules) to decide
> >>>> where the modules should be loaded.
> >>>
> >>> Yes, this fix is correct. The existing code is just bogus, it makes no
> >>> sense to place the modules exactly at the end of RAM since they will
> >>> spin over the end.
> >>
> >> I'm not sure to understand why you are talking about RAM... currently
> >> the modules are placed just after the kernel (which in this case is
> >> always before ramend).
> >
> > I misread the change.
> >
> > I think the current code matches the dom0 place_modules call (modulo the
> > condition being expressed differently). Perhaps that case is also wrong?
> 
> The 2 if of libxc  express the first if of dom0 place_modules, right?
> 
> If dom0 as less than 128MB of RAM, we are trying to load the modules as 
> high as possible.

In Xen we have:
    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;
    else
    {
        panic("Unable to find suitable location for dtb+initrd");

In libxc we have:
    if ( ramend >= ram128mb + modsize && kernend < ram128mb )
        modbase = ram128mb;
    else if ( ramend >= kernend + modsize )
        modbase = kernend;
    else
        return -1;

I think the second if and assignments are equivalent. I think the
problem comes from the differences in the first if which are in fact
different. They also differ in that userspace won't try and place the
modules before the kernel as a last resort.

Switching the Xen side to use the  libxc variable names makes it less
confusing to compare IMO:

    if ( kernend < MIN(ram128mb, ramend - modsize) )
        addr = MIN(ram128mb, ramend - modsize);
    else if ( ramend - kernend >= modsize )
        addr = kernend;
    else if ( kernel_start - mem_start >= total )
        addr = kernel_start - total;
    else
    {
        panic("Unable to find suitable location for dtb+initrd");

I think we need to use that MIN(ram128mb, ramend - modsize) calculation
in libxc too.

I'm also wondering if the second statement actually make sense with that
change. If kernelend >= ramend - modsize (so the first test fails) then 
I'm not convinced that ramend - kernend >= modsize can ever be true.

Ian.

Re: [PATCH v2] tools: implement initial ramdisk support for ARM.

[Ian Campbell]

On Tue, 2014-04-08 at 17:53 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH v2] tools: implement initial ramdisk support for ARM."):
> > On Fri, 2014-04-04 at 16:00 +0100, Ian Jackson wrote:
> > > I think this is a viable backport candidate.  Can you let me know when
> > > it's in-tree and I'll add it to my list
> > 
> > It's in now, commit 314c9815e2f5dc8a9fec11e0cf9b49b16ed0e96b
> 
> Noted, thanks.

I thought I'd mentioned this but I don't see my mail:

You will also need to backport a fix at the same time:
6f4ff74 tools: arm: improve placement of initial modules.

Re: [PATCH v2] tools: implement initial ramdisk support for ARM.

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH v2] tools: implement initial ramdisk support for ARM."):
> On Tue, 2014-04-08 at 17:53 +0100, Ian Jackson wrote:
> > Ian Campbell writes ("Re: [PATCH v2] tools: implement initial ramdisk support for ARM."):
> > > On Fri, 2014-04-04 at 16:00 +0100, Ian Jackson wrote:
> > > > I think this is a viable backport candidate.  Can you let me know when
> > > > it's in-tree and I'll add it to my list
> > > 
> > > It's in now, commit 314c9815e2f5dc8a9fec11e0cf9b49b16ed0e96b
> > 
> > Noted, thanks.
> 
> I thought I'd mentioned this but I don't see my mail:
> 
> You will also need to backport a fix at the same time:
> 6f4ff74 tools: arm: improve placement of initial modules.

Thanks.  That seems to be on my list already.

Ian.

Re: [PATCH v2] tools: implement initial ramdisk support for ARM.

[Ian Jackson]

Ian Jackson writes ("Re: [PATCH v2] tools: implement initial ramdisk support for ARM."):
> Ian Campbell writes ("Re: [PATCH v2] tools: implement initial ramdisk support for ARM."):
> > You will also need to backport a fix at the same time:
> > 6f4ff74 tools: arm: improve placement of initial modules.
> 
> Thanks.  That seems to be on my list already.

Backported to 4.4.

Ian.

Re: [PATCH v2] tools: implement initial ramdisk support for ARM.

[Julien Grall]

Hi Ian,

On 04/04/2014 02:28 PM, Ian Campbell wrote:
>      struct xc_dom_seg kernel_seg;
> +    /* If ramdisk_seg.vstart is non zero then the ramdisk will be
> +     * loaded at that address, otherwise it will automatically placed.
> +     *
> +     * If automatic placement is used and the ramdisk is gzip
> +     * compressed then it will be decompressed as it is loaded. If the
> +     * ramdisk has been explicitly placed then it is loaded as is
> +     * itherwise decompressing risks undoing the manual placement.

otherwise?

[..]

> @@ -326,7 +364,6 @@ int xc_dom_feature_translated(struct xc_dom_image *dom)
>   * mode: C
>   * c-file-style: "BSD"
>   * c-basic-offset: 4
> - * tab-width: 4

Does it mean that nearly every emacs magic are wrong on the other files?

[..]

> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 36e70b5..c3fa439 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -407,8 +407,8 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
>          LOGE(ERROR, "xc_dom_parse_image failed");
>          goto out;
>      }
> -    if ( (ret = libxl__arch_domain_configure(gc, info, dom)) != 0 ) {
> -        LOGE(ERROR, "libxl__arch_domain_configure failed");
> +    if ( (ret = libxl__arch_domain_init_hw_description(gc, info, dom)) != 0 ) {

Should not it be ((..))? It seems the function is using 2 different
coding style.

-- 
Julien Grall

Re: [PATCH v2] tools: implement initial ramdisk support for ARM.

[Ian Campbell]

On Fri, 2014-04-04 at 16:16 +0100, Julien Grall wrote:
> > @@ -326,7 +364,6 @@ int xc_dom_feature_translated(struct xc_dom_image *dom)
> >   * mode: C
> >   * c-file-style: "BSD"
> >   * c-basic-offset: 4
> > - * tab-width: 4
> 
> Does it mean that nearly every emacs magic are wrong on the other files?

I thought there weren't many others, but looking again I'm wrong. I
think I may have been thinking of "indent-tabs-mode: nil".

I think my emacs must have been playing up when I first edited this file
and I confused myself into thinking this change had fixed it.

> > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> > index 36e70b5..c3fa439 100644
> > --- a/tools/libxl/libxl_dom.c
> > +++ b/tools/libxl/libxl_dom.c
> > @@ -407,8 +407,8 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
> >          LOGE(ERROR, "xc_dom_parse_image failed");
> >          goto out;
> >      }
> > -    if ( (ret = libxl__arch_domain_configure(gc, info, dom)) != 0 ) {
> > -        LOGE(ERROR, "libxl__arch_domain_configure failed");
> > +    if ( (ret = libxl__arch_domain_init_hw_description(gc, info, dom)) != 0 ) {
> 
> Should not it be ((..))? It seems the function is using 2 different
> coding style.

Looks like some Xen list braces have slipped in here. Probably best for
this new addition to be consistent with the surroundings though.

Ian.

Re: [PATCH v2] tools: implement initial ramdisk support for ARM.

[Julien Grall]

On 04/04/2014 04:20 PM, Ian Campbell wrote:
> On Fri, 2014-04-04 at 16:16 +0100, Julien Grall wrote:
>>> @@ -326,7 +364,6 @@ int xc_dom_feature_translated(struct xc_dom_image *dom)
>>>   * mode: C
>>>   * c-file-style: "BSD"
>>>   * c-basic-offset: 4
>>> - * tab-width: 4
>>
>> Does it mean that nearly every emacs magic are wrong on the other files?
> 
> I thought there weren't many others, but looking again I'm wrong. I
> think I may have been thinking of "indent-tabs-mode: nil".
> 
> I think my emacs must have been playing up when I first edited this file
> and I confused myself into thinking this change had fixed it.
> 
>>> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
>>> index 36e70b5..c3fa439 100644
>>> --- a/tools/libxl/libxl_dom.c
>>> +++ b/tools/libxl/libxl_dom.c
>>> @@ -407,8 +407,8 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
>>>          LOGE(ERROR, "xc_dom_parse_image failed");
>>>          goto out;
>>>      }
>>> -    if ( (ret = libxl__arch_domain_configure(gc, info, dom)) != 0 ) {
>>> -        LOGE(ERROR, "libxl__arch_domain_configure failed");
>>> +    if ( (ret = libxl__arch_domain_init_hw_description(gc, info, dom)) != 0 ) {
>>
>> Should not it be ((..))? It seems the function is using 2 different
>> coding style.
> 
> Looks like some Xen list braces have slipped in here. Probably best for
> this new addition to be consistent with the surroundings though.

Ok. I forgot to give my ack on this patch (with the emacs fix)

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

-- 
Julien Grall

Re: [PATCH v2] tools: implement initial ramdisk support for ARM.

[Ian Campbell]

On Fri, 2014-04-04 at 16:23 +0100, Julien Grall wrote:
> Ok. I forgot to give my ack on this patch (with the emacs fix)
> 
> Acked-by: Julien Grall <julien.grall@linaro.org>

Applied with the spelling fix, dropping the emacs thing.

Thanks.
Ian.