Series comparison

-[PATCH] x86/xen: fix balloon target initialization for PVH dom0
+[PATCH v3] x86/xen: fix balloon target initialization for PVH dom0
 ...
 dom0 memory target is later adjusted from the toolstack.
 Fix this by using xen_released_pages to account for any pages that are part
 of the memory map, but are already unpopulated when the balloon driver is
 initialized.  This accounts for any regions used for scratch remote
-mappings.
+mappings.  Note on x86 xen_released_pages definition is moved to
 enlighten.c so it's uniformly available for all Xen-enabled builds.
 Take the opportunity to unify PV with PVH/HVM guests regarding the usage of
 get_num_physpages(), as that avoids having to add different logic for PV vs
 PVH in both balloon_add_regions() and arch_xen_unpopulated_init().
 Much like a6aa4eb994ee, the code in this changeset should have been part of
 fc4e893.
 Fixes: a6aa4eb994ee ('xen/x86: add extra pages to unpopulated-alloc if available')
 Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
+Reviewed-by: Juergen Gross <jgross@suse.com>
+Cc: stable@vger.kernel.org
 ---
-I think it's easier to unify the PV and PVH/HVM paths here regarding the
+Changes since v2:
-usage of get_num_physpages(), as otherwise the fix needs to add further PV
+ - For x86: Move xen_released_pages definition from setup.c (PV specific)
-vs HVM divergences in both balloon_add_regions() and
+   to enlighten.c (shared between all guest modes).
-arch_xen_unpopulated_init(), but it also has a higher risk of breaking PV
-in subtle ways.
+Changes since v1:
  - Replace BUG_ON() with a WARN and failure to initialize the balloon
    driver.
 ---
- arch/x86/xen/enlighten.c |  7 +++++++
+ arch/x86/xen/enlighten.c | 10 ++++++++++
- drivers/xen/balloon.c    | 19 +++++++++++--------
+ arch/x86/xen/setup.c     |  3 ---
-files changed, 18 insertions(+), 8 deletions(-)
+ drivers/xen/balloon.c    | 34 ++++++++++++++++++++++++----------
 files changed, 34 insertions(+), 13 deletions(-)
 diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
 index XXXXXXX..XXXXXXX 100644
 --- a/arch/x86/xen/enlighten.c
 +++ b/arch/x86/xen/enlighten.c
+@@ -XXX,XX +XXX,XX @@ EXPORT_SYMBOL(xen_start_flags);
+  */
+ struct shared_info *HYPERVISOR_shared_info = &xen_dummy_shared_info;
++/* Number of pages released from the initial allocation. */
++unsigned long xen_released_pages;
++
+ static __ref void xen_get_vendor(void)
+ {
+     init_cpu_devs();
 @@ -XXX,XX +XXX,XX @@ int __init arch_xen_unpopulated_init(struct resource **res)
              xen_free_unpopulated_pages(1, &pg);
          }
 +        /*
 ...
 +         */
 +        xen_released_pages += xen_extra_mem[i].n_pfns;
          /* Zero so region is not also added to the balloon driver. */
          xen_extra_mem[i].n_pfns = 0;
      }
+diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
+index XXXXXXX..XXXXXXX 100644
+--- a/arch/x86/xen/setup.c
++++ b/arch/x86/xen/setup.c
+@@ -XXX,XX +XXX,XX @@
+ #define GB(x) ((uint64_t)(x) * 1024 * 1024 * 1024)
+-/* Number of pages released from the initial allocation. */
+-unsigned long xen_released_pages;
+-
+ /* Memory map would allow PCI passthrough. */
+ bool xen_pv_pci_possible;
 diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
 index XXXXXXX..XXXXXXX 100644
 --- a/drivers/xen/balloon.c
 +++ b/drivers/xen/balloon.c
