11:11 a.m.
Changes from [v1]:
* several patches have been pushed;
* of the remaining changes, only the ones related to SCSI and USB
controllers have been retained. I still intend to pursue the
rest, but those two are where the real nasty stuff happens, so
I'm focusing on them only for now;
* improve the handling of USB controllers on s390x;
* make all the code dealing with the legacy USB controller obsolete
and get rid of it;
* use out arguments to return models, making the new helpers fall
in line with the usual libvirt API conventions.
[v1]
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/G5...
Andrea Bolognani (17):
tests: Add controller-scsi-default-unavailable
qemu: Rename qemuDomainDefaultSCSIControllerModel()
qemu: Move error reporting out of
qemuDomainDefaultSCSIControllerModel()
qemu: Improve qemuDomainDefaultSCSIControllerModel()
qemu: Clean up qemuDomainDefaultSCSIControllerModel()
qemu: Use virtio-scsi by default on RISC-V
tests: Add usb-legacy-multiple
tests: Add usb-legacy-device
qemu: Always default to no USB controller on s390x
qemu: Only use legacy USB controller if actually needed
qemu: Validate USB controllers earlier
qemu: Improve error message for USB controller validation
qemu: Drop all code dealing with the legacy USB controller
qemu: Add qemuDomainDefaultUSBControllerModel()
qemu: Extend qemuDomainDefaultUSBControllerModel()
qemu: Clean up qemuDomainDefaultUSBControllerModel()
qemu: Use qemu-xhci by default on RISC-V
src/qemu/qemu_command.c | 145 +--------
src/qemu/qemu_domain.c | 300 +++++++++++++-----
src/qemu/qemu_domain.h | 6 +-
src/qemu/qemu_hotplug.c | 21 +-
src/qemu/qemu_validate.c | 69 +++-
.../qemuhotplug-base-ccw-live+ccw-virtio.xml | 5 +-
...with-2-ccw-virtio+ccw-virtio-1-reverse.xml | 5 +-
...otplug-base-ccw-live-with-2-ccw-virtio.xml | 5 +-
...-with-ccw-virtio+ccw-virtio-2-explicit.xml | 5 +-
...-ccw-live-with-ccw-virtio+ccw-virtio-2.xml | 5 +-
...uhotplug-base-ccw-live-with-ccw-virtio.xml | 5 +-
.../qemuhotplug-base-ccw-live.xml | 5 +-
.../arm-vexpressa9-basic.aarch64-latest.args | 1 -
.../arm-vexpressa9-nodevs.aarch64-latest.args | 1 -
.../arm-vexpressa9-virtio.aarch64-latest.args | 1 -
...scsi-default-unavailable.x86_64-latest.err | 1 +
.../controller-scsi-default-unavailable.xml | 15 +
.../disk-arm-virtio-sd.aarch64-latest.args | 1 -
...ault-models.riscv64-latest.abi-update.args | 12 +-
...fault-models.riscv64-latest.abi-update.xml | 25 +-
...64-virt-default-models.riscv64-latest.args | 12 +-
...v64-virt-default-models.riscv64-latest.xml | 25 +-
.../s390-usb-address.s390x-latest.xml | 6 +-
.../sparc-minimal.sparc-latest.args | 1 -
...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.err | 2 +-
...-default-unavailable-q35.x86_64-latest.xml | 33 --
...ntroller-implicit-isapc.x86_64-latest.args | 1 -
...ler-nec-xhci-unavailable.x86_64-latest.xml | 33 --
.../usb-legacy-device.x86_64-latest.err | 1 +
tests/qemuxmlconfdata/usb-legacy-device.xml | 15 +
.../usb-legacy-multiple.x86_64-latest.err | 1 +
tests/qemuxmlconfdata/usb-legacy-multiple.xml | 15 +
tests/qemuxmlconftest.c | 28 +-
36 files changed, 418 insertions(+), 453 deletions(-)
create mode 100644
tests/qemuxmlconfdata/controller-scsi-default-unavailable.x86_64-latest.err
create mode 100644 tests/qemuxmlconfdata/controller-scsi-default-unavailable.xml
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
create mode 100644 tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.err
create mode 100644 tests/qemuxmlconfdata/usb-legacy-device.xml
create mode 100644 tests/qemuxmlconfdata/usb-legacy-multiple.x86_64-latest.err
create mode 100644 tests/qemuxmlconfdata/usb-legacy-multiple.xml
--
2.43.0
11:11 a.m.
New subject: [PATCH v2 01/17] tests: Add controller-scsi-default-unavailable
This provides coverage for the (very unlikely) scenario in
which none of the possible devices are built into QEMU.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
...ler-scsi-default-unavailable.x86_64-latest.err | 1 +
.../controller-scsi-default-unavailable.xml | 15 +++++++++++++++
tests/qemuxmlconftest.c | 6 ++++++
3 files changed, 22 insertions(+)
create mode 100644
tests/qemuxmlconfdata/controller-scsi-default-unavailable.x86_64-latest.err
create mode 100644 tests/qemuxmlconfdata/controller-scsi-default-unavailable.xml
diff --git a/tests/qemuxmlconfdata/controller-scsi-default-unavailable.x86_64-latest.err
b/tests/qemuxmlconfdata/controller-scsi-default-unavailable.x86_64-latest.err
new file mode 100644
index 0000000000..3648abd871
--- /dev/null
+++ b/tests/qemuxmlconfdata/controller-scsi-default-unavailable.x86_64-latest.err
@@ -0,0 +1 @@
+internal error: Unable to determine model for SCSI controller idx=0
diff --git a/tests/qemuxmlconfdata/controller-scsi-default-unavailable.xml
b/tests/qemuxmlconfdata/controller-scsi-default-unavailable.xml
new file mode 100644
index 0000000000..e438dcf5bc
--- /dev/null
+++ b/tests/qemuxmlconfdata/controller-scsi-default-unavailable.xml
@@ -0,0 +1,15 @@
+<domain type='qemu'>
+ <name>guest</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219136</memory>
+ <vcpu placement='static'>8</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc'>hvm</type>
+ </os>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ <controller type='scsi'/>
+ <controller type='usb' model='none'/>
+ <memballoon model='none'/>
+ </devices>
+</domain>
diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c
index e1ee1fbce3..c43f4cab67 100644
--- a/tests/qemuxmlconftest.c
+++ b/tests/qemuxmlconftest.c
@@ -1587,6 +1587,12 @@ mymain(void)
DO_TEST_CAPS_LATEST_PARSE_ERROR("disk-scsi-disk-vpd-build-error");
DO_TEST_CAPS_LATEST("controller-virtio-scsi");
DO_TEST_CAPS_LATEST("controller-scsi-auto");
+ DO_TEST_FULL("controller-scsi-default-unavailable",
".x86_64-latest",
+ ARG_CAPS_ARCH, "x86_64",
+ ARG_CAPS_VER, "latest",
+ ARG_FLAGS, FLAG_EXPECT_PARSE_ERROR,
+ ARG_QEMU_CAPS_DEL, QEMU_CAPS_SCSI_LSI, QEMU_CAPS_VIRTIO_SCSI,
QEMU_CAPS_LAST,
+ ARG_END);
DO_TEST_CAPS_LATEST("disk-sata-device");
DO_TEST_CAPS_LATEST("disk-aio");
DO_TEST_CAPS_LATEST("disk-aio-io_uring");
--
2.43.0
3:21 a.m.
New subject: [PATCH v2 01/17] tests: Add controller-scsi-default-unavailable
On Wed, Feb 14, 2024 at 18:11:08 +0100, Andrea Bolognani wrote:
This provides coverage for the (very unlikely) scenario in
which none of the possible devices are built into QEMU.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
...ler-scsi-default-unavailable.x86_64-latest.err | 1 +
.../controller-scsi-default-unavailable.xml | 15 +++++++++++++++
tests/qemuxmlconftest.c | 6 ++++++
3 files changed, 22 insertions(+)
create mode 100644
tests/qemuxmlconfdata/controller-scsi-default-unavailable.x86_64-latest.err
create mode 100644 tests/qemuxmlconfdata/controller-scsi-default-unavailable.xml
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
11:11 a.m.
New subject: [PATCH v2 02/17] qemu: Rename qemuDomainDefaultSCSIControllerModel()
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_domain.c | 8 ++++----
src/qemu/qemu_domain.h | 6 +++---
src/qemu/qemu_hotplug.c | 2 +-
3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index d7be544710..01f4ed6917 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4107,9 +4107,9 @@ qemuDomainDefAddDefaultAudioBackend(virQEMUDriver *driver,
* Returns model on success, -1 on failure with error set.
*/
int
-qemuDomainGetSCSIControllerModel(const virDomainDef *def,
- const virDomainControllerDef *cont,
- virQEMUCaps *qemuCaps)
+qemuDomainDefaultSCSIControllerModel(const virDomainDef *def,
+ const virDomainControllerDef *cont,
+ virQEMUCaps *qemuCaps)
{
if (cont->model > 0)
return cont->model;
@@ -5614,7 +5614,7 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont,
switch (cont->type) {
case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
/* Set the default SCSI controller model if not already set */
- cont->model = qemuDomainGetSCSIControllerModel(def, cont, qemuCaps);
+ cont->model = qemuDomainDefaultSCSIControllerModel(def, cont, qemuCaps);
if (cont->model < 0)
return -1;
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 98c188cc8f..f3aad2ef5c 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -838,9 +838,9 @@ bool qemuDomainHasBuiltinESP(const virDomainDef *def);
bool qemuDomainNeedsFDC(const virDomainDef *def);
bool qemuDomainSupportsPCI(const virDomainDef *def);
bool qemuDomainSupportsPCIMultibus(const virDomainDef *def);
-int qemuDomainGetSCSIControllerModel(const virDomainDef *def,
- const virDomainControllerDef *cont,
- virQEMUCaps *qemuCaps);
+int qemuDomainDefaultSCSIControllerModel(const virDomainDef *def,
+ const virDomainControllerDef *cont,
+ virQEMUCaps *qemuCaps);
void qemuDomainUpdateCurrentMemorySize(virDomainObj *vm);
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index b9c502613c..a2498e793a 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -878,7 +878,7 @@ qemuDomainFindOrCreateSCSIDiskController(virDomainObj *vm,
cont->idx = controller;
if (model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT)
- cont->model = qemuDomainGetSCSIControllerModel(vm->def, cont,
priv->qemuCaps);
+ cont->model = qemuDomainDefaultSCSIControllerModel(vm->def, cont,
priv->qemuCaps);
else
cont->model = model;
--
2.43.0
3:22 a.m.
New subject: [PATCH v2 02/17] qemu: Rename
qemuDomainDefaultSCSIControllerModel()
On Wed, Feb 14, 2024 at 18:11:09 +0100, Andrea Bolognani wrote:
Please mention the old name at least in the commit message if you don't
want to make the summary too long.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_domain.c | 8 ++++----
src/qemu/qemu_domain.h | 6 +++---
src/qemu/qemu_hotplug.c | 2 +-
3 files changed, 8 insertions(+), 8 deletions(-)
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
11:11 a.m.
New subject: [PATCH v2 03/17] qemu: Move error reporting out of qemuDomainDefaultSCSIControllerModel()
We want this helper to work more like other similar ones, where
error reporting is performed by the caller. This introduces a
small amount of code duplication but makes for a cleaner API.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_domain.c | 9 +++++----
src/qemu/qemu_hotplug.c | 3 +++
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 01f4ed6917..22e6b9fd3e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4125,9 +4125,6 @@ qemuDomainDefaultSCSIControllerModel(const virDomainDef *def,
if (qemuDomainHasBuiltinESP(def))
return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_NCR53C90;
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unable to determine model for SCSI controller
idx=%1$d"),
- cont->idx);
return -1;
}
@@ -5616,8 +5613,12 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont,
/* Set the default SCSI controller model if not already set */
cont->model = qemuDomainDefaultSCSIControllerModel(def, cont, qemuCaps);
- if (cont->model < 0)
+ if (cont->model < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unable to determine model for SCSI controller
idx=%1$d"),
+ cont->idx);
return -1;
+ }
break;
case VIR_DOMAIN_CONTROLLER_TYPE_USB:
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index a2498e793a..a9dbc37915 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -883,6 +883,9 @@ qemuDomainFindOrCreateSCSIDiskController(virDomainObj *vm,
cont->model = model;
if (cont->model < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unable to determine model for SCSI controller
idx=%1$d"),
+ cont->idx);
VIR_FREE(cont);
return NULL;
}
--
2.43.0
6:41 a.m.
New subject: [PATCH v2 03/17] qemu: Move error reporting out of
qemuDomainDefaultSCSIControllerModel()
On Wed, Feb 14, 2024 at 18:11:10 +0100, Andrea Bolognani wrote:
We want this helper to work more like other similar ones, where
error reporting is performed by the caller. This introduces a
small amount of code duplication but makes for a cleaner API.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_domain.c | 9 +++++----
src/qemu/qemu_hotplug.c | 3 +++
2 files changed, 8 insertions(+), 4 deletions(-)
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
11:11 a.m.
New subject: [PATCH v2 04/17] qemu: Improve qemuDomainDefaultSCSIControllerModel()
Make the helper stateless. This requires the caller to check
whether it needs to be called in the first place instead of
adding this check inside the function, which makes for more
readable, if a little more verbose, code.
At the same time, change it so that it uses an out argument to
return the selected model back to the caller, and only use the
return value to signal whether the function completed its task
successfully. This is consistent with most APIs in libvirt, and
removes the ambiguity caused by the fact that the value of
VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT is -1.
In order to have a nice API while still keeping the
implementation short and readable, a small wrapper is required.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_domain.c | 51 ++++++++++++++++++++++++-----------------
src/qemu/qemu_domain.h | 4 ++--
src/qemu/qemu_hotplug.c | 18 ++++++++-------
3 files changed, 42 insertions(+), 31 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 22e6b9fd3e..ab2cc427a2 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4096,6 +4096,26 @@ qemuDomainDefAddDefaultAudioBackend(virQEMUDriver *driver,
}
+/* Internal. Use qemuDomainDefaultSCSIControllerModel() instead */
+static virDomainControllerModelSCSI
+qemuDomainDefaultSCSIControllerModelInternal(const virDomainDef *def,
+ virQEMUCaps *qemuCaps)
+{
+ if (qemuDomainIsPSeries(def))
+ return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI;
+ if (ARCH_IS_S390(def->os.arch))
+ return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI;
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_LSI))
+ return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC;
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_SCSI))
+ return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI;
+ if (qemuDomainHasBuiltinESP(def))
+ return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_NCR53C90;
+
+ return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT;
+}
+
+
/**
* @def: Domain definition
* @cont: Domain controller def
@@ -4107,25 +4127,12 @@ qemuDomainDefAddDefaultAudioBackend(virQEMUDriver *driver,
* Returns model on success, -1 on failure with error set.
*/
int
-qemuDomainDefaultSCSIControllerModel(const virDomainDef *def,
- const virDomainControllerDef *cont,
+qemuDomainDefaultSCSIControllerModel(virDomainControllerModelSCSI *model,
+ const virDomainDef *def,
virQEMUCaps *qemuCaps)
{
- if (cont->model > 0)
- return cont->model;
-
- if (qemuDomainIsPSeries(def))
- return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI;
- if (ARCH_IS_S390(def->os.arch))
- return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI;
- if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_LSI))
- return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC;
- if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_SCSI))
- return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI;
- if (qemuDomainHasBuiltinESP(def))
- return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_NCR53C90;
-
- return -1;
+ *model = qemuDomainDefaultSCSIControllerModelInternal(def, qemuCaps);
+ return 0;
}
@@ -5610,10 +5617,12 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont,
{
switch (cont->type) {
case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
- /* Set the default SCSI controller model if not already set */
- cont->model = qemuDomainDefaultSCSIControllerModel(def, cont, qemuCaps);
-
- if (cont->model < 0) {
+ /* If no model is set, try to come up with a reasonable
+ * default. If one can't be determined, error out */
+ if ((cont->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT ||
+ cont->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO) &&
+ (qemuDomainDefaultSCSIControllerModel(&cont->model, def, qemuCaps)
< 0 ||
+ cont->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unable to determine model for SCSI controller
idx=%1$d"),
cont->idx);
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index f3aad2ef5c..dae817d655 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -838,8 +838,8 @@ bool qemuDomainHasBuiltinESP(const virDomainDef *def);
bool qemuDomainNeedsFDC(const virDomainDef *def);
bool qemuDomainSupportsPCI(const virDomainDef *def);
bool qemuDomainSupportsPCIMultibus(const virDomainDef *def);
-int qemuDomainDefaultSCSIControllerModel(const virDomainDef *def,
- const virDomainControllerDef *cont,
+int qemuDomainDefaultSCSIControllerModel(virDomainControllerModelSCSI *model,
+ const virDomainDef *def,
virQEMUCaps *qemuCaps);
void qemuDomainUpdateCurrentMemorySize(virDomainObj *vm);
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index a9dbc37915..b96c706804 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -850,7 +850,7 @@ qemuDomainFindOrCreateSCSIDiskController(virDomainObj *vm,
size_t i;
virDomainControllerDef *cont;
qemuDomainObjPrivate *priv = vm->privateData;
- int model = -1;
+ virDomainControllerModelSCSI model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT;
for (i = 0; i < vm->def->ncontrollers; i++) {
cont = vm->def->controllers[i];
@@ -876,13 +876,15 @@ qemuDomainFindOrCreateSCSIDiskController(virDomainObj *vm,
* now hotplug a controller */
cont = virDomainControllerDefNew(VIR_DOMAIN_CONTROLLER_TYPE_SCSI);
cont->idx = controller;
-
- if (model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT)
- cont->model = qemuDomainDefaultSCSIControllerModel(vm->def, cont,
priv->qemuCaps);
- else
- cont->model = model;
-
- if (cont->model < 0) {
+ cont->model = model;
+
+ /* If no model has been discovered by looking at existing SCSI
+ * controllers, try to come up with a reasonable default. If one
+ * can't be determined, error out */
+ if ((cont->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT ||
+ cont->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO) &&
+ (qemuDomainDefaultSCSIControllerModel(&cont->model, vm->def,
priv->qemuCaps) < 0 ||
+ cont->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unable to determine model for SCSI controller
idx=%1$d"),
cont->idx);
--
2.43.0
6:57 a.m.
New subject: [PATCH v2 04/17] qemu: Improve
qemuDomainDefaultSCSIControllerModel()
On Wed, Feb 14, 2024 at 18:11:11 +0100, Andrea Bolognani wrote:
Make the helper stateless. This requires the caller to check
whether it needs to be called in the first place instead of
adding this check inside the function, which makes for more
readable, if a little more verbose, code.
At the same time, change it so that it uses an out argument to
return the selected model back to the caller, and only use the
return value to signal whether the function completed its task
successfully. This is consistent with most APIs in libvirt, and
removes the ambiguity caused by the fact that the value of
VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT is -1.
In order to have a nice API while still keeping the
implementation short and readable, a small wrapper is required.
Well arguably the wrapper is not needed unless you really want to use:
return VIR_DOMAIN_CONTROLLER_MODEL_SCS*;
rather than:
*model = VIR_DOMAIN_CONTROLLER_MODEL_SCS*;
return 0;
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_domain.c | 51 ++++++++++++++++++++++++-----------------
src/qemu/qemu_domain.h | 4 ++--
src/qemu/qemu_hotplug.c | 18 ++++++++-------
3 files changed, 42 insertions(+), 31 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 22e6b9fd3e..ab2cc427a2 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4096,6 +4096,26 @@ qemuDomainDefAddDefaultAudioBackend(virQEMUDriver *driver,
}
+/* Internal. Use qemuDomainDefaultSCSIControllerModel() instead */
+static virDomainControllerModelSCSI
+qemuDomainDefaultSCSIControllerModelInternal(const virDomainDef *def,
+ virQEMUCaps *qemuCaps)
+{
+ if (qemuDomainIsPSeries(def))
+ return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI;
+ if (ARCH_IS_S390(def->os.arch))
+ return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI;
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_LSI))
+ return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC;
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_SCSI))
+ return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI;
+ if (qemuDomainHasBuiltinESP(def))
+ return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_NCR53C90;
+
+ return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT;
+}
+
+
/**
* @def: Domain definition
* @cont: Domain controller def
@@ -4107,25 +4127,12 @@ qemuDomainDefAddDefaultAudioBackend(virQEMUDriver *driver,
* Returns model on success, -1 on failure with error set.
So now this always returns 0 -> success, thus the return value is
pointless. Further up the next patch which modifies the documentation
still keeps the claim. Additionally the description will read:
Not being able to come up with a value is NOT considered a failure.
which will mean that this function will never fail.
IMO it would be sufficient to just declare:
virDomainControllerModelSCSI
qemuDomainDefaultSCSIControllerModel(const virDomainDef *def,
const virDomainControllerDef *cont,
...
and just document the same claims. Thus if _DEFAULT is returned it's up
to the caller to decide.
Fabricating a return value doesn't seem to make much sense for me and
once you only return success you might as well as return the value
directly. It'd simplify the checker conditions.
7:46 a.m.
New subject: [PATCH v2 04/17] qemu: Improve qemuDomainDefaultSCSIControllerModel()
On Thu, Feb 15, 2024 at 01:57:27PM +0100, Peter Krempa wrote:
On Wed, Feb 14, 2024 at 18:11:11 +0100, Andrea Bolognani wrote:
> In order to have a nice API while still keeping the
> implementation short and readable, a small wrapper is required.
Well arguably the wrapper is not needed unless you really want to use:
return VIR_DOMAIN_CONTROLLER_MODEL_SCS*;
rather than:
*model = VIR_DOMAIN_CONTROLLER_MODEL_SCS*;
return 0;
I really do want to be able to use the former style. It doesn't make
as big an impact here, but for the USB part the difference is quite
stark.
So now this always returns 0 -> success, thus the return value is
pointless. Further up the next patch which modifies the documentation
still keeps the claim. Additionally the description will read:
Not being able to come up with a value is NOT considered a failure.
which will mean that this function will never fail.
IMO it would be sufficient to just declare:
virDomainControllerModelSCSI
qemuDomainDefaultSCSIControllerModel(const virDomainDef *def,
const virDomainControllerDef *cont,
...
and just document the same claims. Thus if _DEFAULT is returned it's up
to the caller to decide.
Fabricating a return value doesn't seem to make much sense for me and
once you only return success you might as well as return the value
directly. It'd simplify the checker conditions.
You're right, I can absolutely get rid of the wrapper.
Failure states that needed to be reported actually existed at some
point, but those went away as I reworked the code, and I never went
back to reconsider the API.
Thanks a lot for the suggestion, and consider it done :)
--
Andrea Bolognani / Red Hat / Virtualization
8:16 a.m.
New subject: [PATCH v2 04/17] qemu: Improve
qemuDomainDefaultSCSIControllerModel()
On Thu, Feb 15, 2024 at 05:46:28 -0800, Andrea Bolognani wrote:
On Thu, Feb 15, 2024 at 01:57:27PM +0100, Peter Krempa wrote:
> On Wed, Feb 14, 2024 at 18:11:11 +0100, Andrea Bolognani wrote:
> > In order to have a nice API while still keeping the
> > implementation short and readable, a small wrapper is required.
>
> Well arguably the wrapper is not needed unless you really want to use:
>
> return VIR_DOMAIN_CONTROLLER_MODEL_SCS*;
>
> rather than:
>
> *model = VIR_DOMAIN_CONTROLLER_MODEL_SCS*;
> return 0;
I really do want to be able to use the former style. It doesn't make
as big an impact here, but for the USB part the difference is quite
stark.
> So now this always returns 0 -> success, thus the return value is
> pointless. Further up the next patch which modifies the documentation
> still keeps the claim. Additionally the description will read:
>
> Not being able to come up with a value is NOT considered a failure.
>
> which will mean that this function will never fail.
>
> IMO it would be sufficient to just declare:
>
> virDomainControllerModelSCSI
> qemuDomainDefaultSCSIControllerModel(const virDomainDef *def,
> const virDomainControllerDef *cont,
> ...
>
> and just document the same claims. Thus if _DEFAULT is returned it's up
> to the caller to decide.
>
> Fabricating a return value doesn't seem to make much sense for me and
> once you only return success you might as well as return the value
> directly. It'd simplify the checker conditions.
You're right, I can absolutely get rid of the wrapper.
Failure states that needed to be reported actually existed at some
point, but those went away as I reworked the code, and I never went
back to reconsider the API.
Thanks a lot for the suggestion, and consider it done :)
You can use:
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
11:11 a.m.
New subject: [PATCH v2 05/17] qemu: Clean up qemuDomainDefaultSCSIControllerModel()
Use a better order for sections, improve comments.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_domain.c | 39 +++++++++++++++++++++++++++++----------
1 file changed, 29 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ab2cc427a2..f4af8fb6ba 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4101,30 +4101,49 @@ static virDomainControllerModelSCSI
qemuDomainDefaultSCSIControllerModelInternal(const virDomainDef *def,
virQEMUCaps *qemuCaps)
{
- if (qemuDomainIsPSeries(def))
- return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI;
+ /* For machine types with built-in SCSI controllers, the choice
+ * of model is obvious */
+ if (qemuDomainHasBuiltinESP(def))
+ return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_NCR53C90;
+
+ /* Most new architectures should ideally use virtio */
if (ARCH_IS_S390(def->os.arch))
return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI;
+
+ /* pSeries has its own special default */
+ if (qemuDomainIsPSeries(def))
+ return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI;
+
+ /* If there is no preference, base the choice on device
+ * availability. In this case, lsilogic is favored over
+ * virtio-scsi for backwards compatibility reasons */
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_LSI))
return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC;
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_SCSI))
return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI;
- if (qemuDomainHasBuiltinESP(def))
- return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_NCR53C90;
return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT;
}
/**
- * @def: Domain definition
- * @cont: Domain controller def
- * @qemuCaps: qemu capabilities
+ * qemuDomainDefaultSCSIControllerModel:
+ * @model: return location
+ * @def: domain definition
+ * @qemuCaps: QEMU capabilities, or NULL
+ *
+ * Choose a reasonable model to use for a SCSI controller where a
+ * specific one hasn't been provided by the user.
+ *
+ * The choice is based on a number of factors, including the guest's
+ * architecture and machine type. @qemuCaps, if provided, might be
+ * taken into consideration too.
*
- * If the controller model is already defined, return it immediately;
- * otherwise, based on the @qemuCaps return a default model value.
+ * Not being able to come up with a value is NOT considered a
+ * failure. It's up to the caller to decide how to handle that
+ * scenario.
*
- * Returns model on success, -1 on failure with error set.
+ * Returns: 0 on success, <0 on failure.
*/
int
qemuDomainDefaultSCSIControllerModel(virDomainControllerModelSCSI *model,
--
2.43.0
7 a.m.
New subject: [PATCH v2 05/17] qemu: Clean up
qemuDomainDefaultSCSIControllerModel()
On Wed, Feb 14, 2024 at 18:11:12 +0100, Andrea Bolognani wrote:
Use a better order for sections, improve comments.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_domain.c | 39 +++++++++++++++++++++++++++++----------
1 file changed, 29 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ab2cc427a2..f4af8fb6ba 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4101,30 +4101,49 @@ static virDomainControllerModelSCSI
qemuDomainDefaultSCSIControllerModelInternal(const virDomainDef *def,
virQEMUCaps *qemuCaps)
{
- if (qemuDomainIsPSeries(def))
- return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI;
+ /* For machine types with built-in SCSI controllers, the choice
+ * of model is obvious */
+ if (qemuDomainHasBuiltinESP(def))
+ return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_NCR53C90;
+
+ /* Most new architectures should ideally use virtio */
if (ARCH_IS_S390(def->os.arch))
return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI;
+
+ /* pSeries has its own special default */
+ if (qemuDomainIsPSeries(def))
+ return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI;
+
+ /* If there is no preference, base the choice on device
+ * availability. In this case, lsilogic is favored over
+ * virtio-scsi for backwards compatibility reasons */
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_LSI))
return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC;
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_SCSI))
return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI;
- if (qemuDomainHasBuiltinESP(def))
- return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_NCR53C90;
return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT;
}
/**
- * @def: Domain definition
- * @cont: Domain controller def
- * @qemuCaps: qemu capabilities
+ * qemuDomainDefaultSCSIControllerModel:
+ * @model: return location
+ * @def: domain definition
+ * @qemuCaps: QEMU capabilities, or NULL
+ *
+ * Choose a reasonable model to use for a SCSI controller where a
+ * specific one hasn't been provided by the user.
+ *
+ * The choice is based on a number of factors, including the guest's
+ * architecture and machine type. @qemuCaps, if provided, might be
+ * taken into consideration too.
*
- * If the controller model is already defined, return it immediately;
- * otherwise, based on the @qemuCaps return a default model value.
+ * Not being able to come up with a value is NOT considered a
+ * failure. It's up to the caller to decide how to handle that
+ * scenario.
*
- * Returns model on success, -1 on failure with error set.
+ * Returns: 0 on success, <0 on failure.
As noted before, the return value here has no useful value, and you
might as well as directly return the model.
*/
int
qemuDomainDefaultSCSIControllerModel(virDomainControllerModelSCSI *model,
--
2.43.0
_______________________________________________
Devel mailing list -- devel(a)lists.libvirt.org
To unsubscribe send an email to devel-leave(a)lists.libvirt.org
11:11 a.m.
New subject: [PATCH v2 06/17] qemu: Use virtio-scsi by default on RISC-V
Just like piix3-uhci for USB, the choice of lsilogic for SCSI
was dictated entirely by that being the default for legacy x86
guests. Use virtio-scsi instead.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_domain.c | 4 +++-
...virt-default-models.riscv64-latest.abi-update.args | 5 +++--
...-virt-default-models.riscv64-latest.abi-update.xml | 11 ++++++++---
.../riscv64-virt-default-models.riscv64-latest.args | 5 +++--
.../riscv64-virt-default-models.riscv64-latest.xml | 11 ++++++++---
5 files changed, 25 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index f4af8fb6ba..31d634b151 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4107,8 +4107,10 @@ qemuDomainDefaultSCSIControllerModelInternal(const virDomainDef
*def,
return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_NCR53C90;
/* Most new architectures should ideally use virtio */
- if (ARCH_IS_S390(def->os.arch))
+ if (ARCH_IS_S390(def->os.arch) ||
+ qemuDomainIsRISCVVirt(def)) {
return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI;
+ }
/* pSeries has its own special default */
if (qemuDomainIsPSeries(def))
diff --git
a/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.abi-update.args
b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.abi-update.args
index 28b56d876c..7990f7f6a0 100644
--- a/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.abi-update.args
+++ b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.abi-update.args
@@ -30,13 +30,14 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-guest/.config \
-device
'{"driver":"pcie-pci-bridge","id":"pci.3","bus":"pci.1","addr":"0x0"}'
\
-device
'{"driver":"pcie-root-port","port":10,"chassis":4,"id":"pci.4","bus":"pcie.0","addr":"0x1.0x2"}'
\
-device
'{"driver":"pcie-root-port","port":11,"chassis":5,"id":"pci.5","bus":"pcie.0","addr":"0x1.0x3"}'
\
+-device
'{"driver":"pcie-root-port","port":12,"chassis":6,"id":"pci.6","bus":"pcie.0","addr":"0x1.0x4"}'
\
-device
'{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.3","addr":"0x1"}'
\
--device
'{"driver":"lsi","id":"scsi0","bus":"pci.3","addr":"0x2"}'
\
+-device
'{"driver":"virtio-scsi-pci","id":"scsi0","bus":"pci.4","addr":"0x0"}'
\
-netdev '{"type":"user","id":"hostnet0"}'
\
-device
'{"driver":"virtio-net-pci","netdev":"hostnet0","id":"net0","mac":"52:54:00:09:a4:37","bus":"pci.2","addr":"0x0"}'
\
-chardev pty,id=charserial0 \
-serial chardev:charserial0 \
-audiodev
'{"id":"audio1","driver":"none"}' \
--device
'{"driver":"virtio-vga","id":"video0","max_outputs":1,"bus":"pci.4","addr":"0x0"}'
\
+-device
'{"driver":"virtio-vga","id":"video0","max_outputs":1,"bus":"pci.5","addr":"0x0"}'
\
-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
-msg timestamp=on
diff --git
a/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.abi-update.xml
b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.abi-update.xml
index 942bd21f9e..72dc447d33 100644
--- a/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.abi-update.xml
+++ b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.abi-update.xml
@@ -17,8 +17,8 @@
<controller type='usb' index='0' model='piix3-uhci'>
<address type='pci' domain='0x0000' bus='0x03'
slot='0x01' function='0x0'/>
</controller>
- <controller type='scsi' index='0' model='lsilogic'>
- <address type='pci' domain='0x0000' bus='0x03'
slot='0x02' function='0x0'/>
+ <controller type='scsi' index='0' model='virtio-scsi'>
+ <address type='pci' domain='0x0000' bus='0x04'
slot='0x00' function='0x0'/>
</controller>
<controller type='pci' index='0' model='pcie-root'/>
<controller type='pci' index='1'
model='pcie-root-port'>
@@ -45,6 +45,11 @@
<target chassis='5' port='0xb'/>
<address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x3'/>
</controller>
+ <controller type='pci' index='6'
model='pcie-root-port'>
+ <model name='pcie-root-port'/>
+ <target chassis='6' port='0xc'/>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x4'/>
+ </controller>
<interface type='user'>
<mac address='52:54:00:09:a4:37'/>
<model type='virtio'/>
@@ -61,7 +66,7 @@
<audio id='1' type='none'/>
<video>
<model type='virtio' heads='1' primary='yes'/>
- <address type='pci' domain='0x0000' bus='0x04'
slot='0x00' function='0x0'/>
+ <address type='pci' domain='0x0000' bus='0x05'
slot='0x00' function='0x0'/>
</video>
<memballoon model='none'/>
</devices>
diff --git a/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.args
b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.args
index 28b56d876c..7990f7f6a0 100644
--- a/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.args
+++ b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.args
@@ -30,13 +30,14 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-guest/.config \
-device
'{"driver":"pcie-pci-bridge","id":"pci.3","bus":"pci.1","addr":"0x0"}'
\
-device
'{"driver":"pcie-root-port","port":10,"chassis":4,"id":"pci.4","bus":"pcie.0","addr":"0x1.0x2"}'
\
-device
'{"driver":"pcie-root-port","port":11,"chassis":5,"id":"pci.5","bus":"pcie.0","addr":"0x1.0x3"}'
\
+-device
'{"driver":"pcie-root-port","port":12,"chassis":6,"id":"pci.6","bus":"pcie.0","addr":"0x1.0x4"}'
\
-device
'{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.3","addr":"0x1"}'
\
--device
'{"driver":"lsi","id":"scsi0","bus":"pci.3","addr":"0x2"}'
\
+-device
'{"driver":"virtio-scsi-pci","id":"scsi0","bus":"pci.4","addr":"0x0"}'
\
-netdev '{"type":"user","id":"hostnet0"}'
\
-device
'{"driver":"virtio-net-pci","netdev":"hostnet0","id":"net0","mac":"52:54:00:09:a4:37","bus":"pci.2","addr":"0x0"}'
\
-chardev pty,id=charserial0 \
-serial chardev:charserial0 \
-audiodev
'{"id":"audio1","driver":"none"}' \
--device
'{"driver":"virtio-vga","id":"video0","max_outputs":1,"bus":"pci.4","addr":"0x0"}'
\
+-device
'{"driver":"virtio-vga","id":"video0","max_outputs":1,"bus":"pci.5","addr":"0x0"}'
\
-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
-msg timestamp=on
diff --git a/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.xml
b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.xml
index 942bd21f9e..72dc447d33 100644
--- a/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.xml
+++ b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.xml
@@ -17,8 +17,8 @@
<controller type='usb' index='0' model='piix3-uhci'>
<address type='pci' domain='0x0000' bus='0x03'
slot='0x01' function='0x0'/>
</controller>
- <controller type='scsi' index='0' model='lsilogic'>
- <address type='pci' domain='0x0000' bus='0x03'
slot='0x02' function='0x0'/>
+ <controller type='scsi' index='0' model='virtio-scsi'>
+ <address type='pci' domain='0x0000' bus='0x04'
slot='0x00' function='0x0'/>
</controller>
<controller type='pci' index='0' model='pcie-root'/>
<controller type='pci' index='1'
model='pcie-root-port'>
@@ -45,6 +45,11 @@
<target chassis='5' port='0xb'/>
<address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x3'/>
</controller>
+ <controller type='pci' index='6'
model='pcie-root-port'>
+ <model name='pcie-root-port'/>
+ <target chassis='6' port='0xc'/>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x4'/>
+ </controller>
<interface type='user'>
<mac address='52:54:00:09:a4:37'/>
<model type='virtio'/>
@@ -61,7 +66,7 @@
<audio id='1' type='none'/>
<video>
<model type='virtio' heads='1' primary='yes'/>
- <address type='pci' domain='0x0000' bus='0x04'
slot='0x00' function='0x0'/>
+ <address type='pci' domain='0x0000' bus='0x05'
slot='0x00' function='0x0'/>
</video>
<memballoon model='none'/>
</devices>
--
2.43.0
7:02 a.m.
New subject: [PATCH v2 06/17] qemu: Use virtio-scsi by default on RISC-V
On Wed, Feb 14, 2024 at 18:11:13 +0100, Andrea Bolognani wrote:
Just like piix3-uhci for USB, the choice of lsilogic for SCSI
was dictated entirely by that being the default for legacy x86
guests. Use virtio-scsi instead.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_domain.c | 4 +++-
...virt-default-models.riscv64-latest.abi-update.args | 5 +++--
...-virt-default-models.riscv64-latest.abi-update.xml | 11 ++++++++---
.../riscv64-virt-default-models.riscv64-latest.args | 5 +++--
.../riscv64-virt-default-models.riscv64-latest.xml | 11 ++++++++---
5 files changed, 25 insertions(+), 11 deletions(-)
So this is a rather important change. You should note it at least in the
release news. It might be worth even add a note to the XML definition
documentation, or at least the qemu driver specific page.
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
7:54 a.m.
New subject: [PATCH v2 06/17] qemu: Use virtio-scsi by default on RISC-V
On Thu, Feb 15, 2024 at 02:02:25PM +0100, Peter Krempa wrote:
On Wed, Feb 14, 2024 at 18:11:13 +0100, Andrea Bolognani wrote:
> Just like piix3-uhci for USB, the choice of lsilogic for SCSI
> was dictated entirely by that being the default for legacy x86
> guests. Use virtio-scsi instead.
>
> Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
> ---
> src/qemu/qemu_domain.c | 4 +++-
> ...virt-default-models.riscv64-latest.abi-update.args | 5 +++--
> ...-virt-default-models.riscv64-latest.abi-update.xml | 11 ++++++++---
> .../riscv64-virt-default-models.riscv64-latest.args | 5 +++--
> .../riscv64-virt-default-models.riscv64-latest.xml | 11 ++++++++---
> 5 files changed, 25 insertions(+), 11 deletions(-)
So this is a rather important change. You should note it at least in the
release news.
Strongly agreed. I'll be sure to handle that.
It might be worth even add a note to the XML definition
documentation, or at least the qemu driver specific page.
Not sure about this. We don't seem to explicitly document most of the
existing defaults, so I wouldn't even know where a change in defaults
could possibly fit in.
I think I'd skip this part, if that's okay with you.
--
Andrea Bolognani / Red Hat / Virtualization
8:18 a.m.
New subject: [PATCH v2 06/17] qemu: Use virtio-scsi by default on RISC-V
On Thu, Feb 15, 2024 at 05:54:58 -0800, Andrea Bolognani wrote:
On Thu, Feb 15, 2024 at 02:02:25PM +0100, Peter Krempa wrote:
> On Wed, Feb 14, 2024 at 18:11:13 +0100, Andrea Bolognani wrote:
> > Just like piix3-uhci for USB, the choice of lsilogic for SCSI
> > was dictated entirely by that being the default for legacy x86
> > guests. Use virtio-scsi instead.
> >
> > Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
> > ---
> > src/qemu/qemu_domain.c | 4 +++-
> > ...virt-default-models.riscv64-latest.abi-update.args | 5 +++--
> > ...-virt-default-models.riscv64-latest.abi-update.xml | 11 ++++++++---
> > .../riscv64-virt-default-models.riscv64-latest.args | 5 +++--
> > .../riscv64-virt-default-models.riscv64-latest.xml | 11 ++++++++---
> > 5 files changed, 25 insertions(+), 11 deletions(-)
>
> So this is a rather important change. You should note it at least in the
> release news.
Strongly agreed. I'll be sure to handle that.
> It might be worth even add a note to the XML definition
> documentation, or at least the qemu driver specific page.
Not sure about this. We don't seem to explicitly document most of the
existing defaults, so I wouldn't even know where a change in defaults
could possibly fit in.
I think I'd skip this part, if that's okay with you.
I'm okay with not adding it to 'formatdomain.rst' but IMO a section in
'drvqemu.rst' outlining these changes would make sense as it's driver
specific.
11:11 a.m.
New subject: [PATCH v2 07/17] tests: Add usb-legacy-multiple
We have special handling for this configuration, so make sure
that there is some test coverage too.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
.../usb-legacy-multiple.x86_64-latest.err | 1 +
.../usb-legacy-multiple.x86_64-latest.xml | 32 +++++++++++++++++++
tests/qemuxmlconfdata/usb-legacy-multiple.xml | 15 +++++++++
tests/qemuxmlconftest.c | 7 ++++
4 files changed, 55 insertions(+)
create mode 100644 tests/qemuxmlconfdata/usb-legacy-multiple.x86_64-latest.err
create mode 100644 tests/qemuxmlconfdata/usb-legacy-multiple.x86_64-latest.xml
create mode 100644 tests/qemuxmlconfdata/usb-legacy-multiple.xml
diff --git a/tests/qemuxmlconfdata/usb-legacy-multiple.x86_64-latest.err
b/tests/qemuxmlconfdata/usb-legacy-multiple.x86_64-latest.err
new file mode 100644
index 0000000000..4cf41f9406
--- /dev/null
+++ b/tests/qemuxmlconfdata/usb-legacy-multiple.x86_64-latest.err
@@ -0,0 +1 @@
+unsupported configuration: Multiple legacy USB controllers are not supported
diff --git a/tests/qemuxmlconfdata/usb-legacy-multiple.x86_64-latest.xml
b/tests/qemuxmlconfdata/usb-legacy-multiple.x86_64-latest.xml
new file mode 100644
index 0000000000..431599283d
--- /dev/null
+++ b/tests/qemuxmlconfdata/usb-legacy-multiple.x86_64-latest.xml
@@ -0,0 +1,32 @@
+<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/qemuxmlconfdata/usb-legacy-multiple.xml
b/tests/qemuxmlconfdata/usb-legacy-multiple.xml
new file mode 100644
index 0000000000..4b2547f348
--- /dev/null
+++ b/tests/qemuxmlconfdata/usb-legacy-multiple.xml
@@ -0,0 +1,15 @@
+<domain type='qemu'>
+ <name>guest</name>
+ <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid>
+ <memory>4194304</memory>
+ <vcpu>4</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc'>hvm</type>
+ </os>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ <controller type='usb'/>
+ <controller type='usb'/>
+ <memballoon model='none'/>
+ </devices>
+</domain>
diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c
index c43f4cab67..b9dcdb81b9 100644
--- a/tests/qemuxmlconftest.c
+++ b/tests/qemuxmlconftest.c
@@ -1870,6 +1870,13 @@ mymain(void)
ARG_QEMU_CAPS_DEL, QEMU_CAPS_DEVICE_QEMU_XHCI, QEMU_CAPS_NEC_USB_XHCI,
QEMU_CAPS_ICH9_USB_EHCI1, QEMU_CAPS_LAST,
ARG_END);
+ DO_TEST_FULL("usb-legacy-multiple", ".x86_64-latest",
+ ARG_CAPS_ARCH, "x86_64",
+ ARG_CAPS_VER, "latest",
+ ARG_FLAGS, FLAG_EXPECT_FAILURE,
+ ARG_QEMU_CAPS_DEL, QEMU_CAPS_PIIX3_USB_UHCI, QEMU_CAPS_LAST,
+ ARG_END);
+
DO_TEST_CAPS_LATEST("usb-none");
DO_TEST_CAPS_LATEST("usb-controller-piix3");
--
2.43.0
7:03 a.m.
New subject: [PATCH v2 07/17] tests: Add usb-legacy-multiple
On Wed, Feb 14, 2024 at 18:11:14 +0100, Andrea Bolognani wrote:
We have special handling for this configuration, so make sure
that there is some test coverage too.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
.../usb-legacy-multiple.x86_64-latest.err | 1 +
.../usb-legacy-multiple.x86_64-latest.xml | 32 +++++++++++++++++++
tests/qemuxmlconfdata/usb-legacy-multiple.xml | 15 +++++++++
tests/qemuxmlconftest.c | 7 ++++
4 files changed, 55 insertions(+)
create mode 100644 tests/qemuxmlconfdata/usb-legacy-multiple.x86_64-latest.err
create mode 100644 tests/qemuxmlconfdata/usb-legacy-multiple.x86_64-latest.xml
create mode 100644 tests/qemuxmlconfdata/usb-legacy-multiple.xml
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
11:11 a.m.
New subject: [PATCH v2 08/17] tests: Add usb-legacy-device
This provides coverage for the scenario in which an attempt is
made to use a USB device together with the legacy USB controller.
Note that, while the test case passes, the configuration it
produces doesn't actually work:
$ qemu-system-x86_64 -M pc -usb -device usb-tablet,bus=usb.0
Bus 'usb.0' not found
This is because the bus created by the legacy USB controller is
automatically assigned ID "usb-bus.0", but libvirt doesn't take
this into consideration when assigning addresses to USB devices.
In other words, the legacy USB controller will only work as long
as no attempt is made to attach devices to it, which arguably
doesn't make for a very useful controller.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
.../usb-legacy-device.x86_64-latest.args | 33 +++++++++++++++++++
.../usb-legacy-device.x86_64-latest.xml | 30 +++++++++++++++++
tests/qemuxmlconfdata/usb-legacy-device.xml | 15 +++++++++
tests/qemuxmlconftest.c | 5 +++
4 files changed, 83 insertions(+)
create mode 100644 tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.args
create mode 100644 tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.xml
create mode 100644 tests/qemuxmlconfdata/usb-legacy-device.xml
diff --git a/tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.args
b/tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.args
new file mode 100644
index 0000000000..1ef9965cbd
--- /dev/null
+++ b/tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.args
@@ -0,0 +1,33 @@
+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.xml
b/tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.xml
new file mode 100644
index 0000000000..2204c03380
--- /dev/null
+++ b/tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.xml
@@ -0,0 +1,30 @@
+<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-device.xml
b/tests/qemuxmlconfdata/usb-legacy-device.xml
new file mode 100644
index 0000000000..f85b293022
--- /dev/null
+++ b/tests/qemuxmlconfdata/usb-legacy-device.xml
@@ -0,0 +1,15 @@
+<domain type='qemu'>
+ <name>guest</name>
+ <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid>
+ <memory>4194304</memory>
+ <vcpu>4</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc'>hvm</type>
+ </os>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ <controller type='usb'/>
+ <input type='tablet' bus='usb'/>
+ <memballoon model='none'/>
+ </devices>
+</domain>
diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c
index b9dcdb81b9..b7778975c3 100644
--- a/tests/qemuxmlconftest.c
+++ b/tests/qemuxmlconftest.c
@@ -1876,6 +1876,11 @@ mymain(void)
ARG_FLAGS, FLAG_EXPECT_FAILURE,
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_QEMU_CAPS_DEL, QEMU_CAPS_PIIX3_USB_UHCI, QEMU_CAPS_LAST,
+ ARG_END);
DO_TEST_CAPS_LATEST("usb-none");
--
2.43.0
7:29 a.m.
New subject: [PATCH v2 08/17] tests: Add usb-legacy-device
On Wed, Feb 14, 2024 at 18:11:15 +0100, Andrea Bolognani wrote:
This provides coverage for the scenario in which an attempt is
made to use a USB device together with the legacy USB controller.
Note that, while the test case passes, the configuration it
produces doesn't actually work:
$ qemu-system-x86_64 -M pc -usb -device usb-tablet,bus=usb.0
Bus 'usb.0' not found
This is because the bus created by the legacy USB controller is
automatically assigned ID "usb-bus.0", but libvirt doesn't take
this into consideration when assigning addresses to USB devices.
In other words, the legacy USB controller will only work as long
as no attempt is made to attach devices to it, which arguably
doesn't make for a very useful controller.
Looking at the history the change of the bus name is documented in
commit f9618633a86cc74b33f178f05154d4edbc08c0fc :
usb: update docs for bus name change
At some point the default usb bus name changed from 'usb.0' to
'usb-bus.0' (probably as part of the qom conversion). Update
the usb documentation accordingly.
Released in qemu-1.5.0. The most likely commit which did the change was
0d936928ef87ca1bb7b41b5b89c400c699a7691c ("convert busses to QOM")
Thus arguably we can remove support for '-usb' at least for x86_64, but
almost certainly for anything else too.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
.../usb-legacy-device.x86_64-latest.args | 33 +++++++++++++++++++
.../usb-legacy-device.x86_64-latest.xml | 30 +++++++++++++++++
tests/qemuxmlconfdata/usb-legacy-device.xml | 15 +++++++++
tests/qemuxmlconftest.c | 5 +++
4 files changed, 83 insertions(+)
create mode 100644 tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.args
create mode 100644 tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.xml
create mode 100644 tests/qemuxmlconfdata/usb-legacy-device.xml
Preliminarily:
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
and I'll see how this changes after the patches reworking the code.
7:39 a.m.
New subject: [PATCH v2 08/17] tests: Add usb-legacy-device
On Thu, Feb 15, 2024 at 02:29:32PM +0100, Peter Krempa wrote:
On Wed, Feb 14, 2024 at 18:11:15 +0100, Andrea Bolognani wrote:
> This provides coverage for the scenario in which an attempt is
> made to use a USB device together with the legacy USB controller.
>
> Note that, while the test case passes, the configuration it
> produces doesn't actually work:
>
> $ qemu-system-x86_64 -M pc -usb -device usb-tablet,bus=usb.0
> Bus 'usb.0' not found
>
> This is because the bus created by the legacy USB controller is
> automatically assigned ID "usb-bus.0", but libvirt doesn't take
> this into consideration when assigning addresses to USB devices.
>
> In other words, the legacy USB controller will only work as long
> as no attempt is made to attach devices to it, which arguably
> doesn't make for a very useful controller.
Looking at the history the change of the bus name is documented in
commit f9618633a86cc74b33f178f05154d4edbc08c0fc :
usb: update docs for bus name change
At some point the default usb bus name changed from 'usb.0' to
'usb-bus.0' (probably as part of the qom conversion). Update
the usb documentation accordingly.
Released in qemu-1.5.0. The most likely commit which did the change was
0d936928ef87ca1bb7b41b5b89c400c699a7691c ("convert busses to QOM")
Good spelunking. I didn't go that far back, I just checked that QEMU
4.2.0 also behaved like this.
Thus arguably we can remove support for '-usb' at least for
x86_64, but
almost certainly for anything else too.
Spoiler: that's exactly what happens a few additional patches in ;)
--
Andrea Bolognani / Red Hat / Virtualization
11:11 a.m.
New subject: [PATCH v2 09/17] qemu: Always default to no USB controller on s390x
When support for s390x was introduced in libvirt, it naturally
followed the conventions established at the time for x86, which
were to have a USB controller added by default.
Later, in 2013, commit 3a82f628a964 made the default USB
controller model for s390x VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE,
effectively overriding the architecture-independent default.
However, an exception was carved out at the time: if the USB
controller had an address assigned to it, then it would be left
alone.
A couple of years later, commit 09ab9dcc85ec changed things
again in two ways: for starters, libvirt would no longer
automatically attempt to add a USB controller to newly-defined
s390x guests; moreover, the command line generator was changed
so that the legacy USB controller (-usb) would never be used
on s390x.
In other words, unless a model name is explicitly provided for
the USB controller, which is something that only actually works
when using a recent QEMU version (see commit f9ed4d385ab8),
s390x guests will never have USB controllers attached to them.
Remove the exception carved out a decade ago and always
reflect this fact accurately in the guest XML.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_domain.c | 14 +++++++++-----
.../qemuhotplug-base-ccw-live+ccw-virtio.xml | 5 +----
...live-with-2-ccw-virtio+ccw-virtio-1-reverse.xml | 5 +----
...qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml | 5 +----
...-live-with-ccw-virtio+ccw-virtio-2-explicit.xml | 5 +----
...-base-ccw-live-with-ccw-virtio+ccw-virtio-2.xml | 5 +----
.../qemuhotplug-base-ccw-live-with-ccw-virtio.xml | 5 +----
.../qemuhotplug-base-ccw-live.xml | 5 +----
.../s390-usb-address.s390x-latest.xml | 6 +-----
9 files changed, 17 insertions(+), 38 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 31d634b151..03ad590759 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5669,11 +5669,9 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont,
cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI;
if (ARCH_IS_S390(def->os.arch)) {
- if (cont->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
- /* set the default USB model to none for s390 unless an
- * address is found */
- cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE;
- }
+ /* No default model on s390x, one has to be provided
+ * explicitly by the user */
+ cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE;
} else if (ARCH_IS_PPC64(def->os.arch)) {
/* To not break migration we need to set default USB controller
* for ppc64 to pci-ohci if we cannot change ABI of the VM.
@@ -5698,6 +5696,12 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont,
cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
}
}
+
+ /* Make sure the 'none' USB controller doesn't have an address
+ * associated with it, as that would trip up later checks */
+ if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE)
+ cont->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE;
+
/* forbid usb model 'qusb1' and 'qusb2' in this kind of
hyperviosr */
if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB1 ||
cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB2) {
diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live+ccw-virtio.xml
b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live+ccw-virtio.xml
index 6e879ded86..300dea1382 100644
--- a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live+ccw-virtio.xml
+++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live+ccw-virtio.xml
@@ -29,11 +29,8 @@
<alias name='virtio-disk4'/>
<address type='ccw' cssid='0xfe' ssid='0x0'
devno='0x0000'/>
</disk>
- <controller type='usb' index='0'>
+ <controller type='usb' index='0' model='none'>
<alias name='usb'/>
- <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'>
- <zpci uid='0x0001' fid='0x00000000'/>
- </address>
</controller>
<controller type='scsi' index='0' model='virtio-scsi'>
<alias name='scsi0'/>
diff --git
a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-reverse.xml
b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-reverse.xml
index 9b16951e46..882a509eeb 100644
---
a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-reverse.xml
+++
b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-reverse.xml
@@ -39,11 +39,8 @@
<alias name='virtio-disk1'/>
<address type='ccw' cssid='0xfe' ssid='0x0'
devno='0x0000'/>
</disk>
- <controller type='usb' index='0'>
+ <controller type='usb' index='0' model='none'>
<alias name='usb'/>
- <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'>
- <zpci uid='0x0001' fid='0x00000000'/>
- </address>
</controller>
<controller type='scsi' index='0' model='virtio-scsi'>
<alias name='scsi0'/>
diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml
b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml
index b5292a7ed2..6167d54bd2 100644
--- a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml
+++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml
@@ -29,11 +29,8 @@
<alias name='virtio-disk0'/>
<address type='ccw' cssid='0xfe' ssid='0x0'
devno='0x0001'/>
</disk>
- <controller type='usb' index='0'>
+ <controller type='usb' index='0' model='none'>
<alias name='usb'/>
- <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'>
- <zpci uid='0x0001' fid='0x00000000'/>
- </address>
</controller>
<controller type='scsi' index='0' model='virtio-scsi'>
<alias name='scsi0'/>
diff --git
a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2-explicit.xml
b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2-explicit.xml
index f37868101c..67a5c84a6c 100644
---
a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2-explicit.xml
+++
b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2-explicit.xml
@@ -38,11 +38,8 @@
<alias name='virtio-disk4'/>
<address type='ccw' cssid='0xfe' ssid='0x0'
devno='0x0000'/>
</disk>
- <controller type='usb' index='0'>
+ <controller type='usb' index='0' model='none'>
<alias name='usb'/>
- <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'>
- <zpci uid='0x0001' fid='0x00000000'/>
- </address>
</controller>
<controller type='scsi' index='0' model='virtio-scsi'>
<alias name='scsi0'/>
diff --git
a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2.xml
b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2.xml
index f37868101c..67a5c84a6c 100644
---
a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2.xml
+++
b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2.xml
@@ -38,11 +38,8 @@
<alias name='virtio-disk4'/>
<address type='ccw' cssid='0xfe' ssid='0x0'
devno='0x0000'/>
</disk>
- <controller type='usb' index='0'>
+ <controller type='usb' index='0' model='none'>
<alias name='usb'/>
- <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'>
- <zpci uid='0x0001' fid='0x00000000'/>
- </address>
</controller>
<controller type='scsi' index='0' model='virtio-scsi'>
<alias name='scsi0'/>
diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio.xml
b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio.xml
index 42f89a07a2..07bbfa24a2 100644
--- a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio.xml
+++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio.xml
@@ -28,11 +28,8 @@
<alias name='virtio-disk4'/>
<address type='ccw' cssid='0xfe' ssid='0x0'
devno='0x0000'/>
</disk>
- <controller type='usb' index='0'>
+ <controller type='usb' index='0' model='none'>
<alias name='usb'/>
- <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'>
- <zpci uid='0x0001' fid='0x00000000'/>
- </address>
</controller>
<controller type='scsi' index='0' model='virtio-scsi'>
<alias name='scsi0'/>
diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live.xml
b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live.xml
index f0570b5cf4..4869103a06 100644
--- a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live.xml
+++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live.xml
@@ -19,11 +19,8 @@
<on_crash>restart</on_crash>
<devices>
<emulator>/usr/bin/qemu-system-s390x</emulator>
- <controller type='usb' index='0'>
+ <controller type='usb' index='0' model='none'>
<alias name='usb'/>
- <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'>
- <zpci uid='0x0001' fid='0x00000000'/>
- </address>
</controller>
<controller type='scsi' index='0' model='virtio-scsi'>
<alias name='scsi0'/>
diff --git a/tests/qemuxmlconfdata/s390-usb-address.s390x-latest.xml
b/tests/qemuxmlconfdata/s390-usb-address.s390x-latest.xml
index 595d0b1a1e..e17098a3df 100644
--- a/tests/qemuxmlconfdata/s390-usb-address.s390x-latest.xml
+++ b/tests/qemuxmlconfdata/s390-usb-address.s390x-latest.xml
@@ -17,11 +17,7 @@
<on_crash>destroy</on_crash>
<devices>
<emulator>/usr/bin/qemu-system-s390x</emulator>
- <controller type='usb' index='0'>
- <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'>
- <zpci uid='0x0001' fid='0x00000000'/>
- </address>
- </controller>
+ <controller type='usb' index='0' model='none'/>
<controller type='pci' index='0' model='pci-root'/>
<audio id='1' type='none'/>
<memballoon model='none'/>
--
2.43.0
8:20 a.m.
New subject: [PATCH v2 09/17] qemu: Always default to no USB controller on
s390x
On Wed, Feb 14, 2024 at 18:11:16 +0100, Andrea Bolognani wrote:
When support for s390x was introduced in libvirt, it naturally
followed the conventions established at the time for x86, which
were to have a USB controller added by default.
Later, in 2013, commit 3a82f628a964 made the default USB
controller model for s390x VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE,
effectively overriding the architecture-independent default.
However, an exception was carved out at the time: if the USB
controller had an address assigned to it, then it would be left
alone.
A couple of years later, commit 09ab9dcc85ec changed things
again in two ways: for starters, libvirt would no longer
automatically attempt to add a USB controller to newly-defined
s390x guests; moreover, the command line generator was changed
so that the legacy USB controller (-usb) would never be used
on s390x.
In other words, unless a model name is explicitly provided for
the USB controller, which is something that only actually works
when using a recent QEMU version (see commit f9ed4d385ab8),
s390x guests will never have USB controllers attached to them.
Remove the exception carved out a decade ago and always
reflect this fact accurately in the guest XML.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_domain.c | 14 +++++++++-----
.../qemuhotplug-base-ccw-live+ccw-virtio.xml | 5 +----
...live-with-2-ccw-virtio+ccw-virtio-1-reverse.xml | 5 +----
...qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml | 5 +----
...-live-with-ccw-virtio+ccw-virtio-2-explicit.xml | 5 +----
...-base-ccw-live-with-ccw-virtio+ccw-virtio-2.xml | 5 +----
.../qemuhotplug-base-ccw-live-with-ccw-virtio.xml | 5 +----
.../qemuhotplug-base-ccw-live.xml | 5 +----
.../s390-usb-address.s390x-latest.xml | 6 +-----
9 files changed, 17 insertions(+), 38 deletions(-)
Okay, so the change is that addresses no longer get reserved for these
devices as they reallistically never existed in s390 VMs.
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
3:49 a.m.
New subject: [PATCH v2 09/17] qemu: Always default to no USB controller on
s390x
On 14/02/2024 18.11, Andrea Bolognani wrote:
When support for s390x was introduced in libvirt, it naturally
followed the conventions established at the time for x86, which
were to have a USB controller added by default.
Later, in 2013, commit 3a82f628a964 made the default USB
controller model for s390x VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE,
effectively overriding the architecture-independent default.
However, an exception was carved out at the time: if the USB
controller had an address assigned to it, then it would be left
alone.
A couple of years later, commit 09ab9dcc85ec changed things
again in two ways: for starters, libvirt would no longer
automatically attempt to add a USB controller to newly-defined
s390x guests; moreover, the command line generator was changed
so that the legacy USB controller (-usb) would never be used
on s390x.
In other words, unless a model name is explicitly provided for
the USB controller, which is something that only actually works
when using a recent QEMU version (see commit f9ed4d385ab8),
s390x guests will never have USB controllers attached to them.
Remove the exception carved out a decade ago and always
reflect this fact accurately in the guest XML.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_domain.c | 14 +++++++++-----
.../qemuhotplug-base-ccw-live+ccw-virtio.xml | 5 +----
...live-with-2-ccw-virtio+ccw-virtio-1-reverse.xml | 5 +----
...qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml | 5 +----
...-live-with-ccw-virtio+ccw-virtio-2-explicit.xml | 5 +----
...-base-ccw-live-with-ccw-virtio+ccw-virtio-2.xml | 5 +----
.../qemuhotplug-base-ccw-live-with-ccw-virtio.xml | 5 +----
.../qemuhotplug-base-ccw-live.xml | 5 +----
.../s390-usb-address.s390x-latest.xml | 6 +-----
9 files changed, 17 insertions(+), 38 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 31d634b151..03ad590759 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5669,11 +5669,9 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont,
cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI;
if (ARCH_IS_S390(def->os.arch)) {
- if (cont->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
- /* set the default USB model to none for s390 unless an
- * address is found */
- cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE;
- }
+ /* No default model on s390x, one has to be provided
+ * explicitly by the user */
+ cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE;
} else if (ARCH_IS_PPC64(def->os.arch)) {
/* To not break migration we need to set default USB controller
* for ppc64 to pci-ohci if we cannot change ABI of the VM.
@@ -5698,6 +5696,12 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont,
cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
}
}
+
+ /* Make sure the 'none' USB controller doesn't have an address
+ * associated with it, as that would trip up later checks */
+ if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE)
+ cont->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE;
+
/* forbid usb model 'qusb1' and 'qusb2' in this kind of
hyperviosr */
if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB1 ||
cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB2) {
diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live+ccw-virtio.xml
b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live+ccw-virtio.xml
index 6e879ded86..300dea1382 100644
--- a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live+ccw-virtio.xml
+++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live+ccw-virtio.xml
@@ -29,11 +29,8 @@
<alias name='virtio-disk4'/>
<address type='ccw' cssid='0xfe' ssid='0x0'
devno='0x0000'/>
</disk>
- <controller type='usb' index='0'>
+ <controller type='usb' index='0' model='none'>
<alias name='usb'/>
- <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'>
- <zpci uid='0x0001' fid='0x00000000'/>
- </address>
</controller>
<controller type='scsi' index='0'
model='virtio-scsi'>
<alias name='scsi0'/>
diff --git
a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-reverse.xml
b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-reverse.xml
index 9b16951e46..882a509eeb 100644
---
a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-reverse.xml
+++
b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-reverse.xml
@@ -39,11 +39,8 @@
<alias name='virtio-disk1'/>
<address type='ccw' cssid='0xfe' ssid='0x0'
devno='0x0000'/>
</disk>
- <controller type='usb' index='0'>
+ <controller type='usb' index='0' model='none'>
<alias name='usb'/>
- <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'>
- <zpci uid='0x0001' fid='0x00000000'/>
- </address>
</controller>
<controller type='scsi' index='0'
model='virtio-scsi'>
<alias name='scsi0'/>
diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml
b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml
index b5292a7ed2..6167d54bd2 100644
--- a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml
+++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml
@@ -29,11 +29,8 @@
<alias name='virtio-disk0'/>
<address type='ccw' cssid='0xfe' ssid='0x0'
devno='0x0001'/>
</disk>
- <controller type='usb' index='0'>
+ <controller type='usb' index='0' model='none'>
<alias name='usb'/>
- <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'>
- <zpci uid='0x0001' fid='0x00000000'/>
- </address>
</controller>
<controller type='scsi' index='0'
model='virtio-scsi'>
<alias name='scsi0'/>
diff --git
a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2-explicit.xml
b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2-explicit.xml
index f37868101c..67a5c84a6c 100644
---
a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2-explicit.xml
+++
b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2-explicit.xml
@@ -38,11 +38,8 @@
<alias name='virtio-disk4'/>
<address type='ccw' cssid='0xfe' ssid='0x0'
devno='0x0000'/>
</disk>
- <controller type='usb' index='0'>
+ <controller type='usb' index='0' model='none'>
<alias name='usb'/>
- <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'>
- <zpci uid='0x0001' fid='0x00000000'/>
- </address>
</controller>
<controller type='scsi' index='0'
model='virtio-scsi'>
<alias name='scsi0'/>
diff --git
a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2.xml
b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2.xml
index f37868101c..67a5c84a6c 100644
---
a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2.xml
+++
b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2.xml
@@ -38,11 +38,8 @@
<alias name='virtio-disk4'/>
<address type='ccw' cssid='0xfe' ssid='0x0'
devno='0x0000'/>
</disk>
- <controller type='usb' index='0'>
+ <controller type='usb' index='0' model='none'>
<alias name='usb'/>
- <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'>
- <zpci uid='0x0001' fid='0x00000000'/>
- </address>
</controller>
<controller type='scsi' index='0'
model='virtio-scsi'>
<alias name='scsi0'/>
diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio.xml
b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio.xml
index 42f89a07a2..07bbfa24a2 100644
--- a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio.xml
+++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio.xml
@@ -28,11 +28,8 @@
<alias name='virtio-disk4'/>
<address type='ccw' cssid='0xfe' ssid='0x0'
devno='0x0000'/>
</disk>
- <controller type='usb' index='0'>
+ <controller type='usb' index='0' model='none'>
<alias name='usb'/>
- <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'>
- <zpci uid='0x0001' fid='0x00000000'/>
- </address>
</controller>
<controller type='scsi' index='0'
model='virtio-scsi'>
<alias name='scsi0'/>
diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live.xml
b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live.xml
index f0570b5cf4..4869103a06 100644
--- a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live.xml
+++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live.xml
@@ -19,11 +19,8 @@
<on_crash>restart</on_crash>
<devices>
<emulator>/usr/bin/qemu-system-s390x</emulator>
- <controller type='usb' index='0'>
+ <controller type='usb' index='0' model='none'>
<alias name='usb'/>
- <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'>
- <zpci uid='0x0001' fid='0x00000000'/>
- </address>
</controller>
<controller type='scsi' index='0'
model='virtio-scsi'>
<alias name='scsi0'/>
diff --git a/tests/qemuxmlconfdata/s390-usb-address.s390x-latest.xml
b/tests/qemuxmlconfdata/s390-usb-address.s390x-latest.xml
index 595d0b1a1e..e17098a3df 100644
--- a/tests/qemuxmlconfdata/s390-usb-address.s390x-latest.xml
+++ b/tests/qemuxmlconfdata/s390-usb-address.s390x-latest.xml
@@ -17,11 +17,7 @@
<on_crash>destroy</on_crash>
<devices>
<emulator>/usr/bin/qemu-system-s390x</emulator>
- <controller type='usb' index='0'>
- <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'>
- <zpci uid='0x0001' fid='0x00000000'/>
- </address>
- </controller>
+ <controller type='usb' index='0' model='none'/>
<controller type='pci' index='0' model='pci-root'/>
<audio id='1' type='none'/>
<memballoon model='none'/>
Reviewed-by: Thomas Huth <thuth(a)redhat.com>
8:03 a.m.
New subject: [PATCH v2 09/17] qemu: Always default to no USB controller on
s390x
On 2/14/24 18:11, Andrea Bolognani wrote:
When support for s390x was introduced in libvirt, it naturally
followed the conventions established at the time for x86, which
were to have a USB controller added by default.
Later, in 2013, commit 3a82f628a964 made the default USB
controller model for s390x VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE,
effectively overriding the architecture-independent default.
However, an exception was carved out at the time: if the USB
controller had an address assigned to it, then it would be left
alone.
A couple of years later, commit 09ab9dcc85ec changed things
again in two ways: for starters, libvirt would no longer
automatically attempt to add a USB controller to newly-defined
s390x guests; moreover, the command line generator was changed
so that the legacy USB controller (-usb) would never be used
on s390x.
In other words, unless a model name is explicitly provided for
the USB controller, which is something that only actually works
when using a recent QEMU version (see commit f9ed4d385ab8),
s390x guests will never have USB controllers attached to them.
Remove the exception carved out a decade ago and always
reflect this fact accurately in the guest XML.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_domain.c | 14 +++++++++-----
.../qemuhotplug-base-ccw-live+ccw-virtio.xml | 5 +----
...live-with-2-ccw-virtio+ccw-virtio-1-reverse.xml | 5 +----
...qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml | 5 +----
...-live-with-ccw-virtio+ccw-virtio-2-explicit.xml | 5 +----
...-base-ccw-live-with-ccw-virtio+ccw-virtio-2.xml | 5 +----
.../qemuhotplug-base-ccw-live-with-ccw-virtio.xml | 5 +----
.../qemuhotplug-base-ccw-live.xml | 5 +----
.../s390-usb-address.s390x-latest.xml | 6 +-----
9 files changed, 17 insertions(+), 38 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 31d634b151..03ad590759 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5669,11 +5669,9 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont,
cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI;
if (ARCH_IS_S390(def->os.arch)) {
- if (cont->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
- /* set the default USB model to none for s390 unless an
- * address is found */
- cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE;
- }
+ /* No default model on s390x, one has to be provided
+ * explicitly by the user */
+ cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE;
} else if (ARCH_IS_PPC64(def->os.arch)) {
/* To not break migration we need to set default USB controller
* for ppc64 to pci-ohci if we cannot change ABI of the VM.
@@ -5698,6 +5696,12 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont,
cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
}
}
+
+ /* Make sure the 'none' USB controller doesn't have an address
+ * associated with it, as that would trip up later checks */
+ if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE)
+ cont->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE;
+
/* forbid usb model 'qusb1' and 'qusb2' in this kind of
hyperviosr */
if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB1 ||
cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB2) {
diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live+ccw-virtio.xml
b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live+ccw-virtio.xml
index 6e879ded86..300dea1382 100644
--- a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live+ccw-virtio.xml
+++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live+ccw-virtio.xml
@@ -29,11 +29,8 @@
<alias name='virtio-disk4'/>
<address type='ccw' cssid='0xfe' ssid='0x0'
devno='0x0000'/>
</disk>
- <controller type='usb' index='0'>
+ <controller type='usb' index='0' model='none'>
<alias name='usb'/>
- <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'>
- <zpci uid='0x0001' fid='0x00000000'/>
- </address>
</controller>
<controller type='scsi' index='0'
model='virtio-scsi'>
<alias name='scsi0'/>
diff --git
a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-reverse.xml
b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-reverse.xml
index 9b16951e46..882a509eeb 100644
---
a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-reverse.xml
+++
b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-reverse.xml
@@ -39,11 +39,8 @@
<alias name='virtio-disk1'/>
<address type='ccw' cssid='0xfe' ssid='0x0'
devno='0x0000'/>
</disk>
- <controller type='usb' index='0'>
+ <controller type='usb' index='0' model='none'>
<alias name='usb'/>
- <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'>
- <zpci uid='0x0001' fid='0x00000000'/>
- </address>
</controller>
<controller type='scsi' index='0'
model='virtio-scsi'>
<alias name='scsi0'/>
diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml
b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml
index b5292a7ed2..6167d54bd2 100644
--- a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml
+++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml
@@ -29,11 +29,8 @@
<alias name='virtio-disk0'/>
<address type='ccw' cssid='0xfe' ssid='0x0'
devno='0x0001'/>
</disk>
- <controller type='usb' index='0'>
+ <controller type='usb' index='0' model='none'>
<alias name='usb'/>
- <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'>
- <zpci uid='0x0001' fid='0x00000000'/>
- </address>
</controller>
<controller type='scsi' index='0'
model='virtio-scsi'>
<alias name='scsi0'/>
diff --git
a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2-explicit.xml
b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2-explicit.xml
index f37868101c..67a5c84a6c 100644
---
a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2-explicit.xml
+++
b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2-explicit.xml
@@ -38,11 +38,8 @@
<alias name='virtio-disk4'/>
<address type='ccw' cssid='0xfe' ssid='0x0'
devno='0x0000'/>
</disk>
- <controller type='usb' index='0'>
+ <controller type='usb' index='0' model='none'>
<alias name='usb'/>
- <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'>
- <zpci uid='0x0001' fid='0x00000000'/>
- </address>
</controller>
<controller type='scsi' index='0'
model='virtio-scsi'>
<alias name='scsi0'/>
diff --git
a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2.xml
b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2.xml
index f37868101c..67a5c84a6c 100644
---
a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2.xml
+++
b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2.xml
@@ -38,11 +38,8 @@
<alias name='virtio-disk4'/>
<address type='ccw' cssid='0xfe' ssid='0x0'
devno='0x0000'/>
</disk>
- <controller type='usb' index='0'>
+ <controller type='usb' index='0' model='none'>
<alias name='usb'/>
- <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'>
- <zpci uid='0x0001' fid='0x00000000'/>
- </address>
</controller>
<controller type='scsi' index='0'
model='virtio-scsi'>
<alias name='scsi0'/>
diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio.xml
b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio.xml
index 42f89a07a2..07bbfa24a2 100644
--- a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio.xml
+++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio.xml
@@ -28,11 +28,8 @@
<alias name='virtio-disk4'/>
<address type='ccw' cssid='0xfe' ssid='0x0'
devno='0x0000'/>
</disk>
- <controller type='usb' index='0'>
+ <controller type='usb' index='0' model='none'>
<alias name='usb'/>
- <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'>
- <zpci uid='0x0001' fid='0x00000000'/>
- </address>
</controller>
<controller type='scsi' index='0'
model='virtio-scsi'>
<alias name='scsi0'/>
diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live.xml
b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live.xml
index f0570b5cf4..4869103a06 100644
--- a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live.xml
+++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live.xml
@@ -19,11 +19,8 @@
<on_crash>restart</on_crash>
<devices>
<emulator>/usr/bin/qemu-system-s390x</emulator>
- <controller type='usb' index='0'>
+ <controller type='usb' index='0' model='none'>
<alias name='usb'/>
- <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'>
- <zpci uid='0x0001' fid='0x00000000'/>
- </address>
</controller>
<controller type='scsi' index='0'
model='virtio-scsi'>
<alias name='scsi0'/>
diff --git a/tests/qemuxmlconfdata/s390-usb-address.s390x-latest.xml
b/tests/qemuxmlconfdata/s390-usb-address.s390x-latest.xml
index 595d0b1a1e..e17098a3df 100644
--- a/tests/qemuxmlconfdata/s390-usb-address.s390x-latest.xml
+++ b/tests/qemuxmlconfdata/s390-usb-address.s390x-latest.xml
@@ -17,11 +17,7 @@
<on_crash>destroy</on_crash>
<devices>
<emulator>/usr/bin/qemu-system-s390x</emulator>
- <controller type='usb' index='0'>
- <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'>
- <zpci uid='0x0001' fid='0x00000000'/>
- </address>
- </controller>
+ <controller type='usb' index='0' model='none'/>
<controller type='pci' index='0' model='pci-root'/>
<audio id='1' type='none'/>
<memballoon model='none'/>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
9:09 a.m.
New subject: [PATCH v2 09/17] qemu: Always default to no USB controller on
s390x
On Wed, Feb 14, 2024 at 18:11:16 +0100, Andrea Bolognani wrote:
When support for s390x was introduced in libvirt, it naturally
followed the conventions established at the time for x86, which
were to have a USB controller added by default.
Later, in 2013, commit 3a82f628a964 made the default USB
controller model for s390x VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE,
effectively overriding the architecture-independent default.
However, an exception was carved out at the time: if the USB
controller had an address assigned to it, then it would be left
alone.
A couple of years later, commit 09ab9dcc85ec changed things
again in two ways: for starters, libvirt would no longer
automatically attempt to add a USB controller to newly-defined
s390x guests; moreover, the command line generator was changed
so that the legacy USB controller (-usb) would never be used
on s390x.
In other words, unless a model name is explicitly provided for
the USB controller, which is something that only actually works
when using a recent QEMU version (see commit f9ed4d385ab8),
s390x guests will never have USB controllers attached to them.
Remove the exception carved out a decade ago and always
reflect this fact accurately in the guest XML.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_domain.c | 14 +++++++++-----
.../qemuhotplug-base-ccw-live+ccw-virtio.xml | 5 +----
...live-with-2-ccw-virtio+ccw-virtio-1-reverse.xml | 5 +----
...qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml | 5 +----
...-live-with-ccw-virtio+ccw-virtio-2-explicit.xml | 5 +----
...-base-ccw-live-with-ccw-virtio+ccw-virtio-2.xml | 5 +----
.../qemuhotplug-base-ccw-live-with-ccw-virtio.xml | 5 +----
.../qemuhotplug-base-ccw-live.xml | 5 +----
.../s390-usb-address.s390x-latest.xml | 6 +-----
9 files changed, 17 insertions(+), 38 deletions(-)
[...]
@@ -5698,6 +5696,12 @@
qemuDomainControllerDefPostParse(virDomainControllerDef *cont,
cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
}
}
+
+ /* Make sure the 'none' USB controller doesn't have an address
+ * associated with it, as that would trip up later checks */
+ if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE)
+ cont->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE;
One thing I'm slightly unsure about is whether the removal of address
won't have effect on generation of other addresses and thus in certain
very weird situations could trip up the virDomainDefCheckABIStability check.
Since the usb controller itself was never seen by the guest ABI that
part will be okay, but not reserving the address for it could cause
issues.
Now for migration this shouldn't be a problem unless somebody is passing
very weird migration XMLs.
For new VMs it can theoretically cause re-ordering of devices on the PCI
bus.
9:46 a.m.
New subject: [PATCH v2 09/17] qemu: Always default to no USB controller on s390x
On Fri, Feb 16, 2024 at 04:09:12PM +0100, Peter Krempa wrote:
On Wed, Feb 14, 2024 at 18:11:16 +0100, Andrea Bolognani wrote:
> + /* Make sure the 'none' USB controller doesn't have an address
> + * associated with it, as that would trip up later checks */
> + if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE)
> + cont->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE;
One thing I'm slightly unsure about is whether the removal of address
won't have effect on generation of other addresses and thus in certain
very weird situations could trip up the virDomainDefCheckABIStability check.
Since the usb controller itself was never seen by the guest ABI that
part will be okay, but not reserving the address for it could cause
issues.
Now for migration this shouldn't be a problem unless somebody is passing
very weird migration XMLs.
For new VMs it can theoretically cause re-ordering of devices on the PCI
bus.
For new VMs, the guest ABI has not been set in stone yet so even if
devices and controllers were to be shuffled around it wouldn't
matter.
For existing VMs, all addressess will have been recorded in the
domain XML and libvirt would never attempt to change them.
The point about migration is potentially a good one though. The
incoming XML will have the (default) model and address, but after
parsing they will be gone. Will that trip the ABI stability check?
I'm never sure at what point of the process that gets executed, and
on which inputs.
--
Andrea Bolognani / Red Hat / Virtualization
2:55 a.m.
New subject: [PATCH v2 09/17] qemu: Always default to no USB controller on
s390x
On Fri, Feb 16, 2024 at 07:46:01 -0800, Andrea Bolognani wrote:
On Fri, Feb 16, 2024 at 04:09:12PM +0100, Peter Krempa wrote:
> On Wed, Feb 14, 2024 at 18:11:16 +0100, Andrea Bolognani wrote:
> > + /* Make sure the 'none' USB controller doesn't have an
address
> > + * associated with it, as that would trip up later checks */
> > + if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE)
> > + cont->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE;
>
> One thing I'm slightly unsure about is whether the removal of address
> won't have effect on generation of other addresses and thus in certain
> very weird situations could trip up the virDomainDefCheckABIStability check.
>
> Since the usb controller itself was never seen by the guest ABI that
> part will be okay, but not reserving the address for it could cause
> issues.
>
> Now for migration this shouldn't be a problem unless somebody is passing
> very weird migration XMLs.
>
> For new VMs it can theoretically cause re-ordering of devices on the PCI
> bus.
For new VMs, the guest ABI has not been set in stone yet so even if
devices and controllers were to be shuffled around it wouldn't
matter.
For existing VMs, all addressess will have been recorded in the
domain XML and libvirt would never attempt to change them.
Note that historically we've considered that in most cases a partial XML
to be freshly defined or used with virDomainCreateXML() should be ABI
compatible with how we've treated it historically. It is in certain
cases not practical/possible, but generally this should be kept as much
as possible.
Basically we can't decide to remove an auto-added device. For shuffling
addresses around, while we shouldn't do just for the heck of it, it can
be a reasonable change. If a user cares about the address they should
put it into the XML.
The point about migration is potentially a good one though. The
incoming XML will have the (default) model and address, but after
parsing they will be gone. Will that trip the ABI stability check?
No, unless you're providing a custom migration XML there's no ABI
stability check happening. The XML formatted by libvirt is considered to
be authoritative and well-enough defined. Additionally there's nothing
to check the ABI against, as the source of the migration is simply
declaring what's happening.
I'm never sure at what point of the process that gets executed,
and
on which inputs.
11:11 a.m.
New subject: [PATCH v2 10/17] qemu: Only use legacy USB controller if actually needed
With the way the code is currently written, we can end up using
the legacy USB controller (-usb) for a guest that doesn't have
any USB controllers at all in its configuration.
This is pretty harmless, since in these cases QEMU will ignore
the request and not create any USB controller, but it's also
incorrect and gets in the way of further changes that we want
to make.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_command.c | 1 +
tests/qemuxmlconfdata/arm-vexpressa9-basic.aarch64-latest.args | 1 -
tests/qemuxmlconfdata/arm-vexpressa9-nodevs.aarch64-latest.args | 1 -
tests/qemuxmlconfdata/arm-vexpressa9-virtio.aarch64-latest.args | 1 -
tests/qemuxmlconfdata/disk-arm-virtio-sd.aarch64-latest.args | 1 -
tests/qemuxmlconfdata/sparc-minimal.sparc-latest.args | 1 -
.../usb-controller-implicit-isapc.x86_64-latest.args | 1 -
7 files changed, 1 insertion(+), 6 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 2719574fb5..7824c31bde 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2962,6 +2962,7 @@ qemuBuildLegacyUSBControllerCommandLine(virCommand *cmd,
}
if (nusb == 0 &&
+ nlegacy > 0 &&
!qemuBuildDomainForbidLegacyUSBController(def) &&
!ARCH_IS_S390(def->os.arch)) {
/* We haven't added any USB controller yet, but we haven't been asked
diff --git a/tests/qemuxmlconfdata/arm-vexpressa9-basic.aarch64-latest.args
b/tests/qemuxmlconfdata/arm-vexpressa9-basic.aarch64-latest.args
index 1ffef8383e..72d10a732e 100644
--- a/tests/qemuxmlconfdata/arm-vexpressa9-basic.aarch64-latest.args
+++ b/tests/qemuxmlconfdata/arm-vexpressa9-basic.aarch64-latest.args
@@ -30,7 +30,6 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-armtest/.config \
-initrd /arm.initrd \
-append 'console=ttyAMA0,115200n8 rw root=/dev/mmcblk0p3 rootwait
physmap.enabled=0' \
-dtb /arm.dtb \
--usb \
-drive file=/arm.raw,format=raw,if=sd,index=0 \
-netdev '{"type":"user","id":"hostnet0"}'
\
-net nic,macaddr=52:54:00:09:a4:37,netdev=hostnet0,model=lan9118,id=net0 \
diff --git a/tests/qemuxmlconfdata/arm-vexpressa9-nodevs.aarch64-latest.args
b/tests/qemuxmlconfdata/arm-vexpressa9-nodevs.aarch64-latest.args
index 86c0d47849..de96ada570 100644
--- a/tests/qemuxmlconfdata/arm-vexpressa9-nodevs.aarch64-latest.args
+++ b/tests/qemuxmlconfdata/arm-vexpressa9-nodevs.aarch64-latest.args
@@ -30,7 +30,6 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-armtest/.config \
-initrd /arm.initrd \
-append console=ttyAMA0,115200n8 \
-dtb /arm.dtb \
--usb \
-audiodev
'{"id":"audio1","driver":"none"}' \
-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
-msg timestamp=on
diff --git a/tests/qemuxmlconfdata/arm-vexpressa9-virtio.aarch64-latest.args
b/tests/qemuxmlconfdata/arm-vexpressa9-virtio.aarch64-latest.args
index 79b569aa38..b4c295be6b 100644
--- a/tests/qemuxmlconfdata/arm-vexpressa9-virtio.aarch64-latest.args
+++ b/tests/qemuxmlconfdata/arm-vexpressa9-virtio.aarch64-latest.args
@@ -31,7 +31,6 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-armtest/.config \
-append 'console=ttyAMA0,115200n8 rw root=/dev/vda3 rootwait physmap.enabled=0'
\
-dtb /arm.dtb \
-device
'{"driver":"virtio-serial-device","id":"virtio-serial0"}'
\
--usb \
-blockdev
'{"driver":"file","filename":"/arm.raw","node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}'
\
-blockdev
'{"node-name":"libvirt-1-format","read-only":false,"driver":"raw","file":"libvirt-1-storage"}'
\
-device
'{"driver":"virtio-blk-device","drive":"libvirt-1-format","id":"virtio-disk0","bootindex":1}'
\
diff --git a/tests/qemuxmlconfdata/disk-arm-virtio-sd.aarch64-latest.args
b/tests/qemuxmlconfdata/disk-arm-virtio-sd.aarch64-latest.args
index 2572b51f95..3328189b44 100644
--- a/tests/qemuxmlconfdata/disk-arm-virtio-sd.aarch64-latest.args
+++ b/tests/qemuxmlconfdata/disk-arm-virtio-sd.aarch64-latest.args
@@ -30,7 +30,6 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-armtest/.config \
-initrd /arm.initrd \
-append 'console=ttyAMA0,115200n8 rw root=/dev/vda3 rootwait physmap.enabled=0'
\
-dtb /arm.dtb \
--usb \
-drive file=/arm-sd.qcow2,format=qcow2,if=sd,index=0 \
-drive
file.driver=nbd,file.server.type=inet,file.server.host=localhost,file.server.port=10809,file.export=export,format=qcow2,if=sd,index=1
\
-drive
file.driver=gluster,file.volume=Volume3,file.path=Image.qcow2,file.server.0.type=inet,file.server.0.host=example.org,file.server.0.port=6000,file.server.1.type=inet,file.server.1.host=example.org,file.server.1.port=24007,file.server.2.type=unix,file.server.2.path=/path/to/sock,file.debug=4,format=qcow2,if=sd,index=2
\
diff --git a/tests/qemuxmlconfdata/sparc-minimal.sparc-latest.args
b/tests/qemuxmlconfdata/sparc-minimal.sparc-latest.args
index 1cb520cf4a..b285efa615 100644
--- a/tests/qemuxmlconfdata/sparc-minimal.sparc-latest.args
+++ b/tests/qemuxmlconfdata/sparc-minimal.sparc-latest.args
@@ -24,7 +24,6 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-redhat62sparc/.config \
-rtc base=utc \
-no-shutdown \
-boot strict=on \
--usb \
-blockdev
'{"driver":"file","filename":"/home/berrange/VirtualMachines/redhat-6.2-sparc.img","node-name":"libvirt-2-storage","auto-read-only":true,"discard":"unmap"}'
\
-blockdev
'{"node-name":"libvirt-2-format","read-only":false,"driver":"qcow2","file":"libvirt-2-storage"}'
\
-device
scsi-hd,bus=scsi.0,scsi-id=0,device_id=drive-scsi0-0-0-0,drive=libvirt-2-format,id=scsi0-0-0-0,bootindex=1
\
diff --git a/tests/qemuxmlconfdata/usb-controller-implicit-isapc.x86_64-latest.args
b/tests/qemuxmlconfdata/usb-controller-implicit-isapc.x86_64-latest.args
index 62dc26ee8b..cf8a176cd8 100644
--- a/tests/qemuxmlconfdata/usb-controller-implicit-isapc.x86_64-latest.args
+++ b/tests/qemuxmlconfdata/usb-controller-implicit-isapc.x86_64-latest.args
@@ -26,7 +26,6 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
-rtc base=utc \
-no-shutdown \
-boot strict=on \
--usb \
-audiodev
'{"id":"audio1","driver":"none"}' \
-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
-msg timestamp=on
--
2.43.0
11:11 a.m.
New subject: [PATCH v2 11/17] qemu: Validate USB controllers earlier
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(a)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
10:01 a.m.
New subject: [PATCH v2 11/17] qemu: Validate USB controllers earlier
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(a)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(a)redhat.com>
10:46 a.m.
New subject: [PATCH v2 11/17] qemu: Validate USB controllers earlier
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
2:23 a.m.
New subject: [PATCH v2 11/17] qemu: Validate USB controllers earlier
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.
4:29 a.m.
New subject: [PATCH v2 11/17] qemu: Validate USB controllers earlier
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
8:54 a.m.
New subject: [PATCH v2 11/17] qemu: Validate USB controllers earlier
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.
9:21 a.m.
New subject: [PATCH v2 11/17] qemu: Validate USB controllers earlier
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
11:11 a.m.
New subject: [PATCH v2 12/17] qemu: Improve error message for USB controller validation
Use the same wording as for SCSI controllers, which also
happens to contain additional information (the controller's
index).
The new error message and error type are more accurate anyway:
in most cases, it's perfectly fine for the user not to provide
a controller model explicitly, as libvirt will try to figure
out a reasonable default.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_validate.c | 5 +++--
...b-controller-default-unavailable-i440fx.x86_64-latest.err | 2 +-
.../usb-controller-default-unavailable-q35.x86_64-latest.err | 2 +-
tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.err | 2 +-
tests/qemuxmlconfdata/usb-legacy-multiple.x86_64-latest.err | 2 +-
5 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index ad1621a120..1c661b5b34 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -3560,8 +3560,9 @@ qemuValidateDomainDeviceDefControllerUSB(const
virDomainControllerDef *def,
return 0;
if (def->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("no model provided for USB controller"));
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unable to determine model for USB controller
idx=%1$d"),
+ def->idx);
return -1;
}
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
index 7a71aa107d..cac4e8e760 100644
--- a/tests/qemuxmlconfdata/usb-controller-default-unavailable-i440fx.x86_64-latest.err
+++ b/tests/qemuxmlconfdata/usb-controller-default-unavailable-i440fx.x86_64-latest.err
@@ -1 +1 @@
-unsupported configuration: no model provided for USB controller
+internal error: Unable to determine model for USB controller idx=0
diff --git
a/tests/qemuxmlconfdata/usb-controller-default-unavailable-q35.x86_64-latest.err
b/tests/qemuxmlconfdata/usb-controller-default-unavailable-q35.x86_64-latest.err
index 7a71aa107d..cac4e8e760 100644
--- a/tests/qemuxmlconfdata/usb-controller-default-unavailable-q35.x86_64-latest.err
+++ b/tests/qemuxmlconfdata/usb-controller-default-unavailable-q35.x86_64-latest.err
@@ -1 +1 @@
-unsupported configuration: no model provided for USB controller
+internal error: Unable to determine model for USB controller idx=0
diff --git a/tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.err
b/tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.err
index 7a71aa107d..cac4e8e760 100644
--- a/tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.err
+++ b/tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.err
@@ -1 +1 @@
-unsupported configuration: no model provided for USB controller
+internal error: Unable to determine model for USB controller idx=0
diff --git a/tests/qemuxmlconfdata/usb-legacy-multiple.x86_64-latest.err
b/tests/qemuxmlconfdata/usb-legacy-multiple.x86_64-latest.err
index 7a71aa107d..cac4e8e760 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: no model provided for USB controller
+internal error: Unable to determine model for USB controller idx=0
--
2.43.0
10:02 a.m.
New subject: [PATCH v2 12/17] qemu: Improve error message for USB controller
validation
On Wed, Feb 14, 2024 at 18:11:19 +0100, Andrea Bolognani wrote:
Use the same wording as for SCSI controllers, which also
happens to contain additional information (the controller's
index).
The new error message and error type are more accurate anyway:
in most cases, it's perfectly fine for the user not to provide
a controller model explicitly, as libvirt will try to figure
out a reasonable default.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_validate.c | 5 +++--
...b-controller-default-unavailable-i440fx.x86_64-latest.err | 2 +-
.../usb-controller-default-unavailable-q35.x86_64-latest.err | 2 +-
tests/qemuxmlconfdata/usb-legacy-device.x86_64-latest.err | 2 +-
tests/qemuxmlconfdata/usb-legacy-multiple.x86_64-latest.err | 2 +-
5 files changed, 7 insertions(+), 6 deletions(-)
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
11:11 a.m.
New subject: [PATCH v2 13/17] qemu: Drop all code dealing with the legacy USB controller
With the recent changes, we have ensured that all situations
that would have until now caused this code to be executed are
either handled by dropping the USB controller or by raising
an error.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_command.c | 78 -----------------------------------------
src/qemu/qemu_domain.c | 11 ------
2 files changed, 89 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index a544a3ccdc..28bdcf8d99 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2855,62 +2855,6 @@ qemuBuildControllerDevProps(const virDomainDef *domainDef,
}
-static bool
-qemuBuildDomainForbidLegacyUSBController(const virDomainDef *def)
-{
- if (qemuDomainIsQ35(def) ||
- qemuDomainIsARMVirt(def) ||
- qemuDomainIsRISCVVirt(def))
- return true;
-
- return false;
-}
-
-
-static int
-qemuBuildLegacyUSBControllerCommandLine(virCommand *cmd,
- const virDomainDef *def)
-{
- size_t i;
- size_t nlegacy = 0;
- size_t nusb = 0;
-
- for (i = 0; i < def->ncontrollers; i++) {
- virDomainControllerDef *cont = def->controllers[i];
-
- if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_USB)
- continue;
-
- /* If we have mode='none', there are no other USB controllers */
- if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE)
- return 0;
-
- if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT)
- nlegacy++;
- else
- nusb++;
- }
-
- if (nlegacy > 1) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Multiple legacy USB controllers are not
supported"));
- return -1;
- }
-
- if (nusb == 0 &&
- nlegacy > 0 &&
- !qemuBuildDomainForbidLegacyUSBController(def) &&
- !ARCH_IS_S390(def->os.arch)) {
- /* We haven't added any USB controller yet, but we haven't been asked
- * not to add one either. Add a legacy USB controller, unless we're
- * creating a kind of guest we want to keep legacy-free */
- virCommandAddArg(cmd, "-usb");
- }
-
- return 0;
-}
-
-
/**
* qemuBuildSkipController:
* @controller: Controller to check
@@ -2997,25 +2941,6 @@ qemuBuildControllersByTypeCommandLine(virCommand *cmd,
continue;
}
- if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB &&
- cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT &&
- !qemuBuildDomainForbidLegacyUSBController(def)) {
-
- /* An appropriate default USB controller model should already
- * have been selected in qemuDomainDeviceDefPostParse(); if
- * we still have no model by now, we have to fall back to the
- * legacy USB controller.
- *
- * Note that we *don't* want to end up with the legacy USB
- * controller for q35 and virt machines, so we go ahead and
- * fail in qemuBuildControllerDevProps(); on the other hand,
- * for s390 machines we want to ignore any USB controller
- * (see 548ba43028 for the full story), so we skip
- * qemuBuildControllerDevProps() but we don't ultimately end
- * up adding the legacy USB controller */
- continue;
- }
-
if (qemuBuildControllerDevProps(def, cont, qemuCaps, &props) < 0)
return -1;
@@ -3071,9 +2996,6 @@ qemuBuildControllersCommandLine(virCommand *cmd,
return -1;
}
- if (qemuBuildLegacyUSBControllerCommandLine(cmd, def) < 0)
- return -1;
-
return 0;
}
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 03ad590759..c194928ed1 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5653,17 +5653,6 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont,
case VIR_DOMAIN_CONTROLLER_TYPE_USB:
if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT &&
qemuCaps) {
- /* Pick a suitable default model for the USB controller if none
- * has been selected by the user and we have the qemuCaps for
- * figuring out which controllers are supported.
- *
- * We rely on device availability instead of setting the model
- * unconditionally because, for some machine types, there's a
- * chance we will get away with using the legacy USB controller
- * when the relevant device is not available.
- *
- * See qemuBuildControllersCommandLine() */
-
/* Default USB controller is piix3-uhci if available. */
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI))
cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI;
--
2.43.0
11:11 a.m.
New subject: [PATCH v2 14/17] qemu: Add qemuDomainDefaultUSBControllerModel()
Extract the logic from qemuDomainControllerDefPostParse().
The behavior is unchanged, we simply use an out argument to
return the model and entertain the possibility of the process
failing, even though the current implementation never will.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_domain.c | 80 +++++++++++++++++++++++++-----------------
1 file changed, 48 insertions(+), 32 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c194928ed1..a970bf5c18 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4157,6 +4157,47 @@ qemuDomainDefaultSCSIControllerModel(virDomainControllerModelSCSI
*model,
}
+static int
+qemuDomainDefaultUSBControllerModel(virDomainControllerModelUSB *model,
+ const virDomainDef *def,
+ virQEMUCaps *qemuCaps,
+ unsigned int parseFlags)
+{
+ /* Default USB controller is piix3-uhci if available. */
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI))
+ *model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI;
+
+ if (ARCH_IS_S390(def->os.arch)) {
+ /* No default model on s390x, one has to be provided
+ * explicitly by the user */
+ *model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE;
+ } else if (ARCH_IS_PPC64(def->os.arch)) {
+ /* To not break migration we need to set default USB controller
+ * for ppc64 to pci-ohci if we cannot change ABI of the VM.
+ * The nec-usb-xhci or qemu-xhci controller is used as default
+ * only for newly defined domains or devices. */
+ if ((parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) &&
+ virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI)) {
+ *model = VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
+ } else if ((parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) &&
+ virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI)) {
+ *model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
+ } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_OHCI)) {
+ *model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI;
+ } else {
+ /* Explicitly fallback to legacy USB controller for PPC64. */
+ *model = VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT;
+ }
+ } else if (def->os.arch == VIR_ARCH_AARCH64) {
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI))
+ *model = VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
+ else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI))
+ *model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
+ }
+
+ return 0;
+}
+
static int
qemuDomainDefAddDefaultDevices(virQEMUDriver *driver,
virDomainDef *def,
@@ -5652,38 +5693,13 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont,
break;
case VIR_DOMAIN_CONTROLLER_TYPE_USB:
- if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT &&
qemuCaps) {
- /* Default USB controller is piix3-uhci if available. */
- if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI))
- cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI;
-
- if (ARCH_IS_S390(def->os.arch)) {
- /* No default model on s390x, one has to be provided
- * explicitly by the user */
- cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE;
- } else if (ARCH_IS_PPC64(def->os.arch)) {
- /* To not break migration we need to set default USB controller
- * for ppc64 to pci-ohci if we cannot change ABI of the VM.
- * The nec-usb-xhci or qemu-xhci controller is used as default
- * only for newly defined domains or devices. */
- if ((parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) &&
- virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI)) {
- cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
- } else if ((parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) &&
- virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI)) {
- cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
- } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_OHCI)) {
- cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI;
- } else {
- /* Explicitly fallback to legacy USB controller for PPC64. */
- cont->model = -1;
- }
- } else if (def->os.arch == VIR_ARCH_AARCH64) {
- if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI))
- cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
- else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI))
- cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
- }
+ if (qemuCaps &&
+ cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT &&
+ qemuDomainDefaultUSBControllerModel(&cont->model, def, qemuCaps,
parseFlags) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unable to determine model for USB controller
idx=%1$d"),
+ cont->idx);
+ return -1;
}
/* Make sure the 'none' USB controller doesn't have an address
--
2.43.0
11:11 a.m.
New subject: [PATCH v2 15/17] qemu: Extend qemuDomainDefaultUSBControllerModel()
In addition to the code in qemuDomainControllerDefPostParse(),
which we have just factored into its own function, we also have
some code in qemuDomainDefAddDefaultDevices() that deals with
choosing the model for a USB controller, specifically for q35
guests. Integrate it into the newly-created function.
Since we want slightly different behaviors depending on whether
the USB controller that we're working on is the one that we try
to automatically add for certain new guests (addDefaultUSB), we
need to introduce a new parameter to the function.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_domain.c | 50 ++++++++++++++++++++++++++++--------------
1 file changed, 33 insertions(+), 17 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index a970bf5c18..6801a883f4 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4159,6 +4159,7 @@ qemuDomainDefaultSCSIControllerModel(virDomainControllerModelSCSI
*model,
static int
qemuDomainDefaultUSBControllerModel(virDomainControllerModelUSB *model,
+ bool autoAdded,
const virDomainDef *def,
virQEMUCaps *qemuCaps,
unsigned int parseFlags)
@@ -4195,16 +4196,34 @@ qemuDomainDefaultUSBControllerModel(virDomainControllerModelUSB
*model,
*model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
}
+ if (ARCH_IS_X86(def->os.arch)) {
+ if (qemuDomainIsQ35(def) && autoAdded) {
+ /* Prefer adding a USB3 controller if supported, fall back
+ * to USB2 if there is no USB3 available, and if that's
+ * unavailable don't add anything.
+ */
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI))
+ *model = VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
+ else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI))
+ *model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
+ else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_USB_EHCI1))
+ *model = VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1;
+ else
+ *model = VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT;
+ }
+ }
+
return 0;
}
static int
qemuDomainDefAddDefaultDevices(virQEMUDriver *driver,
virDomainDef *def,
- virQEMUCaps *qemuCaps)
+ virQEMUCaps *qemuCaps,
+ unsigned int parseFlags)
{
bool addDefaultUSB = false;
- int usbModel = -1; /* "default for machinetype" */
+ virDomainControllerModelUSB usbModel = VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT;
int pciRoot; /* index within def->controllers */
bool addImplicitSATA = false;
bool addPCIRoot = false;
@@ -4235,19 +4254,6 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver *driver,
addPCIeRoot = true;
addImplicitSATA = true;
addITCOWatchdog = true;
-
- /* Prefer adding a USB3 controller if supported, fall back
- * to USB2 if there is no USB3 available, and if that's
- * unavailable don't add anything.
- */
- if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI))
- usbModel = VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
- else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI))
- usbModel = VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
- else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_USB_EHCI1))
- usbModel = VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1;
- else
- addDefaultUSB = false;
break;
}
if (qemuDomainIsI440FX(def))
@@ -4340,6 +4346,16 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver *driver,
break;
}
+ if (addDefaultUSB) {
+ /* If no reasonable model can be figured out, we should
+ * simply not add the default USB controller */
+ if (qemuDomainDefaultUSBControllerModel(&usbModel, true, def, qemuCaps,
parseFlags) < 0 ||
+ usbModel == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT) {
+ addDefaultUSB = false;
+ }
+ }
+
+
if (addDefaultUSB &&
virDomainControllerFind(def, VIR_DOMAIN_CONTROLLER_TYPE_USB, 0) < 0
&&
virDomainDefAddUSBController(def, 0, usbModel) < 0)
@@ -5083,7 +5099,7 @@ qemuDomainDefPostParse(virDomainDef *def,
if (qemuDomainDefBootPostParse(def, driver, parseFlags) < 0)
return -1;
- if (qemuDomainDefAddDefaultDevices(driver, def, qemuCaps) < 0)
+ if (qemuDomainDefAddDefaultDevices(driver, def, qemuCaps, parseFlags) < 0)
return -1;
if (qemuDomainDefSetDefaultCPU(def, driver->hostarch, qemuCaps) < 0)
@@ -5695,7 +5711,7 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont,
case VIR_DOMAIN_CONTROLLER_TYPE_USB:
if (qemuCaps &&
cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT &&
- qemuDomainDefaultUSBControllerModel(&cont->model, def, qemuCaps,
parseFlags) < 0) {
+ qemuDomainDefaultUSBControllerModel(&cont->model, false, def,
qemuCaps, parseFlags) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unable to determine model for USB controller
idx=%1$d"),
cont->idx);
--
2.43.0
11:11 a.m.
New subject: [PATCH v2 16/17] qemu: Clean up qemuDomainDefaultUSBControllerModel()
Just like qemuDomainDefaultSCSIControllerModel() before that,
split the function into an internal part and a trivial public
wrapper.
This allows us to rewrite the internal logic in the much more
compact and readable
if (condition)
return value;
if (other_condition)
return other_value;
return default_value;
style, while still implementing the usual error handling
behavior and avoiding any ambiguity caused by
VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT having value -1.
The conversion is straightforward if messy, with one notable
exception: the piix3-uhci case, which serves as our fallback,
has to be moved from the top of the function to the bottom due
the change from the previous set-and-overwrite approach to the
new return-early approach.
The use of 'else if' is eliminated completely, comments are
updated and improved.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_domain.c | 143 +++++++++++++++++++++++++++++------------
1 file changed, 101 insertions(+), 42 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 6801a883f4..db09af496b 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4157,62 +4157,121 @@ qemuDomainDefaultSCSIControllerModel(virDomainControllerModelSCSI
*model,
}
-static int
-qemuDomainDefaultUSBControllerModel(virDomainControllerModelUSB *model,
- bool autoAdded,
- const virDomainDef *def,
- virQEMUCaps *qemuCaps,
- unsigned int parseFlags)
+/* Internal. Use qemuDomainDefaultUSBControllerModel() instead */
+static virDomainControllerModelUSB
+qemuDomainDefaultUSBControllerModelInternal(bool autoAdded,
+ const virDomainDef *def,
+ virQEMUCaps *qemuCaps,
+ unsigned int parseFlags)
{
- /* Default USB controller is piix3-uhci if available. */
- if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI))
- *model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI;
+ bool abiUpdate = !!(parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE);
if (ARCH_IS_S390(def->os.arch)) {
/* No default model on s390x, one has to be provided
* explicitly by the user */
- *model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE;
- } else if (ARCH_IS_PPC64(def->os.arch)) {
- /* To not break migration we need to set default USB controller
- * for ppc64 to pci-ohci if we cannot change ABI of the VM.
- * The nec-usb-xhci or qemu-xhci controller is used as default
- * only for newly defined domains or devices. */
- if ((parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) &&
- virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI)) {
- *model = VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
- } else if ((parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) &&
- virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI)) {
- *model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
- } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_OHCI)) {
- *model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI;
- } else {
- /* Explicitly fallback to legacy USB controller for PPC64. */
- *model = VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT;
- }
- } else if (def->os.arch == VIR_ARCH_AARCH64) {
+ return VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE;
+ }
+
+ if (ARCH_IS_PPC64(def->os.arch)) {
+ /* Newly-defined guests should use USB3 if possible */
+ if (abiUpdate && virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI))
+ return VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
+ if (abiUpdate && virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI))
+ return VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
+
+ /* To preserve backwards compatibility, existing guests need to
+ * use pci-ohci (USB2) instead */
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_OHCI))
+ return VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI;
+
+ /* If neither USB3 nor USB2 can be used, bail */
+ return VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT;
+ }
+
+ if (def->os.arch == VIR_ARCH_AARCH64) {
+ /* Prefer USB3 */
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI))
- *model = VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
- else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI))
- *model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
+ return VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI))
+ return VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
+
+ /* If USB3 is unavailable, we should probably bail by
+ * returning VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT here
+ * instead of possibly falling back to piix3-uhci (USB1), but
+ * historically we haven't done that and starting now might
+ * cause backwards compatibility issues */
}
if (ARCH_IS_X86(def->os.arch)) {
if (qemuDomainIsQ35(def) && autoAdded) {
- /* Prefer adding a USB3 controller if supported, fall back
- * to USB2 if there is no USB3 available, and if that's
- * unavailable don't add anything.
- */
+ /* Prefer USB3 */
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI))
- *model = VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
- else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI))
- *model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
- else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_USB_EHCI1))
- *model = VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1;
- else
- *model = VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT;
+ return VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI))
+ return VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
+
+ /* Fall back to USB2 */
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_USB_EHCI1))
+ return VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1;
+
+ /* If no suitable device is available, simply avoid
+ * adding the controller.
+ *
+ * Note that this is only the case for the USB controller
+ * that gets automatically added for new guests, and only
+ * for the q35 machine type */
+ return VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT;
}
}
+ /* The default USB controller is piix3-uhci (USB1) if available.
+ * This choice is a fairly poor one, rooted primarily in historical
+ * reasons; thankfully, in most cases we will overwrite it with a
+ * much more reasonable value below */
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI))
+ return VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI;
+
+ return VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT;
+}
+
+
+/**
+ * qemuDomainDefaultUSBControllerModel:
+ * @model: return location
+ * @autoAdded: whether this controller is being automatically added
+ * @def: domain definition
+ * @cont: controller definition, or NULL
+ * @qemuCaps: QEMU capabilities, or NULL
+ * @parseFlags: parse flags
+ *
+ * Choose a reasonable model to use for a USB controller where a
+ * specific one hasn't been provided by the user.
+ *
+ * The choice is based on a number of factors, including the guest's
+ * architecture and machine type. @qemuCaps, if provided, might be
+ * taken into consideration too.
+ *
+ * @autoAdded should be true is this is a controller that libvirt is
+ * trying to automatically add on domain creation for the user's
+ * convenience: in that case, the function might decide that the best
+ * course of action is to not add the controller after all. This
+ * decision will be communicated to the caller by setting @model
+ * to VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT.
+ *
+ * Not being able to come up with a value is NOT considered a
+ * failure. It's up to the caller to decide how to handle that
+ * scenario.
+ *
+ * Returns: 0 on success, <0 on failure.
+ */
+static int
+qemuDomainDefaultUSBControllerModel(virDomainControllerModelUSB *model,
+ bool autoAdded,
+ const virDomainDef *def,
+ virQEMUCaps *qemuCaps,
+ unsigned int parseFlags)
+{
+ *model = qemuDomainDefaultUSBControllerModelInternal(autoAdded, def, qemuCaps,
parseFlags);
return 0;
}
--
2.43.0
11:11 a.m.
New subject: [PATCH v2 17/17] qemu: Use qemu-xhci by default on RISC-V
Currently we fall back to the x86-derived default of piix3-uhci,
which is a USB1 controller that's not virtualization-friendly
and overall a terrible choice for a modern architecture. The
fact that we didn't choose a better default when RISC-V support
was introduced was an oversight which is now addressed.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_domain.c | 9 +++++++
...ault-models.riscv64-latest.abi-update.args | 15 +++++------
...fault-models.riscv64-latest.abi-update.xml | 26 ++++++++-----------
...64-virt-default-models.riscv64-latest.args | 15 +++++------
...v64-virt-default-models.riscv64-latest.xml | 26 ++++++++-----------
5 files changed, 45 insertions(+), 46 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index db09af496b..47058de2f2 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4166,6 +4166,15 @@ qemuDomainDefaultUSBControllerModelInternal(bool autoAdded,
{
bool abiUpdate = !!(parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE);
+ /* For modern architectures we want to use qemu-xhci (USB3), with
+ * no fallback if that's not available */
+ if (qemuDomainIsRISCVVirt(def)) {
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI)) {
+ return VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
+ }
+ return VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT;
+ }
+
if (ARCH_IS_S390(def->os.arch)) {
/* No default model on s390x, one has to be provided
* explicitly by the user */
diff --git
a/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.abi-update.args
b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.abi-update.args
index 7990f7f6a0..155c5aed9c 100644
--- a/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.abi-update.args
+++ b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.abi-update.args
@@ -27,17 +27,16 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-guest/.config \
-boot strict=on \
-device
'{"driver":"pcie-root-port","port":8,"chassis":1,"id":"pci.1","bus":"pcie.0","multifunction":true,"addr":"0x1"}'
\
-device
'{"driver":"pcie-root-port","port":9,"chassis":2,"id":"pci.2","bus":"pcie.0","addr":"0x1.0x1"}'
\
--device
'{"driver":"pcie-pci-bridge","id":"pci.3","bus":"pci.1","addr":"0x0"}'
\
--device
'{"driver":"pcie-root-port","port":10,"chassis":4,"id":"pci.4","bus":"pcie.0","addr":"0x1.0x2"}'
\
--device
'{"driver":"pcie-root-port","port":11,"chassis":5,"id":"pci.5","bus":"pcie.0","addr":"0x1.0x3"}'
\
--device
'{"driver":"pcie-root-port","port":12,"chassis":6,"id":"pci.6","bus":"pcie.0","addr":"0x1.0x4"}'
\
--device
'{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.3","addr":"0x1"}'
\
--device
'{"driver":"virtio-scsi-pci","id":"scsi0","bus":"pci.4","addr":"0x0"}'
\
+-device
'{"driver":"pcie-root-port","port":10,"chassis":3,"id":"pci.3","bus":"pcie.0","addr":"0x1.0x2"}'
\
+-device
'{"driver":"pcie-root-port","port":11,"chassis":4,"id":"pci.4","bus":"pcie.0","addr":"0x1.0x3"}'
\
+-device
'{"driver":"pcie-root-port","port":12,"chassis":5,"id":"pci.5","bus":"pcie.0","addr":"0x1.0x4"}'
\
+-device
'{"driver":"qemu-xhci","id":"usb","bus":"pci.2","addr":"0x0"}'
\
+-device
'{"driver":"virtio-scsi-pci","id":"scsi0","bus":"pci.3","addr":"0x0"}'
\
-netdev '{"type":"user","id":"hostnet0"}'
\
--device
'{"driver":"virtio-net-pci","netdev":"hostnet0","id":"net0","mac":"52:54:00:09:a4:37","bus":"pci.2","addr":"0x0"}'
\
+-device
'{"driver":"virtio-net-pci","netdev":"hostnet0","id":"net0","mac":"52:54:00:09:a4:37","bus":"pci.1","addr":"0x0"}'
\
-chardev pty,id=charserial0 \
-serial chardev:charserial0 \
-audiodev
'{"id":"audio1","driver":"none"}' \
--device
'{"driver":"virtio-vga","id":"video0","max_outputs":1,"bus":"pci.5","addr":"0x0"}'
\
+-device
'{"driver":"virtio-vga","id":"video0","max_outputs":1,"bus":"pci.4","addr":"0x0"}'
\
-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
-msg timestamp=on
diff --git
a/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.abi-update.xml
b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.abi-update.xml
index 72dc447d33..4628cd7895 100644
--- a/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.abi-update.xml
+++ b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.abi-update.xml
@@ -14,11 +14,11 @@
<on_crash>destroy</on_crash>
<devices>
<emulator>/usr/bin/qemu-system-riscv64</emulator>
- <controller type='usb' index='0' model='piix3-uhci'>
- <address type='pci' domain='0x0000' bus='0x03'
slot='0x01' function='0x0'/>
+ <controller type='usb' index='0' model='qemu-xhci'>
+ <address type='pci' domain='0x0000' bus='0x02'
slot='0x00' function='0x0'/>
</controller>
<controller type='scsi' index='0' model='virtio-scsi'>
- <address type='pci' domain='0x0000' bus='0x04'
slot='0x00' function='0x0'/>
+ <address type='pci' domain='0x0000' bus='0x03'
slot='0x00' function='0x0'/>
</controller>
<controller type='pci' index='0' model='pcie-root'/>
<controller type='pci' index='1'
model='pcie-root-port'>
@@ -31,29 +31,25 @@
<target chassis='2' port='0x9'/>
<address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x1'/>
</controller>
- <controller type='pci' index='3'
model='pcie-to-pci-bridge'>
- <model name='pcie-pci-bridge'/>
- <address type='pci' domain='0x0000' bus='0x01'
slot='0x00' function='0x0'/>
- </controller>
- <controller type='pci' index='4'
model='pcie-root-port'>
+ <controller type='pci' index='3'
model='pcie-root-port'>
<model name='pcie-root-port'/>
- <target chassis='4' port='0xa'/>
+ <target chassis='3' port='0xa'/>
<address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'/>
</controller>
- <controller type='pci' index='5'
model='pcie-root-port'>
+ <controller type='pci' index='4'
model='pcie-root-port'>
<model name='pcie-root-port'/>
- <target chassis='5' port='0xb'/>
+ <target chassis='4' port='0xb'/>
<address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x3'/>
</controller>
- <controller type='pci' index='6'
model='pcie-root-port'>
+ <controller type='pci' index='5'
model='pcie-root-port'>
<model name='pcie-root-port'/>
- <target chassis='6' port='0xc'/>
+ <target chassis='5' port='0xc'/>
<address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x4'/>
</controller>
<interface type='user'>
<mac address='52:54:00:09:a4:37'/>
<model type='virtio'/>
- <address type='pci' domain='0x0000' bus='0x02'
slot='0x00' function='0x0'/>
+ <address type='pci' domain='0x0000' bus='0x01'
slot='0x00' function='0x0'/>
</interface>
<serial type='pty'>
<target type='system-serial' port='0'>
@@ -66,7 +62,7 @@
<audio id='1' type='none'/>
<video>
<model type='virtio' heads='1' primary='yes'/>
- <address type='pci' domain='0x0000' bus='0x05'
slot='0x00' function='0x0'/>
+ <address type='pci' domain='0x0000' bus='0x04'
slot='0x00' function='0x0'/>
</video>
<memballoon model='none'/>
</devices>
diff --git a/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.args
b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.args
index 7990f7f6a0..155c5aed9c 100644
--- a/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.args
+++ b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.args
@@ -27,17 +27,16 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-guest/.config \
-boot strict=on \
-device
'{"driver":"pcie-root-port","port":8,"chassis":1,"id":"pci.1","bus":"pcie.0","multifunction":true,"addr":"0x1"}'
\
-device
'{"driver":"pcie-root-port","port":9,"chassis":2,"id":"pci.2","bus":"pcie.0","addr":"0x1.0x1"}'
\
--device
'{"driver":"pcie-pci-bridge","id":"pci.3","bus":"pci.1","addr":"0x0"}'
\
--device
'{"driver":"pcie-root-port","port":10,"chassis":4,"id":"pci.4","bus":"pcie.0","addr":"0x1.0x2"}'
\
--device
'{"driver":"pcie-root-port","port":11,"chassis":5,"id":"pci.5","bus":"pcie.0","addr":"0x1.0x3"}'
\
--device
'{"driver":"pcie-root-port","port":12,"chassis":6,"id":"pci.6","bus":"pcie.0","addr":"0x1.0x4"}'
\
--device
'{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.3","addr":"0x1"}'
\
--device
'{"driver":"virtio-scsi-pci","id":"scsi0","bus":"pci.4","addr":"0x0"}'
\
+-device
'{"driver":"pcie-root-port","port":10,"chassis":3,"id":"pci.3","bus":"pcie.0","addr":"0x1.0x2"}'
\
+-device
'{"driver":"pcie-root-port","port":11,"chassis":4,"id":"pci.4","bus":"pcie.0","addr":"0x1.0x3"}'
\
+-device
'{"driver":"pcie-root-port","port":12,"chassis":5,"id":"pci.5","bus":"pcie.0","addr":"0x1.0x4"}'
\
+-device
'{"driver":"qemu-xhci","id":"usb","bus":"pci.2","addr":"0x0"}'
\
+-device
'{"driver":"virtio-scsi-pci","id":"scsi0","bus":"pci.3","addr":"0x0"}'
\
-netdev '{"type":"user","id":"hostnet0"}'
\
--device
'{"driver":"virtio-net-pci","netdev":"hostnet0","id":"net0","mac":"52:54:00:09:a4:37","bus":"pci.2","addr":"0x0"}'
\
+-device
'{"driver":"virtio-net-pci","netdev":"hostnet0","id":"net0","mac":"52:54:00:09:a4:37","bus":"pci.1","addr":"0x0"}'
\
-chardev pty,id=charserial0 \
-serial chardev:charserial0 \
-audiodev
'{"id":"audio1","driver":"none"}' \
--device
'{"driver":"virtio-vga","id":"video0","max_outputs":1,"bus":"pci.5","addr":"0x0"}'
\
+-device
'{"driver":"virtio-vga","id":"video0","max_outputs":1,"bus":"pci.4","addr":"0x0"}'
\
-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
-msg timestamp=on
diff --git a/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.xml
b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.xml
index 72dc447d33..4628cd7895 100644
--- a/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.xml
+++ b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.xml
@@ -14,11 +14,11 @@
<on_crash>destroy</on_crash>
<devices>
<emulator>/usr/bin/qemu-system-riscv64</emulator>
- <controller type='usb' index='0' model='piix3-uhci'>
- <address type='pci' domain='0x0000' bus='0x03'
slot='0x01' function='0x0'/>
+ <controller type='usb' index='0' model='qemu-xhci'>
+ <address type='pci' domain='0x0000' bus='0x02'
slot='0x00' function='0x0'/>
</controller>
<controller type='scsi' index='0' model='virtio-scsi'>
- <address type='pci' domain='0x0000' bus='0x04'
slot='0x00' function='0x0'/>
+ <address type='pci' domain='0x0000' bus='0x03'
slot='0x00' function='0x0'/>
</controller>
<controller type='pci' index='0' model='pcie-root'/>
<controller type='pci' index='1'
model='pcie-root-port'>
@@ -31,29 +31,25 @@
<target chassis='2' port='0x9'/>
<address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x1'/>
</controller>
- <controller type='pci' index='3'
model='pcie-to-pci-bridge'>
- <model name='pcie-pci-bridge'/>
- <address type='pci' domain='0x0000' bus='0x01'
slot='0x00' function='0x0'/>
- </controller>
- <controller type='pci' index='4'
model='pcie-root-port'>
+ <controller type='pci' index='3'
model='pcie-root-port'>
<model name='pcie-root-port'/>
- <target chassis='4' port='0xa'/>
+ <target chassis='3' port='0xa'/>
<address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'/>
</controller>
- <controller type='pci' index='5'
model='pcie-root-port'>
+ <controller type='pci' index='4'
model='pcie-root-port'>
<model name='pcie-root-port'/>
- <target chassis='5' port='0xb'/>
+ <target chassis='4' port='0xb'/>
<address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x3'/>
</controller>
- <controller type='pci' index='6'
model='pcie-root-port'>
+ <controller type='pci' index='5'
model='pcie-root-port'>
<model name='pcie-root-port'/>
- <target chassis='6' port='0xc'/>
+ <target chassis='5' port='0xc'/>
<address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x4'/>
</controller>
<interface type='user'>
<mac address='52:54:00:09:a4:37'/>
<model type='virtio'/>
- <address type='pci' domain='0x0000' bus='0x02'
slot='0x00' function='0x0'/>
+ <address type='pci' domain='0x0000' bus='0x01'
slot='0x00' function='0x0'/>
</interface>
<serial type='pty'>
<target type='system-serial' port='0'>
@@ -66,7 +62,7 @@
<audio id='1' type='none'/>
<video>
<model type='virtio' heads='1' primary='yes'/>
- <address type='pci' domain='0x0000' bus='0x05'
slot='0x00' function='0x0'/>
+ <address type='pci' domain='0x0000' bus='0x04'
slot='0x00' function='0x0'/>
</video>
<memballoon model='none'/>
</devices>
--
2.43.0
311
days inactive
316
days old
43 comments
4 participants
participants (4)
-
Andrea Bolognani -
Boris Fiuczynski -
Peter Krempa -
Thomas Huth