10:08 a.m.
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
10:08 a.m.
New subject: [PATCH 1/4] tests: Add coverage for panic on riscv64
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(a)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
10:08 a.m.
New subject: [PATCH 2/4] qemu: Refactor default panic model
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(a)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
10:08 a.m.
New subject: [PATCH 3/4] qemu: Sometimes the default panic model doesn't exist
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(a)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
10:08 a.m.
New subject: [PATCH 4/4] qemu: Use pvpanic by default on aarch64
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(a)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
5:13 a.m.
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(a)redhat.com>
Michal
120
days inactive
121
days old
5 comments
2 participants
participants (2)
-
Andrea Bolognani -
Michal Prívozník