Control: clone -1 -2
Control: reassign -2 qemu 1:2.7+dfsg-1
Control: retitle -2 qemu since 2.7 fails to hotplug USB devices from udev rules
Hi,
On Tue, 29 Nov 2016 08:14:03 +0100, Guido Günther wrote:
> On Mon, Nov 28, 2016 at 10:56:33PM +0000, David Gilmour wrote:
> > USB hotplug on this host was working normally as recently as a couple
> > of months ago; possibly a stretch update caused a regression. As of
> > now, I am unable to make USB hotplug to guests work.
> >
> > Thanks so much for looking into this.
>
> I won't have time to look into this, sorry. I suggest to:
>
> * set libvirt debugging to debug
> * check which monitor commands get issued to attach the device
>
> Create a new script run from the udev rule that
>
> * checks the necessary device nodes are actually there
> * uses the above monitor commands via "virsh qemu-monitor-command"
>
> if it still fails it's within qemu if not there's something broken in
> libvirt.
I did have some time and motivation to look into this, since we do this
for VM support in the bit-babbler (USB TRNG support) package too, and the
view that I have of it is that it's a libvirt bug, triggered by a change
in the qemu 2.7 release. So I'm cloning a copy of it there too, since
we may want to back out that change until this is fixed properly in
libvirt.
The crux of the problem is that although QEMU gives us the option to
hotplug a USB device by vendor/product ID, or by hostbus+hostport
(the physical usb port on the host) or by hostbus+hostaddr (the logical
USB device number which changes each time it is plugged in or gets
re-enumerated) - libvirt will *always* pass it to QEMU using the
hostaddr that it determines, even if in its config you passed that as
vendor/product ID. And libvirt gives us no option to use the hostport
at all.
This was somewhat workable (though suboptimal) in the case where you
might have multiple devices with the same product ID (a much better
solution there would be for libvirt to support vendor/product/serial
as a unique identifying tuple which never changes) - until this change
was made in QEMU 2.7:
commit e058fa2dd599ccc780d334558be9c1d155222b80
Author: Gerd Hoffmann <kra...@redhat.com>
Date: Fri Jun 3 11:12:55 2016 +0200
usb-host: add special case for bus+addr
This patch changes usb-host behavior in case we hostbus= and hostaddr=
properties are used to identify the usb device in question. Instead of
adding the device to the hotplug watchlist we try to open directly using
the given bus number and device address.
Putting a device specified by hostaddr to the hotplug watchlist isn't
a great idea as the address isn't a fixed property. It changes every
time the device is plugged in. So considering this case as "use the
device at bus:addr _now_" is more sane. Also usb-host will throw errors
in case it can't initialize the host device.
Note: For devices on the hotplug watchlist (hostport or vendorid or
productid specified) qemu continues to ignore errors and keeps
monitoring the usb bus to see if the device eventually shows up.
Signed-off-by: Gerd Hoffmann <kra...@redhat.com>
Message-id: 1464945175-28939-1-git-send-email-kra...@redhat.com
diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index 8b774f4..da59c29 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -81,6 +81,7 @@ struct USBHostDevice {
uint32_t iso_urb_frames;
uint32_t options;
uint32_t loglevel;
+ bool needs_autoscan;
/* state */
QTAILQ_ENTRY(USBHostDevice) next;
@@ -974,9 +975,32 @@ static void usb_host_exit_notifier(struct Notifier *n,
void *data)
}
}
+static libusb_device *usb_host_find_ref(int bus, int addr)
+{
+ libusb_device **devs = NULL;
+ libusb_device *ret = NULL;
+ int i, n;
+
+ if (usb_host_init() != 0) {
+ return NULL;
+ }
+ n = libusb_get_device_list(ctx, &devs);
+ for (i = 0; i < n; i++) {
+ if (libusb_get_bus_number(devs[i]) == bus &&
+ libusb_get_device_address(devs[i]) == addr) {
+ ret = libusb_ref_device(devs[i]);
+ break;
+ }
+ }
+ libusb_free_device_list(devs, 1);
+ return ret;
+}
+
static void usb_host_realize(USBDevice *udev, Error **errp)
{
USBHostDevice *s = USB_HOST_DEVICE(udev);
+ libusb_device *ldev;
+ int rc;
if (s->match.vendor_id > 0xffff) {
error_setg(errp, "vendorid out of range");
@@ -997,11 +1021,33 @@ static void usb_host_realize(USBDevice *udev, Error
**errp)
QTAILQ_INIT(&s->requests);
QTAILQ_INIT(&s->isorings);
+ if (s->match.addr && s->match.bus_num &&
+ !s->match.vendor_id &&
+ !s->match.product_id &&
+ !s->match.port) {
+ s->needs_autoscan = false;
+ ldev = usb_host_find_ref(s->match.bus_num,
+ s->match.addr);
+ if (!ldev) {
+ error_setg(errp, "failed to find host usb device %d:%d",
+ s->match.bus_num, s->match.addr);
+ return;
+ }
+ rc = usb_host_open(s, ldev);
+ libusb_unref_device(ldev);
+ if (rc < 0) {
+ error_setg(errp, "failed to open host usb device %d:%d",
+ s->match.bus_num, s->match.addr);
+ return;
+ }
+ } else {
+ s->needs_autoscan = true;
+ QTAILQ_INSERT_TAIL(&hostdevs, s, next);
+ usb_host_auto_check(NULL);
+ }
+
s->exit.notify = usb_host_exit_notifier;
qemu_add_exit_notifier(&s->exit);
-
- QTAILQ_INSERT_TAIL(&hostdevs, s, next);
- usb_host_auto_check(NULL);
}
static void usb_host_instance_init(Object *obj)
@@ -1019,7 +1065,9 @@ static void usb_host_handle_destroy(USBDevice *udev)
USBHostDevice *s = USB_HOST_DEVICE(udev);
qemu_remove_exit_notifier(&s->exit);
- QTAILQ_REMOVE(&hostdevs, s, next);
+ if (s->needs_autoscan) {
+ QTAILQ_REMOVE(&hostdevs, s, next);
+ }
usb_host_close(s);
}
So what happening here is that when `virsh attach-device` is called from
a udev rule, libudev has not yet notified libusb that the device has
been plugged in, so QEMU doesn't yet see it as present, and when libvirt
passed the logic address, QEMU looks that up, and errors out because it
doesn't know about it yet. QED.
The problem is exacerbated by Cleverness ruling out all of the easy
workarounds that I can currently see. libvirt doesn't pass on the
vendor or product ID even if we specify it along with an <address>
in its config snippet (which would bypass the new check in QEMU and
have it added to its hotplug watchlist again). And systemd doesn't
let us background and delay the call to virsh until after it has
delivered the event via libudev ...
Ideally what we want here is for libvirt to better support the full
functionality that QEMU provides - allowing us to actually pass the
hostport and/or vendor and product ID when desired, since there are
good reasons to want to do that aside from this bug. And even better
if (both) allowed specifying devices by vendor/product/serial to
allow uniquely designating one (or more) of multiple similar devices
without needing external hacks to first map that to a logical device
number or physical port address.
In the meantime, it would be nice if we can relax that test in QEMU
to keep things working until a Real Fix is in. I can certainly see
the sense in being cautious about trusting a transient logical address
there - but if adding (and removing it again) already needs to be
managed by external logic to deal with the current less-than-ideal
state of things, then it's not unreasonable for us to be able to
say to it "trust us, you can't see it yet, but we've got this under
control, just wait until you get your notification event too").
Please do keep me in the CC for any discussion of this, I am very
interested in us ultimately Getting This Right as a supported thing.
I'd planned to open some discussion on improving this a while ago
(http://bitbabbler.org/blog.html#vm_hotplug), but this bug has now
made it a more urgent thing to deal with than it previously was.
Cheers,
Ron
