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

[Alejandro Vallejo <alejandro.garciavallejo@amd.com>]

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