Re: REGRESSION [PATCH v2 08/13] libxc: Check xc_domain_maximum_gpfn for negative return values

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Thu, Mar 26, 2015 at 09:07:05PM +0000, Andrew Cooper wrote:
> On 20/03/15 17:03, Ian Campbell wrote:
> >On Fri, 2015-03-20 at 11:45 -0400, Konrad Rzeszutek Wilk wrote:
> >> From 45bd7cd377b0b8364757cc2bc0bd8d6a13523a97 Mon Sep 17 00:00:00 2001
> >>From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >>Date: Fri, 13 Mar 2015 14:57:44 -0400
> >>Subject: [PATCH] libxc: Check xc_domain_maximum_gpfn for negative return
> >>  values
> >>
> >>Instead of assuming everything is always OK. We stash
> >>the gpfns value as an parameter. Since we use it in three
> >>of places we might as well update xc_domain_maximum_gpfn
> >>to do the right thing.
> >>
> >>Suggested-by: Ian Campbell <ian.campbell@citrix.com>
> >>Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >Acked + applied along with the rest of the series, thanks,
> 
> This change as unfortunately causes a regression in migration v2, because
> the fenceposting has changed and the function no longer returns the maximum
> gpfn.  It returns one past the maximum gpfn.
> 
> It would appear that migration v2 was the only consumer which actually want
> the max gpfn.
> 
> Can we either rename the function to accurately name the value it returns
> (although I am out of ideas as to what this might be), or undo the
> fenceposting change so that it continues to return the value it claims to
> return.

This should do it? (I didn't fix the ';' issue in here).

>From a7206930f867025234966c7b784bead9b174930b Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Fri, 27 Mar 2015 15:36:02 -0400
Subject: [PATCH] libxc: Re-institute the old xc_maximum_ram_page and add
 xc_maximum_gpfn

The commit 1781f00ea62edb72bed4fe1b6959eeed427e988f
"libxc: Check xc_maximum_ram_page for negative return values."
broke migrationv2 (out of tree) as it wanted the raw
value instead of the +1 manipulation the rest of the callers do.

As such we resurrect xc_maximum_ram_page and rename the
version that was added in the above mention commit to
xc_maximum_gpfn.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxc/include/xenctrl.h | 3 ++-
 tools/libxc/xc_offline_page.c | 2 +-
 tools/libxc/xc_private.c      | 7 ++++++-
 tools/libxc/xg_save_restore.h | 2 +-
 tools/misc/xen-mfndump.c      | 4 ++--
 5 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 4e9537e..c3d22de 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1509,7 +1509,8 @@ int xc_mmuext_op(xc_interface *xch, struct mmuext_op *op, unsigned int nr_ops,
                  domid_t dom);
 
 /* System wide memory properties */
-int xc_maximum_ram_page(xc_interface *xch, unsigned long *max_mfn);
+long xc_maximum_ram_page(xc_interface *xch);
+int xc_maximum_gpfn(xc_interface *xch, unsigned long *max_mfn);
 
 /* Get current total pages allocated to a domain. */
 long xc_get_tot_pages(xc_interface *xch, uint32_t domid);
diff --git a/tools/libxc/xc_offline_page.c b/tools/libxc/xc_offline_page.c
index b1d169c..4bf35ea 100644
--- a/tools/libxc/xc_offline_page.c
+++ b/tools/libxc/xc_offline_page.c
@@ -433,7 +433,7 @@ int xc_exchange_page(xc_interface *xch, int domid, xen_pfn_t mfn)
     }
 
     /* Map M2P and obtain gpfn */
-    rc = xc_maximum_ram_page(xch, &max_mfn);
+    rc = xc_maximum_gpfn(xch, &max_mfn);
     if ( rc || !(m2p_table = xc_map_m2p(xch, max_mfn, PROT_READ, NULL)) )
     {
         PERROR("Failed to map live M2P table");
diff --git a/tools/libxc/xc_private.c b/tools/libxc/xc_private.c
index 2eb44b6..ef0d778 100644
--- a/tools/libxc/xc_private.c
+++ b/tools/libxc/xc_private.c
@@ -535,7 +535,12 @@ int do_memory_op(xc_interface *xch, int cmd, void *arg, size_t len)
     return ret;
 }
 
-int xc_maximum_ram_page(xc_interface *xch, unsigned long *max_mfn)
+long xc_maximum_ram_page(xc_interface *xch)
+{
+    return do_memory_op(xch, XENMEM_maximum_ram_page, NULL, 0);
+}
+
+int xc_maximum_gpfn(xc_interface *xch, unsigned long *max_mfn)
 {
     long rc = do_memory_op(xch, XENMEM_maximum_ram_page, NULL, 0);
 
diff --git a/tools/libxc/xg_save_restore.h b/tools/libxc/xg_save_restore.h
index 832c329..c760df8 100644
--- a/tools/libxc/xg_save_restore.h
+++ b/tools/libxc/xg_save_restore.h
@@ -311,7 +311,7 @@ static inline int get_platform_info(xc_interface *xch, uint32_t dom,
     if (xc_version(xch, XENVER_capabilities, &xen_caps) != 0)
         return 0;
 
-    if (xc_maximum_ram_page(xch, max_mfn))
+    if (xc_maximum_gpfn(xch, max_mfn))
         return 0;
 
     *hvirt_start = xen_params.virt_start;
diff --git a/tools/misc/xen-mfndump.c b/tools/misc/xen-mfndump.c
index 0c018e0..12555b8 100644
--- a/tools/misc/xen-mfndump.c
+++ b/tools/misc/xen-mfndump.c
@@ -41,7 +41,7 @@ int dump_m2p_func(int argc, char *argv[])
     }
 
     /* Map M2P and obtain gpfn */
-    if ( xc_maximum_ram_page(xch, &max_mfn) < 0 );
+    if ( xc_maximum_gpfn(xch, &max_mfn) < 0 );
     {
         ERROR("Failed to get the maximum mfn");
         return -1;
@@ -182,7 +182,7 @@ int dump_ptes_func(int argc, char *argv[])
     }
 
     /* Map M2P and obtain gpfn */
-    rc = xc_maximum_ram_page(xch, &max_mfn);
+    rc = xc_maximum_gpfn(xch, &max_mfn);
     if ( rc || (mfn > max_mfn) ||
          !(m2p_table = xc_map_m2p(xch, max_mfn, PROT_READ, NULL)) )
     {
-- 
2.1.0

> 
> ~Andrew