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. Cheers, Alejandro
