All of lore.kernel.org
 help / color / mirror / Atom feed
* [Xen-devel] [PATCH 0/4] Don't allocate dom->p2m_host[] for translated domains
@ 2019-12-17 20:15 Andrew Cooper
  2019-12-17 20:15 ` [Xen-devel] [PATCH 1/4] tools/dombuilder: xc_dom_x86 cleanup Andrew Cooper
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Andrew Cooper @ 2019-12-17 20:15 UTC (permalink / raw)
  To: Xen-devel
  Cc: Stefano Stabellini, Julien Grall, Wei Liu, Andrew Cooper,
	Varad Gautam, Ian Jackson, Volodymyr Babchuk

Vastly drop xl's memory usage for HVM (x86 and ARM) guest construction.

See
https://lore.kernel.org/xen-devel/1562159202-11316-1-git-send-email-vrd@amazon.de/T/#u
for the origins of this work, but ultimately I think this is a far cleaner
solution to the problem.

Andrew Cooper (4):
  tools/dombuilder: xc_dom_x86 cleanup
  tools/dombuilder: Remove PV-only, mandatory hooks
  tools/dombuilder: Remove p2m_guest from the common interface
  tools/dombuilder: Don't allocate dom->p2m_host[] for translated
    domains

 stubdom/grub/kexec.c         |  36 ++++------
 tools/libxc/include/xc_dom.h |  24 +++----
 tools/libxc/xc_dom_arm.c     |  30 --------
 tools/libxc/xc_dom_boot.c    |   6 +-
 tools/libxc/xc_dom_core.c    |  43 +----------
 tools/libxc/xc_dom_x86.c     | 166 ++++++++++++++++++++++---------------------
 6 files changed, 114 insertions(+), 191 deletions(-)

-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Xen-devel] [PATCH 1/4] tools/dombuilder: xc_dom_x86 cleanup
  2019-12-17 20:15 [Xen-devel] [PATCH 0/4] Don't allocate dom->p2m_host[] for translated domains Andrew Cooper
@ 2019-12-17 20:15 ` Andrew Cooper
  2019-12-17 20:15 ` [Xen-devel] [PATCH 2/4] tools/dombuilder: Remove PV-only, mandatory hooks Andrew Cooper
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 14+ messages in thread
From: Andrew Cooper @ 2019-12-17 20:15 UTC (permalink / raw)
  To: Xen-devel
  Cc: Stefano Stabellini, Julien Grall, Wei Liu, Andrew Cooper,
	Varad Gautam, Ian Jackson, Volodymyr Babchuk

The two xc_dom_params structures for PV pagetables are never modified and can
live in .rodata.  Reduce their scope to the alloc_pgtable_*() functions which
construct xc_dom_image_x86 appropriately.

Rename {alloc,setup}_pgtables() to {alloc,setup}_pgtables_pv() to highlight
that they are PV only, and drop some _x86() suffixes from static helpers.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Varad Gautam <vrd@amazon.de>
---
 tools/libxc/xc_dom_x86.c | 60 ++++++++++++++++++++++--------------------------
 1 file changed, 28 insertions(+), 32 deletions(-)

diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index 9e279d6768..1ce3c798ef 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -103,7 +103,7 @@ struct xc_dom_image_x86 {
     unsigned n_mappings;
 #define MAPPING_MAX 2
     struct xc_dom_x86_mapping maps[MAPPING_MAX];
-    struct xc_dom_params *params;
+    const struct xc_dom_params *params;
 };
 
 /* get guest IO ABI protocol */
@@ -235,7 +235,7 @@ static int count_pgtables(struct xc_dom_image *dom, xen_vaddr_t from,
     return 0;
 }
 
-static int alloc_pgtables(struct xc_dom_image *dom)
+static int alloc_pgtables_pv(struct xc_dom_image *dom)
 {
     int pages, extra_pages;
     xen_vaddr_t try_virt_end;
@@ -268,20 +268,20 @@ static int alloc_pgtables(struct xc_dom_image *dom)
 /* ------------------------------------------------------------------------ */
 /* i386 pagetables                                                          */
 
-static struct xc_dom_params x86_32_params = {
-    .levels = PGTBL_LEVELS_I386,
-    .vaddr_mask = bits_to_mask(VIRT_BITS_I386),
-    .lvl_prot[0] = _PAGE_PRESENT|_PAGE_RW|_PAGE_ACCESSED,
-    .lvl_prot[1] = _PAGE_PRESENT|_PAGE_RW|_PAGE_ACCESSED|_PAGE_DIRTY|_PAGE_USER,
-    .lvl_prot[2] = _PAGE_PRESENT,
-};
-
 static int alloc_pgtables_x86_32_pae(struct xc_dom_image *dom)
 {
+    static const struct xc_dom_params x86_32_params = {
+        .levels = PGTBL_LEVELS_I386,
+        .vaddr_mask = bits_to_mask(VIRT_BITS_I386),
+        .lvl_prot[0] = _PAGE_PRESENT|_PAGE_RW|_PAGE_ACCESSED,
+        .lvl_prot[1] = _PAGE_PRESENT|_PAGE_RW|_PAGE_ACCESSED|_PAGE_DIRTY|_PAGE_USER,
+        .lvl_prot[2] = _PAGE_PRESENT,
+    };
     struct xc_dom_image_x86 *domx86 = dom->arch_private;
 
     domx86->params = &x86_32_params;
-    return alloc_pgtables(dom);
+
+    return alloc_pgtables_pv(dom);
 }
 
 #define pfn_to_paddr(pfn) ((xen_paddr_t)(pfn) << PAGE_SHIFT_X86)
@@ -355,7 +355,7 @@ static xen_pfn_t move_l3_below_4G(struct xc_dom_image *dom,
     return l3mfn;
 }
 
-static x86_pgentry_t *get_pg_table_x86(struct xc_dom_image *dom, int m, int l)
+static x86_pgentry_t *get_pg_table(struct xc_dom_image *dom, int m, int l)
 {
     struct xc_dom_image_x86 *domx86 = dom->arch_private;
     struct xc_dom_x86_mapping *map;
@@ -371,8 +371,7 @@ static x86_pgentry_t *get_pg_table_x86(struct xc_dom_image *dom, int m, int l)
     return NULL;
 }
 
-static x86_pgentry_t get_pg_prot_x86(struct xc_dom_image *dom, int l,
-                                     xen_pfn_t pfn)
+static x86_pgentry_t get_pg_prot(struct xc_dom_image *dom, int l, xen_pfn_t pfn)
 {
     struct xc_dom_image_x86 *domx86 = dom->arch_private;
     struct xc_dom_x86_mapping *map;
@@ -396,7 +395,7 @@ static x86_pgentry_t get_pg_prot_x86(struct xc_dom_image *dom, int l,
     return prot;
 }
 
-static int setup_pgtables_x86(struct xc_dom_image *dom)
+static int setup_pgtables_pv(struct xc_dom_image *dom)
 {
     struct xc_dom_image_x86 *domx86 = dom->arch_private;
     struct xc_dom_x86_mapping *map1, *map2;
@@ -413,7 +412,7 @@ static int setup_pgtables_x86(struct xc_dom_image *dom)
             map1 = domx86->maps + m1;
             from = map1->lvls[l].from;
             to = map1->lvls[l].to;
-            pg = get_pg_table_x86(dom, m1, l);
+            pg = get_pg_table(dom, m1, l);
             if ( !pg )
                 return -1;
             for ( m2 = 0; m2 < domx86->n_mappings; m2++ )
@@ -433,7 +432,7 @@ static int setup_pgtables_x86(struct xc_dom_image *dom)
                 for ( p = p_s; p <= p_e; p++ )
                 {
                     pg[p] = pfn_to_paddr(xc_dom_p2m(dom, pfn)) |
-                            get_pg_prot_x86(dom, l, pfn);
+                            get_pg_prot(dom, l, pfn);
                     pfn++;
                 }
             }
@@ -464,32 +463,32 @@ static int setup_pgtables_x86_32_pae(struct xc_dom_image *dom)
         }
     }
 
-    return setup_pgtables_x86(dom);
+    return setup_pgtables_pv(dom);
 }
 
 /* ------------------------------------------------------------------------ */
 /* x86_64 pagetables                                                        */
 
-static struct xc_dom_params x86_64_params = {
-    .levels = PGTBL_LEVELS_X86_64,
-    .vaddr_mask = bits_to_mask(VIRT_BITS_X86_64),
-    .lvl_prot[0] = _PAGE_PRESENT|_PAGE_RW|_PAGE_ACCESSED,
-    .lvl_prot[1] = _PAGE_PRESENT|_PAGE_RW|_PAGE_ACCESSED|_PAGE_DIRTY|_PAGE_USER,
-    .lvl_prot[2] = _PAGE_PRESENT|_PAGE_RW|_PAGE_ACCESSED|_PAGE_DIRTY|_PAGE_USER,
-    .lvl_prot[3] = _PAGE_PRESENT|_PAGE_RW|_PAGE_ACCESSED|_PAGE_DIRTY|_PAGE_USER,
-};
-
 static int alloc_pgtables_x86_64(struct xc_dom_image *dom)
 {
+    const static struct xc_dom_params x86_64_params = {
+        .levels = PGTBL_LEVELS_X86_64,
+        .vaddr_mask = bits_to_mask(VIRT_BITS_X86_64),
+        .lvl_prot[0] = _PAGE_PRESENT|_PAGE_RW|_PAGE_ACCESSED,
+        .lvl_prot[1] = _PAGE_PRESENT|_PAGE_RW|_PAGE_ACCESSED|_PAGE_DIRTY|_PAGE_USER,
+        .lvl_prot[2] = _PAGE_PRESENT|_PAGE_RW|_PAGE_ACCESSED|_PAGE_DIRTY|_PAGE_USER,
+        .lvl_prot[3] = _PAGE_PRESENT|_PAGE_RW|_PAGE_ACCESSED|_PAGE_DIRTY|_PAGE_USER,
+    };
     struct xc_dom_image_x86 *domx86 = dom->arch_private;
 
     domx86->params = &x86_64_params;
-    return alloc_pgtables(dom);
+
+    return alloc_pgtables_pv(dom);
 }
 
 static int setup_pgtables_x86_64(struct xc_dom_image *dom)
 {
-    return setup_pgtables_x86(dom);
+    return setup_pgtables_pv(dom);
 }
 
 /* ------------------------------------------------------------------------ */
@@ -1908,9 +1907,6 @@ static struct xc_dom_arch xc_hvm_32 = {
     .sizeof_pfn = 4,
     .alloc_magic_pages = alloc_magic_pages_hvm,
     .alloc_pgtables = alloc_pgtables_hvm,
-    .setup_pgtables = NULL,
-    .start_info = NULL,
-    .shared_info = NULL,
     .vcpu = vcpu_hvm,
     .meminit = meminit_hvm,
     .bootearly = bootearly,
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Xen-devel] [PATCH 2/4] tools/dombuilder: Remove PV-only, mandatory hooks
  2019-12-17 20:15 [Xen-devel] [PATCH 0/4] Don't allocate dom->p2m_host[] for translated domains Andrew Cooper
  2019-12-17 20:15 ` [Xen-devel] [PATCH 1/4] tools/dombuilder: xc_dom_x86 cleanup Andrew Cooper
