diff mbox

[Xen-devel,v4,7/9] tools: arm: prepare guest FDT building for multiple RAM banks

Message ID 1399980574-12515-7-git-send-email-ian.campbell@citrix.com
State New
Headers show

Commit Message

Ian Campbell May 13, 2014, 11:29 a.m. UTC 
This required exposing the sizes of the banks determined by the domain builder
up to libxl via xc_dom_image.

Since the domain build needs to know the size of the DTB we create placeholder
nodes for each possible bank and when we finialise the DTB we fill in the ones
which are actually populated and NOP out the rest.

Note that the number of guest RAM banks is still 1 after this change.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v4: New patch
---
 tools/libxc/xc_dom.h     |   10 +++++-
 tools/libxc/xc_dom_arm.c |   12 +++-----
 tools/libxl/libxl_arm.c  |   77 ++++++++++++++++++++++++++++++++++++----------
 3 files changed, 74 insertions(+), 25 deletions(-)

Comments

Julien Grall May 13, 2014, 12:35 p.m. UTC | #1 
Hi Ian,

On 05/13/2014 12:29 PM, Ian Campbell wrote:
> This required exposing the sizes of the banks determined by the domain builder
> up to libxl via xc_dom_image.
> 
> Since the domain build needs to know the size of the DTB we create placeholder
> nodes for each possible bank and when we finialise the DTB we fill in the ones

finalise

> which are actually populated and NOP out the rest.

It's annoying that we can't dissociate bank setup (i.e defining the size
of each banks) and placing modules (kernel, DTB,...).

It would avoid to have 2 step to create memory node.

Anyway, I'm fine with this solution for now.

[..]

>  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;
> +    int i;
> +    const uint64_t bankbase[GUEST_RAM_BANKS] = {
> +        GUEST_RAM0_BASE
> +    };

You've duplicate this variable in multiple place (twice in libxl and
once in libxc).

I think it's better if we can avoid duplicating this variable, maybe by
storing in xc_dom_image?

