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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Fri, Mar 27, 2015 at 08:04:44PM +0000, Andrew Cooper wrote:
> On 27/03/15 19:42, Konrad Rzeszutek Wilk wrote:
> > 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@xxxxxxxxxx>
> >>>> 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@xxxxxxxxxx>
> >>>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> >>> 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@xxxxxxxxxx>
> > 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@xxxxxxxxxx>
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> 
> It is not xc_maximum_ram_page() which was the problem.  It is
> xc_domain_maximum_gpfn() which had its fenceposting changed.
> 
> The new xc_maximum_ram_page() is fine, and really does want to keep its
> new API.  xc_domain_maximum_gpfn() is also fine keeping its new API, but
> wants to not adjust max_pfn by 1.
> 

<blushes>

From dbca252c1ad53a83ec5cf0a0d5aa62ca711bd0c0 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Date: Fri, 27 Mar 2015 16:36:36 -0400
Subject: [PATCH] libxc: Re-institute the old xc_domain_maximum_gpfn and add
 xc_domain_nr_gpfns

The commit a8f8a590e02d2d2b717257c0bd9a8b396103bdf4
"libxc: Check xc_domain_maximum_gpfn for negative return values"
broke migrationv2 (out of tree) as it wanted the raw value instead
of the +1 value.

Resurrect the old xc_domain_maximum_gpfn and rename the new
xc_domain_maximum_gpfn to xc_domain_nr_gpfns.

Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
---
 tools/libxc/include/xenctrl.h | 4 +++-
 tools/libxc/xc_core_arm.c     | 2 +-
 tools/libxc/xc_core_x86.c     | 6 +++---
 tools/libxc/xc_domain.c       | 6 +++++-
 tools/libxc/xc_domain_save.c  | 2 +-
 5 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 4e9537e..5d7d653 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1315,7 +1315,9 @@ int xc_domain_get_tsc_info(xc_interface *xch,
 
 int xc_domain_disable_migrate(xc_interface *xch, uint32_t domid);
 
-int xc_domain_maximum_gpfn(xc_interface *xch, domid_t domid, xen_pfn_t *gpfns);
+int xc_domain_maximum_gpfn(xc_interface *xch, domid_t domid);
+
+int xc_domain_nr_gpfns(xc_interface *xch, domid_t domid, xen_pfn_t *gpfns);
 
 int xc_domain_increase_reservation(xc_interface *xch,
                                    uint32_t domid,
diff --git a/tools/libxc/xc_core_arm.c b/tools/libxc/xc_core_arm.c
index c3f5868..57d4715 100644
--- a/tools/libxc/xc_core_arm.c
+++ b/tools/libxc/xc_core_arm.c
@@ -45,7 +45,7 @@ xc_core_arch_memory_map_get(xc_interface *xch, struct 
xc_core_arch_context *unus
     xen_pfn_t p2m_size = 0;
     xc_core_memory_map_t *map;
 
-    if ( xc_domain_maximum_gpfn(xch, info->domid, &p2m_size) < 0 )
+    if ( xc_domain_nr_gpfns(xch, info->domid, &p2m_size) < 0 )
         return -1;
 
     map = malloc(sizeof(*map));
diff --git a/tools/libxc/xc_core_x86.c b/tools/libxc/xc_core_x86.c
index 4552e43..93ebcbb 100644
--- a/tools/libxc/xc_core_x86.c
+++ b/tools/libxc/xc_core_x86.c
@@ -50,7 +50,7 @@ xc_core_arch_memory_map_get(xc_interface *xch, struct 
xc_core_arch_context *unus
     xen_pfn_t p2m_size = 0;
     xc_core_memory_map_t *map;
 
-    if ( xc_domain_maximum_gpfn(xch, info->domid, &p2m_size) < 0 )
+    if ( xc_domain_nr_gpfns(xch, info->domid, &p2m_size) < 0 )
         return -1;
 
     map = malloc(sizeof(*map));
@@ -85,7 +85,7 @@ xc_core_arch_map_p2m_rw(xc_interface *xch, struct 
domain_info_context *dinfo, xc
     int err;
     int i;
 
-    if ( xc_domain_maximum_gpfn(xch, info->domid, &dinfo->p2m_size) < 0 )
+    if ( xc_domain_nr_gpfns(xch, info->domid, &dinfo->p2m_size) < 0 )
     {
         ERROR("Could not get maximum GPFN!");
         goto out;
@@ -212,7 +212,7 @@ int
 xc_core_arch_get_scratch_gpfn(xc_interface *xch, domid_t domid,
                               xen_pfn_t *gpfn)
 {
-    return xc_domain_maximum_gpfn(xch, domid, gpfn);
+    return xc_domain_nr_gpfns(xch, domid, gpfn);
 }
 
 /*
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index da88e08..ddaa872 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -788,8 +788,12 @@ int xc_domain_get_tsc_info(xc_interface *xch,
     return rc;
 }
 
+int xc_domain_maximum_gpfn(xc_interface *xch, domid_t domid)
+{
+    return do_memory_op(xch, XENMEM_maximum_gpfn, &domid, sizeof(domid));
+}
 
-int xc_domain_maximum_gpfn(xc_interface *xch, domid_t domid, xen_pfn_t *gpfns)
+int xc_domain_nr_gpfns(xc_interface *xch, domid_t domid, xen_pfn_t *gpfns)
 {
     int rc = do_memory_op(xch, XENMEM_maximum_gpfn, &domid, sizeof(domid));
 
diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
index b611c07..cef6995 100644
--- a/tools/libxc/xc_domain_save.c
+++ b/tools/libxc/xc_domain_save.c
@@ -939,7 +939,7 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t 
dom, uint32_t max_iter
     }
 
     /* Get the size of the P2M table */
-    if ( xc_domain_maximum_gpfn(xch, dom, &dinfo->p2m_size) < 0 )
+    if ( xc_domain_nr_gpfns(xch, dom, &dinfo->p2m_size) < 0 )
     {
         ERROR("Could not get maximum GPFN!");
         goto out;
-- 
2.1.0


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel