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

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

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?

If dummy functions are warranted, my point stands. Why not make them return
"0", rather than some errno to avoid the conditionals to begin with? If they
aren't, then I'm just making a racket over nothing and feel free to ignore me
:)

FTAOD, this is more of a question to the maintainers than anything else. Is
exploiting dead-code elimination in order to avoid dummy functions something we
want to do moving forward?

Cheers,
Alejandro