[Qemu-devel] [PATCH 0/4] usb: hotplug support for usb-bot and usb-uas

[Qemu-devel] [PATCH 0/4] usb: hotplug support for usb-bot and usb-uas

[Gerd Hoffmann]

  Hi,

Composite device hotplug is a long-standing issue in qemu.

PCI multifunction hotplug has been solved recently in a pci-specific
way, by simply not showing hot-plugged functions to the guest until
function #0 is plugged.

So I figured we maybe should check whenever we can apply a simliar trick
to USB composite devices.  As the usb subsystem already can hide usb
devices from the guest (used for example with usb-host, in case the
physical usb device is not plugged in the host machine) this turned out
to be surprisingly simple.

So, here we go.  Basic idea is the "attached" state becomes settable via
monitor.  Now you can create a usb-bot (or usb-uas) device, then attach
the scsi device(s) you want, and when you are done you flip attached to
true to make the composed device visible to the guest.

pleae review & comment,
  Gerd

Gerd Hoffmann (4):
  usb: make USBDevice->attached bool
  usb: add attached property
  usb-bot: hotplug support
  usb-uas: hotplug support

 hw/usb/bus.c         | 50 ++++++++++++++++++++++++++++++++++++++++++++++----
 hw/usb/dev-storage.c |  8 ++++++--
 hw/usb/dev-uas.c     |  5 +++++
 include/hw/usb.h     |  3 ++-
 4 files changed, 59 insertions(+), 7 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/4] usb: make USBDevice->attached bool

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/bus.c     | 8 ++++----
 include/hw/usb.h | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index 1bbe930..dd28041 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -52,9 +52,9 @@ static int usb_device_post_load(void *opaque, int version_id)
     USBDevice *dev = opaque;
 
     if (dev->state == USB_STATE_NOTATTACHED) {
-        dev->attached = 0;
+        dev->attached = false;
     } else {
-        dev->attached = 1;
+        dev->attached = true;
     }
     if (dev->setup_index < 0 ||
         dev->setup_len < 0 ||
@@ -530,7 +530,7 @@ void usb_device_attach(USBDevice *dev, Error **errp)
         return;
     }
 
-    dev->attached++;
+    dev->attached = true;
     usb_attach(port);
 }
 
@@ -544,7 +544,7 @@ int usb_device_detach(USBDevice *dev)
     trace_usb_port_detach(bus->busnr, port->path);
 
     usb_detach(port);
-    dev->attached--;
+    dev->attached = false;
     return 0;
 }
 
diff --git a/include/hw/usb.h b/include/hw/usb.h
index c8b6e7b..f8432f9 100644
--- a/include/hw/usb.h
+++ b/include/hw/usb.h
@@ -234,7 +234,7 @@ struct USBDevice {
     uint8_t addr;
     char product_desc[32];
     int auto_attach;
-    int attached;
+    bool attached;
 
     int32_t state;
     uint8_t setup_buf[8];
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/4] usb: make USBDevice->attached bool

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH 2/4] usb: add attached property

[Gerd Hoffmann]

USB devices in attached state are visible to the guest.  This patch adds
a QOM property for this.  Write access is opt-in per device.  Some
devices manage attached state automatically (usb-host, usb-serial), so
we can't enable write access universally but have to do it on a case by
case base.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/bus.c     | 42 ++++++++++++++++++++++++++++++++++++++++++
 include/hw/usb.h |  1 +
 2 files changed, 43 insertions(+)

diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index dd28041..17e0479 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -733,6 +733,47 @@ USBDevice *usbdevice_create(const char *cmdline)
     return dev;
 }
 
