From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Vrabel Subject: Re: [Patch] common/memory: Fix ABI breakage for XENMEM_add_to_physmap Date: Wed, 15 Jan 2014 10:49:18 +0000 Message-ID: <52D667AE.3040506@citrix.com> References: <1389730896-29439-1-git-send-email-andrew.cooper3@citrix.com> <1389779590.12434.131.camel@kazak.uk.xensource.com> <52D65B74.9070604@citrix.com> <1389782102.12434.163.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: <1389782102.12434.163.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: Andrew Cooper , Keir Fraser , Jan Beulich , Xen-devel List-Id: xen-devel@lists.xenproject.org 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 >>>> 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 > > 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