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

[Jan Beulich <jbeulich@xxxxxxxx>]

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