Re: [PATCH for-4.5] reset PCI devices on force removal even when QEMU returns error

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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;
> > > >          }
> > > 
> > > 
> >