[PATCH 0/3] qemu: Improve handling of architecture-specific defaults (SCSI)
I was working on this last year, then sort of lost track. Jim's recent patch[1] caused me to remember about this work and look into picking it up again. This is only half of the original series, which itself was reduced in scope compared to the first revision. I'll try to get around to everything, but addressing one area at the time is most likely to succeed. Of course this will no longer apply cleanly once Jim's patch has been pushed. The conflict will be trivial to solve though. Changes from [v2]: * several patches have been pushed; * address review comments. 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. [1] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/B45SL... [v2] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/FZ6BT... [v1] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/G53MR... Andrea Bolognani (3): qemu: Improve qemuDomainDefaultSCSIControllerModel() qemu: Clean up qemuDomainDefaultSCSIControllerModel() qemu: Use virtio-scsi by default on RISC-V src/qemu/qemu_domain.c | 48 +++++++++++++------ src/qemu/qemu_domain.h | 5 +- src/qemu/qemu_hotplug.c | 16 ++++--- src/qemu/qemu_postparse.c | 11 +++-- ...ault-models.riscv64-latest.abi-update.args | 5 +- ...fault-models.riscv64-latest.abi-update.xml | 11 +++-- ...64-virt-default-models.riscv64-latest.args | 5 +- ...v64-virt-default-models.riscv64-latest.xml | 11 +++-- 8 files changed, 73 insertions(+), 39 deletions(-) -- 2.50.0
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. We also update callers to check the return value against VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT instead of a functionally equivalent, but semantically less meaningful, check for whether the return value is negative. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 25 +++++++++++++++---------- src/qemu/qemu_domain.h | 5 ++--- src/qemu/qemu_hotplug.c | 16 +++++++++------- src/qemu/qemu_postparse.c | 11 +++++++---- 4 files changed, 33 insertions(+), 24 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2d62bcc62b..c408fb8c88 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4234,22 +4234,27 @@ qemuDomainDefAddDefaultAudioBackend(virQEMUDriver *driver, /** * @def: Domain definition - * @cont: Domain controller def * @qemuCaps: qemu capabilities * - * If the controller model is already defined, return it immediately; - * otherwise, based on the @qemuCaps return a default model value. + * Choose a reasonable model to use for a SCSI controller where a + * specific one hasn't been provided by the user. * - * Returns model on success, -1 on failure. + * 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 no sensible choice can be made for the controller model, + * VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT will be returned. It's + * likely that a failure will need to be reported in this scenario, + * but the handling is entirely up to the caller. + * + * Returns: a valid virDomainControllerModelSCSI value if one could + * be determined, or VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT */ -int +virDomainControllerModelSCSI qemuDomainDefaultSCSIControllerModel(const virDomainDef *def, - const virDomainControllerDef *cont, 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) || qemuDomainIsLoongArchVirt(def)) @@ -4261,7 +4266,7 @@ qemuDomainDefaultSCSIControllerModel(const virDomainDef *def, if (qemuDomainHasBuiltinESP(def)) return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_NCR53C90; - return -1; + return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT; } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 751607c12b..49f83613e3 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -838,9 +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, - virQEMUCaps *qemuCaps); +virDomainControllerModelSCSI qemuDomainDefaultSCSIControllerModel(const virDomainDef *def, + virQEMUCaps *qemuCaps); int qemuDomainDefAddDefaultAudioBackend(virQEMUDriver *driver, virDomainDef *def); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index c7f31ab264..073bd97d3a 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -930,7 +930,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]; @@ -956,13 +956,15 @@ qemuDomainFindOrCreateSCSIDiskController(virDomainObj *vm, * now hotplug a controller */ cont = virDomainControllerDefNew(VIR_DOMAIN_CONTROLLER_TYPE_SCSI); cont->idx = controller; + cont->model = model; - if (model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT) - cont->model = qemuDomainDefaultSCSIControllerModel(vm->def, cont, priv->qemuCaps); - else - cont->model = model; - - if (cont->model < 0) { + /* If no model has been discovered by looking at existing SCSI + * controllers, try to come up with a reasonable default. If one + * cannot be determined, error out */ + if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT) { + cont->model = qemuDomainDefaultSCSIControllerModel(vm->def, priv->qemuCaps); + } + if (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_postparse.c b/src/qemu/qemu_postparse.c index 3f9a1064c7..9c2427970d 100644 --- a/src/qemu/qemu_postparse.c +++ b/src/qemu/qemu_postparse.c @@ -343,10 +343,13 @@ 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 cannot be determined, error out */ + if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT || + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO) { + cont->model = qemuDomainDefaultSCSIControllerModel(def, qemuCaps); + } + if (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.50.0
Use a better order for sections, improve comments, tweak formatting. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c408fb8c88..a6c0b581b2 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4233,8 +4233,9 @@ qemuDomainDefAddDefaultAudioBackend(virQEMUDriver *driver, /** - * @def: Domain definition - * @qemuCaps: qemu capabilities + * qemuDomainDefaultSCSIControllerModel: + * @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. @@ -4255,16 +4256,27 @@ virDomainControllerModelSCSI qemuDomainDefaultSCSIControllerModel(const virDomainDef *def, virQEMUCaps *qemuCaps) { + /* 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) || + qemuDomainIsLoongArchVirt(def)) + 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 (ARCH_IS_S390(def->os.arch) || qemuDomainIsLoongArchVirt(def)) - return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI; + + /* 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; } -- 2.50.0
Using lsilogic on RISC-V was never an actual decision, but rather a consequence of that being the default for legacy x86 guests. Using virtio-scsi is a much more sensible choice. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 1 + ...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, 23 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index a6c0b581b2..4cb740d8dd 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4263,6 +4263,7 @@ qemuDomainDefaultSCSIControllerModel(const virDomainDef *def, /* Most new architectures should ideally use virtio */ if (ARCH_IS_S390(def->os.arch) || + qemuDomainIsRISCVVirt(def) || qemuDomainIsLoongArchVirt(def)) return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI; 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 265c9d3e80..00ecc83126 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 @@ -31,8 +31,9 @@ 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 \ @@ -41,6 +42,6 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-guest/.config \ -tpmdev emulator,id=tpm-tpm0,chardev=chrtpm \ -device '{"driver":"tpm-tis-device","tpmdev":"tpm-tpm0","id":"tpm0"}' \ -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 92ee2d7503..4543364c89 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 @@ -20,8 +20,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'> @@ -48,6 +48,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'/> @@ -67,7 +72,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 265c9d3e80..00ecc83126 100644 --- a/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.args +++ b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.args @@ -31,8 +31,9 @@ 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 \ @@ -41,6 +42,6 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-guest/.config \ -tpmdev emulator,id=tpm-tpm0,chardev=chrtpm \ -device '{"driver":"tpm-tis-device","tpmdev":"tpm-tpm0","id":"tpm0"}' \ -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 92ee2d7503..4543364c89 100644 --- a/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.xml +++ b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.xml @@ -20,8 +20,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'> @@ -48,6 +48,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'/> @@ -67,7 +72,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.50.0
On 7/4/25 14:36, Andrea Bolognani via Devel wrote:
I was working on this last year, then sort of lost track. Jim's recent patch[1] caused me to remember about this work and look into picking it up again.
This is only half of the original series, which itself was reduced in scope compared to the first revision. I'll try to get around to everything, but addressing one area at the time is most likely to succeed.
Of course this will no longer apply cleanly once Jim's patch has been pushed. The conflict will be trivial to solve though.
Changes from [v2]:
* several patches have been pushed;
* address review comments.
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.
[1] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/B45SL... [v2] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/FZ6BT... [v1] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/G53MR...
Andrea Bolognani (3): qemu: Improve qemuDomainDefaultSCSIControllerModel() qemu: Clean up qemuDomainDefaultSCSIControllerModel() qemu: Use virtio-scsi by default on RISC-V
src/qemu/qemu_domain.c | 48 +++++++++++++------ src/qemu/qemu_domain.h | 5 +- src/qemu/qemu_hotplug.c | 16 ++++--- src/qemu/qemu_postparse.c | 11 +++-- ...ault-models.riscv64-latest.abi-update.args | 5 +- ...fault-models.riscv64-latest.abi-update.xml | 11 +++-- ...64-virt-default-models.riscv64-latest.args | 5 +- ...v64-virt-default-models.riscv64-latest.xml | 11 +++-- 8 files changed, 73 insertions(+), 39 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Andrea Bolognani -
Michal Prívozník