On Fri, 12 Aug 2016, Paul Durrant wrote: > > -----Original Message----- > > From: Stefano Stabellini [mailto:sstabell...@kernel.org] > > Sent: 11 August 2016 21:07 > > To: Paul Durrant > > Cc: Anthony Perard; xen-de...@lists.xenproject.org; qemu- > > de...@nongnu.org; Stefano Stabellini > > Subject: RE: [PATCH] xen: handle inbound migration of VMs without ioreq > > server pages > > > > On Tue, 9 Aug 2016, Paul Durrant wrote: > > > > -----Original Message----- > > > > From: Anthony PERARD [mailto:anthony.per...@citrix.com] > > > > Sent: 09 August 2016 16:19 > > > > To: Paul Durrant > > > > Cc: xen-de...@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 case > > > > > 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 simply > > > > > as an indication that the guest is too old to support the ioreq server > > > > > API. Instead a boolean is set to cause reversion to old behaviour such > > > > > that the default ioreq server is then used. > > > > > > > > > > Signed-off-by: Paul Durrant <paul.durr...@citrix.com> > > > > > Cc: Stefano Stabellini <sstabell...@kernel.org> > > > > > Cc: Anthony Perard <anthony.per...@citrix.com> > > > > > > > > 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 > > > > *bufioreq_evtchn) > > > > > > > > With that fixed: Acked-by: Anthony PERARD > > <anthony.per...@citrix.com> > > > > > > > > Thanks, > > > > > > > > > > Ok, I'll post v2 with those fixes and your ack. Thanks, > > > > Thank you for fixing this bug and Thanks Anthony for the review. > > > > 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. > > > > Would you be up for rewriting the fix so that instead of introducing > > > > if (use_default_ioreq_server) { > > return; > > } > > > > 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? > > > > > > The call sites would be something like: > > > > ioreq_server->unmap_io_range_from_ioreq_server(xc, dom, ioservid, 0, > > start_addr, end_addr); > > > > At boot time, if xc_hvm_create_ioreq_server returns error: > > > > ioreq_server = empty_stubs_functions; > > > > else > > > > ioreq_server = useful_functions; > > > > > > 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.
All right, given that Anthony already acked it, it's two vs. one. I'll commit it as is.
