On Mon, Jun 23, 2025 at 21:59:13 +0200, Peter Krempa wrote:
> From: Peter Krempa <pkre...@redhat.com>
>
> Historically libvirt specified 'usb-storage' as driver for USB disks.
> This though combined with '-blockdev' doesn't properly configure the
> device to look like CDROM for <disk type='cdrom'>.
>
> 'usb-bot' acts like a controler and allows explicitly connecting a
> -device to it.
>
> In qemu the devices share implementation so they are effectively
> identical and can be used interchangably. There is though a slight
> difference in how the storage device itself (the SCSI bit) looks when
> CDROM as they were not declared as cdrom before.
Looks like some words were dropped from the last sentence above.
> As this is effectively a bugfix we'll be fixing the behaviour for the
> default configuration. The possibility to explicitly set the model is
> added as a possibility for working around possible problems if they'd
> appear.
>
> Signed-off-by: Peter Krempa <pkre...@redhat.com>
> ---
> docs/formatdomain.rst | 23 +++++--
> src/conf/domain_conf.c | 2 +
> src/conf/domain_conf.h | 3 +
> src/conf/schemas/domaincommon.rng | 2 +
> src/qemu/qemu_domain_address.c | 2 +
> .../disk-usb-device-model.x86_64-latest.args | 46 +++++++++++++
> .../disk-usb-device-model.x86_64-latest.xml | 64 +++++++++++++++++++
> .../qemuxmlconfdata/disk-usb-device-model.xml | 46 +++++++++++++
> tests/qemuxmlconftest.c | 1 +
> 9 files changed, 185 insertions(+), 4 deletions(-)
> create mode 100644
> tests/qemuxmlconfdata/disk-usb-device-model.x86_64-latest.args
> create mode 100644
> tests/qemuxmlconfdata/disk-usb-device-model.x86_64-latest.xml
> create mode 100644 tests/qemuxmlconfdata/disk-usb-device-model.xml
>
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> index ae054a52b3..e4ebf061c7 100644
> --- a/docs/formatdomain.rst
> +++ b/docs/formatdomain.rst
> @@ -2862,10 +2862,25 @@ paravirtualized driver is specified via the ``disk``
> element.
>
> ``model``
> Indicates the emulated device model of the disk. Typically this is
> - indicated solely by the ``bus`` property but for ``bus`` "virtio" the
> - model can be specified further with "virtio", "virtio-transitional" or
> - "virtio-non-transitional". See `virtio device models`_
> - for more details. :since:`Since 5.2.0`
> + indicated solely by the ``bus`` property.
> +
> + For ``bus`` "virtio" the model can be specified further with "virtio",
> + "virtio-transitional" or "virtio-non-transitional". See `virtio device
> + models`_ for more details. :since:`Since 5.2.0`
> +
> + For ``bus`` "usb" the model can be specified further with
> ``usb-storage``
> + or ``usb-bot``. There is no difference between the two models for
> + ``<disk type='disk'``. However with ``usb-bot`` a device configured as
Missing > in the disk element.
> + ``<disk type='cdrom'>`` is properly exposed as a cdrom device inside
> the
> + guest OS. Unfortunately this configuration is not ABI compatible and
> thus
> + it can't be interchanged.
This is not ABI compatible with usb-storage model, right? I suggest
being explicit about it.
> +
> + The QEMU hypervisor driver will pick ``usb-bot`` for cold starts or
> + hotplug for cdrom devices to properly configure the devices. This is
> + not compatible for migration to older versions of libvirt and explicit
> + configuration needs to be used.
So are you saying starting an existing domain with usb cdrom after
upgrading libvirt will cause issues during migration to an older
libvirt? I don't think we can do this. We should rather pick usb-storage
(which is the case now as I understood from the description) unless
usb-bot is set explicitly in the XML.
> + :since:`Since 11.5.0`; relevant only for ``QEMU`` hypervisor.
> +
> ``rawio``
> Indicates whether the disk needs rawio capability. Valid settings are
> "yes" or "no" (default is "no"). If any one disk in a domain has
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 1a6c8afb1d..2882a7746b 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1399,6 +1399,8 @@ VIR_ENUM_IMPL(virDomainDiskModel,
> "virtio",
> "virtio-transitional",
> "virtio-non-transitional",
> + "usb-storage",
> + "usb-bot",
> );
>
> VIR_ENUM_IMPL(virDomainDiskMirrorState,
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 3d380073cf..73e8a2fb99 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -439,6 +439,9 @@ typedef enum {
> VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL,
> VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL,
>
Extra empty line?
> + VIR_DOMAIN_DISK_MODEL_USB_STORAGE,
> + VIR_DOMAIN_DISK_MODEL_USB_BOT,
> +
> VIR_DOMAIN_DISK_MODEL_LAST
> } virDomainDiskModel;
>
> diff --git a/src/conf/schemas/domaincommon.rng
> b/src/conf/schemas/domaincommon.rng
> index b1fe51f519..a80005562a 100644
> --- a/src/conf/schemas/domaincommon.rng
> +++ b/src/conf/schemas/domaincommon.rng
> @@ -1766,6 +1766,8 @@
> <value>virtio</value>
> <value>virtio-transitional</value>
> <value>virtio-non-transitional</value>
> + <value>usb-storage</value>
> + <value>usb-bot</value>
> </choice>
> </attribute>
> </optional>
> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> index bb86cfa0c3..3a23f70d39 100644
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -729,6 +729,8 @@
> qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDef *dev,
> case VIR_DOMAIN_DISK_MODEL_DEFAULT:
> return virtioFlags;
> case VIR_DOMAIN_DISK_MODEL_LAST:
> + case VIR_DOMAIN_DISK_MODEL_USB_STORAGE:
> + case VIR_DOMAIN_DISK_MODEL_USB_BOT:
> break;
> }
> return 0;
> diff --git a/tests/qemuxmlconfdata/disk-usb-device-model.x86_64-latest.args
> b/tests/qemuxmlconfdata/disk-usb-device-model.x86_64-latest.args
> new file mode 100644
> index 0000000000..6d31319a49
> --- /dev/null
> +++ b/tests/qemuxmlconfdata/disk-usb-device-model.x86_64-latest.args
> @@ -0,0 +1,46 @@
> +LC_ALL=C \
> +PATH=/bin \
> +HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1 \
> +USER=test \
> +LOGNAME=test \
> +XDG_DATA_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.local/share \
> +XDG_CACHE_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.cache \
> +XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
> +/usr/bin/qemu-system-x86_64 \
> +-name guest=QEMUGuest1,debug-threads=on \
> +-S \
> +-object
> '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/var/lib/libvirt/qemu/domain--1-QEMUGuest1/master-key.aes"}'
> \
> +-machine pc,usb=off,dump-guest-core=off,memory-backend=pc.ram,acpi=off \
> +-accel tcg \
> +-cpu qemu64 \
> +-m size=219136k \
> +-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":224395264}' \
> +-overcommit mem-lock=off \
> +-smp 1,sockets=1,cores=1,threads=1 \
> +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
> +-display none \
> +-no-user-config \
> +-nodefaults \
> +-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \
> +-mon chardev=charmonitor,id=monitor,mode=control \
> +-rtc base=utc \
> +-no-shutdown \
> +-boot strict=on \
> +-device
> '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \
> +-device '{"driver":"usb-hub","id":"hub0","bus":"usb.0","port":"1"}' \
> +-blockdev
> '{"driver":"file","filename":"/tmp/img1","node-name":"libvirt-6-storage","read-only":false}'
> \
> +-device
> '{"driver":"usb-storage","bus":"usb.0","port":"2","drive":"libvirt-6-storage","id":"usb-disk0","bootindex":1,"removable":false}'
> \
> +-blockdev
> '{"driver":"file","filename":"/tmp/img2","node-name":"libvirt-5-storage","read-only":true}'
> \
> +-device
> '{"driver":"usb-storage","bus":"usb.0","port":"1.1","drive":"libvirt-5-storage","id":"usb-disk1","removable":false}'
> \
> +-blockdev
> '{"driver":"file","filename":"/tmp/img3","node-name":"libvirt-4-storage","read-only":false}'
> \
> +-device
> '{"driver":"usb-storage","bus":"usb.0","port":"1.2","drive":"libvirt-4-storage","id":"usb-disk2","removable":false}'
> \
> +-blockdev
> '{"driver":"file","filename":"/tmp/img4","node-name":"libvirt-3-storage","read-only":true}'
> \
> +-device
> '{"driver":"usb-storage","bus":"usb.0","port":"1.3","drive":"libvirt-3-storage","id":"usb-disk3","removable":false}'
> \
> +-blockdev
> '{"driver":"file","filename":"/tmp/img5","node-name":"libvirt-2-storage","read-only":false}'
> \
> +-device
> '{"driver":"usb-storage","bus":"usb.0","port":"1.4","drive":"libvirt-2-storage","id":"usb-disk4","removable":false}'
> \
> +-blockdev
> '{"driver":"file","filename":"/tmp/img6","node-name":"libvirt-1-storage","read-only":true}'
> \
> +-device
> '{"driver":"usb-storage","bus":"usb.0","port":"1.5","drive":"libvirt-1-storage","id":"usb-disk5","removable":false}'
> \
Well, this is strange. The XML uses both usb-storage and usb-bot, but
only usb-storage is present in *.args. This kind of makes sense since
the code for handling model is not wired up yet, but having the tests in
a commit that describes the new models and how libvirt handles them is
confusing.
Jirka
