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@xxxxxxxxxx> --- 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@xxxxxxxxxxxxxxxxx To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Devel mailing list -- devel@xxxxxxxxxxxxxxxxx To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
