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

On Tue, Jun 24, 2025 at 11:25:03 +0200, Jiri Denemark wrote: > 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. Yes it will not work if you start a VM and attempt to migrate it to an older version. The reason why I did this is that this very technically is a ABI bug fix. Prior to use of '-blockdev', when the disk backend was instantiated via -drive, we'd format 'media=cdrom' with the backend which would be picked up with the 'usb-storage' frontend to make it into a cdrom: Now this went unnoticed at that time. While I debated just changing it, the fact that it silently corrupts inside the guest was a dealbreaker for me. But at the same time it's IMO unacceptable to default to the broken configuration which actually did work before. Thus I decided to do this where users can switch to the broken configuration (or just detach the cdrom) but the config with no extra bits will result to the proper configuration as it should have before. Resulting to the broken config is obviously much easier on the logic here but I really don't think we should do it.7

> > 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 > > 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. I thought about activating the capability later, but decided against as it was showing all updates together. So while this ordering creates a history point where this doesn't work , it's much easier to see what the patches are doing actually.