[PATCH 0/4] qemu: Fill in panic model automatically on aarch64
Back when pvpanic-pci support was introduced 1.5 years ago (!), we required the user to manually provide the model name. This is incovenient and doesn't match the behavior seen on other architectures. Make things more user friendly. Andrea Bolognani (4): tests: Add coverage for panic on riscv64 qemu: Refactor default panic model qemu: Sometimes the default panic model doesn't exist qemu: Use pvpanic by default on aarch64 src/qemu/qemu_domain.c | 36 ++++++++++++------- src/qemu/qemu_validate.c | 6 +++- .../aarch64-panic-no-model.aarch64-latest.err | 1 - ...ault-models.aarch64-latest.abi-update.args | 1 + ...fault-models.aarch64-latest.abi-update.xml | 3 ++ ...64-virt-default-models.aarch64-latest.args | 1 + ...h64-virt-default-models.aarch64-latest.xml | 3 ++ .../aarch64-virt-default-models.xml | 2 +- .../riscv64-panic-no-model.riscv64-latest.err | 1 + ...o-model.xml => riscv64-panic-no-model.xml} | 4 +-- tests/qemuxmlconftest.c | 2 +- 11 files changed, 42 insertions(+), 18 deletions(-) delete mode 100644 tests/qemuxmlconfdata/aarch64-panic-no-model.aarch64-latest.err create mode 100644 tests/qemuxmlconfdata/riscv64-panic-no-model.riscv64-latest.err rename tests/qemuxmlconfdata/{aarch64-panic-no-model.xml => riscv64-panic-no-model.xml} (65%) -- 2.46.0
It merely duplicates the existing aarch64 coverage right now, but it will become actually useful with the upcoming changes. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- .../riscv64-panic-no-model.riscv64-latest.err | 1 + tests/qemuxmlconfdata/riscv64-panic-no-model.xml | 13 +++++++++++++ tests/qemuxmlconftest.c | 1 + 3 files changed, 15 insertions(+) create mode 100644 tests/qemuxmlconfdata/riscv64-panic-no-model.riscv64-latest.err create mode 100644 tests/qemuxmlconfdata/riscv64-panic-no-model.xml diff --git a/tests/qemuxmlconfdata/riscv64-panic-no-model.riscv64-latest.err b/tests/qemuxmlconfdata/riscv64-panic-no-model.riscv64-latest.err new file mode 100644 index 0000000000..8e3f2c194d --- /dev/null +++ b/tests/qemuxmlconfdata/riscv64-panic-no-model.riscv64-latest.err @@ -0,0 +1 @@ +unsupported configuration: the QEMU binary does not support the ISA panic device diff --git a/tests/qemuxmlconfdata/riscv64-panic-no-model.xml b/tests/qemuxmlconfdata/riscv64-panic-no-model.xml new file mode 100644 index 0000000000..9731a997ea --- /dev/null +++ b/tests/qemuxmlconfdata/riscv64-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='riscv64' machine='virt'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-system-riscv64</emulator> + <panic/> + </devices> +</domain> diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c index f7c0cf4ad0..e97d0e7bdc 100644 --- a/tests/qemuxmlconftest.c +++ b/tests/qemuxmlconftest.c @@ -2649,6 +2649,7 @@ mymain(void) 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_ARCH_LATEST_PARSE_ERROR("riscv64-panic-no-model", "riscv64"); DO_TEST_CAPS_LATEST("pvpanic-pci-x86_64"); DO_TEST_CAPS_ARCH_LATEST("pvpanic-pci-aarch64", "aarch64"); -- 2.46.0
Perform decisions based on the architecture and machine type in a single place instead of duplicating them. This technically adds new behavior for MODEL_ISA in qemuDomainDefAddDefaultDevices(), but it doesn't make any difference functionally since we don't set addPanicDevice outside of ppc64(le) and s390(x). If we did, the lack of handling for that value would be a latent bug. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d1767c326d..e460e48fce 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4150,6 +4150,19 @@ qemuDomainGetSCSIControllerModel(const virDomainDef *def, } +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; +} + + static int qemuDomainDefAddDefaultDevices(virQEMUDriver *driver, virDomainDef *def, @@ -4397,13 +4410,12 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver *driver, return -1; if (addPanicDevice) { + virDomainPanicModel defaultModel = qemuDomainDefaultPanicModel(def); size_t j; + 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 == defaultModel) break; } @@ -6100,14 +6112,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.46.0
Right now the fallback behavior is to use MODEL_ISA if we haven't been able to find a better match, but that's not very useful as we're still going to hit an error later, when QEMU_CAPS_DEVICE_PANIC is not found at Validate time. Instead of doing that, allow the MODEL_DEFAULT to get all the way to Validate and report an error upon encountering it. The reported error changes slightly, but other than that the set of configurations that allow and block remains the same. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 5 ++++- src/qemu/qemu_validate.c | 6 +++++- .../aarch64-panic-no-model.aarch64-latest.err | 2 +- .../riscv64-panic-no-model.riscv64-latest.err | 2 +- 4 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e460e48fce..0cc335eb30 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4159,7 +4159,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 f74c538efe..43418033f6 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -1048,8 +1048,12 @@ qemuValidateDomainDefPanic(const virDomainDef *def, } break; - /* default model value was changed before in post parse */ case VIR_DOMAIN_PANIC_MODEL_DEFAULT: + /* PostParse couldn't figure out a sensible default model */ + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("no panic model provided, and no default for the architecture and machine type")); + 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..139249bbc5 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 panic model provided, and no default for the architecture and machine type diff --git a/tests/qemuxmlconfdata/riscv64-panic-no-model.riscv64-latest.err b/tests/qemuxmlconfdata/riscv64-panic-no-model.riscv64-latest.err index 8e3f2c194d..139249bbc5 100644 --- a/tests/qemuxmlconfdata/riscv64-panic-no-model.riscv64-latest.err +++ b/tests/qemuxmlconfdata/riscv64-panic-no-model.riscv64-latest.err @@ -1 +1 @@ -unsupported configuration: the QEMU binary does not support the ISA panic device +unsupported configuration: no panic model provided, and no default for the architecture and machine type -- 2.46.0
pvpanic-pci is the only reasonable implementation of a panic device for aarch64/virt guests. Right now we're asking users to provide the model name manually, but we can be more helpful and fill it in automatically instead. With this change, the aarch64-panic-no-model test no longer fails and so it's no longer useful to us. Instead, we can amend the aarch64-virt-default-models test case to include panic coverage, something that until now wasn't possible. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 3 +++ .../aarch64-panic-no-model.aarch64-latest.err | 1 - tests/qemuxmlconfdata/aarch64-panic-no-model.xml | 13 ------------- ...rt-default-models.aarch64-latest.abi-update.args | 1 + ...irt-default-models.aarch64-latest.abi-update.xml | 3 +++ .../aarch64-virt-default-models.aarch64-latest.args | 1 + .../aarch64-virt-default-models.aarch64-latest.xml | 3 +++ .../qemuxmlconfdata/aarch64-virt-default-models.xml | 2 +- tests/qemuxmlconftest.c | 1 - 9 files changed, 12 insertions(+), 16 deletions(-) delete mode 100644 tests/qemuxmlconfdata/aarch64-panic-no-model.aarch64-latest.err delete mode 100644 tests/qemuxmlconfdata/aarch64-panic-no-model.xml diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0cc335eb30..4b3b5077e0 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4162,6 +4162,9 @@ qemuDomainDefaultPanicModel(const virDomainDef *def) if (ARCH_IS_X86(def->os.arch)) return VIR_DOMAIN_PANIC_MODEL_ISA; + if (qemuDomainIsARMVirt(def)) + return VIR_DOMAIN_PANIC_MODEL_PVPANIC; + return VIR_DOMAIN_PANIC_MODEL_DEFAULT; } diff --git a/tests/qemuxmlconfdata/aarch64-panic-no-model.aarch64-latest.err b/tests/qemuxmlconfdata/aarch64-panic-no-model.aarch64-latest.err deleted file mode 100644 index 139249bbc5..0000000000 --- a/tests/qemuxmlconfdata/aarch64-panic-no-model.aarch64-latest.err +++ /dev/null @@ -1 +0,0 @@ -unsupported configuration: no panic model provided, and no default for the architecture and machine type diff --git a/tests/qemuxmlconfdata/aarch64-panic-no-model.xml b/tests/qemuxmlconfdata/aarch64-panic-no-model.xml deleted file mode 100644 index 5207e48bbd..0000000000 --- a/tests/qemuxmlconfdata/aarch64-panic-no-model.xml +++ /dev/null @@ -1,13 +0,0 @@ -<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/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.abi-update.args b/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.abi-update.args index a503f45d0c..96fb251d80 100644 --- a/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.abi-update.args +++ b/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.abi-update.args @@ -44,4 +44,5 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-guest/.config \ -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 \ +-device '{"driver":"pvpanic-pci","bus":"pcie.0","addr":"0x2"}' \ -msg timestamp=on diff --git a/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.abi-update.xml b/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.abi-update.xml index bbe1dd931d..f27e7e1522 100644 --- a/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.abi-update.xml +++ b/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.abi-update.xml @@ -78,5 +78,8 @@ <address type='pci' domain='0x0000' bus='0x05' slot='0x00' function='0x0'/> </video> <memballoon model='none'/> + <panic model='pvpanic'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </panic> </devices> </domain> diff --git a/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.args b/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.args index a503f45d0c..96fb251d80 100644 --- a/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.args +++ b/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.args @@ -44,4 +44,5 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-guest/.config \ -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 \ +-device '{"driver":"pvpanic-pci","bus":"pcie.0","addr":"0x2"}' \ -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 index bbe1dd931d..f27e7e1522 100644 --- a/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.xml +++ b/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.xml @@ -78,5 +78,8 @@ <address type='pci' domain='0x0000' bus='0x05' slot='0x00' function='0x0'/> </video> <memballoon model='none'/> + <panic model='pvpanic'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </panic> </devices> </domain> diff --git a/tests/qemuxmlconfdata/aarch64-virt-default-models.xml b/tests/qemuxmlconfdata/aarch64-virt-default-models.xml index d9ad495e75..a8029d888d 100644 --- a/tests/qemuxmlconfdata/aarch64-virt-default-models.xml +++ b/tests/qemuxmlconfdata/aarch64-virt-default-models.xml @@ -19,6 +19,6 @@ </tpm> <video/> <memballoon model='none'/> - <!-- No default model for <panic/> on aarch64 --> + <panic/> </devices> </domain> diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c index e97d0e7bdc..5497fb2ba1 100644 --- a/tests/qemuxmlconftest.c +++ b/tests/qemuxmlconftest.c @@ -2648,7 +2648,6 @@ 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_ARCH_LATEST_PARSE_ERROR("riscv64-panic-no-model", "riscv64"); DO_TEST_CAPS_LATEST("pvpanic-pci-x86_64"); -- 2.46.0
On 8/27/24 17:08, Andrea Bolognani wrote:
Back when pvpanic-pci support was introduced 1.5 years ago (!), we required the user to manually provide the model name. This is incovenient and doesn't match the behavior seen on other architectures. Make things more user friendly.
Andrea Bolognani (4): tests: Add coverage for panic on riscv64 qemu: Refactor default panic model qemu: Sometimes the default panic model doesn't exist qemu: Use pvpanic by default on aarch64
src/qemu/qemu_domain.c | 36 ++++++++++++------- src/qemu/qemu_validate.c | 6 +++- .../aarch64-panic-no-model.aarch64-latest.err | 1 - ...ault-models.aarch64-latest.abi-update.args | 1 + ...fault-models.aarch64-latest.abi-update.xml | 3 ++ ...64-virt-default-models.aarch64-latest.args | 1 + ...h64-virt-default-models.aarch64-latest.xml | 3 ++ .../aarch64-virt-default-models.xml | 2 +- .../riscv64-panic-no-model.riscv64-latest.err | 1 + ...o-model.xml => riscv64-panic-no-model.xml} | 4 +-- tests/qemuxmlconftest.c | 2 +- 11 files changed, 42 insertions(+), 18 deletions(-) delete mode 100644 tests/qemuxmlconfdata/aarch64-panic-no-model.aarch64-latest.err create mode 100644 tests/qemuxmlconfdata/riscv64-panic-no-model.riscv64-latest.err rename tests/qemuxmlconfdata/{aarch64-panic-no-model.xml => riscv64-panic-no-model.xml} (65%)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Andrea Bolognani -
Michal Prívozník