Re: [PATCH 3/4] qemu: validate machine virt ras feature
On 4/24/24 16:59, Kristina Hanicova wrote:
Signed-off-by: Kristina Hanicova <khanicov(a)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