Right now we call qemuValidateDomainDeviceDefControllerUSB()
quite late, just as we're generating the QEMU command line.
The intention here is to prevent configurations from being
rejected, even though a default USB controller model could not
be found, because using -usb could work as a last resort.
As it turns out, this premise is flawed, as can easily be
demonstrated by using a build of QEMU which has the default
USB controller compiled out:
$ qemu-system-x86_64 -M pc -device piix3-usb-uhci
'piix3-usb-uhci' is not a valid device model name
$ qemu-system-x86_64 -M pc -usb
missing object type 'piix3-usb-uhci'
In other words, if the device that needs to be built into QEMU
in order for -usb to work was available, we would have detected
it and used it via -device instead; if we didn't, using -usb
won't change anything.
Unsurprisingly, a number of test cases start failing, or fail
in a different way, because of this change. That's okay: even
in the unlikely event that there actually are any existing
guests with such problematic configurations out there, they
will not disappear but merely fail to start, and the user will
have the opportunity to fix them.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
src/qemu/qemu_command.c | 68 +------------------
src/qemu/qemu_validate.c | 68 ++++++++++++++++++-
...ault-unavailable-i440fx.x86_64-latest.args | 33 ---------
...fault-unavailable-i440fx.x86_64-latest.err | 1 +
...fault-unavailable-i440fx.x86_64-latest.xml | 31 ---------
...-default-unavailable-q35.x86_64-latest.xml | 33 ---------
...ler-nec-xhci-unavailable.x86_64-latest.xml | 33 ---------
.../usb-legacy-device.x86_64-latest.args | 33 ---------
.../usb-legacy-device.x86_64-latest.err | 1 +
.../usb-legacy-device.x86_64-latest.xml | 30 --------
.../usb-legacy-multiple.x86_64-latest.err | 2 +-
.../usb-legacy-multiple.x86_64-latest.xml | 32 ---------
tests/qemuxmlconftest.c | 12 ++--
13 files changed, 79 insertions(+), 298 deletions(-)
delete mode 100644 tests/qemuxmlconfdata/usb-controller-default-unavailable-i440fx.x86_64-latest.args
create mode 100644 tests/qemuxmlconfdata/usb-controller-default-unavailable-i440fx.x86_64-latest.err
delete mode 100644 tests/qemuxmlconfdata/usb-controller-default-unavailable-i440fx.x86_64-latest.xml
delete mode 100644 tests/qemuxmlconfdata/usb-controller-default-unavailable-q35.x86_64-latest.xml
delete mode 100644 tests/qemuxmlconfdata/usb-controller-nec-xhci-unavailable.x86_64-latest.xml
delete mode 100644 tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.args
create mode 100644 tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.err
delete mode 100644 tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.xml
delete mode 100644 tests/qemuxmlconfdata/usb-legacy-multiple.x86_64-latest.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 7824c31bde..a544a3ccdc 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2505,66 +2505,6 @@ qemuBuildFilesystemCommandLine(virCommand *cmd,
}
-static int
-qemuControllerModelUSBToCaps(int model)
-{
- switch (model) {
- case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI:
- return QEMU_CAPS_PIIX3_USB_UHCI;
- case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX4_UHCI:
- return QEMU_CAPS_PIIX4_USB_UHCI;
- case VIR_DOMAIN_CONTROLLER_MODEL_USB_EHCI:
- return QEMU_CAPS_USB_EHCI;
- case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1:
- case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1:
- case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI2:
- case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3:
- return QEMU_CAPS_ICH9_USB_EHCI1;
- case VIR_DOMAIN_CONTROLLER_MODEL_USB_VT82C686B_UHCI:
- return QEMU_CAPS_VT82C686B_USB_UHCI;
- case VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI:
- return QEMU_CAPS_PCI_OHCI;
- case VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI:
- return QEMU_CAPS_NEC_USB_XHCI;
- case VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI:
- return QEMU_CAPS_DEVICE_QEMU_XHCI;
- default:
- return -1;
- }
-}
-
-
-static int
-qemuValidateDomainDeviceDefControllerUSB(const virDomainControllerDef *def,
- virQEMUCaps *qemuCaps)
-{
- if (def->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("no model provided for USB controller"));
- return -1;
- }
-
- if (!virQEMUCapsGet(qemuCaps, qemuControllerModelUSBToCaps(def->model))) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("USB controller model '%1$s' not supported in this QEMU binary"),
- virDomainControllerModelUSBTypeToString(def->model));
- return -1;
- }
-
- if (def->opts.usbopts.ports != -1) {
- if (def->model != VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI &&
- def->model != VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("usb controller type '%1$s' doesn't support 'ports' with this QEMU binary"),
- virDomainControllerModelUSBTypeToString(def->model));
- return -1;
- }
- }
-
- return 0;
-}
-
-
static const char *
qemuBuildUSBControllerFindMasterAlias(const virDomainDef *domainDef,
const virDomainControllerDef *def)
@@ -2592,14 +2532,10 @@ qemuBuildUSBControllerFindMasterAlias(const virDomainDef *domainDef,
static virJSONValue *
qemuBuildUSBControllerDevProps(const virDomainDef *domainDef,
- virDomainControllerDef *def,
- virQEMUCaps *qemuCaps)
+ virDomainControllerDef *def)
{
g_autoptr(virJSONValue) props = NULL;
- if (qemuValidateDomainDeviceDefControllerUSB(def, qemuCaps) < 0)
- return NULL;
-
if (virJSONValueObjectAdd(&props,
"s:driver", qemuControllerModelUSBTypeToString(def->model),
"k:p2", def->opts.usbopts.ports,
@@ -2886,7 +2822,7 @@ qemuBuildControllerDevProps(const virDomainDef *domainDef,
break;
case VIR_DOMAIN_CONTROLLER_TYPE_USB:
- if (!(props = qemuBuildUSBControllerDevProps(domainDef, def, qemuCaps)))
+ if (!(props = qemuBuildUSBControllerDevProps(domainDef, def)))
return -1;
break;
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 73afd094a9..ad1621a120 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -3523,6 +3523,69 @@ qemuValidateDomainDeviceDefControllerSCSI(const virDomainControllerDef *controll
}
+static int
+qemuControllerModelUSBToCaps(int model)
+{
+ switch (model) {
+ case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI:
+ return QEMU_CAPS_PIIX3_USB_UHCI;
+ case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX4_UHCI:
+ return QEMU_CAPS_PIIX4_USB_UHCI;
+ case VIR_DOMAIN_CONTROLLER_MODEL_USB_EHCI:
+ return QEMU_CAPS_USB_EHCI;
+ case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1:
+ case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1:
+ case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI2:
+ case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3:
+ return QEMU_CAPS_ICH9_USB_EHCI1;
+ case VIR_DOMAIN_CONTROLLER_MODEL_USB_VT82C686B_UHCI:
+ return QEMU_CAPS_VT82C686B_USB_UHCI;
+ case VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI:
+ return QEMU_CAPS_PCI_OHCI;
+ case VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI:
+ return QEMU_CAPS_NEC_USB_XHCI;
+ case VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI:
+ return QEMU_CAPS_DEVICE_QEMU_XHCI;
+ default:
+ return -1;
+ }
+}
+
+
+static int
+qemuValidateDomainDeviceDefControllerUSB(const virDomainControllerDef *def,
+ virQEMUCaps *qemuCaps)
+{
+ if (def->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE)
+ return 0;
+
+ if (def->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("no model provided for USB controller"));
+ return -1;
+ }
+
+ if (!virQEMUCapsGet(qemuCaps, qemuControllerModelUSBToCaps(def->model))) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("USB controller model '%1$s' not supported in this QEMU binary"),
+ virDomainControllerModelUSBTypeToString(def->model));
+ return -1;
+ }
+
+ if (def->opts.usbopts.ports != -1) {
+ if (def->model != VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI &&
+ def->model != VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("usb controller type '%1$s' doesn't support 'ports' with this QEMU binary"),
+ virDomainControllerModelUSBTypeToString(def->model));
+ return -1;
+ }
+ }
+
+ return 0;
+}
+
+
/**
* virValidateControllerPCIModelNameToQEMUCaps:
* @modelName: model name
@@ -4150,10 +4213,13 @@ qemuValidateDomainDeviceDefController(const virDomainControllerDef *controller,
qemuCaps);
break;
+ case VIR_DOMAIN_CONTROLLER_TYPE_USB:
+ ret = qemuValidateDomainDeviceDefControllerUSB(controller, qemuCaps);
+ break;
+
case VIR_DOMAIN_CONTROLLER_TYPE_FDC:
case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL:
case VIR_DOMAIN_CONTROLLER_TYPE_CCID:
- case VIR_DOMAIN_CONTROLLER_TYPE_USB:
case VIR_DOMAIN_CONTROLLER_TYPE_XENBUS:
case VIR_DOMAIN_CONTROLLER_TYPE_ISA:
case VIR_DOMAIN_CONTROLLER_TYPE_LAST:
diff --git a/tests/qemuxmlconfdata/usb-controller-default-unavailable-i440fx.x86_64-latest.args b/tests/qemuxmlconfdata/usb-controller-default-unavailable-i440fx.x86_64-latest.args
deleted file mode 100644
index c8de26e4ee..0000000000
--- a/tests/qemuxmlconfdata/usb-controller-default-unavailable-i440fx.x86_64-latest.args
+++ /dev/null
@@ -1,33 +0,0 @@
-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 \
--usb \
--audiodev '{"id":"audio1","driver":"none"}' \
--device '{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.0","addr":"0x2"}' \
--sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
--msg timestamp=on
diff --git a/tests/qemuxmlconfdata/usb-controller-default-unavailable-i440fx.x86_64-latest.err b/tests/qemuxmlconfdata/usb-controller-default-unavailable-i440fx.x86_64-latest.err
new file mode 100644
index 0000000000..7a71aa107d
--- /dev/null
+++ b/tests/qemuxmlconfdata/usb-controller-default-unavailable-i440fx.x86_64-latest.err
@@ -0,0 +1 @@
+unsupported configuration: no model provided for USB controller
diff --git a/tests/qemuxmlconfdata/usb-controller-default-unavailable-i440fx.x86_64-latest.xml b/tests/qemuxmlconfdata/usb-controller-default-unavailable-i440fx.x86_64-latest.xml
deleted file mode 100644
index 183cfe3b9a..0000000000
--- a/tests/qemuxmlconfdata/usb-controller-default-unavailable-i440fx.x86_64-latest.xml
+++ /dev/null
@@ -1,31 +0,0 @@
-<domain type='qemu'>
- <name>QEMUGuest1</name>
- <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
- <memory unit='KiB'>219136</memory>
- <currentMemory unit='KiB'>219136</currentMemory>
- <vcpu placement='static'>1</vcpu>
- <os>
- <type arch='x86_64' machine='pc'>hvm</type>
- <boot dev='hd'/>
- </os>
- <cpu mode='custom' match='exact' check='none'>
- <model fallback='forbid'>qemu64</model>
- </cpu>
- <clock offset='utc'/>
- <on_poweroff>destroy</on_poweroff>
- <on_reboot>restart</on_reboot>
- <on_crash>destroy</on_crash>
- <devices>
- <emulator>/usr/bin/qemu-system-x86_64</emulator>
- <controller type='usb' index='0'>
- <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
- </controller>
- <controller type='pci' index='0' model='pci-root'/>
- <input type='mouse' bus='ps2'/>
- <input type='keyboard' bus='ps2'/>
- <audio id='1' type='none'/>
- <memballoon model='virtio'>
- <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
- </memballoon>
- </devices>
-</domain>
diff --git a/tests/qemuxmlconfdata/usb-controller-default-unavailable-q35.x86_64-latest.xml b/tests/qemuxmlconfdata/usb-controller-default-unavailable-q35.x86_64-latest.xml
deleted file mode 100644
index c857816a3e..0000000000
--- a/tests/qemuxmlconfdata/usb-controller-default-unavailable-q35.x86_64-latest.xml
+++ /dev/null
@@ -1,33 +0,0 @@
-<domain type='qemu'>
- <name>q35-test</name>
- <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid>
- <memory unit='KiB'>2097152</memory>
- <currentMemory unit='KiB'>2097152</currentMemory>
- <vcpu placement='static'>1</vcpu>
- <os>
- <type arch='x86_64' machine='q35'>hvm</type>
- <boot dev='hd'/>
- </os>
- <cpu mode='custom' match='exact' check='none'>
- <model fallback='forbid'>qemu64</model>
- </cpu>
- <clock offset='utc'/>
- <on_poweroff>destroy</on_poweroff>
- <on_reboot>restart</on_reboot>
- <on_crash>destroy</on_crash>
- <devices>
- <emulator>/usr/bin/qemu-system-x86_64</emulator>
- <controller type='pci' index='0' model='pcie-root'/>
- <controller type='usb' index='0'>
- <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/>
- </controller>
- <controller type='sata' index='0'>
- <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/>
- </controller>
- <input type='mouse' bus='ps2'/>
- <input type='keyboard' bus='ps2'/>
- <audio id='1' type='none'/>
- <watchdog model='itco' action='reset'/>
- <memballoon model='none'/>
- </devices>
-</domain>
diff --git a/tests/qemuxmlconfdata/usb-controller-nec-xhci-unavailable.x86_64-latest.xml b/tests/qemuxmlconfdata/usb-controller-nec-xhci-unavailable.x86_64-latest.xml
deleted file mode 100644
index e6f61c20c3..0000000000
--- a/tests/qemuxmlconfdata/usb-controller-nec-xhci-unavailable.x86_64-latest.xml
+++ /dev/null
@@ -1,33 +0,0 @@
-<domain type='qemu'>
- <name>q35-test</name>
- <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid>
- <memory unit='KiB'>2097152</memory>
- <currentMemory unit='KiB'>2097152</currentMemory>
- <vcpu placement='static'>1</vcpu>
- <os>
- <type arch='x86_64' machine='q35'>hvm</type>
- <boot dev='hd'/>
- </os>
- <cpu mode='custom' match='exact' check='none'>
- <model fallback='forbid'>qemu64</model>
- </cpu>
- <clock offset='utc'/>
- <on_poweroff>destroy</on_poweroff>
- <on_reboot>restart</on_reboot>
- <on_crash>destroy</on_crash>
- <devices>
- <emulator>/usr/bin/qemu-system-x86_64</emulator>
- <controller type='pci' index='0' model='pcie-root'/>
- <controller type='usb' index='0' model='nec-xhci'>
- <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/>
- </controller>
- <controller type='sata' index='0'>
- <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/>
- </controller>
- <input type='mouse' bus='ps2'/>
- <input type='keyboard' bus='ps2'/>
- <audio id='1' type='none'/>
- <watchdog model='itco' action='reset'/>
- <memballoon model='none'/>
- </devices>
-</domain>
diff --git a/tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.args b/tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.args
deleted file mode 100644
index 1ef9965cbd..0000000000
--- a/tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.args
+++ /dev/null
@@ -1,33 +0,0 @@
-LC_ALL=C \
-PATH=/bin \
-HOME=/var/lib/libvirt/qemu/domain--1-guest \
-USER=test \
-LOGNAME=test \
-XDG_DATA_HOME=/var/lib/libvirt/qemu/domain--1-guest/.local/share \
-XDG_CACHE_HOME=/var/lib/libvirt/qemu/domain--1-guest/.cache \
-XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-guest/.config \
-/usr/bin/qemu-system-x86_64 \
--name guest=guest,debug-threads=on \
--S \
--object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/var/lib/libvirt/qemu/domain--1-guest/master-key.aes"}' \
--machine pc,usb=off,dump-guest-core=off,memory-backend=pc.ram,acpi=off \
--accel tcg \
--cpu qemu64 \
--m size=4194304k \
--object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":4294967296}' \
--overcommit mem-lock=off \
--smp 4,sockets=4,cores=1,threads=1 \
--uuid 1ccfd97d-5eb4-478a-bbe6-88d254c16db7 \
--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 \
--usb \
--device '{"driver":"usb-tablet","id":"input0","bus":"usb.0","port":"1"}' \
--audiodev '{"id":"audio1","driver":"none"}' \
--sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
--msg timestamp=on
diff --git a/tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.err b/tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.err
new file mode 100644
index 0000000000..7a71aa107d
--- /dev/null
+++ b/tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.err
@@ -0,0 +1 @@
+unsupported configuration: no model provided for USB controller
diff --git a/tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.xml b/tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.xml
deleted file mode 100644
index 2204c03380..0000000000
--- a/tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.xml
+++ /dev/null
@@ -1,30 +0,0 @@
-<domain type='qemu'>
- <name>guest</name>
- <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid>
- <memory unit='KiB'>4194304</memory>
- <currentMemory unit='KiB'>4194304</currentMemory>
- <vcpu placement='static'>4</vcpu>
- <os>
- <type arch='x86_64' machine='pc'>hvm</type>
- <boot dev='hd'/>
- </os>
- <cpu mode='custom' match='exact' check='none'>
- <model fallback='forbid'>qemu64</model>
- </cpu>
- <clock offset='utc'/>
- <on_poweroff>destroy</on_poweroff>
- <on_reboot>restart</on_reboot>
- <on_crash>destroy</on_crash>
- <devices>
- <emulator>/usr/bin/qemu-system-x86_64</emulator>
- <controller type='usb' index='0'>
- <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
- </controller>
- <controller type='pci' index='0' model='pci-root'/>
- <input type='tablet' bus='usb'/>
- <input type='mouse' bus='ps2'/>
- <input type='keyboard' bus='ps2'/>
- <audio id='1' type='none'/>
- <memballoon model='none'/>
- </devices>
-</domain>
diff --git a/tests/qemuxmlconfdata/usb-legacy-multiple.x86_64-latest.err b/tests/qemuxmlconfdata/usb-legacy-multiple.x86_64-latest.err
index 4cf41f9406..7a71aa107d 100644
--- a/tests/qemuxmlconfdata/usb-legacy-multiple.x86_64-latest.err
+++ b/tests/qemuxmlconfdata/usb-legacy-multiple.x86_64-latest.err
@@ -1 +1 @@
-unsupported configuration: Multiple legacy USB controllers are not supported
+unsupported configuration: no model provided for USB controller
diff --git a/tests/qemuxmlconfdata/usb-legacy-multiple.x86_64-latest.xml b/tests/qemuxmlconfdata/usb-legacy-multiple.x86_64-latest.xml
deleted file mode 100644
index 431599283d..0000000000
--- a/tests/qemuxmlconfdata/usb-legacy-multiple.x86_64-latest.xml
+++ /dev/null
@@ -1,32 +0,0 @@
-<domain type='qemu'>
- <name>guest</name>
- <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid>
- <memory unit='KiB'>4194304</memory>
- <currentMemory unit='KiB'>4194304</currentMemory>
- <vcpu placement='static'>4</vcpu>
- <os>
- <type arch='x86_64' machine='pc'>hvm</type>
- <boot dev='hd'/>
- </os>
- <cpu mode='custom' match='exact' check='none'>
- <model fallback='forbid'>qemu64</model>
- </cpu>
- <clock offset='utc'/>
- <on_poweroff>destroy</on_poweroff>
- <on_reboot>restart</on_reboot>
- <on_crash>destroy</on_crash>
- <devices>
- <emulator>/usr/bin/qemu-system-x86_64</emulator>
- <controller type='usb' index='0'>
- <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
- </controller>
- <controller type='usb' index='1'>
- <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
- </controller>
- <controller type='pci' index='0' model='pci-root'/>
- <input type='mouse' bus='ps2'/>
- <input type='keyboard' bus='ps2'/>
- <audio id='1' type='none'/>
- <memballoon model='none'/>
- </devices>
-</domain>
diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c
index b7778975c3..db8243905c 100644
--- a/tests/qemuxmlconftest.c
+++ b/tests/qemuxmlconftest.c
@@ -1848,17 +1848,18 @@ mymain(void)
DO_TEST_CAPS_LATEST_PARSE_ERROR("usb-controller-default-isapc");
DO_TEST_CAPS_LATEST("usb-controller-default-i440fx");
DO_TEST_CAPS_LATEST("usb-controller-default-q35");
- /* i440fx downgrades to use '-usb' if the explicit controller is not present */
+ /* Both i440fx and q35 error out when the default USB controller
+ * type is not available */
DO_TEST_FULL("usb-controller-default-unavailable-i440fx", ".x86_64-latest",
ARG_CAPS_ARCH, "x86_64",
ARG_CAPS_VER, "latest",
+ ARG_FLAGS, FLAG_EXPECT_PARSE_ERROR,
ARG_QEMU_CAPS_DEL, QEMU_CAPS_PIIX3_USB_UHCI, QEMU_CAPS_LAST,
ARG_END);
- /* q35 fails instead */
DO_TEST_FULL("usb-controller-default-unavailable-q35", ".x86_64-latest",
ARG_CAPS_ARCH, "x86_64",
ARG_CAPS_VER, "latest",
- ARG_FLAGS, FLAG_EXPECT_FAILURE,
+ ARG_FLAGS, FLAG_EXPECT_PARSE_ERROR,
ARG_QEMU_CAPS_DEL, QEMU_CAPS_PIIX3_USB_UHCI, QEMU_CAPS_LAST,
ARG_END);
/* However, if the USB controller is the one that gets added
@@ -1873,12 +1874,13 @@ mymain(void)
DO_TEST_FULL("usb-legacy-multiple", ".x86_64-latest",
ARG_CAPS_ARCH, "x86_64",
ARG_CAPS_VER, "latest",
- ARG_FLAGS, FLAG_EXPECT_FAILURE,
+ ARG_FLAGS, FLAG_EXPECT_PARSE_ERROR,
ARG_QEMU_CAPS_DEL, QEMU_CAPS_PIIX3_USB_UHCI, QEMU_CAPS_LAST,
ARG_END);
DO_TEST_FULL("usb-legacy-device", ".x86_64-latest",
ARG_CAPS_ARCH, "x86_64",
ARG_CAPS_VER, "latest",
+ ARG_FLAGS, FLAG_EXPECT_PARSE_ERROR,
ARG_QEMU_CAPS_DEL, QEMU_CAPS_PIIX3_USB_UHCI, QEMU_CAPS_LAST,
ARG_END);
@@ -1895,7 +1897,7 @@ mymain(void)
DO_TEST_FULL("usb-controller-nec-xhci-unavailable", ".x86_64-latest",
ARG_CAPS_ARCH, "x86_64",
ARG_CAPS_VER, "latest",
- ARG_FLAGS, FLAG_EXPECT_FAILURE,
+ ARG_FLAGS, FLAG_EXPECT_PARSE_ERROR,
ARG_QEMU_CAPS_DEL, QEMU_CAPS_NEC_USB_XHCI, QEMU_CAPS_LAST,
ARG_END);
DO_TEST_CAPS_LATEST("usb-controller-nex-xhci-autoassign");
--
2.43.0
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
On Wed, Feb 14, 2024 at 18:11:18 +0100, Andrea Bolognani wrote:
> Right now we call qemuValidateDomainDeviceDefControllerUSB()
> quite late, just as we're generating the QEMU command line.
>
> The intention here is to prevent configurations from being
> rejected, even though a default USB controller model could not
> be found, because using -usb could work as a last resort.
>
> As it turns out, this premise is flawed, as can easily be
> demonstrated by using a build of QEMU which has the default
> USB controller compiled out:
>
> $ qemu-system-x86_64 -M pc -device piix3-usb-uhci
> 'piix3-usb-uhci' is not a valid device model name
>
> $ qemu-system-x86_64 -M pc -usb
> missing object type 'piix3-usb-uhci'
Could you please elaborate how you've got this? I had to disable the
CONFIG_I440FX=n option to achieve that (without messing with the machine
dependencies in the first place) which also means that that the 'pc'
machine type will not work:
'piix3-usb-uhci' is provided by 'hw/usb/hcd-uhci.c', which is
conditionally compiled based on the follwing rule:
system_ss.add(when: 'CONFIG_USB_UHCI', if_true: files('hcd-uhci.c'))
USB_UHCI is selected(required) by 'config PIIX' (hw/isa/Kconfig).
PIIX is selected by 'config I440FX'.
Thus it is impossible to build qemu with a 'pc' machine type but
missing 'piix3-usb-uhci.
By breaking the 'config PIIX' option you can achieve that.
Thus by definition all 'pc' machine-type based devices must have the
'piix3-usb-uhci' type compiled in.
And which thus implies that '-usb' will never be used in such case.
> In other words, if the device that needs to be built into QEMU
> in order for -usb to work was available, we would have detected
> it and used it via -device instead; if we didn't, using -usb
> won't change anything.
So if the above is the case, I'd rephrase this paragraph to say that
qemu can't be built without the explicitly detectable controller.
Now the question is only whether that applies for qemu-4.2 and other
machines. as well. But if yes, all the -usb thing can be removed.
I'll get back to that tomorrow.
>
> Unsurprisingly, a number of test cases start failing, or fail
> in a different way, because of this change. That's okay: even
> in the unlikely event that there actually are any existing
> guests with such problematic configurations out there, they
> will not disappear but merely fail to start, and the user will
> have the opportunity to fix them.
>
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
> src/qemu/qemu_command.c | 68 +------------------
> src/qemu/qemu_validate.c | 68 ++++++++++++++++++-
> ...ault-unavailable-i440fx.x86_64-latest.args | 33 ---------
> ...fault-unavailable-i440fx.x86_64-latest.err | 1 +
> ...fault-unavailable-i440fx.x86_64-latest.xml | 31 ---------
> ...-default-unavailable-q35.x86_64-latest.xml | 33 ---------
> ...ler-nec-xhci-unavailable.x86_64-latest.xml | 33 ---------
> .../usb-legacy-device.x86_64-latest.args | 33 ---------
> .../usb-legacy-device.x86_64-latest.err | 1 +
> .../usb-legacy-device.x86_64-latest.xml | 30 --------
> .../usb-legacy-multiple.x86_64-latest.err | 2 +-
> .../usb-legacy-multiple.x86_64-latest.xml | 32 ---------
> tests/qemuxmlconftest.c | 12 ++--
> 13 files changed, 79 insertions(+), 298 deletions(-)
> delete mode 100644 tests/qemuxmlconfdata/usb-controller-default-unavailable-i440fx.x86_64-latest.args
> create mode 100644 tests/qemuxmlconfdata/usb-controller-default-unavailable-i440fx.x86_64-latest.err
> delete mode 100644 tests/qemuxmlconfdata/usb-controller-default-unavailable-i440fx.x86_64-latest.xml
> delete mode 100644 tests/qemuxmlconfdata/usb-controller-default-unavailable-q35.x86_64-latest.xml
> delete mode 100644 tests/qemuxmlconfdata/usb-controller-nec-xhci-unavailable.x86_64-latest.xml
> delete mode 100644 tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.args
> create mode 100644 tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.err
> delete mode 100644 tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.xml
> delete mode 100644 tests/qemuxmlconfdata/usb-legacy-multiple.x86_64-latest.xml
> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> index 73afd094a9..ad1621a120 100644
> --- a/src/qemu/qemu_validate.c
> +++ b/src/qemu/qemu_validate.c
> @@ -3523,6 +3523,69 @@ qemuValidateDomainDeviceDefControllerSCSI(const virDomainControllerDef *controll
> }
>
>
> +static int
> +qemuControllerModelUSBToCaps(int model)
Any reason why 'model' doesn't use the proper type:
virDomainControllerModelUSB and the return value type isn't virQEMUCapsFlags?
> +{
> + switch (model) {
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI:
> + return QEMU_CAPS_PIIX3_USB_UHCI;
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX4_UHCI:
> + return QEMU_CAPS_PIIX4_USB_UHCI;
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_EHCI:
> + return QEMU_CAPS_USB_EHCI;
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1:
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1:
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI2:
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3:
> + return QEMU_CAPS_ICH9_USB_EHCI1;
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_VT82C686B_UHCI:
> + return QEMU_CAPS_VT82C686B_USB_UHCI;
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI:
> + return QEMU_CAPS_PCI_OHCI;
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI:
> + return QEMU_CAPS_NEC_USB_XHCI;
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI:
> + return QEMU_CAPS_DEVICE_QEMU_XHCI;
> + default:
> + return -1;
In case you'll need a catch-all use QEMU_CAPS_LAST as "error"
> + }
> +}
> +
> +
> +static int
> +qemuValidateDomainDeviceDefControllerUSB(const virDomainControllerDef *def,
> + virQEMUCaps *qemuCaps)
> +{
> + if (def->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE)
> + return 0;
> +
> + if (def->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("no model provided for USB controller"));
> + return -1;
> + }
> +
> + if (!virQEMUCapsGet(qemuCaps, qemuControllerModelUSBToCaps(def->model))) {
Return value is used directly as capability flag.
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("USB controller model '%1$s' not supported in this QEMU binary"),
> + virDomainControllerModelUSBTypeToString(def->model));
> + return -1;
> + }
> +
> + if (def->opts.usbopts.ports != -1) {
> + if (def->model != VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI &&
> + def->model != VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("usb controller type '%1$s' doesn't support 'ports' with this QEMU binary"),
> + virDomainControllerModelUSBTypeToString(def->model));
> + return -1;
> + }
> + }
> +
> + return 0;
> +}
[...]
> --audiodev '{"id":"audio1","driver":"none"}' \
> --device '{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.0","addr":"0x2"}' \
> --sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
> --msg timestamp=on
> diff --git a/tests/qemuxmlconfdata/usb-controller-default-unavailable-i440fx.x86_64-latest.err b/tests/qemuxmlconfdata/usb-controller-default-unavailable-i440fx.x86_64-latest.err
> new file mode 100644
> index 0000000000..7a71aa107d
> --- /dev/null
> +++ b/tests/qemuxmlconfdata/usb-controller-default-unavailable-i440fx.x86_64-latest.err
> @@ -0,0 +1 @@
> +unsupported configuration: no model provided for USB controller
The summary and first paragraph of the commit message makes it seem that
this is just moving when the validation happens but the commit is in
fact severly reworking the semantics of the validation too. Please
clarify that in the commit message; especially the summary.
The change itself makes very much sense to me at least for the 'x86'
machines, as the '-usb' reallistically won't ever be used. I'll have a
look at the possible implications for other machines, but as such I like
the direction of this patch, thus once you reword the commit message:
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
On Thu, Feb 15, 2024 at 05:01:44PM +0100, Peter Krempa wrote:
> On Wed, Feb 14, 2024 at 18:11:18 +0100, Andrea Bolognani wrote:
> > Right now we call qemuValidateDomainDeviceDefControllerUSB()
> > quite late, just as we're generating the QEMU command line.
> >
> > The intention here is to prevent configurations from being
> > rejected, even though a default USB controller model could not
> > be found, because using -usb could work as a last resort.
> >
> > As it turns out, this premise is flawed, as can easily be
> > demonstrated by using a build of QEMU which has the default
> > USB controller compiled out:
> >
> > $ qemu-system-x86_64 -M pc -device piix3-usb-uhci
> > 'piix3-usb-uhci' is not a valid device model name
> >
> > $ qemu-system-x86_64 -M pc -usb
> > missing object type 'piix3-usb-uhci'
>
> Could you please elaborate how you've got this? I had to disable the
> CONFIG_I440FX=n option to achieve that (without messing with the machine
> dependencies in the first place) which also means that that the 'pc'
> machine type will not work:
>
> 'piix3-usb-uhci' is provided by 'hw/usb/hcd-uhci.c', which is
> conditionally compiled based on the follwing rule:
>
> system_ss.add(when: 'CONFIG_USB_UHCI', if_true: files('hcd-uhci.c'))
>
> USB_UHCI is selected(required) by 'config PIIX' (hw/isa/Kconfig).
>
> PIIX is selected by 'config I440FX'.
>
> Thus it is impossible to build qemu with a 'pc' machine type but
> missing 'piix3-usb-uhci.
>
> By breaking the 'config PIIX' option you can achieve that.
>
> Thus by definition all 'pc' machine-type based devices must have the
> 'piix3-usb-uhci' type compiled in.
>
> And which thus implies that '-usb' will never be used in such case.
The exact diff is at the bottom of the message, but yeah, I've had to
hack things up quite a bit just to convince QEMU's build system to
produce a binary that contains the pc machine type but no
piix3-usb-uhci device.
To be honest I don't understand QEMU's build system very well, so
it's possible that the only reason why I had to go to such lengths is
lack of knowledge. The experience you describe above seems to suggest
otherwise though :)
> > In other words, if the device that needs to be built into QEMU
> > in order for -usb to work was available, we would have detected
> > it and used it via -device instead; if we didn't, using -usb
> > won't change anything.
>
> So if the above is the case, I'd rephrase this paragraph to say that
> qemu can't be built without the explicitly detectable controller.
>
> Now the question is only whether that applies for qemu-4.2 and other
> machines. as well. But if yes, all the -usb thing can be removed.
Note that we're discussing two slightly different claims:
* that -usb cannot work when the default USB controller has been
compiled out;
* that the default USB controller cannot be compiled out.
Either one being true across architectures and machine types is
sufficient ground for dropping -usb usage from libvirt. I'm quite
convinced that the former always holds true, and that's what I was
referring to in the commit message.
Anyway, we can just ask an actual QEMU developer to confirm.
> > +++ b/src/qemu/qemu_validate.c
> > @@ -3523,6 +3523,69 @@ qemuValidateDomainDeviceDefControllerSCSI(const virDomainControllerDef *controll
> > }
> >
> >
> > +static int
> > +qemuControllerModelUSBToCaps(int model)
>
> Any reason why 'model' doesn't use the proper type:
> virDomainControllerModelUSB and the return value type isn't virQEMUCapsFlags?
I've just moved the code exacty as is, modulo adding the
if (def->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE)
return 0;
bit without which it would clearly not work correctly.
I can add a further commit that cleans things up a bit.
> The summary and first paragraph of the commit message makes it seem that
> this is just moving when the validation happens but the commit is in
> fact severly reworking the semantics of the validation too. Please
> clarify that in the commit message; especially the summary.
Okay, I'll try to come up with something that does a better job at
explaining the changes.
diff --git a/configs/devices/i386-softmmu/default.mak
b/configs/devices/i386-softmmu/default.mak
index 598c6646df..bbf783db19 100644
--- a/configs/devices/i386-softmmu/default.mak
+++ b/configs/devices/i386-softmmu/default.mak
@@ -26,7 +26,7 @@
# Boards:
#
-CONFIG_ISAPC=y
+CONFIG_ISAPC=n
CONFIG_I440FX=y
CONFIG_Q35=y
CONFIG_MICROVM=y
diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
index 040a18c070..13cd91e587 100644
--- a/hw/isa/Kconfig
+++ b/hw/isa/Kconfig
@@ -42,7 +42,6 @@ config PIIX
select IDE_PIIX
select ISA_BUS
select MC146818RTC
- select USB_UHCI
config VT82C686
bool
@@ -51,7 +50,6 @@ config VT82C686
select ACPI_SMBUS
select SERIAL_ISA
select FDC_ISA
- select USB_UHCI
select APM
select I8254
select I8257
diff --git a/hw/usb/Kconfig b/hw/usb/Kconfig
index 0f486764ed..feb4bb5b5a 100644
--- a/hw/usb/Kconfig
+++ b/hw/usb/Kconfig
@@ -3,7 +3,7 @@ config USB
config USB_UHCI
bool
- default y if PCI_DEVICES
+ default n
depends on PCI
select USB
--
Andrea Bolognani / Red Hat / Virtualization
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
On Thu, Feb 15, 2024 at 08:46:17 -0800, Andrea Bolognani wrote:
> On Thu, Feb 15, 2024 at 05:01:44PM +0100, Peter Krempa wrote:
> > On Wed, Feb 14, 2024 at 18:11:18 +0100, Andrea Bolognani wrote:
> > > Right now we call qemuValidateDomainDeviceDefControllerUSB()
> > > quite late, just as we're generating the QEMU command line.
> > >
> > > The intention here is to prevent configurations from being
> > > rejected, even though a default USB controller model could not
> > > be found, because using -usb could work as a last resort.
> > >
> > > As it turns out, this premise is flawed, as can easily be
> > > demonstrated by using a build of QEMU which has the default
> > > USB controller compiled out:
> > >
> > > $ qemu-system-x86_64 -M pc -device piix3-usb-uhci
> > > 'piix3-usb-uhci' is not a valid device model name
> > >
> > > $ qemu-system-x86_64 -M pc -usb
> > > missing object type 'piix3-usb-uhci'
> >
> > Could you please elaborate how you've got this? I had to disable the
> > CONFIG_I440FX=n option to achieve that (without messing with the machine
> > dependencies in the first place) which also means that that the 'pc'
> > machine type will not work:
> >
> > 'piix3-usb-uhci' is provided by 'hw/usb/hcd-uhci.c', which is
> > conditionally compiled based on the follwing rule:
> >
> > system_ss.add(when: 'CONFIG_USB_UHCI', if_true: files('hcd-uhci.c'))
> >
> > USB_UHCI is selected(required) by 'config PIIX' (hw/isa/Kconfig).
> >
> > PIIX is selected by 'config I440FX'.
> >
> > Thus it is impossible to build qemu with a 'pc' machine type but
> > missing 'piix3-usb-uhci.
> >
> > By breaking the 'config PIIX' option you can achieve that.
> >
> > Thus by definition all 'pc' machine-type based devices must have the
> > 'piix3-usb-uhci' type compiled in.
> >
> > And which thus implies that '-usb' will never be used in such case.
>
> The exact diff is at the bottom of the message, but yeah, I've had to
> hack things up quite a bit just to convince QEMU's build system to
> produce a binary that contains the pc machine type but no
> piix3-usb-uhci device.
>
> To be honest I don't understand QEMU's build system very well, so
> it's possible that the only reason why I had to go to such lengths is
> lack of knowledge. The experience you describe above seems to suggest
> otherwise though :)
TBH, it wasn't exactly straightforward at first. Until I've understood
the relationship between "config USB_UHCI" and CONFIG_USB_UHCI, as it
was un-greppable before. After that it is relatively staightforward as
the hierarchy of declarations can be explored easily.
I'll investigate a bit more in terms of implications to machines. One
thing I've observed is that e.g. with q35, which doesn't require you
breaking the machine type definitions you can get a qemu without the
UHCI subsystem by setting just:
CONFIG_ISAPC=n
CONFIG_I440FX=n
CONFIG_USB_UHCI=n
Which then fails with similar error as above:
$ ./build/qemu-system-x86_64 -M q35 -usb
qemu-system-x86_64: could not find a module for type 'ich9-usb-uhci1'
I'll look a bit more deeply about other machines arches we care about to
see if we can justify this nicely.
I very much love to see that the '-usb' stuff can be deleted.
> > > In other words, if the device that needs to be built into QEMU
> > > in order for -usb to work was available, we would have detected
> > > it and used it via -device instead; if we didn't, using -usb
> > > won't change anything.
> >
> > So if the above is the case, I'd rephrase this paragraph to say that
> > qemu can't be built without the explicitly detectable controller.
> >
> > Now the question is only whether that applies for qemu-4.2 and other
> > machines. as well. But if yes, all the -usb thing can be removed.
>
> Note that we're discussing two slightly different claims:
>
> * that -usb cannot work when the default USB controller has been
> compiled out;
>
> * that the default USB controller cannot be compiled out.
>
> Either one being true across architectures and machine types is
> sufficient ground for dropping -usb usage from libvirt. I'm quite
> convinced that the former always holds true, and that's what I was
> referring to in the commit message.
IMO we should only ever consider stuff that is possible to achieve with
qemu. If it's impossible to cleanly compile out the default USB
controller that is the main point our deductions should revolve around
whether if the default USB is always compiled in the 'fallback' means
make any sense, and not the other way around.
> Anyway, we can just ask an actual QEMU developer to confirm.
>
> > > +++ b/src/qemu/qemu_validate.c
> > > @@ -3523,6 +3523,69 @@ qemuValidateDomainDeviceDefControllerSCSI(const virDomainControllerDef *controll
> > > }
> > >
> > >
> > > +static int
> > > +qemuControllerModelUSBToCaps(int model)
> >
> > Any reason why 'model' doesn't use the proper type:
> > virDomainControllerModelUSB and the return value type isn't virQEMUCapsFlags?
>
> I've just moved the code exacty as is, modulo adding the
>
> if (def->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE)
> return 0;
>
> bit without which it would clearly not work correctly.
>
> I can add a further commit that cleans things up a bit.
In a commit that clearly modifies the logic, reasoning that it's just a
code move is IMO not appropriate. If logic is being modified the code
should be brought up to standards right away, thus squash in that
cleanup.
> > The summary and first paragraph of the commit message makes it seem that
> > this is just moving when the validation happens but the commit is in
> > fact severly reworking the semantics of the validation too. Please
> > clarify that in the commit message; especially the summary.
>
> Okay, I'll try to come up with something that does a better job at
> explaining the changes.
>
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
On Fri, Feb 16, 2024 at 09:23:12AM +0100, Peter Krempa wrote: > I'll investigate a bit more in terms of implications to machines. One > thing I've observed is that e.g. with q35, which doesn't require you > breaking the machine type definitions you can get a qemu without the > UHCI subsystem by setting just: > > CONFIG_ISAPC=n > CONFIG_I440FX=n > CONFIG_USB_UHCI=n > > Which then fails with similar error as above: > > $ ./build/qemu-system-x86_64 -M q35 -usb > qemu-system-x86_64: could not find a module for type 'ich9-usb-uhci1' Interestingly, when I apply that configuration I get a slighly different behavior: $ ./build/qemu-system-x86_64 -M q35 -usb qemu-system-x86_64: unknown type 'ich9-usb-uhci1' Aborted (core dumped) You've made those changes to configs/devices/i386-softmmu/default.mak, right? I wonder why our builds don't act the same. > > Note that we're discussing two slightly different claims: > > > > 1) that -usb cannot work when the default USB controller has been > > compiled out; > > > > 2) that the default USB controller cannot be compiled out. > > > > Either one being true across architectures and machine types is > > sufficient ground for dropping -usb usage from libvirt. I'm quite > > convinced that the former always holds true, and that's what I was > > referring to in the commit message. > > IMO we should only ever consider stuff that is possible to achieve with > qemu. If it's impossible to cleanly compile out the default USB > controller that is the main point our deductions should revolve around > whether if the default USB is always compiled in the 'fallback' means > make any sense, and not the other way around. Well, just above you have demonstrated that it is indeed possible to create a QEMU binary that includes a machine type but not the device used by its default USB controller, exclusively through the standard configuration mechanisms offered by QEMU's build system. So that already tells us that claim (2) does not hold. Which is fine, because at least so far (1) still does and either one allows us to drop -usb from libvirt. > > > Any reason why 'model' doesn't use the proper type: > > > virDomainControllerModelUSB and the return value type isn't virQEMUCapsFlags? > > > > I've just moved the code exacty as is, modulo adding the > > > > if (def->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) > > return 0; > > > > bit without which it would clearly not work correctly. > > > > I can add a further commit that cleans things up a bit. > > In a commit that clearly modifies the logic, reasoning that it's just a > code move is IMO not appropriate. If logic is being modified the code > should be brought up to standards right away, thus squash in that > cleanup. Sure :) -- Andrea Bolognani / Red Hat / Virtualization _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
On Fri, Feb 16, 2024 at 02:29:50 -0800, Andrea Bolognani wrote:
> On Fri, Feb 16, 2024 at 09:23:12AM +0100, Peter Krempa wrote:
> > I'll investigate a bit more in terms of implications to machines. One
> > thing I've observed is that e.g. with q35, which doesn't require you
> > breaking the machine type definitions you can get a qemu without the
> > UHCI subsystem by setting just:
> >
> > CONFIG_ISAPC=n
> > CONFIG_I440FX=n
> > CONFIG_USB_UHCI=n
> >
> > Which then fails with similar error as above:
> >
> > $ ./build/qemu-system-x86_64 -M q35 -usb
> > qemu-system-x86_64: could not find a module for type 'ich9-usb-uhci1'
>
> Interestingly, when I apply that configuration I get a slighly
> different behavior:
>
> $ ./build/qemu-system-x86_64 -M q35 -usb
> qemu-system-x86_64: unknown type 'ich9-usb-uhci1'
> Aborted (core dumped)
>
> You've made those changes to configs/devices/i386-softmmu/default.mak,
> right? I wonder why our builds don't act the same.
You don't have 'CONFIG_MODULES' enabled and I do, see 'qdev_new'.
I went through qemu to see were the '-usb' is actually used.
'-usb' is converted to --machine TYPE,usb=on which is then accessible
either via machine_usb() function or via MachineState type's 'usb'
property.
machine_usb() is used from:
hw/arm/realview.c=static void realview_init(MachineState *machine,
hw/arm/realview.c: if (machine_usb(machine)) {
hw/arm/versatilepb.c=static void versatile_init(MachineState *machine, int board_id)
hw/arm/versatilepb.c: if (machine_usb(machine)) {
hw/i386/acpi-microvm.c=static void acpi_dsdt_add_xhci(Aml *scope, MicrovmMachineState *mms)
hw/i386/acpi-microvm.c: if (machine_usb(MACHINE(mms))) {
hw/i386/microvm.c=static void microvm_devices_init(MicrovmMachineState *mms)
hw/i386/microvm.c: if (x86_machine_is_acpi_enabled(x86ms) && machine_usb(MACHINE(mms))) {
hw/i386/pc_piix.c=static void pc_init1(MachineState *machine,
hw/i386/pc_piix.c: machine_usb(machine), &error_abort);
hw/i386/pc_q35.c=static void pc_q35_init(MachineState *machine)
hw/i386/pc_q35.c: if (machine_usb(machine)) {
hw/ppc/mac_oldworld.c=static void ppc_heathrow_init(MachineState *machine)
hw/ppc/mac_oldworld.c: if (machine_usb(machine)) {
The value is also directly read from the following places (I've renamed
it to be able to grep for it).
hw/ppc/mac_newworld.c: machine->xxusb |= defaults_enabled() && !machine->usb_disabled;
hw/ppc/mac_newworld.c: if (machine->xxusb) {
hw/ppc/spapr.c: machine->xxusb |= defaults_enabled() && !machine->usb_disabled;
hw/ppc/spapr.c: if (machine->xxusb) {
So if I didn't miss anything these are the only machines that we need to
care about with -usb.
For normal x86 the things are relatibvely simple:
- isapc doesn't support USB
- I440FX can't be compiled in without the proper default USB controller
that would be picked by libvirt
- for q35 libvirt explicitly doesn't ever use -usb
- microvm machine is weird though as it seems to use TYPE_XHCI_SYSBUS
USB controller which is MMIO?!?
Then there are machines which create the default controller via:
pci_create_simple(pci_bus, -1, "pci-ohci");
This is the case for:
hw/arm/realview.c
hw/arm/versatilepb.c
hw/ppc/mac_oldworld.c
hw/ppc/mac_newworld.c
and hw/ppc/spapr.c:
uses
pci_create_simple(phb->bus, -1, "pci-ohci");
or
pci_create_simple(phb->bus, -1, "nec-usb-xhci");
both of which our code should be able to detect and use properly at
least after some tweaking.
Currently we don't seem to ever consider 'pci-ohci' as the last resort
controller type for any of the above machines.
For arm both 'config REALVIEW' and 'config VERSATILE' require
CONFIG_USB_OHCI.
For PPC 'config MAC_NEWWORLD' implies USB_OHCI_PCI, but 'MAC_OLDWORLD
does not.
Either way we could add 'pci-ohci' as the very last resort at least for
non-x86 machines.
Thus the only open question is about the 'microvm' weird controller.
I've defined a VM as:
<domain type='kvm'>
<name>microvm</name>
<uuid>e739ac15-61b5-48c2-acc8-e7fb2b79774f</uuid>
<memory unit='KiB'>1024000</memory>
<currentMemory unit='KiB'>1024000</currentMemory>
<vcpu placement='static'>1</vcpu>
<os>
<type arch='x86_64' machine='microvm'>hvm</type>
<boot dev='hd'/>
</os>
<cpu mode='custom' match='exact' check='none'>
<model fallback='forbid'>qemu64</model>
</cpu>
<clock offset='utc'/>
<on_poweroff>destroy</on_poweroff>
<on_reboot>restart</on_reboot>
<on_crash>destroy</on_crash>
<devices>
<emulator>/home/pipo/git/qemu.git/build/x86_64-softmmu/qemu-system-x86_64</emulator>
<disk type='file' device='disk'>
<driver name='qemu' type='qcow2'/>
<source file='/var/lib/libvirt/images/alpine.qcow2'/>
<target dev='vda' bus='virtio'/>
<address type='virtio-mmio'/>
</disk>
<controller type='usb' index='0'>
<address type='virtio-mmio'/>
</controller>
<input type='keyboard' bus='ps2'/>
<input type='mouse' bus='ps2'/>
<audio id='1' type='none'/>
<memballoon model='none'/>
</devices>
</domain>
Which results into:
/home/pipo/git/qemu.git/build/x86_64-softmmu/qemu-system-x86_64 \
-name guest=microvm,debug-threads=on \
-S \
-object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/var/lib/libvirt/qemu/domain-17-microvm/master-key.aes"}' \
-machine microvm,usb=off,dump-guest-core=off,memory-backend=microvm.ram,acpi=off \
-accel kvm \
-cpu qemu64 \
-m size=1024000k \
-object '{"qom-type":"memory-backend-ram","id":"microvm.ram","size":1048576000}' \
-overcommit mem-lock=off \
-smp 1,sockets=1,cores=1,threads=1 \
-uuid e739ac15-61b5-48c2-acc8-e7fb2b79774f \
-display none \
-no-user-config \
-nodefaults \
-chardev socket,id=charmonitor,fd=40,server=on,wait=off \
-mon chardev=charmonitor,id=monitor,mode=control \
-rtc base=utc \
-no-shutdown \
-boot strict=on \
-usb \
-blockdev '{"driver":"file","filename":"/var/lib/libvirt/images/alpine.qcow2","node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \
-blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"qcow2","file":"libvirt-1-storage","backing":null}' \
-device '{"driver":"virtio-blk-device","drive":"libvirt-1-format","id":"virtio-disk0","bootindex":1}' \
-audiodev '{"id":"audio1","driver":"none"}' \
-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
-msg timestamp=on
Now as you've noted, the USB controller is not useful at all as no
devices can't be added to the implicit controller via libvirt due to the
bus name mismatch, but a definition like this is possible and starts
(for a microvm machine to do something you also need to use direct
kernel boot as the firmware alegedly doesn't support boot from block
devices).
The question now is whether we care about microvm. Specifically dropping
'-usb' will most likely cause migration incompatibility.
Or perhaps the question might be whether we care about migration
compatibility when '-usb' as last resort was used as we didnt' control
the USB controller in the first place.
I've also added some tests for the versatilepb machine to see what we
generate, those might make sense to be merged before this.
I think that the path forward for everything except 'microvm' is to add
'pci-ohci' as last resort USB controller.
If we decide that we do care about 'microvm's USB controller then we'll
need to keep adding it via the machine property (so that we can at least
drop all the legacy '-usb' stuff). At that point we should also fix the
implicit bus name.
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
On Fri, Feb 16, 2024 at 03:54:00PM +0100, Peter Krempa wrote:
> I went through qemu to see were the '-usb' is actually used.
Thanks for the excellent analysis!
> Then there are machines which create the default controller via:
>
> pci_create_simple(pci_bus, -1, "pci-ohci");
>
> This is the case for:
> hw/arm/realview.c
> hw/arm/versatilepb.c
> hw/ppc/mac_oldworld.c
> hw/ppc/mac_newworld.c
>
> and hw/ppc/spapr.c:
>
> uses
> pci_create_simple(phb->bus, -1, "pci-ohci");
> or
> pci_create_simple(phb->bus, -1, "nec-usb-xhci");
>
> both of which our code should be able to detect and use properly at
> least after some tweaking.
>
> Currently we don't seem to ever consider 'pci-ohci' as the last resort
> controller type for any of the above machines.
No tweaking necessary for pseries, I think: we already check both
before giving up and falling back to the default USB controller.
> I think that the path forward for everything except 'microvm' is to add
> 'pci-ohci' as last resort USB controller.
>
> The question now is whether we care about microvm. Specifically dropping
> '-usb' will most likely cause migration incompatibility.
>
> If we decide that we do care about 'microvm's USB controller then we'll
> need to keep adding it via the machine property (so that we can at least
> drop all the legacy '-usb' stuff). At that point we should also fix the
> implicit bus name.
I think it's completely fine to ignore any concerns about breaking
migration for microvm on account of several facts:
* we don't have formal support for it, and specifically there is
zero test coverage to ensure that it doesn't break as a
consequence of any random change;
* it's a minimal machine designed for fast-booting, minimal guests,
which makes it unlikely that anyone would want to use USB with
it;
* for the reason above, anyone interested in microvm will probably
use QEMU directly to further minimize the overhead;
* as noted, the default USB controller is unusable anyway;
* it's not a versioned machine type, which means that ABI
stability is not guaranteed at the QEMU level.
I could probably come up with a few more, but I think that would be
overkill :)
I agree with making pci-ohci a last resort fallback for the arm and
ppc machine types you've identified. I would not make the fallback
apply more widely though, as that might result in it being selected
for machine types that can't actually use it (e.g. arm machines
without PCI support). In that case, an early failure will probably be
preferable.
> I've also added some tests for the versatilepb machine to see what we
> generate, those might make sense to be merged before this.
Please post them, I'll take care of reviewing.
--
Andrea Bolognani / Red Hat / Virtualization
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
© 2016 - 2025 Red Hat, Inc.