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
