[v1] implement 'ras' feature support

[PATCH 3/4] qemu: validate machine virt ras feature

Posted by Kristina Hanicova 1 year, 9 months ago

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
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

Re: [PATCH 3/4] qemu: validate machine virt ras feature

Posted by Michal Prívozník 1 year, 9 months ago

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
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

Re: [PATCH 3/4] qemu: validate machine virt ras feature

Posted by Ján Tomko 1 year, 9 months ago

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
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

Re: [PATCH 3/4] qemu: validate machine virt ras feature

Posted by Andrea Bolognani 1 year, 9 months ago

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
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org