+@@ -XXX,XX +XXX,XX @@ void xen_free_ballooned_pages(unsigned int nr_pages, struct page **pages)
+ }
+ EXPORT_SYMBOL(xen_free_ballooned_pages);
+-static void __init balloon_add_regions(void)
++static int __init balloon_add_regions(void)
+ {
+     unsigned long start_pfn, pages;
+     unsigned long pfn, extra_pfn_end;
 @@ -XXX,XX +XXX,XX @@ static void __init balloon_add_regions(void)
          for (pfn = start_pfn; pfn < extra_pfn_end; pfn++)
              balloon_append(pfn_to_page(pfn));
 -        balloon_stats.total_pages += extra_pfn_end - start_pfn;
 +        /*
 +         * Extra regions are accounted for in the physmap, but need
 +         * decreasing from current_pages to balloon down the initial
 +         * allocation, because they are already accounted for in
 +         * total_pages.
 +         */
-+        BUG_ON(extra_pfn_end - start_pfn >=
++        if (extra_pfn_end - start_pfn >= balloon_stats.current_pages) {
-+               balloon_stats.current_pages);
++            WARN(1, "Extra pages underflow current target");
 +            return -ERANGE;
 +        }
 +        balloon_stats.current_pages -= extra_pfn_end - start_pfn;
      }
++
++    return 0;
  }
-@@ -XXX,XX +XXX,XX @@ static int __init balloon_init(void)
+ static int __init balloon_init(void)
  {
      struct task_struct *task;
 +    int rc;
      if (!xen_domain())
          return -ENODEV;
      pr_info("Initialising balloon driver\n");
 -#ifdef CONFIG_XEN_PV
 -    balloon_stats.current_pages = xen_pv_domain()
 -        ? min(xen_start_info->nr_pages - xen_released_pages, max_pfn)
 -        : get_num_physpages();
 -#else
 -    balloon_stats.current_pages = get_num_physpages();
 -#endif
-+    BUG_ON(xen_released_pages >= get_num_physpages());
++    if (xen_released_pages >= get_num_physpages()) {
 +        WARN(1, "Released pages underflow current target");
 +        return -ERANGE;
 +    }
 +
 +    balloon_stats.current_pages = get_num_physpages() - xen_released_pages;
      balloon_stats.target_pages  = balloon_stats.current_pages;
      balloon_stats.balloon_low   = 0;
      balloon_stats.balloon_high  = 0;
+@@ -XXX,XX +XXX,XX @@ static int __init balloon_init(void)
+     register_sysctl_init("xen/balloon", balloon_table);
+ #endif
+-    balloon_add_regions();
++    rc = balloon_add_regions();
++    if (rc)
++        return rc;
+     task = kthread_run(balloon_thread, NULL, "xen-balloon");
+     if (IS_ERR(task)) {
 --
 .48.1

PVH dom0 re-uses logic from PV dom0, in which RAM ranges not assigned to
dom0 are re-used as scratch memory to map foreign and grant pages.  Such
logic relies on reporting those unpopulated ranges as RAM to Linux, and
mark them as reserved.  This way Linux creates the underlying page
structures required for metadata management.

Such approach works fine on PV because the initial balloon target is
calculated using specific Xen data, that doesn't take into account the
memory type changes described above.  However on HVM and PVH the initial
balloon target is calculated using get_num_physpages(), and that function
does take into account the unpopulated RAM regions used as scratch space
for remote domain mappings.

This leads to PVH dom0 having an incorrect initial balloon target, which
causes malfunction (excessive memory freeing) of the balloon driver if the
dom0 memory target is later adjusted from the toolstack.

Take the opportunity to unify PV with PVH/HVM guests regarding the usage of
get_num_physpages(), as that avoids having to add different logic for PV vs
PVH in both balloon_add_regions() and arch_xen_unpopulated_init().

Much like a6aa4eb994ee, the code in this changeset should have been part of
38620fc4e893.

Fixes: a6aa4eb994ee ('xen/x86: add extra pages to unpopulated-alloc if available')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
I think it's easier to unify the PV and PVH/HVM paths here regarding the
usage of get_num_physpages(), as otherwise the fix needs to add further PV
vs HVM divergences in both balloon_add_regions() and
arch_xen_unpopulated_init(), but it also has a higher risk of breaking PV
in subtle ways.
---
 arch/x86/xen/enlighten.c |  7 +++++++
 drivers/xen/balloon.c    | 19 +++++++++++--------
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index XXXXXXX..XXXXXXX 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -XXX,XX +XXX,XX @@ int __init arch_xen_unpopulated_init(struct resource **res)
 			xen_free_unpopulated_pages(1, &pg);
 		}
 
+		/*
+		 * Account for the region being in the physmap but unpopulated.
+		 * The value in xen_released_pages is used by the balloon
+		 * driver to know how much of the physmap is unpopulated and
+		 * set an accurate initial memory target.
+		 */
+		xen_released_pages += xen_extra_mem[i].n_pfns;
 		/* Zero so region is not also added to the balloon driver. */
 		xen_extra_mem[i].n_pfns = 0;
 	}
diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index XXXXXXX..XXXXXXX 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -XXX,XX +XXX,XX @@ static void __init balloon_add_regions(void)
 		for (pfn = start_pfn; pfn < extra_pfn_end; pfn++)
 			balloon_append(pfn_to_page(pfn));
 
-		balloon_stats.total_pages += extra_pfn_end - start_pfn;
+		/*
+		 * Extra regions are accounted for in the physmap, but need
+		 * decreasing from current_pages to balloon down the initial
+		 * allocation, because they are already accounted for in
+		 * total_pages.
+		 */
+		BUG_ON(extra_pfn_end - start_pfn >=
+		       balloon_stats.current_pages);
+		balloon_stats.current_pages -= extra_pfn_end - start_pfn;
 	}
 }
 
@@ -XXX,XX +XXX,XX @@ static int __init balloon_init(void)
 
 	pr_info("Initialising balloon driver\n");
 
-#ifdef CONFIG_XEN_PV
-	balloon_stats.current_pages = xen_pv_domain()
-		? min(xen_start_info->nr_pages - xen_released_pages, max_pfn)
-		: get_num_physpages();
-#else
-	balloon_stats.current_pages = get_num_physpages();
-#endif
+	BUG_ON(xen_released_pages >= get_num_physpages());
+	balloon_stats.current_pages = get_num_physpages() - xen_released_pages;
 	balloon_stats.target_pages  = balloon_stats.current_pages;
 	balloon_stats.balloon_low   = 0;
 	balloon_stats.balloon_high  = 0;
-- 
2.48.1

Fix this by using xen_released_pages to account for any pages that are part
of the memory map, but are already unpopulated when the balloon driver is
initialized.  This accounts for any regions used for scratch remote
mappings.  Note on x86 xen_released_pages definition is moved to
enlighten.c so it's uniformly available for all Xen-enabled builds.

Much like a6aa4eb994ee, the code in this changeset should have been part of
38620fc4e893.

Fixes: a6aa4eb994ee ('xen/x86: add extra pages to unpopulated-alloc if available')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
Cc: stable@vger.kernel.org
---
Changes since v2:
 - For x86: Move xen_released_pages definition from setup.c (PV specific)
   to enlighten.c (shared between all guest modes).

Changes since v1:
 - Replace BUG_ON() with a WARN and failure to initialize the balloon
   driver.
---
 arch/x86/xen/enlighten.c | 10 ++++++++++
 arch/x86/xen/setup.c     |  3 ---
 drivers/xen/balloon.c    | 34 ++++++++++++++++++++++++----------
 3 files changed, 34 insertions(+), 13 deletions(-)

diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index XXXXXXX..XXXXXXX 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -XXX,XX +XXX,XX @@ EXPORT_SYMBOL(xen_start_flags);
  */
 struct shared_info *HYPERVISOR_shared_info = &xen_dummy_shared_info;
 
