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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.01.14 at 11:35, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> 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_c 
> ode_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.

Not exactly: 4.4 now also accepts what 4.3 would reject.

> 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":

I already did in an earlier reply (or at least it was meant to be that
way).

> Release-Ack: Ian Campbell

Thanks.

> Next time I might be in a worse mood.

Hopefully not too much worse ;-)

Jan


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