* [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.