+/* Number of pages released from the initial allocation. */
+unsigned long xen_released_pages;
+
 static __ref void xen_get_vendor(void)
 {
 	init_cpu_devs();
@@ -XXX,XX +XXX,XX @@ int __init arch_xen_unpopulated_init(struct resource **res)
 			xen_free_unpopulated_pages(1, &pg);
 		}
 
+		/*
+		 * Account for the region being in the physmap but unpopulated.
+		 * The value in xen_released_pages is used by the balloon
+		 * driver to know how much of the physmap is unpopulated and
+		 * set an accurate initial memory target.
+		 */
+		xen_released_pages += xen_extra_mem[i].n_pfns;
 		/* Zero so region is not also added to the balloon driver. */
 		xen_extra_mem[i].n_pfns = 0;
 	}
diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
index XXXXXXX..XXXXXXX 100644
--- a/arch/x86/xen/setup.c
+++ b/arch/x86/xen/setup.c
@@ -XXX,XX +XXX,XX @@
 
 #define GB(x) ((uint64_t)(x) * 1024 * 1024 * 1024)
 
-/* Number of pages released from the initial allocation. */
-unsigned long xen_released_pages;
-
 /* Memory map would allow PCI passthrough. */
 bool xen_pv_pci_possible;
 
diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index XXXXXXX..XXXXXXX 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -XXX,XX +XXX,XX @@ void xen_free_ballooned_pages(unsigned int nr_pages, struct page **pages)
 }
 EXPORT_SYMBOL(xen_free_ballooned_pages);
 
-static void __init balloon_add_regions(void)
+static int __init balloon_add_regions(void)
 {
 	unsigned long start_pfn, pages;
 	unsigned long pfn, extra_pfn_end;
@@ -XXX,XX +XXX,XX @@ static void __init balloon_add_regions(void)
 		for (pfn = start_pfn; pfn < extra_pfn_end; pfn++)
 			balloon_append(pfn_to_page(pfn));
 
-		balloon_stats.total_pages += extra_pfn_end - start_pfn;
+		/*
+		 * Extra regions are accounted for in the physmap, but need
+		 * decreasing from current_pages to balloon down the initial
+		 * allocation, because they are already accounted for in
+		 * total_pages.
+		 */
+		if (extra_pfn_end - start_pfn >= balloon_stats.current_pages) {
+			WARN(1, "Extra pages underflow current target");
+			return -ERANGE;
+		}
+		balloon_stats.current_pages -= extra_pfn_end - start_pfn;
 	}
+
+	return 0;
 }
 
 static int __init balloon_init(void)
 {
 	struct task_struct *task;
+	int rc;
 
 	if (!xen_domain())
 		return -ENODEV;
 
 	pr_info("Initialising balloon driver\n");
 
-#ifdef CONFIG_XEN_PV
-	balloon_stats.current_pages = xen_pv_domain()
-		? min(xen_start_info->nr_pages - xen_released_pages, max_pfn)
-		: get_num_physpages();
-#else
-	balloon_stats.current_pages = get_num_physpages();
-#endif
+	if (xen_released_pages >= get_num_physpages()) {
+		WARN(1, "Released pages underflow current target");
+		return -ERANGE;
+	}
+
+	balloon_stats.current_pages = get_num_physpages() - xen_released_pages;
 	balloon_stats.target_pages  = balloon_stats.current_pages;
 	balloon_stats.balloon_low   = 0;
 	balloon_stats.balloon_high  = 0;
@@ -XXX,XX +XXX,XX @@ static int __init balloon_init(void)
 	register_sysctl_init("xen/balloon", balloon_table);
 #endif
 
-	balloon_add_regions();
+	rc = balloon_add_regions();
+	if (rc)
+		return rc;
 
 	task = kthread_run(balloon_thread, NULL, "xen-balloon");
 	if (IS_ERR(task)) {
-- 
2.48.1