[PATCH 00/33] qemu: Improve handling of architecture-specific defaults

This is a significantly expanded upon follow up to [1], and specifically the last 5 patches in that series. While looking at implementing the improvements suggested by Peter, I realized that there were many additional areas in which our handling of defaults was suboptimal, with the relevant code scattered all over the place and sometimes duplicated. So I set out to rationalize things. I ended up changing very little in terms of observable behavior outside of RISC-V, where I feel that we still have leeway to make things right before it reaches significant adoption and backwards compatibility becomes a major concern. [1] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/MREXS... Andrea Bolognani (33): tests: Add usb-controller-automatic-unavailable-q35 tests: Add aarch64-panic-no-model tests: Add title-and-description tests: Drop existing <title> and <description> tags tests: Rename and minimize no-memory tests: Add minimal cases for many architectures tests: Drop minimal tests: Add default-models cases for many architectures qemu: Fix a few comments qemu: Default to no USB and no memballoon for new architectures qemu: Clean up qemuDomainDefaultNetModel() qemu: Drop qemuDomainFindSCSIControllerModel() qemu: Drop qemuDomainSetSCSIControllerModel() qemu: Add missing error handling qemu: Simplify qemuDomainFindOrCreateSCSIDiskController() qemu: Move qemuDomainGetSCSIControllerModel() qemu: Rename qemuDomainDefaultSCSIControllerModel() qemu: Clean up qemuDomainDefaultSCSIControllerModel() qemu: Add qemuDomainDefaultUSBControllerModel() qemu: Enhance qemuDomainDefaultUSBControllerModel() qemu: Clean up qemuDomainDefaultUSBControllerModel() qemu: Move qemuDomainForbidLegacyUSBController() qemu: Enhance qemuDomainForbidLegacyUSBController() qemu: Add qemuDomainDefaultSerialType() qemu: Add qemuDomainDefaultSerialModel() qemu: Add qemuDomainDefaultPanicModel() qemu: Use qemuDomainDefaultPanicModel() more qemu: Rename qemuDomainDefaultVideoModel() qemu: Move qemuDomainDefault*() functions together qemu: Only default to <panic model='isa'/> on x86 qemu: Don't add memballoon by default on RISC-V qemu: Use qemu-xhci by default on RISC-V qemu: Use virtio-scsi by default on RISC-V src/qemu/qemu_alias.c | 13 +- src/qemu/qemu_command.c | 23 +- src/qemu/qemu_domain.c | 588 ++++++++++++------ src/qemu/qemu_domain.h | 4 + src/qemu/qemu_domain_address.c | 87 --- src/qemu/qemu_domain_address.h | 11 - src/qemu/qemu_hotplug.c | 13 +- src/qemu/qemu_validate.c | 9 +- tests/qemuxmlconfdata/440fx-wrong-root.xml | 5 - .../aarch64-panic-no-model.aarch64-latest.err | 1 + .../aarch64-panic-no-model.xml | 13 + ...64-virt-default-models.aarch64-latest.args | 44 ++ ...h64-virt-default-models.aarch64-latest.xml | 79 +++ .../aarch64-virt-default-models.xml | 21 + .../aarch64-virt-minimal.aarch64-latest.args | 31 + .../aarch64-virt-minimal.aarch64-latest.xml | 26 + .../qemuxmlconfdata/aarch64-virt-minimal.xml | 12 + .../cpu-host-model-features.x86_64-latest.xml | 5 - .../cpu-host-model-features.xml | 5 - ...ost-passthrough-features.x86_64-latest.xml | 5 - .../cpu-host-passthrough-features.xml | 5 - .../cpu-tsc-frequency.x86_64-latest.xml | 5 - tests/qemuxmlconfdata/cpu-tsc-frequency.xml | 5 - .../disk-cdrom-bus-other.x86_64-latest.xml | 1 - .../qemuxmlconfdata/disk-cdrom-bus-other.xml | 1 - tests/qemuxmlconfdata/minimal-no-memory.xml | 25 - .../minimal.x86_64-latest.args | 36 -- tests/qemuxmlconfdata/minimal.xml | 34 - tests/qemuxmlconfdata/missing-machine.xml | 1 - ...latest.err => no-memory.x86_64-latest.err} | 0 tests/qemuxmlconfdata/no-memory.xml | 11 + ...4-pseries-default-models.ppc64-latest.args | 38 ++ ...64-pseries-default-models.ppc64-latest.xml | 53 ++ .../ppc64-pseries-default-models.xml | 21 + .../ppc64-pseries-minimal.ppc64-latest.args | 33 + .../ppc64-pseries-minimal.ppc64-latest.xml | 33 + .../qemuxmlconfdata/ppc64-pseries-minimal.xml | 12 + ...64-virt-default-models.riscv64-latest.args | 42 ++ ...v64-virt-default-models.riscv64-latest.xml | 69 ++ .../riscv64-virt-default-models.xml | 21 + .../riscv64-virt-minimal.riscv64-latest.args | 30 + .../riscv64-virt-minimal.riscv64-latest.xml | 20 + .../qemuxmlconfdata/riscv64-virt-minimal.xml | 12 + ...s390x-ccw-default-models.s390x-latest.args | 37 ++ .../s390x-ccw-default-models.s390x-latest.xml | 46 ++ .../s390x-ccw-default-models.xml | 21 + .../s390x-ccw-minimal.s390x-latest.args | 32 + .../s390x-ccw-minimal.s390x-latest.xml | 27 + tests/qemuxmlconfdata/s390x-ccw-minimal.xml | 12 + .../title-and-description.x86_64-latest.args | 31 + ...> title-and-description.x86_64-latest.xml} | 20 +- .../qemuxmlconfdata/title-and-description.xml | 19 + ...tomatic-unavailable-q35.x86_64-latest.args | 33 + ...utomatic-unavailable-q35.x86_64-latest.xml | 30 + ...b-controller-automatic-unavailable-q35.xml | 20 + ...86_64-pc-default-models.x86_64-latest.args | 39 ++ ...x86_64-pc-default-models.x86_64-latest.xml | 50 ++ .../x86_64-pc-default-models.xml | 21 + .../x86_64-pc-minimal.x86_64-latest.args | 33 + ...ml => x86_64-pc-minimal.x86_64-latest.xml} | 24 +- tests/qemuxmlconfdata/x86_64-pc-minimal.xml | 12 + ...6_64-q35-default-models.x86_64-latest.args | 44 ++ ...86_64-q35-default-models.x86_64-latest.xml | 68 ++ .../x86_64-q35-default-models.xml | 21 + .../x86_64-q35-minimal.x86_64-latest.args | 38 ++ .../x86_64-q35-minimal.x86_64-latest.xml | 50 ++ tests/qemuxmlconfdata/x86_64-q35-minimal.xml | 12 + tests/qemuxmlconftest.c | 28 +- 68 files changed, 1783 insertions(+), 488 deletions(-) create mode 100644 tests/qemuxmlconfdata/aarch64-panic-no-model.aarch64-latest.err create mode 100644 tests/qemuxmlconfdata/aarch64-panic-no-model.xml create mode 100644 tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.args create mode 100644 tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.xml create mode 100644 tests/qemuxmlconfdata/aarch64-virt-default-models.xml create mode 100644 tests/qemuxmlconfdata/aarch64-virt-minimal.aarch64-latest.args create mode 100644 tests/qemuxmlconfdata/aarch64-virt-minimal.aarch64-latest.xml create mode 100644 tests/qemuxmlconfdata/aarch64-virt-minimal.xml delete mode 100644 tests/qemuxmlconfdata/minimal-no-memory.xml delete mode 100644 tests/qemuxmlconfdata/minimal.x86_64-latest.args delete mode 100644 tests/qemuxmlconfdata/minimal.xml rename tests/qemuxmlconfdata/{minimal-no-memory.x86_64-latest.err => no-memory.x86_64-latest.err} (100%) create mode 100644 tests/qemuxmlconfdata/no-memory.xml create mode 100644 tests/qemuxmlconfdata/ppc64-pseries-default-models.ppc64-latest.args create mode 100644 tests/qemuxmlconfdata/ppc64-pseries-default-models.ppc64-latest.xml create mode 100644 tests/qemuxmlconfdata/ppc64-pseries-default-models.xml create mode 100644 tests/qemuxmlconfdata/ppc64-pseries-minimal.ppc64-latest.args create mode 100644 tests/qemuxmlconfdata/ppc64-pseries-minimal.ppc64-latest.xml create mode 100644 tests/qemuxmlconfdata/ppc64-pseries-minimal.xml create mode 100644 tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.args create mode 100644 tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.xml create mode 100644 tests/qemuxmlconfdata/riscv64-virt-default-models.xml create mode 100644 tests/qemuxmlconfdata/riscv64-virt-minimal.riscv64-latest.args create mode 100644 tests/qemuxmlconfdata/riscv64-virt-minimal.riscv64-latest.xml create mode 100644 tests/qemuxmlconfdata/riscv64-virt-minimal.xml create mode 100644 tests/qemuxmlconfdata/s390x-ccw-default-models.s390x-latest.args create mode 100644 tests/qemuxmlconfdata/s390x-ccw-default-models.s390x-latest.xml create mode 100644 tests/qemuxmlconfdata/s390x-ccw-default-models.xml create mode 100644 tests/qemuxmlconfdata/s390x-ccw-minimal.s390x-latest.args create mode 100644 tests/qemuxmlconfdata/s390x-ccw-minimal.s390x-latest.xml create mode 100644 tests/qemuxmlconfdata/s390x-ccw-minimal.xml create mode 100644 tests/qemuxmlconfdata/title-and-description.x86_64-latest.args copy tests/qemuxmlconfdata/{440fx-wrong-root.xml => title-and-description.x86_64-latest.xml} (58%) create mode 100644 tests/qemuxmlconfdata/title-and-description.xml create mode 100644 tests/qemuxmlconfdata/usb-controller-automatic-unavailable-q35.x86_64-latest.args create mode 100644 tests/qemuxmlconfdata/usb-controller-automatic-unavailable-q35.x86_64-latest.xml create mode 100644 tests/qemuxmlconfdata/usb-controller-automatic-unavailable-q35.xml create mode 100644 tests/qemuxmlconfdata/x86_64-pc-default-models.x86_64-latest.args create mode 100644 tests/qemuxmlconfdata/x86_64-pc-default-models.x86_64-latest.xml create mode 100644 tests/qemuxmlconfdata/x86_64-pc-default-models.xml create mode 100644 tests/qemuxmlconfdata/x86_64-pc-minimal.x86_64-latest.args rename tests/qemuxmlconfdata/{minimal.x86_64-latest.xml => x86_64-pc-minimal.x86_64-latest.xml} (52%) create mode 100644 tests/qemuxmlconfdata/x86_64-pc-minimal.xml create mode 100644 tests/qemuxmlconfdata/x86_64-q35-default-models.x86_64-latest.args create mode 100644 tests/qemuxmlconfdata/x86_64-q35-default-models.x86_64-latest.xml create mode 100644 tests/qemuxmlconfdata/x86_64-q35-default-models.xml create mode 100644 tests/qemuxmlconfdata/x86_64-q35-minimal.x86_64-latest.args create mode 100644 tests/qemuxmlconfdata/x86_64-q35-minimal.x86_64-latest.xml create mode 100644 tests/qemuxmlconfdata/x86_64-q35-minimal.xml -- 2.43.0

For q35 guests, we normally add a USB controller by default, but there's a scenario in which we can decide to skip it. Add test coverage for it. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- ...tomatic-unavailable-q35.x86_64-latest.args | 33 +++++++++++++++++++ ...utomatic-unavailable-q35.x86_64-latest.xml | 30 +++++++++++++++++ ...b-controller-automatic-unavailable-q35.xml | 20 +++++++++++ tests/qemuxmlconftest.c | 9 +++++ 4 files changed, 92 insertions(+) create mode 100644 tests/qemuxmlconfdata/usb-controller-automatic-unavailable-q35.x86_64-latest.args create mode 100644 tests/qemuxmlconfdata/usb-controller-automatic-unavailable-q35.x86_64-latest.xml create mode 100644 tests/qemuxmlconfdata/usb-controller-automatic-unavailable-q35.xml diff --git a/tests/qemuxmlconfdata/usb-controller-automatic-unavailable-q35.x86_64-latest.args b/tests/qemuxmlconfdata/usb-controller-automatic-unavailable-q35.x86_64-latest.args new file mode 100644 index 0000000000..e883ce10cc --- /dev/null +++ b/tests/qemuxmlconfdata/usb-controller-automatic-unavailable-q35.x86_64-latest.args @@ -0,0 +1,33 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/var/lib/libvirt/qemu/domain--1-q35-test \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/var/lib/libvirt/qemu/domain--1-q35-test/.local/share \ +XDG_CACHE_HOME=/var/lib/libvirt/qemu/domain--1-q35-test/.cache \ +XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-q35-test/.config \ +/usr/bin/qemu-system-x86_64 \ +-name guest=q35-test,debug-threads=on \ +-S \ +-object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/var/lib/libvirt/qemu/domain--1-q35-test/master-key.aes"}' \ +-machine q35,usb=off,dump-guest-core=off,memory-backend=pc.ram,acpi=off \ +-accel tcg \ +-cpu qemu64 \ +-m size=2097152k \ +-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":2147483648}' \ +-overcommit mem-lock=off \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid 11dbdcdd-4c3b-482b-8903-9bdb8c0a2774 \ +-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 \ +-audiodev '{"id":"audio1","driver":"none"}' \ +-global ICH9-LPC.noreboot=off \ +-watchdog-action reset \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxmlconfdata/usb-controller-automatic-unavailable-q35.x86_64-latest.xml b/tests/qemuxmlconfdata/usb-controller-automatic-unavailable-q35.x86_64-latest.xml new file mode 100644 index 0000000000..11ca806c63 --- /dev/null +++ b/tests/qemuxmlconfdata/usb-controller-automatic-unavailable-q35.x86_64-latest.xml @@ -0,0 +1,30 @@ +<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='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-automatic-unavailable-q35.xml b/tests/qemuxmlconfdata/usb-controller-automatic-unavailable-q35.xml new file mode 100644 index 0000000000..d89dc4afe8 --- /dev/null +++ b/tests/qemuxmlconfdata/usb-controller-automatic-unavailable-q35.xml @@ -0,0 +1,20 @@ +<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> + <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'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c index b6fb5e5052..301c683448 100644 --- a/tests/qemuxmlconftest.c +++ b/tests/qemuxmlconftest.c @@ -1817,12 +1817,21 @@ mymain(void) ARG_CAPS_VER, "latest", 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_QEMU_CAPS_DEL, QEMU_CAPS_PIIX3_USB_UHCI, QEMU_CAPS_LAST, ARG_END); + /* However, if the USB controller is the one that gets added + * automatically for every guest instead of one that the user has + * explicitly asked for, we prefer simply skipping it */ + DO_TEST_FULL("usb-controller-automatic-unavailable-q35", ".x86_64-latest", + ARG_CAPS_ARCH, "x86_64", + ARG_CAPS_VER, "latest", + 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_CAPS_LATEST("usb-none"); -- 2.43.0

On Wed, Jan 24, 2024 at 20:37:21 +0100, Andrea Bolognani wrote:
For q35 guests, we normally add a USB controller by default, but there's a scenario in which we can decide to skip it. Add test coverage for it.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- ...tomatic-unavailable-q35.x86_64-latest.args | 33 +++++++++++++++++++ ...utomatic-unavailable-q35.x86_64-latest.xml | 30 +++++++++++++++++ ...b-controller-automatic-unavailable-q35.xml | 20 +++++++++++ tests/qemuxmlconftest.c | 9 +++++ 4 files changed, 92 insertions(+) create mode 100644 tests/qemuxmlconfdata/usb-controller-automatic-unavailable-q35.x86_64-latest.args create mode 100644 tests/qemuxmlconfdata/usb-controller-automatic-unavailable-q35.x86_64-latest.xml
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

This demonstrates that on aarch64, where a native panic device doesn't exist, it's necessary for the user to specify the model explicitly. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- .../aarch64-panic-no-model.aarch64-latest.err | 1 + tests/qemuxmlconfdata/aarch64-panic-no-model.xml | 13 +++++++++++++ tests/qemuxmlconftest.c | 1 + 3 files changed, 15 insertions(+) create mode 100644 tests/qemuxmlconfdata/aarch64-panic-no-model.aarch64-latest.err create mode 100644 tests/qemuxmlconfdata/aarch64-panic-no-model.xml diff --git a/tests/qemuxmlconfdata/aarch64-panic-no-model.aarch64-latest.err b/tests/qemuxmlconfdata/aarch64-panic-no-model.aarch64-latest.err new file mode 100644 index 0000000000..8e3f2c194d --- /dev/null +++ b/tests/qemuxmlconfdata/aarch64-panic-no-model.aarch64-latest.err @@ -0,0 +1 @@ +unsupported configuration: the QEMU binary does not support the ISA panic device diff --git a/tests/qemuxmlconfdata/aarch64-panic-no-model.xml b/tests/qemuxmlconfdata/aarch64-panic-no-model.xml new file mode 100644 index 0000000000..5207e48bbd --- /dev/null +++ b/tests/qemuxmlconfdata/aarch64-panic-no-model.xml @@ -0,0 +1,13 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory>4194304</memory> + <vcpu>4</vcpu> + <os> + <type arch='aarch64' machine='virt'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-system-aarch64</emulator> + <panic/> + </devices> +</domain> diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c index 301c683448..33c4448414 100644 --- a/tests/qemuxmlconftest.c +++ b/tests/qemuxmlconftest.c @@ -2480,6 +2480,7 @@ mymain(void) DO_TEST_CAPS_LATEST("panic"); DO_TEST_CAPS_LATEST("panic-double"); DO_TEST_CAPS_LATEST("panic-no-address"); + DO_TEST_CAPS_ARCH_LATEST_PARSE_ERROR("aarch64-panic-no-model", "aarch64"); DO_TEST_CAPS_LATEST("pvpanic-pci-x86_64"); DO_TEST_CAPS_ARCH_LATEST("pvpanic-pci-aarch64", "aarch64"); -- 2.43.0

On Wed, Jan 24, 2024 at 20:37:22 +0100, Andrea Bolognani wrote:
This demonstrates that on aarch64, where a native panic device doesn't exist, it's necessary for the user to specify the model explicitly.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- .../aarch64-panic-no-model.aarch64-latest.err | 1 + tests/qemuxmlconfdata/aarch64-panic-no-model.xml | 13 +++++++++++++ tests/qemuxmlconftest.c | 1 + 3 files changed, 15 insertions(+) create mode 100644 tests/qemuxmlconfdata/aarch64-panic-no-model.aarch64-latest.err create mode 100644 tests/qemuxmlconfdata/aarch64-panic-no-model.xml
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

We have a few test cases that cover the ability to set <title> and <description> for a guest as a side effect. Introduce an explicit test case for the functionality. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- .../title-and-description.x86_64-latest.args | 31 ++++++++++++++++++ .../title-and-description.x86_64-latest.xml | 32 +++++++++++++++++++ .../qemuxmlconfdata/title-and-description.xml | 19 +++++++++++ tests/qemuxmlconftest.c | 1 + 4 files changed, 83 insertions(+) create mode 100644 tests/qemuxmlconfdata/title-and-description.x86_64-latest.args create mode 100644 tests/qemuxmlconfdata/title-and-description.x86_64-latest.xml create mode 100644 tests/qemuxmlconfdata/title-and-description.xml diff --git a/tests/qemuxmlconfdata/title-and-description.x86_64-latest.args b/tests/qemuxmlconfdata/title-and-description.x86_64-latest.args new file mode 100644 index 0000000000..e12f137233 --- /dev/null +++ b/tests/qemuxmlconfdata/title-and-description.x86_64-latest.args @@ -0,0 +1,31 @@ +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=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 \ +-audiodev '{"id":"audio1","driver":"none"}' \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxmlconfdata/title-and-description.x86_64-latest.xml b/tests/qemuxmlconfdata/title-and-description.x86_64-latest.xml new file mode 100644 index 0000000000..2418893a6d --- /dev/null +++ b/tests/qemuxmlconfdata/title-and-description.x86_64-latest.xml @@ -0,0 +1,32 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <title>this is the guest title</title> + <description> + this is the + guest description + </description> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</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' model='none'/> + <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/title-and-description.xml b/tests/qemuxmlconfdata/title-and-description.xml new file mode 100644 index 0000000000..7be1316b07 --- /dev/null +++ b/tests/qemuxmlconfdata/title-and-description.xml @@ -0,0 +1,19 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <title>this is the guest title</title> + <description> + this is the + guest description + </description> + <memory unit='KiB'>219100</memory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' model='none'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c index 33c4448414..bd4e2c59fe 100644 --- a/tests/qemuxmlconftest.c +++ b/tests/qemuxmlconftest.c @@ -1228,6 +1228,7 @@ mymain(void) DO_TEST_CAPS_LATEST("minimal"); DO_TEST_CAPS_LATEST_PARSE_ERROR("minimal-no-memory"); + DO_TEST_CAPS_LATEST("title-and-description"); DO_TEST_CAPS_LATEST("genid"); DO_TEST_CAPS_LATEST("genid-auto"); -- 2.43.0

On Wed, Jan 24, 2024 at 20:37:23 +0100, Andrea Bolognani wrote:
We have a few test cases that cover the ability to set <title> and <description> for a guest as a side effect. Introduce an explicit test case for the functionality.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- .../title-and-description.x86_64-latest.args | 31 ++++++++++++++++++ .../title-and-description.x86_64-latest.xml | 32 +++++++++++++++++++ .../qemuxmlconfdata/title-and-description.xml | 19 +++++++++++ tests/qemuxmlconftest.c | 1 + 4 files changed, 83 insertions(+) create mode 100644 tests/qemuxmlconfdata/title-and-description.x86_64-latest.args create mode 100644 tests/qemuxmlconfdata/title-and-description.x86_64-latest.xml create mode 100644 tests/qemuxmlconfdata/title-and-description.xml
This one, since it has no impact on the commandline, would qualify also for genericxml2xmltest. Reviewed-by: Peter Krempa <pkrempa@redhat.com>

