[PATCH 0/4] implement 'ras' feature support
*** BLURB HERE *** Kristina Hanicova (4): Introduce QEMU_CAPS_MACHINE_VIRT_RAS capability conf: parse and format machine virt ras feature qemu: validate machine virt ras feature qemu: format machine virt ras feature and test it docs/formatdomain.rst | 5 +++ src/conf/domain_conf.c | 6 +++- src/conf/domain_conf.h | 1 + src/conf/schemas/domaincommon.rng | 5 +++ src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 5 +++ src/qemu/qemu_validate.c | 16 ++++++++++ .../caps_5.2.0_aarch64.xml | 1 + .../caps_6.0.0_aarch64.xml | 1 + .../caps_6.2.0_aarch64.xml | 1 + .../caps_7.0.0_aarch64+hvf.xml | 1 + .../caps_7.0.0_aarch64.xml | 1 + .../caps_8.2.0_aarch64.xml | 1 + .../caps_8.2.0_armv7l.xml | 1 + .../aarch64-features-ras.aarch64-latest.args | 31 +++++++++++++++++++ .../aarch64-features-ras.aarch64-latest.xml | 1 + .../qemuxmlconfdata/aarch64-features-ras.xml | 26 ++++++++++++++++ tests/qemuxmlconftest.c | 2 ++ 19 files changed, 107 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxmlconfdata/aarch64-features-ras.aarch64-latest.args create mode 120000 tests/qemuxmlconfdata/aarch64-features-ras.aarch64-latest.xml create mode 100644 tests/qemuxmlconfdata/aarch64-features-ras.xml -- 2.42.0
The capability can be used to detect if the qemu binary already supports 'ras' feature for 'virt' machine type. Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_5.2.0_aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_6.0.0_aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_6.2.0_aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_7.0.0_aarch64+hvf.xml | 1 + tests/qemucapabilitiesdata/caps_7.0.0_aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_8.2.0_aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_8.2.0_armv7l.xml | 1 + 9 files changed, 10 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 21f93c6774..9da7faf27d 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -706,6 +706,7 @@ VIR_ENUM_IMPL(virQEMUCaps, "blockjob.backing-mask-protocol", /* QEMU_CAPS_BLOCKJOB_BACKING_MASK_PROTOCOL */ "display-reload", /* QEMU_CAPS_DISPLAY_RELOAD */ "usb-mtp", /* QEMU_CAPS_DEVICE_USB_MTP */ + "machine.virt.ras", /* QEMU_CAPS_MACHINE_VIRT_RAS */ ); @@ -1733,6 +1734,7 @@ static struct virQEMUCapsStringFlags virQEMUCapsMachinePropsPSeries[] = { static struct virQEMUCapsStringFlags virQEMUCapsMachinePropsVirt[] = { { "iommu", QEMU_CAPS_MACHINE_VIRT_IOMMU }, + { "ras", QEMU_CAPS_MACHINE_VIRT_RAS }, }; static struct virQEMUCapsStringFlags virQEMUCapsMachinePropsGeneric[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 5082967cba..618584a1ed 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -685,6 +685,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_BLOCKJOB_BACKING_MASK_PROTOCOL, /* backing-mask-protocol of block-commit/block-stream */ QEMU_CAPS_DISPLAY_RELOAD, /* 'display-reload' qmp command is supported */ QEMU_CAPS_DEVICE_USB_MTP, /* -device usb-mtp */ + QEMU_CAPS_MACHINE_VIRT_RAS, /* -machine virt,ras= */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_5.2.0_aarch64.xml b/tests/qemucapabilitiesdata/caps_5.2.0_aarch64.xml index ddd30feaff..905726f7c1 100644 --- a/tests/qemucapabilitiesdata/caps_5.2.0_aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_5.2.0_aarch64.xml @@ -125,6 +125,7 @@ <flag name='usb-host.guest-resets-all'/> <flag name='virtio-crypto'/> <flag name='usb-mtp'/> + <flag name='machine.virt.ras'/> <version>5002000</version> <microcodeVersion>61700243</microcodeVersion> <package>v5.2.0</package> diff --git a/tests/qemucapabilitiesdata/caps_6.0.0_aarch64.xml b/tests/qemucapabilitiesdata/caps_6.0.0_aarch64.xml index 7cf7c0707b..97bbcdeb68 100644 --- a/tests/qemucapabilitiesdata/caps_6.0.0_aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_6.0.0_aarch64.xml @@ -136,6 +136,7 @@ <flag name='pvpanic-pci'/> <flag name='display-reload'/> <flag name='usb-mtp'/> + <flag name='machine.virt.ras'/> <version>6000000</version> <microcodeVersion>61700242</microcodeVersion> <package>v6.0.0</package> diff --git a/tests/qemucapabilitiesdata/caps_6.2.0_aarch64.xml b/tests/qemucapabilitiesdata/caps_6.2.0_aarch64.xml index 13253f13c0..a7a6f54019 100644 --- a/tests/qemucapabilitiesdata/caps_6.2.0_aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_6.2.0_aarch64.xml @@ -149,6 +149,7 @@ <flag name='virtio-gpu.blob'/> <flag name='display-reload'/> <flag name='usb-mtp'/> + <flag name='machine.virt.ras'/> <version>6001050</version> <microcodeVersion>61700244</microcodeVersion> <package></package> diff --git a/tests/qemucapabilitiesdata/caps_7.0.0_aarch64+hvf.xml b/tests/qemucapabilitiesdata/caps_7.0.0_aarch64+hvf.xml index b359f22b03..ab21278e50 100644 --- a/tests/qemucapabilitiesdata/caps_7.0.0_aarch64+hvf.xml +++ b/tests/qemucapabilitiesdata/caps_7.0.0_aarch64+hvf.xml @@ -157,6 +157,7 @@ <flag name='virtio-gpu.blob'/> <flag name='display-reload'/> <flag name='usb-mtp'/> + <flag name='machine.virt.ras'/> <version>6002092</version> <microcodeVersion>61700243</microcodeVersion> <package>v7.0.0-rc2</package> diff --git a/tests/qemucapabilitiesdata/caps_7.0.0_aarch64.xml b/tests/qemucapabilitiesdata/caps_7.0.0_aarch64.xml index c6f31b94fd..f53c3ddcd8 100644 --- a/tests/qemucapabilitiesdata/caps_7.0.0_aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_7.0.0_aarch64.xml @@ -157,6 +157,7 @@ <flag name='virtio-gpu.blob'/> <flag name='display-reload'/> <flag name='usb-mtp'/> + <flag name='machine.virt.ras'/> <version>6002092</version> <microcodeVersion>61700243</microcodeVersion> <package>v7.0.0-rc2</package> diff --git a/tests/qemucapabilitiesdata/caps_8.2.0_aarch64.xml b/tests/qemucapabilitiesdata/caps_8.2.0_aarch64.xml index fe4c65c9b7..c9d99f56cb 100644 --- a/tests/qemucapabilitiesdata/caps_8.2.0_aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_8.2.0_aarch64.xml @@ -165,6 +165,7 @@ <flag name='virtio-mem-pci.dynamic-memslots'/> <flag name='display-reload'/> <flag name='usb-mtp'/> + <flag name='machine.virt.ras'/> <version>8002000</version> <microcodeVersion>61700246</microcodeVersion> <package>v8.2.0</package> diff --git a/tests/qemucapabilitiesdata/caps_8.2.0_armv7l.xml b/tests/qemucapabilitiesdata/caps_8.2.0_armv7l.xml index e26dc43963..b5a1d426dc 100644 --- a/tests/qemucapabilitiesdata/caps_8.2.0_armv7l.xml +++ b/tests/qemucapabilitiesdata/caps_8.2.0_armv7l.xml @@ -172,6 +172,7 @@ <flag name='virtio-mem-pci.dynamic-memslots'/> <flag name='display-reload'/> <flag name='usb-mtp'/> + <flag name='machine.virt.ras'/> <version>8002000</version> <microcodeVersion>0</microcodeVersion> <package>qemu-8.2.0-7.fc39</package> -- 2.42.0
On Wed, Apr 24, 2024 at 04:59:38PM GMT, Kristina Hanicova wrote:
Introduce QEMU_CAPS_MACHINE_VIRT_RAS capability
Nit: missing 'qemu: ' prefix.
The capability can be used to detect if the qemu binary already supports 'ras' feature for 'virt' machine type.
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_5.2.0_aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_6.0.0_aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_6.2.0_aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_7.0.0_aarch64+hvf.xml | 1 + tests/qemucapabilitiesdata/caps_7.0.0_aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_8.2.0_aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_8.2.0_armv7l.xml | 1 + 9 files changed, 10 insertions(+)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- docs/formatdomain.rst | 5 +++++ src/conf/domain_conf.c | 6 +++++- src/conf/domain_conf.h | 1 + src/conf/schemas/domaincommon.rng | 5 +++++ src/qemu/qemu_validate.c | 1 + 5 files changed, 17 insertions(+), 1 deletion(-) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 0211abaa1a..3a8aafcc19 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -2021,6 +2021,7 @@ Hypervisors may allow certain CPU / machine features to be toggled on/off. <tb-cache unit='MiB'>128</tb-cache> </tcg> <async-teardown enabled='yes'/> + <ras state='on'/> </features> ... @@ -2256,6 +2257,10 @@ are: Depending on the ``enabled`` attribute (values ``yes``, ``no``) enable or disable QEMU asynchronous teardown to improve memory reclaiming on a guest. :since:`Since 9.6.0` (QEMU only) +``ras`` + Report host memory errors to a guest using ACPI and guest external abort + exceptions when enabled (``on``). Disabled (``off``) by default. + :since:`Since 10.3.0` (QEMU/KVM and ARM virt guests only) Time keeping ------------ diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9df2b76efb..9eaec76b07 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -184,6 +184,7 @@ VIR_ENUM_IMPL(virDomainFeature, "ibs", "tcg", "async-teardown", + "ras", ); VIR_ENUM_IMPL(virDomainCapabilitiesPolicy, @@ -16842,7 +16843,8 @@ virDomainFeaturesDefParse(virDomainDef *def, case VIR_DOMAIN_FEATURE_HTM: case VIR_DOMAIN_FEATURE_NESTED_HV: - case VIR_DOMAIN_FEATURE_CCF_ASSIST: { + case VIR_DOMAIN_FEATURE_CCF_ASSIST: + case VIR_DOMAIN_FEATURE_RAS: { virTristateSwitch state; if (virXMLPropTristateSwitch(nodes[i], "state", @@ -20689,6 +20691,7 @@ virDomainDefFeaturesCheckABIStability(virDomainDef *src, case VIR_DOMAIN_FEATURE_HTM: case VIR_DOMAIN_FEATURE_NESTED_HV: case VIR_DOMAIN_FEATURE_CCF_ASSIST: + case VIR_DOMAIN_FEATURE_RAS: if (src->features[i] != dst->features[i]) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("State of feature '%1$s' differs: source: '%2$s', destination: '%3$s'"), @@ -27392,6 +27395,7 @@ virDomainDefFormatFeatures(virBuffer *buf, case VIR_DOMAIN_FEATURE_HTM: case VIR_DOMAIN_FEATURE_NESTED_HV: case VIR_DOMAIN_FEATURE_CCF_ASSIST: + case VIR_DOMAIN_FEATURE_RAS: switch ((virTristateSwitch) def->features[i]) { case VIR_TRISTATE_SWITCH_LAST: case VIR_TRISTATE_SWITCH_ABSENT: diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 356c25405b..c2abdb9f52 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2171,6 +2171,7 @@ typedef enum { VIR_DOMAIN_FEATURE_IBS, VIR_DOMAIN_FEATURE_TCG, VIR_DOMAIN_FEATURE_ASYNC_TEARDOWN, + VIR_DOMAIN_FEATURE_RAS, VIR_DOMAIN_FEATURE_LAST } virDomainFeature; diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng index d84e030255..86d9e391d8 100644 --- a/src/conf/schemas/domaincommon.rng +++ b/src/conf/schemas/domaincommon.rng @@ -6795,6 +6795,11 @@ </optional> </element> </optional> + <optional> + <element name="ras"> + <ref name="featurestate"/> + </element> + </optional> </interleave> </element> </optional> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index b02da95c1e..b33618b494 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -226,6 +226,7 @@ qemuValidateDomainDefFeatures(const virDomainDef *def, } break; + case VIR_DOMAIN_FEATURE_RAS: case VIR_DOMAIN_FEATURE_SMM: case VIR_DOMAIN_FEATURE_KVM: case VIR_DOMAIN_FEATURE_XEN: -- 2.42.0
On Wed, Apr 24, 2024 at 04:59:39PM GMT, Kristina Hanicova wrote:
+++ b/docs/formatdomain.rst @@ -2256,6 +2257,10 @@ are: +``ras`` + Report host memory errors to a guest using ACPI and guest external abort + exceptions when enabled (``on``). Disabled (``off``) by default.
This is the current status, but there is no guarantee that a future versioned machine type will not enable the feature by default. I would just say If the attribute is not defined, the hypervisor default will be used. as is the case for several existing features.
+ :since:`Since 10.3.0` (QEMU/KVM and ARM virt guests only)
Of course this will need updating since 10.3.0 has already entered freeze. -- Andrea Bolognani / Red Hat / Virtualization
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/qemu/qemu_validate.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index b33618b494..c8bee6f23d 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -69,6 +69,7 @@ qemuValidateDomainDefPSeriesFeature(const virDomainDef *def, case VIR_DOMAIN_FEATURE_HTM: case VIR_DOMAIN_FEATURE_NESTED_HV: case VIR_DOMAIN_FEATURE_CCF_ASSIST: + case VIR_DOMAIN_FEATURE_RAS: if (!virTristateSwitchTypeToString(def->features[feature])) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Invalid setting for pseries feature '%1$s'"), @@ -227,6 +228,20 @@ qemuValidateDomainDefFeatures(const virDomainDef *def, break; case VIR_DOMAIN_FEATURE_RAS: + if (def->features[i] == VIR_TRISTATE_SWITCH_ON && + !qemuDomainIsARMVirt(def)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("ras feature is only supported with ARM Virt machines")); + return -1; + } + if (def->features[i] == VIR_TRISTATE_SWITCH_ON && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_VIRT_RAS)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("ras feature is not available with this QEMU binary")); + return -1; + } + break; + case VIR_DOMAIN_FEATURE_SMM: case VIR_DOMAIN_FEATURE_KVM: case VIR_DOMAIN_FEATURE_XEN: -- 2.42.0
On 4/24/24 16:59, Kristina Hanicova wrote:
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/qemu/qemu_validate.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index b33618b494..c8bee6f23d 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -69,6 +69,7 @@ qemuValidateDomainDefPSeriesFeature(const virDomainDef *def, case VIR_DOMAIN_FEATURE_HTM: case VIR_DOMAIN_FEATURE_NESTED_HV: case VIR_DOMAIN_FEATURE_CCF_ASSIST: + case VIR_DOMAIN_FEATURE_RAS: if (!virTristateSwitchTypeToString(def->features[feature])) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Invalid setting for pseries feature '%1$s'"), @@ -227,6 +228,20 @@ qemuValidateDomainDefFeatures(const virDomainDef *def, break;
case VIR_DOMAIN_FEATURE_RAS: + if (def->features[i] == VIR_TRISTATE_SWITCH_ON && + !qemuDomainIsARMVirt(def)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("ras feature is only supported with ARM Virt machines")); + return -1; + } + if (def->features[i] == VIR_TRISTATE_SWITCH_ON && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_VIRT_RAS)) {
Just realized, this checks for the capability only when: <features> <ras state='on'/> </features> But the next patch adds 'ras=' onto cmd line even for the following case: <ras state='off'/> But unfortunately, I don't think we have a good way out. How I see our options: 1) check for != VIR_TRISTATE_SWITCH_ABSENT here, just like we're doing in the next patch. Problem is, when user specifies <ras state='off'/> and we're talking to an older QEMU (say 4.2.0), which doesn't support (toggling) the feature, well then users are would be unable to start such guest. Even though the feature might already be off by default. 2) change check in the next patch to == VIR_TRISTATE_SWITCH_ON, just like we're doing here. This means, we're effectively able to just format ras=on onto the cmd line. Well, what if the default changes in the future (say QEMU-10.0.0) and users want to turn it off? 3) do nothing and keep both patches as they are. Looking around other features (VIR_DOMAIN_FEATURE_CCF_ASSIST, VIR_DOMAIN_FEATURE_NESTED_HV, ...) - they suffer the same. Let's stick with the option 3) then. Michal
On a Thursday in 2024, Michal Prívozník wrote:
On 4/24/24 16:59, Kristina Hanicova wrote:
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/qemu/qemu_validate.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index b33618b494..c8bee6f23d 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -69,6 +69,7 @@ qemuValidateDomainDefPSeriesFeature(const virDomainDef *def, case VIR_DOMAIN_FEATURE_HTM: case VIR_DOMAIN_FEATURE_NESTED_HV: case VIR_DOMAIN_FEATURE_CCF_ASSIST: + case VIR_DOMAIN_FEATURE_RAS: if (!virTristateSwitchTypeToString(def->features[feature])) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Invalid setting for pseries feature '%1$s'"), @@ -227,6 +228,20 @@ qemuValidateDomainDefFeatures(const virDomainDef *def, break;
case VIR_DOMAIN_FEATURE_RAS: + if (def->features[i] == VIR_TRISTATE_SWITCH_ON && + !qemuDomainIsARMVirt(def)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("ras feature is only supported with ARM Virt machines")); + return -1; + } + if (def->features[i] == VIR_TRISTATE_SWITCH_ON && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_VIRT_RAS)) {
Just realized, this checks for the capability only when:
<features> <ras state='on'/> </features>
But the next patch adds 'ras=' onto cmd line even for the following case:
<ras state='off'/>
But unfortunately, I don't think we have a good way out. How I see our options:
1) check for != VIR_TRISTATE_SWITCH_ABSENT here, just like we're doing in the next patch. Problem is, when user specifies <ras state='off'/> and we're talking to an older QEMU (say 4.2.0), which doesn't support (toggling) the feature, well then users are would be unable to start such guest. Even though the feature might already be off by default.
I prefer this one. Don't see any point in toggling a feature that: 1) was not even present in the QEMU they're using 2) is currently off by default and possibly will be for some time.
2) change check in the next patch to == VIR_TRISTATE_SWITCH_ON, just like we're doing here. This means, we're effectively able to just format ras=on onto the cmd line. Well, what if the default changes in the future (say QEMU-10.0.0) and users want to turn it off?
3) do nothing and keep both patches as they are.
Worst of the three - if we're letting QEMU report the error, why bother with the validation? Jano
Looking around other features (VIR_DOMAIN_FEATURE_CCF_ASSIST, VIR_DOMAIN_FEATURE_NESTED_HV, ...) - they suffer the same.
Let's stick with the option 3) then.
Michal _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
On Thu, Apr 25, 2024 at 02:09:47PM GMT, Ján Tomko wrote:
On a Thursday in 2024, Michal Prívozník wrote:
On 4/24/24 16:59, Kristina Hanicova wrote:
case VIR_DOMAIN_FEATURE_RAS: + if (def->features[i] == VIR_TRISTATE_SWITCH_ON && + !qemuDomainIsARMVirt(def)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("ras feature is only supported with ARM Virt machines")); + return -1; + } + if (def->features[i] == VIR_TRISTATE_SWITCH_ON && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_VIRT_RAS)) {
Just realized, this checks for the capability only when:
<features> <ras state='on'/> </features>
But the next patch adds 'ras=' onto cmd line even for the following case:
<ras state='off'/>
But unfortunately, I don't think we have a good way out. How I see our options:
1) check for != VIR_TRISTATE_SWITCH_ABSENT here, just like we're doing in the next patch. Problem is, when user specifies <ras state='off'/> and we're talking to an older QEMU (say 4.2.0), which doesn't support (toggling) the feature, well then users are would be unable to start such guest. Even though the feature might already be off by default.
I prefer this one. Don't see any point in toggling a feature that: 1) was not even present in the QEMU they're using 2) is currently off by default and possibly will be for some time.
I agree with Jano. If you want the ability to explicitly control the status of RAS support, then use a version of QEMU that implements the necessary toggle. -- Andrea Bolognani / Red Hat / Virtualization
Resolves: https://issues.redhat.com/browse/RHEL-7489 Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/qemu/qemu_command.c | 5 +++ .../aarch64-features-ras.aarch64-latest.args | 31 +++++++++++++++++++ .../aarch64-features-ras.aarch64-latest.xml | 1 + .../qemuxmlconfdata/aarch64-features-ras.xml | 26 ++++++++++++++++ tests/qemuxmlconftest.c | 2 ++ 5 files changed, 65 insertions(+) create mode 100644 tests/qemuxmlconfdata/aarch64-features-ras.aarch64-latest.args create mode 120000 tests/qemuxmlconfdata/aarch64-features-ras.aarch64-latest.xml create mode 100644 tests/qemuxmlconfdata/aarch64-features-ras.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8d4442c360..807f013713 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6858,6 +6858,11 @@ qemuAppendDomainFeaturesMachineParam(virBuffer *buf, virBufferAsprintf(buf, ",cap-ibs=%s", str); } + if (def->features[VIR_DOMAIN_FEATURE_RAS] != VIR_TRISTATE_SWITCH_ABSENT) { + const char *str = virTristateSwitchTypeToString(def->features[VIR_DOMAIN_FEATURE_RAS]); + virBufferAsprintf(buf, ",ras=%s", str); + } + return 0; } diff --git a/tests/qemuxmlconfdata/aarch64-features-ras.aarch64-latest.args b/tests/qemuxmlconfdata/aarch64-features-ras.aarch64-latest.args new file mode 100644 index 0000000000..f903d7152f --- /dev/null +++ b/tests/qemuxmlconfdata/aarch64-features-ras.aarch64-latest.args @@ -0,0 +1,31 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/var/lib/libvirt/qemu/domain--1-aarch64test \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/var/lib/libvirt/qemu/domain--1-aarch64test/.local/share \ +XDG_CACHE_HOME=/var/lib/libvirt/qemu/domain--1-aarch64test/.cache \ +XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-aarch64test/.config \ +/usr/bin/qemu-system-aarch64 \ +-name guest=aarch64test,debug-threads=on \ +-S \ +-object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/var/lib/libvirt/qemu/domain--1-aarch64test/master-key.aes"}' \ +-machine virt,usb=off,gic-version=3,ras=on,dump-guest-core=off,memory-backend=mach-virt.ram,acpi=off \ +-accel kvm \ +-cpu host \ +-m size=1048576k \ +-object '{"qom-type":"memory-backend-ram","id":"mach-virt.ram","size":1073741824}' \ +-overcommit mem-lock=off \ +-smp 1,sockets=1,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-features-ras.aarch64-latest.xml b/tests/qemuxmlconfdata/aarch64-features-ras.aarch64-latest.xml new file mode 120000 index 0000000000..4abae8df8c --- /dev/null +++ b/tests/qemuxmlconfdata/aarch64-features-ras.aarch64-latest.xml @@ -0,0 +1 @@ +aarch64-features-ras.xml \ No newline at end of file diff --git a/tests/qemuxmlconfdata/aarch64-features-ras.xml b/tests/qemuxmlconfdata/aarch64-features-ras.xml new file mode 100644 index 0000000000..bbe948dc97 --- /dev/null +++ b/tests/qemuxmlconfdata/aarch64-features-ras.xml @@ -0,0 +1,26 @@ +<domain type='kvm'> + <name>aarch64test</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='aarch64' machine='virt'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <gic version='3'/> + <ras state='on'/> + </features> + <cpu mode='host-passthrough' check='none'/> + <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='none'/> + <controller type='pci' index='0' model='pcie-root'/> + <audio id='1' type='none'/> + </devices> +</domain> diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c index 6165bb6f1d..619686b9a7 100644 --- a/tests/qemuxmlconftest.c +++ b/tests/qemuxmlconftest.c @@ -2643,6 +2643,8 @@ mymain(void) /* SVE aarch64 CPU features work on modern QEMU */ DO_TEST_CAPS_ARCH_LATEST("aarch64-features-sve", "aarch64"); + DO_TEST_CAPS_ARCH_LATEST("aarch64-features-ras", "aarch64"); + DO_TEST_CAPS_ARCH_LATEST("clock-timer-armvtimer", "aarch64"); qemuTestSetHostArch(&driver, VIR_ARCH_NONE); -- 2.42.0
On 4/24/24 16:59, Kristina Hanicova wrote:
*** BLURB HERE ***
Kristina Hanicova (4): Introduce QEMU_CAPS_MACHINE_VIRT_RAS capability conf: parse and format machine virt ras feature qemu: validate machine virt ras feature qemu: format machine virt ras feature and test it
docs/formatdomain.rst | 5 +++ src/conf/domain_conf.c | 6 +++- src/conf/domain_conf.h | 1 + src/conf/schemas/domaincommon.rng | 5 +++ src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 5 +++ src/qemu/qemu_validate.c | 16 ++++++++++ .../caps_5.2.0_aarch64.xml | 1 + .../caps_6.0.0_aarch64.xml | 1 + .../caps_6.2.0_aarch64.xml | 1 + .../caps_7.0.0_aarch64+hvf.xml | 1 + .../caps_7.0.0_aarch64.xml | 1 + .../caps_8.2.0_aarch64.xml | 1 + .../caps_8.2.0_armv7l.xml | 1 + .../aarch64-features-ras.aarch64-latest.args | 31 +++++++++++++++++++ .../aarch64-features-ras.aarch64-latest.xml | 1 + .../qemuxmlconfdata/aarch64-features-ras.xml | 26 ++++++++++++++++ tests/qemuxmlconftest.c | 2 ++ 19 files changed, 107 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxmlconfdata/aarch64-features-ras.aarch64-latest.args create mode 120000 tests/qemuxmlconfdata/aarch64-features-ras.aarch64-latest.xml create mode 100644 tests/qemuxmlconfdata/aarch64-features-ras.xml
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> But I'll postpone pushing these for a bit to give others a chance to express their position on the problem I'm mentioning in 3/4. Michal
participants (4)
-
Andrea Bolognani -
Ján Tomko -
Kristina Hanicova -
Michal Prívozník