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

[Jan Beulich <jbeulich@xxxxxxxx>]

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/Makefile
>>> +++ b/xen/arch/x86/hvm/Makefile
>>> @@ -1,6 +1,6 @@
>>>   obj-$(CONFIG_AMD_SVM) += svm/
>>>   obj-$(CONFIG_INTEL_VMX) += vmx/
>>> -obj-y += viridian/
>>> +obj-$(CONFIG_VIRIDIAN) += viridian/
>>
>> With this, what is the point of the additions to 
>> viridian_load_{domain,vcpu}_ctxt()?
>> You're adding dead code there, aren't you?
> 
> Hgrr. And we end up back to v3 regarding this part.
> Check in viridian_load_{domain,vcpu}_ctxt() may/may not work depending on 
> toolstack
> behavior which is not strictly defined (loading HVM_PARAM_VIRIDIAN 
> before/after
> viridian_load_{domain,vcpu}_ctxt()).
> 
> So what's the conclusion here - drop this check?

Well, Misra wants us to not have any dead code. Hence why would we add new
instances of such?

>>> --- 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.

Jan