From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH] libxl: fix "xl mem-set" regression from 0c029c4da2 Date: Wed, 13 May 2015 14:46:28 +0100 Message-ID: <1431524788.8263.278.camel@citrix.com> References: <5537A9E80200007800074B19@mail.emea.novell.com> <1429711278.30934.14.camel@citrix.com> <5537CF2A0200007800074C20@mail.emea.novell.com> <1429716987.30934.44.camel@citrix.com> <5537E9710200007800074DFD@mail.emea.novell.com> <1429725323.6174.25.camel@citrix.com> <555316DE0200007800079D60@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YsX8J-0002gh-HV for xen-devel@lists.xenproject.org; Wed, 13 May 2015 13:55:35 +0000 In-Reply-To: <555316DE0200007800079D60@mail.emea.novell.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: Jan Beulich , Stefano Stabellini Cc: Ian Jackson , Wei Liu , xen-devel List-Id: xen-devel@lists.xenproject.org On Wed, 2015-05-13 at 08:18 +0100, Jan Beulich wrote: > >>> On 22.04.15 at 19:55, wrote: > > On Wed, 2015-04-22 at 17:33 +0100, Jan Beulich wrote: > >> >>> On 22.04.15 at 17:36, wrote: > >> > On Wed, 2015-04-22 at 15:41 +0100, Jan Beulich wrote: > >> >> >>> On 22.04.15 at 16:01, wrote: > >> >> > On Wed, 2015-04-22 at 13:02 +0100, Jan Beulich wrote: > >> >> >> Said commit ("libxl_set_memory_target: retain the same maxmem offset on > >> >> >> top of the current target") caused a regression for "xl mem-set" > >> >> >> against Dom0: While prior to creation of the first domain this works, > >> >> >> the first domain creation involving ballooning breaks. Due to "enforce" > >> >> >> not being set in the domain creation case, and due to Dom0's initial > >> >> >> ->max_pages (in the hypervisor) being UINT_MAX, the calculation of > >> >> >> "memorykb" in the first "xl mem-set" adusting the target upwards > >> >> >> subsequent to domain creation and termination may cause an overflow, > >> >> >> resulting in Dom0's maximum getting to a very small value. This small > >> >> >> maximum will the make the subsequent setting of the PoD target fail. > >> >> >> > >> >> >> Signed-off-by: Jan Beulich > >> >> >> --- > >> >> >> Note that this only fixes the immediate problem - there appear to be > >> >> >> further issues lurking here: > >> >> >> - libxl_set_memory_target()'s *_memkb variables all being 32-bit, > >> >> >> - libxl_domain_setmaxmem()'s max_memkb parameter being 32-bit, > >> >> > > >> >> > I think that increasing the width of these variables wouldn't break the > >> >> > API guarantee which we make, at least not in a practical way, since any > >> >> > existing 32-bit arguments passed will just get promoted. > >> >> > >> >> No, not even on 64-bit. On 32-bit, two arguments slots are needed > >> >> for what so far requires only one. On 64-bit (at least x86-64), the > >> >> calling code isn't required to zero-extend a value calculated in a > >> >> register (e.g. a result of earlier calculations which had more than > >> >> 32 significant bits could be passed unchanged to the called function); > >> >> it just so happens that 32-bit arithmetic on registers would always > >> >> implicitly zero the upper halves (and iirc that's the same on ARM64). > >> > > >> > You seem to be talking about ABI? As I tried to note in my response for > >> > libxl we only make guarantees about the API (P not B in the middle). > >> > >> Oh, okay. That would mean the libxl side is fine. The change to the > >> libxc interface might however still be a problem for qemu (Stefano > >> was telling me something to the effect of a compatibility layer in > >> upstream qemu to deal with such version differences, but of course > >> the implication - without versioned libxc interfaces - would still be > >> that existing qemu binaries on top of updated libxc would no > >> longer work). > > > > Correct, but we would surely bump the SONAME for a change such as this, > > so it wouldn't matter in practice. > > So what's the situation with this patch? Can it go in? Is someone > working on a better fix for the described problem? Stefano, Are you? Regardless it seems to me that this patch is correct in its own right, having maxmem_kb be a 64-bit type is the correct type for it to have. Ian.