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

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

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@citrix.com>
>> CC: Keir Fraser <keir@xen.org>
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Ian Campbell <Ian.Campbell@citrix.com>
>>
>> ---
>>
>> 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

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