From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [Patch] common/memory: Fix ABI breakage for XENMEM_add_to_physmap Date: Wed, 15 Jan 2014 09:57:08 +0000 Message-ID: <52D65B74.9070604@citrix.com> References: <1389730896-29439-1-git-send-email-andrew.cooper3@citrix.com> <1389779590.12434.131.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1389779590.12434.131.camel@kazak.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: Keir Fraser , Jan Beulich , Xen-devel List-Id: xen-devel@lists.xenproject.org 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 >> CC: Keir Fraser >> CC: Jan Beulich >> CC: Ian Campbell >> >> --- >> >> 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