Re: [PATCH v2 08/13] conf: introduce usb disk models 'usb-storage' and 'usb-bot'

On Mon, Jun 23, 2025 at 21:59:13 +0200, Peter Krempa wrote: > From: Peter Krempa <pkrempa(a)redhat.com&gt; > > 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 <pkrempa(a)redhat.com&gt; > ---

> + ``<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.