* [PATCH for-4.5] reset PCI devices on force removal even when QEMU returns error @ 2014-11-28 16:53 Stefano Stabellini 2014-12-02 13:43 ` Ian Campbell 2014-12-12 15:13 ` Sander Eikelenboom 0 siblings, 2 replies; 9+ messages in thread From: Stefano Stabellini @ 2014-11-28 16:53 UTC (permalink / raw) To: xen-devel Cc: Wei Liu (Intern), Ian Campbell, Stefano Stabellini, Ian Jackson, linux On do_pci_remove when QEMU returns error, we just bail out early without resetting the device. On domain shutdown we are racing with QEMU exiting and most often QEMU closes the QMP connection before executing the requested command. In these cases if force=1, it makes sense to go ahead with rest of the PCI device removal, that includes resetting the device and calling xc_deassign_device. Otherwise we risk not resetting the device properly on domain shutdown. Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c index 316643c..0ac0b93 100644 --- a/tools/libxl/libxl_pci.c +++ b/tools/libxl/libxl_pci.c @@ -1243,7 +1245,7 @@ static int do_pci_remove(libxl__gc *gc, uint32_t domid, rc = ERROR_INVAL; goto out_fail; } - if (rc) { + if (rc && !force) { rc = ERROR_FAIL; goto out_fail; } ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH for-4.5] reset PCI devices on force removal even when QEMU returns error 2014-11-28 16:53 [PATCH for-4.5] reset PCI devices on force removal even when QEMU returns error Stefano Stabellini @ 2014-12-02 13:43 ` Ian Campbell 2014-12-02 15:09 ` Boris Ostrovsky 2014-12-12 15:13 ` Sander Eikelenboom 1 sibling, 1 reply; 9+ messages in thread From: Ian Campbell @ 2014-12-02 13:43 UTC (permalink / raw) To: Stefano Stabellini Cc: xen-devel, Wei Liu (Intern), Ian Jackson, linux, Boris Ostrovsky On Fri, 2014-11-28 at 16:53 +0000, Stefano Stabellini wrote: CCing Boris because he was fixing a similar sounding issue in the same area. Not sure if this is related to those patches or not. > On do_pci_remove when QEMU returns error, we just bail out early without > resetting the device. On domain shutdown we are racing with QEMU exiting > and most often QEMU closes the QMP connection before executing the > requested command. > > In these cases if force=1, it makes sense to go ahead with rest of the > PCI device removal, that includes resetting the device and calling > xc_deassign_device. Otherwise we risk not resetting the device properly > on domain shutdown. ISTR seeing a conversation along the lines of there being a better solution which was more 4.6 material, is that right? > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> Acked-by: Ian Campbell <ian.campbell@citrix.com> But, I'd prefer to see a version which logs when the qemu side has failed but it is continuing. Probably just adding right after the existing if: if (rc) LOG("Something appropriate"); would do the trick. Also this needs RM input from Konrad. > > diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c > index 316643c..0ac0b93 100644 > --- a/tools/libxl/libxl_pci.c > +++ b/tools/libxl/libxl_pci.c > @@ -1243,7 +1245,7 @@ static int do_pci_remove(libxl__gc *gc, uint32_t domid, > rc = ERROR_INVAL; > goto out_fail; > } > - if (rc) { > + if (rc && !force) { > rc = ERROR_FAIL; > goto out_fail; > } ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH for-4.5] reset PCI devices on force removal even when QEMU returns error 2014-12-02 13:43 ` Ian Campbell @ 2014-12-02 15:09 ` Boris Ostrovsky 0 siblings, 0 replies; 9+ messages in thread From: Boris Ostrovsky @ 2014-12-02 15:09 UTC (permalink / raw) To: Ian Campbell, Stefano Stabellini Cc: Wei Liu (Intern), xen-devel, Ian Jackson, linux On 12/02/2014 08:43 AM, Ian Campbell wrote: > On Fri, 2014-11-28 at 16:53 +0000, Stefano Stabellini wrote: > > CCing Boris because he was fixing a similar sounding issue in the same > area. Not sure if this is related to those patches or not. Not really. I was trying to fix something that another Stefano's patch from yesterday is trying to address: http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg00160.html although I thought that removing the call to xc_domain_irq_permission() would be the solution. (which reminds me --- and maybe I shouldn't overload this thread --- that this patch from IanJ needs to get in as well: http://lists.xenproject.org/archives/html/xen-devel/2014-11/msg01342.html ) Thanks. -boris > >> On do_pci_remove when QEMU returns error, we just bail out early without >> resetting the device. On domain shutdown we are racing with QEMU exiting >> and most often QEMU closes the QMP connection before executing the >> requested command. >> >> In these cases if force=1, it makes sense to go ahead with rest of the >> PCI device removal, that includes resetting the device and calling >> xc_deassign_device. Otherwise we risk not resetting the device properly >> on domain shutdown. > ISTR seeing a conversation along the lines of there being a better > solution which was more 4.6 material, is that right? > >> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > Acked-by: Ian Campbell <ian.campbell@citrix.com> > > But, I'd prefer to see a version which logs when the qemu side has > failed but it is continuing. Probably just adding right after the > existing if: > if (rc) > LOG("Something appropriate"); > would do the trick. > > Also this needs RM input from Konrad. > >> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c >> index 316643c..0ac0b93 100644 >> --- a/tools/libxl/libxl_pci.c >> +++ b/tools/libxl/libxl_pci.c >> @@ -1243,7 +1245,7 @@ static int do_pci_remove(libxl__gc *gc, uint32_t domid, >> rc = ERROR_INVAL; >> goto out_fail; >> } >> - if (rc) { >> + if (rc && !force) { >> rc = ERROR_FAIL; >> goto out_fail; >> } > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH for-4.5] reset PCI devices on force removal even when QEMU returns error 2014-11-28 16:53 [PATCH for-4.5] reset PCI devices on force removal even when QEMU returns error Stefano Stabellini 2014-12-02 13:43 ` Ian Campbell @ 2014-12-12 15:13 ` Sander Eikelenboom 2014-12-12 16:50 ` Konrad Rzeszutek Wilk 1 sibling, 1 reply; 9+ messages in thread From: Sander Eikelenboom @ 2014-12-12 15:13 UTC (permalink / raw) To: Stefano Stabellini; +Cc: Wei Liu (Intern), xen-devel, Ian Jackson, Ian Campbell Hi Konrad, This doesn't seem to be applied yet, nor does it seem to have a release-(N)ACK from you ? -- Sander Friday, November 28, 2014, 5:53:09 PM, you wrote: > On do_pci_remove when QEMU returns error, we just bail out early without > resetting the device. On domain shutdown we are racing with QEMU exiting > and most often QEMU closes the QMP connection before executing the > requested command. > In these cases if force=1, it makes sense to go ahead with rest of the > PCI device removal, that includes resetting the device and calling > xc_deassign_device. Otherwise we risk not resetting the device properly > on domain shutdown. > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c > index 316643c..0ac0b93 100644 > --- a/tools/libxl/libxl_pci.c > +++ b/tools/libxl/libxl_pci.c > @@ -1243,7 +1245,7 @@ static int do_pci_remove(libxl__gc *gc, uint32_t domid, > rc = ERROR_INVAL; > goto out_fail; > } > - if (rc) { > + if (rc && !force) { > rc = ERROR_FAIL; > goto out_fail; > } ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH for-4.5] reset PCI devices on force removal even when QEMU returns error 2014-12-12 15:13 ` Sander Eikelenboom @ 2014-12-12 16:50 ` Konrad Rzeszutek Wilk 2014-12-15 11:13 ` Stefano Stabellini 0 siblings, 1 reply; 9+ messages in thread From: Konrad Rzeszutek Wilk @ 2014-12-12 16:50 UTC (permalink / raw) To: Sander Eikelenboom Cc: Wei Liu (Intern), xen-devel, Ian Jackson, Ian Campbell, Stefano Stabellini On Fri, Dec 12, 2014 at 04:13:52PM +0100, Sander Eikelenboom wrote: > Hi Konrad, > > This doesn't seem to be applied yet, nor does it seem to have a release-(N)ACK > from you ? Hm, Stefano: - Is this a regression? - What are the risks of this not going in? I presume that it just means we haven't reset it in sysfs. But the xc_deassign_device operation if not done will not affect the hypervisor - which will move the device to dom0 upon guest teardown. > > -- > Sander > > > > Friday, November 28, 2014, 5:53:09 PM, you wrote: > > > On do_pci_remove when QEMU returns error, we just bail out early without > > resetting the device. On domain shutdown we are racing with QEMU exiting > > and most often QEMU closes the QMP connection before executing the > > requested command. > > > In these cases if force=1, it makes sense to go ahead with rest of the > > PCI device removal, that includes resetting the device and calling > > xc_deassign_device. Otherwise we risk not resetting the device properly > > on domain shutdown. > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > > diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c > > index 316643c..0ac0b93 100644 > > --- a/tools/libxl/libxl_pci.c > > +++ b/tools/libxl/libxl_pci.c > > @@ -1243,7 +1245,7 @@ static int do_pci_remove(libxl__gc *gc, uint32_t domid, > > rc = ERROR_INVAL; > > goto out_fail; > > } > > - if (rc) { > > + if (rc && !force) { > > rc = ERROR_FAIL; > > goto out_fail; > > } > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH for-4.5] reset PCI devices on force removal even when QEMU returns error 2014-12-12 16:50 ` Konrad Rzeszutek Wilk @ 2014-12-15 11:13 ` Stefano Stabellini 2014-12-17 21:07 ` Konrad Rzeszutek Wilk 0 siblings, 1 reply; 9+ messages in thread From: Stefano Stabellini @ 2014-12-15 11:13 UTC (permalink / raw) To: Konrad Rzeszutek Wilk Cc: xen-devel, Wei Liu (Intern), Ian Campbell, Stefano Stabellini, Ian Jackson, Sander Eikelenboom On Fri, 12 Dec 2014, Konrad Rzeszutek Wilk wrote: > On Fri, Dec 12, 2014 at 04:13:52PM +0100, Sander Eikelenboom wrote: > > Hi Konrad, > > > > This doesn't seem to be applied yet, nor does it seem to have a release-(N)ACK > > from you ? > > Hm, Stefano: > > - Is this a regression? I don't think so. Probably a regression compared to the xend toolstack though. > - What are the risks of this not going in? I presume that it just means > we haven't reset it in sysfs. But the xc_deassign_device operation > if not done will not affect the hypervisor - which will move the > device to dom0 upon guest teardown. The device becomes unusable until somebody manually resets it. > > > > -- > > Sander > > > > > > > > Friday, November 28, 2014, 5:53:09 PM, you wrote: > > > > > On do_pci_remove when QEMU returns error, we just bail out early without > > > resetting the device. On domain shutdown we are racing with QEMU exiting > > > and most often QEMU closes the QMP connection before executing the > > > requested command. > > > > > In these cases if force=1, it makes sense to go ahead with rest of the > > > PCI device removal, that includes resetting the device and calling > > > xc_deassign_device. Otherwise we risk not resetting the device properly > > > on domain shutdown. > > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > > > > diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c > > > index 316643c..0ac0b93 100644 > > > --- a/tools/libxl/libxl_pci.c > > > +++ b/tools/libxl/libxl_pci.c > > > @@ -1243,7 +1245,7 @@ static int do_pci_remove(libxl__gc *gc, uint32_t domid, > > > rc = ERROR_INVAL; > > > goto out_fail; > > > } > > > - if (rc) { > > > + if (rc && !force) { > > > rc = ERROR_FAIL; > > > goto out_fail; > > > } > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH for-4.5] reset PCI devices on force removal even when QEMU returns error 2014-12-15 11:13 ` Stefano Stabellini @ 2014-12-17 21:07 ` Konrad Rzeszutek Wilk 2015-01-06 11:45 ` Ian Campbell 0 siblings, 1 reply; 9+ messages in thread From: Konrad Rzeszutek Wilk @ 2014-12-17 21:07 UTC (permalink / raw) To: Stefano Stabellini Cc: Sander Eikelenboom, xen-devel, Ian Jackson, Ian Campbell, Wei Liu (Intern) On Mon, Dec 15, 2014 at 11:13:06AM +0000, Stefano Stabellini wrote: > On Fri, 12 Dec 2014, Konrad Rzeszutek Wilk wrote: > > On Fri, Dec 12, 2014 at 04:13:52PM +0100, Sander Eikelenboom wrote: > > > Hi Konrad, > > > > > > This doesn't seem to be applied yet, nor does it seem to have a release-(N)ACK > > > from you ? > > > > Hm, Stefano: > > > > - Is this a regression? > > I don't think so. Probably a regression compared to the xend toolstack > though. OK, so that is Xen 4.4 -> Xen 4.5 regression then. Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Thanks. > > > > - What are the risks of this not going in? I presume that it just means > > we haven't reset it in sysfs. But the xc_deassign_device operation > > if not done will not affect the hypervisor - which will move the > > device to dom0 upon guest teardown. > > The device becomes unusable until somebody manually resets it. > > > > > > > > -- > > > Sander > > > > > > > > > > > > Friday, November 28, 2014, 5:53:09 PM, you wrote: > > > > > > > On do_pci_remove when QEMU returns error, we just bail out early without > > > > resetting the device. On domain shutdown we are racing with QEMU exiting > > > > and most often QEMU closes the QMP connection before executing the > > > > requested command. > > > > > > > In these cases if force=1, it makes sense to go ahead with rest of the > > > > PCI device removal, that includes resetting the device and calling > > > > xc_deassign_device. Otherwise we risk not resetting the device properly > > > > on domain shutdown. > > > > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > > > > > > diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c > > > > index 316643c..0ac0b93 100644 > > > > --- a/tools/libxl/libxl_pci.c > > > > +++ b/tools/libxl/libxl_pci.c > > > > @@ -1243,7 +1245,7 @@ static int do_pci_remove(libxl__gc *gc, uint32_t domid, > > > > rc = ERROR_INVAL; > > > > goto out_fail; > > > > } > > > > - if (rc) { > > > > + if (rc && !force) { > > > > rc = ERROR_FAIL; > > > > goto out_fail; > > > > } > > > > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH for-4.5] reset PCI devices on force removal even when QEMU returns error 2014-12-17 21:07 ` Konrad Rzeszutek Wilk @ 2015-01-06 11:45 ` Ian Campbell 2015-01-06 14:02 ` Sander Eikelenboom 0 siblings, 1 reply; 9+ messages in thread From: Ian Campbell @ 2015-01-06 11:45 UTC (permalink / raw) To: Konrad Rzeszutek Wilk Cc: Sander Eikelenboom, xen-devel, Ian Jackson, Wei Liu (Intern), Stefano Stabellini On Wed, 2014-12-17 at 16:07 -0500, Konrad Rzeszutek Wilk wrote: > On Mon, Dec 15, 2014 at 11:13:06AM +0000, Stefano Stabellini wrote: > > On Fri, 12 Dec 2014, Konrad Rzeszutek Wilk wrote: > > > On Fri, Dec 12, 2014 at 04:13:52PM +0100, Sander Eikelenboom wrote: > > > > Hi Konrad, > > > > > > > > This doesn't seem to be applied yet, nor does it seem to have a release-(N)ACK > > > > from you ? > > > > > > Hm, Stefano: > > > > > > - Is this a regression? > > > > I don't think so. Probably a regression compared to the xend toolstack > > though. > > OK, so that is Xen 4.4 -> Xen 4.5 regression then. > > Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> An updated version with the logging which I had indicated I would prefer doesn't appear to be forthcoming so I've just applied this one. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH for-4.5] reset PCI devices on force removal even when QEMU returns error 2015-01-06 11:45 ` Ian Campbell @ 2015-01-06 14:02 ` Sander Eikelenboom 0 siblings, 0 replies; 9+ messages in thread From: Sander Eikelenboom @ 2015-01-06 14:02 UTC (permalink / raw) To: Ian Campbell; +Cc: Wei Liu (Intern), Stefano Stabellini, Ian Jackson, xen-devel Tuesday, January 6, 2015, 12:45:17 PM, you wrote: > On Wed, 2014-12-17 at 16:07 -0500, Konrad Rzeszutek Wilk wrote: >> On Mon, Dec 15, 2014 at 11:13:06AM +0000, Stefano Stabellini wrote: >> > On Fri, 12 Dec 2014, Konrad Rzeszutek Wilk wrote: >> > > On Fri, Dec 12, 2014 at 04:13:52PM +0100, Sander Eikelenboom wrote: >> > > > Hi Konrad, >> > > > >> > > > This doesn't seem to be applied yet, nor does it seem to have a release-(N)ACK >> > > > from you ? >> > > >> > > Hm, Stefano: >> > > >> > > - Is this a regression? >> > >> > I don't think so. Probably a regression compared to the xend toolstack >> > though. >> >> OK, so that is Xen 4.4 -> Xen 4.5 regression then. >> >> Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > An updated version with the logging which I had indicated I would prefer > doesn't appear to be forthcoming so I've just applied this one. I wasn't aware of that, thx for applying ! -- Sander ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-01-06 14:02 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-11-28 16:53 [PATCH for-4.5] reset PCI devices on force removal even when QEMU returns error Stefano Stabellini 2014-12-02 13:43 ` Ian Campbell 2014-12-02 15:09 ` Boris Ostrovsky 2014-12-12 15:13 ` Sander Eikelenboom 2014-12-12 16:50 ` Konrad Rzeszutek Wilk 2014-12-15 11:13 ` Stefano Stabellini 2014-12-17 21:07 ` Konrad Rzeszutek Wilk 2015-01-06 11:45 ` Ian Campbell 2015-01-06 14:02 ` Sander Eikelenboom
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.