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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 27/03/15 20:41, Konrad Rzeszutek Wilk wrote:
> 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@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>
>> 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@oracle.com>
> 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@citrix.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  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);

It is perfectly fine for xc_domain_maximum_gpfn() to keep its *gpfn
pointer, as this is a rather more sane interface which doesn't truncate
the hypervisor long in a 64bit toolstack.

~Andrew