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

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

+xen-devel (sent it on private by mistake)

On Mon, 17 Mar 2025, 09:29 Jan Beulich, <jbeulich@xxxxxxxx> wrote:

> On 17.03.2025 10:19, Alejandro Vallejo wrote:
> > Hi,
> >
> > I'm surprised this isn't already possible. Neat!
> >
> > On Mon, 17 Mar 2025, 07:19 Sergiy Kibrik, <Sergiy_Kibrik@xxxxxxxx>
> wrote:
> >
> >> Add config option HVM_VIRIDIAN that covers viridian code within HVM.
> >> Calls to viridian functions guarded by is_viridian_domain() and related
> >> macros.
> >> Having this option may be beneficial by reducing code footprint for
> systems
> >> that are not using Hyper-V.
> >>
> >> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@xxxxxxxx>
> >> ---
> >>  xen/arch/x86/Kconfig                  |  5 +++++
> >>  xen/arch/x86/hvm/Makefile             |  2 +-
> >>  xen/arch/x86/hvm/hvm.c                | 27 ++++++++++++++++++---------
> >>  xen/arch/x86/hvm/vlapic.c             | 11 +++++++----
> >>  xen/arch/x86/include/asm/hvm/domain.h |  4 ++--
> >>  xen/arch/x86/include/asm/hvm/hvm.h    |  3 ++-
> >>  xen/arch/x86/include/asm/hvm/vcpu.h   |  3 ++-
> >>  7 files changed, 37 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> >> index 6e41bc0fb4..34f9b79d98 100644
> >> --- a/xen/arch/x86/Kconfig
> >> +++ b/xen/arch/x86/Kconfig
> >> @@ -348,6 +348,11 @@ config HYPERV_GUEST
> >>
> >>  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.
> >
> > 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"?).
>
> I'm slightly puzzled: Here you're worried about ambiguity, yet then ...
>
> > 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.
>
> ... you suggest to introduce ambiguity here. I'd expect VIRIDIAN alone
> to cover whatever enlightenments Xen might want to use itself, when
> run on top of Viridian.
>
> Jan
>

For the define name, I did say it it was a matter of preference rather
than a worry. I'm perfectly happy with HVM_VIRIDIAN :)

CONFIG_{HVM_}VIRIDIAN would only be seen in code, where we use that
name exclusively for enlightenments given to a guest, AFAIK, so I don't
see as much ambiguity. Doesn't matter much either way.

However,

Non-Xen-developers rely on nconfig/menuconfig titles and descriptions
alone to make decisions, so not providing adequate background (even
if under EXPERT) seems less than great.

Cheers,
Alejandro