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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2014-01-15 at 10:44 +0000, Jan Beulich wrote:
> >>> 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.

That is a valid point, thanks. With that having been pointed out I think
it is pretty obvious that this should go in.

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

I saw your reviewed by but didn't know if it applied to 4.4 or 4.5 so I
wanted check.

Ian.


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