Now that we have an explicit test case for the feature, we can drop a bunch of duplicated accidental coverage. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/qemuxmlconfdata/440fx-wrong-root.xml | 5 ----- .../cpu-host-model-features.x86_64-latest.xml | 5 ----- tests/qemuxmlconfdata/cpu-host-model-features.xml | 5 ----- .../cpu-host-passthrough-features.x86_64-latest.xml | 5 ----- tests/qemuxmlconfdata/cpu-host-passthrough-features.xml | 5 ----- tests/qemuxmlconfdata/cpu-tsc-frequency.x86_64-latest.xml | 5 ----- tests/qemuxmlconfdata/cpu-tsc-frequency.xml | 5 ----- tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.xml | 1 - tests/qemuxmlconfdata/disk-cdrom-bus-other.xml | 1 - tests/qemuxmlconfdata/minimal.x86_64-latest.xml | 5 ----- tests/qemuxmlconfdata/minimal.xml | 5 ----- tests/qemuxmlconfdata/missing-machine.xml | 1 - 12 files changed, 48 deletions(-) diff --git a/tests/qemuxmlconfdata/440fx-wrong-root.xml b/tests/qemuxmlconfdata/440fx-wrong-root.xml index 803f28d569..9197b21c06 100644 --- a/tests/qemuxmlconfdata/440fx-wrong-root.xml +++ b/tests/qemuxmlconfdata/440fx-wrong-root.xml @@ -1,11 +1,6 @@ <domain type='qemu'> <name>QEMUGuest1</name> <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> - <title>A description of the test machine.</title> - <description> - A test of qemu's minimal configuration. - This test also tests the description and title elements. - </description> <memory unit='KiB'>219100</memory> <currentMemory unit='KiB'>219100</currentMemory> <os> diff --git a/tests/qemuxmlconfdata/cpu-host-model-features.x86_64-latest.xml b/tests/qemuxmlconfdata/cpu-host-model-features.x86_64-latest.xml index 0d4b236371..dd927e1075 100644 --- a/tests/qemuxmlconfdata/cpu-host-model-features.x86_64-latest.xml +++ b/tests/qemuxmlconfdata/cpu-host-model-features.x86_64-latest.xml @@ -1,11 +1,6 @@ <domain type='kvm'> <name>QEMUGuest1</name> <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> - <title>A description of the test machine.</title> - <description> - A test of qemu's minimal configuration. - This test also tests the description and title elements. - </description> <memory unit='KiB'>219100</memory> <currentMemory unit='KiB'>219100</currentMemory> <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu> diff --git a/tests/qemuxmlconfdata/cpu-host-model-features.xml b/tests/qemuxmlconfdata/cpu-host-model-features.xml index 6a00b34136..c13ae375d6 100644 --- a/tests/qemuxmlconfdata/cpu-host-model-features.xml +++ b/tests/qemuxmlconfdata/cpu-host-model-features.xml @@ -1,11 +1,6 @@ <domain type='kvm'> <name>QEMUGuest1</name> <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> - <title>A description of the test machine.</title> - <description> - A test of qemu's minimal configuration. - This test also tests the description and title elements. - </description> <memory unit='KiB'>219100</memory> <currentMemory unit='KiB'>219100</currentMemory> <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu> diff --git a/tests/qemuxmlconfdata/cpu-host-passthrough-features.x86_64-latest.xml b/tests/qemuxmlconfdata/cpu-host-passthrough-features.x86_64-latest.xml index 35c99b533a..02bddfb145 100644 --- a/tests/qemuxmlconfdata/cpu-host-passthrough-features.x86_64-latest.xml +++ b/tests/qemuxmlconfdata/cpu-host-passthrough-features.x86_64-latest.xml @@ -1,11 +1,6 @@ <domain type='kvm'> <name>QEMUGuest1</name> <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> - <title>A description of the test machine.</title> - <description> - A test of qemu's minimal configuration. - This test also tests the description and title elements. - </description> <memory unit='KiB'>219100</memory> <currentMemory unit='KiB'>219100</currentMemory> <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu> diff --git a/tests/qemuxmlconfdata/cpu-host-passthrough-features.xml b/tests/qemuxmlconfdata/cpu-host-passthrough-features.xml index d8bd36c1f2..e87b5fe2f1 100644 --- a/tests/qemuxmlconfdata/cpu-host-passthrough-features.xml +++ b/tests/qemuxmlconfdata/cpu-host-passthrough-features.xml @@ -1,11 +1,6 @@ <domain type='kvm'> <name>QEMUGuest1</name> <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> - <title>A description of the test machine.</title> - <description> - A test of qemu's minimal configuration. - This test also tests the description and title elements. - </description> <memory unit='KiB'>219100</memory> <currentMemory unit='KiB'>219100</currentMemory> <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu> diff --git a/tests/qemuxmlconfdata/cpu-tsc-frequency.x86_64-latest.xml b/tests/qemuxmlconfdata/cpu-tsc-frequency.x86_64-latest.xml index ad889ca691..068a2d6c54 100644 --- a/tests/qemuxmlconfdata/cpu-tsc-frequency.x86_64-latest.xml +++ b/tests/qemuxmlconfdata/cpu-tsc-frequency.x86_64-latest.xml @@ -1,11 +1,6 @@ <domain type='kvm'> <name>QEMUGuest1</name> <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> - <title>A description of the test machine.</title> - <description> - A test of qemu's minimal configuration. - This test also tests the description and title elements. - </description> <memory unit='KiB'>219100</memory> <currentMemory unit='KiB'>219100</currentMemory> <vcpu placement='static'>1</vcpu> diff --git a/tests/qemuxmlconfdata/cpu-tsc-frequency.xml b/tests/qemuxmlconfdata/cpu-tsc-frequency.xml index afaef6e8e5..2acafe8f13 100644 --- a/tests/qemuxmlconfdata/cpu-tsc-frequency.xml +++ b/tests/qemuxmlconfdata/cpu-tsc-frequency.xml @@ -1,11 +1,6 @@ <domain type='kvm'> <name>QEMUGuest1</name> <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> - <title>A description of the test machine.</title> - <description> - A test of qemu's minimal configuration. - This test also tests the description and title elements. - </description> <memory unit='KiB'>219100</memory> <currentMemory unit='KiB'>219100</currentMemory> <vcpu placement='static'>1</vcpu> diff --git a/tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.xml b/tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.xml index 8da45000d2..312e735947 100644 --- a/tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.xml +++ b/tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.xml @@ -1,7 +1,6 @@ <domain type='qemu'> <name>QEMUGuest1</name> <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> - <description>This test is meant for testing CDROMS with buses which don't really support them</description> <memory unit='KiB'>219100</memory> <currentMemory unit='KiB'>219100</currentMemory> <vcpu placement='static'>1</vcpu> diff --git a/tests/qemuxmlconfdata/disk-cdrom-bus-other.xml b/tests/qemuxmlconfdata/disk-cdrom-bus-other.xml index 2f3efec61f..9564dc11be 100644 --- a/tests/qemuxmlconfdata/disk-cdrom-bus-other.xml +++ b/tests/qemuxmlconfdata/disk-cdrom-bus-other.xml @@ -1,7 +1,6 @@ <domain type='qemu'> <name>QEMUGuest1</name> <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> - <description>This test is meant for testing CDROMS with buses which don't really support them</description> <memory unit='KiB'>219100</memory> <currentMemory unit='KiB'>219100</currentMemory> <vcpu placement='static'>1</vcpu> diff --git a/tests/qemuxmlconfdata/minimal.x86_64-latest.xml b/tests/qemuxmlconfdata/minimal.x86_64-latest.xml index 5f26f47508..ffa03532bd 100644 --- a/tests/qemuxmlconfdata/minimal.x86_64-latest.xml +++ b/tests/qemuxmlconfdata/minimal.x86_64-latest.xml @@ -1,11 +1,6 @@ <domain type='qemu'> <name>QEMUGuest1</name> <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> - <title>A description of the test machine.</title> - <description> - A test of qemu's minimal configuration. - This test also tests the description and title elements. - </description> <memory unit='KiB'>219100</memory> <currentMemory unit='KiB'>219100</currentMemory> <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu> diff --git a/tests/qemuxmlconfdata/minimal.xml b/tests/qemuxmlconfdata/minimal.xml index 8854059ab1..a77240d87c 100644 --- a/tests/qemuxmlconfdata/minimal.xml +++ b/tests/qemuxmlconfdata/minimal.xml @@ -1,11 +1,6 @@ <domain type='qemu'> <name>QEMUGuest1</name> <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> - <title>A description of the test machine.</title> - <description> - A test of qemu's minimal configuration. - This test also tests the description and title elements. - </description> <memory unit='KiB'>219100</memory> <currentMemory unit='KiB'>219100</currentMemory> <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu> diff --git a/tests/qemuxmlconfdata/missing-machine.xml b/tests/qemuxmlconfdata/missing-machine.xml index d9935d3338..9170667568 100644 --- a/tests/qemuxmlconfdata/missing-machine.xml +++ b/tests/qemuxmlconfdata/missing-machine.xml @@ -1,7 +1,6 @@ <domain type='qemu'> <name>QEMUGuest1</name> <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> - <title>A description of the test machine.</title> <memory unit='KiB'>219100</memory> <currentMemory unit='KiB'>219100</currentMemory> <vcpu placement='static' cpuset='1'>1</vcpu> -- 2.43.0

On Wed, Jan 24, 2024 at 20:37:24 +0100, Andrea Bolognani wrote:
Now that we have an explicit test case for the feature, we can drop a bunch of duplicated accidental coverage.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/qemuxmlconfdata/440fx-wrong-root.xml | 5 ----- .../cpu-host-model-features.x86_64-latest.xml | 5 ----- tests/qemuxmlconfdata/cpu-host-model-features.xml | 5 ----- .../cpu-host-passthrough-features.x86_64-latest.xml | 5 ----- tests/qemuxmlconfdata/cpu-host-passthrough-features.xml | 5 ----- tests/qemuxmlconfdata/cpu-tsc-frequency.x86_64-latest.xml | 5 ----- tests/qemuxmlconfdata/cpu-tsc-frequency.xml | 5 ----- tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.xml | 1 - tests/qemuxmlconfdata/disk-cdrom-bus-other.xml | 1 - tests/qemuxmlconfdata/minimal.x86_64-latest.xml | 5 ----- tests/qemuxmlconfdata/minimal.xml | 5 ----- tests/qemuxmlconfdata/missing-machine.xml | 1 - 12 files changed, 48 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/qemuxmlconfdata/minimal-no-memory.xml | 25 ------------------- ...latest.err => no-memory.x86_64-latest.err} | 0 tests/qemuxmlconfdata/no-memory.xml | 11 ++++++++ tests/qemuxmlconftest.c | 2 +- 4 files changed, 12 insertions(+), 26 deletions(-) delete mode 100644 tests/qemuxmlconfdata/minimal-no-memory.xml rename tests/qemuxmlconfdata/{minimal-no-memory.x86_64-latest.err => no-memory.x86_64-latest.err} (100%) create mode 100644 tests/qemuxmlconfdata/no-memory.xml diff --git a/tests/qemuxmlconfdata/minimal-no-memory.xml b/tests/qemuxmlconfdata/minimal-no-memory.xml deleted file mode 100644 index dfa2b6dd89..0000000000 --- a/tests/qemuxmlconfdata/minimal-no-memory.xml +++ /dev/null @@ -1,25 +0,0 @@ -<domain type='qemu'> - <name>QEMUGuest1</name> - <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> - <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu> - <os> - <type arch='x86_64' machine='pc'>hvm</type> - <boot dev='hd'/> - </os> - <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> - <disk type='block' device='disk'> - <source dev='/dev/HostVG/QEMUGuest1'/> - <target dev='hda' bus='ide'/> - <address type='drive' controller='0' bus='0' target='0' unit='0'/> - </disk> - <controller type='usb' index='0'/> - <controller type='ide' index='0'/> - <controller type='pci' index='0' model='pci-root'/> - <memballoon model='virtio'/> - </devices> -</domain> diff --git a/tests/qemuxmlconfdata/minimal-no-memory.x86_64-latest.err b/tests/qemuxmlconfdata/no-memory.x86_64-latest.err similarity index 100% rename from tests/qemuxmlconfdata/minimal-no-memory.x86_64-latest.err rename to tests/qemuxmlconfdata/no-memory.x86_64-latest.err diff --git a/tests/qemuxmlconfdata/no-memory.xml b/tests/qemuxmlconfdata/no-memory.xml new file mode 100644 index 0000000000..cff4f00f9d --- /dev/null +++ b/tests/qemuxmlconfdata/no-memory.xml @@ -0,0 +1,11 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <vcpu>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + </devices> +</domain> diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c index bd4e2c59fe..6ecfe64d97 100644 --- a/tests/qemuxmlconftest.c +++ b/tests/qemuxmlconftest.c @@ -1227,7 +1227,7 @@ mymain(void) g_unsetenv("PIPEWIRE_RUNTIME_DIR"); DO_TEST_CAPS_LATEST("minimal"); - DO_TEST_CAPS_LATEST_PARSE_ERROR("minimal-no-memory"); + DO_TEST_CAPS_LATEST_PARSE_ERROR("no-memory"); DO_TEST_CAPS_LATEST("title-and-description"); DO_TEST_CAPS_LATEST("genid"); -- 2.43.0

On Wed, Jan 24, 2024 at 20:37:25 +0100, Andrea Bolognani wrote:
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/qemuxmlconfdata/minimal-no-memory.xml | 25 ------------------- ...latest.err => no-memory.x86_64-latest.err} | 0 tests/qemuxmlconfdata/no-memory.xml | 11 ++++++++ tests/qemuxmlconftest.c | 2 +- 4 files changed, 12 insertions(+), 26 deletions(-) delete mode 100644 tests/qemuxmlconfdata/minimal-no-memory.xml rename tests/qemuxmlconfdata/{minimal-no-memory.x86_64-latest.err => no-memory.x86_64-latest.err} (100%) create mode 100644 tests/qemuxmlconfdata/no-memory.xml
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

We currently have a single test case called "minimal", which suffers from two big flaws: * it's limited to the x86_64/pc machine type; * it explicitly enables a number of devices. Add several test cases, one for each of the architectures and machine types that we have good support for. Unlike the existing one, they're *really* minimal: no devices or controllers at all are present in the input XML. So the new test cases demonstrate exactly what devices and controller libvirt will decide to add automatically. Note that we use the ABI_UPDATE variant of the test macros because, in some cases, the behavior for new guests is not the same as that for existing ones due to backward compatibility concerns, and we specifically care about the former. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- .../aarch64-virt-minimal.aarch64-latest.args | 31 ++++++++++++ .../aarch64-virt-minimal.aarch64-latest.xml | 26 ++++++++++ .../qemuxmlconfdata/aarch64-virt-minimal.xml | 12 +++++ .../ppc64-pseries-minimal.ppc64-latest.args | 33 ++++++++++++ .../ppc64-pseries-minimal.ppc64-latest.xml | 33 ++++++++++++ .../qemuxmlconfdata/ppc64-pseries-minimal.xml | 12 +++++ .../riscv64-virt-minimal.riscv64-latest.args | 33 ++++++++++++ .../riscv64-virt-minimal.riscv64-latest.xml | 33 ++++++++++++ .../qemuxmlconfdata/riscv64-virt-minimal.xml | 12 +++++ .../s390x-ccw-minimal.s390x-latest.args | 32 ++++++++++++ .../s390x-ccw-minimal.s390x-latest.xml | 27 ++++++++++ tests/qemuxmlconfdata/s390x-ccw-minimal.xml | 12 +++++ .../x86_64-pc-minimal.x86_64-latest.args | 33 ++++++++++++ .../x86_64-pc-minimal.x86_64-latest.xml | 31 ++++++++++++ tests/qemuxmlconfdata/x86_64-pc-minimal.xml | 12 +++++ .../x86_64-q35-minimal.x86_64-latest.args | 38 ++++++++++++++ .../x86_64-q35-minimal.x86_64-latest.xml | 50 +++++++++++++++++++ tests/qemuxmlconfdata/x86_64-q35-minimal.xml | 12 +++++ tests/qemuxmlconftest.c | 7 +++ 19 files changed, 479 insertions(+) create mode 100644 tests/qemuxmlconfdata/aarch64-virt-minimal.aarch64-latest.args create mode 100644 tests/qemuxmlconfdata/aarch64-virt-minimal.aarch64-latest.xml create mode 100644 tests/qemuxmlconfdata/aarch64-virt-minimal.xml create mode 100644 tests/qemuxmlconfdata/ppc64-pseries-minimal.ppc64-latest.args create mode 100644 tests/qemuxmlconfdata/ppc64-pseries-minimal.ppc64-latest.xml create mode 100644 tests/qemuxmlconfdata/ppc64-pseries-minimal.xml create mode 100644 tests/qemuxmlconfdata/riscv64-virt-minimal.riscv64-latest.args create mode 100644 tests/qemuxmlconfdata/riscv64-virt-minimal.riscv64-latest.xml create mode 100644 tests/qemuxmlconfdata/riscv64-virt-minimal.xml create mode 100644 tests/qemuxmlconfdata/s390x-ccw-minimal.s390x-latest.args create mode 100644 tests/qemuxmlconfdata/s390x-ccw-minimal.s390x-latest.xml create mode 100644 tests/qemuxmlconfdata/s390x-ccw-minimal.xml create mode 100644 tests/qemuxmlconfdata/x86_64-pc-minimal.x86_64-latest.args create mode 100644 tests/qemuxmlconfdata/x86_64-pc-minimal.x86_64-latest.xml create mode 100644 tests/qemuxmlconfdata/x86_64-pc-minimal.xml create mode 100644 tests/qemuxmlconfdata/x86_64-q35-minimal.x86_64-latest.args create mode 100644 tests/qemuxmlconfdata/x86_64-q35-minimal.x86_64-latest.xml create mode 100644 tests/qemuxmlconfdata/x86_64-q35-minimal.xml diff --git a/tests/qemuxmlconfdata/aarch64-virt-minimal.aarch64-latest.args b/tests/qemuxmlconfdata/aarch64-virt-minimal.aarch64-latest.args new file mode 100644 index 0000000000..51a196e03e --- /dev/null +++ b/tests/qemuxmlconfdata/aarch64-virt-minimal.aarch64-latest.args @@ -0,0 +1,31 @@ +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-aarch64 \ +-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 virt,usb=off,gic-version=2,dump-guest-core=off,memory-backend=mach-virt.ram,acpi=off \ +-accel tcg \ +-cpu cortex-a15 \ +-m size=4194304k \ +-object '{"qom-type":"memory-backend-ram","id":"mach-virt.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 \ +-audiodev '{"id":"audio1","driver":"none"}' \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxmlconfdata/aarch64-virt-minimal.aarch64-latest.xml b/tests/qemuxmlconfdata/aarch64-virt-minimal.aarch64-latest.xml new file mode 100644 index 0000000000..25b415d390 --- /dev/null +++ b/tests/qemuxmlconfdata/aarch64-virt-minimal.aarch64-latest.xml @@ -0,0 +1,26 @@ +<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='aarch64' machine='virt'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <gic version='2'/> + </features> + <cpu mode='custom' match='exact' check='none'> + <model fallback='forbid'>cortex-a15</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-aarch64</emulator> + <controller type='pci' index='0' model='pcie-root'/> + <audio id='1' type='none'/> + </devices> +</domain> diff --git a/tests/qemuxmlconfdata/aarch64-virt-minimal.xml b/tests/qemuxmlconfdata/aarch64-virt-minimal.xml new file mode 100644 index 0000000000..5b44c14131 --- /dev/null +++ b/tests/qemuxmlconfdata/aarch64-virt-minimal.xml @@ -0,0 +1,12 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory>4194304</memory> + <vcpu>4</vcpu> + <os> + <type arch='aarch64' machine='virt'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-system-aarch64</emulator> + </devices> +</domain> diff --git a/tests/qemuxmlconfdata/ppc64-pseries-minimal.ppc64-latest.args b/tests/qemuxmlconfdata/ppc64-pseries-minimal.ppc64-latest.args new file mode 100644 index 0000000000..5039957a47 --- /dev/null +++ b/tests/qemuxmlconfdata/ppc64-pseries-minimal.ppc64-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-ppc64 \ +-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 pseries,usb=off,dump-guest-core=off,memory-backend=ppc_spapr.ram \ +-accel tcg \ +-cpu POWER9 \ +-m size=4194304k \ +-object '{"qom-type":"memory-backend-ram","id":"ppc_spapr.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 \ +-device '{"driver":"qemu-xhci","id":"usb","bus":"pci.0","addr":"0x1"}' \ +-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/ppc64-pseries-minimal.ppc64-latest.xml b/tests/qemuxmlconfdata/ppc64-pseries-minimal.ppc64-latest.xml new file mode 100644 index 0000000000..bdb6b1e03d --- /dev/null +++ b/tests/qemuxmlconfdata/ppc64-pseries-minimal.ppc64-latest.xml @@ -0,0 +1,33 @@ +<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='ppc64' machine='pseries'>hvm</type> + <boot dev='hd'/> + </os> + <cpu mode='custom' match='exact' check='none'> + <model fallback='forbid'>POWER9</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-ppc64</emulator> + <controller type='usb' index='0' model='qemu-xhci'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller> + <controller type='pci' index='0' model='pci-root'> + <model name='spapr-pci-host-bridge'/> + <target index='0'/> + </controller> + <audio id='1' type='none'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </memballoon> + <panic model='pseries'/> + </devices> +</domain> diff --git a/tests/qemuxmlconfdata/ppc64-pseries-minimal.xml b/tests/qemuxmlconfdata/ppc64-pseries-minimal.xml new file mode 100644 index 0000000000..125e651519 --- /dev/null +++ b/tests/qemuxmlconfdata/ppc64-pseries-minimal.xml @@ -0,0 +1,12 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory>4194304</memory> + <vcpu>4</vcpu> + <os> + <type arch='ppc64' machine='pseries'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-system-ppc64</emulator> + </devices> +</domain> diff --git a/tests/qemuxmlconfdata/riscv64-virt-minimal.riscv64-latest.args b/tests/qemuxmlconfdata/riscv64-virt-minimal.riscv64-latest.args new file mode 100644 index 0000000000..fcb80b009e --- /dev/null +++ b/tests/qemuxmlconfdata/riscv64-virt-minimal.riscv64-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-riscv64 \ +-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 virt,usb=off,dump-guest-core=off,memory-backend=riscv_virt_board.ram \ +-accel tcg \ +-m size=4194304k \ +-object '{"qom-type":"memory-backend-ram","id":"riscv_virt_board.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 \ +-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"}' \ +-audiodev '{"id":"audio1","driver":"none"}' \ +-device '{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.1","addr":"0x0"}' \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxmlconfdata/riscv64-virt-minimal.riscv64-latest.xml b/tests/qemuxmlconfdata/riscv64-virt-minimal.riscv64-latest.xml new file mode 100644 index 0000000000..54363bb426 --- /dev/null +++ b/tests/qemuxmlconfdata/riscv64-virt-minimal.riscv64-latest.xml @@ -0,0 +1,33 @@ +<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='riscv64' machine='virt'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-riscv64</emulator> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='pcie-root-port'> + <model name='pcie-root-port'/> + <target chassis='1' port='0x8'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0' multifunction='on'/> + </controller> + <controller type='pci' index='2' model='pcie-root-port'> + <model name='pcie-root-port'/> + <target chassis='2' port='0x9'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <audio id='1' type='none'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxmlconfdata/riscv64-virt-minimal.xml b/tests/qemuxmlconfdata/riscv64-virt-minimal.xml new file mode 100644 index 0000000000..fb67b333c1 --- /dev/null +++ b/tests/qemuxmlconfdata/riscv64-virt-minimal.xml @@ -0,0 +1,12 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory>4194304</memory> + <vcpu>4</vcpu> + <os> + <type arch='riscv64' machine='virt'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-system-riscv64</emulator> + </devices> +</domain> diff --git a/tests/qemuxmlconfdata/s390x-ccw-minimal.s390x-latest.args b/tests/qemuxmlconfdata/s390x-ccw-minimal.s390x-latest.args new file mode 100644 index 0000000000..84098e580e --- /dev/null +++ b/tests/qemuxmlconfdata/s390x-ccw-minimal.s390x-latest.args @@ -0,0 +1,32 @@ +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-s390x \ +-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 s390-ccw-virtio,usb=off,dump-guest-core=off,memory-backend=s390.ram \ +-accel tcg \ +-cpu qemu \ +-m size=4194304k \ +-object '{"qom-type":"memory-backend-ram","id":"s390.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 \ +-audiodev '{"id":"audio1","driver":"none"}' \ +-device '{"driver":"virtio-balloon-ccw","id":"balloon0","devno":"fe.0.0000"}' \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxmlconfdata/s390x-ccw-minimal.s390x-latest.xml b/tests/qemuxmlconfdata/s390x-ccw-minimal.s390x-latest.xml new file mode 100644 index 0000000000..df8e578212 --- /dev/null +++ b/tests/qemuxmlconfdata/s390x-ccw-minimal.s390x-latest.xml @@ -0,0 +1,27 @@ +<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='s390x' machine='s390-ccw-virtio'>hvm</type> + <boot dev='hd'/> + </os> + <cpu mode='custom' match='exact' check='none'> + <model fallback='forbid'>qemu</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-s390x</emulator> + <controller type='pci' index='0' model='pci-root'/> + <audio id='1' type='none'/> + <memballoon model='virtio'> + <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0000'/> + </memballoon> + <panic model='s390'/> + </devices> +</domain> diff --git a/tests/qemuxmlconfdata/s390x-ccw-minimal.xml b/tests/qemuxmlconfdata/s390x-ccw-minimal.xml new file mode 100644 index 0000000000..3f5202bfdd --- /dev/null +++ b/tests/qemuxmlconfdata/s390x-ccw-minimal.xml @@ -0,0 +1,12 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory>4194304</memory> + <vcpu>4</vcpu> + <os> + <type arch='s390x' machine='s390-ccw-virtio'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-system-s390x</emulator> + </devices> +</domain> diff --git a/tests/qemuxmlconfdata/x86_64-pc-minimal.x86_64-latest.args b/tests/qemuxmlconfdata/x86_64-pc-minimal.x86_64-latest.args new file mode 100644 index 0000000000..606fd70519 --- /dev/null +++ b/tests/qemuxmlconfdata/x86_64-pc-minimal.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 \ +-device '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \ +-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/x86_64-pc-minimal.x86_64-latest.xml b/tests/qemuxmlconfdata/x86_64-pc-minimal.x86_64-latest.xml new file mode 100644 index 0000000000..3fde74460c --- /dev/null +++ b/tests/qemuxmlconfdata/x86_64-pc-minimal.x86_64-latest.xml @@ -0,0 +1,31 @@ +<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' model='piix3-uhci'> + <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/x86_64-pc-minimal.xml b/tests/qemuxmlconfdata/x86_64-pc-minimal.xml new file mode 100644 index 0000000000..33cebaebd8 --- /dev/null +++ b/tests/qemuxmlconfdata/x86_64-pc-minimal.xml @@ -0,0 +1,12 @@ +<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> + </devices> +</domain> diff --git a/tests/qemuxmlconfdata/x86_64-q35-minimal.x86_64-latest.args b/tests/qemuxmlconfdata/x86_64-q35-minimal.x86_64-latest.args new file mode 100644 index 0000000000..9566106b94 --- /dev/null +++ b/tests/qemuxmlconfdata/x86_64-q35-minimal.x86_64-latest.args @@ -0,0 +1,38 @@ +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 q35,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 \ +-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-root-port","port":10,"chassis":3,"id":"pci.3","bus":"pcie.0","addr":"0x1.0x2"}' \ +-device '{"driver":"qemu-xhci","id":"usb","bus":"pci.1","addr":"0x0"}' \ +-audiodev '{"id":"audio1","driver":"none"}' \ +-global ICH9-LPC.noreboot=off \ +-watchdog-action reset \ +-device '{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.2","addr":"0x0"}' \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxmlconfdata/x86_64-q35-minimal.x86_64-latest.xml b/tests/qemuxmlconfdata/x86_64-q35-minimal.x86_64-latest.xml new file mode 100644 index 0000000000..10d245e3b5 --- /dev/null +++ b/tests/qemuxmlconfdata/x86_64-q35-minimal.x86_64-latest.xml @@ -0,0 +1,50 @@ +<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='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='usb' index='0' model='qemu-xhci'> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </controller> + <controller type='sata' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='pcie-root-port'> + <model name='pcie-root-port'/> + <target chassis='1' port='0x8'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0' multifunction='on'/> + </controller> + <controller type='pci' index='2' model='pcie-root-port'> + <model name='pcie-root-port'/> + <target chassis='2' port='0x9'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='pci' index='3' model='pcie-root-port'> + <model name='pcie-root-port'/> + <target chassis='3' port='0xa'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' 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='virtio'> + <address type='pci' domain='0x0000' bus='0x02' slot='0x00' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxmlconfdata/x86_64-q35-minimal.xml b/tests/qemuxmlconfdata/x86_64-q35-minimal.xml new file mode 100644 index 0000000000..31eba6d70f --- /dev/null +++ b/tests/qemuxmlconfdata/x86_64-q35-minimal.xml @@ -0,0 +1,12 @@ +<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='q35'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + </devices> +</domain> diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c index 6ecfe64d97..c80bcd9b4d 100644 --- a/tests/qemuxmlconftest.c +++ b/tests/qemuxmlconftest.c @@ -1226,6 +1226,13 @@ mymain(void) g_unsetenv("PIPEWIRE_REMOTE"); g_unsetenv("PIPEWIRE_RUNTIME_DIR"); + DO_TEST_CAPS_LATEST_ABI_UPDATE("x86_64-pc-minimal"); + DO_TEST_CAPS_LATEST_ABI_UPDATE("x86_64-q35-minimal"); + DO_TEST_CAPS_ARCH_LATEST_ABI_UPDATE("aarch64-virt-minimal", "aarch64"); + DO_TEST_CAPS_ARCH_LATEST_ABI_UPDATE("riscv64-virt-minimal", "riscv64"); + DO_TEST_CAPS_ARCH_LATEST_ABI_UPDATE("ppc64-pseries-minimal", "ppc64"); + DO_TEST_CAPS_ARCH_LATEST_ABI_UPDATE("s390x-ccw-minimal", "s390x"); + DO_TEST_CAPS_LATEST("minimal"); DO_TEST_CAPS_LATEST_PARSE_ERROR("no-memory"); DO_TEST_CAPS_LATEST("title-and-description"); -- 2.43.0

On Wed, Jan 24, 2024 at 20:37:26 +0100, Andrea Bolognani wrote:
We currently have a single test case called "minimal", which suffers from two big flaws:
* it's limited to the x86_64/pc machine type; * it explicitly enables a number of devices.
Add several test cases, one for each of the architectures and machine types that we have good support for.
Unlike the existing one, they're *really* minimal: no devices or controllers at all are present in the input XML. So the new test cases demonstrate exactly what devices and controller libvirt will decide to add automatically.
Note that we use the ABI_UPDATE variant of the test macros because, in some cases, the behavior for new guests is not the same as that for existing ones due to backward compatibility concerns, and we specifically care about the former.
IMO it would make sense to also add the non-ABI update cases.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- .../aarch64-virt-minimal.aarch64-latest.args | 31 ++++++++++++ .../aarch64-virt-minimal.aarch64-latest.xml | 26 ++++++++++ .../qemuxmlconfdata/aarch64-virt-minimal.xml | 12 +++++ .../ppc64-pseries-minimal.ppc64-latest.args | 33 ++++++++++++ .../ppc64-pseries-minimal.ppc64-latest.xml | 33 ++++++++++++ .../qemuxmlconfdata/ppc64-pseries-minimal.xml | 12 +++++ .../riscv64-virt-minimal.riscv64-latest.args | 33 ++++++++++++ .../riscv64-virt-minimal.riscv64-latest.xml | 33 ++++++++++++ .../qemuxmlconfdata/riscv64-virt-minimal.xml | 12 +++++ .../s390x-ccw-minimal.s390x-latest.args | 32 ++++++++++++ .../s390x-ccw-minimal.s390x-latest.xml | 27 ++++++++++ tests/qemuxmlconfdata/s390x-ccw-minimal.xml | 12 +++++ .../x86_64-pc-minimal.x86_64-latest.args | 33 ++++++++++++ .../x86_64-pc-minimal.x86_64-latest.xml | 31 ++++++++++++ tests/qemuxmlconfdata/x86_64-pc-minimal.xml | 12 +++++ .../x86_64-q35-minimal.x86_64-latest.args | 38 ++++++++++++++ .../x86_64-q35-minimal.x86_64-latest.xml | 50 +++++++++++++++++++ tests/qemuxmlconfdata/x86_64-q35-minimal.xml | 12 +++++
even if you decide to do the above, you can use Reviewed-by: Peter Krempa <pkrempa@redhat.com> without reposting.

On Thu, Jan 25, 2024 at 10:46:39AM +0100, Peter Krempa wrote:
On Wed, Jan 24, 2024 at 20:37:26 +0100, Andrea Bolognani wrote:
We currently have a single test case called "minimal", which suffers from two big flaws:
* it's limited to the x86_64/pc machine type; * it explicitly enables a number of devices.
Add several test cases, one for each of the architectures and machine types that we have good support for.
Unlike the existing one, they're *really* minimal: no devices or controllers at all are present in the input XML. So the new test cases demonstrate exactly what devices and controller libvirt will decide to add automatically.
Note that we use the ABI_UPDATE variant of the test macros because, in some cases, the behavior for new guests is not the same as that for existing ones due to backward compatibility concerns, and we specifically care about the former.
IMO it would make sense to also add the non-ABI update cases.
Agreed.
even if you decide to do the above, you can use
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
without reposting.
In order to flush things a bit, my plan would be to push patches tests: Add usb-controller-automatic-unavailable-q35 tests: Add aarch64-panic-no-model tests: Add title-and-description tests: Drop existing <title> and <description> tags tests: Rename and minimize no-memory tests: Add minimal cases for many architectures tests: Drop minimal tests: Add default-models cases for many architectures qemu: Fix a few comments qemu: Default to no USB and no memballoon for new architectures qemu: Clean up qemuDomainDefaultNetModel() qemu: Drop qemuDomainFindSCSIControllerModel() qemu: Drop qemuDomainSetSCSIControllerModel() qemu: Add missing error handling qemu: Move qemuDomainGetSCSIControllerModel() that is, 01-14 and 16, with the following simple changes based on your suggestions: * add title-and-description to genericxml2xmltest instead of qemuxmlconftest; * have both ABI_UPDATE and regular variants of the minimal qemuxmlconftest cases, as well as the default-models cases. Can I go ahead without reposting? -- Andrea Bolognani / Red Hat / Virtualization

In order to flush things a bit, my plan would be to push patches
tests: Add usb-controller-automatic-unavailable-q35 tests: Add aarch64-panic-no-model tests: Add title-and-description tests: Drop existing <title> and <description> tags tests: Rename and minimize no-memory tests: Add minimal cases for many architectures tests: Drop minimal tests: Add default-models cases for many architectures qemu: Fix a few comments qemu: Default to no USB and no memballoon for new architectures qemu: Clean up qemuDomainDefaultNetModel() qemu: Drop qemuDomainFindSCSIControllerModel() qemu: Drop qemuDomainSetSCSIControllerModel() qemu: Add missing error handling qemu: Move qemuDomainGetSCSIControllerModel()
that is, 01-14 and 16, with the following simple changes based on your suggestions:
* add title-and-description to genericxml2xmltest instead of qemuxmlconftest;
* have both ABI_UPDATE and regular variants of the minimal qemuxmlconftest cases, as well as the default-models cases.
Can I go ahead without reposting?
Sure, go ahead.

We have just added a number of test cases that supersede it. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- .../minimal.x86_64-latest.args | 36 ----------------- .../qemuxmlconfdata/minimal.x86_64-latest.xml | 40 ------------------- tests/qemuxmlconfdata/minimal.xml | 29 -------------- tests/qemuxmlconftest.c | 1 - 4 files changed, 106 deletions(-) delete mode 100644 tests/qemuxmlconfdata/minimal.x86_64-latest.args delete mode 100644 tests/qemuxmlconfdata/minimal.x86_64-latest.xml delete mode 100644 tests/qemuxmlconfdata/minimal.xml diff --git a/tests/qemuxmlconfdata/minimal.x86_64-latest.args b/tests/qemuxmlconfdata/minimal.x86_64-latest.args deleted file mode 100644 index 486a07b157..0000000000 --- a/tests/qemuxmlconfdata/minimal.x86_64-latest.args +++ /dev/null @@ -1,36 +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 \ --device '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \ --blockdev '{"driver":"host_device","filename":"/dev/HostVG/QEMUGuest1","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":"ide-hd","bus":"ide.0","unit":0,"drive":"libvirt-1-format","id":"ide0-0-0","bootindex":1}' \ --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/minimal.x86_64-latest.xml b/tests/qemuxmlconfdata/minimal.x86_64-latest.xml deleted file mode 100644 index ffa03532bd..0000000000 --- a/tests/qemuxmlconfdata/minimal.x86_64-latest.xml +++ /dev/null @@ -1,40 +0,0 @@ -<domain type='qemu'> - <name>QEMUGuest1</name> - <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> - <memory unit='KiB'>219100</memory> - <currentMemory unit='KiB'>219100</currentMemory> - <vcpu placement='static' cpuset='1-4,8-20,525'>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> - <disk type='block' device='disk'> - <driver name='qemu' type='raw'/> - <source dev='/dev/HostVG/QEMUGuest1'/> - <target dev='hda' bus='ide'/> - <address type='drive' controller='0' bus='0' target='0' unit='0'/> - </disk> - <controller type='usb' index='0' model='piix3-uhci'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> - </controller> - <controller type='ide' index='0'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> - </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/minimal.xml b/tests/qemuxmlconfdata/minimal.xml deleted file mode 100644 index a77240d87c..0000000000 --- a/tests/qemuxmlconfdata/minimal.xml +++ /dev/null @@ -1,29 +0,0 @@ -<domain type='qemu'> - <name>QEMUGuest1</name> - <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> - <memory unit='KiB'>219100</memory> - <currentMemory unit='KiB'>219100</currentMemory> - <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu> - <os> - <type arch='x86_64' machine='pc'>hvm</type> - <boot dev='hd'/> - </os> - <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> - <disk type='block' device='disk'> - <source dev='/dev/HostVG/QEMUGuest1'/> - <target dev='hda' bus='ide'/> - <address type='drive' controller='0' bus='0' target='0' unit='0'/> - </disk> - <controller type='usb' index='0'/> - <controller type='ide' index='0'/> - <controller type='pci' index='0' model='pci-root'/> - <input type='mouse' bus='ps2'/> - <input type='keyboard' bus='ps2'/> - <memballoon model='virtio'/> - </devices> -</domain> diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c index c80bcd9b4d..445e6ff5e1 100644 --- a/tests/qemuxmlconftest.c +++ b/tests/qemuxmlconftest.c @@ -1233,7 +1233,6 @@ mymain(void) DO_TEST_CAPS_ARCH_LATEST_ABI_UPDATE("ppc64-pseries-minimal", "ppc64"); DO_TEST_CAPS_ARCH_LATEST_ABI_UPDATE("s390x-ccw-minimal", "s390x"); - DO_TEST_CAPS_LATEST("minimal"); DO_TEST_CAPS_LATEST_PARSE_ERROR("no-memory"); DO_TEST_CAPS_LATEST("title-and-description"); -- 2.43.0

On Wed, Jan 24, 2024 at 20:37:27 +0100, Andrea Bolognani wrote:
We have just added a number of test cases that supersede it.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- .../minimal.x86_64-latest.args | 36 ----------------- .../qemuxmlconfdata/minimal.x86_64-latest.xml | 40 ------------------- tests/qemuxmlconfdata/minimal.xml | 29 -------------- tests/qemuxmlconftest.c | 1 - 4 files changed, 106 deletions(-) delete mode 100644 tests/qemuxmlconfdata/minimal.x86_64-latest.args delete mode 100644 tests/qemuxmlconfdata/minimal.x86_64-latest.xml delete mode 100644 tests/qemuxmlconfdata/minimal.xml
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