@ 2019-12-17 20:15 ` Andrew Cooper
  2019-12-23 18:12   ` Julien Grall
  2019-12-17 20:15 ` [Xen-devel] [PATCH 3/4] tools/dombuilder: Remove p2m_guest from the common interface Andrew Cooper
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Andrew Cooper @ 2019-12-17 20:15 UTC (permalink / raw)
  To: Xen-devel
  Cc: Stefano Stabellini, Julien Grall, Wei Liu, Andrew Cooper,
	Varad Gautam, Ian Jackson, Volodymyr Babchuk

Currently, the setup_pgtable() hook is optional, but alloc_pgtable() hook is
not.  Both are specific to x86 PV guests, and stubbed in various ways by the
dombuilders for translated guests (x86 HVM, ARM).

Make alloc_pgtables() optional, and drop all the stubs for translated guest
types.

No change in the constructed guests.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Varad Gautam <vrd@amazon.de>
---
 tools/libxc/include/xc_dom.h |  3 ++-
 tools/libxc/xc_dom_arm.c     | 21 ---------------------
 tools/libxc/xc_dom_boot.c    |  6 +++---
 tools/libxc/xc_dom_core.c    |  3 ++-
 tools/libxc/xc_dom_x86.c     |  7 -------
 5 files changed, 7 insertions(+), 33 deletions(-)

diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
index 5900bbe8fa..9ff1cb8b07 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -253,8 +253,9 @@ void xc_dom_register_loader(struct xc_dom_loader *loader);
 /* --- arch specific hooks ----------------------------------------- */
 
 struct xc_dom_arch {
-    /* pagetable setup */
     int (*alloc_magic_pages) (struct xc_dom_image * dom);
+
+    /* pagetable setup - x86 PV only */
     int (*alloc_pgtables) (struct xc_dom_image * dom);
     int (*alloc_p2m_list) (struct xc_dom_image * dom);
     int (*setup_pgtables) (struct xc_dom_image * dom);
diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
index 5b9eca6087..7e0fb9169f 100644
--- a/tools/libxc/xc_dom_arm.c
+++ b/tools/libxc/xc_dom_arm.c
@@ -47,23 +47,6 @@ const char *xc_domain_get_native_protocol(xc_interface *xch,
 }
 
 /* ------------------------------------------------------------------------ */
-/*
- * arm guests are hybrid and start off with paging disabled, therefore no
- * pagetables and nothing to do here.
- */
-static int alloc_pgtables_arm(struct xc_dom_image *dom)
-{
-    DOMPRINTF_CALLED(dom->xch);
-    return 0;
-}
-
-static int setup_pgtables_arm(struct xc_dom_image *dom)
-{
-    DOMPRINTF_CALLED(dom->xch);
-    return 0;
-}
-
-/* ------------------------------------------------------------------------ */
 
 static int alloc_magic_pages(struct xc_dom_image *dom)
 {
@@ -539,8 +522,6 @@ static struct xc_dom_arch xc_dom_32 = {
     .page_shift = PAGE_SHIFT_ARM,
     .sizeof_pfn = 8,
     .alloc_magic_pages = alloc_magic_pages,
-    .alloc_pgtables = alloc_pgtables_arm,
-    .setup_pgtables = setup_pgtables_arm,
     .start_info = start_info_arm,
     .shared_info = shared_info_arm,
     .vcpu = vcpu_arm32,
@@ -555,8 +536,6 @@ static struct xc_dom_arch xc_dom_64 = {
     .page_shift = PAGE_SHIFT_ARM,
     .sizeof_pfn = 8,
     .alloc_magic_pages = alloc_magic_pages,
-    .alloc_pgtables = alloc_pgtables_arm,
-    .setup_pgtables = setup_pgtables_arm,
     .start_info = start_info_arm,
     .shared_info = shared_info_arm,
     .vcpu = vcpu_arm64,
diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
index 918ee4d045..79dbbf6571 100644
--- a/tools/libxc/xc_dom_boot.c
+++ b/tools/libxc/xc_dom_boot.c
@@ -199,9 +199,9 @@ int xc_dom_boot_image(struct xc_dom_image *dom)
     /* initial mm setup */
     if ( (rc = xc_dom_update_guest_p2m(dom)) != 0 )
         return rc;
-    if ( dom->arch_hooks->setup_pgtables )
-        if ( (rc = dom->arch_hooks->setup_pgtables(dom)) != 0 )
-            return rc;
+    if ( dom->arch_hooks->setup_pgtables &&
+         (rc = dom->arch_hooks->setup_pgtables(dom)) != 0 )
+        return rc;
 
     /* start info page */
     if ( dom->arch_hooks->start_info )
diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
index 9bd04cb2d5..fc77804a7e 100644
--- a/tools/libxc/xc_dom_core.c
+++ b/tools/libxc/xc_dom_core.c
@@ -1247,7 +1247,8 @@ int xc_dom_build_image(struct xc_dom_image *dom)
         goto err;
     if ( dom->arch_hooks->alloc_magic_pages(dom) != 0 )
         goto err;
-    if ( dom->arch_hooks->alloc_pgtables(dom) != 0 )
+    if ( dom->arch_hooks->alloc_pgtables &&
+         dom->arch_hooks->alloc_pgtables(dom) != 0 )
         goto err;
     if ( dom->alloc_bootstack )
     {
diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index 1ce3c798ef..d2acff1061 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -1690,12 +1690,6 @@ static int bootlate_pv(struct xc_dom_image *dom)
     return 0;
 }
 
-static int alloc_pgtables_hvm(struct xc_dom_image *dom)
-{
-    DOMPRINTF("%s: doing nothing", __func__);
-    return 0;
-}
-
 /*
  * The memory layout of the start_info page and the modules, and where the
  * addresses are stored:
@@ -1906,7 +1900,6 @@ static struct xc_dom_arch xc_hvm_32 = {
     .page_shift = PAGE_SHIFT_X86,
     .sizeof_pfn = 4,
     .alloc_magic_pages = alloc_magic_pages_hvm,
-    .alloc_pgtables = alloc_pgtables_hvm,
     .vcpu = vcpu_hvm,
     .meminit = meminit_hvm,
     .bootearly = bootearly,
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Xen-devel] [PATCH 3/4] tools/dombuilder: Remove p2m_guest from the common interface
  2019-12-17 20:15 [Xen-devel] [PATCH 0/4] Don't allocate dom->p2m_host[] for translated domains Andrew Cooper
  2019-12-17 20:15 ` [Xen-devel] [PATCH 1/4] tools/dombuilder: xc_dom_x86 cleanup Andrew Cooper
  2019-12-17 20:15 ` [Xen-devel] [PATCH 2/4] tools/dombuilder: Remove PV-only, mandatory hooks Andrew Cooper
@ 2019-12-17 20:15 ` Andrew Cooper
  2019-12-17 20:15 ` [Xen-devel] [PATCH 4/4] tools/dombuilder: Don't allocate dom->p2m_host[] for translated domains Andrew Cooper
  2019-12-31 16:37 ` [Xen-devel] [PATCH 0/4] " Wei Liu
  4 siblings, 0 replies; 14+ messages in thread
From: Andrew Cooper @ 2019-12-17 20:15 UTC (permalink / raw)
  To: Xen-devel
  Cc: Stefano Stabellini, Julien Grall, Wei Liu, Andrew Cooper,
	Varad Gautam, Ian Jackson, Volodymyr Babchuk

In-guest p2m's are a concept specific to x86 PV guests.  alloc_p2m_list() is
the only hook which initialises dom->p2m_guest, making
xc_dom_update_guest_p2m() a nop for non-PV guests.

Move p2m_guest into xc_dom_image_x86 and adjust alloc_p2m_list() to match.

Drop xc_dom_update_guest_p2m() entirely.

One caller, move_l3_below_4G(), only uses it to modify a single entry, so
rewriting the whole guest p2m is wasteful - opencode the single update
instead.  The other caller is common code.  Instead, move the logic into the
setup_pgtables() hooks, which know their own sizeof_pfn and can do away with
the switch statement.

No change in the constructed guests.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Varad Gautam <vrd@amazon.de>
---
 stubdom/grub/kexec.c         |  8 --------
 tools/libxc/include/xc_dom.h |  2 --
 tools/libxc/xc_dom_boot.c    |  2 --
 tools/libxc/xc_dom_core.c    | 40 ----------------------------------------
 tools/libxc/xc_dom_x86.c     | 41 +++++++++++++++++++++++++++++++++++------
 5 files changed, 35 insertions(+), 58 deletions(-)