>  
>      const struct xc_dom_seg *ramdisk = dom->ramdisk_blob ?
>          &dom->ramdisk_seg : NULL;
> @@ -552,9 +588,16 @@ int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
>          assert(!res);
>  
>          val = cpu_to_fdt64(ramdisk->vend);
> -        res = fdt_setprop_inplace(fdt, chosen,PROP_INITRD_END,
> +        res = fdt_setprop_inplace(fdt, chosen, PROP_INITRD_END,
>                                    &val, sizeof(val));

Spurious change?

Regards,
Ian Campbell May 15, 2014, 11:27 a.m. UTC | #2 
On Tue, 2014-05-13 at 13:35 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 05/13/2014 12:29 PM, Ian Campbell wrote:
> > This required exposing the sizes of the banks determined by the domain builder
> > up to libxl via xc_dom_image.
> > 
> > Since the domain build needs to know the size of the DTB we create placeholder
> > nodes for each possible bank and when we finialise the DTB we fill in the ones
> 
> finalise
> 
> > which are actually populated and NOP out the rest.
> 
> It's annoying that we can't dissociate bank setup (i.e defining the size
> of each banks) and placing modules (kernel, DTB,...).
> 
> It would avoid to have 2 step to create memory node.

We have to have 2 steps for the initrd case (that's unavoidable), given
that having something similar for the memory is not the end of the
world.

> Anyway, I'm fine with this solution for now.
> 
> [..]
> 
> >  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;
> > +    int i;
> > +    const uint64_t bankbase[GUEST_RAM_BANKS] = {
> > +        GUEST_RAM0_BASE
> > +    };
> 
> You've duplicate this variable in multiple place (twice in libxl and
> once in libxc).
> 
> I think it's better if we can avoid duplicating this variable, maybe by
> storing in xc_dom_image?

I'll take a look. Since these are currently constants I might just
#define GUEST_RAM_BASES { GUEST_RAM0_BASE , GUEST_RAM1_BASE }
So that these become const uunt64_t bankbase[] = GUEST_RAM_BASES;

> 
> >  
> >      const struct xc_dom_seg *ramdisk = dom->ramdisk_blob ?
> >          &dom->ramdisk_seg : NULL;
> > @@ -552,9 +588,16 @@ int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
> >          assert(!res);
> >  
> >          val = cpu_to_fdt64(ramdisk->vend);
> > -        res = fdt_setprop_inplace(fdt, chosen,PROP_INITRD_END,
> > +        res = fdt_setprop_inplace(fdt, chosen, PROP_INITRD_END,
> >                                    &val, sizeof(val));
> 
> Spurious change?

It was deliberate, I just spotted the coding style error while I was in
the region. Naughty to slip it in here I know. I'll mention it in the
commit log.

Ian.
diff mbox

Patch

diff --git a/tools/libxc/xc_dom.h b/tools/libxc/xc_dom.h
index c9af0ce..6ae6a9f 100644
--- a/tools/libxc/xc_dom.h
+++ b/tools/libxc/xc_dom.h
@@ -114,13 +114,21 @@  struct xc_dom_image {
 
     /* physical memory
      *
-     * A PV guest has a single contiguous block of physical RAM,
+     * An x86 PV guest has a single contiguous block of physical RAM,
      * consisting of total_pages starting at rambase_pfn.
+     *
+     * An ARM guest has GUEST_RAM_BANKS regions of RAM, with
+     * rambank_size[i] pages in each. The lowest RAM address
+     * (corresponding to the base of the p2m arrays above) is stored
+     * in rambase_pfn.
      */
     xen_pfn_t rambase_pfn;
     xen_pfn_t total_pages;
     struct xc_dom_phys *phys_pages;
     int realmodearea_log;
+#if defined (__arm__) || defined(__aarch64__)
+    xen_pfn_t rambank_size[GUEST_RAM_BANKS];
+#endif
 
     /* malloc memory pool */
     struct xc_dom_mem *memblocks;
diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
index 6c255ac..c83965d 100644
--- a/tools/libxc/xc_dom_arm.c
+++ b/tools/libxc/xc_dom_arm.c
@@ -305,7 +305,6 @@  int arch_setup_meminit(struct xc_dom_image *dom)
     const uint64_t ram128mb = GUEST_RAM0_BASE + (128<<20);
 
     xen_pfn_t p2m_size;
-    xen_pfn_t rambank_size[GUEST_RAM_BANKS];
     uint64_t bank0end;
 
     assert(dom->rambase_pfn << XC_PAGE_SHIFT == GUEST_RAM0_BASE);
@@ -345,10 +344,10 @@  int arch_setup_meminit(struct xc_dom_image *dom)
 
         p2m_size = ( bankbase[i] + banksize - bankbase[0] ) >> XC_PAGE_SHIFT;
 
-        rambank_size[i] = banksize >> XC_PAGE_SHIFT;
+        dom->rambank_size[i] = banksize >> XC_PAGE_SHIFT;
     }
 
-    assert(rambank_size[0] != 0);
+    assert(dom->rambank_size[0] != 0);
     assert(ramsize == 0); /* Too much RAM is rejected above */
 
     dom->p2m_host = xc_dom_malloc(dom, sizeof(xen_pfn_t) * p2m_size);
@@ -358,11 +357,10 @@  int arch_setup_meminit(struct xc_dom_image *dom)
         dom->p2m_host[pfn] = INVALID_MFN;
 
     /* setup initial p2m and allocate guest memory */
-    for (i = 0; rambank_size[i] && i < GUEST_RAM_BANKS; i++)
-    {
+    for (i = 0; dom->rambank_size[i] && i < GUEST_RAM_BANKS; i++) {
         if ((rc = populate_guest_memory(dom,
                                         bankbase[i] >> XC_PAGE_SHIFT,
-                                        rambank_size[i])))
+                                        dom->rambank_size[i])))
             return rc;
     }
 
@@ -374,7 +372,7 @@  int arch_setup_meminit(struct xc_dom_image *dom)
      * If changing this then consider
      * xen/arch/arm/kernel.c:place_modules as well.
      */
-    bank0end = bankbase[0] + ((uint64_t)rambank_size[0] << XC_PAGE_SHIFT);
+    bank0end = bankbase[0] + ((uint64_t)dom->rambank_size[0] << XC_PAGE_SHIFT);
 
     if ( bank0end >= ram128mb + modsize && kernend < ram128mb )
         modbase = ram128mb;
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index 215ef9e..90fca67 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -255,24 +255,33 @@  static int make_psci_node(libxl__gc *gc, void *fdt)
     return 0;
 }
 