These are similar to the minimal cases that we just introduced, but are intended to demonstrate what device or controller model libvirt will choose when one is not provided by the user. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- ...64-virt-default-models.aarch64-latest.args | 44 +++++++++++ ...h64-virt-default-models.aarch64-latest.xml | 79 +++++++++++++++++++ .../aarch64-virt-default-models.xml | 21 +++++ ...4-pseries-default-models.ppc64-latest.args | 38 +++++++++ ...64-pseries-default-models.ppc64-latest.xml | 53 +++++++++++++ .../ppc64-pseries-default-models.xml | 21 +++++ ...64-virt-default-models.riscv64-latest.args | 42 ++++++++++ ...v64-virt-default-models.riscv64-latest.xml | 68 ++++++++++++++++ .../riscv64-virt-default-models.xml | 21 +++++ ...s390x-ccw-default-models.s390x-latest.args | 37 +++++++++ .../s390x-ccw-default-models.s390x-latest.xml | 46 +++++++++++ .../s390x-ccw-default-models.xml | 21 +++++ ...86_64-pc-default-models.x86_64-latest.args | 39 +++++++++ ...x86_64-pc-default-models.x86_64-latest.xml | 50 ++++++++++++ .../x86_64-pc-default-models.xml | 21 +++++ ...6_64-q35-default-models.x86_64-latest.args | 44 +++++++++++ ...86_64-q35-default-models.x86_64-latest.xml | 68 ++++++++++++++++ .../x86_64-q35-default-models.xml | 21 +++++ tests/qemuxmlconftest.c | 7 ++ 19 files changed, 741 insertions(+) create mode 100644 tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.args create mode 100644 tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.xml create mode 100644 tests/qemuxmlconfdata/aarch64-virt-default-models.xml create mode 100644 tests/qemuxmlconfdata/ppc64-pseries-default-models.ppc64-latest.args create mode 100644 tests/qemuxmlconfdata/ppc64-pseries-default-models.ppc64-latest.xml create mode 100644 tests/qemuxmlconfdata/ppc64-pseries-default-models.xml create mode 100644 tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.args create mode 100644 tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.xml create mode 100644 tests/qemuxmlconfdata/riscv64-virt-default-models.xml create mode 100644 tests/qemuxmlconfdata/s390x-ccw-default-models.s390x-latest.args create mode 100644 tests/qemuxmlconfdata/s390x-ccw-default-models.s390x-latest.xml create mode 100644 tests/qemuxmlconfdata/s390x-ccw-default-models.xml create mode 100644 tests/qemuxmlconfdata/x86_64-pc-default-models.x86_64-latest.args create mode 100644 tests/qemuxmlconfdata/x86_64-pc-default-models.x86_64-latest.xml create mode 100644 tests/qemuxmlconfdata/x86_64-pc-default-models.xml create mode 100644 tests/qemuxmlconfdata/x86_64-q35-default-models.x86_64-latest.args create mode 100644 tests/qemuxmlconfdata/x86_64-q35-default-models.x86_64-latest.xml create mode 100644 tests/qemuxmlconfdata/x86_64-q35-default-models.xml diff --git a/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.args b/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.args new file mode 100644 index 0000000000..0c4acf800f --- /dev/null +++ b/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.args @@ -0,0 +1,44 @@ +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-aarch64 \ +-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 virt,usb=off,gic-version=2,dump-guest-core=off,memory-backend=mach-virt.ram,acpi=off \ +-accel tcg \ +-cpu cortex-a15 \ +-m size=4194304k \ +-object '{"qom-type":"memory-backend-ram","id":"mach-virt.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 \ +-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-root-port","port":10,"chassis":3,"id":"pci.3","bus":"pcie.0","addr":"0x1.0x2"}' \ +-device '{"driver":"pcie-pci-bridge","id":"pci.4","bus":"pci.1","addr":"0x0"}' \ +-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":"qemu-xhci","id":"usb","bus":"pci.3","addr":"0x0"}' \ +-device '{"driver":"lsi","id":"scsi0","bus":"pci.4","addr":"0x1"}' \ +-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-gpu-pci","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/aarch64-virt-default-models.aarch64-latest.xml b/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.xml new file mode 100644 index 0000000000..87be062c89 --- /dev/null +++ b/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.xml @@ -0,0 +1,79 @@ +<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='aarch64' machine='virt'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <gic version='2'/> + </features> + <cpu mode='custom' match='exact' check='none'> + <model fallback='forbid'>cortex-a15</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-aarch64</emulator> + <controller type='usb' index='0' model='qemu-xhci'> + <address type='pci' domain='0x0000' bus='0x03' slot='0x00' function='0x0'/> + </controller> + <controller type='scsi' index='0' model='lsilogic'> + <address type='pci' domain='0x0000' bus='0x04' slot='0x01' function='0x0'/> + </controller> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='pcie-root-port'> + <model name='pcie-root-port'/> + <target chassis='1' port='0x8'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0' multifunction='on'/> + </controller> + <controller type='pci' index='2' model='pcie-root-port'> + <model name='pcie-root-port'/> + <target chassis='2' port='0x9'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='pci' index='3' model='pcie-root-port'> + <model name='pcie-root-port'/> + <target chassis='3' port='0xa'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='4' 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='5' model='pcie-root-port'> + <model name='pcie-root-port'/> + <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'/> + <address type='pci' domain='0x0000' bus='0x02' slot='0x00' function='0x0'/> + </interface> + <serial type='pty'> + <target type='system-serial' port='0'> + <model name='pl011'/> + </target> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + </console> + <audio id='1' type='none'/> + <video> + <model type='virtio' heads='1' primary='yes'/> + <address type='pci' domain='0x0000' bus='0x05' slot='0x00' function='0x0'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxmlconfdata/aarch64-virt-default-models.xml b/tests/qemuxmlconfdata/aarch64-virt-default-models.xml new file mode 100644 index 0000000000..cf7f330c0b --- /dev/null +++ b/tests/qemuxmlconfdata/aarch64-virt-default-models.xml @@ -0,0 +1,21 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory>4194304</memory> + <vcpu>4</vcpu> + <os> + <type arch='aarch64' machine='virt'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-system-aarch64</emulator> + <controller type='usb'/> + <controller type='scsi'/> + <interface type='user'> + <mac address='52:54:00:09:a4:37'/> + </interface> + <serial type='pty'/> + <video/> + <memballoon model='none'/> + <!-- No default model for <panic/> on aarch64 --> + </devices> +</domain> diff --git a/tests/qemuxmlconfdata/ppc64-pseries-default-models.ppc64-latest.args b/tests/qemuxmlconfdata/ppc64-pseries-default-models.ppc64-latest.args new file mode 100644 index 0000000000..1395f19bfe --- /dev/null +++ b/tests/qemuxmlconfdata/ppc64-pseries-default-models.ppc64-latest.args @@ -0,0 +1,38 @@ +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-ppc64 \ +-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 pseries,usb=off,dump-guest-core=off,memory-backend=ppc_spapr.ram \ +-accel tcg \ +-cpu POWER9 \ +-m size=4194304k \ +-object '{"qom-type":"memory-backend-ram","id":"ppc_spapr.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 \ +-device '{"driver":"qemu-xhci","id":"usb","bus":"pci.0","addr":"0x2"}' \ +-device '{"driver":"spapr-vscsi","id":"scsi0","reg":8192}' \ +-netdev user,id=hostnet0 \ +-device '{"driver":"rtl8139","netdev":"hostnet0","id":"net0","mac":"52:54:00:09:a4:37","bus":"pci.0","addr":"0x1"}' \ +-chardev pty,id=charserial0 \ +-device '{"driver":"spapr-vty","chardev":"charserial0","id":"serial0","reg":805306368}' \ +-audiodev '{"id":"audio1","driver":"none"}' \ +-device '{"driver":"VGA","id":"video0","vgamem_mb":16,"bus":"pci.0","addr":"0x3"}' \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxmlconfdata/ppc64-pseries-default-models.ppc64-latest.xml b/tests/qemuxmlconfdata/ppc64-pseries-default-models.ppc64-latest.xml new file mode 100644 index 0000000000..2304c6f786 --- /dev/null +++ b/tests/qemuxmlconfdata/ppc64-pseries-default-models.ppc64-latest.xml @@ -0,0 +1,53 @@ +<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='ppc64' machine='pseries'>hvm</type> + <boot dev='hd'/> + </os> + <cpu mode='custom' match='exact' check='none'> + <model fallback='forbid'>POWER9</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-ppc64</emulator> + <controller type='usb' index='0' model='qemu-xhci'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </controller> + <controller type='scsi' index='0' model='ibmvscsi'> + <address type='spapr-vio' reg='0x00002000'/> + </controller> + <controller type='pci' index='0' model='pci-root'> + <model name='spapr-pci-host-bridge'/> + <target index='0'/> + </controller> + <interface type='user'> + <mac address='52:54:00:09:a4:37'/> + <model type='rtl8139'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </interface> + <serial type='pty'> + <target type='spapr-vio-serial' port='0'> + <model name='spapr-vty'/> + </target> + <address type='spapr-vio' reg='0x30000000'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + <address type='spapr-vio' reg='0x30000000'/> + </console> + <audio id='1' type='none'/> + <video> + <model type='vga' vram='16384' heads='1' primary='yes'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </video> + <memballoon model='none'/> + <panic model='pseries'/> + </devices> +</domain> diff --git a/tests/qemuxmlconfdata/ppc64-pseries-default-models.xml b/tests/qemuxmlconfdata/ppc64-pseries-default-models.xml new file mode 100644 index 0000000000..4c2d16f01a --- /dev/null +++ b/tests/qemuxmlconfdata/ppc64-pseries-default-models.xml @@ -0,0 +1,21 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory>4194304</memory> + <vcpu>4</vcpu> + <os> + <type arch='ppc64' machine='pseries'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-system-ppc64</emulator> + <controller type='usb'/> + <controller type='scsi'/> + <interface type='user'> + <mac address='52:54:00:09:a4:37'/> + </interface> + <serial type='pty'/> + <video/> + <memballoon model='none'/> + <panic/> + </devices> +</domain> diff --git a/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.args b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.args new file mode 100644 index 0000000000..28b56d876c --- /dev/null +++ b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.args @@ -0,0 +1,42 @@ +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-riscv64 \ +-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 virt,usb=off,dump-guest-core=off,memory-backend=riscv_virt_board.ram \ +-accel tcg \ +-m size=4194304k \ +-object '{"qom-type":"memory-backend-ram","id":"riscv_virt_board.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 \ +-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":"piix3-usb-uhci","id":"usb","bus":"pci.3","addr":"0x1"}' \ +-device '{"driver":"lsi","id":"scsi0","bus":"pci.3","addr":"0x2"}' \ +-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"}' \ +-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 new file mode 100644 index 0000000000..942bd21f9e --- /dev/null +++ b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.xml @@ -0,0 +1,68 @@ +<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='riscv64' machine='virt'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <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> + <controller type='scsi' index='0' model='lsilogic'> + <address type='pci' domain='0x0000' bus='0x03' slot='0x02' function='0x0'/> + </controller> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='pcie-root-port'> + <model name='pcie-root-port'/> + <target chassis='1' port='0x8'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0' multifunction='on'/> + </controller> + <controller type='pci' index='2' model='pcie-root-port'> + <model name='pcie-root-port'/> + <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'> + <model name='pcie-root-port'/> + <target chassis='4' port='0xa'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='5' model='pcie-root-port'> + <model name='pcie-root-port'/> + <target chassis='5' port='0xb'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x3'/> + </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'/> + </interface> + <serial type='pty'> + <target type='system-serial' port='0'> + <model name='16550a'/> + </target> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + </console> + <audio id='1' type='none'/> + <video> + <model type='virtio' heads='1' primary='yes'/> + <address type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x0'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxmlconfdata/riscv64-virt-default-models.xml b/tests/qemuxmlconfdata/riscv64-virt-default-models.xml new file mode 100644 index 0000000000..d421b080a8 --- /dev/null +++ b/tests/qemuxmlconfdata/riscv64-virt-default-models.xml @@ -0,0 +1,21 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory>4194304</memory> + <vcpu>4</vcpu> + <os> + <type arch='riscv64' machine='virt'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-system-riscv64</emulator> + <controller type='usb'/> + <controller type='scsi'/> + <interface type='user'> + <mac address='52:54:00:09:a4:37'/> + </interface> + <serial type='pty'/> + <video/> + <memballoon model='none'/> + <!-- No default model for <panic/> on riscv64 --> + </devices> +</domain> diff --git a/tests/qemuxmlconfdata/s390x-ccw-default-models.s390x-latest.args b/tests/qemuxmlconfdata/s390x-ccw-default-models.s390x-latest.args new file mode 100644 index 0000000000..4c27502208 --- /dev/null +++ b/tests/qemuxmlconfdata/s390x-ccw-default-models.s390x-latest.args @@ -0,0 +1,37 @@ +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-s390x \ +-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 s390-ccw-virtio,usb=off,dump-guest-core=off,memory-backend=s390.ram \ +-accel tcg \ +-cpu qemu \ +-m size=4194304k \ +-object '{"qom-type":"memory-backend-ram","id":"s390.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 \ +-device '{"driver":"virtio-scsi-ccw","id":"scsi0","devno":"fe.0.0002"}' \ +-netdev '{"type":"user","id":"hostnet0"}' \ +-device '{"driver":"virtio-net-ccw","netdev":"hostnet0","id":"net0","mac":"52:54:00:09:a4:37","devno":"fe.0.0000"}' \ +-chardev pty,id=charserial0 \ +-device '{"driver":"sclpconsole","chardev":"charserial0","id":"serial0"}' \ +-audiodev '{"id":"audio1","driver":"none"}' \ +-device '{"driver":"virtio-gpu-ccw","id":"video0","max_outputs":1,"devno":"fe.0.0001"}' \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxmlconfdata/s390x-ccw-default-models.s390x-latest.xml b/tests/qemuxmlconfdata/s390x-ccw-default-models.s390x-latest.xml new file mode 100644 index 0000000000..9e0af389a9 --- /dev/null +++ b/tests/qemuxmlconfdata/s390x-ccw-default-models.s390x-latest.xml @@ -0,0 +1,46 @@ +<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='s390x' machine='s390-ccw-virtio'>hvm</type> + <boot dev='hd'/> + </os> + <cpu mode='custom' match='exact' check='none'> + <model fallback='forbid'>qemu</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-s390x</emulator> + <controller type='usb' index='0' model='none'/> + <controller type='scsi' index='0' model='virtio-scsi'> + <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0002'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <interface type='user'> + <mac address='52:54:00:09:a4:37'/> + <model type='virtio'/> + <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0000'/> + </interface> + <serial type='pty'> + <target type='sclp-serial' port='0'> + <model name='sclpconsole'/> + </target> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + </console> + <audio id='1' type='none'/> + <video> + <model type='virtio' heads='1' primary='yes'/> + <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0001'/> + </video> + <memballoon model='none'/> + <panic model='s390'/> + </devices> +</domain> diff --git a/tests/qemuxmlconfdata/s390x-ccw-default-models.xml b/tests/qemuxmlconfdata/s390x-ccw-default-models.xml new file mode 100644 index 0000000000..a196129628 --- /dev/null +++ b/tests/qemuxmlconfdata/s390x-ccw-default-models.xml @@ -0,0 +1,21 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory>4194304</memory> + <vcpu>4</vcpu> + <os> + <type arch='s390x' machine='s390-ccw-virtio'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-system-s390x</emulator> + <controller type='usb'/> + <controller type='scsi'/> + <interface type='user'> + <mac address='52:54:00:09:a4:37'/> + </interface> + <serial type='pty'/> + <video/> + <memballoon model='none'/> + <panic/> + </devices> +</domain> diff --git a/tests/qemuxmlconfdata/x86_64-pc-default-models.x86_64-latest.args b/tests/qemuxmlconfdata/x86_64-pc-default-models.x86_64-latest.args new file mode 100644 index 0000000000..3220a40959 --- /dev/null +++ b/tests/qemuxmlconfdata/x86_64-pc-default-models.x86_64-latest.args @@ -0,0 +1,39 @@ +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 \ +-device '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \ +-device '{"driver":"lsi","id":"scsi0","bus":"pci.0","addr":"0x4"}' \ +-netdev '{"type":"user","id":"hostnet0"}' \ +-device '{"driver":"rtl8139","netdev":"hostnet0","id":"net0","mac":"52:54:00:09:a4:37","bus":"pci.0","addr":"0x3"}' \ +-chardev pty,id=charserial0 \ +-device '{"driver":"isa-serial","chardev":"charserial0","id":"serial0","index":0}' \ +-audiodev '{"id":"audio1","driver":"none"}' \ +-device '{"driver":"cirrus-vga","id":"video0","bus":"pci.0","addr":"0x2"}' \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ +-device '{"driver":"pvpanic"}' \ +-msg timestamp=on diff --git a/tests/qemuxmlconfdata/x86_64-pc-default-models.x86_64-latest.xml b/tests/qemuxmlconfdata/x86_64-pc-default-models.x86_64-latest.xml new file mode 100644 index 0000000000..dc563fdaf9 --- /dev/null +++ b/tests/qemuxmlconfdata/x86_64-pc-default-models.x86_64-latest.xml @@ -0,0 +1,50 @@ +<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' model='piix3-uhci'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='scsi' index='0' model='lsilogic'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <interface type='user'> + <mac address='52:54:00:09:a4:37'/> + <model type='rtl8139'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </interface> + <serial type='pty'> + <target type='isa-serial' port='0'> + <model name='isa-serial'/> + </target> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + </console> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <audio id='1' type='none'/> + <video> + <model type='cirrus' vram='16384' heads='1' primary='yes'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </video> + <memballoon model='none'/> + <panic model='isa'/> + </devices> +</domain> diff --git a/tests/qemuxmlconfdata/x86_64-pc-default-models.xml b/tests/qemuxmlconfdata/x86_64-pc-default-models.xml new file mode 100644 index 0000000000..6727d2f6a0 --- /dev/null +++ b/tests/qemuxmlconfdata/x86_64-pc-default-models.xml @@ -0,0 +1,21 @@ +<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='scsi'/> + <interface type='user'> + <mac address='52:54:00:09:a4:37'/> + </interface> + <serial type='pty'/> + <video/> + <memballoon model='none'/> + <panic/> + </devices> +</domain> diff --git a/tests/qemuxmlconfdata/x86_64-q35-default-models.x86_64-latest.args b/tests/qemuxmlconfdata/x86_64-q35-default-models.x86_64-latest.args new file mode 100644 index 0000000000..b9905c6446 --- /dev/null +++ b/tests/qemuxmlconfdata/x86_64-q35-default-models.x86_64-latest.args @@ -0,0 +1,44 @@ +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 q35,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 \ +-device '{"driver":"pcie-root-port","port":16,"chassis":1,"id":"pci.1","bus":"pcie.0","multifunction":true,"addr":"0x2"}' \ +-device '{"driver":"pcie-pci-bridge","id":"pci.2","bus":"pci.1","addr":"0x0"}' \ +-device '{"driver":"pcie-root-port","port":17,"chassis":3,"id":"pci.3","bus":"pcie.0","addr":"0x2.0x1"}' \ +-device '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.2","addr":"0x2"}' \ +-device '{"driver":"lsi","id":"scsi0","bus":"pci.2","addr":"0x3"}' \ +-netdev '{"type":"user","id":"hostnet0"}' \ +-device '{"driver":"rtl8139","netdev":"hostnet0","id":"net0","mac":"52:54:00:09:a4:37","bus":"pci.2","addr":"0x1"}' \ +-chardev pty,id=charserial0 \ +-device '{"driver":"isa-serial","chardev":"charserial0","id":"serial0","index":0}' \ +-audiodev '{"id":"audio1","driver":"none"}' \ +-device '{"driver":"cirrus-vga","id":"video0","bus":"pcie.0","addr":"0x1"}' \ +-global ICH9-LPC.noreboot=off \ +-watchdog-action reset \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ +-device '{"driver":"pvpanic"}' \ +-msg timestamp=on diff --git a/tests/qemuxmlconfdata/x86_64-q35-default-models.x86_64-latest.xml b/tests/qemuxmlconfdata/x86_64-q35-default-models.x86_64-latest.xml new file mode 100644 index 0000000000..782fe39491 --- /dev/null +++ b/tests/qemuxmlconfdata/x86_64-q35-default-models.x86_64-latest.xml @@ -0,0 +1,68 @@ +<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='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='usb' index='0' model='piix3-uhci'> + <address type='pci' domain='0x0000' bus='0x02' slot='0x02' function='0x0'/> + </controller> + <controller type='scsi' index='0' model='lsilogic'> + <address type='pci' domain='0x0000' bus='0x02' slot='0x03' function='0x0'/> + </controller> + <controller type='sata' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='pcie-root-port'> + <model name='pcie-root-port'/> + <target chassis='1' port='0x10'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0' multifunction='on'/> + </controller> + <controller type='pci' index='2' 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='3' model='pcie-root-port'> + <model name='pcie-root-port'/> + <target chassis='3' port='0x11'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x1'/> + </controller> + <interface type='user'> + <mac address='52:54:00:09:a4:37'/> + <model type='rtl8139'/> + <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/> + </interface> + <serial type='pty'> + <target type='isa-serial' port='0'> + <model name='isa-serial'/> + </target> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + </console> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <audio id='1' type='none'/> + <video> + <model type='cirrus' vram='16384' heads='1' primary='yes'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </video> + <watchdog model='itco' action='reset'/> + <memballoon model='none'/> + <panic model='isa'/> + </devices> +</domain> diff --git a/tests/qemuxmlconfdata/x86_64-q35-default-models.xml b/tests/qemuxmlconfdata/x86_64-q35-default-models.xml new file mode 100644 index 0000000000..5cdf07f9d3 --- /dev/null +++ b/tests/qemuxmlconfdata/x86_64-q35-default-models.xml @@ -0,0 +1,21 @@ +<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='q35'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb'/> + <controller type='scsi'/> + <interface type='user'> + <mac address='52:54:00:09:a4:37'/> + </interface> + <serial type='pty'/> + <video/> + <memballoon model='none'/> + <panic/> + </devices> +</domain> diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c index 445e6ff5e1..ce8f0d1964 100644 --- a/tests/qemuxmlconftest.c +++ b/tests/qemuxmlconftest.c @@ -1233,6 +1233,13 @@ mymain(void) DO_TEST_CAPS_ARCH_LATEST_ABI_UPDATE("ppc64-pseries-minimal", "ppc64"); DO_TEST_CAPS_ARCH_LATEST_ABI_UPDATE("s390x-ccw-minimal", "s390x"); + DO_TEST_CAPS_LATEST_ABI_UPDATE("x86_64-pc-default-models"); + DO_TEST_CAPS_LATEST_ABI_UPDATE("x86_64-q35-default-models"); + DO_TEST_CAPS_ARCH_LATEST_ABI_UPDATE("aarch64-virt-default-models", "aarch64"); + DO_TEST_CAPS_ARCH_LATEST_ABI_UPDATE("riscv64-virt-default-models", "riscv64"); + DO_TEST_CAPS_ARCH_LATEST_ABI_UPDATE("ppc64-pseries-default-models", "ppc64"); + DO_TEST_CAPS_ARCH_LATEST_ABI_UPDATE("s390x-ccw-default-models", "s390x"); + DO_TEST_CAPS_LATEST_PARSE_ERROR("no-memory"); DO_TEST_CAPS_LATEST("title-and-description"); -- 2.43.0

On Wed, Jan 24, 2024 at 20:37:28 +0100, Andrea Bolognani wrote:
These are similar to the minimal cases that we just introduced, but are intended to demonstrate what device or controller model libvirt will choose when one is not provided by the user.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- ...64-virt-default-models.aarch64-latest.args | 44 +++++++++++ ...h64-virt-default-models.aarch64-latest.xml | 79 +++++++++++++++++++ .../aarch64-virt-default-models.xml | 21 +++++ ...4-pseries-default-models.ppc64-latest.args | 38 +++++++++ ...64-pseries-default-models.ppc64-latest.xml | 53 +++++++++++++ .../ppc64-pseries-default-models.xml | 21 +++++ ...64-virt-default-models.riscv64-latest.args | 42 ++++++++++ ...v64-virt-default-models.riscv64-latest.xml | 68 ++++++++++++++++ .../riscv64-virt-default-models.xml | 21 +++++ ...s390x-ccw-default-models.s390x-latest.args | 37 +++++++++ .../s390x-ccw-default-models.s390x-latest.xml | 46 +++++++++++ .../s390x-ccw-default-models.xml | 21 +++++ ...86_64-pc-default-models.x86_64-latest.args | 39 +++++++++ ...x86_64-pc-default-models.x86_64-latest.xml | 50 ++++++++++++ .../x86_64-pc-default-models.xml | 21 +++++ ...6_64-q35-default-models.x86_64-latest.args | 44 +++++++++++ ...86_64-q35-default-models.x86_64-latest.xml | 68 ++++++++++++++++ .../x86_64-q35-default-models.xml | 21 +++++
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

They reference functions that have since been renamed. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 6 +++--- src/qemu/qemu_domain.c | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b8f071ff2a..4aad7066e9 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2817,7 +2817,7 @@ qemuBuildControllerPCIDevProps(virDomainControllerDef *def, /** - * qemuBuildControllerDevStr: + * qemuBuildControllerDevProps: * @domainDef: domain definition * @def: controller definition * @qemuCaps: QEMU binary capabilities @@ -3070,10 +3070,10 @@ qemuBuildControllersByTypeCommandLine(virCommand *cmd, * * 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 qemuBuildControllerDevStr(); on the other hand, + * 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 - * qemuBuildControllerDevStr() but we don't ultimately end + * qemuBuildControllerDevProps() but we don't ultimately end * up adding the legacy USB controller */ continue; } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index de36641137..7292739d1f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5597,7 +5597,7 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont, * chance we will get away with using the legacy USB controller * when the relevant device is not available. * - * See qemuBuildControllerDevCommandLine() */ + * See qemuBuildControllersCommandLine() */ /* Default USB controller is piix3-uhci if available. */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI)) -- 2.43.0

On Wed, Jan 24, 2024 at 20:37:29 +0100, Andrea Bolognani wrote:
They reference functions that have since been renamed.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 6 +++--- src/qemu/qemu_domain.c | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

The current defaults, that can be altered on a per-architecture basis, are derived from the historical x86 behavior. Every time support for a new architecture is added to libvirt, care must be taken to override these default: if that doesn't happen, guests will end up with additional hardware, which is something that's generally undesirable. Turn things around, and require architectures to explicitly ask for the devices to be created by default instead. The behavior for existing architectures is preserved. Signed-off-by: Andrea Bolognani <abologna@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 7292739d1f..d56ac929ea 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4109,13 +4109,13 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver *driver, virDomainDef *def, virQEMUCaps *qemuCaps) { - bool addDefaultUSB = true; + bool addDefaultUSB = false; int usbModel = -1; /* "default for machinetype" */ int pciRoot; /* index within def->controllers */ bool addImplicitSATA = false; bool addPCIRoot = false; bool addPCIeRoot = false; - bool addDefaultMemballoon = true; + bool addDefaultMemballoon = false; bool addDefaultUSBKBD = false; bool addDefaultUSBMouse = false; bool addPanicDevice = false; @@ -4129,10 +4129,14 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver *driver, switch (def->os.arch) { case VIR_ARCH_I686: case VIR_ARCH_X86_64: + addDefaultMemballoon = true; + if (STREQ(def->os.machine, "isapc")) { - addDefaultUSB = false; break; } + + addDefaultUSB = true; + if (qemuDomainIsQ35(def)) { addPCIeRoot = true; addImplicitSATA = true; @@ -4157,16 +4161,12 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver *driver, break; case VIR_ARCH_ARMV6L: - addDefaultUSB = false; - addDefaultMemballoon = false; if (STREQ(def->os.machine, "versatilepb")) addPCIRoot = true; break; case VIR_ARCH_ARMV7L: case VIR_ARCH_AARCH64: - addDefaultUSB = false; - addDefaultMemballoon = false; if (qemuDomainIsARMVirt(def)) addPCIeRoot = true; break; @@ -4174,8 +4174,10 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver *driver, case VIR_ARCH_PPC64: case VIR_ARCH_PPC64LE: addPCIRoot = true; + addDefaultUSB = true; addDefaultUSBKBD = true; addDefaultUSBMouse = true; + addDefaultMemballoon = true; /* For pSeries guests, the firmware provides the same * functionality as the pvpanic device, so automatically * add the definition if not already present */ @@ -4188,29 +4190,28 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver *driver, case VIR_ARCH_PPCEMB: case VIR_ARCH_SH4: case VIR_ARCH_SH4EB: + addDefaultUSB = true; + addDefaultMemballoon = true; addPCIRoot = true; break; case VIR_ARCH_RISCV32: case VIR_ARCH_RISCV64: - addDefaultUSB = false; + addDefaultMemballoon = true; if (qemuDomainIsRISCVVirt(def)) addPCIeRoot = true; break; case VIR_ARCH_S390: case VIR_ARCH_S390X: - addDefaultUSB = false; + addDefaultMemballoon = true; addPanicDevice = true; addPCIRoot = virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ZPCI); break; - case VIR_ARCH_SPARC: - addDefaultUSB = false; - addDefaultMemballoon = false; - break; - case VIR_ARCH_SPARC64: + addDefaultUSB = true; + addDefaultMemballoon = true; addPCIRoot = true; break; @@ -4218,6 +4219,8 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver *driver, case VIR_ARCH_MIPSEL: case VIR_ARCH_MIPS64: case VIR_ARCH_MIPS64EL: + addDefaultUSB = true; + addDefaultMemballoon = true; if (qemuDomainIsMipsMalta(def)) addPCIRoot = true; break; @@ -4233,6 +4236,7 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver *driver, case VIR_ARCH_PARISC: case VIR_ARCH_PARISC64: case VIR_ARCH_PPCLE: + case VIR_ARCH_SPARC: case VIR_ARCH_UNICORE32: case VIR_ARCH_XTENSA: case VIR_ARCH_XTENSAEB: -- 2.43.0

Group things together where it makes sense, avoid unnecessary uses of 'else if', plus other tweaks. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d56ac929ea..9289c1fa18 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5396,12 +5396,14 @@ static int qemuDomainDefaultNetModel(const virDomainDef *def, virQEMUCaps *qemuCaps) { - if (ARCH_IS_S390(def->os.arch)) + /* When there are no backwards compatibility concerns getting in + * the way, virtio is a good default */ + if (ARCH_IS_S390(def->os.arch) || + qemuDomainIsRISCVVirt(def)) { return VIR_DOMAIN_NET_MODEL_VIRTIO; + } - if (def->os.arch == VIR_ARCH_ARMV6L || - def->os.arch == VIR_ARCH_ARMV7L || - def->os.arch == VIR_ARCH_AARCH64) { + if (ARCH_IS_ARM(def->os.arch)) { if (STREQ(def->os.machine, "versatilepb")) return VIR_DOMAIN_NET_MODEL_SMC91C111; @@ -5413,10 +5415,6 @@ qemuDomainDefaultNetModel(const virDomainDef *def, return VIR_DOMAIN_NET_MODEL_LAN9118; } - /* virtio is a sensible default for RISC-V virt guests */ - if (qemuDomainIsRISCVVirt(def)) - return VIR_DOMAIN_NET_MODEL_VIRTIO; - /* In all other cases the model depends on the capabilities. If they were * not provided don't report any default. */ if (!qemuCaps) @@ -5427,9 +5425,9 @@ qemuDomainDefaultNetModel(const virDomainDef *def, * system than the previous one */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_RTL8139)) return VIR_DOMAIN_NET_MODEL_RTL8139; - else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_E1000)) + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_E1000)) return VIR_DOMAIN_NET_MODEL_E1000; - else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_NET)) + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_NET)) return VIR_DOMAIN_NET_MODEL_VIRTIO; /* We've had no luck detecting support for any network device, -- 2.43.0

On Wed, Jan 24, 2024 at 20:37:31 +0100, Andrea Bolognani wrote:
Group things together where it makes sense, avoid unnecessary uses of 'else if', plus other tweaks.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-)
@@ -5427,9 +5425,9 @@ qemuDomainDefaultNetModel(const virDomainDef *def, * system than the previous one */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_RTL8139)) return VIR_DOMAIN_NET_MODEL_RTL8139; - else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_E1000)) + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_E1000)) return VIR_DOMAIN_NET_MODEL_E1000; - else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_NET)) + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_NET)) return VIR_DOMAIN_NET_MODEL_VIRTIO;
Separate these by empty lines once you've broke up the 'else if' pattern. Reviewed-by: Peter Krempa <pkrempa@redhat.com>

On Thu, Jan 25, 2024 at 11:12:33AM +0100, Peter Krempa wrote:
On Wed, Jan 24, 2024 at 20:37:31 +0100, Andrea Bolognani wrote:
@@ -5427,9 +5425,9 @@ qemuDomainDefaultNetModel(const virDomainDef *def, * system than the previous one */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_RTL8139)) return VIR_DOMAIN_NET_MODEL_RTL8139; - else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_E1000)) + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_E1000)) return VIR_DOMAIN_NET_MODEL_E1000; - else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_NET)) + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_NET)) return VIR_DOMAIN_NET_MODEL_VIRTIO;
Separate these by empty lines once you've broke up the 'else if' pattern.
When a bunch of capabilities are being checked one after the other in first-one-wins fashion, I kinda find it more readable not to have empty lines in between them, but I'm happy either way :) -- Andrea Bolognani / Red Hat / Virtualization

It only has a single caller. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_alias.c | 13 +++++++++++-- src/qemu/qemu_domain_address.c | 26 -------------------------- src/qemu/qemu_domain_address.h | 3 --- 3 files changed, 11 insertions(+), 31 deletions(-) diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index 686b5a4d80..d2c28c182b 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -214,8 +214,17 @@ qemuAssignDeviceDiskAlias(virDomainDef *def, if (!disk->info.alias) { if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) { - controllerModel = qemuDomainFindSCSIControllerModel(def, - &disk->info); + virDomainControllerDef *cont; + + if (!(cont = virDomainDeviceFindSCSIController(def, &disk->info.addr.drive))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unable to find a SCSI controller for idx=%1$d"), + disk->info.addr.drive.controller); + return -1; + } + + controllerModel = cont->model; + if (controllerModel < 0) return -1; } diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 413f152230..22af903b6a 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -99,32 +99,6 @@ qemuDomainSetSCSIControllerModel(const virDomainDef *def, } -/** - * @def: Domain definition - * @info: Domain device info - * - * Using the device info, find the controller related to the - * device by index and use that controller to return the model. - * - * Returns the model if found, -1 if not with an error message set - */ -int -qemuDomainFindSCSIControllerModel(const virDomainDef *def, - virDomainDeviceInfo *info) -{ - virDomainControllerDef *cont; - - if (!(cont = virDomainDeviceFindSCSIController(def, &info->addr.drive))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unable to find a SCSI controller for idx=%1$d"), - info->addr.drive.controller); - return -1; - } - - return cont->model; -} - - static int qemuDomainAssignVirtioSerialAddresses(virDomainDef *def) { diff --git a/src/qemu/qemu_domain_address.h b/src/qemu/qemu_domain_address.h index dbb5de915e..a571312469 100644 --- a/src/qemu/qemu_domain_address.h +++ b/src/qemu/qemu_domain_address.h @@ -32,9 +32,6 @@ int qemuDomainSetSCSIControllerModel(const virDomainDef *def, virDomainControllerDef *cont, virQEMUCaps *qemuCaps); -int qemuDomainFindSCSIControllerModel(const virDomainDef *def, - virDomainDeviceInfo *info); - int qemuDomainAssignAddresses(virDomainDef *def, virQEMUCaps *qemuCaps, virQEMUDriver *driver, -- 2.43.0

On Wed, Jan 24, 2024 at 20:37:32 +0100, Andrea Bolognani wrote:
It only has a single caller.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_alias.c | 13 +++++++++++-- src/qemu/qemu_domain_address.c | 26 -------------------------- src/qemu/qemu_domain_address.h | 3 --- 3 files changed, 11 insertions(+), 31 deletions(-)
diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index 686b5a4d80..d2c28c182b 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -214,8 +214,17 @@ qemuAssignDeviceDiskAlias(virDomainDef *def, if (!disk->info.alias) { if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) { - controllerModel = qemuDomainFindSCSIControllerModel(def, - &disk->info); + virDomainControllerDef *cont; + + if (!(cont = virDomainDeviceFindSCSIController(def, &disk->info.addr.drive))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unable to find a SCSI controller for idx=%1$d"), + disk->info.addr.drive.controller);
Broken alignment. With that fixed: Reviewed-by: Peter Krempa <pkrempa@redhat.com>

It only has a single caller. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 4 +++- src/qemu/qemu_domain_address.c | 25 ------------------------- src/qemu/qemu_domain_address.h | 4 ---- 3 files changed, 3 insertions(+), 30 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9289c1fa18..48d721a809 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5584,7 +5584,9 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont, switch (cont->type) { case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: /* Set the default SCSI controller model if not already set */ - if (qemuDomainSetSCSIControllerModel(def, cont, qemuCaps) < 0) + cont->model = qemuDomainGetSCSIControllerModel(def, cont, qemuCaps); + + if (cont->model < 0) return -1; break; diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 22af903b6a..0a7273dc07 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -74,31 +74,6 @@ qemuDomainGetSCSIControllerModel(const virDomainDef *def, } -/** - * @def: Domain definition - * @cont: Domain controller def - * @qemuCaps: qemu capabilities - * - * Set the controller model based on the existing value and the - * capabilities if possible. - * - * Returns 0 on success, -1 on failure with error set. - */ -int -qemuDomainSetSCSIControllerModel(const virDomainDef *def, - virDomainControllerDef *cont, - virQEMUCaps *qemuCaps) -{ - int model = qemuDomainGetSCSIControllerModel(def, cont, qemuCaps); - - if (model < 0) - return -1; - - cont->model = model; - return 0; -} - - static int qemuDomainAssignVirtioSerialAddresses(virDomainDef *def) { diff --git a/src/qemu/qemu_domain_address.h b/src/qemu/qemu_domain_address.h index a571312469..0ac6285f2e 100644 --- a/src/qemu/qemu_domain_address.h +++ b/src/qemu/qemu_domain_address.h @@ -28,10 +28,6 @@ int qemuDomainGetSCSIControllerModel(const virDomainDef *def, const virDomainControllerDef *cont, virQEMUCaps *qemuCaps); -int qemuDomainSetSCSIControllerModel(const virDomainDef *def, - virDomainControllerDef *cont, - virQEMUCaps *qemuCaps); - int qemuDomainAssignAddresses(virDomainDef *def, virQEMUCaps *qemuCaps, virQEMUDriver *driver, -- 2.43.0

On Wed, Jan 24, 2024 at 20:37:33 +0100, Andrea Bolognani wrote:
It only has a single caller.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 4 +++- src/qemu/qemu_domain_address.c | 25 ------------------------- src/qemu/qemu_domain_address.h | 4 ---- 3 files changed, 3 insertions(+), 30 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9289c1fa18..48d721a809 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5584,7 +5584,9 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont, switch (cont->type) { case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: /* Set the default SCSI controller model if not already set */ - if (qemuDomainSetSCSIControllerModel(def, cont, qemuCaps) < 0) + cont->model = qemuDomainGetSCSIControllerModel(def, cont, qemuCaps); + + if (cont->model < 0) return -1;
This is not future proof for the case when 'model' gets turned into a proper enum.

On Thu, Jan 25, 2024 at 11:15:14 +0100, Peter Krempa wrote:
On Wed, Jan 24, 2024 at 20:37:33 +0100, Andrea Bolognani wrote:
It only has a single caller.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 4 +++- src/qemu/qemu_domain_address.c | 25 ------------------------- src/qemu/qemu_domain_address.h | 4 ---- 3 files changed, 3 insertions(+), 30 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9289c1fa18..48d721a809 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5584,7 +5584,9 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont, switch (cont->type) { case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: /* Set the default SCSI controller model if not already set */ - if (qemuDomainSetSCSIControllerModel(def, cont, qemuCaps) < 0) + cont->model = qemuDomainGetSCSIControllerModel(def, cont, qemuCaps); + + if (cont->model < 0) return -1;
This is not future proof for the case when 'model' gets turned into a proper enum.
Based on further patches, it seems to be a problem in more places. Reviewed-by: Peter Krempa <pkrempa@redhat.com>

On Thu, Jan 25, 2024 at 11:17:46AM +0100, Peter Krempa wrote:
On Thu, Jan 25, 2024 at 11:15:14 +0100, Peter Krempa wrote:
On Wed, Jan 24, 2024 at 20:37:33 +0100, Andrea Bolognani wrote:
@@ -5584,7 +5584,9 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont, switch (cont->type) { case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: /* Set the default SCSI controller model if not already set */ - if (qemuDomainSetSCSIControllerModel(def, cont, qemuCaps) < 0) + cont->model = qemuDomainGetSCSIControllerModel(def, cont, qemuCaps); + + if (cont->model < 0) return -1;
This is not future proof for the case when 'model' gets turned into a proper enum.
Based on further patches, it seems to be a problem in more places.
I'm not sure I understand. Can you please explain what "a proper enum" means in this context? Is that connected to some condition that could present itself in the current code, or as a consequence of a potential future refactoring? Based on the fact that you ultimately ACKed the patch, I assume the latter. -- Andrea Bolognani / Red Hat / Virtualization

On Thu, Jan 25, 2024 at 10:16:55 -0800, Andrea Bolognani wrote:
On Thu, Jan 25, 2024 at 11:17:46AM +0100, Peter Krempa wrote:
On Thu, Jan 25, 2024 at 11:15:14 +0100, Peter Krempa wrote:
On Wed, Jan 24, 2024 at 20:37:33 +0100, Andrea Bolognani wrote:
@@ -5584,7 +5584,9 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont, switch (cont->type) { case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: /* Set the default SCSI controller model if not already set */ - if (qemuDomainSetSCSIControllerModel(def, cont, qemuCaps) < 0) + cont->model = qemuDomainGetSCSIControllerModel(def, cont, qemuCaps); + + if (cont->model < 0) return -1;
This is not future proof for the case when 'model' gets turned into a proper enum.
Based on further patches, it seems to be a problem in more places.
I'm not sure I understand. Can you please explain what "a proper enum" means in this context? Is that connected to some condition that
If you turn the 'model' member from 'int' to the appropriate enum type the '< 0' check will no longer work if the enum has no negative members. This is due to the fact that the compiler then treats the enum as 'unsigned'
could present itself in the current code, or as a consequence of a potential future refactoring? Based on the fact that you ultimately ACKed the patch, I assume the latter.
There are multiple other instances of exactly the same problem for this enum and additionally this one has a native -1 member, so my point was moot mostly.

qemuDomainGetSCSIControllerModel() can return -1 on failure, but qemuDomainFindOrCreateSCSIDiskController() didn't implement any handling for this scenario. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_hotplug.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 0e45bd53e1..afb720fc0b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -881,6 +881,11 @@ qemuDomainFindOrCreateSCSIDiskController(virDomainObj *vm, else cont->model = model; + if (cont->model < 0) { + VIR_FREE(cont); + return NULL; + } + VIR_INFO("No SCSI controller present, hotplugging one model=%s", virDomainControllerModelSCSITypeToString(cont->model)); if (qemuDomainAttachControllerDevice(vm, cont) < 0) { -- 2.43.0

On Wed, Jan 24, 2024 at 20:37:34 +0100, Andrea Bolognani wrote:
qemuDomainGetSCSIControllerModel() can return -1 on failure, but qemuDomainFindOrCreateSCSIDiskController() didn't implement any handling for this scenario.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_hotplug.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 0e45bd53e1..afb720fc0b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -881,6 +881,11 @@ qemuDomainFindOrCreateSCSIDiskController(virDomainObj *vm, else cont->model = model;
+ if (cont->model < 0) { + VIR_FREE(cont); + return NULL; + } + VIR_INFO("No SCSI controller present, hotplugging one model=%s", virDomainControllerModelSCSITypeToString(cont->model));
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

Not a massive difference, but it will make upcoming changes nicer. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_hotplug.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index afb720fc0b..c883fef0e9 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -876,10 +876,9 @@ qemuDomainFindOrCreateSCSIDiskController(virDomainObj *vm, cont = g_new0(virDomainControllerDef, 1); cont->type = VIR_DOMAIN_CONTROLLER_TYPE_SCSI; cont->idx = controller; - if (model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT) - cont->model = qemuDomainGetSCSIControllerModel(vm->def, cont, priv->qemuCaps); - else - cont->model = model; + cont->model = model; + + cont->model = qemuDomainGetSCSIControllerModel(vm->def, cont, priv->qemuCaps); if (cont->model < 0) { VIR_FREE(cont); -- 2.43.0

On Wed, Jan 24, 2024 at 20:37:35 +0100, Andrea Bolognani wrote:
Not a massive difference, but it will make upcoming changes nicer.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_hotplug.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index afb720fc0b..c883fef0e9 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -876,10 +876,9 @@ qemuDomainFindOrCreateSCSIDiskController(virDomainObj *vm, cont = g_new0(virDomainControllerDef, 1); cont->type = VIR_DOMAIN_CONTROLLER_TYPE_SCSI; cont->idx = controller; - if (model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT) - cont->model = qemuDomainGetSCSIControllerModel(vm->def, cont, priv->qemuCaps); - else - cont->model = model; + cont->model = model; + + cont->model = qemuDomainGetSCSIControllerModel(vm->def, cont, priv->qemuCaps);
This makes no sense at first look. Commit message doesn't explain it, nothing in the code explains it. You have to look into qemuDomainGetSCSIControllerModel to see why it can be done. I've looked ahead to see how you've refactored it, at which point it makes sense, but I'm not really persuaded that this can't be part of the commit that is actually fixing the function to not depend on the value in the 'model' struct. IMO this patch makes the code strictly worse, even when it's short lived.

On Thu, Jan 25, 2024 at 01:07:10PM +0100, Peter Krempa wrote:
On Wed, Jan 24, 2024 at 20:37:35 +0100, Andrea Bolognani wrote:
@@ -876,10 +876,9 @@ qemuDomainFindOrCreateSCSIDiskController(virDomainObj *vm, cont = g_new0(virDomainControllerDef, 1); cont->type = VIR_DOMAIN_CONTROLLER_TYPE_SCSI; cont->idx = controller; - if (model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT) - cont->model = qemuDomainGetSCSIControllerModel(vm->def, cont, priv->qemuCaps); - else - cont->model = model; + cont->model = model; + + cont->model = qemuDomainGetSCSIControllerModel(vm->def, cont, priv->qemuCaps);
This makes no sense at first look. Commit message doesn't explain it, nothing in the code explains it. You have to look into qemuDomainGetSCSIControllerModel to see why it can be done.
I've looked ahead to see how you've refactored it, at which point it makes sense, but I'm not really persuaded that this can't be part of the commit that is actually fixing the function to not depend on the value in the 'model' struct.
IMO this patch makes the code strictly worse, even when it's short lived.
That's why I've justified in the commit message as being something that would pay off later on ;) I can roll it into the later patch. I think I had it there initially, but it seemed that it would make reviewing the change harder. Since you're the one reviewing, I'm happy to go with your preference. -- Andrea Bolognani / Red Hat / Virtualization

On Thu, Jan 25, 2024 at 10:20:05 -0800, Andrea Bolognani wrote:
On Thu, Jan 25, 2024 at 01:07:10PM +0100, Peter Krempa wrote:
On Wed, Jan 24, 2024 at 20:37:35 +0100, Andrea Bolognani wrote:
@@ -876,10 +876,9 @@ qemuDomainFindOrCreateSCSIDiskController(virDomainObj *vm, cont = g_new0(virDomainControllerDef, 1); cont->type = VIR_DOMAIN_CONTROLLER_TYPE_SCSI; cont->idx = controller; - if (model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT) - cont->model = qemuDomainGetSCSIControllerModel(vm->def, cont, priv->qemuCaps); - else - cont->model = model; + cont->model = model; + + cont->model = qemuDomainGetSCSIControllerModel(vm->def, cont, priv->qemuCaps);
This makes no sense at first look. Commit message doesn't explain it, nothing in the code explains it. You have to look into qemuDomainGetSCSIControllerModel to see why it can be done.
I've looked ahead to see how you've refactored it, at which point it makes sense, but I'm not really persuaded that this can't be part of the commit that is actually fixing the function to not depend on the value in the 'model' struct.
IMO this patch makes the code strictly worse, even when it's short lived.
That's why I've justified in the commit message as being something that would pay off later on ;)
I can roll it into the later patch. I think I had it there initially, but it seemed that it would make reviewing the change harder. Since you're the one reviewing, I'm happy to go with your preference.
Please do so, this doesn't get an approval from me in this form.

It has nothing to do with assigning addresses, so it makes more sense to have it in qemu_domain. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 37 ++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 3 +++ src/qemu/qemu_domain_address.c | 36 --------------------------------- src/qemu/qemu_domain_address.h | 4 ---- 4 files changed, 40 insertions(+), 40 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 48d721a809..672f1ce59f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4104,6 +4104,43 @@ qemuDomainDefAddDefaultAudioBackend(virQEMUDriver *driver, return 0; } + +/** + * @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. + * + * Returns model on success, -1 on failure with error set. + */ +int +qemuDomainGetSCSIControllerModel(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)) + 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; + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to determine model for SCSI controller idx=%1$d"), + cont->idx); + return -1; +} + + static int qemuDomainDefAddDefaultDevices(virQEMUDriver *driver, virDomainDef *def, diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index b4512cc80e..4dfc0ab5c7 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -836,6 +836,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); void qemuDomainUpdateCurrentMemorySize(virDomainObj *vm); diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 0a7273dc07..49fd4d454d 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -38,42 +38,6 @@ VIR_LOG_INIT("qemu.qemu_domain_address"); #define VIO_ADDR_TPM 0x4000ul -/** - * @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. - * - * Returns model on success, -1 on failure with error set. - */ -int -qemuDomainGetSCSIControllerModel(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)) - 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; - - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to determine model for SCSI controller idx=%1$d"), - cont->idx); - return -1; -} - - static int qemuDomainAssignVirtioSerialAddresses(virDomainDef *def) { diff --git a/src/qemu/qemu_domain_address.h b/src/qemu/qemu_domain_address.h index 0ac6285f2e..78fcd74234 100644 --- a/src/qemu/qemu_domain_address.h +++ b/src/qemu/qemu_domain_address.h @@ -24,10 +24,6 @@ #include "qemu_conf.h" #include "qemu_capabilities.h" -int qemuDomainGetSCSIControllerModel(const virDomainDef *def, - const virDomainControllerDef *cont, - virQEMUCaps *qemuCaps); - int qemuDomainAssignAddresses(virDomainDef *def, virQEMUCaps *qemuCaps, virQEMUDriver *driver, -- 2.43.0

On Wed, Jan 24, 2024 at 20:37:36 +0100, Andrea Bolognani wrote:
It has nothing to do with assigning addresses, so it makes more sense to have it in qemu_domain.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 37 ++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 3 +++ src/qemu/qemu_domain_address.c | 36 --------------------------------- src/qemu/qemu_domain_address.h | 4 ---- 4 files changed, 40 insertions(+), 40 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

The function is modified to be stateless, which is more consistent with existing helpers that deal with figuring out default models for devices, and its name needs to change accordingly. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 21 +++++++++++---------- src/qemu/qemu_domain.h | 6 +++--- src/qemu/qemu_hotplug.c | 5 +++-- 3 files changed, 17 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 672f1ce59f..97336d5002 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4106,23 +4106,23 @@ qemuDomainDefAddDefaultAudioBackend(virQEMUDriver *driver, /** + * qemuDomainDefaultSCSIControllerModel: * @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. + * Decides the preferred model for a SCSI controller that it to be + * added to @def. The choice might be based on various factors, + * including the architecture, machine type and what devices are + * available according to @qemuCaps. * * 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; - if (qemuDomainIsPSeries(def)) return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI; if (ARCH_IS_S390(def->os.arch)) @@ -5621,9 +5621,10 @@ 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); + if (cont->model <= 0) + cont->model = qemuDomainDefaultSCSIControllerModel(def, cont, qemuCaps); - if (cont->model < 0) + if (cont->model <= 0) return -1; break; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 4dfc0ab5c7..48f966fa2a 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -836,9 +836,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 c883fef0e9..4fd7b7f756 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -878,9 +878,10 @@ qemuDomainFindOrCreateSCSIDiskController(virDomainObj *vm, cont->idx = controller; cont->model = model; - cont->model = qemuDomainGetSCSIControllerModel(vm->def, cont, priv->qemuCaps); + if (cont->model <= 0) + cont->model = qemuDomainDefaultSCSIControllerModel(vm->def, cont, priv->qemuCaps); - if (cont->model < 0) { + if (cont->model <= 0) { VIR_FREE(cont); return NULL; } -- 2.43.0

Summary states just "Rename" On Wed, Jan 24, 2024 at 20:37:37 +0100, Andrea Bolognani wrote:
The function is modified to be stateless, which is more consistent with existing helpers that deal with figuring out default models for devices, and its name needs to change accordingly.
But logic is changed. Don't mislead in the summary. It's acceptable to rename the function as side effect of logic changes rather than have logic changes as side effect of 'rename'
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 21 +++++++++++---------- src/qemu/qemu_domain.h | 6 +++--- src/qemu/qemu_hotplug.c | 5 +++-- 3 files changed, 17 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 672f1ce59f..97336d5002 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4106,23 +4106,23 @@ qemuDomainDefAddDefaultAudioBackend(virQEMUDriver *driver,
/** + * qemuDomainDefaultSCSIControllerModel: * @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. + * Decides the preferred model for a SCSI controller that it to be + * added to @def. The choice might be based on various factors, + * including the architecture, machine type and what devices are + * available according to @qemuCaps. * * 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; - if (qemuDomainIsPSeries(def)) return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI; if (ARCH_IS_S390(def->os.arch)) @@ -5621,9 +5621,10 @@ 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); + if (cont->model <= 0) + cont->model = qemuDomainDefaultSCSIControllerModel(def, cont, qemuCaps);
- if (cont->model < 0) + if (cont->model <= 0)
The function comment states:
* * Returns model on success, -1 on failure with error set. */
While '0' currently can't happen the check contradicts the documented return value. The function documents that error is set only on -1 thus returning '-1' here if qemuDomainDefaultSCSIControllerModel return 0 would propagate a failure without error reported. Either you modify the function docs to state that also 0 is considered error and error is to be set, or don't modify the checks. Also this still is sub-optimal as VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT is a valid enum member with value of -1; Obviously the preferrable solution would be to return the model via pointer/argument and just return success error from the main function to avoid the problems described above. With the commit message fixed and one of: - remove the 'cont->model <= 0' changes - the comment describing the return values improved Reviewed-by: Peter Krempa <pkrempa@redhat.com>

On Thu, Jan 25, 2024 at 01:45:30PM +0100, Peter Krempa wrote:
Summary states just "Rename" But logic is changed. Don't mislead in the summary. It's acceptable to rename the function as side effect of logic changes rather than have logic changes as side effect of 'rename'
You're right.
case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: /* Set the default SCSI controller model if not already set */ - cont->model = qemuDomainGetSCSIControllerModel(def, cont, qemuCaps); + if (cont->model <= 0) + cont->model = qemuDomainDefaultSCSIControllerModel(def, cont, qemuCaps);
- if (cont->model < 0) + if (cont->model <= 0)
The function comment states:
* * Returns model on success, -1 on failure with error set. */
While '0' currently can't happen the check contradicts the documented return value. The function documents that error is set only on -1 thus returning '-1' here if qemuDomainDefaultSCSIControllerModel return 0 would propagate a failure without error reported.
Either you modify the function docs to state that also 0 is considered error and error is to be set, or don't modify the checks.
Yeah, good catch.
Also this still is sub-optimal as VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT is a valid enum member with value of -1;
That's true. However, both DEFAULT and AUTO are effectively non-choices, and since the purpose of these helpers is to resolve DEFAULT[*] to an actual model, I think considering them as error conditions is only fair. The alternative course of action, which IIRC is what happens for USB controllers, is to accept DEFAULT in postparse only to reject it further down the stack. I'm not convinced that's preferrable, but I can give that approach a try if you want me to. [*] I haven't really considered AUTO, and I'm not sure why it exists. I'll be sure to look into it. -- Andrea Bolognani / Red Hat / Virtualization

On Thu, Jan 25, 2024 at 10:34:54 -0800, Andrea Bolognani wrote:
On Thu, Jan 25, 2024 at 01:45:30PM +0100, Peter Krempa wrote:
Summary states just "Rename" But logic is changed. Don't mislead in the summary. It's acceptable to rename the function as side effect of logic changes rather than have logic changes as side effect of 'rename'
You're right.
case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: /* Set the default SCSI controller model if not already set */ - cont->model = qemuDomainGetSCSIControllerModel(def, cont, qemuCaps); + if (cont->model <= 0) + cont->model = qemuDomainDefaultSCSIControllerModel(def, cont, qemuCaps);
- if (cont->model < 0) + if (cont->model <= 0)
The function comment states:
* * Returns model on success, -1 on failure with error set. */
While '0' currently can't happen the check contradicts the documented return value. The function documents that error is set only on -1 thus returning '-1' here if qemuDomainDefaultSCSIControllerModel return 0 would propagate a failure without error reported.
Either you modify the function docs to state that also 0 is considered error and error is to be set, or don't modify the checks.
Yeah, good catch.
Also this still is sub-optimal as VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT is a valid enum member with value of -1;
That's true.
However, both DEFAULT and AUTO are effectively non-choices, and since the purpose of these helpers is to resolve DEFAULT[*] to an actual model, I think considering them as error conditions is only fair.
It can be considered an error but must not be described as "-1 is an error" as -1 is a valid enum value. Based on the fact that libvirt uses '-1' widely as an error value here it can mislead about the actual meaninig. That's why I've also asked to use the enum type as return value in subsequent patches. IT's for anyone trying to change the function to think about what they are doing, and seeing '-1 is an error' simply evokes the default pattern we use which is not the case here.
The alternative course of action, which IIRC is what happens for USB controllers, is to accept DEFAULT in postparse only to reject it further down the stack. I'm not convinced that's preferrable, but I can give that approach a try if you want me to.
[*] I haven't really considered AUTO, and I'm not sure why it exists. I'll be sure to look into it. -- Andrea Bolognani / Red Hat / Virtualization

Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 97336d5002..7475fb4f39 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4123,16 +4123,26 @@ qemuDomainDefaultSCSIControllerModel(const virDomainDef *def, const virDomainControllerDef *cont, 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; + + /* pSeries has its own special default */ if (qemuDomainIsPSeries(def)) return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI; + + /* Most new architectures should ideally use virtio */ if (ARCH_IS_S390(def->os.arch)) return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI; + + /* If there is no preference, base the choice on device + * availability. lsilogic is favored 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; virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to determine model for SCSI controller idx=%1$d"), -- 2.43.0

On Wed, Jan 24, 2024 at 20:37:38 +0100, Andrea Bolognani wrote:
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)

Extract the logic from qemuDomainControllerDefPostParse(). The code is mostly unchanged, but there's a subtle difference: the piix3-uhci has been moved from the top of the chunk to the bottom. This is because the original code set cont->model directly, which made it okay to start with a suboptimal default and subsequently overwrite it with a better one; now that we return the selected value instead, we need to make sure that we only ever consider piix3-uhci if everything else has failed. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 100 +++++++++++++++++++++++------------------ 1 file changed, 56 insertions(+), 44 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 7475fb4f39..09f572b0b5 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4151,6 +4151,61 @@ qemuDomainDefaultSCSIControllerModel(const virDomainDef *def, } +static int +qemuDomainDefaultUSBControllerModel(const virDomainDef *def, + const virDomainControllerDef *cont, + virQEMUCaps *qemuCaps, + unsigned int parseFlags) +{ + /* 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() */ + + 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 */ + return 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)) { + return VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI; + } else if ((parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI)) { + return VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI; + } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_OHCI)) { + return VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI; + } else { + /* Explicitly fallback to legacy USB controller for PPC64. */ + return -1; + } + } else if (def->os.arch == VIR_ARCH_AARCH64) { + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI)) + return VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI; + else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI)) + return VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI; + } + + /* Default USB controller is piix3-uhci if available. */ + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI)) + return VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI; + + return -1; +} + + static int qemuDomainDefAddDefaultDevices(virQEMUDriver *driver, virDomainDef *def, @@ -5640,50 +5695,7 @@ 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; - - 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; - } - } 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; - } + cont->model = qemuDomainDefaultUSBControllerModel(def, cont, qemuCaps, parseFlags); } /* forbid usb model 'qusb1' and 'qusb2' in this kind of hyperviosr */ if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB1 || -- 2.43.0

On Wed, Jan 24, 2024 at 20:37:39 +0100, Andrea Bolognani wrote:
Extract the logic from qemuDomainControllerDefPostParse().
The code is mostly unchanged, but there's a subtle difference: the piix3-uhci has been moved from the top of the chunk to the bottom. This is because the original code set cont->model directly, which made it okay to start with a suboptimal default and subsequently overwrite it with a better one; now that we return the selected value instead, we need to make sure that we only ever consider piix3-uhci if everything else has failed.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 100 +++++++++++++++++++++++------------------ 1 file changed, 56 insertions(+), 44 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 7475fb4f39..09f572b0b5 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4151,6 +4151,61 @@ qemuDomainDefaultSCSIControllerModel(const virDomainDef *def, }
+static int +qemuDomainDefaultUSBControllerModel(const virDomainDef *def, + const virDomainControllerDef *cont, + virQEMUCaps *qemuCaps, + unsigned int parseFlags) +{ + /* 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() */ + + 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 */ + return 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)) { + return VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI; + } else if ((parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI)) { + return VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI; + } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_OHCI)) { + return VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI; + } else { + /* Explicitly fallback to legacy USB controller for PPC64. */ + return -1;
So this is extracting the logic of passing VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT as model, which is -1, thus at this point faithful representation what the code did. The very next commit though declares -1 to be an error in the function comment which is not true. Extract it here as the proper enum value.
+ } + } else if (def->os.arch == VIR_ARCH_AARCH64) { + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI)) + return VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI; + else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI)) + return VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI; + } + + /* Default USB controller is piix3-uhci if available. */ + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI)) + return VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI; + + return -1;
This is later on documented via a comment but also should use the proper enum value. Reviewed-by: Peter Krempa <pkrempa@redhat.com>

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), introduce a new parameter to the function and a new possible return value. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 74 ++++++++++++++++++++++++++++++++---------- 1 file changed, 56 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 09f572b0b5..d992b51877 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4151,9 +4151,35 @@ qemuDomainDefaultSCSIControllerModel(const virDomainDef *def, } +/** + * qemuDomainDefaultUSBControllerModel: + * @def: domain definition + * @cont: controller definition, or NULL + * @autoAdded: whether this controller is being automatically added + * @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 to simply not + * add the controller instead of reporting a failure. + * + * Returns: >=0 (a virDomainControllerModelUSB value) on success, + * -1 on error, and + * -2 if no suitable model could be found but it's okay to + * just skip the controller altogether. + */ static int qemuDomainDefaultUSBControllerModel(const virDomainDef *def, const virDomainControllerDef *cont, + bool autoAdded, virQEMUCaps *qemuCaps, unsigned int parseFlags) { @@ -4169,7 +4195,7 @@ qemuDomainDefaultUSBControllerModel(const virDomainDef *def, * See qemuBuildControllersCommandLine() */ if (ARCH_IS_S390(def->os.arch)) { - if (cont->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + if (cont && cont->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { /* set the default USB model to none for s390 unless an * address is found */ return VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE; @@ -4198,6 +4224,22 @@ qemuDomainDefaultUSBControllerModel(const virDomainDef *def, return 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)) + 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 (virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_USB_EHCI1)) + return VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1; + return -2; + } + } + /* Default USB controller is piix3-uhci if available. */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI)) return VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI; @@ -4209,7 +4251,8 @@ qemuDomainDefaultUSBControllerModel(const virDomainDef *def, static int qemuDomainDefAddDefaultDevices(virQEMUDriver *driver, virDomainDef *def, - virQEMUCaps *qemuCaps) + virQEMUCaps *qemuCaps, + unsigned int parseFlags) { bool addDefaultUSB = false; int usbModel = -1; /* "default for machinetype" */ @@ -4243,20 +4286,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)) addPCIRoot = true; @@ -4348,6 +4377,15 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver *driver, break; } + if (addDefaultUSB) { + usbModel = qemuDomainDefaultUSBControllerModel(def, NULL, true, qemuCaps, parseFlags); + /* A return value of -2 indicates that no reasonable default + * could be figured out, and that we should handle that by + * not adding the USB controller */ + if (usbModel == -2) + addDefaultUSB = false; + } + if (addDefaultUSB && virDomainControllerFind(def, VIR_DOMAIN_CONTROLLER_TYPE_USB, 0) < 0 && virDomainDefAddUSBController(def, 0, usbModel) < 0) @@ -5091,7 +5129,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 +5733,7 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont, case VIR_DOMAIN_CONTROLLER_TYPE_USB: if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT && qemuCaps) { - cont->model = qemuDomainDefaultUSBControllerModel(def, cont, qemuCaps, parseFlags); + cont->model = qemuDomainDefaultUSBControllerModel(def, cont, false, qemuCaps, parseFlags); } /* forbid usb model 'qusb1' and 'qusb2' in this kind of hyperviosr */ if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB1 || -- 2.43.0

On Wed, Jan 24, 2024 at 20:37:40 +0100, Andrea Bolognani wrote:
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), introduce a new parameter to the function and a new possible return value.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 74 ++++++++++++++++++++++++++++++++---------- 1 file changed, 56 insertions(+), 18 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 09f572b0b5..d992b51877 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4151,9 +4151,35 @@ qemuDomainDefaultSCSIControllerModel(const virDomainDef *def, }
+/** + * qemuDomainDefaultUSBControllerModel: + * @def: domain definition + * @cont: controller definition, or NULL + * @autoAdded: whether this controller is being automatically added + * @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 to simply not + * add the controller instead of reporting a failure. + * + * Returns: >=0 (a virDomainControllerModelUSB value) on success, + * -1 on error, and
This is NOT an error and is misrepresenting the _DEFAULT case which has -1, which is also a success case at least in some situations.
+ * -2 if no suitable model could be found but it's okay to + * just skip the controller altogether.
IMO this should be VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE and not a new arbitrary value. I also don't think that this function needs to know whether the controller was auto-added, or not
+ */ static int qemuDomainDefaultUSBControllerModel(const virDomainDef *def, const virDomainControllerDef *cont, + bool autoAdded, virQEMUCaps *qemuCaps, unsigned int parseFlags) { @@ -4169,7 +4195,7 @@ qemuDomainDefaultUSBControllerModel(const virDomainDef *def, * See qemuBuildControllersCommandLine() */
if (ARCH_IS_S390(def->os.arch)) { - if (cont->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + if (cont && cont->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { /* set the default USB model to none for s390 unless an * address is found */ return VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE; @@ -4198,6 +4224,22 @@ qemuDomainDefaultUSBControllerModel(const virDomainDef *def, return 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)) + 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 (virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_USB_EHCI1)) + return VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1; + return -2; + } + } + /* Default USB controller is piix3-uhci if available. */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI)) return VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI; @@ -4209,7 +4251,8 @@ qemuDomainDefaultUSBControllerModel(const virDomainDef *def, static int qemuDomainDefAddDefaultDevices(virQEMUDriver *driver, virDomainDef *def, - virQEMUCaps *qemuCaps) + virQEMUCaps *qemuCaps, + unsigned int parseFlags) { bool addDefaultUSB = false; int usbModel = -1; /* "default for machinetype" */ @@ -4243,20 +4286,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)) addPCIRoot = true; @@ -4348,6 +4377,15 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver *driver, break; }
+ if (addDefaultUSB) { + usbModel = qemuDomainDefaultUSBControllerModel(def, NULL, true, qemuCaps, parseFlags); + /* A return value of -2 indicates that no reasonable default + * could be figured out, and that we should handle that by + * not adding the USB controller */ + if (usbModel == -2) + addDefaultUSB = false; + } + if (addDefaultUSB && virDomainControllerFind(def, VIR_DOMAIN_CONTROLLER_TYPE_USB, 0) < 0 && virDomainDefAddUSBController(def, 0, usbModel) < 0) @@ -5091,7 +5129,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 +5733,7 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont,
case VIR_DOMAIN_CONTROLLER_TYPE_USB: if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT && qemuCaps) { - cont->model = qemuDomainDefaultUSBControllerModel(def, cont, qemuCaps, parseFlags); + cont->model = qemuDomainDefaultUSBControllerModel(def, cont, false, qemuCaps, parseFlags);
The code can check here explicitly whether _NONE was returned and report appropriate error.
} /* forbid usb model 'qusb1' and 'qusb2' in this kind of hyperviosr */ if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB1 || -- 2.43.0 _______________________________________________ Devel mailing list -- devel@lists.libvirt.org
ws> To unsubscribe send an email to devel-leave@lists.libvirt.org

On Thu, Jan 25, 2024 at 03:00:10PM +0100, Peter Krempa wrote:
On Wed, Jan 24, 2024 at 20:37:40 +0100, Andrea Bolognani wrote:
+/** + * qemuDomainDefaultUSBControllerModel: + * @def: domain definition + * @cont: controller definition, or NULL + * @autoAdded: whether this controller is being automatically added + * @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 to simply not + * add the controller instead of reporting a failure. + * + * Returns: >=0 (a virDomainControllerModelUSB value) on success, + * -1 on error, and
This is NOT an error and is misrepresenting the _DEFAULT case which has -1, which is also a success case at least in some situations.
Fair enough.
I also don't think that this function needs to know whether the controller was auto-added, or not
It does though. For q35, we need to handle two cases: 1. a USB controller was present in the input XML, with no model provided; 2. a USB controller was NOT present in the input XML, but we're trying to add one by default. The outcome is different, as can be seen by comparing the output files for the relevant default-models and minimal tests: 1. the controller will be piix3-uhci; 2. the controller will be qemu-xhci. To complicate things further, the behavior is also different if the necessary devices are not available in the QEMU binary: 1. an error will be reported; 2. the controller will simply not be added. Personally I think that this is borderline madness, but it is the current behavior and with this series I'm only trying to reorganize things, not change them, at least until the last few patches. So, the additional argument is necessary in order to know which one of the two behaviors is desired/expected by the caller. Of course we didn't have this problem when we had two distinct chunks of code dealing with this default, but that came with its own issues and overall this approach seems preferable to me.
+ * -2 if no suitable model could be found but it's okay to + * just skip the controller altogether.
IMO this should be VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE and not a new arbitrary value.
Technically, yeah, that would work. But mostly by chance. Right now the -2 return value is only treated differently in the context of the auto-added USB controller, which is something that we don't do for s390x guests. But for those, NONE is a valid return value that needs to be handled by the other caller of the helper... Making the two overlap would IMO make the code more fragile against future changes, however unlikely they might be. And yes, that's another piece of borderline madness that however we have to preserve :)
@@ -5695,7 +5733,7 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont,
case VIR_DOMAIN_CONTROLLER_TYPE_USB: if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT && qemuCaps) { - cont->model = qemuDomainDefaultUSBControllerModel(def, cont, qemuCaps, parseFlags); + cont->model = qemuDomainDefaultUSBControllerModel(def, cont, false, qemuCaps, parseFlags);
The code can check here explicitly whether _NONE was returned and report appropriate error.
This is exactly the place where we need to make sure that we do *not* consider NONE an error, because of s390x. -- Andrea Bolognani / Red Hat / Virtualization

On Thu, Jan 25, 2024 at 10:59:29 -0800, Andrea Bolognani wrote:
On Thu, Jan 25, 2024 at 03:00:10PM +0100, Peter Krempa wrote:
On Wed, Jan 24, 2024 at 20:37:40 +0100, Andrea Bolognani wrote:
+/** + * qemuDomainDefaultUSBControllerModel: + * @def: domain definition + * @cont: controller definition, or NULL + * @autoAdded: whether this controller is being automatically added + * @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 to simply not + * add the controller instead of reporting a failure. + * + * Returns: >=0 (a virDomainControllerModelUSB value) on success, + * -1 on error, and
This is NOT an error and is misrepresenting the _DEFAULT case which has -1, which is also a success case at least in some situations.
Fair enough.
I also don't think that this function needs to know whether the controller was auto-added, or not
It does though. For q35, we need to handle two cases:
1. a USB controller was present in the input XML, with no model provided;
2. a USB controller was NOT present in the input XML, but we're trying to add one by default.
The outcome is different, as can be seen by comparing the output files for the relevant default-models and minimal tests:
1. the controller will be piix3-uhci;
2. the controller will be qemu-xhci.
To complicate things further, the behavior is also different if the necessary devices are not available in the QEMU binary:
1. an error will be reported;
2. the controller will simply not be added.
Personally I think that this is borderline madness, but it is the current behavior and with this series I'm only trying to reorganize things, not change them, at least until the last few patches.
So, the additional argument is necessary in order to know which one of the two behaviors is desired/expected by the caller.
Oh, that's indeed madness. I didn't realize originally that there's a difference between auto-selecting a model for an existing controller and auto-selecting a model for an auto-added controller. This obviously makes sense in the extent of the existing logic.
Of course we didn't have this problem when we had two distinct chunks of code dealing with this default, but that came with its own issues and overall this approach seems preferable to me.
+ * -2 if no suitable model could be found but it's okay to + * just skip the controller altogether.
IMO this should be VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE and not a new arbitrary value.
Technically, yeah, that would work. But mostly by chance.
Right now the -2 return value is only treated differently in the context of the auto-added USB controller, which is something that we don't do for s390x guests. But for those, NONE is a valid return value that needs to be handled by the other caller of the helper...
Making the two overlap would IMO make the code more fragile against future changes, however unlikely they might be.
And yes, that's another piece of borderline madness that however we have to preserve :)
@@ -5695,7 +5733,7 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont,
case VIR_DOMAIN_CONTROLLER_TYPE_USB: if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT && qemuCaps) { - cont->model = qemuDomainDefaultUSBControllerModel(def, cont, qemuCaps, parseFlags); + cont->model = qemuDomainDefaultUSBControllerModel(def, cont, false, qemuCaps, parseFlags);
The code can check here explicitly whether _NONE was returned and report appropriate error.
This is exactly the place where we need to make sure that we do *not* consider NONE an error, because of s390x.
As long as you properly document the return values and mention the actual enum value name in addition to -1: Reviewed-by: Peter Krempa <pkrempa@redhat.com>

Mostly reduce the number of 'else if' and improve comments. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 62 ++++++++++++++++++++++++------------------ 1 file changed, 36 insertions(+), 26 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d992b51877..dcf73c0e08 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4183,11 +4183,9 @@ qemuDomainDefaultUSBControllerModel(const virDomainDef *def, virQEMUCaps *qemuCaps, unsigned int parseFlags) { - /* 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 + bool abiUpdate = !!(parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE); + + /* 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. @@ -4200,50 +4198,62 @@ qemuDomainDefaultUSBControllerModel(const virDomainDef *def, * address is found */ return 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)) { + } + + if (ARCH_IS_PPC64(def->os.arch)) { + /* In order not to break migration compatibility, we need to + * set default USB controller to pci-ohci (USB2) for existing + * guests. New guests can use USB3 instead */ + if (abiUpdate && virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI)) return VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI; - } else if ((parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) && - virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI)) { + if (abiUpdate && virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI)) return VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI; - } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_OHCI)) { + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_OHCI)) return VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI; - } else { - /* Explicitly fallback to legacy USB controller for PPC64. */ - return -1; - } - } else if (def->os.arch == VIR_ARCH_AARCH64) { + + /* For ppc64 specifically, returning -1 here will result in + * an attempt to use the legacy USB controller rather than an + * outright failure */ + return -1; + } + + if (def->os.arch == VIR_ARCH_AARCH64) { + /* Prefer USB3 */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI)) return VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI; - else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI)) + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI)) return VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI; + + /* We should probably return -1 here and avoid the + * possibility of 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)) 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 */ return -2; } } - /* Default USB controller is piix3-uhci if available. */ + /* piix3-uhci (USB1) is the last ditch effort before giving up */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI)) return VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI; + /* No suitable model could be found. This will return in either + * an error or the use of the legacy USB controller */ return -1; } -- 2.43.0

On Wed, Jan 24, 2024 at 20:37:41 +0100, Andrea Bolognani wrote:
Mostly reduce the number of 'else if' and improve comments.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 62 ++++++++++++++++++++++++------------------ 1 file changed, 36 insertions(+), 26 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d992b51877..dcf73c0e08 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c
[...]
@@ -4200,50 +4198,62 @@ qemuDomainDefaultUSBControllerModel(const virDomainDef *def,
[...]
- } else if (def->os.arch == VIR_ARCH_AARCH64) { + + /* For ppc64 specifically, returning -1 here will result in
VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT
+ * an attempt to use the legacy USB controller rather than an + * outright failure */ + return -1;
VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT
+ } + + if (def->os.arch == VIR_ARCH_AARCH64) { + /* Prefer USB3 */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI)) return VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI; - else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI)) + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI)) return VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI; + + /* We should probably return -1 here and avoid the
VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT/VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE
+ * possibility of 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)) 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 */ return -2; } }
- /* Default USB controller is piix3-uhci if available. */ + /* piix3-uhci (USB1) is the last ditch effort before giving up */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI)) return VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI;
+ /* No suitable model could be found. This will return in either + * an error or the use of the legacy USB controller */
The caller will report this as an error or use the legacy USB controller.
return -1; }
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

