Re: [XEN][PATCH v7] x86: make Viridian support optional

[Grygorii Strashko <grygorii_strashko@xxxxxxxx>]

On 12.11.25 16:27, Alejandro Vallejo wrote:
> On Wed Nov 12, 2025 at 7:40 AM CET, Jan Beulich wrote:
> > On 11.11.2025 19:25, Grygorii Strashko wrote:
> > > On 06.11.25 15:47, Jan Beulich wrote:
> > > > On 06.11.2025 14:42, Grygorii Strashko wrote:
> > > > > On 06.11.25 13:35, Jan Beulich wrote:
> > > > > > On 31.10.2025 17:17, Grygorii Strashko wrote:
> > > > > > > --- a/xen/arch/x86/hvm/hvm.c
> > > > > > > +++ b/xen/arch/x86/hvm/hvm.c
> > > > > > > @@ -4231,8 +4231,9 @@ static int hvm_set_param(struct domain *d, uint32_t 
> > > > > > > index, uint64_t value)
> > > > > > >                 rc = -EINVAL;
> > > > > > >             break;
> > > > > > >         case HVM_PARAM_VIRIDIAN:
> > > > > > > -        if ( (value & ~HVMPV_feature_mask) ||
> > > > > > > -             !(value & HVMPV_base_freq) )
> > > > > > > +        if ( !IS_ENABLED(CONFIG_VIRIDIAN) && value )
> > > > > > > +            rc = -ENODEV;
> > > > > > > +        else if ( (value & ~HVMPV_feature_mask) || !(value & HVMPV_base_freq) )
> > > > > > >                 rc = -EINVAL;
> > > > > > I find the check for value to be (non-)zero a little dubious here: If any caller
> > > > > > passed in 0, it would get back -EINVAL anyway. Imo -ENODEV would be more 
> > > > > > suitable
> > > > > > in that case as well. Things would be different if 0 was a valid value to pass 
> > > > > > in.
> > > > > The idea was to distinguish between "Feature enabled, Invalid parameter" and 
> > > > > "Feature disabled".
> > > > "
> > > > But you don't, or else the addition would need to live after the -EINVAL checks.
> > > > I also question the utility of knowing "parameter was invalid" when the feature
> > > > isn't available anyway.
> > > My understanding here - I need to drop non-zero "value" check.
> > > will be:
> > >
> > >      case HVM_PARAM_VIRIDIAN:
> > >          if ( !IS_ENABLED(CONFIG_VIRIDIAN) )
> > >              rc = -ENODEV;
> > >          else if ( (value & ~HVMPV_feature_mask) || !(value & HVMPV_base_freq) )
> > >              rc = -EINVAL;
> > >          break;
> > Yes, or alternatively
> >
> >      case HVM_PARAM_VIRIDIAN:
> >          if ( (value & ~HVMPV_feature_mask) || !(value & HVMPV_base_freq) )
> >              rc = -EINVAL;
> >          else if ( !IS_ENABLED(CONFIG_VIRIDIAN) )
> >              rc = -ENODEV;
> >          break;
> >
> > Both have their up- and down-sides.
> >
> > Jan
> We should aim for Grygorii's form, imo. If anything because it eliminates
> the second else-if on !VIRIDIAN, leading to a marginal binary size reduction.
>
> There's no value in validation when viridian support just isn't there.
I'm planning to resend with below diff applied:

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index c09fb2ba6873..7299cfa90ad5 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4231,7 +4231,7 @@ static int hvm_set_param(struct domain *d, uint32_t 
index, uint64_t value)
             rc = -EINVAL;
         break;
     case HVM_PARAM_VIRIDIAN:
-        if ( !IS_ENABLED(CONFIG_VIRIDIAN) && value )
+        if ( !IS_ENABLED(CONFIG_VIRIDIAN) )
             rc = -ENODEV;
         else if ( (value & ~HVMPV_feature_mask) || !(value & HVMPV_base_freq) )
             rc = -EINVAL;
diff --git a/xen/arch/x86/hvm/viridian/viridian.c 
b/xen/arch/x86/hvm/viridian/viridian.c
index 5fb148b2aa15..90e749ceb581 100644
--- a/xen/arch/x86/hvm/viridian/viridian.c
+++ b/xen/arch/x86/hvm/viridian/viridian.c
@@ -1117,9 +1117,6 @@ static int cf_check viridian_load_domain_ctxt(
     struct viridian_domain *vd = d->arch.hvm.viridian;
     struct hvm_viridian_domain_context ctxt;
 
-    if ( !IS_ENABLED(CONFIG_VIRIDIAN) )
-        return -EILSEQ;
-
     if ( hvm_load_entry_zeroextend(VIRIDIAN_DOMAIN, h, &ctxt) != 0 )
         return -EINVAL;
 
@@ -1156,9 +1153,6 @@ static int cf_check viridian_load_vcpu_ctxt(
     struct vcpu *v;
     struct hvm_viridian_vcpu_context ctxt;
 
-    if ( !IS_ENABLED(CONFIG_VIRIDIAN) )
-        return -EILSEQ;
-
     if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
     {
         dprintk(XENLOG_G_ERR, "HVM restore: dom%d has no vcpu%u\n",


PS: Not sure I have enough stamina and enthusiasm to continue post v8 version.

--
Best regards,
-grygorii