[PATCH 0/5] libxc: support building large pv-domains

[PATCH 0/5] libxc: support building large pv-domains

[Juergen Gross]

The Xen hypervisor supports starting a dom0 with large memory (up to
the TB range) by not including the initrd and p2m list in the initial
kernel mapping. Especially the p2m list can grow larger than the
available virtual space in the initial mapping.

The started kernel is indicating the support of each feature via
elf notes.

This series enables the domain builder in libxc to do the same as the
hypervisor. This enables starting of huge pv-domUs via xl.

Unmapped initrd is supported for 64 and 32 bit domains, omitting the
p2m from initial kernel mapping is possible for 64 bit domains only.

Tested with:
- 32 bit domU (kernel not supporting unmapped initrd)
- 32 bit domU (kernel supporting unmapped initrd)
- 1 GB 64 bit domU (kernel supporting unmapped initrd, not p2m)
- 1 GB 64 bit domU (kernel supporting unmapped initrd and p2m)
- 900GB 64 bit domU (kernel supporting unmapped initrd and p2m)

Juergen Gross (5):
  libxc: remove allocate member from struct xc_dom_image
  libxc: do initrd processing of domain builder in own function
  libxc: create unmapped initrd in domain builder if supported
  libxc: split p2m allocation in domain builder from other magic pages
  libxc: create p2m list outside of kernel mapping if supported

 tools/libxc/include/xc_dom.h |   4 +-
 tools/libxc/xc_dom_core.c    | 123 +++++++++++++++++++++++++++++--------------
 tools/libxc/xc_dom_x86.c     | 120 ++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 204 insertions(+), 43 deletions(-)

-- 
2.1.4

[PATCH 1/5] libxc: remove allocate member from struct xc_dom_image

[Juergen Gross]

The allocate() callback in struct xc_dom_image is never set. Remove it.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/libxc/include/xc_dom.h | 2 --
 tools/libxc/xc_dom_core.c    | 4 ----
 2 files changed, 6 deletions(-)

diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
index 600aef6..a4ca8a4 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -177,8 +177,6 @@ struct xc_dom_image {
 
     /* kernel loader */
     struct xc_dom_arch *arch_hooks;
-    /* allocate up to virt_alloc_end */
-    int (*allocate) (struct xc_dom_image * dom, xen_vaddr_t up_to);
 };
 
 /* --- pluggable kernel loader ------------------------------------- */
diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
index 8466677..b432762 100644
--- a/tools/libxc/xc_dom_core.c
+++ b/tools/libxc/xc_dom_core.c
@@ -579,8 +579,6 @@ int xc_dom_alloc_segment(struct xc_dom_image *dom,
 
     seg->vend = start + pages * page_size;
     dom->virt_alloc_end = seg->vend;
-    if (dom->allocate)
-        dom->allocate(dom, dom->virt_alloc_end);
 
     DOMPRINTF("%-20s:   %-12s : 0x%" PRIx64 " -> 0x%" PRIx64
               "  (pfn 0x%" PRIpfn " + 0x%" PRIpfn " pages)",
@@ -603,8 +601,6 @@ int xc_dom_alloc_page(struct xc_dom_image *dom, char *name)
 
     start = dom->virt_alloc_end;
     dom->virt_alloc_end += page_size;
-    if (dom->allocate)
-        dom->allocate(dom, dom->virt_alloc_end);
     pfn = (start - dom->parms.virt_base) / page_size;
 
     DOMPRINTF("%-20s:   %-12s : 0x%" PRIx64 " (pfn 0x%" PRIpfn ")",
-- 
2.1.4

[PATCH 2/5] libxc: do initrd processing of domain builder in own function

[Juergen Gross]

Factor out the initrd processing in xc_dom_build_image() into an own
function to prepare starting a domain with unmapped initrd.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/libxc/xc_dom_core.c | 77 ++++++++++++++++++++++++++---------------------
 1 file changed, 43 insertions(+), 34 deletions(-)

diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
index b432762..b510bbd 100644
--- a/tools/libxc/xc_dom_core.c
+++ b/tools/libxc/xc_dom_core.c
@@ -953,6 +953,48 @@ int xc_dom_update_guest_p2m(struct xc_dom_image *dom)
     return 0;
 }
 
+static int xc_dom_build_ramdisk(struct xc_dom_image *dom)
+{
+    size_t unziplen, ramdisklen;
+    void *ramdiskmap;
+
+    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",
+                              dom->ramdisk_seg.vstart, ramdisklen) != 0 )
+        goto err;
+    ramdiskmap = xc_dom_seg_to_ptr(dom, &dom->ramdisk_seg);
+    if ( ramdiskmap == NULL )
+    {
+        DOMPRINTF("%s: xc_dom_seg_to_ptr(dom, &dom->ramdisk_seg) => NULL",
+                  __FUNCTION__);
+        goto err;
+    }
+    if ( unziplen )
+    {
+        if ( xc_dom_do_gunzip(dom->xch, dom->ramdisk_blob, dom->ramdisk_size,
+                              ramdiskmap, ramdisklen) == -1 )
+            goto err;
+    }
+    else
+        memcpy(ramdiskmap, dom->ramdisk_blob, dom->ramdisk_size);
+
+    return 0;
+
+ err:
+    return -1;
+}
+
 int xc_dom_build_image(struct xc_dom_image *dom)
 {
     unsigned int page_size;
@@ -980,41 +1022,8 @@ int xc_dom_build_image(struct xc_dom_image *dom)
     /* load ramdisk */
     if ( dom->ramdisk_blob )
     {
-        size_t unziplen, ramdisklen;
-        void *ramdiskmap;
-
-        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",
-                                  dom->ramdisk_seg.vstart,
-                                  ramdisklen) != 0 )
+        if ( xc_dom_build_ramdisk(dom) != 0 )
             goto err;
-        ramdiskmap = xc_dom_seg_to_ptr(dom, &dom->ramdisk_seg);
-        if ( ramdiskmap == NULL )
-        {
-            DOMPRINTF("%s: xc_dom_seg_to_ptr(dom, &dom->ramdisk_seg) => NULL",
-                      __FUNCTION__);
-            goto err;
-        }
-        if ( unziplen )
-        {
-            if ( xc_dom_do_gunzip(dom->xch,
-                                  dom->ramdisk_blob, dom->ramdisk_size,
-                                  ramdiskmap, ramdisklen) == -1 )
-                goto err;
-        }
-        else
-            memcpy(ramdiskmap, dom->ramdisk_blob, dom->ramdisk_size);
     }
 
     /* load devicetree */
-- 
2.1.4

[PATCH 3/5] libxc: create unmapped initrd in domain builder if supported

[Juergen Gross]

In case the kernel of a new pv-domU indicates it is supporting an
unmapped initrd, don't waste precious virtual space for the initrd,
but allocate only guest physical memory for it.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/libxc/xc_dom_core.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
index b510bbd..bb668b1 100644
--- a/tools/libxc/xc_dom_core.c
+++ b/tools/libxc/xc_dom_core.c
@@ -1019,8 +1019,10 @@ int xc_dom_build_image(struct xc_dom_image *dom)
     if ( dom->kernel_loader->loader(dom) != 0 )
         goto err;
 
-    /* load ramdisk */
-    if ( dom->ramdisk_blob )
+    /* Load ramdisk if initial mapping required. */
+    if ( dom->ramdisk_blob &&
+         (!dom->parms.elf_notes[XEN_ELFNOTE_MOD_START_PFN].data.num ||
+          dom->ramdisk_seg.vstart) )
     {
         if ( xc_dom_build_ramdisk(dom) != 0 )
             goto err;
@@ -1059,10 +1061,28 @@ int xc_dom_build_image(struct xc_dom_image *dom)
     }
     if ( dom->alloc_bootstack )
         dom->bootstack_pfn = xc_dom_alloc_page(dom, "boot stack");
+
     DOMPRINTF("%-20s: virt_alloc_end : 0x%" PRIx64 "",
               __FUNCTION__, dom->virt_alloc_end);
     DOMPRINTF("%-20s: virt_pgtab_end : 0x%" PRIx64 "",
               __FUNCTION__, dom->virt_pgtab_end);
