Re: [Xen-devel] [PATCH] libxl: do not rely on guest to respond when forcing pci device removal

[Gianni Tedesco <gianni.tedesco@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > > # 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@xxxxxxxxxx>
> > > 
> > > 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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel