qemu: Replace usb-storage with usb-bot

usb-storage is a compound device that automatically creates a USB mass
storage device and a SCSI device as its backend. Unfortunately it lacks
some configuration options that are usually present with a SCSI device,
and cannot represent CD-ROM in particular.

Replace usb-storage with usb-bot, which can be combined with a manually
created SCSI device. libvirt will configure the SCSI device in a way
identical with how QEMU does for usb-storage except that now it respects
a configuration option to represent CD-ROM.

Resolves: https://gitlab.com/libvirt/libvirt/-/issues/368
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>

src/qemu/qemu_alias.c

@@ -268,8 +268,23 @@ qemuAssignDeviceDiskAlias(virDomainDef *def,
             break;
 
         case VIR_DOMAIN_DISK_BUS_USB:
-            diskPriv->qomName = g_strdup_printf("/machine/peripheral/%s/%s.0/legacy[0]",
-                                                disk->info.alias, disk->info.alias);
+            switch (disk->model) {
+            case VIR_DOMAIN_DISK_MODEL_USB_STORAGE:
+                diskPriv->qomName = g_strdup_printf("/machine/peripheral/%s/%s.0/legacy[0]",
+                                                    disk->info.alias, disk->info.alias);
+                break;
+
+            case VIR_DOMAIN_DISK_MODEL_USB_BOT:
+                diskPriv->qomName = g_strdup_printf("%s-device", disk->info.alias);
+                break;
+
+            case VIR_DOMAIN_DISK_MODEL_DEFAULT:
+            case VIR_DOMAIN_DISK_MODEL_VIRTIO:
+            case VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL:
+            case VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL:
+            case VIR_DOMAIN_DISK_MODEL_LAST:
+                break;
+            }
             break;
 
         case VIR_DOMAIN_DISK_BUS_XEN:

src/qemu/qemu_capabilities.c

@@ -6482,7 +6482,8 @@ virQEMUCapsFillDomainDeviceDiskCaps(virQEMUCaps *qemuCaps,
                              VIR_DOMAIN_DISK_BUS_VIRTIO,
                              /* VIR_DOMAIN_DISK_BUS_SD */);
 
-    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_STORAGE))
+    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_STORAGE) ||
+        virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_BOT))
         VIR_DOMAIN_CAPS_ENUM_SET(disk->bus, VIR_DOMAIN_DISK_BUS_USB);
 
     if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_AHCI))

src/qemu/qemu_command.c

@@ -1626,6 +1626,33 @@ qemuBuildIothreadMappingProps(GSList *iothreads)
     return g_steal_pointer(&ret);
 }
 
+int
+qemuBuildDiskBusProps(const virDomainDef *def,
+                      const virDomainDiskDef *disk,
+                      virJSONValue **propsRet)
+{
+    g_autoptr(virJSONValue) props = NULL;
+
+    *propsRet = NULL;
+
+    if (disk->bus != VIR_DOMAIN_DISK_BUS_USB ||
+        disk->model != VIR_DOMAIN_DISK_MODEL_USB_BOT)
+        return 0;
+
+    if (virJSONValueObjectAdd(&props,
+                              "s:driver", "usb-bot",
+                              "s:id", disk->info.alias,
+                              "S:serial", disk->serial,
+                              NULL) < 0)
+        return -1;
+
+    if (qemuBuildDeviceAddressProps(props, def, &disk->info) < 0)
+        return -1;
+
+    *propsRet = g_steal_pointer(&props);
+
+    return 0;
+}
 
 virJSONValue *
 qemuBuildDiskDeviceProps(const virDomainDef *def,
@@ -1652,6 +1679,18 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
     const char *rpolicy = NULL;
     const char *model = NULL;
     const char *product = NULL;
+    const char *alias = disk->info.alias;
+    g_autofree char *usbdiskalias = NULL;
+    const virDomainDeviceInfo *deviceinfo = &disk->info;
+    virDomainDeviceInfo usbSCSIinfo = {
+        .type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE,
+        .addr.drive = { .diskbus = VIR_DOMAIN_DISK_BUS_USB },
+        .effectiveBootIndex = deviceinfo->effectiveBootIndex,
+        .alias = deviceinfo->alias,
+    };
+
+    if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE)
+        disk->info.addr.drive.diskbus = disk->bus;
 
     switch (disk->bus) {
     case VIR_DOMAIN_DISK_BUS_IDE:
@@ -1735,13 +1774,35 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
         break;
 
     case VIR_DOMAIN_DISK_BUS_USB:
-        driver = "usb-storage";
+        switch (disk->model) {
+        case VIR_DOMAIN_DISK_MODEL_USB_STORAGE:
+            driver = "usb-storage";
 
-        if (disk->removable == VIR_TRISTATE_SWITCH_ABSENT)
-            removable = VIR_TRISTATE_SWITCH_OFF;
-        else
-            removable = disk->removable;
+            if (disk->removable == VIR_TRISTATE_SWITCH_ABSENT)
+                removable = VIR_TRISTATE_SWITCH_OFF;
+            else
+                removable = disk->removable;
+            break;
 
+        case VIR_DOMAIN_DISK_MODEL_USB_BOT:
+            if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
+                driver = "scsi-cd";
+            } else {
+                driver = "scsi-hd";
+                removable = disk->removable;
+            }
+
+            deviceinfo = &usbSCSIinfo;
+            alias = usbdiskalias = g_strdup_printf("%s-device", disk->info.alias);
+            break;
+
+        case VIR_DOMAIN_DISK_MODEL_DEFAULT:
+        case VIR_DOMAIN_DISK_MODEL_VIRTIO:
+        case VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL:
+        case VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL:
+        case VIR_DOMAIN_DISK_MODEL_LAST:
+            break;
+        }
         break;
 
     case VIR_DOMAIN_DISK_BUS_FDC:
@@ -1771,10 +1832,7 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
             return NULL;
     }
 
-    if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE)
-        disk->info.addr.drive.diskbus = disk->bus;
-
-    if (qemuBuildDeviceAddressProps(props, def, &disk->info) < 0)
+    if (qemuBuildDeviceAddressProps(props, def, deviceinfo) < 0)
         return NULL;
 
     if (disk->src->shared)
@@ -1836,7 +1894,7 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
                               "T:share-rw", shareRW,
                               "S:drive", drive,
                               "S:chardev", chardev,
-                              "s:id", disk->info.alias,
+                              "s:id", alias,
                               "p:bootindex", bootindex,
                               "S:loadparm", bootLoadparm,
                               "p:logical_block_size", logical_block_size,
@@ -2201,6 +2259,7 @@ qemuBuildDiskCommandLine(virCommand *cmd,
                          virQEMUCaps *qemuCaps)
 {
     g_autoptr(virJSONValue) devprops = NULL;
+    g_autoptr(virJSONValue) busprops = NULL;
 
     if (qemuBuildDiskSourceCommandLine(cmd, disk, qemuCaps) < 0)
         return -1;
@@ -2216,6 +2275,13 @@ qemuBuildDiskCommandLine(virCommand *cmd,
     if (qemuCommandAddExtDevice(cmd, &disk->info, def, qemuCaps) < 0)
         return -1;
 
+    if (qemuBuildDiskBusProps(def, disk, &busprops) < 0)
+        return -1;
+
+    if (busprops &&
+        qemuBuildDeviceCommandlineFromJSON(cmd, busprops, def, qemuCaps) < 0)
+        return -1;
+
     if (!(devprops = qemuBuildDiskDeviceProps(def, disk, qemuCaps)))
         return -1;
 

src/qemu/qemu_command.h

@@ -129,6 +129,11 @@ qemuBuildThrottleFiltersAttachPrepareBlockdev(virDomainDiskDef *disk);
 qemuBlockThrottleFiltersData *
 qemuBuildThrottleFiltersDetachPrepareBlockdev(virDomainDiskDef *disk);
 
+int
+qemuBuildDiskBusProps(const virDomainDef *def,
+                      const virDomainDiskDef *disk,
+                      virJSONValue **propsRet);
+
 virJSONValue *
 qemuBuildDiskDeviceProps(const virDomainDef *def,
                          virDomainDiskDef *disk,

src/qemu/qemu_hotplug.c

@@ -703,8 +703,10 @@ qemuDomainAttachDiskGeneric(virDomainObj *vm,
     g_autoptr(qemuBlockStorageSourceChainData) data = NULL;
     g_autoptr(qemuBlockThrottleFiltersData) filterData = NULL;
     qemuDomainObjPrivate *priv = vm->privateData;
+    g_autoptr(virJSONValue) busprops = NULL;
     g_autoptr(virJSONValue) devprops = NULL;
     bool extensionDeviceAttached = false;
+    bool busAdded = false;
     int rc;
     g_autoptr(qemuSnapshotDiskContext) transientDiskSnapshotCtxt = NULL;
     bool origReadonly = disk->src->readonly;
@@ -774,6 +776,9 @@ qemuDomainAttachDiskGeneric(virDomainObj *vm,
         }
     }
 
+    if (qemuBuildDiskBusProps(vm->def, disk, &busprops) < 0)
+        goto rollback;
+
     if (!(devprops = qemuBuildDiskDeviceProps(vm->def, disk, priv->qemuCaps)))
         goto rollback;
 
@@ -783,6 +788,10 @@ qemuDomainAttachDiskGeneric(virDomainObj *vm,
     if ((rc = qemuDomainAttachExtensionDevice(priv->mon, &disk->info)) == 0)
         extensionDeviceAttached = true;
 
+    if (rc == 0 && busprops &&
+        (rc = qemuMonitorAddDeviceProps(priv->mon, &busprops)) == 0)
+        busAdded = true;
+
     if (rc == 0)
         rc = qemuMonitorAddDeviceProps(priv->mon, &devprops);
 
@@ -811,6 +820,11 @@ qemuDomainAttachDiskGeneric(virDomainObj *vm,
         }
     }
 
+    if (rc == 0 &&
+        disk->bus == VIR_DOMAIN_DISK_BUS_USB &&
+        disk->model == VIR_DOMAIN_DISK_MODEL_USB_BOT)
+        rc = qemuMonitorSetUSBDiskAttached(priv->mon, disk->info.alias);
+
     qemuDomainObjExitMonitor(vm);
 
     if (rc < 0)
@@ -822,6 +836,9 @@ qemuDomainAttachDiskGeneric(virDomainObj *vm,
     if (qemuDomainObjEnterMonitorAsync(vm, asyncJob) < 0)
         return -1;
 
+    if (busAdded)
+        ignore_value(qemuMonitorDelDevice(priv->mon, disk->info.alias));
+
     if (extensionDeviceAttached)
         ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &disk->info));
 

src/qemu/qemu_validate.c

@@ -3183,16 +3183,40 @@ qemuValidateDomainDeviceDefDiskFrontend(const virDomainDiskDef *disk,
         break;
 
     case VIR_DOMAIN_DISK_BUS_USB:
-        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_STORAGE)) {
-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                           _("This QEMU doesn't support '-device usb-storage'"));
+        switch (disk->model) {
+        case VIR_DOMAIN_DISK_MODEL_DEFAULT:
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("USB disk model was not selected. This QEMU doesn't support 'usb-storage' or 'usb-bot'."));
             return -1;
-        }
 
-        if (disk->model == VIR_DOMAIN_DISK_MODEL_USB_STORAGE &&
-            virStorageSourceIsEmpty(disk->src)) {
+        case VIR_DOMAIN_DISK_MODEL_USB_STORAGE:
+            if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_STORAGE)) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                               _("This QEMU doesn't support '-device usb-storage'"));
+                return -1;
+            }
+
+            if (virStorageSourceIsEmpty(disk->src)) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                               _("'usb' disk must not be empty"));
+                return -1;
+            }
+            break;
+
+        case VIR_DOMAIN_DISK_MODEL_USB_BOT:
+            if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_BOT)) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                               _("This QEMU doesn't support '-device usb-bot'"));
+                return -1;
+            }
+            break;
+
+        case VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL:
+        case VIR_DOMAIN_DISK_MODEL_VIRTIO:
+        case VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL:
+        case VIR_DOMAIN_DISK_MODEL_LAST:
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                           _("'usb' disk must not be empty"));
+                           _("USB disk supports only the following models: 'usb-storage', 'usb-bot'"));
             return -1;
         }
 

tests/qemuhotplugtest.c

@@ -746,7 +746,9 @@ mymain(void)
     DO_TEST_ATTACH("x86_64", "base-live", "cdrom-usb", false, true,
                    "blockdev-add", QMP_OK,
                    "device_add", QMP_OK,
-                   "query-block", QMP_EMPTY_ARRAY);
+                   "device_add", QMP_OK,
+                   "query-block", QMP_EMPTY_ARRAY,
+                   "qom-set", QMP_OK);
     DO_TEST_DETACH("x86_64", "base-live", "cdrom-usb", true, true,
                    "device_del", QMP_OK);
     DO_TEST_DETACH("x86_64", "base-live", "cdrom-usb", false, false,

tests/qemuxmlconfdata/disk-cdrom-usb-empty.x86_64-latest.abi-update.args

@@ -27,7 +27,8 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
 -no-shutdown \
 -boot strict=on \
 -device '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \
--device '{"driver":"usb-storage","bus":"usb.0","port":"1","id":"usb-disk1","removable":false}' \
+-device '{"driver":"usb-bot","id":"usb-disk1","bus":"usb.0","port":"1"}' \
+-device '{"driver":"scsi-cd","bus":"usb-disk1.0","scsi-id":0,"lun":0,"id":"usb-disk1-device"}' \
 -audiodev '{"id":"audio1","driver":"none"}' \
 -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
 -msg timestamp=on

tests/qemuxmlconfdata/disk-usb-device.x86_64-latest.abi-update.args

@@ -32,19 +32,23 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
 -blockdev '{"driver":"file","filename":"/tmp/img1","node-name":"libvirt-12-storage","read-only":false}' \
 -device '{"driver":"usb-storage","bus":"usb.0","port":"1.1","drive":"libvirt-12-storage","id":"usb-disk0","bootindex":1,"removable":false}' \
 -blockdev '{"driver":"file","filename":"/tmp/img2","node-name":"libvirt-11-storage","read-only":true}' \
--device '{"driver":"usb-storage","bus":"usb.0","port":"1.2","drive":"libvirt-11-storage","id":"usb-disk1","removable":false}' \
+-device '{"driver":"usb-bot","id":"usb-disk1","bus":"usb.0","port":"1.2"}' \
+-device '{"driver":"scsi-cd","bus":"usb-disk1.0","scsi-id":0,"lun":0,"drive":"libvirt-11-storage","id":"usb-disk1-device"}' \
 -blockdev '{"driver":"file","filename":"/tmp/img3","node-name":"libvirt-10-storage","read-only":false}' \
 -device '{"driver":"usb-storage","bus":"usb.0","port":"1.3","drive":"libvirt-10-storage","id":"usb-disk2","removable":false,"serial":"testserial1"}' \
 -blockdev '{"driver":"file","filename":"/tmp/img4","node-name":"libvirt-9-storage","read-only":true}' \
--device '{"driver":"usb-storage","bus":"usb.0","port":"1.4","drive":"libvirt-9-storage","id":"usb-disk3","removable":false,"serial":"testserial2"}' \
+-device '{"driver":"usb-bot","id":"usb-disk3","serial":"testserial2","bus":"usb.0","port":"1.4"}' \
+-device '{"driver":"scsi-cd","bus":"usb-disk3.0","scsi-id":0,"lun":0,"drive":"libvirt-9-storage","id":"usb-disk3-device","serial":"testserial2"}' \
 -blockdev '{"driver":"file","filename":"/tmp/img5","node-name":"libvirt-8-storage","read-only":false}' \
 -device '{"driver":"usb-storage","bus":"usb.0","port":"1.5","drive":"libvirt-8-storage","id":"ua-test1","removable":false}' \
 -blockdev '{"driver":"file","filename":"/tmp/img6","node-name":"libvirt-7-storage","read-only":true}' \
--device '{"driver":"usb-storage","bus":"usb.0","port":"1.6","drive":"libvirt-7-storage","id":"ua-test2","removable":false}' \
+-device '{"driver":"usb-bot","id":"ua-test2","bus":"usb.0","port":"1.6"}' \
+-device '{"driver":"scsi-cd","bus":"ua-test2.0","scsi-id":0,"lun":0,"drive":"libvirt-7-storage","id":"ua-test2-device"}' \
 -blockdev '{"driver":"file","filename":"/tmp/img7","node-name":"libvirt-6-storage","read-only":false}' \
 -device '{"driver":"usb-storage","bus":"usb.0","port":"1.7","drive":"libvirt-6-storage","id":"ua-test3","removable":false,"serial":"testserial3"}' \
 -blockdev '{"driver":"file","filename":"/tmp/img8","node-name":"libvirt-5-storage","read-only":true}' \
--device '{"driver":"usb-storage","bus":"usb.0","port":"1.8","drive":"libvirt-5-storage","id":"ua-test4","removable":false,"serial":"testserial4"}' \
+-device '{"driver":"usb-bot","id":"ua-test4","serial":"testserial4","bus":"usb.0","port":"1.8"}' \
+-device '{"driver":"scsi-cd","bus":"ua-test4.0","scsi-id":0,"lun":0,"drive":"libvirt-5-storage","id":"ua-test4-device","serial":"testserial4"}' \
 -blockdev '{"driver":"file","filename":"/tmp/img9","node-name":"libvirt-4-storage","read-only":false}' \
 -device '{"driver":"usb-storage","bus":"usb.0","port":"2.1","drive":"libvirt-4-storage","id":"usb-disk8","removable":true}' \
 -blockdev '{"driver":"file","filename":"/tmp/imga","node-name":"libvirt-3-storage","read-only":false}' \