+
+    /* Prepare allocating unmapped memory. */
+    if ( dom->virt_pgtab_end )
+        dom->virt_alloc_end = dom->virt_pgtab_end;
+
+    /* Load ramdisk if no initial mapping required. */
+    if ( dom->ramdisk_blob && !dom->ramdisk_seg.vstart &&
+         dom->parms.elf_notes[XEN_ELFNOTE_MOD_START_PFN].data.num )
+    {
+        if ( xc_dom_build_ramdisk(dom) != 0 )
+            goto err;
+        dom->flags |= SIF_MOD_START_PFN;
+        dom->ramdisk_seg.vend = dom->ramdisk_seg.vend - dom->ramdisk_seg.vstart;
+        dom->ramdisk_seg.vstart = dom->ramdisk_seg.pfn;
+        dom->ramdisk_seg.vend += dom->ramdisk_seg.vstart;
+    }
+
     return 0;
 
  err:
-- 
2.1.4

[PATCH 4/5] libxc: split p2m allocation in domain builder from other magic pages

[Juergen Gross]

Add an own function to allocate the p2m list in the domain builder in
order to prepare allocating the p2m list outside of the initial kernel
mapping. This will be needed to support loading domains with huge
memory (>512 GB).

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/libxc/include/xc_dom.h |  1 +
 tools/libxc/xc_dom_core.c    |  3 +++
 tools/libxc/xc_dom_x86.c     | 11 ++++++++++-
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
index a4ca8a4..43b1eab 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -199,6 +199,7 @@ void xc_dom_register_loader(struct xc_dom_loader *loader);
 struct xc_dom_arch {
     /* pagetable setup */
     int (*alloc_magic_pages) (struct xc_dom_image * dom);
+    int (*alloc_p2m_list) (struct xc_dom_image * dom);
     int (*count_pgtables) (struct xc_dom_image * dom);
     int (*setup_pgtables) (struct xc_dom_image * dom);
 
diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
index bb668b1..81b642e 100644
--- a/tools/libxc/xc_dom_core.c
+++ b/tools/libxc/xc_dom_core.c
@@ -1048,6 +1048,9 @@ int xc_dom_build_image(struct xc_dom_image *dom)
     }
 
     /* allocate other pages */
+    if ( dom->arch_hooks->alloc_p2m_list &&
+         dom->arch_hooks->alloc_p2m_list(dom) != 0 )
+        goto err;
     if ( dom->arch_hooks->alloc_magic_pages(dom) != 0 )
         goto err;
     if ( dom->arch_hooks->count_pgtables )
diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index 3d40fa4..91d4e49 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -438,7 +438,7 @@ pfn_error:
 
 /* ------------------------------------------------------------------------ */
 