diff --git a/stubdom/grub/kexec.c b/stubdom/grub/kexec.c
index 61ca082d42..10891eabcc 100644
--- a/stubdom/grub/kexec.c
+++ b/stubdom/grub/kexec.c
@@ -320,14 +320,6 @@ void kexec(void *kernel, long kernel_size, void *module, long module_size, char
     do_exchange(dom, PHYS_PFN(_boot_target - dom->parms.virt_base),
             virt_to_mfn(&_boot_page));
 
-    /* Make sure the bootstrap page table does not RW-map any of our current
-     * page table frames */
-    if ( (rc = xc_dom_update_guest_p2m(dom))) {
-        printk("xc_dom_update_guest_p2m returned %d\n", rc);
-        errnum = ERR_BOOT_FAILURE;
-        goto out;
-    }
-
     if ( dom->arch_hooks->setup_pgtables )
         if ( (rc = dom->arch_hooks->setup_pgtables(dom))) {
             printk("setup_pgtables returned %d\n", rc);
diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
index 9ff1cb8b07..b7d0faf7e1 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -133,7 +133,6 @@ struct xc_dom_image {
      * Note that the input is offset by rambase.
      */
     xen_pfn_t *p2m_host;
-    void *p2m_guest;
 
     /* physical memory
      *
@@ -331,7 +330,6 @@ int xc_dom_devicetree_mem(struct xc_dom_image *dom, const void *mem,
 int xc_dom_parse_image(struct xc_dom_image *dom);
 int xc_dom_set_arch_hooks(struct xc_dom_image *dom);
 int xc_dom_build_image(struct xc_dom_image *dom);
-int xc_dom_update_guest_p2m(struct xc_dom_image *dom);
 
 int xc_dom_boot_xen_init(struct xc_dom_image *dom, xc_interface *xch,
                          uint32_t domid);
diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
index 79dbbf6571..bb599b33ba 100644
--- a/tools/libxc/xc_dom_boot.c
+++ b/tools/libxc/xc_dom_boot.c
@@ -197,8 +197,6 @@ int xc_dom_boot_image(struct xc_dom_image *dom)
         return -1;
 
     /* initial mm setup */
-    if ( (rc = xc_dom_update_guest_p2m(dom)) != 0 )
-        return rc;
     if ( dom->arch_hooks->setup_pgtables &&
          (rc = dom->arch_hooks->setup_pgtables(dom)) != 0 )
         return rc;
diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
index fc77804a7e..f30c73b5e8 100644
--- a/tools/libxc/xc_dom_core.c
+++ b/tools/libxc/xc_dom_core.c
@@ -969,46 +969,6 @@ int xc_dom_mem_init(struct xc_dom_image *dom, unsigned int mem_mb)
     return 0;
 }
 
-int xc_dom_update_guest_p2m(struct xc_dom_image *dom)
-{
-    uint32_t *p2m_32;
-    uint64_t *p2m_64;
-    xen_pfn_t i;
-
-    if ( !dom->p2m_guest )
-        return 0;
-
-    switch ( dom->arch_hooks->sizeof_pfn )
-    {
-    case 4:
-        DOMPRINTF("%s: dst 32bit, pages 0x%" PRIpfn "",
-                  __FUNCTION__, dom->p2m_size);
-        p2m_32 = dom->p2m_guest;
-        for ( i = 0; i < dom->p2m_size; i++ )
-            if ( dom->p2m_host[i] != INVALID_PFN )
-                p2m_32[i] = dom->p2m_host[i];
-            else
-                p2m_32[i] = (uint32_t) - 1;
-        break;
-    case 8:
-        DOMPRINTF("%s: dst 64bit, pages 0x%" PRIpfn "",
-                  __FUNCTION__, dom->p2m_size);
-        p2m_64 = dom->p2m_guest;
-        for ( i = 0; i < dom->p2m_size; i++ )
-            if ( dom->p2m_host[i] != INVALID_PFN )
-                p2m_64[i] = dom->p2m_host[i];
-            else
-                p2m_64[i] = (uint64_t) - 1;
-        break;
-    default:
-        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
-                     "sizeof_pfn is invalid (is %d, can be 4 or 8)",
-                     dom->arch_hooks->sizeof_pfn);
-        return -1;
-    }
-    return 0;
-}
-
 static int xc_dom_build_module(struct xc_dom_image *dom, unsigned int mod)
 {
     size_t unziplen, modulelen;
diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index d2acff1061..f21662c8b9 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -104,6 +104,9 @@ struct xc_dom_image_x86 {
 #define MAPPING_MAX 2
     struct xc_dom_x86_mapping maps[MAPPING_MAX];
     const struct xc_dom_params *params;
+
+    /* PV: Pointer to the in-guest P2M. */
+    void *p2m_guest;
 };
 
 /* get guest IO ABI protocol */
@@ -296,6 +299,8 @@ static xen_pfn_t move_l3_below_4G(struct xc_dom_image *dom,
                                   xen_pfn_t l3pfn,
                                   xen_pfn_t l3mfn)
 {
+    struct xc_dom_image_x86 *domx86 = dom->arch_private;
+    uint32_t *p2m_guest = domx86->p2m_guest;
     xen_pfn_t new_l3mfn;
     struct xc_mmu *mmu;
     void *l3tab;
@@ -313,9 +318,7 @@ static xen_pfn_t move_l3_below_4G(struct xc_dom_image *dom,
     if ( !new_l3mfn )
         goto out;
 
-    dom->p2m_host[l3pfn] = new_l3mfn;
-    if ( xc_dom_update_guest_p2m(dom) != 0 )
-        goto out;
+    p2m_guest[l3pfn] = dom->p2m_host[l3pfn] = new_l3mfn;
 
     if ( xc_add_mmu_update(dom->xch, mmu,
                            (((unsigned long long)new_l3mfn)
@@ -444,7 +447,17 @@ static int setup_pgtables_pv(struct xc_dom_image *dom)
 static int setup_pgtables_x86_32_pae(struct xc_dom_image *dom)
 {
     struct xc_dom_image_x86 *domx86 = dom->arch_private;
-    xen_pfn_t l3mfn, l3pfn;
+    uint32_t *p2m_guest = domx86->p2m_guest;
+    xen_pfn_t l3mfn, l3pfn, i;
+
+    /* Copy dom->p2m_host[] into the guest. */
+    for ( i = 0; i < dom->p2m_size; ++i )
+    {
+        if ( dom->p2m_host[i] != INVALID_PFN )
+            p2m_guest[i] = dom->p2m_host[i];
+        else
+            p2m_guest[i] = -1;
+    }
 
     l3pfn = domx86->maps[0].lvls[2].pfn;
     l3mfn = xc_dom_p2m(dom, l3pfn);
@@ -488,6 +501,19 @@ static int alloc_pgtables_x86_64(struct xc_dom_image *dom)
 
 static int setup_pgtables_x86_64(struct xc_dom_image *dom)
 {
+    struct xc_dom_image_x86 *domx86 = dom->arch_private;
+    uint64_t *p2m_guest = domx86->p2m_guest;
+    xen_pfn_t i;
+
+    /* Copy dom->p2m_host[] into the guest. */
+    for ( i = 0; i < dom->p2m_size; ++i )
+    {
+        if ( dom->p2m_host[i] != INVALID_PFN )
+            p2m_guest[i] = dom->p2m_host[i];
+        else
+            p2m_guest[i] = -1;
+    }
+
     return setup_pgtables_pv(dom);
 }
 
@@ -495,11 +521,14 @@ static int setup_pgtables_x86_64(struct xc_dom_image *dom)
 
 static int alloc_p2m_list(struct xc_dom_image *dom, size_t p2m_alloc_size)
 {
+    struct xc_dom_image_x86 *domx86 = dom->arch_private;
+
     if ( xc_dom_alloc_segment(dom, &dom->p2m_seg, "phys2mach",
                               0, p2m_alloc_size) )
         return -1;
-    dom->p2m_guest = xc_dom_seg_to_ptr(dom, &dom->p2m_seg);
-    if ( dom->p2m_guest == NULL )
+
+    domx86->p2m_guest = xc_dom_seg_to_ptr(dom, &dom->p2m_seg);
+    if ( domx86->p2m_guest == NULL )
         return -1;
 
     return 0;
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Xen-devel] [PATCH 4/4] tools/dombuilder: Don't allocate dom->p2m_host[] for translated domains
  2019-12-17 20:15 [Xen-devel] [PATCH 0/4] Don't allocate dom->p2m_host[] for translated domains Andrew Cooper
                   ` (2 preceding siblings ...)
  2019-12-17 20:15 ` [Xen-devel] [PATCH 3/4] tools/dombuilder: Remove p2m_guest from the common interface Andrew Cooper
@ 2019-12-17 20:15 ` Andrew Cooper
  2019-12-23 18:23   ` Julien Grall
  2020-01-03 14:25   ` Jan Beulich
  2019-12-31 16:37 ` [Xen-devel] [PATCH 0/4] " Wei Liu
  4 siblings, 2 replies; 14+ messages in thread
From: Andrew Cooper @ 2019-12-17 20:15 UTC (permalink / raw)
  To: Xen-devel
  Cc: Stefano Stabellini, Julien Grall, Wei Liu, Andrew Cooper,
	Varad Gautam, Ian Jackson, Volodymyr Babchuk

xc_dom_p2m() and dom->p2m_host[] implement a linear transform for translated
domains, but waste a substantial chunk of RAM doing so.

ARM literally never reads dom->p2m_host[] (because of the xc_dom_translated()
short circuit in xc_dom_p2m()).  Drop it all.

x86 HVM does use dom->p2m_host[] for xc_domain_populate_physmap_exact() calls
when populating 4k pages.  Reuse the same tactic from 2M/1G ranges and use an
on-stack array instead.  Drop the memory allocation.

x86 PV guests do use dom->p2m_host[] as a non-identity transform.  Rename the
field to pv_p2m to make it clear it is PV-only.

No change in the constructed guests.

Reported-by: Varad Gautam <vrd@amazon.de>
Reported-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Varad Gautam <vrd@amazon.de>
---
 stubdom/grub/kexec.c         | 28 ++++++++---------
 tools/libxc/include/xc_dom.h | 19 ++++++------
 tools/libxc/xc_dom_arm.c     |  9 ------
 tools/libxc/xc_dom_x86.c     | 72 ++++++++++++++++++--------------------------
 4 files changed, 52 insertions(+), 76 deletions(-)

diff --git a/stubdom/grub/kexec.c b/stubdom/grub/kexec.c
index 10891eabcc..0e68b969a2 100644
--- a/stubdom/grub/kexec.c
+++ b/stubdom/grub/kexec.c
@@ -87,17 +87,17 @@ static void do_exchange(struct xc_dom_image *dom, xen_pfn_t target_pfn, xen_pfn_
     xen_pfn_t target_mfn;
 
     for (source_pfn = 0; source_pfn < start_info.nr_pages; source_pfn++)
-        if (dom->p2m_host[source_pfn] == source_mfn)
+        if (dom->pv_p2m[source_pfn] == source_mfn)
             break;
     ASSERT(source_pfn < start_info.nr_pages);
 
-    target_mfn = dom->p2m_host[target_pfn];
+    target_mfn = dom->pv_p2m[target_pfn];
 
     /* Put target MFN at source PFN */
-    dom->p2m_host[source_pfn] = target_mfn;
+    dom->pv_p2m[source_pfn] = target_mfn;
 
     /* Put source MFN at target PFN */
-    dom->p2m_host[target_pfn] = source_mfn;
+    dom->pv_p2m[target_pfn] = source_mfn;
 }
 
 int kexec_allocate(struct xc_dom_image *dom)
@@ -110,7 +110,7 @@ int kexec_allocate(struct xc_dom_image *dom)
     pages_moved2pfns = realloc(pages_moved2pfns, new_allocated * sizeof(*pages_moved2pfns));
     for (i = allocated; i < new_allocated; i++) {
         /* Exchange old page of PFN i with a newly allocated page.  */
-        xen_pfn_t old_mfn = dom->p2m_host[i];
+        xen_pfn_t old_mfn = dom->pv_p2m[i];
         xen_pfn_t new_pfn;
         xen_pfn_t new_mfn;
 
@@ -122,7 +122,7 @@ int kexec_allocate(struct xc_dom_image *dom)
 	/*
 	 * If PFN of newly allocated page (new_pfn) is less then currently
 	 * requested PFN (i) then look for relevant PFN/MFN pair. In this
-	 * situation dom->p2m_host[new_pfn] no longer contains proper MFN
+	 * situation dom->pv_p2m[new_pfn] no longer contains proper MFN
 	 * because original page with new_pfn was moved earlier
 	 * to different location.
 	 */
@@ -132,10 +132,10 @@ int kexec_allocate(struct xc_dom_image *dom)
 	pages_moved2pfns[i] = new_pfn;
 
         /* Put old page at new PFN */
-        dom->p2m_host[new_pfn] = old_mfn;
+        dom->pv_p2m[new_pfn] = old_mfn;
 
         /* Put new page at PFN i */
-        dom->p2m_host[i] = new_mfn;
+        dom->pv_p2m[i] = new_mfn;
     }
 
     allocated = new_allocated;
@@ -282,11 +282,11 @@ void kexec(void *kernel, long kernel_size, void *module, long module_size, char
     dom->p2m_size = dom->total_pages;
 
     /* setup initial p2m */
-    dom->p2m_host = malloc(sizeof(*dom->p2m_host) * dom->p2m_size);
+    dom->pv_p2m = malloc(sizeof(*dom->pv_p2m) * dom->p2m_size);
 
     /* Start with our current P2M */
     for (i = 0; i < dom->p2m_size; i++)
-        dom->p2m_host[i] = pfn_to_mfn(i);
+        dom->pv_p2m[i] = pfn_to_mfn(i);
 
     if ( (rc = xc_dom_build_image(dom)) != 0 ) {
         printk("xc_dom_build_image returned %d\n", rc);
@@ -373,7 +373,7 @@ void kexec(void *kernel, long kernel_size, void *module, long module_size, char
     _boot_oldpdmfn = virt_to_mfn(start_info.pt_base);
     DEBUG("boot old pd mfn %lx\n", _boot_oldpdmfn);
     DEBUG("boot pd virt %lx\n", dom->pgtables_seg.vstart);
-    _boot_pdmfn = dom->p2m_host[PHYS_PFN(dom->pgtables_seg.vstart - dom->parms.virt_base)];
+    _boot_pdmfn = dom->pv_p2m[PHYS_PFN(dom->pgtables_seg.vstart - dom->parms.virt_base)];
     DEBUG("boot pd mfn %lx\n", _boot_pdmfn);
     _boot_stack = _boot_target + PAGE_SIZE;
     DEBUG("boot stack %lx\n", _boot_stack);
@@ -384,13 +384,13 @@ void kexec(void *kernel, long kernel_size, void *module, long module_size, char
 
     /* Keep only useful entries */
     for (nr_m2p_updates = pfn = 0; pfn < start_info.nr_pages; pfn++)
-        if (dom->p2m_host[pfn] != pfn_to_mfn(pfn))
+        if (dom->pv_p2m[pfn] != pfn_to_mfn(pfn))
             nr_m2p_updates++;
 
     m2p_updates = malloc(sizeof(*m2p_updates) * nr_m2p_updates);
     for (i = pfn = 0; pfn < start_info.nr_pages; pfn++)
-        if (dom->p2m_host[pfn] != pfn_to_mfn(pfn)) {
-            m2p_updates[i].ptr = PFN_PHYS(dom->p2m_host[pfn]) | MMU_MACHPHYS_UPDATE;
+        if (dom->pv_p2m[pfn] != pfn_to_mfn(pfn)) {
+            m2p_updates[i].ptr = PFN_PHYS(dom->pv_p2m[pfn]) | MMU_MACHPHYS_UPDATE;
             m2p_updates[i].val = pfn;
             i++;
         }
diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
index b7d0faf7e1..d2e316f35e 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -123,16 +123,12 @@ struct xc_dom_image {
 
     /* other state info */
     uint32_t f_active[XENFEAT_NR_SUBMAPS];
+
     /*
-     * p2m_host maps guest physical addresses an offset from
-     * rambase_pfn (see below) into gfns.
-     *
-     * For a pure PV guest this means that it maps GPFNs into MFNs for
-     * a hybrid guest this means that it maps GPFNs to GPFNS.
-     *
-     * Note that the input is offset by rambase.
+     * pv_p2m is specific to x86 PV guests, and maps GFNs to MFNs.  It is
+     * eventually copied into guest context.
      */
-    xen_pfn_t *p2m_host;
+    xen_pfn_t *pv_p2m;
 
     /* physical memory
      *
@@ -433,9 +429,12 @@ static inline xen_pfn_t xc_dom_p2m(struct xc_dom_image *dom, xen_pfn_t pfn)
 {
     if ( xc_dom_translated(dom) )
         return pfn;
-    if (pfn < dom->rambase_pfn || pfn >= dom->rambase_pfn + dom->total_pages)
+
+    /* x86 PV only now. */
+    if ( pfn >= dom->total_pages )
         return INVALID_MFN;
-    return dom->p2m_host[pfn - dom->rambase_pfn];
+
+    return dom->pv_p2m[pfn];
 }
 
 #endif /* _XC_DOM_H */
diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
index 7e0fb9169f..931404c222 100644
--- a/tools/libxc/xc_dom_arm.c
+++ b/tools/libxc/xc_dom_arm.c
@@ -348,9 +348,6 @@ static int populate_guest_memory(struct xc_dom_image *dom,
         }
     }
 
-    for ( pfn = 0; pfn < nr_pfns; pfn++ )
-        dom->p2m_host[pfn] = base_pfn + pfn;
-
 out:
     free(extents);
     return rc < 0 ? rc : 0;
@@ -359,7 +356,6 @@ static int populate_guest_memory(struct xc_dom_image *dom,
 static int meminit(struct xc_dom_image *dom)
 {
     int i, rc;
-    xen_pfn_t pfn;
     uint64_t modbase;
 
     uint64_t ramsize = (uint64_t)dom->total_pages << XC_PAGE_SHIFT;
@@ -423,11 +419,6 @@ static int meminit(struct xc_dom_image *dom)
     assert(ramsize == 0); /* Too much RAM is rejected above */
 
     dom->p2m_size = p2m_size;
-    dom->p2m_host = xc_dom_malloc(dom, sizeof(xen_pfn_t) * p2m_size);
-    if ( dom->p2m_host == NULL )
-        return -EINVAL;
-    for ( pfn = 0; pfn < p2m_size; pfn++ )
-        dom->p2m_host[pfn] = INVALID_PFN;
 
     /* setup initial p2m and allocate guest memory */
     for ( i = 0; i < GUEST_RAM_BANKS && dom->rambank_size[i]; i++ )
diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index f21662c8b9..819afcb03f 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -318,7 +318,7 @@ static xen_pfn_t move_l3_below_4G(struct xc_dom_image *dom,
     if ( !new_l3mfn )
         goto out;
 
-    p2m_guest[l3pfn] = dom->p2m_host[l3pfn] = new_l3mfn;
+    p2m_guest[l3pfn] = dom->pv_p2m[l3pfn] = new_l3mfn;
 
     if ( xc_add_mmu_update(dom->xch, mmu,
                            (((unsigned long long)new_l3mfn)
@@ -450,11 +450,11 @@ static int setup_pgtables_x86_32_pae(struct xc_dom_image *dom)
     uint32_t *p2m_guest = domx86->p2m_guest;
     xen_pfn_t l3mfn, l3pfn, i;
 
-    /* Copy dom->p2m_host[] into the guest. */
+    /* Copy dom->pv_p2m[] into the guest. */
     for ( i = 0; i < dom->p2m_size; ++i )
     {
-        if ( dom->p2m_host[i] != INVALID_PFN )
-            p2m_guest[i] = dom->p2m_host[i];
+        if ( dom->pv_p2m[i] != INVALID_PFN )
+            p2m_guest[i] = dom->pv_p2m[i];
         else
             p2m_guest[i] = -1;
     }
@@ -505,11 +505,11 @@ static int setup_pgtables_x86_64(struct xc_dom_image *dom)
     uint64_t *p2m_guest = domx86->p2m_guest;
     xen_pfn_t i;
 
-    /* Copy dom->p2m_host[] into the guest. */
+    /* Copy dom->pv_p2m[] into the guest. */
     for ( i = 0; i < dom->p2m_size; ++i )
     {
-        if ( dom->p2m_host[i] != INVALID_PFN )
-            p2m_guest[i] = dom->p2m_host[i];
+        if ( dom->pv_p2m[i] != INVALID_PFN )
+            p2m_guest[i] = dom->pv_p2m[i];
         else
             p2m_guest[i] = -1;
     }
@@ -1245,11 +1245,11 @@ static int meminit_pv(struct xc_dom_image *dom)
         return -EINVAL;
     }
 
-    dom->p2m_host = xc_dom_malloc(dom, sizeof(xen_pfn_t) * dom->p2m_size);
-    if ( dom->p2m_host == NULL )
+    dom->pv_p2m = xc_dom_malloc(dom, sizeof(xen_pfn_t) * dom->p2m_size);
+    if ( dom->pv_p2m == NULL )
         return -EINVAL;
     for ( pfn = 0; pfn < dom->p2m_size; pfn++ )
-        dom->p2m_host[pfn] = INVALID_PFN;
+        dom->pv_p2m[pfn] = INVALID_PFN;
 
     /* allocate guest memory */
     for ( i = 0; i < nr_vmemranges; i++ )
@@ -1269,7 +1269,7 @@ static int meminit_pv(struct xc_dom_image *dom)
         pfn_base = vmemranges[i].start >> PAGE_SHIFT;
 
         for ( pfn = pfn_base; pfn < pfn_base+pages; pfn++ )
-            dom->p2m_host[pfn] = pfn;
+            dom->pv_p2m[pfn] = pfn;
 
         pfn_base_idx = pfn_base;
         while ( super_pages ) {
@@ -1279,7 +1279,7 @@ static int meminit_pv(struct xc_dom_image *dom)
             for ( pfn = pfn_base_idx, j = 0;
                   pfn < pfn_base_idx + (count << SUPERPAGE_2MB_SHIFT);
                   pfn += SUPERPAGE_2MB_NR_PFNS, j++ )
-                extents[j] = dom->p2m_host[pfn];
+                extents[j] = dom->pv_p2m[pfn];
             rc = xc_domain_populate_physmap(dom->xch, dom->guest_domid, count,
                                             SUPERPAGE_2MB_SHIFT, memflags,
                                             extents);
@@ -1292,7 +1292,7 @@ static int meminit_pv(struct xc_dom_image *dom)
             {
                 mfn = extents[j];
                 for ( k = 0; k < SUPERPAGE_2MB_NR_PFNS; k++, pfn++ )
-                    dom->p2m_host[pfn] = mfn + k;
+                    dom->pv_p2m[pfn] = mfn + k;
             }
             pfn_base_idx = pfn;
         }
@@ -1301,7 +1301,7 @@ static int meminit_pv(struct xc_dom_image *dom)
         {
             allocsz = min_t(uint64_t, 1024 * 1024, pages - j);
             rc = xc_domain_populate_physmap_exact(dom->xch, dom->guest_domid,
-                     allocsz, 0, memflags, &dom->p2m_host[pfn_base + j]);
+                     allocsz, 0, memflags, &dom->pv_p2m[pfn_base + j]);
 
             if ( rc )
             {
@@ -1428,25 +1428,6 @@ static int meminit_hvm(struct xc_dom_image *dom)
     }
 
     dom->p2m_size = p2m_size;
-    dom->p2m_host = xc_dom_malloc(dom, sizeof(xen_pfn_t) *
-                                      dom->p2m_size);
-    if ( dom->p2m_host == NULL )
-    {
-        DOMPRINTF("Could not allocate p2m");
-        goto error_out;
-    }
-
-    for ( i = 0; i < p2m_size; i++ )
-        dom->p2m_host[i] = ((xen_pfn_t)-1);
-    for ( vmemid = 0; vmemid < nr_vmemranges; vmemid++ )
-    {
-        uint64_t pfn;
-
-        for ( pfn = vmemranges[vmemid].start >> PAGE_SHIFT;
-              pfn < vmemranges[vmemid].end >> PAGE_SHIFT;
-              pfn++ )
-            dom->p2m_host[pfn] = pfn;
-    }
 
     /*
      * Try to claim pages for early warning of insufficient memory available.
@@ -1488,14 +1469,16 @@ static int meminit_hvm(struct xc_dom_image *dom)
      * We attempt to allocate 1GB pages if possible. It falls back on 2MB
      * pages if 1GB allocation fails. 4KB pages will be used eventually if
      * both fail.
-     * 
-     * Under 2MB mode, we allocate pages in batches of no more than 8MB to 
-     * ensure that we can be preempted and hence dom0 remains responsive.
      */
     if ( dom->device_model )
     {
+        xen_pfn_t extents[0xa0];
+
+        for ( i = 0; i < ARRAY_SIZE(extents); ++i )
+            extents[i] = i;
+
         rc = xc_domain_populate_physmap_exact(
-            xch, domid, 0xa0, 0, memflags, &dom->p2m_host[0x00]);
+            xch, domid, 0xa0, 0, memflags, extents);
         if ( rc != 0 )
         {
             DOMPRINTF("Could not populate low memory (< 0xA0).\n");
@@ -1538,7 +1521,7 @@ static int meminit_hvm(struct xc_dom_image *dom)
             if ( count > max_pages )
                 count = max_pages;
 
-            cur_pfn = dom->p2m_host[cur_pages];
+            cur_pfn = cur_pages;
 
             /* Take care the corner cases of super page tails */
             if ( ((cur_pfn & (SUPERPAGE_1GB_NR_PFNS-1)) != 0) &&
@@ -1564,8 +1547,7 @@ static int meminit_hvm(struct xc_dom_image *dom)
                 xen_pfn_t sp_extents[nr_extents];
 
                 for ( i = 0; i < nr_extents; i++ )
-                    sp_extents[i] =
-                        dom->p2m_host[cur_pages+(i<<SUPERPAGE_1GB_SHIFT)];
+                    sp_extents[i] = cur_pages + (i << SUPERPAGE_1GB_SHIFT);
 
                 done = xc_domain_populate_physmap(xch, domid, nr_extents,
                                                   SUPERPAGE_1GB_SHIFT,
@@ -1604,8 +1586,7 @@ static int meminit_hvm(struct xc_dom_image *dom)
                     xen_pfn_t sp_extents[nr_extents];
 
                     for ( i = 0; i < nr_extents; i++ )
-                        sp_extents[i] =
-                            dom->p2m_host[cur_pages+(i<<SUPERPAGE_2MB_SHIFT)];
+                        sp_extents[i] = cur_pages + (i << SUPERPAGE_2MB_SHIFT);
 
                     done = xc_domain_populate_physmap(xch, domid, nr_extents,
                                                       SUPERPAGE_2MB_SHIFT,
@@ -1624,8 +1605,13 @@ static int meminit_hvm(struct xc_dom_image *dom)
             /* Fall back to 4kB extents. */
             if ( count != 0 )
             {
+                xen_pfn_t extents[count];
+
+                for ( i = 0; i < count; ++i )
+                    extents[i] = cur_pages + i;
+
                 rc = xc_domain_populate_physmap_exact(
-                    xch, domid, count, 0, new_memflags, &dom->p2m_host[cur_pages]);
+                    xch, domid, count, 0, new_memflags, extents);
                 cur_pages += count;
                 stat_normal_pages += count;
             }
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [Xen-devel] [PATCH 2/4] tools/dombuilder: Remove PV-only, mandatory hooks
  2019-12-17 20:15 ` [Xen-devel] [PATCH 2/4] tools/dombuilder: Remove PV-only, mandatory hooks Andrew Cooper
@ 2019-12-23 18:12   ` Julien Grall
  0 siblings, 0 replies; 14+ messages in thread
From: Julien Grall @ 2019-12-23 18:12 UTC (permalink / raw)
  To: Andrew Cooper, Xen-devel
  Cc: Ian Jackson, Varad Gautam, Stefano Stabellini, Volodymyr Babchuk,
	Wei Liu

Hi Andrew,

On 17/12/2019 21:15, Andrew Cooper wrote:
> Currently, the setup_pgtable() hook is optional, but alloc_pgtable() hook is
> not.  Both are specific to x86 PV guests, and stubbed in various ways by the
> dombuilders for translated guests (x86 HVM, ARM).
> 
> Make alloc_pgtables() optional, and drop all the stubs for translated guest
> types.
> 
> No change in the constructed guests.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Cheers,

> ---
> CC: Ian Jackson <Ian.Jackson@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Varad Gautam <vrd@amazon.de>
> ---
>   tools/libxc/include/xc_dom.h |  3 ++-
>   tools/libxc/xc_dom_arm.c     | 21 ---------------------
>   tools/libxc/xc_dom_boot.c    |  6 +++---
>   tools/libxc/xc_dom_core.c    |  3 ++-
>   tools/libxc/xc_dom_x86.c     |  7 -------
>   5 files changed, 7 insertions(+), 33 deletions(-)
> 
> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
> index 5900bbe8fa..9ff1cb8b07 100644
> --- a/tools/libxc/include/xc_dom.h
> +++ b/tools/libxc/include/xc_dom.h
> @@ -253,8 +253,9 @@ void xc_dom_register_loader(struct xc_dom_loader *loader);
>   /* --- arch specific hooks ----------------------------------------- */
>   
>   struct xc_dom_arch {
> -    /* pagetable setup */
>       int (*alloc_magic_pages) (struct xc_dom_image * dom);
> +
> +    /* pagetable setup - x86 PV only */
>       int (*alloc_pgtables) (struct xc_dom_image * dom);
>       int (*alloc_p2m_list) (struct xc_dom_image * dom);
>       int (*setup_pgtables) (struct xc_dom_image * dom);
> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
> index 5b9eca6087..7e0fb9169f 100644
> --- a/tools/libxc/xc_dom_arm.c
> +++ b/tools/libxc/xc_dom_arm.c
> @@ -47,23 +47,6 @@ const char *xc_domain_get_native_protocol(xc_interface *xch,
>   }
>   
>   /* ------------------------------------------------------------------------ */
> -/*
> - * arm guests are hybrid and start off with paging disabled, therefore no
> - * pagetables and nothing to do here.
> - */
> -static int alloc_pgtables_arm(struct xc_dom_image *dom)
> -{
> -    DOMPRINTF_CALLED(dom->xch);
> -    return 0;
> -}
> -
> -static int setup_pgtables_arm(struct xc_dom_image *dom)
> -{
> -    DOMPRINTF_CALLED(dom->xch);
> -    return 0;
> -}
> -
> -/* ------------------------------------------------------------------------ */
>   
>   static int alloc_magic_pages(struct xc_dom_image *dom)
>   {
> @@ -539,8 +522,6 @@ static struct xc_dom_arch xc_dom_32 = {
>       .page_shift = PAGE_SHIFT_ARM,
>       .sizeof_pfn = 8,
>       .alloc_magic_pages = alloc_magic_pages,
> -    .alloc_pgtables = alloc_pgtables_arm,
> -    .setup_pgtables = setup_pgtables_arm,
>       .start_info = start_info_arm,
>       .shared_info = shared_info_arm,
>       .vcpu = vcpu_arm32,
> @@ -555,8 +536,6 @@ static struct xc_dom_arch xc_dom_64 = {
>       .page_shift = PAGE_SHIFT_ARM,
>       .sizeof_pfn = 8,
>       .alloc_magic_pages = alloc_magic_pages,
> -    .alloc_pgtables = alloc_pgtables_arm,
> -    .setup_pgtables = setup_pgtables_arm,
>       .start_info = start_info_arm,
>       .shared_info = shared_info_arm,
>       .vcpu = vcpu_arm64,
> diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
> index 918ee4d045..79dbbf6571 100644
> --- a/tools/libxc/xc_dom_boot.c
> +++ b/tools/libxc/xc_dom_boot.c
> @@ -199,9 +199,9 @@ int xc_dom_boot_image(struct xc_dom_image *dom)
>       /* initial mm setup */
>       if ( (rc = xc_dom_update_guest_p2m(dom)) != 0 )
>           return rc;
> -    if ( dom->arch_hooks->setup_pgtables )
> -        if ( (rc = dom->arch_hooks->setup_pgtables(dom)) != 0 )
> -            return rc;
> +    if ( dom->arch_hooks->setup_pgtables &&
> +         (rc = dom->arch_hooks->setup_pgtables(dom)) != 0 )
> +        return rc;
>   
>       /* start info page */
>       if ( dom->arch_hooks->start_info )
> diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
> index 9bd04cb2d5..fc77804a7e 100644
> --- a/tools/libxc/xc_dom_core.c
> +++ b/tools/libxc/xc_dom_core.c
> @@ -1247,7 +1247,8 @@ int xc_dom_build_image(struct xc_dom_image *dom)
>           goto err;
>       if ( dom->arch_hooks->alloc_magic_pages(dom) != 0 )
>           goto err;
> -    if ( dom->arch_hooks->alloc_pgtables(dom) != 0 )
> +    if ( dom->arch_hooks->alloc_pgtables &&
> +         dom->arch_hooks->alloc_pgtables(dom) != 0 )
>           goto err;
>       if ( dom->alloc_bootstack )
>       {
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index 1ce3c798ef..d2acff1061 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -1690,12 +1690,6 @@ static int bootlate_pv(struct xc_dom_image *dom)
>       return 0;
>   }
>   
> -static int alloc_pgtables_hvm(struct xc_dom_image *dom)
> -{
> -    DOMPRINTF("%s: doing nothing", __func__);
> -    return 0;
> -}
> -
>   /*
>    * The memory layout of the start_info page and the modules, and where the
>    * addresses are stored:
> @@ -1906,7 +1900,6 @@ static struct xc_dom_arch xc_hvm_32 = {
>       .page_shift = PAGE_SHIFT_X86,
>       .sizeof_pfn = 4,
>       .alloc_magic_pages = alloc_magic_pages_hvm,
> -    .alloc_pgtables = alloc_pgtables_hvm,
>       .vcpu = vcpu_hvm,
>       .meminit = meminit_hvm,
>       .bootearly = bootearly,
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Xen-devel] [PATCH 4/4] tools/dombuilder: Don't allocate dom->p2m_host[] for translated domains
  2019-12-17 20:15 ` [Xen-devel] [PATCH 4/4] tools/dombuilder: Don't allocate dom->p2m_host[] for translated domains Andrew Cooper
@ 2019-12-23 18:23   ` Julien Grall
  2020-01-02 17:49     ` Andrew Cooper
  2020-01-03 14:25   ` Jan Beulich
  1 sibling, 1 reply; 14+ messages in thread
From: Julien Grall @ 2019-12-23 18:23 UTC (permalink / raw)
  To: Andrew Cooper, Xen-devel
  Cc: Ian Jackson, Varad Gautam, Stefano Stabellini, Volodymyr Babchuk,
	Wei Liu

Hi,

On 17/12/2019 21:15, Andrew Cooper wrote:
> xc_dom_p2m() and dom->p2m_host[] implement a linear transform for translated
> domains, but waste a substantial chunk of RAM doing so.
> 
> ARM literally never reads dom->p2m_host[] (because of the xc_dom_translated()
> short circuit in xc_dom_p2m()).  Drop it all.
> 
> x86 HVM does use dom->p2m_host[] for xc_domain_populate_physmap_exact() calls
> when populating 4k pages.  Reuse the same tactic from 2M/1G ranges and use an
> on-stack array instead.  Drop the memory allocation.
> 
> x86 PV guests do use dom->p2m_host[] as a non-identity transform.  Rename the
> field to pv_p2m to make it clear it is PV-only.

Nice cleanup! This will probably make slightly faster guest boot :).

> @@ -359,7 +356,6 @@ static int populate_guest_memory(struct xc_dom_image *dom,
>   static int meminit(struct xc_dom_image *dom)
>   {
>       int i, rc;
> -    xen_pfn_t pfn;
>       uint64_t modbase;
>   
>       uint64_t ramsize = (uint64_t)dom->total_pages << XC_PAGE_SHIFT;
> @@ -423,11 +419,6 @@ static int meminit(struct xc_dom_image *dom)
>       assert(ramsize == 0); /* Too much RAM is rejected above */
>   
>       dom->p2m_size = p2m_size;

Do we need to keep p2m_size?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Xen-devel] [PATCH 0/4] Don't allocate dom->p2m_host[] for translated domains
  2019-12-17 20:15 [Xen-devel] [PATCH 0/4] Don't allocate dom->p2m_host[] for translated domains Andrew Cooper
                   ` (3 preceding siblings ...)
  2019-12-17 20:15 ` [Xen-devel] [PATCH 4/4] tools/dombuilder: Don't allocate dom->p2m_host[] for translated domains Andrew Cooper
@ 2019-12-31 16:37 ` Wei Liu
  4 siblings, 0 replies; 14+ messages in thread
From: Wei Liu @ 2019-12-31 16:37 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Stefano Stabellini, Julien Grall, Wei Liu, Varad Gautam,
	Ian Jackson, Xen-devel, Volodymyr Babchuk

On Tue, Dec 17, 2019 at 08:15:46PM +0000, Andrew Cooper wrote:
> Vastly drop xl's memory usage for HVM (x86 and ARM) guest construction.
> 
> See
> https://lore.kernel.org/xen-devel/1562159202-11316-1-git-send-email-vrd@amazon.de/T/#u
> for the origins of this work, but ultimately I think this is a far cleaner
> solution to the problem.
> 
> Andrew Cooper (4):
>   tools/dombuilder: xc_dom_x86 cleanup
>   tools/dombuilder: Remove PV-only, mandatory hooks
>   tools/dombuilder: Remove p2m_guest from the common interface
>   tools/dombuilder: Don't allocate dom->p2m_host[] for translated
>     domains

The code looks good to me.

Acked-by: Wei Liu <wl@xen.org>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Xen-devel] [PATCH 4/4] tools/dombuilder: Don't allocate dom->p2m_host[] for translated domains
  2019-12-23 18:23   ` Julien Grall
@ 2020-01-02 17:49     ` Andrew Cooper
  2020-01-03 10:44       ` Julien Grall
  0 siblings, 1 reply; 14+ messages in thread
From: Andrew Cooper @ 2020-01-02 17:49 UTC (permalink / raw)
  To: Julien Grall, Xen-devel
  Cc: Ian Jackson, Varad Gautam, Stefano Stabellini, Volodymyr Babchuk,
	Wei Liu

On 23/12/2019 18:23, Julien Grall wrote:
> Hi,
>
> On 17/12/2019 21:15, Andrew Cooper wrote:
>> xc_dom_p2m() and dom->p2m_host[] implement a linear transform for
>> translated
>> domains, but waste a substantial chunk of RAM doing so.
>>
>> ARM literally never reads dom->p2m_host[] (because of the
>> xc_dom_translated()
>> short circuit in xc_dom_p2m()).  Drop it all.
>>
>> x86 HVM does use dom->p2m_host[] for
>> xc_domain_populate_physmap_exact() calls
>> when populating 4k pages.  Reuse the same tactic from 2M/1G ranges
>> and use an
>> on-stack array instead.  Drop the memory allocation.
>>
>> x86 PV guests do use dom->p2m_host[] as a non-identity transform. 
>> Rename the
>> field to pv_p2m to make it clear it is PV-only.
>
> Nice cleanup! This will probably make slightly faster guest boot :).
>
>> @@ -359,7 +356,6 @@ static int populate_guest_memory(struct
>> xc_dom_image *dom,
>>   static int meminit(struct xc_dom_image *dom)
>>   {
>>       int i, rc;
>> -    xen_pfn_t pfn;
>>       uint64_t modbase;
>>         uint64_t ramsize = (uint64_t)dom->total_pages << XC_PAGE_SHIFT;
>> @@ -423,11 +419,6 @@ static int meminit(struct xc_dom_image *dom)
>>       assert(ramsize == 0); /* Too much RAM is rejected above */
>>         dom->p2m_size = p2m_size;
>
> Do we need to keep p2m_size?

Yes, I think so.

There are some common checks which would fail if it becomes 0, and
importantly, it is used to locate safe gfns for temporary physmap mappings.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Xen-devel] [PATCH 4/4] tools/dombuilder: Don't allocate dom->p2m_host[] for translated domains
  2020-01-02 17:49     ` Andrew Cooper
@ 2020-01-03 10:44       ` Julien Grall
  2020-01-08  9:36         ` Julien Grall
  0 siblings, 1 reply; 14+ messages in thread
From: Julien Grall @ 2020-01-03 10:44 UTC (permalink / raw)
  To: Andrew Cooper, Xen-devel
  Cc: Ian Jackson, Varad Gautam, Stefano Stabellini, Volodymyr Babchuk,
	Wei Liu

Hi Andrew,

On 02/01/2020 17:49, Andrew Cooper wrote:
> On 23/12/2019 18:23, Julien Grall wrote:
>> Hi,
>>
>> On 17/12/2019 21:15, Andrew Cooper wrote:
>>> xc_dom_p2m() and dom->p2m_host[] implement a linear transform for
>>> translated
>>> domains, but waste a substantial chunk of RAM doing so.
>>>
>>> ARM literally never reads dom->p2m_host[] (because of the
>>> xc_dom_translated()
>>> short circuit in xc_dom_p2m()).  Drop it all.
>>>
>>> x86 HVM does use dom->p2m_host[] for
>>> xc_domain_populate_physmap_exact() calls
>>> when populating 4k pages.  Reuse the same tactic from 2M/1G ranges
>>> and use an
>>> on-stack array instead.  Drop the memory allocation.
>>>
>>> x86 PV guests do use dom->p2m_host[] as a non-identity transform.
>>> Rename the
>>> field to pv_p2m to make it clear it is PV-only.
>>
>> Nice cleanup! This will probably make slightly faster guest boot :).
>>
>>> @@ -359,7 +356,6 @@ static int populate_guest_memory(struct
>>> xc_dom_image *dom,
>>>    static int meminit(struct xc_dom_image *dom)
>>>    {
>>>        int i, rc;
>>> -    xen_pfn_t pfn;
>>>        uint64_t modbase;
>>>          uint64_t ramsize = (uint64_t)dom->total_pages << XC_PAGE_SHIFT;
>>> @@ -423,11 +419,6 @@ static int meminit(struct xc_dom_image *dom)
>>>        assert(ramsize == 0); /* Too much RAM is rejected above */
>>>          dom->p2m_size = p2m_size;
>>
>> Do we need to keep p2m_size?
> 
> Yes, I think so.
> 
> There are some common checks which would fail if it becomes 0, and
> importantly, it is used to locate safe gfns for temporary physmap mappings.

Do you mean the scratch page? If so, on Arm we use a fix address in the 
memory map rather than p2m_size.

I have looked at the usage of the p2m_size in the common code and I 
didn't spot any path that can be used by Arm and using p2m_size.

Also, I don't think p2m_size is calculated correctly on Arm. It only 
englobe the RAM and doesn't take into account the rest of the mappings 
(e.g MMIO...). So I am not sure how this could be used in common code.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Xen-devel] [PATCH 4/4] tools/dombuilder: Don't allocate dom->p2m_host[] for translated domains
  2019-12-17 20:15 ` [Xen-devel] [PATCH 4/4] tools/dombuilder: Don't allocate dom->p2m_host[] for translated domains Andrew Cooper
  2019-12-23 18:23   ` Julien Grall
@ 2020-01-03 14:25   ` Jan Beulich
  2020-01-03 15:02     ` Andrew Cooper
  1 sibling, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2020-01-03 14:25 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Stefano Stabellini, Julien Grall, Wei Liu, Varad Gautam,
	Ian Jackson, Xen-devel, Volodymyr Babchuk

On 17.12.2019 21:15, Andrew Cooper wrote:
> --- a/tools/libxc/include/xc_dom.h
> +++ b/tools/libxc/include/xc_dom.h
> @@ -123,16 +123,12 @@ struct xc_dom_image {
>  
>      /* other state info */
>      uint32_t f_active[XENFEAT_NR_SUBMAPS];
> +
>      /*
> -     * p2m_host maps guest physical addresses an offset from
> -     * rambase_pfn (see below) into gfns.

The removal of this part of the comment and ...

> -     * For a pure PV guest this means that it maps GPFNs into MFNs for
> -     * a hybrid guest this means that it maps GPFNs to GPFNS.
> -     *
> -     * Note that the input is offset by rambase.
> +     * pv_p2m is specific to x86 PV guests, and maps GFNs to MFNs.  It is
> +     * eventually copied into guest context.
>       */
> -    xen_pfn_t *p2m_host;
> +    xen_pfn_t *pv_p2m;
>  
>      /* physical memory
>       *
> @@ -433,9 +429,12 @@ static inline xen_pfn_t xc_dom_p2m(struct xc_dom_image *dom, xen_pfn_t pfn)
>  {
>      if ( xc_dom_translated(dom) )
>          return pfn;
> -    if (pfn < dom->rambase_pfn || pfn >= dom->rambase_pfn + dom->total_pages)
> +
> +    /* x86 PV only now. */
> +    if ( pfn >= dom->total_pages )
>          return INVALID_MFN;
> -    return dom->p2m_host[pfn - dom->rambase_pfn];
> +
> +    return dom->pv_p2m[pfn];
>  }

... the dropping of all uses of rambase_pfn here make it look
like you're doing away with that concept altogether. Is this
really correct? If so, I guess you want to
- say a word in this regard in the description, the more that
  you don't fully get rid of this (the structure field and
  some uses elsewhere remain),
- drop/adjust the respective comment fragment next to the
  field a little further down in the structure.

> @@ -1245,11 +1245,11 @@ static int meminit_pv(struct xc_dom_image *dom)
>          return -EINVAL;
>      }
>  
> -    dom->p2m_host = xc_dom_malloc(dom, sizeof(xen_pfn_t) * dom->p2m_size);
> -    if ( dom->p2m_host == NULL )
> +    dom->pv_p2m = xc_dom_malloc(dom, sizeof(xen_pfn_t) * dom->p2m_size);

Since you're touching the line anyway, perhaps better
sizeof(*dom->pv_p2m)?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Xen-devel] [PATCH 4/4] tools/dombuilder: Don't allocate dom->p2m_host[] for translated domains
  2020-01-03 14:25   ` Jan Beulich
@ 2020-01-03 15:02     ` Andrew Cooper
  2020-01-03 15:33       ` Jan Beulich
  0 siblings, 1 reply; 14+ messages in thread
From: Andrew Cooper @ 2020-01-03 15:02 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Stefano Stabellini, Julien Grall, Wei Liu, Varad Gautam,
	Ian Jackson, Xen-devel, Volodymyr Babchuk

On 03/01/2020 14:25, Jan Beulich wrote:
> On 17.12.2019 21:15, Andrew Cooper wrote:
>> --- a/tools/libxc/include/xc_dom.h
>> +++ b/tools/libxc/include/xc_dom.h
>> @@ -123,16 +123,12 @@ struct xc_dom_image {
>>  
>>      /* other state info */
>>      uint32_t f_active[XENFEAT_NR_SUBMAPS];
>> +
>>      /*
>> -     * p2m_host maps guest physical addresses an offset from
>> -     * rambase_pfn (see below) into gfns.
> The removal of this part of the comment and ...
>
>> -     * For a pure PV guest this means that it maps GPFNs into MFNs for
>> -     * a hybrid guest this means that it maps GPFNs to GPFNS.
>> -     *
>> -     * Note that the input is offset by rambase.
>> +     * pv_p2m is specific to x86 PV guests, and maps GFNs to MFNs.  It is
>> +     * eventually copied into guest context.
>>       */
>> -    xen_pfn_t *p2m_host;
>> +    xen_pfn_t *pv_p2m;
>>  
>>      /* physical memory
>>       *
>> @@ -433,9 +429,12 @@ static inline xen_pfn_t xc_dom_p2m(struct xc_dom_image *dom, xen_pfn_t pfn)
>>  {
>>      if ( xc_dom_translated(dom) )
>>          return pfn;
>> -    if (pfn < dom->rambase_pfn || pfn >= dom->rambase_pfn + dom->total_pages)
>> +
>> +    /* x86 PV only now. */
>> +    if ( pfn >= dom->total_pages )
>>          return INVALID_MFN;
>> -    return dom->p2m_host[pfn - dom->rambase_pfn];
>> +
>> +    return dom->pv_p2m[pfn];
>>  }
> ... the dropping of all uses of rambase_pfn here make it look
> like you're doing away with that concept altogether. Is this
> really correct?

Well - it is effectively dead code here, because of the
xc_dom_translated() in context above, and it being 0 outside of ARM.

The (nonzero) value is used by other bits of ARM code.

>  If so, I guess you want to
> - say a word in this regard in the description, the more that
>   you don't fully get rid of this (the structure field and
>   some uses elsewhere remain),
> - drop/adjust the respective comment fragment next to the
>   field a little further down in the structure.

The domain builder is made almost exclusively of bitrot, but I don't
have an ARM system to do any testing on.  Given how fragile the code is,
I'm not comfortable doing speculative deletion of code I'm not certain
is unused.

>
>> @@ -1245,11 +1245,11 @@ static int meminit_pv(struct xc_dom_image *dom)
>>          return -EINVAL;
>>      }
>>  
>> -    dom->p2m_host = xc_dom_malloc(dom, sizeof(xen_pfn_t) * dom->p2m_size);
>> -    if ( dom->p2m_host == NULL )
>> +    dom->pv_p2m = xc_dom_malloc(dom, sizeof(xen_pfn_t) * dom->p2m_size);
> Since you're touching the line anyway, perhaps better
> sizeof(*dom->pv_p2m)?

Will fix.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Xen-devel] [PATCH 4/4] tools/dombuilder: Don't allocate dom->p2m_host[] for translated domains
  2020-01-03 15:02     ` Andrew Cooper
@ 2020-01-03 15:33       ` Jan Beulich
  0 siblings, 0 replies; 14+ messages in thread
From: Jan Beulich @ 2020-01-03 15:33 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Stefano Stabellini, Julien Grall, Wei Liu, Varad Gautam,
	Ian Jackson, Xen-devel, Volodymyr Babchuk

On 03.01.2020 16:02, Andrew Cooper wrote:
> On 03/01/2020 14:25, Jan Beulich wrote:
>> On 17.12.2019 21:15, Andrew Cooper wrote:
>>> --- a/tools/libxc/include/xc_dom.h
>>> +++ b/tools/libxc/include/xc_dom.h
>>> @@ -123,16 +123,12 @@ struct xc_dom_image {
>>>  
>>>      /* other state info */
>>>      uint32_t f_active[XENFEAT_NR_SUBMAPS];
>>> +
>>>      /*
>>> -     * p2m_host maps guest physical addresses an offset from
>>> -     * rambase_pfn (see below) into gfns.
>> The removal of this part of the comment and ...
>>
>>> -     * For a pure PV guest this means that it maps GPFNs into MFNs for
>>> -     * a hybrid guest this means that it maps GPFNs to GPFNS.
>>> -     *
>>> -     * Note that the input is offset by rambase.
>>> +     * pv_p2m is specific to x86 PV guests, and maps GFNs to MFNs.  It is
>>> +     * eventually copied into guest context.
>>>       */
>>> -    xen_pfn_t *p2m_host;
>>> +    xen_pfn_t *pv_p2m;
>>>  
>>>      /* physical memory
>>>       *
>>> @@ -433,9 +429,12 @@ static inline xen_pfn_t xc_dom_p2m(struct xc_dom_image *dom, xen_pfn_t pfn)
>>>  {
>>>      if ( xc_dom_translated(dom) )
>>>          return pfn;
>>> -    if (pfn < dom->rambase_pfn || pfn >= dom->rambase_pfn + dom->total_pages)
>>> +
>>> +    /* x86 PV only now. */
>>> +    if ( pfn >= dom->total_pages )
>>>          return INVALID_MFN;
>>> -    return dom->p2m_host[pfn - dom->rambase_pfn];
>>> +
>>> +    return dom->pv_p2m[pfn];
>>>  }
>> ... the dropping of all uses of rambase_pfn here make it look
>> like you're doing away with that concept altogether. Is this
>> really correct?
> 
> Well - it is effectively dead code here, because of the
> xc_dom_translated() in context above, and it being 0 outside of ARM.
> 
> The (nonzero) value is used by other bits of ARM code.
> 
>>  If so, I guess you want to
>> - say a word in this regard in the description, the more that
>>   you don't fully get rid of this (the structure field and
>>   some uses elsewhere remain),
>> - drop/adjust the respective comment fragment next to the
>>   field a little further down in the structure.
> 
> The domain builder is made almost exclusively of bitrot, but I don't
> have an ARM system to do any testing on.  Given how fragile the code is,
> I'm not comfortable doing speculative deletion of code I'm not certain
> is unused.

My remark was about this (non-Arm) part of the comment:

     * An x86 PV guest has one or more blocks of physical RAM,
     * consisting of total_pages starting at rambase_pfn. The start
     * address and size of each block is controlled by vNUMA
     * structures.

I did in no way mean to ask for speculative deletion of code.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Xen-devel] [PATCH 4/4] tools/dombuilder: Don't allocate dom->p2m_host[] for translated domains
  2020-01-03 10:44       ` Julien Grall
@ 2020-01-08  9:36         ` Julien Grall
  0 siblings, 0 replies; 14+ messages in thread
From: Julien Grall @ 2020-01-08  9:36 UTC (permalink / raw)
  To: Andrew Cooper, Xen-devel
  Cc: Ian Jackson, Varad Gautam, Stefano Stabellini, Volodymyr Babchuk,
	Wei Liu

Hi,

On 03/01/2020 10:44, Julien Grall wrote:
> Hi Andrew,
> 
> On 02/01/2020 17:49, Andrew Cooper wrote:
>> On 23/12/2019 18:23, Julien Grall wrote:
>>> Hi,
>>>
>>> On 17/12/2019 21:15, Andrew Cooper wrote:
>>>> xc_dom_p2m() and dom->p2m_host[] implement a linear transform for
>>>> translated
>>>> domains, but waste a substantial chunk of RAM doing so.
>>>>
>>>> ARM literally never reads dom->p2m_host[] (because of the
>>>> xc_dom_translated()
>>>> short circuit in xc_dom_p2m()).  Drop it all.
>>>>
>>>> x86 HVM does use dom->p2m_host[] for
>>>> xc_domain_populate_physmap_exact() calls
>>>> when populating 4k pages.  Reuse the same tactic from 2M/1G ranges
>>>> and use an
>>>> on-stack array instead.  Drop the memory allocation.
>>>>
>>>> x86 PV guests do use dom->p2m_host[] as a non-identity transform.
>>>> Rename the
>>>> field to pv_p2m to make it clear it is PV-only.
>>>
>>> Nice cleanup! This will probably make slightly faster guest boot :).
>>>
>>>> @@ -359,7 +356,6 @@ static int populate_guest_memory(struct
>>>> xc_dom_image *dom,
>>>>    static int meminit(struct xc_dom_image *dom)
>>>>    {
>>>>        int i, rc;
>>>> -    xen_pfn_t pfn;
>>>>        uint64_t modbase;
>>>>          uint64_t ramsize = (uint64_t)dom->total_pages << 
>>>> XC_PAGE_SHIFT;
>>>> @@ -423,11 +419,6 @@ static int meminit(struct xc_dom_image *dom)
>>>>        assert(ramsize == 0); /* Too much RAM is rejected above */
>>>>          dom->p2m_size = p2m_size;
>>>
>>> Do we need to keep p2m_size?
>>
>> Yes, I think so.
>>
>> There are some common checks which would fail if it becomes 0, and
>> importantly, it is used to locate safe gfns for temporary physmap 
>> mappings.
> 
> Do you mean the scratch page? If so, on Arm we use a fix address in the 
> memory map rather than p2m_size.
> 
> I have looked at the usage of the p2m_size in the common code and I 
> didn't spot any path that can be used by Arm and using p2m_size.
> 
> Also, I don't think p2m_size is calculated correctly on Arm. It only 
> englobe the RAM and doesn't take into account the rest of the mappings 
> (e.g MMIO...). So I am not sure how this could be used in common code.

Thinking a bit more of this. As the p2m_size was IHMO already buggy, it 
should not make any difference after this series. Therefore removing 
p2m_size can be probably done on a follow-up patch.

So I am happy with the patch as-is:

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

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2020-01-08  9:36 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-17 20:15 [Xen-devel] [PATCH 0/4] Don't allocate dom->p2m_host[] for translated domains Andrew Cooper
2019-12-17 20:15 ` [Xen-devel] [PATCH 1/4] tools/dombuilder: xc_dom_x86 cleanup Andrew Cooper
2019-12-17 20:15 ` [Xen-devel] [PATCH 2/4] tools/dombuilder: Remove PV-only, mandatory hooks Andrew Cooper
2019-12-23 18:12   ` Julien Grall
2019-12-17 20:15 ` [Xen-devel] [PATCH 3/4] tools/dombuilder: Remove p2m_guest from the common interface Andrew Cooper
2019-12-17 20:15 ` [Xen-devel] [PATCH 4/4] tools/dombuilder: Don't allocate dom->p2m_host[] for translated domains Andrew Cooper
2019-12-23 18:23   ` Julien Grall
2020-01-02 17:49     ` Andrew Cooper
2020-01-03 10:44       ` Julien Grall
2020-01-08  9:36         ` Julien Grall
2020-01-03 14:25   ` Jan Beulich
2020-01-03 15:02     ` Andrew Cooper
2020-01-03 15:33       ` Jan Beulich
2019-12-31 16:37 ` [Xen-devel] [PATCH 0/4] " Wei Liu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.