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