-static int alloc_magic_pages(struct xc_dom_image *dom)
+static int alloc_p2m_list(struct xc_dom_image *dom)
 {
     size_t p2m_alloc_size = dom->p2m_size * dom->arch_hooks->sizeof_pfn;
 
@@ -450,6 +450,13 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
     if ( dom->p2m_guest == NULL )
         return -1;
 
+    return 0;
+}
+
+/* ------------------------------------------------------------------------ */
+
+static int alloc_magic_pages(struct xc_dom_image *dom)
+{
     /* allocate special pages */
     dom->start_info_pfn = xc_dom_alloc_page(dom, "start info");
     dom->xenstore_pfn = xc_dom_alloc_page(dom, "xenstore");
@@ -676,6 +683,7 @@ static struct xc_dom_arch xc_dom_32_pae = {
     .page_shift = PAGE_SHIFT_X86,
     .sizeof_pfn = 4,
     .alloc_magic_pages = alloc_magic_pages,
+    .alloc_p2m_list = alloc_p2m_list,
     .count_pgtables = count_pgtables_x86_32_pae,
     .setup_pgtables = setup_pgtables_x86_32_pae,
     .start_info = start_info_x86_32,
@@ -689,6 +697,7 @@ static struct xc_dom_arch xc_dom_64 = {
     .page_shift = PAGE_SHIFT_X86,
     .sizeof_pfn = 8,
     .alloc_magic_pages = alloc_magic_pages,
+    .alloc_p2m_list = alloc_p2m_list,
     .count_pgtables = count_pgtables_x86_64,
     .setup_pgtables = setup_pgtables_x86_64,
     .start_info = start_info_x86_64,
-- 
2.1.4

[PATCH 5/5] libxc: create p2m list outside of kernel mapping if supported

[Juergen Gross]

In case the kernel of a new pv-domU indicates it is supporting a p2m
list outside the initial kernel mapping by specifying INIT_P2M, let
the domain builder allocate the memory for the p2m list from physical
guest memory only and map it to the address the kernel is expecting.

This will enable loading pv-domUs larger than 512 GB.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/libxc/include/xc_dom.h |   1 +
 tools/libxc/xc_dom_core.c    |  17 ++++++-
 tools/libxc/xc_dom_x86.c     | 109 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 126 insertions(+), 1 deletion(-)

diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
index 43b1eab..6192fba 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -212,6 +212,7 @@ struct xc_dom_arch {
     char *native_protocol;
     int page_shift;
     int sizeof_pfn;
+    int p2m_base_supported;
 
     struct xc_dom_arch *next;
 };
diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
index 81b642e..1cf77d7 100644
--- a/tools/libxc/xc_dom_core.c
+++ b/tools/libxc/xc_dom_core.c
@@ -734,6 +734,7 @@ struct xc_dom_image *xc_dom_allocate(xc_interface *xch,
     dom->parms.virt_hypercall = UNSET_ADDR;
     dom->parms.virt_hv_start_low = UNSET_ADDR;
     dom->parms.elf_paddr_offset = UNSET_ADDR;
+    dom->parms.p2m_base = UNSET_ADDR;
 
     dom->alloc_malloc += sizeof(*dom);
     return dom;
@@ -1048,7 +1049,11 @@ int xc_dom_build_image(struct xc_dom_image *dom)
     }
 
     /* allocate other pages */
-    if ( dom->arch_hooks->alloc_p2m_list &&
+    if ( !dom->arch_hooks->p2m_base_supported ||
+         dom->parms.p2m_base >= dom->parms.virt_base ||
+         (dom->parms.p2m_base & (XC_DOM_PAGE_SIZE(dom) - 1)) )
+        dom->parms.p2m_base = UNSET_ADDR;
+    if ( dom->arch_hooks->alloc_p2m_list && dom->parms.p2m_base == UNSET_ADDR &&
          dom->arch_hooks->alloc_p2m_list(dom) != 0 )
         goto err;
     if ( dom->arch_hooks->alloc_magic_pages(dom) != 0 )
@@ -1086,6 +1091,16 @@ int xc_dom_build_image(struct xc_dom_image *dom)
         dom->ramdisk_seg.vend += dom->ramdisk_seg.vstart;
     }
 
+    /* Allocate p2m list if outside of initial kernel mapping. */
+    if ( dom->arch_hooks->alloc_p2m_list && dom->parms.p2m_base != UNSET_ADDR )
+    {
+        if ( dom->arch_hooks->alloc_p2m_list(dom) != 0 )
+            goto err;
+        dom->p2m_seg.vend = dom->p2m_seg.vend - dom->p2m_seg.vstart;
+        dom->p2m_seg.vstart = dom->parms.p2m_base;
+        dom->p2m_seg.vend += dom->p2m_seg.vstart;
+    }
+
     return 0;
 
  err:
diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index 91d4e49..debc685 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -46,6 +46,8 @@
 #define bits_to_mask(bits)       (((xen_vaddr_t)1 << (bits))-1)
 #define round_down(addr, mask)   ((addr) & ~(mask))
 #define round_up(addr, mask)     ((addr) | (mask))
+#define round_pg(addr)    (((addr) + PAGE_SIZE_X86 - 1) & ~(PAGE_SIZE_X86 - 1))
+#define round_pfn(addr)   (((addr) + PAGE_SIZE_X86 - 1) / PAGE_SIZE_X86)
 
 /* get guest IO ABI protocol */
 const char *xc_domain_get_native_protocol(xc_interface *xch,
@@ -423,6 +425,81 @@ static int setup_pgtables_x86_64(struct xc_dom_image *dom)
             }
         }
     }
+
+    if ( dom->parms.p2m_base == UNSET_ADDR )
+        return 0;
+
+    /*
+     * Build the page tables for mapping the p2m list at an address
+     * specified by the to be loaded kernel.
+     * l1pfn holds the pfn of the next page table to allocate.
+     * At each level we might already have an entry filled when setting
+     * up the initial kernel mapping. This can happen for the last entry
+     * of each level only!
+     */
+    l3tab = NULL;
+    l2tab = NULL;
+    l1tab = NULL;
+    l1pfn = round_pfn(dom->p2m_size * dom->arch_hooks->sizeof_pfn) +
+            dom->p2m_seg.pfn;
+
+    for ( addr = dom->parms.p2m_base;
+          addr < dom->parms.p2m_base +
+                 dom->p2m_size * dom->arch_hooks->sizeof_pfn;
+          addr += PAGE_SIZE_X86 )
+    {
+        if ( l3tab == NULL )
+        {
+            l4off = l4_table_offset_x86_64(addr);
+            l3pfn = l4tab[l4off] ? l4pfn + dom->pg_l4 : l1pfn++;
+            l3tab = xc_dom_pfn_to_ptr(dom, l3pfn, 1);
+            if ( l3tab == NULL )
+                goto pfn_error;
+            l4tab[l4off] =
+                pfn_to_paddr(xc_dom_p2m_guest(dom, l3pfn)) | L4_PROT;
+        }
+
+        if ( l2tab == NULL )
+        {
+            l3off = l3_table_offset_x86_64(addr);
+            l2pfn = l3tab[l3off] ? l3pfn + dom->pg_l3 : l1pfn++;
+            l2tab = xc_dom_pfn_to_ptr(dom, l2pfn, 1);
+            if ( l2tab == NULL )
+                goto pfn_error;
+            l3tab[l3off] =
+                pfn_to_paddr(xc_dom_p2m_guest(dom, l2pfn)) | L3_PROT;
+        }
+
+        if ( l1tab == NULL )
+        {
+            l2off = l2_table_offset_x86_64(addr);
+            l1pfn = l2tab[l2off] ? l2pfn + dom->pg_l2 : l1pfn;
+            l1tab = xc_dom_pfn_to_ptr(dom, l1pfn, 1);
+            if ( l1tab == NULL )
+                goto pfn_error;
+            l2tab[l2off] =
+                pfn_to_paddr(xc_dom_p2m_guest(dom, l1pfn)) | L2_PROT;
+            l1pfn++;
+        }
+
+        l1off = l1_table_offset_x86_64(addr);
+        pgpfn = ((addr - dom->parms.p2m_base) >> PAGE_SHIFT_X86) +
+                dom->p2m_seg.pfn;
+        l1tab[l1off] =
+            pfn_to_paddr(xc_dom_p2m_guest(dom, pgpfn)) | L1_PROT;
+
+        if ( l1off == (L1_PAGETABLE_ENTRIES_X86_64 - 1) )
+        {
+            l1tab = NULL;
+            if ( l2off == (L2_PAGETABLE_ENTRIES_X86_64 - 1) )
+            {
+                l2tab = NULL;
+                if ( l3off == (L3_PAGETABLE_ENTRIES_X86_64 - 1) )
+                    l3tab = NULL;
+            }
+        }
+    }
+
     return 0;
 
 pfn_error:
@@ -441,6 +518,27 @@ pfn_error:
 static int alloc_p2m_list(struct xc_dom_image *dom)
 {
     size_t p2m_alloc_size = dom->p2m_size * dom->arch_hooks->sizeof_pfn;
+    xen_vaddr_t from, to;
+    xen_pfn_t tables;
+
+    p2m_alloc_size = round_pg(p2m_alloc_size);
+    if ( dom->parms.p2m_base != UNSET_ADDR )
+    {
+        /* Add space for page tables, 64 bit only. */
+        from = dom->parms.p2m_base;
+        to = from + p2m_alloc_size - 1;
+        tables = 0;
+        tables += nr_page_tables(dom, from, to, L4_PAGETABLE_SHIFT_X86_64);
+        if ( to > (xen_vaddr_t)(~0ULL << L4_PAGETABLE_SHIFT_X86_64) )
+            tables--;
+        tables += nr_page_tables(dom, from, to, L3_PAGETABLE_SHIFT_X86_64);
+        if ( to > (xen_vaddr_t)(~0ULL << L3_PAGETABLE_SHIFT_X86_64) )
+            tables--;
+        tables += nr_page_tables(dom, from, to, L2_PAGETABLE_SHIFT_X86_64);
+        if ( to > (xen_vaddr_t)(~0ULL << L2_PAGETABLE_SHIFT_X86_64) )
+            tables--;
+        p2m_alloc_size += tables << PAGE_SHIFT_X86;
+    }
 
     /* allocate phys2mach table */
     if ( xc_dom_alloc_segment(dom, &dom->p2m_seg, "phys2mach",
@@ -540,6 +638,12 @@ static int start_info_x86_64(struct xc_dom_image *dom)
     start_info->pt_base = dom->pgtables_seg.vstart;
     start_info->nr_pt_frames = dom->pgtables;
     start_info->mfn_list = dom->p2m_seg.vstart;
+    if ( dom->parms.p2m_base != UNSET_ADDR )
+    {
+        start_info->first_p2m_pfn = dom->p2m_seg.pfn;
+        start_info->nr_p2m_frames =
+            (dom->p2m_seg.vend - dom->p2m_seg.vstart) >> PAGE_SHIFT_X86;
+    }
 
     start_info->flags = dom->flags;
     start_info->store_mfn = xc_dom_p2m_guest(dom, dom->xenstore_pfn);
@@ -682,6 +786,7 @@ static struct xc_dom_arch xc_dom_32_pae = {
     .native_protocol = XEN_IO_PROTO_ABI_X86_32,
     .page_shift = PAGE_SHIFT_X86,
     .sizeof_pfn = 4,
+    .p2m_base_supported = 0,
     .alloc_magic_pages = alloc_magic_pages,
     .alloc_p2m_list = alloc_p2m_list,
     .count_pgtables = count_pgtables_x86_32_pae,
@@ -696,6 +801,7 @@ static struct xc_dom_arch xc_dom_64 = {
     .native_protocol = XEN_IO_PROTO_ABI_X86_64,
     .page_shift = PAGE_SHIFT_X86,
     .sizeof_pfn = 8,
+    .p2m_base_supported = 1,
     .alloc_magic_pages = alloc_magic_pages,
     .alloc_p2m_list = alloc_p2m_list,
     .count_pgtables = count_pgtables_x86_64,
@@ -1027,7 +1133,10 @@ int arch_setup_bootlate(struct xc_dom_image *dom)
     if ( !xc_dom_feature_translated(dom) )
     {
         /* paravirtualized guest */
+
+        /* Drop references to all initial page tables before pinning. */
         xc_dom_unmap_one(dom, dom->pgtables_seg.pfn);
+        xc_dom_unmap_one(dom, dom->p2m_seg.pfn);
         rc = pin_table(dom->xch, pgd_type,
                        xc_dom_p2m_host(dom, dom->pgtables_seg.pfn),
                        dom->guest_domid);
-- 
2.1.4

Re: [PATCH 1/5] libxc: remove allocate member from struct xc_dom_image

[Ian Jackson]

Juergen Gross writes ("[PATCH 1/5] libxc: remove allocate member from struct xc_dom_image"):
> The allocate() callback in struct xc_dom_image is never set. Remove it.

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

Re: [PATCH 2/5] libxc: do initrd processing of domain builder in own function

[Ian Jackson]

Juergen Gross writes ("[PATCH 2/5] libxc: do initrd processing of domain builder in own function"):
> Factor out the initrd processing in xc_dom_build_image() into an own
> function to prepare starting a domain with unmapped initrd.

I haven't checked that this is just code motion and the obvious
refactoring, but it looks like it is.  On that basis

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

Re: [PATCH 3/5] libxc: create unmapped initrd in domain builder if supported

[Ian Jackson]

Juergen Gross writes ("[PATCH 3/5] libxc: create unmapped initrd in domain builder if supported"):
> In case the kernel of a new pv-domU indicates it is supporting an
> unmapped initrd, don't waste precious virtual space for the initrd,
> but allocate only guest physical memory for it.
...

The name of this ELFNOTE suggests that it applies to all multiboot
modules, not just ramdisks.  In particular, that means perhaps it
ought to apply to device tree blobs too ?

> -    /* load ramdisk */
> -    if ( dom->ramdisk_blob )
> +    /* Load ramdisk if initial mapping required. */
> +    if ( dom->ramdisk_blob &&
> +         (!dom->parms.elf_notes[XEN_ELFNOTE_MOD_START_PFN].data.num ||
> +          dom->ramdisk_seg.vstart) )

After this patch the resulting structure of the code is rather
unfortunate, in that the order of the main processing steps depends on
this ELFNOTE.

Wouldn't it be better to generalise xc_dom_alloc_segment ?

Ian.

Re: [PATCH 3/5] libxc: create unmapped initrd in domain builder if supported

[Julien Grall]

On 11/09/15 13:54, Ian Jackson wrote:
> Juergen Gross writes ("[PATCH 3/5] libxc: create unmapped initrd in domain builder if supported"):
>> In case the kernel of a new pv-domU indicates it is supporting an
>> unmapped initrd, don't waste precious virtual space for the initrd,
>> but allocate only guest physical memory for it.
> ...
> 
> The name of this ELFNOTE suggests that it applies to all multiboot
> modules, not just ramdisks.  In particular, that means perhaps it
> ought to apply to device tree blobs too ?

The device tree blobs is not a multiboot module but directly pass in a
register to the kernel.

FWIW, we don't have any ELF support right now on ARM.

>> -    /* load ramdisk */
>> -    if ( dom->ramdisk_blob )
>> +    /* Load ramdisk if initial mapping required. */
>> +    if ( dom->ramdisk_blob &&
>> +         (!dom->parms.elf_notes[XEN_ELFNOTE_MOD_START_PFN].data.num ||
>> +          dom->ramdisk_seg.vstart) )
> 
> After this patch the resulting structure of the code is rather
> unfortunate, in that the order of the main processing steps depends on
> this ELFNOTE.

Shouldn't we ought to have a common code ELF agnostic? I.e we may have
other kernel image format where we have notes but not ELF notes.

Regards,

-- 
Julien Grall

Re: [PATCH 0/5] libxc: support building large pv-domains

[Ian Campbell]

On Fri, 2015-09-11 at 14:32 +0200, Juergen Gross wrote:
> The Xen hypervisor supports starting a dom0 with large memory (up to
> the TB range) by not including the initrd and p2m list in the initial
> kernel mapping. Especially the p2m list can grow larger than the
> available virtual space in the initial mapping.
> 
> The started kernel is indicating the support of each feature via
> elf notes.
> 
> This series enables the domain builder in libxc to do the same as the
> hypervisor. This enables starting of huge pv-domUs via xl.
> 
> Unmapped initrd is supported for 64 and 32 bit domains, omitting the
> p2m from initial kernel mapping is possible for 64 bit domains only.
> 
> Tested with:
> - 32 bit domU (kernel not supporting unmapped initrd)
> - 32 bit domU (kernel supporting unmapped initrd)
> - 1 GB 64 bit domU (kernel supporting unmapped initrd, not p2m)
> - 1 GB 64 bit domU (kernel supporting unmapped initrd and p2m)
> - 900GB 64 bit domU (kernel supporting unmapped initrd and p2m)
> 
> Juergen Gross (5):
>   libxc: remove allocate member from struct xc_dom_image
>   libxc: do initrd processing of domain builder in own function
>   libxc: create unmapped initrd in domain builder if supported
>   libxc: split p2m allocation in domain builder from other magic pages
>   libxc: create p2m list outside of kernel mapping if supported
> 
>  tools/libxc/include/xc_dom.h |   4 +-
>  tools/libxc/xc_dom_core.c    | 123 +++++++++++++++++++++++++++++--------------
>  tools/libxc/xc_dom_x86.c     | 120 ++++++++++++++++++++++++++++++++++++++++-

How much is this going to conflict with Roger's "Introduce HVM without dm
and new boot ABI" changes to HVM building?

>  3 files changed, 204 insertions(+), 43 deletions(-)
> 

Re: [PATCH 3/5] libxc: create unmapped initrd in domain builder if supported

[Juergen Gross]

On 09/11/2015 02:54 PM, Ian Jackson wrote:
> Juergen Gross writes ("[PATCH 3/5] libxc: create unmapped initrd in domain builder if supported"):
>> In case the kernel of a new pv-domU indicates it is supporting an
>> unmapped initrd, don't waste precious virtual space for the initrd,
>> but allocate only guest physical memory for it.
> ...
>
> The name of this ELFNOTE suggests that it applies to all multiboot
> modules, not just ramdisks.  In particular, that means perhaps it
> ought to apply to device tree blobs too ?

Hmm, in theory, yes.

AFAIK it is only used for initrd on x86. I think support for other
modules can be added as needed.

>> -    /* load ramdisk */
>> -    if ( dom->ramdisk_blob )
>> +    /* Load ramdisk if initial mapping required. */
>> +    if ( dom->ramdisk_blob &&
>> +         (!dom->parms.elf_notes[XEN_ELFNOTE_MOD_START_PFN].data.num ||
>> +          dom->ramdisk_seg.vstart) )
>
> After this patch the resulting structure of the code is rather
> unfortunate, in that the order of the main processing steps depends on
> this ELFNOTE.
>
> Wouldn't it be better to generalise xc_dom_alloc_segment ?

How?

You have to create (and allocate space for) the page tables after
all allocations which should be covered by those page tables. And
you must not allocate the other stuff before that, as this would
again waste virtual address space, which is 1:1 with guest physical
memory.

The only solution would be to calculate the needed sizes of the
single memory chunks first and then do the allocations either in
the mapped or the unmapped region according to the ELFNOTEs. This
would rip at least the initrd processing into two parts, as the
needed memory size is calculated depending on the initrd being
compressed or not.

I thought about building a table containing the sequence of the
single processing steps dependant on the ELFNOTEs and processing this
table in a generic loop afterwards. If you like this approach, I can
give it a try. I just wanted to avoid a complete rework of the main
building function.


Juergen

Re: [PATCH 3/5] libxc: create unmapped initrd in domain builder if supported

[Juergen Gross]

On 09/11/2015 03:15 PM, Julien Grall wrote:
> On 11/09/15 13:54, Ian Jackson wrote:
>> Juergen Gross writes ("[PATCH 3/5] libxc: create unmapped initrd in domain builder if supported"):
>>> In case the kernel of a new pv-domU indicates it is supporting an
>>> unmapped initrd, don't waste precious virtual space for the initrd,
>>> but allocate only guest physical memory for it.
>> ...
>>
>> The name of this ELFNOTE suggests that it applies to all multiboot
>> modules, not just ramdisks.  In particular, that means perhaps it
>> ought to apply to device tree blobs too ?
>
> The device tree blobs is not a multiboot module but directly pass in a
> register to the kernel.
>
> FWIW, we don't have any ELF support right now on ARM.

Okay, I thought so, but I wasn't sure.

>
>>> -    /* load ramdisk */
>>> -    if ( dom->ramdisk_blob )
>>> +    /* Load ramdisk if initial mapping required. */
>>> +    if ( dom->ramdisk_blob &&
>>> +         (!dom->parms.elf_notes[XEN_ELFNOTE_MOD_START_PFN].data.num ||
>>> +          dom->ramdisk_seg.vstart) )
>>
>> After this patch the resulting structure of the code is rather
>> unfortunate, in that the order of the main processing steps depends on
>> this ELFNOTE.
>
> Shouldn't we ought to have a common code ELF agnostic? I.e we may have
> other kernel image format where we have notes but not ELF notes.

dom->parms is the same for all architectures. I think it would have to
be extended in that case.


Juergen

Re: [PATCH 0/5] libxc: support building large pv-domains

[Juergen Gross]

On 09/11/2015 03:28 PM, Ian Campbell wrote:
> On Fri, 2015-09-11 at 14:32 +0200, Juergen Gross wrote:
>> The Xen hypervisor supports starting a dom0 with large memory (up to
>> the TB range) by not including the initrd and p2m list in the initial
>> kernel mapping. Especially the p2m list can grow larger than the
>> available virtual space in the initial mapping.
>>
>> The started kernel is indicating the support of each feature via
>> elf notes.
>>
>> This series enables the domain builder in libxc to do the same as the
>> hypervisor. This enables starting of huge pv-domUs via xl.
>>
>> Unmapped initrd is supported for 64 and 32 bit domains, omitting the
>> p2m from initial kernel mapping is possible for 64 bit domains only.
>>
>> Tested with:
>> - 32 bit domU (kernel not supporting unmapped initrd)
>> - 32 bit domU (kernel supporting unmapped initrd)
>> - 1 GB 64 bit domU (kernel supporting unmapped initrd, not p2m)
>> - 1 GB 64 bit domU (kernel supporting unmapped initrd and p2m)
>> - 900GB 64 bit domU (kernel supporting unmapped initrd and p2m)
>>
>> Juergen Gross (5):
>>    libxc: remove allocate member from struct xc_dom_image
>>    libxc: do initrd processing of domain builder in own function
>>    libxc: create unmapped initrd in domain builder if supported
>>    libxc: split p2m allocation in domain builder from other magic pages
>>    libxc: create p2m list outside of kernel mapping if supported
>>
>>   tools/libxc/include/xc_dom.h |   4 +-
>>   tools/libxc/xc_dom_core.c    | 123 +++++++++++++++++++++++++++++--------------
>>   tools/libxc/xc_dom_x86.c     | 120 ++++++++++++++++++++++++++++++++++++++++-
>
> How much is this going to conflict with Roger's "Introduce HVM without dm
> and new boot ABI" changes to HVM building?

As it is touching the pv domain builder only, I don't think there will
be a conflict. All rights of being wrong reserved. :-)

Juergen

Re: [PATCH 0/5] libxc: support building large pv-domains

[Ian Campbell]

On Fri, 2015-09-11 at 15:42 +0200, Juergen Gross wrote:
> On 09/11/2015 03:28 PM, Ian Campbell wrote:
> > On Fri, 2015-09-11 at 14:32 +0200, Juergen Gross wrote:
> > > The Xen hypervisor supports starting a dom0 with large memory (up to
> > > the TB range) by not including the initrd and p2m list in the initial
> > > kernel mapping. Especially the p2m list can grow larger than the
> > > available virtual space in the initial mapping.
> > > 
> > > The started kernel is indicating the support of each feature via
> > > elf notes.
> > > 
> > > This series enables the domain builder in libxc to do the same as the
> > > hypervisor. This enables starting of huge pv-domUs via xl.
> > > 
> > > Unmapped initrd is supported for 64 and 32 bit domains, omitting the
> > > p2m from initial kernel mapping is possible for 64 bit domains only.
> > > 
> > > Tested with:
> > > - 32 bit domU (kernel not supporting unmapped initrd)
> > > - 32 bit domU (kernel supporting unmapped initrd)
> > > - 1 GB 64 bit domU (kernel supporting unmapped initrd, not p2m)
> > > - 1 GB 64 bit domU (kernel supporting unmapped initrd and p2m)
> > > - 900GB 64 bit domU (kernel supporting unmapped initrd and p2m)
> > > 
> > > Juergen Gross (5):
> > >    libxc: remove allocate member from struct xc_dom_image
> > >    libxc: do initrd processing of domain builder in own function
> > >    libxc: create unmapped initrd in domain builder if supported
> > >    libxc: split p2m allocation in domain builder from other magic
> > > pages
> > >    libxc: create p2m list outside of kernel mapping if supported
> > > 
> > >   tools/libxc/include/xc_dom.h |   4 +-
> > >   tools/libxc/xc_dom_core.c    | 123 +++++++++++++++++++++++++++++---
> > > -----------
> > >   tools/libxc/xc_dom_x86.c     | 120
> > > ++++++++++++++++++++++++++++++++++++++++-
> > 
> > How much is this going to conflict with Roger's "Introduce HVM without
> > dm
> > and new boot ABI" changes to HVM building?
> 
> As it is touching the pv domain builder only, I don't think there will
> be a conflict.

The reason I asked is that the first thing Roger's series does is cause HVM
domains to be built using the PV domain builder...

>  All rights of being wrong reserved. :-)

Warranty void to the limit of your statutory rights ;-)

Ian.

Re: [PATCH 0/5] libxc: support building large pv-domains

[Juergen Gross]

On 09/11/2015 03:53 PM, Ian Campbell wrote:
> On Fri, 2015-09-11 at 15:42 +0200, Juergen Gross wrote:
>> On 09/11/2015 03:28 PM, Ian Campbell wrote:
>>> On Fri, 2015-09-11 at 14:32 +0200, Juergen Gross wrote:
>>>> The Xen hypervisor supports starting a dom0 with large memory (up to
>>>> the TB range) by not including the initrd and p2m list in the initial
>>>> kernel mapping. Especially the p2m list can grow larger than the
>>>> available virtual space in the initial mapping.
>>>>
>>>> The started kernel is indicating the support of each feature via
>>>> elf notes.
>>>>
>>>> This series enables the domain builder in libxc to do the same as the
>>>> hypervisor. This enables starting of huge pv-domUs via xl.
>>>>
>>>> Unmapped initrd is supported for 64 and 32 bit domains, omitting the
>>>> p2m from initial kernel mapping is possible for 64 bit domains only.
>>>>
>>>> Tested with:
>>>> - 32 bit domU (kernel not supporting unmapped initrd)
>>>> - 32 bit domU (kernel supporting unmapped initrd)
>>>> - 1 GB 64 bit domU (kernel supporting unmapped initrd, not p2m)
>>>> - 1 GB 64 bit domU (kernel supporting unmapped initrd and p2m)
>>>> - 900GB 64 bit domU (kernel supporting unmapped initrd and p2m)
>>>>
>>>> Juergen Gross (5):
>>>>     libxc: remove allocate member from struct xc_dom_image
>>>>     libxc: do initrd processing of domain builder in own function
>>>>     libxc: create unmapped initrd in domain builder if supported
>>>>     libxc: split p2m allocation in domain builder from other magic
>>>> pages
>>>>     libxc: create p2m list outside of kernel mapping if supported
>>>>
>>>>    tools/libxc/include/xc_dom.h |   4 +-
>>>>    tools/libxc/xc_dom_core.c    | 123 +++++++++++++++++++++++++++++---
>>>> -----------
>>>>    tools/libxc/xc_dom_x86.c     | 120
>>>> ++++++++++++++++++++++++++++++++++++++++-
>>>
>>> How much is this going to conflict with Roger's "Introduce HVM without
>>> dm
>>> and new boot ABI" changes to HVM building?
>>
>> As it is touching the pv domain builder only, I don't think there will
>> be a conflict.
>
> The reason I asked is that the first thing Roger's series does is cause HVM
> domains to be built using the PV domain builder...

Aah, okay.

OTOH I'm doing nothing different than the hypervisor when loading dom0.
As long as the ELFNOTEs in question (or the corresponding elements in
dom->parms) are not set, the resulting domain image should be the same
as today.

>>   All rights of being wrong reserved. :-)
>
> Warranty void to the limit of your statutory rights ;-)

Ha, good intuition! The disclaimer wasn't a bad idea at the end. ;-)


Juergen

Re: [PATCH 3/5] libxc: create unmapped initrd in domain builder if supported

[Ian Jackson]

Juergen Gross writes ("Re: [PATCH 3/5] libxc: create unmapped initrd in domain builder if supported"):
> I thought about building a table containing the sequence of the
> single processing steps dependant on the ELFNOTEs and processing this
> table in a generic loop afterwards. If you like this approach, I can
> give it a try. I just wanted to avoid a complete rework of the main
> building function.

Hrm, I see what you mean.

Especially if we can't reorganise the code so that it is always
 - things which need to be mapped
 - things which may be unmapped
so that you can switch from mapping to not mapping at one point.  But
perhaps even that would be more complicated.

Thanks for your explanation, anyway.

Ian.

Re: [PATCH 0/5] libxc: support building large pv-domains

[Juergen Gross]

Hi tools maintainers,

On 09/11/2015 02:32 PM, Juergen Gross wrote:
> The Xen hypervisor supports starting a dom0 with large memory (up to
> the TB range) by not including the initrd and p2m list in the initial
> kernel mapping. Especially the p2m list can grow larger than the
> available virtual space in the initial mapping.
>
> The started kernel is indicating the support of each feature via
> elf notes.
>
> This series enables the domain builder in libxc to do the same as the
> hypervisor. This enables starting of huge pv-domUs via xl.
>
> Unmapped initrd is supported for 64 and 32 bit domains, omitting the
> p2m from initial kernel mapping is possible for 64 bit domains only.
>
> Tested with:
> - 32 bit domU (kernel not supporting unmapped initrd)
> - 32 bit domU (kernel supporting unmapped initrd)
> - 1 GB 64 bit domU (kernel supporting unmapped initrd, not p2m)
> - 1 GB 64 bit domU (kernel supporting unmapped initrd and p2m)
> - 900GB 64 bit domU (kernel supporting unmapped initrd and p2m)
>
> Juergen Gross (5):
>    libxc: remove allocate member from struct xc_dom_image
>    libxc: do initrd processing of domain builder in own function
>    libxc: create unmapped initrd in domain builder if supported
>    libxc: split p2m allocation in domain builder from other magic pages
>    libxc: create p2m list outside of kernel mapping if supported
>
>   tools/libxc/include/xc_dom.h |   4 +-
>   tools/libxc/xc_dom_core.c    | 123 +++++++++++++++++++++++++++++--------------
>   tools/libxc/xc_dom_x86.c     | 120 ++++++++++++++++++++++++++++++++++++++++-
>   3 files changed, 204 insertions(+), 43 deletions(-)
>

Patches 1+2 got Acked-by IanJ, what about the other 3 patches?


Juergen

Re: [PATCH 3/5] libxc: create unmapped initrd in domain builder if supported

[Ian Campbell]

On Fri, 2015-09-11 at 15:39 +0200, Juergen Gross wrote:

> > > > -    /* load ramdisk */
> > > > -    if ( dom->ramdisk_blob )
> > > > +    /* Load ramdisk if initial mapping required. */
> > > > +    if ( dom->ramdisk_blob &&
> > > > +         (!dom
> > > > ->parms.elf_notes[XEN_ELFNOTE_MOD_START_PFN].data.num ||
> > > > +          dom->ramdisk_seg.vstart) )
> > > 
> > > After this patch the resulting structure of the code is rather
> > > unfortunate, in that the order of the main processing steps depends
> > > on
> > > this ELFNOTE.
> > 
> > Shouldn't we ought to have a common code ELF agnostic? I.e we may have
> > other kernel image format where we have notes but not ELF notes.
> 
> dom->parms is the same for all architectures. I think it would have to
> be extended in that case.

dom->parms is a struct elf_dom_parms which unfortunately appears to
conflate a bunch of generic stuff which can be parsed from a variety of
image types (virt_start, features, etc) and some ELF specific stuff e.g.
elf_notes.

Really this stuff ought to be split out, along those lines with dom->parms
only containing the former.

This sort of happens now in an obscure way by virtue of the /* raw */ and
/* parsed */ comments in the struct definition, libxc _only_ uses parsed
information but you are now adding a use of elf_notes which is in the raw
section. I agree with Julien that this should be avoided.

I think the easy answer would be for libelf to parse that note into an
appropriate new field which is agnostic to ELF.

Ian.

Re: [PATCH 1/5] libxc: remove allocate member from struct xc_dom_image

[Ian Campbell]

On Fri, 2015-09-11 at 13:44 +0100, Ian Jackson wrote:
> Juergen Gross writes ("[PATCH 1/5] libxc: remove allocate member from
> struct xc_dom_image"):
> > The allocate() callback in struct xc_dom_image is never set. Remove it.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Applied to staging.

This will have a trivial context conflict for Roger's series, which adds a
new field to xc_dom_image below the *allocate which is being removed here.

Re: [PATCH 2/5] libxc: do initrd processing of domain builder in own function

[Ian Campbell]

On Fri, 2015-09-11 at 13:45 +0100, Ian Jackson wrote:
> Juergen Gross writes ("[PATCH 2/5] libxc: do initrd processing of domain
> builder in own function"):
> > Factor out the initrd processing in xc_dom_build_image() into an own
> > function to prepare starting a domain with unmapped initrd.
> 
> I haven't checked that this is just code motion and the obvious
> refactoring, but it looks like it is.  On that basis
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Doesn't look to conflict with Roger's series, so applied.

Ian.

Re: [PATCH 0/5] libxc: support building large pv-domains

[Ian Campbell]

On Fri, 2015-09-11 at 16:01 +0200, Juergen Gross wrote:
> > The reason I asked is that the first thing Roger's series does is cause HVM
> > domains to be built using the PV domain builder...
> 
> Aah, okay.
> 
> OTOH I'm doing nothing different than the hypervisor when loading dom0.
> As long as the ELFNOTEs in question (or the corresponding elements in
> dom->parms) are not set, the resulting domain image should be the same
> as today.

As we all functionality wise my concern is that we are holding off on
applying Roger's series, which is AIUI ready to go, to staging until 4.6 is
released, despite staging being open for business because of how invasive
it is and the impact therefore on any backports we might need to do for
4.6.

Given that it seems unfair to take other series to the same area which came
afterwards if they are going to cause huge conflicts for Roger.

That said, I've looked at your #1 and #2 vs Roger's series and the conflict
would appear to be a trivial context one (of the sort I would routinely
fixup during commit any way). So I've applied those two.

I had a comment on #3. I'm afraid I didn't get to #4 or #5 yet.


Ian.

Re: [PATCH 1/5] libxc: remove allocate member from struct xc_dom_image

[Juergen Gross]

On 09/25/2015 05:39 PM, Ian Campbell wrote:
> On Fri, 2015-09-11 at 13:44 +0100, Ian Jackson wrote:
>> Juergen Gross writes ("[PATCH 1/5] libxc: remove allocate member from
>> struct xc_dom_image"):
>>> The allocate() callback in struct xc_dom_image is never set. Remove it.
>>
>> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
>
> Applied to staging.

Really? Can't see it there.


Juergen

>
> This will have a trivial context conflict for Roger's series, which adds a
> new field to xc_dom_image below the *allocate which is being removed here.
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

Re: [PATCH 1/5] libxc: remove allocate member from struct xc_dom_image

[Ian Campbell]

On Mon, 2015-09-28 at 05:55 +0200, Juergen Gross wrote:
> On 09/25/2015 05:39 PM, Ian Campbell wrote:
> > On Fri, 2015-09-11 at 13:44 +0100, Ian Jackson wrote:
> > > Juergen Gross writes ("[PATCH 1/5] libxc: remove allocate member from
> > > struct xc_dom_image"):
> > > > The allocate() callback in struct xc_dom_image is never set. Remove
> > > > it.
> > > 
> > > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > 
> > Applied to staging.
> 
> Really? Can't see it there.

Indeed, I seem to have somehow not actually applied it. I've put it back in
my queue folder and I'll pick it up when I next go over that.

Sorry for the noise.

Ian.

Re: [PATCH 4/5] libxc: split p2m allocation in domain builder from other magic pages

[Ian Campbell]

On Fri, 2015-09-11 at 14:32 +0200, Juergen Gross wrote:
> Add an own function to allocate the p2m list in the domain builder in

I'm not sure what you mean by "own" here. Perhaps "hook" or "callback"?

> order to prepare allocating the p2m list outside of the initial kernel
> mapping. This will be needed to support loading domains with huge
> memory (>512 GB).
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Other than the above it otherwise LGTM, so with that clarification:

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
>  tools/libxc/include/xc_dom.h |  1 +
>  tools/libxc/xc_dom_core.c    |  3 +++
>  tools/libxc/xc_dom_x86.c     | 11 ++++++++++-
>  3 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
> index a4ca8a4..43b1eab 100644
> --- a/tools/libxc/include/xc_dom.h
> +++ b/tools/libxc/include/xc_dom.h
> @@ -199,6 +199,7 @@ void xc_dom_register_loader(struct xc_dom_loader
> *loader);
>  struct xc_dom_arch {
>      /* pagetable setup */
>      int (*alloc_magic_pages) (struct xc_dom_image * dom);
> +    int (*alloc_p2m_list) (struct xc_dom_image * dom);
>      int (*count_pgtables) (struct xc_dom_image * dom);
>      int (*setup_pgtables) (struct xc_dom_image * dom);
>  
> diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
> index bb668b1..81b642e 100644
> --- a/tools/libxc/xc_dom_core.c
> +++ b/tools/libxc/xc_dom_core.c
> @@ -1048,6 +1048,9 @@ int xc_dom_build_image(struct xc_dom_image *dom)
>      }
>  
>      /* allocate other pages */
> +    if ( dom->arch_hooks->alloc_p2m_list &&
> +         dom->arch_hooks->alloc_p2m_list(dom) != 0 )
> +        goto err;
>      if ( dom->arch_hooks->alloc_magic_pages(dom) != 0 )
>          goto err;
>      if ( dom->arch_hooks->count_pgtables )
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index 3d40fa4..91d4e49 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -438,7 +438,7 @@ pfn_error:
>  
>  /* ---------------------------------------------------------------------
> --- */
>  
> -static int alloc_magic_pages(struct xc_dom_image *dom)
> +static int alloc_p2m_list(struct xc_dom_image *dom)
>  {
>      size_t p2m_alloc_size = dom->p2m_size * dom->arch_hooks->sizeof_pfn;
>  
> @@ -450,6 +450,13 @@ static int alloc_magic_pages(struct xc_dom_image
> *dom)
>      if ( dom->p2m_guest == NULL )
>          return -1;
>  
> +    return 0;
> +}
> +
> +/* ---------------------------------------------------------------------
> --- */
> +
> +static int alloc_magic_pages(struct xc_dom_image *dom)
> +{
>      /* allocate special pages */
>      dom->start_info_pfn = xc_dom_alloc_page(dom, "start info");
>      dom->xenstore_pfn = xc_dom_alloc_page(dom, "xenstore");
> @@ -676,6 +683,7 @@ static struct xc_dom_arch xc_dom_32_pae = {
>      .page_shift = PAGE_SHIFT_X86,
>      .sizeof_pfn = 4,
>      .alloc_magic_pages = alloc_magic_pages,
> +    .alloc_p2m_list = alloc_p2m_list,
>      .count_pgtables = count_pgtables_x86_32_pae,
>      .setup_pgtables = setup_pgtables_x86_32_pae,
>      .start_info = start_info_x86_32,
> @@ -689,6 +697,7 @@ static struct xc_dom_arch xc_dom_64 = {
>      .page_shift = PAGE_SHIFT_X86,
>      .sizeof_pfn = 8,
>      .alloc_magic_pages = alloc_magic_pages,
> +    .alloc_p2m_list = alloc_p2m_list,
>      .count_pgtables = count_pgtables_x86_64,
>      .setup_pgtables = setup_pgtables_x86_64,
>      .start_info = start_info_x86_64,

Re: [PATCH 4/5] libxc: split p2m allocation in domain builder from other magic pages

[Juergen Gross]

On 10/01/2015 02:47 PM, Ian Campbell wrote:
> On Fri, 2015-09-11 at 14:32 +0200, Juergen Gross wrote:
>> Add an own function to allocate the p2m list in the domain builder in
>
> I'm not sure what you mean by "own" here. Perhaps "hook" or "callback"?

Before the p2m list allocation was done in the alloc_magic_pages hook.
I'm just splitting this function.


Juergen

>
>> order to prepare allocating the p2m list outside of the initial kernel
>> mapping. This will be needed to support loading domains with huge
>> memory (>512 GB).
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>
> Other than the above it otherwise LGTM, so with that clarification:
>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
>> ---
>>   tools/libxc/include/xc_dom.h |  1 +
>>   tools/libxc/xc_dom_core.c    |  3 +++
>>   tools/libxc/xc_dom_x86.c     | 11 ++++++++++-
>>   3 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
>> index a4ca8a4..43b1eab 100644
>> --- a/tools/libxc/include/xc_dom.h
>> +++ b/tools/libxc/include/xc_dom.h
>> @@ -199,6 +199,7 @@ void xc_dom_register_loader(struct xc_dom_loader
>> *loader);
>>   struct xc_dom_arch {
>>       /* pagetable setup */
>>       int (*alloc_magic_pages) (struct xc_dom_image * dom);
>> +    int (*alloc_p2m_list) (struct xc_dom_image * dom);
>>       int (*count_pgtables) (struct xc_dom_image * dom);
>>       int (*setup_pgtables) (struct xc_dom_image * dom);
>>
>> diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
>> index bb668b1..81b642e 100644
>> --- a/tools/libxc/xc_dom_core.c
>> +++ b/tools/libxc/xc_dom_core.c
>> @@ -1048,6 +1048,9 @@ int xc_dom_build_image(struct xc_dom_image *dom)
>>       }
>>
>>       /* allocate other pages */
>> +    if ( dom->arch_hooks->alloc_p2m_list &&
>> +         dom->arch_hooks->alloc_p2m_list(dom) != 0 )
>> +        goto err;
>>       if ( dom->arch_hooks->alloc_magic_pages(dom) != 0 )
>>           goto err;
>>       if ( dom->arch_hooks->count_pgtables )
>> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
>> index 3d40fa4..91d4e49 100644
>> --- a/tools/libxc/xc_dom_x86.c
>> +++ b/tools/libxc/xc_dom_x86.c
>> @@ -438,7 +438,7 @@ pfn_error:
>>
>>   /* ---------------------------------------------------------------------
>> --- */
>>
>> -static int alloc_magic_pages(struct xc_dom_image *dom)
>> +static int alloc_p2m_list(struct xc_dom_image *dom)
>>   {
>>       size_t p2m_alloc_size = dom->p2m_size * dom->arch_hooks->sizeof_pfn;
>>
>> @@ -450,6 +450,13 @@ static int alloc_magic_pages(struct xc_dom_image
>> *dom)
>>       if ( dom->p2m_guest == NULL )
>>           return -1;
>>
>> +    return 0;
>> +}
>> +
>> +/* ---------------------------------------------------------------------
>> --- */
>> +
>> +static int alloc_magic_pages(struct xc_dom_image *dom)
>> +{
>>       /* allocate special pages */
>>       dom->start_info_pfn = xc_dom_alloc_page(dom, "start info");
>>       dom->xenstore_pfn = xc_dom_alloc_page(dom, "xenstore");
>> @@ -676,6 +683,7 @@ static struct xc_dom_arch xc_dom_32_pae = {
>>       .page_shift = PAGE_SHIFT_X86,
>>       .sizeof_pfn = 4,
>>       .alloc_magic_pages = alloc_magic_pages,
>> +    .alloc_p2m_list = alloc_p2m_list,
>>       .count_pgtables = count_pgtables_x86_32_pae,
>>       .setup_pgtables = setup_pgtables_x86_32_pae,
>>       .start_info = start_info_x86_32,
>> @@ -689,6 +697,7 @@ static struct xc_dom_arch xc_dom_64 = {
>>       .page_shift = PAGE_SHIFT_X86,
>>       .sizeof_pfn = 8,
>>       .alloc_magic_pages = alloc_magic_pages,
>> +    .alloc_p2m_list = alloc_p2m_list,
>>       .count_pgtables = count_pgtables_x86_64,
>>       .setup_pgtables = setup_pgtables_x86_64,
>>       .start_info = start_info_x86_64,
>

Re: [PATCH 4/5] libxc: split p2m allocation in domain builder from other magic pages

[Ian Campbell]

On Fri, 2015-10-02 at 05:55 +0200, Juergen Gross wrote:
> On 10/01/2015 02:47 PM, Ian Campbell wrote:
> > On Fri, 2015-09-11 at 14:32 +0200, Juergen Gross wrote:
> > > Add an own function to allocate the p2m list in the domain builder in
> > 
> > I'm not sure what you mean by "own" here. Perhaps "hook" or "callback"?
> 
> Before the p2m list allocation was done in the alloc_magic_pages hook.
> I'm just splitting this function.

I get that, but "own" is not the right word for this.

Perhaps you meant "separate (hook|function)" or "specific (hook|function)"
or something like that?

Perhaps something like:

    Refactor allocation of the p2m list out of alloc_magic_pages into a new
    alloc_p2m_list hook.

?

Ian.

> 
> 
> Juergen
> 
> > 
> > > order to prepare allocating the p2m list outside of the initial
> > > kernel
> > > mapping. This will be needed to support loading domains with huge
> > > memory (>512 GB).
> > > 
> > > Signed-off-by: Juergen Gross <jgross@suse.com>
> > 
> > Other than the above it otherwise LGTM, so with that clarification:
> > 
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > > ---
> > >   tools/libxc/include/xc_dom.h |  1 +
> > >   tools/libxc/xc_dom_core.c    |  3 +++
> > >   tools/libxc/xc_dom_x86.c     | 11 ++++++++++-
> > >   3 files changed, 14 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/libxc/include/xc_dom.h
> > > b/tools/libxc/include/xc_dom.h
> > > index a4ca8a4..43b1eab 100644
> > > --- a/tools/libxc/include/xc_dom.h
> > > +++ b/tools/libxc/include/xc_dom.h
> > > @@ -199,6 +199,7 @@ void xc_dom_register_loader(struct xc_dom_loader
> > > *loader);
> > >   struct xc_dom_arch {
> > >       /* pagetable setup */
> > >       int (*alloc_magic_pages) (struct xc_dom_image * dom);
> > > +    int (*alloc_p2m_list) (struct xc_dom_image * dom);
> > >       int (*count_pgtables) (struct xc_dom_image * dom);
> > >       int (*setup_pgtables) (struct xc_dom_image * dom);
> > > 
> > > diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
> > > index bb668b1..81b642e 100644
> > > --- a/tools/libxc/xc_dom_core.c
> > > +++ b/tools/libxc/xc_dom_core.c
> > > @@ -1048,6 +1048,9 @@ int xc_dom_build_image(struct xc_dom_image
> > > *dom)
> > >       }
> > > 
> > >       /* allocate other pages */
> > > +    if ( dom->arch_hooks->alloc_p2m_list &&
> > > +         dom->arch_hooks->alloc_p2m_list(dom) != 0 )
> > > +        goto err;
> > >       if ( dom->arch_hooks->alloc_magic_pages(dom) != 0 )
> > >           goto err;
> > >       if ( dom->arch_hooks->count_pgtables )
> > > diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> > > index 3d40fa4..91d4e49 100644
> > > --- a/tools/libxc/xc_dom_x86.c
> > > +++ b/tools/libxc/xc_dom_x86.c
> > > @@ -438,7 +438,7 @@ pfn_error:
> > > 
> > >   /* ----------------------------------------------------------------
> > > -----
> > > --- */
> > > 
> > > -static int alloc_magic_pages(struct xc_dom_image *dom)
> > > +static int alloc_p2m_list(struct xc_dom_image *dom)
> > >   {
> > >       size_t p2m_alloc_size = dom->p2m_size * dom->arch_hooks
> > > ->sizeof_pfn;
> > > 
> > > @@ -450,6 +450,13 @@ static int alloc_magic_pages(struct xc_dom_image
> > > *dom)
> > >       if ( dom->p2m_guest == NULL )
> > >           return -1;
> > > 
> > > +    return 0;
> > > +}
> > > +
> > > +/* -----------------------------------------------------------------
> > > ----
> > > --- */
> > > +
> > > +static int alloc_magic_pages(struct xc_dom_image *dom)
> > > +{
> > >       /* allocate special pages */
> > >       dom->start_info_pfn = xc_dom_alloc_page(dom, "start info");
> > >       dom->xenstore_pfn = xc_dom_alloc_page(dom, "xenstore");
> > > @@ -676,6 +683,7 @@ static struct xc_dom_arch xc_dom_32_pae = {
> > >       .page_shift = PAGE_SHIFT_X86,
> > >       .sizeof_pfn = 4,
> > >       .alloc_magic_pages = alloc_magic_pages,
> > > +    .alloc_p2m_list = alloc_p2m_list,
> > >       .count_pgtables = count_pgtables_x86_32_pae,
> > >       .setup_pgtables = setup_pgtables_x86_32_pae,
> > >       .start_info = start_info_x86_32,
> > > @@ -689,6 +697,7 @@ static struct xc_dom_arch xc_dom_64 = {
> > >       .page_shift = PAGE_SHIFT_X86,
> > >       .sizeof_pfn = 8,
> > >       .alloc_magic_pages = alloc_magic_pages,
> > > +    .alloc_p2m_list = alloc_p2m_list,
> > >       .count_pgtables = count_pgtables_x86_64,
> > >       .setup_pgtables = setup_pgtables_x86_64,
> > >       .start_info = start_info_x86_64,
> > 
> 

Re: [PATCH 4/5] libxc: split p2m allocation in domain builder from other magic pages

[Juergen Gross]

On 10/02/2015 11:04 AM, Ian Campbell wrote:
> On Fri, 2015-10-02 at 05:55 +0200, Juergen Gross wrote:
>> On 10/01/2015 02:47 PM, Ian Campbell wrote:
>>> On Fri, 2015-09-11 at 14:32 +0200, Juergen Gross wrote:
>>>> Add an own function to allocate the p2m list in the domain builder in
>>>
>>> I'm not sure what you mean by "own" here. Perhaps "hook" or "callback"?
>>
>> Before the p2m list allocation was done in the alloc_magic_pages hook.
>> I'm just splitting this function.
>
> I get that, but "own" is not the right word for this.

Obviously.

> Perhaps you meant "separate (hook|function)" or "specific (hook|function)"
> or something like that?
>
> Perhaps something like:
>
>      Refactor allocation of the p2m list out of alloc_magic_pages into a new
>      alloc_p2m_list hook.

In v2 I've changed the wording in a similar way.


Juergen

Re: [PATCH 4/5] libxc: split p2m allocation in domain builder from other magic pages

[Ian Campbell]

On Fri, 2015-10-02 at 11:14 +0200, Juergen Gross wrote:
> On 10/02/2015 11:04 AM, Ian Campbell wrote:
> > On Fri, 2015-10-02 at 05:55 +0200, Juergen Gross wrote:
> > > On 10/01/2015 02:47 PM, Ian Campbell wrote:
> > > > On Fri, 2015-09-11 at 14:32 +0200, Juergen Gross wrote:
> > > > > Add an own function to allocate the p2m list in the domain
> > > > > builder in
> > > > 
> > > > I'm not sure what you mean by "own" here. Perhaps "hook" or
> > > > "callback"?
> > > 
> > > Before the p2m list allocation was done in the alloc_magic_pages
> > > hook.
> > > I'm just splitting this function.
> > 
> > I get that, but "own" is not the right word for this.
> 
> Obviously.

It occurred to me afterwards that "into its own function" would be one way
to express this, so I see know how you got to that wording.

> > Perhaps you meant "separate (hook|function)" or "specific
> > (hook|function)"
> > or something like that?
> > 
> > Perhaps something like:
> > 
> >      Refactor allocation of the p2m list out of alloc_magic_pages into
> > a new
> >      alloc_p2m_list hook.
> 
> In v2 I've changed the wording in a similar way.

I'll go have a look.