Re: [PATCH v1] x86: make Viridian support optional

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.03.2025 16:18, Alejandro Vallejo wrote:
> On Thu Mar 20, 2025 at 9:39 AM GMT, Sergiy Kibrik wrote:
>> hi Alejandro,
>>
>> 17.03.25 11:19, Alejandro Vallejo:
>> [..]
>>>       endif
>>>
>>>     +config HVM_VIRIDIAN
>>>     +       bool "Viridian enlightenments support" if EXPERT
>>>     +       depends on HVM
>>>     +       default y
>>>     +
>>>
>>>
>>>
>>> I don't see why this should be gated by EXPERT, provided a
>>> suitable (now absent) help message was to exist explaining
>>> what it does in plain simple words.
>>
>> The option is intended primarily for fine-tuned systems optimized for 
>> particular platform and mode of operation. As for generic systems (e.g. 
>> distributions) whey wouldn't want to disable it anyway.
> 
> 
> 
>>>
>>> For the title, I'd say it needs to properly state it refers to
>>> enlightenments for guests, rather than enlightenments for
>>> Xen itself when running under Hyper-V. As it is, it sounds
>>> ambiguous (Maybe "Hyper-V enlighnments for guests"?).
>>>
>>
>> Agree, "Hyper-V enlighnments for guests" is better title.
>> As for help message, would the one below be sufficient?:
>>
>>   help
>>     Support optimizations for Hyper-V guests such as faster hypercalls,
>>     efficient timer and interrupt handling, and enhanced paravirtualized
>>     I/O. This is to improve performance and compatibility of Windows VMs.
>>
>>     If unsure, say Y.
> 
> Sounds good enough to me.
> 
>>
>>
>>> On a personal nitpicky preference note, I'd say HVM_VIRIDIAN sounds
>>> rather redundant, and I think just VIRIDIAN works just as well
>>> while being shorter.
>>>
>>
>> this is rather to highlight the fact that the code is part of HVM 
>> support, a bit of self-documenting
>>
>> [..]
> 
> That's fair enough.
> 
>>>     diff --git a/xen/arch/x86/include/asm/hvm/vcpu.h
>>>     b/xen/arch/x86/include/asm/hvm/vcpu.h
>>>     index 196fed6d5d..bac35ec47a 100644
>>>     --- a/xen/arch/x86/include/asm/hvm/vcpu.h
>>>     +++ b/xen/arch/x86/include/asm/hvm/vcpu.h
>>>     @@ -171,8 +171,9 @@ struct hvm_vcpu {
>>>
>>>           /* Pending hw/sw interrupt (.vector = -1 means nothing
>>>     pending). */
>>>           struct x86_event     inject_event;
>>>     -
>>>     +#ifdef CONFIG_HVM_VIRIDIAN
>>>           struct viridian_vcpu *viridian;
>>>     +#endif
>>>       };
>>>
>>>       #endif /* __ASM_X86_HVM_VCPU_H__ */
>>>
>>>
>>> nit: I suspect the code would be far less cluttered with "if 
>>> viridian..." if the
>>> init/deinit/etc functions had dummy versions of those functions when
>>> !HVM_VIRIDIAN in the header.
>>>
>>
>> as Jan explained some time ago [1] it's preferable to compile as much as 
>> possible in all build configuration. Besides most of calls to viridian 
>> code are already guarded by is_viridian_domain() & not actually require 
>> stubs.
>>
>>   -Sergiy
>>
>> [1] 
>> https://lore.kernel.org/xen-devel/36708a3f-2664-4b04-9f5d-f115d362d100@xxxxxxxx/
> 
> That answer seems to state a preference for...
> 
>     if ( foo_enabled() )
>         rc = foo();
> 
> ... against...
> 
>     #ifdef CONFIG_FOO
>     rc = foo();
>     #endif
> 
> ... where foo() in the header looks like...
> 
>     #ifdef CONFIG_FOO
>     int foo(void);
>     #else /* CONFIG_FOO */
>     static inline int foo(void)
>     {
>         return /*some-error*/;
>     }
>     #endif /* CONFIG_FOO */
> 
> But that's not what's going on here, I think? I didn't initially notice the
> subtlety of the change. On more careful look, it seems to rely on the compiler
> doing dead-code-elimination. The functions missing in the linking stage don't
> cause a compile-time error because the conditionals are completely gone by
> then. Neat as it is, it sounds a bit fragile. Can we really rely on this
> behaviour not changing? Furthermore, does MISRA have views about having dead
> code calls to unimplemented functions?

We use DCE like this in many places, so we already rely on this behavior not
changing.

Jan