Ian Campbell
2011-Mar-08 11:49 UTC
[Xen-devel] [PATCH] libxl: do not rely on guest to respond when forcing pci device removal
# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1299584929 0
# Node ID 5084214b8983045d789a86c01e7a0fede46b5e58
# Parent 0e3211b5c4da98d170ed665c221bcb00e771fc56
libxl: do not rely on guest to respond when forcing pci device removal
This is consistent with the expected semantics of a forced device
removal and also avoids a delay when destroying an HVM domain which
either does not support hot unplug (does not respond to SCI) or has
crashed.
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
diff -r 0e3211b5c4da -r 5084214b8983 tools/libxl/libxl_pci.c
--- a/tools/libxl/libxl_pci.c Tue Mar 08 11:13:12 2011 +0000
+++ b/tools/libxl/libxl_pci.c Tue Mar 08 11:48:49 2011 +0000
@@ -866,7 +866,7 @@ static int do_pci_remove(libxl__gc *gc,
/* Remove all functions at once atomically by only signalling
* device-model for function 0 */
- if ( (pcidev->vdevfn & 0x7) == 0 ) {
+ if ( !force && (pcidev->vdevfn & 0x7) == 0 ) {
xs_write(ctx->xsh, XBT_NULL, path, "pci-rem",
strlen("pci-rem"));
if (libxl__wait_for_device_model(ctx, domid,
"pci-removed", NULL, NULL) < 0) {
LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Device Model
didn''t respond in time");
@@ -874,8 +874,7 @@ static int do_pci_remove(libxl__gc *gc,
* SCI, if it doesn''t respond in time then we may wish
to
* force the removal.
*/
- if ( !force )
- return ERROR_FAIL;
+ return ERROR_FAIL;
}
}
path = libxl__sprintf(gc,
"/local/domain/0/device-model/%d/state", domid);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Stefano Stabellini
2011-Mar-08 11:57 UTC
Re: [Xen-devel] [PATCH] libxl: do not rely on guest to respond when forcing pci device removal
On Tue, 8 Mar 2011, Ian Campbell wrote:> # HG changeset patch > # User Ian Campbell <ian.campbell@citrix.com> > # Date 1299584929 0 > # Node ID 5084214b8983045d789a86c01e7a0fede46b5e58 > # Parent 0e3211b5c4da98d170ed665c221bcb00e771fc56 > libxl: do not rely on guest to respond when forcing pci device removal > > This is consistent with the expected semantics of a forced device > removal and also avoids a delay when destroying an HVM domain which > either does not support hot unplug (does not respond to SCI) or has > crashed. > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> >Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Gianni Tedesco
2011-Mar-08 12:56 UTC
Re: [Xen-devel] [PATCH] libxl: do not rely on guest to respond when forcing pci device removal
On Tue, 2011-03-08 at 11:49 +0000, Ian Campbell wrote:> # HG changeset patch > # User Ian Campbell <ian.campbell@citrix.com> > # Date 1299584929 0 > # Node ID 5084214b8983045d789a86c01e7a0fede46b5e58 > # Parent 0e3211b5c4da98d170ed665c221bcb00e771fc56 > libxl: do not rely on guest to respond when forcing pci device removal > > This is consistent with the expected semantics of a forced device > removal and also avoids a delay when destroying an HVM domain which > either does not support hot unplug (does not respond to SCI) or has > crashed. > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > > diff -r 0e3211b5c4da -r 5084214b8983 tools/libxl/libxl_pci.c > --- a/tools/libxl/libxl_pci.c Tue Mar 08 11:13:12 2011 +0000 > +++ b/tools/libxl/libxl_pci.c Tue Mar 08 11:48:49 2011 +0000 > @@ -866,7 +866,7 @@ static int do_pci_remove(libxl__gc *gc, > > /* Remove all functions at once atomically by only signalling > * device-model for function 0 */ > - if ( (pcidev->vdevfn & 0x7) == 0 ) { > + if ( !force && (pcidev->vdevfn & 0x7) == 0 ) { > xs_write(ctx->xsh, XBT_NULL, path, "pci-rem", strlen("pci-rem"));Shouldn''t we maybe send the pci-rem when force removing to give the guest a chance to do something if it can.> if (libxl__wait_for_device_model(ctx, domid, "pci-removed", NULL, NULL) < 0) {&& !force ) { perhaps?> LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Device Model didn''t respond in time"); > @@ -874,8 +874,7 @@ static int do_pci_remove(libxl__gc *gc, > * SCI, if it doesn''t respond in time then we may wish to > * force the removal. > */ > - if ( !force ) > - return ERROR_FAIL; > + return ERROR_FAIL; > } > } > path = libxl__sprintf(gc, "/local/domain/0/device-model/%d/state", domid);The more I think about any of this code the worse the idea of the "xenstore + wait a bit" API seems, frankly, erroneous. Gianni _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Mar-08 14:17 UTC
Re: [Xen-devel] [PATCH] libxl: do not rely on guest to respond when forcing pci device removal
On Tue, 2011-03-08 at 12:56 +0000, Gianni Tedesco wrote:> On Tue, 2011-03-08 at 11:49 +0000, Ian Campbell wrote: > > # HG changeset patch > > # User Ian Campbell <ian.campbell@citrix.com> > > # Date 1299584929 0 > > # Node ID 5084214b8983045d789a86c01e7a0fede46b5e58 > > # Parent 0e3211b5c4da98d170ed665c221bcb00e771fc56 > > libxl: do not rely on guest to respond when forcing pci device removal > > > > This is consistent with the expected semantics of a forced device > > removal and also avoids a delay when destroying an HVM domain which > > either does not support hot unplug (does not respond to SCI) or has > > crashed. > > > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > > > > diff -r 0e3211b5c4da -r 5084214b8983 tools/libxl/libxl_pci.c > > --- a/tools/libxl/libxl_pci.c Tue Mar 08 11:13:12 2011 +0000 > > +++ b/tools/libxl/libxl_pci.c Tue Mar 08 11:48:49 2011 +0000 > > @@ -866,7 +866,7 @@ static int do_pci_remove(libxl__gc *gc, > > > > /* Remove all functions at once atomically by only signalling > > * device-model for function 0 */ > > - if ( (pcidev->vdevfn & 0x7) == 0 ) { > > + if ( !force && (pcidev->vdevfn & 0x7) == 0 ) { > > xs_write(ctx->xsh, XBT_NULL, path, "pci-rem", strlen("pci-rem")); > > Shouldn''t we maybe send the pci-rem when force removing to give the > guest a chance to do something if it can.My concern was just that if this wasn''t reacted to by qemu it might interfere with us sending other requests in the future (I don''t know and haven''t checked if that is the case).> > > if (libxl__wait_for_device_model(ctx, domid, "pci-removed", NULL, NULL) < 0) { > && !force ) { > > perhaps?Did you mean "!force && libxl__wait..." iow you need the !force to short-circuit the waiting which was the point of the patch. Ian.> > > LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Device Model didn''t respond in time"); > > @@ -874,8 +874,7 @@ static int do_pci_remove(libxl__gc *gc, > > * SCI, if it doesn''t respond in time then we may wish to > > * force the removal. > > */ > > - if ( !force ) > > - return ERROR_FAIL; > > + return ERROR_FAIL; > > } > > } > > path = libxl__sprintf(gc, "/local/domain/0/device-model/%d/state", domid); > > The more I think about any of this code the worse the idea of the > "xenstore + wait a bit" API seems, frankly, erroneous. > > Gianni >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2011-Mar-08 14:23 UTC
Re: [Xen-devel] [PATCH] libxl: do not rely on guest to respond when forcing pci device removal
On Tue, 8 Mar 2011, Ian Campbell wrote:> On Tue, 2011-03-08 at 12:56 +0000, Gianni Tedesco wrote: > > On Tue, 2011-03-08 at 11:49 +0000, Ian Campbell wrote: > > > # HG changeset patch > > > # User Ian Campbell <ian.campbell@citrix.com> > > > # Date 1299584929 0 > > > # Node ID 5084214b8983045d789a86c01e7a0fede46b5e58 > > > # Parent 0e3211b5c4da98d170ed665c221bcb00e771fc56 > > > libxl: do not rely on guest to respond when forcing pci device removal > > > > > > This is consistent with the expected semantics of a forced device > > > removal and also avoids a delay when destroying an HVM domain which > > > either does not support hot unplug (does not respond to SCI) or has > > > crashed. > > > > > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > > > > > > diff -r 0e3211b5c4da -r 5084214b8983 tools/libxl/libxl_pci.c > > > --- a/tools/libxl/libxl_pci.c Tue Mar 08 11:13:12 2011 +0000 > > > +++ b/tools/libxl/libxl_pci.c Tue Mar 08 11:48:49 2011 +0000 > > > @@ -866,7 +866,7 @@ static int do_pci_remove(libxl__gc *gc, > > > > > > /* Remove all functions at once atomically by only signalling > > > * device-model for function 0 */ > > > - if ( (pcidev->vdevfn & 0x7) == 0 ) { > > > + if ( !force && (pcidev->vdevfn & 0x7) == 0 ) { > > > xs_write(ctx->xsh, XBT_NULL, path, "pci-rem", strlen("pci-rem")); > > > > Shouldn''t we maybe send the pci-rem when force removing to give the > > guest a chance to do something if it can. > > My concern was just that if this wasn''t reacted to by qemu it might > interfere with us sending other requests in the future (I don''t know and > haven''t checked if that is the case).The guest OS should be able to shutdown properly no matter if it receives the hot-unplug event or not. And Xen should be able to clean up the resources in use no matter if the guest OS handles the hot-unplug event or not. Therefore avoiding the hot-unplug code path on shutdown is perfectly reasonable. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Mar-08 14:29 UTC
Re: [Xen-devel] [PATCH] libxl: do not rely on guest to respond when forcing pci device removal
On Tue, 2011-03-08 at 14:23 +0000, Stefano Stabellini wrote:> On Tue, 8 Mar 2011, Ian Campbell wrote: > > On Tue, 2011-03-08 at 12:56 +0000, Gianni Tedesco wrote: > > > On Tue, 2011-03-08 at 11:49 +0000, Ian Campbell wrote: > > > > # HG changeset patch > > > > # User Ian Campbell <ian.campbell@citrix.com> > > > > # Date 1299584929 0 > > > > # Node ID 5084214b8983045d789a86c01e7a0fede46b5e58 > > > > # Parent 0e3211b5c4da98d170ed665c221bcb00e771fc56 > > > > libxl: do not rely on guest to respond when forcing pci device removal > > > > > > > > This is consistent with the expected semantics of a forced device > > > > removal and also avoids a delay when destroying an HVM domain which > > > > either does not support hot unplug (does not respond to SCI) or has > > > > crashed. > > > > > > > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > > > > > > > > diff -r 0e3211b5c4da -r 5084214b8983 tools/libxl/libxl_pci.c > > > > --- a/tools/libxl/libxl_pci.c Tue Mar 08 11:13:12 2011 +0000 > > > > +++ b/tools/libxl/libxl_pci.c Tue Mar 08 11:48:49 2011 +0000 > > > > @@ -866,7 +866,7 @@ static int do_pci_remove(libxl__gc *gc, > > > > > > > > /* Remove all functions at once atomically by only signalling > > > > * device-model for function 0 */ > > > > - if ( (pcidev->vdevfn & 0x7) == 0 ) { > > > > + if ( !force && (pcidev->vdevfn & 0x7) == 0 ) { > > > > xs_write(ctx->xsh, XBT_NULL, path, "pci-rem", strlen("pci-rem")); > > > > > > Shouldn''t we maybe send the pci-rem when force removing to give the > > > guest a chance to do something if it can. > > > > My concern was just that if this wasn''t reacted to by qemu it might > > interfere with us sending other requests in the future (I don''t know and > > haven''t checked if that is the case). > > The guest OS should be able to shutdown properly no matter if it > receives the hot-unplug event or not.Or it may have crashed.> And Xen should be able to clean up the resources in use no matter if the > guest OS handles the hot-unplug event or not. > Therefore avoiding the hot-unplug code path on shutdown is perfectly > reasonable.Note that this change impacts the destroy path but not the shutdown path. Avoiding any need to do a graceful teardown is part of the point of the difference between destroy and shutdown. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Gianni Tedesco
2011-Mar-08 14:35 UTC
Re: [Xen-devel] [PATCH] libxl: do not rely on guest to respond when forcing pci device removal
On Tue, 2011-03-08 at 14:17 +0000, Ian Campbell wrote:> On Tue, 2011-03-08 at 12:56 +0000, Gianni Tedesco wrote: > > On Tue, 2011-03-08 at 11:49 +0000, Ian Campbell wrote: > > > # HG changeset patch > > > # User Ian Campbell <ian.campbell@citrix.com> > > > # Date 1299584929 0 > > > # Node ID 5084214b8983045d789a86c01e7a0fede46b5e58 > > > # Parent 0e3211b5c4da98d170ed665c221bcb00e771fc56 > > > libxl: do not rely on guest to respond when forcing pci device removal > > > > > > This is consistent with the expected semantics of a forced device > > > removal and also avoids a delay when destroying an HVM domain which > > > either does not support hot unplug (does not respond to SCI) or has > > > crashed. > > > > > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > > > > > > diff -r 0e3211b5c4da -r 5084214b8983 tools/libxl/libxl_pci.c > > > --- a/tools/libxl/libxl_pci.c Tue Mar 08 11:13:12 2011 +0000 > > > +++ b/tools/libxl/libxl_pci.c Tue Mar 08 11:48:49 2011 +0000 > > > @@ -866,7 +866,7 @@ static int do_pci_remove(libxl__gc *gc, > > > > > > /* Remove all functions at once atomically by only signalling > > > * device-model for function 0 */ > > > - if ( (pcidev->vdevfn & 0x7) == 0 ) { > > > + if ( !force && (pcidev->vdevfn & 0x7) == 0 ) { > > > xs_write(ctx->xsh, XBT_NULL, path, "pci-rem", strlen("pci-rem")); > > > > Shouldn''t we maybe send the pci-rem when force removing to give the > > guest a chance to do something if it can. > > My concern was just that if this wasn''t reacted to by qemu it might > interfere with us sending other requests in the future (I don''t know and > haven''t checked if that is the case).It''s possible but I think it''s unlikely. Generally the qemu hardware emulation works in discrete steps advancing the state machine. The xenstore watching bits work in pretty much the same way.> > > > > if (libxl__wait_for_device_model(ctx, domid, "pci-removed", NULL, NULL) < 0) { > > && !force ) { > > > > perhaps? > > Did you mean "!force && libxl__wait..." iow you need the !force to > short-circuit the waiting which was the point of the patch.No, I meant to do the wait first but just not whinge about it if it times out. But the other way could make sense too, I''ll get to that. I''m just saying, I think force should try and do things in the normal way and the only difference should be that when it completes, the device is always removed regardless of any guest non-cooperativeness. OTOH Stefano makes a good point that this code is also called on shutdown as well as if user requests forceful unplug so in the former case doing the hotplug sequence is pointless and in the latter case the user probably already tried a clean hotplug and it failed. Gianni _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Mar-08 14:41 UTC
Re: [Xen-devel] [PATCH] libxl: do not rely on guest to respond when forcing pci device removal
On Tue, 2011-03-08 at 14:35 +0000, Gianni Tedesco wrote:> On Tue, 2011-03-08 at 14:17 +0000, Ian Campbell wrote: > > On Tue, 2011-03-08 at 12:56 +0000, Gianni Tedesco wrote: > > > On Tue, 2011-03-08 at 11:49 +0000, Ian Campbell wrote: > > > > # HG changeset patch > > > > # User Ian Campbell <ian.campbell@citrix.com> > > > > # Date 1299584929 0 > > > > # Node ID 5084214b8983045d789a86c01e7a0fede46b5e58 > > > > # Parent 0e3211b5c4da98d170ed665c221bcb00e771fc56 > > > > libxl: do not rely on guest to respond when forcing pci device removal > > > > > > > > This is consistent with the expected semantics of a forced device > > > > removal and also avoids a delay when destroying an HVM domain which > > > > either does not support hot unplug (does not respond to SCI) or has > > > > crashed. > > > > > > > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > > > > > > > > diff -r 0e3211b5c4da -r 5084214b8983 tools/libxl/libxl_pci.c > > > > --- a/tools/libxl/libxl_pci.c Tue Mar 08 11:13:12 2011 +0000 > > > > +++ b/tools/libxl/libxl_pci.c Tue Mar 08 11:48:49 2011 +0000 > > > > @@ -866,7 +866,7 @@ static int do_pci_remove(libxl__gc *gc, > > > > > > > > /* Remove all functions at once atomically by only signalling > > > > * device-model for function 0 */ > > > > - if ( (pcidev->vdevfn & 0x7) == 0 ) { > > > > + if ( !force && (pcidev->vdevfn & 0x7) == 0 ) { > > > > xs_write(ctx->xsh, XBT_NULL, path, "pci-rem", strlen("pci-rem")); > > > > > > Shouldn''t we maybe send the pci-rem when force removing to give the > > > guest a chance to do something if it can. > > > > My concern was just that if this wasn''t reacted to by qemu it might > > interfere with us sending other requests in the future (I don''t know and > > haven''t checked if that is the case). > > It''s possible but I think it''s unlikely. Generally the qemu hardware > emulation works in discrete steps advancing the state machine. The > xenstore watching bits work in pretty much the same way. > > > > > > > > if (libxl__wait_for_device_model(ctx, domid, "pci-removed", NULL, NULL) < 0) { > > > && !force ) { > > > > > > perhaps? > > > > Did you mean "!force && libxl__wait..." iow you need the !force to > > short-circuit the waiting which was the point of the patch. > > No, I meant to do the wait first but just not whinge about it if it > times out. But the other way could make sense too, I''ll get to that.The wait and the timeout are the specific issue I am trying to address. When I type "xl destroy foo" I expect domain foo to get shot in the head right now, no questions and no hanging around. Similarly if I do "xl pci-detach --force...".> I''m just saying, I think force should try and do things in the normal > way and the only difference should be that when it completes, the device > is always removed regardless of any guest non-cooperativeness. > > OTOH Stefano makes a good point that this code is also called on > shutdown as well as if user requests forceful unplug so in the former > case doing the hotplug sequence is pointless and in the latter case the > user probably already tried a clean hotplug and it failed.In the shutdown case libxl_domain_destroy is called in response to the LIBXL_EVENT_DOMAIN_DEATH event. Nothing is alive to respond to any unplug request. Ian.> > Gianni >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Gianni Tedesco
2011-Mar-08 14:56 UTC
Re: [Xen-devel] [PATCH] libxl: do not rely on guest to respond when forcing pci device removal
On Tue, 2011-03-08 at 14:41 +0000, Ian Campbell wrote:> On Tue, 2011-03-08 at 14:35 +0000, Gianni Tedesco wrote: > > On Tue, 2011-03-08 at 14:17 +0000, Ian Campbell wrote: > > > On Tue, 2011-03-08 at 12:56 +0000, Gianni Tedesco wrote: > > > > On Tue, 2011-03-08 at 11:49 +0000, Ian Campbell wrote: > > > > > # HG changeset patch > > > > > # User Ian Campbell <ian.campbell@citrix.com> > > > > > # Date 1299584929 0 > > > > > # Node ID 5084214b8983045d789a86c01e7a0fede46b5e58 > > > > > # Parent 0e3211b5c4da98d170ed665c221bcb00e771fc56 > > > > > libxl: do not rely on guest to respond when forcing pci device removal > > > > > > > > > > This is consistent with the expected semantics of a forced device > > > > > removal and also avoids a delay when destroying an HVM domain which > > > > > either does not support hot unplug (does not respond to SCI) or has > > > > > crashed. > > > > > > > > > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > > > > > > > > > > diff -r 0e3211b5c4da -r 5084214b8983 tools/libxl/libxl_pci.c > > > > > --- a/tools/libxl/libxl_pci.c Tue Mar 08 11:13:12 2011 +0000 > > > > > +++ b/tools/libxl/libxl_pci.c Tue Mar 08 11:48:49 2011 +0000 > > > > > @@ -866,7 +866,7 @@ static int do_pci_remove(libxl__gc *gc, > > > > > > > > > > /* Remove all functions at once atomically by only signalling > > > > > * device-model for function 0 */ > > > > > - if ( (pcidev->vdevfn & 0x7) == 0 ) { > > > > > + if ( !force && (pcidev->vdevfn & 0x7) == 0 ) { > > > > > xs_write(ctx->xsh, XBT_NULL, path, "pci-rem", strlen("pci-rem")); > > > > > > > > Shouldn''t we maybe send the pci-rem when force removing to give the > > > > guest a chance to do something if it can. > > > > > > My concern was just that if this wasn''t reacted to by qemu it might > > > interfere with us sending other requests in the future (I don''t know and > > > haven''t checked if that is the case). > > > > It''s possible but I think it''s unlikely. Generally the qemu hardware > > emulation works in discrete steps advancing the state machine. The > > xenstore watching bits work in pretty much the same way. > > > > > > > > > > > if (libxl__wait_for_device_model(ctx, domid, "pci-removed", NULL, NULL) < 0) { > > > > && !force ) { > > > > > > > > perhaps? > > > > > > Did you mean "!force && libxl__wait..." iow you need the !force to > > > short-circuit the waiting which was the point of the patch. > > > > No, I meant to do the wait first but just not whinge about it if it > > times out. But the other way could make sense too, I''ll get to that. > > The wait and the timeout are the specific issue I am trying to address. > When I type "xl destroy foo" I expect domain foo to get shot in the head > right now, no questions and no hanging around. Similarly if I do "xl > pci-detach --force...".OK, fair enough. Honestly, I think my expectation for the detach case to be more as I stated, but I don''t feel strongly about it and it would just add complexity to have it that way. Gianni _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Mar-08 14:58 UTC
Re: [Xen-devel] [PATCH] libxl: do not rely on guest to respond when forcing pci device removal
On Tue, 2011-03-08 at 14:56 +0000, Gianni Tedesco wrote:> On Tue, 2011-03-08 at 14:41 +0000, Ian Campbell wrote: > > On Tue, 2011-03-08 at 14:35 +0000, Gianni Tedesco wrote: > > > On Tue, 2011-03-08 at 14:17 +0000, Ian Campbell wrote: > > > > On Tue, 2011-03-08 at 12:56 +0000, Gianni Tedesco wrote: > > > > > On Tue, 2011-03-08 at 11:49 +0000, Ian Campbell wrote: > > > > > > # HG changeset patch > > > > > > # User Ian Campbell <ian.campbell@citrix.com> > > > > > > # Date 1299584929 0 > > > > > > # Node ID 5084214b8983045d789a86c01e7a0fede46b5e58 > > > > > > # Parent 0e3211b5c4da98d170ed665c221bcb00e771fc56 > > > > > > libxl: do not rely on guest to respond when forcing pci device removal > > > > > > > > > > > > This is consistent with the expected semantics of a forced device > > > > > > removal and also avoids a delay when destroying an HVM domain which > > > > > > either does not support hot unplug (does not respond to SCI) or has > > > > > > crashed. > > > > > > > > > > > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > > > > > > > > > > > > diff -r 0e3211b5c4da -r 5084214b8983 tools/libxl/libxl_pci.c > > > > > > --- a/tools/libxl/libxl_pci.c Tue Mar 08 11:13:12 2011 +0000 > > > > > > +++ b/tools/libxl/libxl_pci.c Tue Mar 08 11:48:49 2011 +0000 > > > > > > @@ -866,7 +866,7 @@ static int do_pci_remove(libxl__gc *gc, > > > > > > > > > > > > /* Remove all functions at once atomically by only signalling > > > > > > * device-model for function 0 */ > > > > > > - if ( (pcidev->vdevfn & 0x7) == 0 ) { > > > > > > + if ( !force && (pcidev->vdevfn & 0x7) == 0 ) { > > > > > > xs_write(ctx->xsh, XBT_NULL, path, "pci-rem", strlen("pci-rem")); > > > > > > > > > > Shouldn''t we maybe send the pci-rem when force removing to give the > > > > > guest a chance to do something if it can. > > > > > > > > My concern was just that if this wasn''t reacted to by qemu it might > > > > interfere with us sending other requests in the future (I don''t know and > > > > haven''t checked if that is the case). > > > > > > It''s possible but I think it''s unlikely. Generally the qemu hardware > > > emulation works in discrete steps advancing the state machine. The > > > xenstore watching bits work in pretty much the same way. > > > > > > > > > > > > > > if (libxl__wait_for_device_model(ctx, domid, "pci-removed", NULL, NULL) < 0) { > > > > > && !force ) { > > > > > > > > > > perhaps? > > > > > > > > Did you mean "!force && libxl__wait..." iow you need the !force to > > > > short-circuit the waiting which was the point of the patch. > > > > > > No, I meant to do the wait first but just not whinge about it if it > > > times out. But the other way could make sense too, I''ll get to that. > > > > The wait and the timeout are the specific issue I am trying to address. > > When I type "xl destroy foo" I expect domain foo to get shot in the head > > right now, no questions and no hanging around. Similarly if I do "xl > > pci-detach --force...". > > OK, fair enough. Honestly, I think my expectation for the detach case to > be more as I stated, but I don''t feel strongly about it and it would > just add complexity to have it that way.That''s the non--force''d way of doing it and AFAICT it behaves pretty much as you describe. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Gianni Tedesco
2011-Mar-08 15:04 UTC
Re: [Xen-devel] [PATCH] libxl: do not rely on guest to respond when forcing pci device removal
On Tue, 2011-03-08 at 14:58 +0000, Ian Campbell wrote:> > OK, fair enough. Honestly, I think my expectation for the detach case to > > be more as I stated, but I don''t feel strongly about it and it would > > just add complexity to have it that way. > > That''s the non--force''d way of doing it and AFAICT it behaves pretty > much as you describe.Except that the non-force way will bail if the guest doesn''t respond and leave everything as it is so that the device can still function. How I described is try to play nice but before returning to caller - make sure everything is removed. Anyway, it''s a minor point. Gianni _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2011-Mar-10 18:22 UTC
Re: [Xen-devel] [PATCH] libxl: do not rely on guest to respond when forcing pci device removal
Ian Campbell writes ("[Xen-devel] [PATCH] libxl: do not rely on guest to
respond when forcing pci device removal"):> libxl: do not rely on guest to respond when forcing pci device removal
Applied, thanks.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel