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