This function is tightly coupled with qemuDomainDefaultUSBControllerModel(), as whether or not the legacy '-usb' option is going to be used depends on the return value of both. As such, it makes sense for them to be close to one another. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 16 ++-------------- src/qemu/qemu_domain.c | 12 ++++++++++++ src/qemu/qemu_domain.h | 1 + 3 files changed, 15 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4aad7066e9..ec4982bbf6 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2918,18 +2918,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) @@ -2961,7 +2949,7 @@ qemuBuildLegacyUSBControllerCommandLine(virCommand *cmd, } if (nusb == 0 && - !qemuBuildDomainForbidLegacyUSBController(def) && + !qemuDomainForbidLegacyUSBController(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 @@ -3061,7 +3049,7 @@ qemuBuildControllersByTypeCommandLine(virCommand *cmd, if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT && - !qemuBuildDomainForbidLegacyUSBController(def)) { + !qemuDomainForbidLegacyUSBController(def)) { /* An appropriate default USB controller model should already * have been selected in qemuDomainDeviceDefPostParse(); if diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index dcf73c0e08..2b7eae295b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4258,6 +4258,18 @@ qemuDomainDefaultUSBControllerModel(const virDomainDef *def, } +bool +qemuDomainForbidLegacyUSBController(const virDomainDef *def) +{ + if (qemuDomainIsQ35(def) || + qemuDomainIsARMVirt(def) || + qemuDomainIsRISCVVirt(def)) + return true; + + return false; +} + + static int qemuDomainDefAddDefaultDevices(virQEMUDriver *driver, virDomainDef *def, diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 48f966fa2a..4e61d741f3 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -839,6 +839,7 @@ bool qemuDomainSupportsPCIMultibus(const virDomainDef *def); int qemuDomainDefaultSCSIControllerModel(const virDomainDef *def, const virDomainControllerDef *cont, virQEMUCaps *qemuCaps); +bool qemuDomainForbidLegacyUSBController(const virDomainDef *def); void qemuDomainUpdateCurrentMemorySize(virDomainObj *vm); -- 2.43.0

On Wed, Jan 24, 2024 at 20:37:42 +0100, Andrea Bolognani wrote:
This function is tightly coupled with qemuDomainDefaultUSBControllerModel(), as whether or not the legacy '-usb' option is going to be used depends on the return value of both. As such, it makes sense for them to be close to one another.
Honestly this logic has nothing to do in the commandline generator, but that involves the decisions done when calling this function. For now: Reviewed-by: Peter Krempa <pkrempa@redhat.com> I'll see how I like the further changes. OTherwise it'll be more beneficial to remove also the rest of the logic and not just this helper.

Currently, we have special handling for USB controllers of s390x guests hardcoded into the command line generator. This is not great from a layering point of view and, given the complex interactions between the various parts, just makes things very confusing. In order to make things easier to reason about and centralize decision making, increase the number of possible return values. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 5 ++--- src/qemu/qemu_domain.c | 28 +++++++++++++++++++++++++--- src/qemu/qemu_domain.h | 2 +- 3 files changed, 28 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ec4982bbf6..deb8867938 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2949,8 +2949,7 @@ qemuBuildLegacyUSBControllerCommandLine(virCommand *cmd, } if (nusb == 0 && - !qemuDomainForbidLegacyUSBController(def) && - !ARCH_IS_S390(def->os.arch)) { + qemuDomainForbidLegacyUSBController(def) == 0) { /* 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 */ @@ -3049,7 +3048,7 @@ qemuBuildControllersByTypeCommandLine(virCommand *cmd, if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT && - !qemuDomainForbidLegacyUSBController(def)) { + qemuDomainForbidLegacyUSBController(def) <= 0) { /* An appropriate default USB controller model should already * have been selected in qemuDomainDeviceDefPostParse(); if diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2b7eae295b..5b93529655 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4258,15 +4258,37 @@ qemuDomainDefaultUSBControllerModel(const virDomainDef *def, } -bool +/** + * qemuDomainForbidLegacyUSBController: + * @def: domain definition + * + * Tells the command line generator whether it's acceptable to fall + * back to the legacy '-usb' option when a specific model hasn't been + * provided or automatically selected for the USB controller. + * + * Returns: 0 if '-usb' can be used, + * >0 if '-usb' should be avoided, and + * <0 if '-usb' should't be used but the fact that we have + * reached the point where that was the only remaining + * option shouldn't be considered an overall failure. + */ +int qemuDomainForbidLegacyUSBController(const virDomainDef *def) { + /* Modern guests should never use the legacy controller */ if (qemuDomainIsQ35(def) || qemuDomainIsARMVirt(def) || qemuDomainIsRISCVVirt(def)) - return true; + return 1; - return false; + /* In the case of s390x, we also never want to use the legacy + * controller but it's okay if that means that we end up creating + * no USB controller at all */ + if (ARCH_IS_S390(def->os.arch)) + return -1; + + /* In all other cases, using the legacy controller is okay */ + return 0; } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 4e61d741f3..ff95c392bd 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -839,7 +839,7 @@ bool qemuDomainSupportsPCIMultibus(const virDomainDef *def); int qemuDomainDefaultSCSIControllerModel(const virDomainDef *def, const virDomainControllerDef *cont, virQEMUCaps *qemuCaps); -bool qemuDomainForbidLegacyUSBController(const virDomainDef *def); +int qemuDomainForbidLegacyUSBController(const virDomainDef *def); void qemuDomainUpdateCurrentMemorySize(virDomainObj *vm); -- 2.43.0

On Wed, Jan 24, 2024 at 20:37:43 +0100, Andrea Bolognani wrote:
Currently, we have special handling for USB controllers of s390x guests hardcoded into the command line generator. This is not great from a layering point of view and, given the complex interactions between the various parts, just makes things very confusing.
In order to make things easier to reason about and centralize decision making, increase the number of possible return values.
Honestly, to centralize decision making, the commandline code should not at all call this function but simply rely on pre-filled list of controllers. If the list is empty or contains a _NONE controller, don't format anything. for _DEFAULT it should do '-usb'. Errors and anything else should be decided before. IMO this patch doesn't do anything for that. For this series you should be able to separate the USB-unrelated changes and get them merged. If you want to have a look at doing this properly then go ahead, otherwise I'll try having a look.

On Thu, Jan 25, 2024 at 04:45:19PM +0100, Peter Krempa wrote:
On Wed, Jan 24, 2024 at 20:37:43 +0100, Andrea Bolognani wrote:
Currently, we have special handling for USB controllers of s390x guests hardcoded into the command line generator. This is not great from a layering point of view and, given the complex interactions between the various parts, just makes things very confusing.
In order to make things easier to reason about and centralize decision making, increase the number of possible return values.
Honestly, to centralize decision making, the commandline code should not at all call this function but simply rely on pre-filled list of controllers. If the list is empty or contains a _NONE controller, don't format anything. for _DEFAULT it should do '-usb'. Errors and anything else should be decided before.
IMO this patch doesn't do anything for that.
For this series you should be able to separate the USB-unrelated changes and get them merged.
If you want to have a look at doing this properly then go ahead, otherwise I'll try having a look.
I can try looking into it, but I'm not convinced that's going to improve things as much as you hope. I'd love to be proven wrong though :) -- Andrea Bolognani / Red Hat / Virtualization

Factor out the existing code. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 5b93529655..b3e63b3648 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4292,6 +4292,26 @@ qemuDomainForbidLegacyUSBController(const virDomainDef *def) } +static int +qemuDomainDefaultSerialType(const virDomainDef *def) +{ + if (ARCH_IS_X86(def->os.arch)) + return VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA; + + if (qemuDomainIsPSeries(def)) + return VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SPAPR_VIO; + + if (ARCH_IS_S390(def->os.arch)) + return VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SCLP; + + if (qemuDomainIsARMVirt(def) || + qemuDomainIsRISCVVirt(def)) + return VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SYSTEM; + + return VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE; +} + + static int qemuDomainDefAddDefaultDevices(virQEMUDriver *driver, virDomainDef *def, @@ -5872,15 +5892,7 @@ qemuDomainChrDefPostParse(virDomainChrDef *chr, /* Set the default serial type */ if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL && chr->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE) { - if (ARCH_IS_X86(def->os.arch)) { - chr->targetType = VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA; - } else if (qemuDomainIsPSeries(def)) { - chr->targetType = VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SPAPR_VIO; - } else if (qemuDomainIsARMVirt(def) || qemuDomainIsRISCVVirt(def)) { - chr->targetType = VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SYSTEM; - } else if (ARCH_IS_S390(def->os.arch)) { - chr->targetType = VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SCLP; - } + chr->targetType = qemuDomainDefaultSerialType(def); } /* Set the default target model */ -- 2.43.0

On Wed, Jan 24, 2024 at 20:37:44 +0100, Andrea Bolognani wrote:
Factor out the existing code.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

Factor out the existing code. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 66 ++++++++++++++++++++++-------------------- 1 file changed, 35 insertions(+), 31 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b3e63b3648..c9d213dd7b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4312,6 +4312,40 @@ qemuDomainDefaultSerialType(const virDomainDef *def) } +static int +qemuDomainDefaultSerialModel(const virDomainDef *def, + const virDomainChrDef *chr) +{ + switch ((virDomainChrSerialTargetType)chr->targetType) { + case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA: + return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL; + case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB: + return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_USB_SERIAL; + case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI: + return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PCI_SERIAL; + case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SPAPR_VIO: + return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SPAPR_VTY; + case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SYSTEM: + if (qemuDomainIsARMVirt(def)) { + return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PL011; + } else if (qemuDomainIsRISCVVirt(def)) { + return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_16550A; + } + break; + case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SCLP: + return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPCONSOLE; + case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA_DEBUG: + return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_DEBUGCON; + case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE: + case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST: + /* Nothing to do */ + break; + } + + return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_NONE; +} + + static int qemuDomainDefAddDefaultDevices(virQEMUDriver *driver, virDomainDef *def, @@ -5898,37 +5932,7 @@ qemuDomainChrDefPostParse(virDomainChrDef *chr, /* Set the default target model */ if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL && chr->targetModel == VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_NONE) { - switch ((virDomainChrSerialTargetType)chr->targetType) { - case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA: - chr->targetModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL; - break; - case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB: - chr->targetModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_USB_SERIAL; - break; - case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI: - chr->targetModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PCI_SERIAL; - break; - case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SPAPR_VIO: - chr->targetModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SPAPR_VTY; - break; - case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SYSTEM: - if (qemuDomainIsARMVirt(def)) { - chr->targetModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PL011; - } else if (qemuDomainIsRISCVVirt(def)) { - chr->targetModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_16550A; - } - break; - case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SCLP: - chr->targetModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPCONSOLE; - break; - case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA_DEBUG: - chr->targetModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_DEBUGCON; - break; - case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE: - case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST: - /* Nothing to do */ - break; - } + chr->targetModel = qemuDomainDefaultSerialModel(def, chr); } /* clear auto generated unix socket path for inactive definitions */ -- 2.43.0

On Wed, Jan 24, 2024 at 20:37:45 +0100, Andrea Bolognani wrote:
Factor out the existing code.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 66 ++++++++++++++++++++++-------------------- 1 file changed, 35 insertions(+), 31 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b3e63b3648..c9d213dd7b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4312,6 +4312,40 @@ qemuDomainDefaultSerialType(const virDomainDef *def) }
+static int +qemuDomainDefaultSerialModel(const virDomainDef *def, + const virDomainChrDef *chr) +{ + switch ((virDomainChrSerialTargetType)chr->targetType) { + case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA: + return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL; + case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB: + return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_USB_SERIAL; + case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI: + return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PCI_SERIAL; + case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SPAPR_VIO: + return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SPAPR_VTY; + case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SYSTEM: + if (qemuDomainIsARMVirt(def)) { + return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PL011; + } else if (qemuDomainIsRISCVVirt(def)) { + return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_16550A; + } + break; + case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SCLP: + return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPCONSOLE; + case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA_DEBUG: + return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_DEBUGCON; + case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE: + case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST: + /* Nothing to do */ + break; + }
Preferrably add visual separation around each case.
+ + return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_NONE; +} + + static int
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

On Wed, Jan 24, 2024 at 20:37:45 +0100, Andrea Bolognani wrote:
Factor out the existing code.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 66 ++++++++++++++++++++++-------------------- 1 file changed, 35 insertions(+), 31 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b3e63b3648..c9d213dd7b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4312,6 +4312,40 @@ qemuDomainDefaultSerialType(const virDomainDef *def) }
+static int
IMO at this point you can return virDomainChrSerialTargetModel.

On Thu, Jan 25, 2024 at 03:50:33PM +0100, Peter Krempa wrote:
On Wed, Jan 24, 2024 at 20:37:45 +0100, Andrea Bolognani wrote:
+static int
IMO at this point you can return virDomainChrSerialTargetModel.
Sure, I can try that. But what's the advantage? Ultimately we store most (all?) models as int inside structs, and only cast them to the proper enum type for switches. Which is a real shame, obviously, but IIRC was done for Some Actual Reasonâ„¢. Or am I misremembering? -- Andrea Bolognani / Red Hat / Virtualization

On Thu, Jan 25, 2024 at 11:07:41 -0800, Andrea Bolognani wrote:
On Thu, Jan 25, 2024 at 03:50:33PM +0100, Peter Krempa wrote:
On Wed, Jan 24, 2024 at 20:37:45 +0100, Andrea Bolognani wrote:
+static int
IMO at this point you can return virDomainChrSerialTargetModel.
Sure, I can try that. But what's the advantage? Ultimately we store most (all?) models as int inside structs, and only cast them to the
This pattern is discouraged and being actively elliminated. The main reason for this pattern to exist was that the enum*TypeFromString function returns -1 on error, so you can't directly assign it into a enum type which is unsigned.
proper enum type for switches. Which is a real shame, obviously, but IIRC was done for Some Actual Reasonâ„¢. Or am I misremembering?
The additional reason here is that if somebody will be editting this function and decide to add a return of an arbitrary -1 or -2, returning a proper enum type will make them reconsider what they are doing and look at all callers before doing so.

Factor out the existing code. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c9d213dd7b..8a37cda464 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4346,6 +4346,19 @@ qemuDomainDefaultSerialModel(const virDomainDef *def, } +static int +qemuDomainDefaultPanicModel(const virDomainDef *def) +{ + if (qemuDomainIsPSeries(def)) + return VIR_DOMAIN_PANIC_MODEL_PSERIES; + + if (ARCH_IS_S390(def->os.arch)) + return VIR_DOMAIN_PANIC_MODEL_S390; + + return VIR_DOMAIN_PANIC_MODEL_ISA; +} + + static int qemuDomainDefAddDefaultDevices(virQEMUDriver *driver, virDomainDef *def, @@ -6123,14 +6136,8 @@ static int qemuDomainDevicePanicDefPostParse(virDomainPanicDef *panic, const virDomainDef *def) { - if (panic->model == VIR_DOMAIN_PANIC_MODEL_DEFAULT) { - if (qemuDomainIsPSeries(def)) - panic->model = VIR_DOMAIN_PANIC_MODEL_PSERIES; - else if (ARCH_IS_S390(def->os.arch)) - panic->model = VIR_DOMAIN_PANIC_MODEL_S390; - else - panic->model = VIR_DOMAIN_PANIC_MODEL_ISA; - } + if (panic->model == VIR_DOMAIN_PANIC_MODEL_DEFAULT) + panic->model = qemuDomainDefaultPanicModel(def); return 0; } -- 2.43.0

On Wed, Jan 24, 2024 at 20:37:46 +0100, Andrea Bolognani wrote:
Factor out the existing code.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c9d213dd7b..8a37cda464 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4346,6 +4346,19 @@ qemuDomainDefaultSerialModel(const virDomainDef *def, }
+static int +qemuDomainDefaultPanicModel(const virDomainDef *def) +{ + if (qemuDomainIsPSeries(def)) + return VIR_DOMAIN_PANIC_MODEL_PSERIES; + + if (ARCH_IS_S390(def->os.arch)) + return VIR_DOMAIN_PANIC_MODEL_S390; +
You see, this is much better ;)
+ return VIR_DOMAIN_PANIC_MODEL_ISA; +} +
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

On Wed, Jan 24, 2024 at 20:37:46 +0100, Andrea Bolognani wrote:
Factor out the existing code.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c9d213dd7b..8a37cda464 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4346,6 +4346,19 @@ qemuDomainDefaultSerialModel(const virDomainDef *def, }
+static int
static virDomainPanicModel
+qemuDomainDefaultPanicModel(const virDomainDef *def) +{ + if (qemuDomainIsPSeries(def)) + return VIR_DOMAIN_PANIC_MODEL_PSERIES; + + if (ARCH_IS_S390(def->os.arch)) + return VIR_DOMAIN_PANIC_MODEL_S390; + + return VIR_DOMAIN_PANIC_MODEL_ISA; +}

qemuDomainDefAddDefaultDevices() contained what is essentialy an open-coded implementation of the helper. Get rid of it. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8a37cda464..62a847d47f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4591,12 +4591,11 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver *driver, if (addPanicDevice) { size_t j; + int defaultPanicModel = qemuDomainDefaultPanicModel(def); + for (j = 0; j < def->npanics; j++) { if (def->panics[j]->model == VIR_DOMAIN_PANIC_MODEL_DEFAULT || - (ARCH_IS_PPC64(def->os.arch) && - def->panics[j]->model == VIR_DOMAIN_PANIC_MODEL_PSERIES) || - (ARCH_IS_S390(def->os.arch) && - def->panics[j]->model == VIR_DOMAIN_PANIC_MODEL_S390)) + def->panics[j]->model == defaultPanicModel) break; } -- 2.43.0

On Wed, Jan 24, 2024 at 20:37:47 +0100, Andrea Bolognani wrote:
qemuDomainDefAddDefaultDevices() contained what is essentialy an open-coded implementation of the helper. Get rid of it.
It would be beneficial to note here that addPanicDevice is 'true' only for ARCH_IS_PPC64 and ARCH_IS_S390 thus this change is equivalent.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8a37cda464..62a847d47f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4591,12 +4591,11 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver *driver,
if (addPanicDevice) { size_t j; + int defaultPanicModel = qemuDomainDefaultPanicModel(def);
virDomainPanicModel instead of int.
+ for (j = 0; j < def->npanics; j++) { if (def->panics[j]->model == VIR_DOMAIN_PANIC_MODEL_DEFAULT || - (ARCH_IS_PPC64(def->os.arch) && - def->panics[j]->model == VIR_DOMAIN_PANIC_MODEL_PSERIES) || - (ARCH_IS_S390(def->os.arch) && - def->panics[j]->model == VIR_DOMAIN_PANIC_MODEL_S390)) + def->panics[j]->model == defaultPanicModel)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

Follow the established naming convention for consistency. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 62a847d47f..22980c25a9 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6096,20 +6096,23 @@ qemuDomainDeviceNetDefPostParse(virDomainNetDef *net, static int -qemuDomainDefaultVideoDevice(const virDomainDef *def, - virQEMUCaps *qemuCaps) +qemuDomainDefaultVideoModel(const virDomainDef *def, + virQEMUCaps *qemuCaps) { if (ARCH_IS_PPC64(def->os.arch)) return VIR_DOMAIN_VIDEO_TYPE_VGA; + if (qemuDomainIsARMVirt(def) || qemuDomainIsRISCVVirt(def) || ARCH_IS_S390(def->os.arch)) { return VIR_DOMAIN_VIDEO_TYPE_VIRTIO; } + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_CIRRUS_VGA)) return VIR_DOMAIN_VIDEO_TYPE_CIRRUS; if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VGA)) return VIR_DOMAIN_VIDEO_TYPE_VGA; + return VIR_DOMAIN_VIDEO_TYPE_DEFAULT; } @@ -6120,7 +6123,7 @@ qemuDomainDeviceVideoDefPostParse(virDomainVideoDef *video, virQEMUCaps *qemuCaps) { if (video->type == VIR_DOMAIN_VIDEO_TYPE_DEFAULT) - video->type = qemuDomainDefaultVideoDevice(def, qemuCaps); + video->type = qemuDomainDefaultVideoModel(def, qemuCaps); if (video->type == VIR_DOMAIN_VIDEO_TYPE_QXL && !video->vgamem) { -- 2.43.0

On Wed, Jan 24, 2024 at 20:37:48 +0100, Andrea Bolognani wrote:
Follow the established naming convention for consistency.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 62a847d47f..22980c25a9 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6096,20 +6096,23 @@ qemuDomainDeviceNetDefPostParse(virDomainNetDef *net,
static int -qemuDomainDefaultVideoDevice(const virDomainDef *def, - virQEMUCaps *qemuCaps) +qemuDomainDefaultVideoModel(const virDomainDef *def,
For consistency with what I've pointed out in previous patches you can also return virDomainVideoType Reviewed-by: Peter Krempa <pkrempa@redhat.com>

Most of the functions responsible for choosing architecture and machine specific defaults are already close to one another, with just a couple of strays. Having everything in one place will hopefully make it harder to miss updating any of the functions when new architectures are being introduced. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 151 ++++++++++++++++++++--------------------- 1 file changed, 75 insertions(+), 76 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 22980c25a9..cede7c9eb2 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4292,6 +4292,81 @@ qemuDomainForbidLegacyUSBController(const virDomainDef *def) } +/** + * qemuDomainDefaultNetModel: + * @def: domain definition + * @qemuCaps: qemu capabilities + * + * Returns the default network model for a given domain. Note that if @qemuCaps + * is NULL this function may return NULL if the default model depends on the + * capabilities. + */ +static int +qemuDomainDefaultNetModel(const virDomainDef *def, + virQEMUCaps *qemuCaps) +{ + /* When there are no backwards compatibility concerns getting in + * the way, virtio is a good default */ + if (ARCH_IS_S390(def->os.arch) || + qemuDomainIsRISCVVirt(def)) { + return VIR_DOMAIN_NET_MODEL_VIRTIO; + } + + if (ARCH_IS_ARM(def->os.arch)) { + if (STREQ(def->os.machine, "versatilepb")) + return VIR_DOMAIN_NET_MODEL_SMC91C111; + + if (qemuDomainIsARMVirt(def)) + return VIR_DOMAIN_NET_MODEL_VIRTIO; + + /* Incomplete. vexpress (and a few others) use this, but not all + * arm boards */ + return VIR_DOMAIN_NET_MODEL_LAN9118; + } + + /* In all other cases the model depends on the capabilities. If they were + * not provided don't report any default. */ + if (!qemuCaps) + return VIR_DOMAIN_NET_MODEL_UNKNOWN; + + /* Try several network devices in turn; each of these devices is + * less likely be supported out-of-the-box by the guest operating + * system than the previous one */ + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_RTL8139)) + return VIR_DOMAIN_NET_MODEL_RTL8139; + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_E1000)) + return VIR_DOMAIN_NET_MODEL_E1000; + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_NET)) + return VIR_DOMAIN_NET_MODEL_VIRTIO; + + /* We've had no luck detecting support for any network device, + * but we have to return something: might as well be rtl8139 */ + return VIR_DOMAIN_NET_MODEL_RTL8139; +} + + +static int +qemuDomainDefaultVideoModel(const virDomainDef *def, + virQEMUCaps *qemuCaps) +{ + if (ARCH_IS_PPC64(def->os.arch)) + return VIR_DOMAIN_VIDEO_TYPE_VGA; + + if (qemuDomainIsARMVirt(def) || + qemuDomainIsRISCVVirt(def) || + ARCH_IS_S390(def->os.arch)) { + return VIR_DOMAIN_VIDEO_TYPE_VIRTIO; + } + + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_CIRRUS_VGA)) + return VIR_DOMAIN_VIDEO_TYPE_CIRRUS; + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VGA)) + return VIR_DOMAIN_VIDEO_TYPE_VGA; + + return VIR_DOMAIN_VIDEO_TYPE_DEFAULT; +} + + static int qemuDomainDefaultSerialType(const virDomainDef *def) { @@ -5633,60 +5708,6 @@ qemuDomainValidateStorageSource(virStorageSource *src, } -/** - * qemuDomainDefaultNetModel: - * @def: domain definition - * @qemuCaps: qemu capabilities - * - * Returns the default network model for a given domain. Note that if @qemuCaps - * is NULL this function may return NULL if the default model depends on the - * capabilities. - */ -static int -qemuDomainDefaultNetModel(const virDomainDef *def, - virQEMUCaps *qemuCaps) -{ - /* When there are no backwards compatibility concerns getting in - * the way, virtio is a good default */ - if (ARCH_IS_S390(def->os.arch) || - qemuDomainIsRISCVVirt(def)) { - return VIR_DOMAIN_NET_MODEL_VIRTIO; - } - - if (ARCH_IS_ARM(def->os.arch)) { - if (STREQ(def->os.machine, "versatilepb")) - return VIR_DOMAIN_NET_MODEL_SMC91C111; - - if (qemuDomainIsARMVirt(def)) - return VIR_DOMAIN_NET_MODEL_VIRTIO; - - /* Incomplete. vexpress (and a few others) use this, but not all - * arm boards */ - return VIR_DOMAIN_NET_MODEL_LAN9118; - } - - /* In all other cases the model depends on the capabilities. If they were - * not provided don't report any default. */ - if (!qemuCaps) - return VIR_DOMAIN_NET_MODEL_UNKNOWN; - - /* Try several network devices in turn; each of these devices is - * less likely be supported out-of-the-box by the guest operating - * system than the previous one */ - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_RTL8139)) - return VIR_DOMAIN_NET_MODEL_RTL8139; - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_E1000)) - return VIR_DOMAIN_NET_MODEL_E1000; - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_NET)) - return VIR_DOMAIN_NET_MODEL_VIRTIO; - - /* We've had no luck detecting support for any network device, - * but we have to return something: might as well be rtl8139 */ - return VIR_DOMAIN_NET_MODEL_RTL8139; -} - - - static bool qemuDomainChrMatchDefaultPath(const char *prefix, const char *infix, @@ -6095,28 +6116,6 @@ qemuDomainDeviceNetDefPostParse(virDomainNetDef *net, } -static int -qemuDomainDefaultVideoModel(const virDomainDef *def, - virQEMUCaps *qemuCaps) -{ - if (ARCH_IS_PPC64(def->os.arch)) - return VIR_DOMAIN_VIDEO_TYPE_VGA; - - if (qemuDomainIsARMVirt(def) || - qemuDomainIsRISCVVirt(def) || - ARCH_IS_S390(def->os.arch)) { - return VIR_DOMAIN_VIDEO_TYPE_VIRTIO; - } - - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_CIRRUS_VGA)) - return VIR_DOMAIN_VIDEO_TYPE_CIRRUS; - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VGA)) - return VIR_DOMAIN_VIDEO_TYPE_VGA; - - return VIR_DOMAIN_VIDEO_TYPE_DEFAULT; -} - - static int qemuDomainDeviceVideoDefPostParse(virDomainVideoDef *video, const virDomainDef *def, -- 2.43.0

On Wed, Jan 24, 2024 at 20:37:49 +0100, Andrea Bolognani wrote:
Most of the functions responsible for choosing architecture and machine specific defaults are already close to one another, with just a couple of strays. Having everything in one place will hopefully make it harder to miss updating any of the functions when new architectures are being introduced.
Too bad that the code is using a eclectic selection of ARCH_IS_* macros together with checkers such as qemuDomainIsRISCVVirt etc, because it's hard to create a proper fix which would be a properly typed switch statement, where the compiler would enforce what you want to achieve here. Also too bad that the list of arches is *massive* to have it everywhere, despite the fac that we effectively ignore a half of them.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 151 ++++++++++++++++++++--------------------- 1 file changed, 75 insertions(+), 76 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

On Thu, Jan 25, 2024 at 03:58:29PM +0100, Peter Krempa wrote:
On Wed, Jan 24, 2024 at 20:37:49 +0100, Andrea Bolognani wrote:
Most of the functions responsible for choosing architecture and machine specific defaults are already close to one another, with just a couple of strays. Having everything in one place will hopefully make it harder to miss updating any of the functions when new architectures are being introduced.
Too bad that the code is using a eclectic selection of ARCH_IS_* macros together with checkers such as qemuDomainIsRISCVVirt etc, because it's hard to create a proper fix which would be a properly typed switch statement, where the compiler would enforce what you want to achieve here.
Unfortunately that's needed because some architectures have wildly different machine types. On Arm, for example, the virt machine type is lean, modern and geared towards PCI/virtio, but you also have machine types implementing very old embedded boards where all controllers are hard-coded. The solution would be to restrict support to the small subset of virt-friendly machine types, one or two per architecture, that we actually test in any capacity, but since things have been free-for-all until now there is the expectation that even machine types that libvirt knows nothing about will keep working the same as they ever have.
Also too bad that the list of arches is *massive* to have it everywhere, despite the fac that we effectively ignore a half of them.
Yeah, my first instinct was to add switch()es everywhere, but as soon as I started I realized that unfortunately it just made the code much, much worse :( -- Andrea Bolognani / Red Hat / Virtualization

Other architectures generally don't have an ISA bus, so this default never worked for them. Since it's now possible for a specific model not to be chosen during postparse, something that couldn't happen until now, we need to handle the scenario by presenting the user with a reasonable error message. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 5 ++++- src/qemu/qemu_validate.c | 9 ++++++++- .../aarch64-panic-no-model.aarch64-latest.err | 2 +- 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index cede7c9eb2..506f03831c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4430,7 +4430,10 @@ qemuDomainDefaultPanicModel(const virDomainDef *def) if (ARCH_IS_S390(def->os.arch)) return VIR_DOMAIN_PANIC_MODEL_S390; - return VIR_DOMAIN_PANIC_MODEL_ISA; + if (ARCH_IS_X86(def->os.arch)) + return VIR_DOMAIN_PANIC_MODEL_ISA; + + return VIR_DOMAIN_PANIC_MODEL_DEFAULT; } diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 01f65c0866..db696b3d5a 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -1009,8 +1009,15 @@ qemuValidateDomainDefPanic(const virDomainDef *def, } break; - /* default model value was changed before in post parse */ + /* If a reasonable default exists for the architecture and + * machine type, it will have been set during postparse. + * Getting there means that we really need the user to + * provide an explicit model name */ case VIR_DOMAIN_PANIC_MODEL_DEFAULT: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("no model provided for panic device")); + return -1; + case VIR_DOMAIN_PANIC_MODEL_LAST: break; } diff --git a/tests/qemuxmlconfdata/aarch64-panic-no-model.aarch64-latest.err b/tests/qemuxmlconfdata/aarch64-panic-no-model.aarch64-latest.err index 8e3f2c194d..2635f23769 100644 --- a/tests/qemuxmlconfdata/aarch64-panic-no-model.aarch64-latest.err +++ b/tests/qemuxmlconfdata/aarch64-panic-no-model.aarch64-latest.err @@ -1 +1 @@ -unsupported configuration: the QEMU binary does not support the ISA panic device +unsupported configuration: no model provided for panic device -- 2.43.0

On Wed, Jan 24, 2024 at 20:37:50 +0100, Andrea Bolognani wrote:
Other architectures generally don't have an ISA bus, so this default never worked for them.
Since it's now possible for a specific model not to be chosen during postparse, something that couldn't happen until now, we need to handle the scenario by presenting the user with a reasonable error message.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 5 ++++- src/qemu/qemu_validate.c | 9 ++++++++- .../aarch64-panic-no-model.aarch64-latest.err | 2 +- 3 files changed, 13 insertions(+), 3 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

The idea of adding devices such as USB controllers or memory balloons by default comes from attempting to match QEMU's own defaults at a time when x86 was the only game in town. The unfortunate consequence of this is that, if the user does NOT want the device in question to be present, they have to create a special XML element with model=none to stop libvirt. This is counter-intuitive. For architectures for which we've added support more recently, such as aarch64, we've generally chosen to do the sensible thing and create very minimal guests by default. The user is of course still able to ask for additional hardware if they so desire. When adding RISC-V support, we accidentally forgot to skip the creation of the default memory balloon. Address that oversight. This is technically a breaking change, but it's fairly safe to apply it because: * it doesn't affect existing guests; * virt-manager will automatically add the memballoon device by default anyway; * RISC-V is still not widely used. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 1 - .../riscv64-virt-minimal.riscv64-latest.args | 3 --- .../riscv64-virt-minimal.riscv64-latest.xml | 13 ------------- 3 files changed, 17 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 506f03831c..bf45198c09 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4517,7 +4517,6 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver *driver, case VIR_ARCH_RISCV32: case VIR_ARCH_RISCV64: - addDefaultMemballoon = true; if (qemuDomainIsRISCVVirt(def)) addPCIeRoot = true; break; diff --git a/tests/qemuxmlconfdata/riscv64-virt-minimal.riscv64-latest.args b/tests/qemuxmlconfdata/riscv64-virt-minimal.riscv64-latest.args index fcb80b009e..c38dddb5fa 100644 --- a/tests/qemuxmlconfdata/riscv64-virt-minimal.riscv64-latest.args +++ b/tests/qemuxmlconfdata/riscv64-virt-minimal.riscv64-latest.args @@ -25,9 +25,6 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-guest/.config \ -rtc base=utc \ -no-shutdown \ -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"}' \ -audiodev '{"id":"audio1","driver":"none"}' \ --device '{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.1","addr":"0x0"}' \ -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ -msg timestamp=on diff --git a/tests/qemuxmlconfdata/riscv64-virt-minimal.riscv64-latest.xml b/tests/qemuxmlconfdata/riscv64-virt-minimal.riscv64-latest.xml index 54363bb426..a96af29587 100644 --- a/tests/qemuxmlconfdata/riscv64-virt-minimal.riscv64-latest.xml +++ b/tests/qemuxmlconfdata/riscv64-virt-minimal.riscv64-latest.xml @@ -15,19 +15,6 @@ <devices> <emulator>/usr/bin/qemu-system-riscv64</emulator> <controller type='pci' index='0' model='pcie-root'/> - <controller type='pci' index='1' model='pcie-root-port'> - <model name='pcie-root-port'/> - <target chassis='1' port='0x8'/> - <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0' multifunction='on'/> - </controller> - <controller type='pci' index='2' model='pcie-root-port'> - <model name='pcie-root-port'/> - <target chassis='2' port='0x9'/> - <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> - </controller> <audio id='1' type='none'/> - <memballoon model='virtio'> - <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> - </memballoon> </devices> </domain> -- 2.43.0

On Wed, Jan 24, 2024 at 20:37:51 +0100, Andrea Bolognani wrote:
The idea of adding devices such as USB controllers or memory balloons by default comes from attempting to match QEMU's own defaults at a time when x86 was the only game in town.
The unfortunate consequence of this is that, if the user does NOT want the device in question to be present, they have to create a special XML element with model=none to stop libvirt. This is counter-intuitive.
For architectures for which we've added support more recently, such as aarch64, we've generally chosen to do the sensible thing and create very minimal guests by default. The user is of course still able to ask for additional hardware if they so desire.
When adding RISC-V support, we accidentally forgot to skip the creation of the default memory balloon. Address that oversight.
This is technically a breaking change, but it's fairly safe to apply it because:
* it doesn't affect existing guests; * virt-manager will automatically add the memballoon device by default anyway; * RISC-V is still not widely used.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 1 - .../riscv64-virt-minimal.riscv64-latest.args | 3 --- .../riscv64-virt-minimal.riscv64-latest.xml | 13 ------------- 3 files changed, 17 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

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@redhat.com> --- src/qemu/qemu_domain.c | 9 +++++++ ...64-virt-default-models.riscv64-latest.args | 11 ++++---- ...v64-virt-default-models.riscv64-latest.xml | 25 +++++++++++-------- 3 files changed, 30 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index bf45198c09..bc83052920 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4248,6 +4248,15 @@ qemuDomainDefaultUSBControllerModel(const virDomainDef *def, } } + if (ARCH_IS_RISCV(def->os.arch)) { + if (qemuDomainIsRISCVVirt(def)) { + /* USB3 only, no fallback */ + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI)) + return VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI; + return -1; + } + } + /* piix3-uhci (USB1) is the last ditch effort before giving up */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI)) return VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI; diff --git a/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.args b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.args index 28b56d876c..3a5cceb5e7 100644 --- a/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.args +++ b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.args @@ -27,16 +27,17 @@ 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":10,"chassis":3,"id":"pci.3","bus":"pcie.0","addr":"0x1.0x2"}' \ +-device '{"driver":"pcie-pci-bridge","id":"pci.4","bus":"pci.1","addr":"0x0"}' \ -device '{"driver":"pcie-root-port","port":11,"chassis":5,"id":"pci.5","bus":"pcie.0","addr":"0x1.0x3"}' \ --device '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.3","addr":"0x1"}' \ --device '{"driver":"lsi","id":"scsi0","bus":"pci.3","addr":"0x2"}' \ +-device '{"driver":"pcie-root-port","port":12,"chassis":6,"id":"pci.6","bus":"pcie.0","addr":"0x1.0x4"}' \ +-device '{"driver":"qemu-xhci","id":"usb","bus":"pci.3","addr":"0x0"}' \ +-device '{"driver":"lsi","id":"scsi0","bus":"pci.4","addr":"0x1"}' \ -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..896bd7d0f0 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='0x03' slot='0x00' function='0x0'/> </controller> <controller type='scsi' index='0' model='lsilogic'> - <address type='pci' domain='0x0000' bus='0x03' slot='0x02' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x04' slot='0x01' function='0x0'/> </controller> <controller type='pci' index='0' model='pcie-root'/> <controller type='pci' index='1' model='pcie-root-port'> @@ -31,20 +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='4' 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='5' model='pcie-root-port'> <model name='pcie-root-port'/> <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

On Wed, Jan 24, 2024 at 20:37:52 +0100, Andrea Bolognani wrote:
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@redhat.com> --- src/qemu/qemu_domain.c | 9 +++++++ ...64-virt-default-models.riscv64-latest.args | 11 ++++---- ...v64-virt-default-models.riscv64-latest.xml | 25 +++++++++++-------- 3 files changed, 30 insertions(+), 15 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

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@redhat.com> --- src/qemu/qemu_domain.c | 3 ++- ...64-virt-default-models.riscv64-latest.args | 13 +++++------ ...v64-virt-default-models.riscv64-latest.xml | 22 ++++++++----------- 3 files changed, 17 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index bc83052920..138cf08b37 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4133,7 +4133,8 @@ qemuDomainDefaultSCSIControllerModel(const virDomainDef *def, return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI; /* 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; /* If there is no preference, base the choice on device diff --git a/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.args b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.args index 3a5cceb5e7..155c5aed9c 100644 --- a/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.args +++ b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.args @@ -28,16 +28,15 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-guest/.config \ -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-root-port","port":10,"chassis":3,"id":"pci.3","bus":"pcie.0","addr":"0x1.0x2"}' \ --device '{"driver":"pcie-pci-bridge","id":"pci.4","bus":"pci.1","addr":"0x0"}' \ --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":"qemu-xhci","id":"usb","bus":"pci.3","addr":"0x0"}' \ --device '{"driver":"lsi","id":"scsi0","bus":"pci.4","addr":"0x1"}' \ +-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 896bd7d0f0..4628cd7895 100644 --- a/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.xml +++ b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.xml @@ -15,10 +15,10 @@ <devices> <emulator>/usr/bin/qemu-system-riscv64</emulator> <controller type='usb' index='0' model='qemu-xhci'> - <address type='pci' domain='0x0000' bus='0x03' slot='0x00' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x02' slot='0x00' function='0x0'/> </controller> - <controller type='scsi' index='0' model='lsilogic'> - <address type='pci' domain='0x0000' bus='0x04' slot='0x01' function='0x0'/> + <controller type='scsi' index='0' model='virtio-scsi'> + <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'> @@ -36,24 +36,20 @@ <target chassis='3' port='0xa'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> </controller> - <controller type='pci' index='4' 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='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

On Wed, Jan 24, 2024 at 20:37:53 +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@redhat.com> --- src/qemu/qemu_domain.c | 3 ++- ...64-virt-default-models.riscv64-latest.args | 13 +++++------ ...v64-virt-default-models.riscv64-latest.xml | 22 ++++++++----------- 3 files changed, 17 insertions(+), 21 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (2)
-
Andrea Bolognani
-
Peter Krempa