On Mon, Jun 23, 2025 at 21:59:14 +0200, Peter Krempa wrote:
> From: Peter Krempa <pkre...@redhat.com>
>
> While 'usb-bot' and 'usb-storage' are ABI and migration compatible for
> disks it's not the case for cdroms. When migrating from a new config
> using 'usb-bot' to an older daemon which would use 'usb-storage' the
> guest os will get I/O errors.
>
> Thus we must properly fill in models for 'usb' disks so that cdroms can
> be handled properly.
>
> When parsing XML fill in the models and drop the appropriate models when
> formatting migratable XML.
>
> The logic is explained in comments in the code.
>
> Signed-off-by: Peter Krempa <pkre...@redhat.com>
> ---
> src/qemu/qemu_domain.c | 21 ++++++++
> src/qemu/qemu_postparse.c | 49 ++++++++++++++++++-
> src/qemu/qemu_postparse.h | 4 +-
> tests/qemublocktest.c | 13 +++--
> .../qemuhotplug-base-live+cdrom-usb.xml | 2 +-
> .../qemuhotplug-base-live+disk-usb.xml | 2 +-
> .../disk-cache.x86_64-latest.xml | 2 +-
> .../disk-usb-device-model.x86_64-latest.xml | 4 +-
> ...test.QEMU_CAPS_DEVICE_USB_BOT-disabled.xml | 24 ++++-----
> ...date.QEMU_CAPS_DEVICE_USB_BOT-disabled.xml | 24 ++++-----
> ...sk-usb-device.x86_64-latest.abi-update.xml | 24 ++++-----
> .../disk-usb-device.x86_64-latest.xml | 24 ++++-----
> 12 files changed, 132 insertions(+), 61 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index ace42b516a..6e147563f3 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -5342,6 +5342,27 @@ qemuDomainDefFormatBufInternal(virQEMUDriver *driver,
> }
> }
>
> + for (i = 0; i < def->ndisks; i++) {
> + virDomainDiskDef *disk = def->disks[i];
> +
> + /* The 'mode' property for USB disks was introduced long after
> USB
s/mode/model/
> + * disks to allow switching between 'usb-storage' and 'usb-bot'
> + * device. Despite sharing identical implementation 'usb-bot'
> allows
> + * proper configuration of USB cdroms. Unfortunately it is not
> ABI
> + * compatible.
> + *
> + * To preserve migration to older daemons we can strip the model
> to
> + * the default if:
> + * - it's a normal disk (not cdrom) as both are identical
> + * - for a usb-cdrom strip the model if it's not 'usb-bot' as
> that
> + * was the old configuration
> + */
> + if (disk->bus == VIR_DOMAIN_DISK_BUS_USB &&
> + (disk->model == VIR_DOMAIN_DISK_MODEL_USB_STORAGE ||
> + disk->device == VIR_DOMAIN_DISK_DEVICE_DISK))
> + disk->model = VIR_DOMAIN_DISK_MODEL_DEFAULT;
> + }
> +
> /* Replace the CPU definition updated according to QEMU with the one
> * used for starting the domain. The updated def will be sent
> * separately for backward compatibility.
> diff --git a/src/qemu/qemu_postparse.c b/src/qemu/qemu_postparse.c
> index 8150dffac6..7db378c5ce 100644
> --- a/src/qemu/qemu_postparse.c
> +++ b/src/qemu/qemu_postparse.c
> @@ -202,7 +202,8 @@
> qemuDomainDeviceDiskDefPostParseRestoreSecAlias(virDomainDiskDef *disk,
>
> int
> qemuDomainDeviceDiskDefPostParse(virDomainDiskDef *disk,
> - unsigned int parseFlags)
> + unsigned int parseFlags,
> + virQEMUCaps *qemuCaps)
> {
> virStorageSource *n;
>
> @@ -220,6 +221,50 @@ qemuDomainDeviceDiskDefPostParse(virDomainDiskDef *disk,
> disk->mirror->format == VIR_STORAGE_FILE_NONE)
> disk->mirror->format = VIR_STORAGE_FILE_RAW;
>
> + /* default USB disk model:
> + *
> + * Historically we didn't use model for USB disks. It became necessary
> once
> + * it turned out that 'usb-storage' doesn't properly expose CDROM devices
> + * with -blockdev. Additionally 'usb-bot' which does properly handle
> them,
> + * while having identical implementation in qemu and driver in guest, are
> + * not in fact ABI compatible. Thus the logic is as follows:
> + *
> + * If ABI update is not allowed:
> + * - use 'usb-storage' for either (unless only 'usb-bot' is supported)
> + *
> + * If ABI update is possible
> + * - for VIR_DOMAIN_DISK_DEVICE_DISK use 'usb-storage' as it doesn't
> matter
> + * (it is identical with 'usb-bot' ABI wise)
> + * - for VIR_DOMAIN_DISK_DEVICE_CDROM use 'usb-bot' if available
> + * (as it properly exposes cdrom)
> + *
> + * When formatting migratable XML the code strips 'usb-storage' to
> preserve
> + * migration to older daemons. If a new definition with 'usb-bot' cdrom
> is
> + * created via new start or hotplug it will fail migrating. Users must
> + * explicitly set the broken config in XML or unplug the device.
> + */
> + if (qemuCaps &&
> + disk->bus == VIR_DOMAIN_DISK_BUS_USB &&
> + disk->model == VIR_DOMAIN_DISK_MODEL_DEFAULT) {
> +
> + if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM &&
> + parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) {
> +
> + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_BOT)) {
> + disk->model = VIR_DOMAIN_DISK_MODEL_USB_BOT;
> + } else if (virQEMUCapsGet(qemuCaps,
> QEMU_CAPS_DEVICE_USB_STORAGE)) {
> + disk->model = VIR_DOMAIN_DISK_MODEL_USB_STORAGE;
> + }
> +
> + } else {
> + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_STORAGE)) {
I would merge this with the else above to reduce nesting:
if (DEVICE_CDROM && ABI_UPDATE) {
} else if (MODEL_USB_STORAGE) {
...
} else if (MODEL_USB_BOT) {
...
}
> + disk->model = VIR_DOMAIN_DISK_MODEL_USB_STORAGE;
> + } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_BOT)) {
> + disk->model = VIR_DOMAIN_DISK_MODEL_USB_BOT;
> + }
> + }
> + }
> +
The logic is OK. I guess my comment in the previous patch was actually
about using VIR_DOMAIN_DEF_PARSE_ABI_UPDATE flag. But you're not
touching this part here so I guess everything should be fine. Although
I'm still surprised we'd allow ABI update when starting a domain, I
thought this was only allowed when defining it...
> /* default disk encryption engine */
> for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) {
> if (n->encryption && n->encryption->engine ==
> VIR_STORAGE_ENCRYPTION_ENGINE_DEFAULT)
Jirka
