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&gt; > --- > 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 _______________________________________________ Devel mailing list -- devel(a)lists.libvirt.org To unsubscribe send an email to devel-leave(a)lists.libvirt.org