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

[David Vrabel <david.vrabel@citrix.com>]

On 15/01/14 10:35, Ian Campbell 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@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
> 
> 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.

It is a guest visible change as it changes the behaviour if the guest
supplies space >= 0x1000.  e.g., space == 0x1000 would be truncated and
it would operate on space == 0x0000 and (potentially) return sucesss
instead of an -EINVAL error.

David