-static int make_memory_node(libxl__gc *gc, void *fdt,
-                            uint64_t base, uint64_t size)
+static int make_memory_nodes(libxl__gc *gc, void *fdt,
+                             const struct xc_dom_image *dom)
 {
-    int res;
-    const char *name = GCSPRINTF("memory@%"PRIx64, base);
+    int res, i;
+    const char *name;
+    const uint64_t bankbase[GUEST_RAM_BANKS] = {
+        GUEST_RAM0_BASE
+    };
 
-    res = fdt_begin_node(fdt, name);
-    if (res) return res;
+    for (i = 0; i < GUEST_RAM_BANKS; i++) {
+        name = GCSPRINTF("memory@%"PRIx64, bankbase[i]);
 
-    res = fdt_property_string(fdt, "device_type", "memory");
-    if (res) return res;
+        LOG(DEBUG, "Creating placeholder node /%s", name);
 
-    res = fdt_property_regs(gc, fdt, ROOT_ADDRESS_CELLS, ROOT_SIZE_CELLS,
-                            1, base, size);
-    if (res) return res;
+        res = fdt_begin_node(fdt, name);
+        if (res) return res;
 
-    res = fdt_end_node(fdt);
-    if (res) return res;
+        res = fdt_property_string(fdt, "device_type", "memory");
+        if (res) return res;
+
+        res = fdt_property_regs(gc, fdt, ROOT_ADDRESS_CELLS, ROOT_SIZE_CELLS,
+                                1, 0, 0);
+        if (res) return res;
+
+        res = fdt_end_node(fdt);
+        if (res) return res;
+    }
 
     return 0;
 }
@@ -489,9 +498,7 @@  next_resize:
         FDT( make_cpus_node(gc, fdt, info->max_vcpus, ainfo) );
         FDT( make_psci_node(gc, fdt) );
 
-        FDT( make_memory_node(gc, fdt,
-                              dom->rambase_pfn << XC_PAGE_SHIFT,
-                              info->target_memkb * 1024) );
+        FDT( make_memory_nodes(gc, fdt, dom) );
         FDT( make_intc_node(gc, fdt,
                             GUEST_GICD_BASE, GUEST_GICD_SIZE,
                             GUEST_GICC_BASE, GUEST_GICD_SIZE) );
@@ -521,11 +528,40 @@  out:
     return rc;
 }
 
+static void finalise_one_memory_node(libxl__gc *gc, void *fdt,
+                                     uint64_t base, uint64_t size)
+{
+    int node, res;
+    const char *name = GCSPRINTF("/memory@%"PRIx64, base);
+
+    node = fdt_path_offset(fdt, name);
+    assert(node > 0);
+
+    if (size == 0) {
+        LOG(DEBUG, "Nopping out placeholder node %s", name);
+        fdt_nop_node(fdt, node);
+    } else {
+        uint32_t regs[ROOT_ADDRESS_CELLS+ROOT_SIZE_CELLS];
+        be32 *cells = &regs[0];
+
+        LOG(DEBUG, "Populating placeholder node %s", name);
+
+        set_range(&cells, ROOT_ADDRESS_CELLS, ROOT_SIZE_CELLS, base, size);
+
+        res = fdt_setprop_inplace(fdt, node, "reg", regs, sizeof(regs));
+        assert(!res);
+    }
+}
+
 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;
+    int i;
+    const uint64_t bankbase[GUEST_RAM_BANKS] = {
+        GUEST_RAM0_BASE
+    };
 
     const struct xc_dom_seg *ramdisk = dom->ramdisk_blob ?
         &dom->ramdisk_seg : NULL;
@@ -552,9 +588,16 @@  int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
         assert(!res);
 
         val = cpu_to_fdt64(ramdisk->vend);
-        res = fdt_setprop_inplace(fdt, chosen,PROP_INITRD_END,
+        res = fdt_setprop_inplace(fdt, chosen, PROP_INITRD_END,
                                   &val, sizeof(val));
         assert(!res);
+
+    }
+
+    for (i = 0; i < GUEST_RAM_BANKS; i++) {
+        const uint64_t size = (uint64_t)dom->rambank_size[i] << XC_PAGE_SHIFT;
+
+        finalise_one_memory_node(gc, fdt, bankbase[i], size);
     }
 
     debug_dump_fdt(gc, fdt);