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-f115d362d...@suse.com/
>
> 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