+static bool usb_get_attached(Object *obj, Error **errp)
+{
+    USBDevice *dev = USB_DEVICE(obj);
+
+    return dev->attached;
+}
+
+static void usb_set_attached(Object *obj, bool value, Error **errp)
+{
+    USBDevice *dev = USB_DEVICE(obj);
+    Error *err = NULL;
+
+    if (dev->attached == value)
+        return;
+
+    if (value) {
+        usb_device_attach(dev, &err);
+        if (err) {
+            error_propagate(errp, err);
+        }
+    } else {
+        usb_device_detach(dev);
+    }
+}
+
+static void usb_device_instance_init(Object *obj)
+{
+    USBDevice *dev = USB_DEVICE(obj);
+    USBDeviceClass *klass = USB_DEVICE_GET_CLASS(dev);
+
+    if (klass->attached_settable) {
+        object_property_add_bool(obj, "attached",
+                                 usb_get_attached, usb_set_attached,
+                                 NULL);
+    } else {
+        object_property_add_bool(obj, "attached",
+                                 usb_get_attached, NULL,
+                                 NULL);
+    }
+}
+
 static void usb_device_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *k = DEVICE_CLASS(klass);
@@ -746,6 +787,7 @@ static const TypeInfo usb_device_type_info = {
     .name = TYPE_USB_DEVICE,
     .parent = TYPE_DEVICE,
     .instance_size = sizeof(USBDevice),
+    .instance_init = usb_device_instance_init,
     .abstract = true,
     .class_size = sizeof(USBDeviceClass),
     .class_init = usb_device_class_init,
diff --git a/include/hw/usb.h b/include/hw/usb.h
index f8432f9..6eaa19c 100644
--- a/include/hw/usb.h
+++ b/include/hw/usb.h
@@ -346,6 +346,7 @@ typedef struct USBDeviceClass {
 
     const char *product_desc;
     const USBDesc *usb_desc;
+    bool attached_settable;
 } USBDeviceClass;
 
 typedef struct USBPortOps {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/4] usb: add attached property

[Markus Armbruster]

Suggest to say

    usb: Add QOM property "attached"

The quotes make more obvious that "attached" is a property name, not an
adjective tacked to property.

Gerd Hoffmann <kraxel@redhat.com> writes:

> USB devices in attached state are visible to the guest.

If I read the code correctly:

* ->attached is true between usb_device_attach() and usb_device_detach()

* Attach and detach is automatic on realize and unrealize, but a device
  can choose to suppress attach on realize, by unsetting ->auto_attach,
  and manage attach/detach itself.

* A detached device is wired up normally in QOM, but the USB core treats
  it as not plugged into its USB port.

Correct?

>                                                          This patch adds
> a QOM property for this.  Write access is opt-in per device.  Some
> devices manage attached state automatically (usb-host, usb-serial), so

Full list of devices unsetting ->auto_attach: dev-serial.c dev-storage.c
host-libusb.c redirect.c.

dev-storage.c does it only for encrypted media without a key, but that
should go away with Dan's "Support LUKS encryption in block devices", so
I guess neglecting to mention it here is okay.

Suggest to add usb-redir to your list.

> we can't enable write access universally but have to do it on a case by
> case base.

Suggest to add: So far, no device opts in.

>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Patch looks good.

Re: [Qemu-devel] [PATCH 2/4] usb: add attached property

[Gerd Hoffmann]

  Hi,

> > USB devices in attached state are visible to the guest.
> 
> If I read the code correctly:
> 
> * ->attached is true between usb_device_attach() and usb_device_detach()
> 
> * Attach and detach is automatic on realize and unrealize, but a device
>   can choose to suppress attach on realize, by unsetting ->auto_attach,
>   and manage attach/detach itself.
> 
> * A detached device is wired up normally in QOM, but the USB core treats
>   it as not plugged into its USB port.
> 
> Correct?

Yes.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 2/4] usb: add attached property

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

>   Hi,
>
>> > USB devices in attached state are visible to the guest.
>> 
>> If I read the code correctly:
>> 
>> * ->attached is true between usb_device_attach() and usb_device_detach()
>> 
>> * Attach and detach is automatic on realize and unrealize, but a device
>>   can choose to suppress attach on realize, by unsetting ->auto_attach,
>>   and manage attach/detach itself.
>> 
>> * A detached device is wired up normally in QOM, but the USB core treats
>>   it as not plugged into its USB port.
>> 
>> Correct?
>
> Yes.

Preferrably with the commit message improved along the line of my
suggestions:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH 3/4] usb-bot: hotplug support

[Gerd Hoffmann]

This patch marks usb-bot as hot-pluggable device, makes attached
property settable and turns off auto-attach in case the device
was hotplugged.

Hot-plugging a usb-bot device with one or more scsi devices can be
done this way now:

  (1) device-add usb-bot,id=foo
  (2) device-add scsi-{hd,cd},bus=foo.0,lun=0
  (2b) optionally add more devices (luns 0 ... 15).
  (3) qom-set foo.attached = true

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/dev-storage.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 597d8fd..275e0ed 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -665,9 +665,14 @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp)
 static void usb_msd_realize_bot(USBDevice *dev, Error **errp)
 {
     MSDState *s = USB_STORAGE_DEV(dev);
+    DeviceState *d = DEVICE(dev);
 
     usb_desc_create_serial(dev);
     usb_desc_init(dev);
+    if (d->hotplugged) {
+        s->dev.auto_attach = 0;
+    }
+
     scsi_bus_new(&s->bus, sizeof(s->bus), DEVICE(dev),
                  &usb_msd_scsi_info_bot, NULL);
     usb_msd_handle_reset(dev);
@@ -839,10 +844,9 @@ static void usb_msd_instance_init(Object *obj)
 static void usb_msd_class_initfn_bot(ObjectClass *klass, void *data)
 {
     USBDeviceClass *uc = USB_DEVICE_CLASS(klass);
-    DeviceClass *dc = DEVICE_CLASS(klass);
 
     uc->realize = usb_msd_realize_bot;
-    dc->hotpluggable = false;
+    uc->attached_settable = true;
 }
 
 static const TypeInfo msd_info = {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 3/4] usb-bot: hotplug support

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

> This patch marks usb-bot as hot-pluggable device, makes attached
> property settable and turns off auto-attach in case the device
> was hotplugged.
>
> Hot-plugging a usb-bot device with one or more scsi devices can be
> done this way now:
>
>   (1) device-add usb-bot,id=foo
>   (2) device-add scsi-{hd,cd},bus=foo.0,lun=0
>   (2b) optionally add more devices (luns 0 ... 15).
>   (3) qom-set foo.attached = true

This isn't exactly pretty, but it beats no hot plug.

A general solution for hot plugging composite devices could perhaps be
prettier, but I'm not aware of any recent work in the area.  Andreas,
Paolo?

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Assuming we want this because we can't have a general solution now:

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH 3/4] usb-bot: hotplug support

[Andreas Färber]

Am 02.02.2016 um 14:36 schrieb Markus Armbruster:
> Gerd Hoffmann <kraxel@redhat.com> writes:
> 
>> This patch marks usb-bot as hot-pluggable device, makes attached
>> property settable and turns off auto-attach in case the device
>> was hotplugged.
>>
>> Hot-plugging a usb-bot device with one or more scsi devices can be
>> done this way now:
>>
>>   (1) device-add usb-bot,id=foo
>>   (2) device-add scsi-{hd,cd},bus=foo.0,lun=0
>>   (2b) optionally add more devices (luns 0 ... 15).
>>   (3) qom-set foo.attached = true
> 
> This isn't exactly pretty, but it beats no hot plug.
> 
> A general solution for hot plugging composite devices could perhaps be
> prettier, but I'm not aware of any recent work in the area.  Andreas,
> Paolo?

Not aware, no. Essentially we'd need a DeviceClass::dont_realize flag,
right? Then foo.attached=true could become foo.realized=true. Question
is then whether the bus would be attachable prior to realization - to be
tested.

Haven't read the full series yet, so puzzled what kind of bot this is
supposed to be. ;)

Cheers,
Andreas

>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> 
> Assuming we want this because we can't have a general solution now:
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 3/4] usb-bot: hotplug support

[Gerd Hoffmann]

On Di, 2016-02-02 at 19:16 +0100, Andreas Färber wrote:
> Am 02.02.2016 um 14:36 schrieb Markus Armbruster:
> > Gerd Hoffmann <kraxel@redhat.com> writes:
> > 
> >> This patch marks usb-bot as hot-pluggable device, makes attached
> >> property settable and turns off auto-attach in case the device
> >> was hotplugged.
> >>
> >> Hot-plugging a usb-bot device with one or more scsi devices can be
> >> done this way now:
> >>
> >>   (1) device-add usb-bot,id=foo
> >>   (2) device-add scsi-{hd,cd},bus=foo.0,lun=0
> >>   (2b) optionally add more devices (luns 0 ... 15).
> >>   (3) qom-set foo.attached = true
> > 
> > This isn't exactly pretty, but it beats no hot plug.
> > 
> > A general solution for hot plugging composite devices could perhaps be
> > prettier, but I'm not aware of any recent work in the area.  Andreas,
> > Paolo?
> 
> Not aware, no. Essentially we'd need a DeviceClass::dont_realize flag,
> right?

Naa, not that simple I think.  We would need some way to create a group
of devices, then plug them all at once.

Case one is multi-function pci.

Case two are usb storage devices (bot + uas aka bulk-only-transport and
usb-attached-scsi).

I'm not aware of other cases.

Multifunction pci has been handled recently with a pci-specific hack:
pci functions are not visible to the guest until function 0 is plugged.
So you just have to plug them in the correct order (function 0 last) to
get things going.  Works because the common pci slot implicitly groups
devices.

So this is handles the usb storage devices with a usb specific hack:
usb devices can exist without being visible to the guest
(attached=false).  We can use that to create the device group (usb
storage adapter and the scsi device(s) connected to it) without the
guest seeing a half-composed device, when done we go flip the
visibility.

cheers,
  Gerd

[Qemu-devel] [PATCH 4/4] usb-uas: hotplug support

[Gerd Hoffmann]

Make attached property settable and turns off auto-attach in case the
device was hotplugged.  Hotplugging works simliar to usb-bot now.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/dev-uas.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
index 38b26c5..4fd0bca 100644
--- a/hw/usb/dev-uas.c
+++ b/hw/usb/dev-uas.c
@@ -899,9 +899,13 @@ static void usb_uas_handle_destroy(USBDevice *dev)
 static void usb_uas_realize(USBDevice *dev, Error **errp)
 {
     UASDevice *uas = USB_UAS(dev);
+    DeviceState *d = DEVICE(dev);
 
     usb_desc_create_serial(dev);
     usb_desc_init(dev);
+    if (d->hotplugged) {
+        uas->dev.auto_attach = 0;
+    }
 
     QTAILQ_INIT(&uas->results);
     QTAILQ_INIT(&uas->requests);
@@ -939,6 +943,7 @@ static void usb_uas_class_initfn(ObjectClass *klass, void *data)
     uc->handle_control = usb_uas_handle_control;
     uc->handle_data    = usb_uas_handle_data;
     uc->handle_destroy = usb_uas_handle_destroy;
+    uc->attached_settable = true;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
     dc->fw_name = "storage";
     dc->vmsd = &vmstate_usb_uas;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 4/4] usb-uas: hotplug support

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

> Make attached property settable and turns off auto-attach in case the
> device was hotplugged.  Hotplugging works simliar to usb-bot now.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Assuming we want this because we can't have a general solution now:

Reviewed-by: Markus Armbruster <armbru@redhat.com>