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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

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) {
>         [stuff]
>      } else if (type != LIBXL_DOMAIN_TYPE_PV)
>          abort();
>      {
>
> 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)


Thanks.
-boris

>
> Thanks
> Ian.
>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
>
> 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, pcidev->func);
>           FILE *f = fopen(sysfs_path, "r");