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

["Alejandro Vallejo" <alejandro.vallejo@xxxxxxxxx>]

On Thu Mar 20, 2025 at 3:22 PM GMT, Jan Beulich wrote:
> 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

I wasn't aware, fair enough. Feel free to ignore then.

Cheers,
Alejandro