[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 1/2] libxl: Wait until QEMU removed the device before tearing it down

On 11/14/2014 09:41 AM, Ian Jackson wrote:
Boris Ostrovsky writes ("[PATCH 1/2] libxl: Wait until QEMU removed the device 
before tearing it down"):
When a device is hot-unplugged libxl sends QEMU a "device-del" message
(via QMP). This call returns after QEMU has initiated device removal by
sending an interrupt to the guest. At some point later QEMU is expected
to clean up after the device (such as unbind/unmap MSIs), which will
occur when the guest signals that the device has been ejected.
I haven't repro'd this bug, but after looking at your patch, and
staring at the code, I think you may have misdiagnosed the problem.

I found this:

     if (type == LIBXL_DOMAIN_TYPE_HVM) {
     } else if (type != LIBXL_DOMAIN_TYPE_PV)

This is, of course, very bizarre.  For a moment I put it down to
coding style, but the subsequent block is the PV pci unplug path.  It
would appear therefore that HVM PCI unplug executes first the HVM
code, and then immediately afterwards the PV code.

This strangeness was introduced in abfb006f
  "tools/libxl: explicitly grant access to needed I/O-memory ranges"
which seems to contain an entirely wrong hunk.

Can you try the patch below and see if it helps your problem ?

It does help with the first half of the problem --- we don't do the unnecessary unmap anymore in what now becomes PV-only 'else if (type != LIBXL_DOMAIN_TYPE_PV)' clause.

But it doesn't help with the second part (the one that shows Linux WARNING), because we still may try to reset the device before guest kernel is done unloading the driver. Reproducing that problem is probably dependent on the guest/driver: in this particular case the igb driver does an IN instruction while unloading the driver and if that instruction fails (which may happen if the device is gone from the POV of toolstack) then it triggers the warning.

AFAICT your other patch probably isn't needed in the light of all
this.  If it is, then I have misunderstood (and perhaps the commit
message could be more explicit about the bug that this is allegedly
fixing - calling a patch `Simplify ....' makes it sound like the kind
of cleanup we don't want to take during the freeze).

And I believe we still need part of the second patch --- the one that removes call to xc_domain_irq_permission() for PV guests (after your patch is applied): this call will fail because xc_physdev_unmap_pirq() above it will cause hypervisor to do unmap_domain_pirq()->irq_deny_access()

So I think we need:
1. Your patch
2. My first patch
3. Part of my second patch as described above. (and make commit message/subject more understandable)



Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 9f40100..bec25a9 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -1255,9 +1255,9 @@ static int do_pci_remove(libxl__gc *gc, uint32_t domid,
              rc = ERROR_FAIL;
              goto out_fail;
-    } else if (type != LIBXL_DOMAIN_TYPE_PV)
-        abort();
-    {
+    } else {
+        assert(type == LIBXL_DOMAIN_TYPE_PV);
          char *sysfs_path = libxl__sprintf(gc, 
SYSFS_PCI_DEV"/"PCI_BDF"/resource", pcidev->domain,
                                           pcidev->bus, pcidev->dev, 
          FILE *f = fopen(sysfs_path, "r");

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.