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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2014-01-15 at 09:57 +0000, Andrew Cooper wrote:
> On 15/01/14 09:53, Ian Campbell wrote:
> > On Tue, 2014-01-14 at 20:21 +0000, Andrew Cooper 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@xxxxxxxxxx>
> >> CC: Keir Fraser <keir@xxxxxxx>
> >> CC: Jan Beulich <JBeulich@xxxxxxxx>
> >> CC: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
> >>
> >> ---
> >>
> >> As this breakage was caused between 4.4-rc1 and -rc2,
> > That's certainly a good indicator, but you've not covered the actual
> > risks and rewards of making this change now:
> > http://wiki.xen.org/wiki/Xen_Roadmap/4.4#Exception_guidelines_for_after_the_code_freeze
> >
> > Please can you do so.
> >
> >
> 
> Contributes towards #1 "Bug-free release"
> 
> Risks:
>  * We now know we have an ABI regression
>  * It is a fairly obvious fix which is unlikely to have hidden issues
> itself.
> 
> Rewards:
>  * We keep the hypervisor ABI compatible with Xen 4.3

IMHO it already is -- the 4.4 ABI is not broken because the truncated
bits are not used in the Xen ABI, 4.4 accepts everything which 4.3 does.
We still very much have the option of deferring this change to 4.5
and/or when the bits become used, with no risk to the Xen 4.4 release.

Please try and consider the guidelines exceptions with an unbiased eye,
rather than just as a mechanism for reconfirming your existing belief
that the patch should go in.

I was tempted to reject this patch just to make a point, but I think if
I'm being honest it probably should go in, so IFF Jan concurs with
"fairly obvious fix which is unlikely to have hidden issues":

Release-Ack: Ian Campbell

Next time I might be in a worse mood.

Ian.

> Alternatives:
>  * Revert the patch which introduced the regression, but that is very
> undesirable as it was fixing another long-running Xen operation, and
> common-ifying some code between x86 and arm
> 
> ~Andrew



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