Re: [Patch] common/memory: Fix ABI breakage for XENMEM_add_to_physmap

[Keir Fraser <keir.xen@gmail.com>]

On 14/01/2014 20:21, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote:

>   caused by c/s 4be86bb194e25e46b6cbee900601bfee76e8090a
> 
> In public/memory.h, struct xen_add_to_physmap has 'space' as an unsigned int,
> but struct xen_add_to_physmap_batch has 'space' as a uint16_t.
> 
> By defining xenmem_add_to_physmap_one() with space defined as uint16_t, the
> now-common xenmem_add_to_physmap() implicitly truncates xatp->space from
> unsigned int to uint16_t, which changes the space switch()'d upon.
> 
> This wouldn't be noticed with any upstream code (of which I am aware), but was
> discovered because of the XenServer support for legacy Windows PV drivers,
> which make XENMEM_add_to_physmap hypercalls using spaces with the top bit set.
> The current Windows PV drivers don't do this any more, but we 'fix' Xen to
> support running VMs with out-of-date tools.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>

Acked-by: Keir Fraser <keir@xen.org>

> ---
> 
> As this breakage was caused between 4.4-rc1 and -rc2, I request a release ack
> for the fix.
> 
> This was caught by a compile failure rather than a functional test.  I have
> encountered a different compile error which turns out to be a bug in the cross
> compiler we are currently using, so I need to fix that before I can
> functionally test a 4.4-rc2 based XenServer.  (Which will be a rather better
> test of whether the functionality of XENMEM_add_to_physmap is actually still
> the same.  If anyone dares look,
> https://github.com/xenserver/xen-4.3.pg/blob/master/xen-legacy-win-xenmapspace
> -quirks.patch
> are the hacks required to make the legacy drivers work on modern Xen.)
> ---
>  xen/arch/arm/mm.c    |    2 +-
>  xen/arch/x86/mm.c    |    2 +-
>  xen/include/xen/mm.h |    2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 293b6e2..127cce0 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -970,7 +970,7 @@ void share_xen_page_with_privileged_guests(
>  
>  int xenmem_add_to_physmap_one(
>      struct domain *d,
> -    uint16_t space,
> +    unsigned int space,
>      domid_t foreign_domid,
>      unsigned long idx,
>      xen_pfn_t gpfn)
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 32c0473..172c68c 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4522,7 +4522,7 @@ static int handle_iomem_range(unsigned long s, unsigned
> long e, void *p)
>  
>  int xenmem_add_to_physmap_one(
>      struct domain *d,
> -    uint16_t space,
> +    unsigned int space,
>      domid_t foreign_domid,
>      unsigned long idx,
>      xen_pfn_t gpfn)
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index f90ed74..b183189 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -356,7 +356,7 @@ static inline unsigned int get_order_from_pages(unsigned
> long nr_pages)
>  
>  void scrub_one_page(struct page_info *);
>  
> -int xenmem_add_to_physmap_one(struct domain *d, uint16_t space,
> +int xenmem_add_to_physmap_one(struct domain *d, unsigned int space,
>                                domid_t foreign_domid,
>                                unsigned long idx, xen_pfn_t gpfn);
>