From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34564) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bYDLI-0005DG-TW for qemu-devel@nongnu.org; Fri, 12 Aug 2016 10:21:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bYDLA-0002Py-4N for qemu-devel@nongnu.org; Fri, 12 Aug 2016 10:21:47 -0400 Received: from smtp.eu.citrix.com ([185.25.65.24]:31827) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bYDL9-0002Nx-QN for qemu-devel@nongnu.org; Fri, 12 Aug 2016 10:21:40 -0400 From: Paul Durrant Date: Fri, 12 Aug 2016 08:55:05 +0000 Message-ID: <7d745399f8754cbea9955e818776861c@AMSPEX02CL03.citrite.net> References: <1470042985-680-1-git-send-email-paul.durrant@citrix.com> <20160809151912.GC1835@perard.uk.xensource.com> <339fd6631cba40518264719ba8cf2018@AMSPEX02CL03.citrite.net> In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] xen: handle inbound migration of VMs without ioreq server pages List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefano Stabellini Cc: Anthony Perard , "xen-devel@lists.xenproject.org" , "qemu-devel@nongnu.org" > -----Original Message----- > From: Stefano Stabellini [mailto:sstabellini@kernel.org] > Sent: 11 August 2016 21:07 > To: Paul Durrant > Cc: Anthony Perard; xen-devel@lists.xenproject.org; qemu- > devel@nongnu.org; Stefano Stabellini > Subject: RE: [PATCH] xen: handle inbound migration of VMs without ioreq > server pages >=20 > On Tue, 9 Aug 2016, Paul Durrant wrote: > > > -----Original Message----- > > > From: Anthony PERARD [mailto:anthony.perard@citrix.com] > > > Sent: 09 August 2016 16:19 > > > To: Paul Durrant > > > Cc: xen-devel@lists.xenproject.org; qemu-devel@nongnu.org; Stefano > > > Stabellini > > > Subject: Re: [PATCH] xen: handle inbound migration of VMs without > ioreq > > > server pages > > > > > > On Mon, Aug 01, 2016 at 10:16:25AM +0100, Paul Durrant wrote: > > > > VMs created on older versions on Xen will not have been provisioned > with > > > > pages to support creation of non-default ioreq servers. In this cas= e > > > > the ioreq server API is not supported and QEMU's only option is to = fall > > > > back to using the default ioreq server pages as it did prior to > > > > commit 3996e85c ("Xen: Use the ioreq-server API when available"). > > > > > > > > This patch therefore changes the code in xen_common.h to stop > > > considering > > > > a failure of xc_hvm_create_ioreq_server() as a hard failure but sim= ply > > > > as an indication that the guest is too old to support the ioreq ser= ver > > > > API. Instead a boolean is set to cause reversion to old behaviour s= uch > > > > that the default ioreq server is then used. > > > > > > > > Signed-off-by: Paul Durrant > > > > Cc: Stefano Stabellini > > > > Cc: Anthony Perard > > > > > > There are just two lines too long: > > > > > > WARNING: line over 80 characters > > > #31: FILE: include/hw/xen/xen_common.h:110: > > > +static inline int xen_get_default_ioreq_server_info(xc_interface *xc= , > > > domid_t dom, > > > > > > WARNING: line over 80 characters > > > #34: FILE: include/hw/xen/xen_common.h:113: > > > + evtchn_port_t *b= ufioreq_evtchn) > > > > > > With that fixed: Acked-by: Anthony PERARD > > > > > > > Thanks, > > > > > > > Ok, I'll post v2 with those fixes and your ack. Thanks, >=20 > Thank you for fixing this bug and Thanks Anthony for the review. >=20 > I was about to commit it but my sense of style rebelled: I really don't > like all the if statements. Too many! Sorry for coming in so late in > commenting on a patch, I realize that it is unfair. >=20 > Would you be up for rewriting the fix so that instead of introducing >=20 > if (use_default_ioreq_server) { > return; > } >=20 > in many functions, we turn xc_hvm_* calls into function pointers calls > that get set to the right behavior depending on the success of > xc_hvm_create_ioreq_server? >=20 >=20 > The call sites would be something like: >=20 > ioreq_server->unmap_io_range_from_ioreq_server(xc, dom, ioservid, 0, > start_addr, end_addr); >=20 > At boot time, if xc_hvm_create_ioreq_server returns error: >=20 > ioreq_server =3D empty_stubs_functions; >=20 > else >=20 > ioreq_server =3D useful_functions; >=20 >=20 > What do you guys think? Personally I don't think it's worth it. This is not a performance critical = codepath but if you insist I will re-factor the code. Paul