| Message ID | 20250317071919.3766976-1-Sergiy_Kibrik@epam.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] x86: make Viridian support optional | expand |
Hi, I'm surprised this isn't already possible. Neat! On Mon, 17 Mar 2025, 07:19 Sergiy Kibrik, <Sergiy_Kibrik@epam.com> 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@epam.com> > --- > 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"?). 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. config MEM_PAGING > bool "Xen memory paging support (UNSUPPORTED)" if UNSUPPORTED > depends on HVM > diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile > index 4c1fa5c6c2..6cc2e74fc4 100644 > --- a/xen/arch/x86/hvm/Makefile > +++ b/xen/arch/x86/hvm/Makefile > @@ -1,6 +1,6 @@ > obj-$(CONFIG_AMD_SVM) += svm/ > obj-$(CONFIG_INTEL_VMX) += vmx/ > -obj-y += viridian/ > +obj-$(CONFIG_HVM_VIRIDIAN) += viridian/ > > obj-y += asid.o > obj-y += dm.o > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > index 2f31180b6f..4f51d0f66c 100644 > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -695,9 +695,12 @@ int hvm_domain_initialise(struct domain *d, > if ( hvm_tsc_scaling_supported ) > d->arch.hvm.tsc_scaling_ratio = hvm_default_tsc_scaling_ratio; > > - rc = viridian_domain_init(d); > - if ( rc ) > - goto fail2; > + if ( is_viridian_domain(d) ) > + { > + rc = viridian_domain_init(d); > + if ( rc ) > + goto fail2; > + } > > rc = alternative_call(hvm_funcs.domain_initialise, d); > if ( rc != 0 ) > @@ -733,7 +736,8 @@ void hvm_domain_relinquish_resources(struct domain *d) > if ( hvm_funcs.nhvm_domain_relinquish_resources ) > alternative_vcall(hvm_funcs.nhvm_domain_relinquish_resources, d); > > - viridian_domain_deinit(d); > + if ( is_viridian_domain(d) ) > + viridian_domain_deinit(d); > > ioreq_server_destroy_all(d); > > @@ -1637,9 +1641,12 @@ int hvm_vcpu_initialise(struct vcpu *v) > && (rc = nestedhvm_vcpu_initialise(v)) < 0 ) /* teardown: > nestedhvm_vcpu_destroy */ > goto fail5; > > - rc = viridian_vcpu_init(v); > - if ( rc ) > - goto fail6; > + if ( is_viridian_domain(v->domain) ) > + { > + rc = viridian_vcpu_init(v); > + if ( rc ) > + goto fail6; > + } > > rc = ioreq_server_add_vcpu_all(d, v); > if ( rc != 0 ) > @@ -1669,13 +1676,15 @@ int hvm_vcpu_initialise(struct vcpu *v) > fail2: > hvm_vcpu_cacheattr_destroy(v); > fail1: > - viridian_vcpu_deinit(v); > + if ( is_viridian_domain(v->domain) ) > + viridian_vcpu_deinit(v); > return rc; > } > > void hvm_vcpu_destroy(struct vcpu *v) > { > - viridian_vcpu_deinit(v); > + if ( is_viridian_domain(v->domain) ) > + viridian_vcpu_deinit(v); > > ioreq_server_remove_vcpu_all(v->domain, v); > > diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c > index 065b2aab5b..b8236dade0 100644 > --- a/xen/arch/x86/hvm/vlapic.c > +++ b/xen/arch/x86/hvm/vlapic.c > @@ -426,7 +426,8 @@ void vlapic_EOI_set(struct vlapic *vlapic) > * priority vector and then recurse to handle the lower priority > * vector. > */ > - bool missed_eoi = viridian_apic_assist_completed(v); > + bool missed_eoi = has_viridian_apic_assist(v->domain) ? > + viridian_apic_assist_completed(v) : false; > int vector; > > again: > @@ -442,7 +443,7 @@ void vlapic_EOI_set(struct vlapic *vlapic) > * NOTE: It is harmless to call viridian_apic_assist_clear() on a > * recursion, even though it is not necessary. > */ > - if ( !missed_eoi ) > + if ( has_viridian_apic_assist(v->domain) && !missed_eoi ) > viridian_apic_assist_clear(v); > > vlapic_clear_vector(vector, &vlapic->regs->data[APIC_ISR]); > @@ -1360,7 +1361,8 @@ int vlapic_has_pending_irq(struct vcpu *v) > * If so, we need to emulate the EOI here before comparing ISR > * with IRR. > */ > - if ( viridian_apic_assist_completed(v) ) > + if ( has_viridian_apic_assist(v->domain) && > + viridian_apic_assist_completed(v) ) > vlapic_EOI_set(vlapic); > > isr = vlapic_find_highest_isr(vlapic); > @@ -1373,7 +1375,8 @@ int vlapic_has_pending_irq(struct vcpu *v) > if ( isr >= 0 && > (irr & 0xf0) <= (isr & 0xf0) ) > { > - viridian_apic_assist_clear(v); > + if ( has_viridian_apic_assist(v->domain) ) > + viridian_apic_assist_clear(v); > return -1; > } > > diff --git a/xen/arch/x86/include/asm/hvm/domain.h > b/xen/arch/x86/include/asm/hvm/domain.h > index 333501d5f2..bc52504cdd 100644 > --- a/xen/arch/x86/include/asm/hvm/domain.h > +++ b/xen/arch/x86/include/asm/hvm/domain.h > @@ -110,9 +110,9 @@ struct hvm_domain { > > /* hypervisor intercepted msix table */ > struct list_head msixtbl_list; > - > +#ifdef CONFIG_HVM_VIRIDIAN > struct viridian_domain *viridian; > - > +#endif > /* > * TSC value that VCPUs use to calculate their tsc_offset value. > * Used during initialization and save/restore. > diff --git a/xen/arch/x86/include/asm/hvm/hvm.h > b/xen/arch/x86/include/asm/hvm/hvm.h > index 963e820113..1bbeece117 100644 > --- a/xen/arch/x86/include/asm/hvm/hvm.h > +++ b/xen/arch/x86/include/asm/hvm/hvm.h > @@ -507,7 +507,8 @@ hvm_get_cpl(struct vcpu *v) > (has_hvm_params(d) ? (d)->arch.hvm.params[HVM_PARAM_VIRIDIAN] : 0) > > #define is_viridian_domain(d) \ > - (is_hvm_domain(d) && (viridian_feature_mask(d) & HVMPV_base_freq)) > + (is_hvm_domain(d) && IS_ENABLED(CONFIG_HVM_VIRIDIAN) && \ > + (viridian_feature_mask(d) & HVMPV_base_freq)) > > #define is_viridian_vcpu(v) \ > is_viridian_domain((v)->domain) > 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. Cheers, Alejandro > >
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@epam.com> 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@epam.com> >> --- >> 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
On 17.03.2025 08:19, Sergiy Kibrik wrote: > --- 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 Imo the prompt wants to include "guest", somewhat along the lines of what Alejandro has also said. > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -695,9 +695,12 @@ int hvm_domain_initialise(struct domain *d, > if ( hvm_tsc_scaling_supported ) > d->arch.hvm.tsc_scaling_ratio = hvm_default_tsc_scaling_ratio; > > - rc = viridian_domain_init(d); > - if ( rc ) > - goto fail2; > + if ( is_viridian_domain(d) ) > + { > + rc = viridian_domain_init(d); > + if ( rc ) > + goto fail2; > + } > > rc = alternative_call(hvm_funcs.domain_initialise, d); > if ( rc != 0 ) > @@ -733,7 +736,8 @@ void hvm_domain_relinquish_resources(struct domain *d) > if ( hvm_funcs.nhvm_domain_relinquish_resources ) > alternative_vcall(hvm_funcs.nhvm_domain_relinquish_resources, d); > > - viridian_domain_deinit(d); > + if ( is_viridian_domain(d) ) > + viridian_domain_deinit(d); > > ioreq_server_destroy_all(d); > > @@ -1637,9 +1641,12 @@ int hvm_vcpu_initialise(struct vcpu *v) > && (rc = nestedhvm_vcpu_initialise(v)) < 0 ) /* teardown: nestedhvm_vcpu_destroy */ > goto fail5; > > - rc = viridian_vcpu_init(v); > - if ( rc ) > - goto fail6; > + if ( is_viridian_domain(v->domain) ) Like you do further up, please also use "d" here and ... > @@ -1669,13 +1676,15 @@ int hvm_vcpu_initialise(struct vcpu *v) > fail2: > hvm_vcpu_cacheattr_destroy(v); > fail1: > - viridian_vcpu_deinit(v); > + if ( is_viridian_domain(v->domain) ) ... here. > --- a/xen/arch/x86/hvm/vlapic.c > +++ b/xen/arch/x86/hvm/vlapic.c > @@ -426,7 +426,8 @@ void vlapic_EOI_set(struct vlapic *vlapic) > * priority vector and then recurse to handle the lower priority > * vector. > */ > - bool missed_eoi = viridian_apic_assist_completed(v); > + bool missed_eoi = has_viridian_apic_assist(v->domain) ? > + viridian_apic_assist_completed(v) : false; bool missed_eoi = has_viridian_apic_assist(v->domain) && viridian_apic_assist_completed(v); ? > --- a/xen/arch/x86/include/asm/hvm/domain.h > +++ b/xen/arch/x86/include/asm/hvm/domain.h > @@ -110,9 +110,9 @@ struct hvm_domain { > > /* hypervisor intercepted msix table */ > struct list_head msixtbl_list; > - > +#ifdef CONFIG_HVM_VIRIDIAN > struct viridian_domain *viridian; > - > +#endif > /* > * TSC value that VCPUs use to calculate their tsc_offset value. > * Used during initialization and save/restore. Why would the blank lines need to go away? > --- a/xen/arch/x86/include/asm/hvm/hvm.h > +++ b/xen/arch/x86/include/asm/hvm/hvm.h > @@ -507,7 +507,8 @@ hvm_get_cpl(struct vcpu *v) > (has_hvm_params(d) ? (d)->arch.hvm.params[HVM_PARAM_VIRIDIAN] : 0) > > #define is_viridian_domain(d) \ > - (is_hvm_domain(d) && (viridian_feature_mask(d) & HVMPV_base_freq)) > + (is_hvm_domain(d) && IS_ENABLED(CONFIG_HVM_VIRIDIAN) && \ > + (viridian_feature_mask(d) & HVMPV_base_freq)) May I suggest to put IS_ENABLED() first? And to adjust (reduce) indentation on the 2nd line? > --- 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 > }; Again, not need for the blank line to go away. Jan
+xen-devel (sent it on private by mistake) On Mon, 17 Mar 2025, 09:29 Jan Beulich, <jbeulich@suse.com> 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@epam.com> > 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@epam.com> > >> --- > >> 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
On 17.03.2025 14:55, Alejandro Vallejo wrote: > On Mon, 17 Mar 2025, 09:29 Jan Beulich, <jbeulich@suse.com> wrote: >> On 17.03.2025 10:19, Alejandro Vallejo wrote: >>> On Mon, 17 Mar 2025, 07:19 Sergiy Kibrik, <Sergiy_Kibrik@epam.com> wrote: >>>> --- 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. > > 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. No question that the prompt wants amending (see also my separate reply to Sergiy), and I fully agree help text wants adding. Jan
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. > 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 [..] > 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@suse.com/
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@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? 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
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@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
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@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 I wasn't aware, fair enough. Feel free to ignore then. Cheers, Alejandro
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 + config MEM_PAGING bool "Xen memory paging support (UNSUPPORTED)" if UNSUPPORTED depends on HVM diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile index 4c1fa5c6c2..6cc2e74fc4 100644 --- a/xen/arch/x86/hvm/Makefile +++ b/xen/arch/x86/hvm/Makefile @@ -1,6 +1,6 @@ obj-$(CONFIG_AMD_SVM) += svm/ obj-$(CONFIG_INTEL_VMX) += vmx/ -obj-y += viridian/ +obj-$(CONFIG_HVM_VIRIDIAN) += viridian/ obj-y += asid.o obj-y += dm.o diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 2f31180b6f..4f51d0f66c 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -695,9 +695,12 @@ int hvm_domain_initialise(struct domain *d, if ( hvm_tsc_scaling_supported ) d->arch.hvm.tsc_scaling_ratio = hvm_default_tsc_scaling_ratio; - rc = viridian_domain_init(d); - if ( rc ) - goto fail2; + if ( is_viridian_domain(d) ) + { + rc = viridian_domain_init(d); + if ( rc ) + goto fail2; + } rc = alternative_call(hvm_funcs.domain_initialise, d); if ( rc != 0 ) @@ -733,7 +736,8 @@ void hvm_domain_relinquish_resources(struct domain *d) if ( hvm_funcs.nhvm_domain_relinquish_resources ) alternative_vcall(hvm_funcs.nhvm_domain_relinquish_resources, d); - viridian_domain_deinit(d); + if ( is_viridian_domain(d) ) + viridian_domain_deinit(d); ioreq_server_destroy_all(d); @@ -1637,9 +1641,12 @@ int hvm_vcpu_initialise(struct vcpu *v) && (rc = nestedhvm_vcpu_initialise(v)) < 0 ) /* teardown: nestedhvm_vcpu_destroy */ goto fail5; - rc = viridian_vcpu_init(v); - if ( rc ) - goto fail6; + if ( is_viridian_domain(v->domain) ) + { + rc = viridian_vcpu_init(v); + if ( rc ) + goto fail6; + } rc = ioreq_server_add_vcpu_all(d, v); if ( rc != 0 ) @@ -1669,13 +1676,15 @@ int hvm_vcpu_initialise(struct vcpu *v) fail2: hvm_vcpu_cacheattr_destroy(v); fail1: - viridian_vcpu_deinit(v); + if ( is_viridian_domain(v->domain) ) + viridian_vcpu_deinit(v); return rc; } void hvm_vcpu_destroy(struct vcpu *v) { - viridian_vcpu_deinit(v); + if ( is_viridian_domain(v->domain) ) + viridian_vcpu_deinit(v); ioreq_server_remove_vcpu_all(v->domain, v); diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c index 065b2aab5b..b8236dade0 100644 --- a/xen/arch/x86/hvm/vlapic.c +++ b/xen/arch/x86/hvm/vlapic.c @@ -426,7 +426,8 @@ void vlapic_EOI_set(struct vlapic *vlapic) * priority vector and then recurse to handle the lower priority * vector. */ - bool missed_eoi = viridian_apic_assist_completed(v); + bool missed_eoi = has_viridian_apic_assist(v->domain) ? + viridian_apic_assist_completed(v) : false; int vector; again: @@ -442,7 +443,7 @@ void vlapic_EOI_set(struct vlapic *vlapic) * NOTE: It is harmless to call viridian_apic_assist_clear() on a * recursion, even though it is not necessary. */ - if ( !missed_eoi ) + if ( has_viridian_apic_assist(v->domain) && !missed_eoi ) viridian_apic_assist_clear(v); vlapic_clear_vector(vector, &vlapic->regs->data[APIC_ISR]); @@ -1360,7 +1361,8 @@ int vlapic_has_pending_irq(struct vcpu *v) * If so, we need to emulate the EOI here before comparing ISR * with IRR. */ - if ( viridian_apic_assist_completed(v) ) + if ( has_viridian_apic_assist(v->domain) && + viridian_apic_assist_completed(v) ) vlapic_EOI_set(vlapic); isr = vlapic_find_highest_isr(vlapic); @@ -1373,7 +1375,8 @@ int vlapic_has_pending_irq(struct vcpu *v) if ( isr >= 0 && (irr & 0xf0) <= (isr & 0xf0) ) { - viridian_apic_assist_clear(v); + if ( has_viridian_apic_assist(v->domain) ) + viridian_apic_assist_clear(v); return -1; } diff --git a/xen/arch/x86/include/asm/hvm/domain.h b/xen/arch/x86/include/asm/hvm/domain.h index 333501d5f2..bc52504cdd 100644 --- a/xen/arch/x86/include/asm/hvm/domain.h +++ b/xen/arch/x86/include/asm/hvm/domain.h @@ -110,9 +110,9 @@ struct hvm_domain { /* hypervisor intercepted msix table */ struct list_head msixtbl_list; - +#ifdef CONFIG_HVM_VIRIDIAN struct viridian_domain *viridian; - +#endif /* * TSC value that VCPUs use to calculate their tsc_offset value. * Used during initialization and save/restore. diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h index 963e820113..1bbeece117 100644 --- a/xen/arch/x86/include/asm/hvm/hvm.h +++ b/xen/arch/x86/include/asm/hvm/hvm.h @@ -507,7 +507,8 @@ hvm_get_cpl(struct vcpu *v) (has_hvm_params(d) ? (d)->arch.hvm.params[HVM_PARAM_VIRIDIAN] : 0) #define is_viridian_domain(d) \ - (is_hvm_domain(d) && (viridian_feature_mask(d) & HVMPV_base_freq)) + (is_hvm_domain(d) && IS_ENABLED(CONFIG_HVM_VIRIDIAN) && \ + (viridian_feature_mask(d) & HVMPV_base_freq)) #define is_viridian_vcpu(v) \ is_viridian_domain((v)->domain) 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__ */
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@epam.com> --- 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(-)