xen/arch/x86/hvm/Kconfig | 10 ++++++++++ xen/arch/x86/hvm/Makefile | 2 +- xen/arch/x86/hvm/hvm.c | 5 +++-- xen/arch/x86/hvm/viridian/viridian.c | 14 ++++++++++---- xen/arch/x86/hvm/vlapic.c | 11 +++++++---- xen/arch/x86/include/asm/hvm/domain.h | 2 ++ xen/arch/x86/include/asm/hvm/hvm.h | 3 ++- xen/arch/x86/include/asm/hvm/vcpu.h | 2 ++ xen/arch/x86/include/asm/hvm/viridian.h | 15 +++++++++++++++ 9 files changed, 52 insertions(+), 12 deletions(-)
From: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
Add config option 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.
[grygorii_strashko@epam.com: fixed NULL pointer deref in
viridian_save_domain_ctxt(); stub viridian_vcpu/domain_init/deinit()]
Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
Signed-off-by: Grygorii Strashko <grygorii_strashko@epam.com>
---
changes in v7:
- s/!is_viridian_vcpu()/!IS_ENABLED(CONFIG_VIRIDIAN) checks in
viridian_load_vcpu/domain_ctxt(), because it is not
guaranteed that HVM_PARAM_VIRIDIAN will be loaded before
viridian_load_vcpu_ctxt() is called (comment from Jason Andryuk)
changes in v6:
- add stubs for viridian_vcpu/domain_init/deinit()
- update Kconfig description
- make set(HVM_PARAM_VIRIDIAN) return -ENODEV
if (!IS_ENABLED(CONFIG_VIRIDIAN) && value)
changes in v5:
- drop "depends on AMD_SVM || INTEL_VMX"
- return -EILSEQ from viridian_load_x() if !VIRIDIAN
changes in v4:
- s/HVM_VIRIDIAN/VIRIDIAN
- add "depends on AMD_SVM || INTEL_VMX"
- add guard !is_viridian_vcpu() checks in viridian_load_vcpu_ctxt/viridian_load_domain_ctxt
changes in v3:
- fixed NULL pointer deref in viridian_save_domain_ctxt() reported for v2,
which caused v2 revert by commit 1fffcf10cd71 ("Revert "x86: make Viridian
support optional"")
v6: https://patchwork.kernel.org/project/xen-devel/patch/20251023151807.560843-1-grygorii_strashko@epam.com/
v5: https://patchwork.kernel.org/project/xen-devel/patch/20250930125215.1087214-1-grygorii_strashko@epam.com/
v4: https://patchwork.kernel.org/project/xen-devel/patch/20250919163139.2821531-1-grygorii_strashko@epam.com/
v3: https://patchwork.kernel.org/project/xen-devel/patch/20250916134114.2214104-1-grygorii_strashko@epam.com/
v2: https://patchwork.kernel.org/project/xen-devel/patch/20250321092633.3982645-1-Sergiy_Kibrik@epam.com/
xen/arch/x86/hvm/Kconfig | 10 ++++++++++
xen/arch/x86/hvm/Makefile | 2 +-
xen/arch/x86/hvm/hvm.c | 5 +++--
xen/arch/x86/hvm/viridian/viridian.c | 14 ++++++++++----
xen/arch/x86/hvm/vlapic.c | 11 +++++++----
xen/arch/x86/include/asm/hvm/domain.h | 2 ++
xen/arch/x86/include/asm/hvm/hvm.h | 3 ++-
xen/arch/x86/include/asm/hvm/vcpu.h | 2 ++
xen/arch/x86/include/asm/hvm/viridian.h | 15 +++++++++++++++
9 files changed, 52 insertions(+), 12 deletions(-)
diff --git a/xen/arch/x86/hvm/Kconfig b/xen/arch/x86/hvm/Kconfig
index c323d767e77c..c1a131d1851a 100644
--- a/xen/arch/x86/hvm/Kconfig
+++ b/xen/arch/x86/hvm/Kconfig
@@ -63,6 +63,16 @@ config ALTP2M
If unsure, stay with defaults.
+config VIRIDIAN
+ bool "Hyper-V enlightenments for guests" if EXPERT
+ default y
+ help
+ Support optimizations for Hyper-V guests such as hypercalls, efficient
+ timers and interrupt handling. This is to improve performance and
+ compatibility of Windows VMs.
+
+ If unsure, say Y.
+
config MEM_PAGING
bool "Xen memory paging support (UNSUPPORTED)" if UNSUPPORTED
depends on VM_EVENT
diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile
index 6ec2c8f2db56..736eb3f966e9 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_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 88ad4100001e..c09fb2ba6873 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4231,8 +4231,9 @@ static int hvm_set_param(struct domain *d, uint32_t index, uint64_t value)
rc = -EINVAL;
break;
case HVM_PARAM_VIRIDIAN:
- if ( (value & ~HVMPV_feature_mask) ||
- !(value & HVMPV_base_freq) )
+ if ( !IS_ENABLED(CONFIG_VIRIDIAN) && value )
+ rc = -ENODEV;
+ else if ( (value & ~HVMPV_feature_mask) || !(value & HVMPV_base_freq) )
rc = -EINVAL;
break;
case HVM_PARAM_IDENT_PT:
diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
index f79cffcb3767..5fb148b2aa15 100644
--- a/xen/arch/x86/hvm/viridian/viridian.c
+++ b/xen/arch/x86/hvm/viridian/viridian.c
@@ -1097,14 +1097,14 @@ static int cf_check viridian_save_domain_ctxt(
{
const struct domain *d = v->domain;
const struct viridian_domain *vd = d->arch.hvm.viridian;
- struct hvm_viridian_domain_context ctxt = {
- .hypercall_gpa = vd->hypercall_gpa.raw,
- .guest_os_id = vd->guest_os_id.raw,
- };
+ struct hvm_viridian_domain_context ctxt = {};
if ( !is_viridian_domain(d) )
return 0;
+ ctxt.hypercall_gpa = vd->hypercall_gpa.raw;
+ ctxt.guest_os_id = vd->guest_os_id.raw,
+
viridian_time_save_domain_ctxt(d, &ctxt);
viridian_synic_save_domain_ctxt(d, &ctxt);
@@ -1117,6 +1117,9 @@ static int cf_check viridian_load_domain_ctxt(
struct viridian_domain *vd = d->arch.hvm.viridian;
struct hvm_viridian_domain_context ctxt;
+ if ( !IS_ENABLED(CONFIG_VIRIDIAN) )
+ return -EILSEQ;
+
if ( hvm_load_entry_zeroextend(VIRIDIAN_DOMAIN, h, &ctxt) != 0 )
return -EINVAL;
@@ -1153,6 +1156,9 @@ static int cf_check viridian_load_vcpu_ctxt(
struct vcpu *v;
struct hvm_viridian_vcpu_context ctxt;
+ if ( !IS_ENABLED(CONFIG_VIRIDIAN) )
+ return -EILSEQ;
+
if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
{
dprintk(XENLOG_G_ERR, "HVM restore: dom%d has no vcpu%u\n",
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 4121285daef8..b315e56d3f18 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -447,7 +447,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);
int vector;
again:
@@ -463,7 +464,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]);
@@ -1375,7 +1376,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);
@@ -1388,7 +1390,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 94d19730b2c1..83be2bd1c29c 100644
--- a/xen/arch/x86/include/asm/hvm/domain.h
+++ b/xen/arch/x86/include/asm/hvm/domain.h
@@ -105,7 +105,9 @@ struct hvm_domain {
/* hypervisor intercepted msix table */
struct list_head msixtbl_list;
+#ifdef CONFIG_VIRIDIAN
struct viridian_domain *viridian;
+#endif
/*
* TSC value that VCPUs use to calculate their tsc_offset value.
diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h
index 340d9d4d5631..ce31401a5d53 100644
--- a/xen/arch/x86/include/asm/hvm/hvm.h
+++ b/xen/arch/x86/include/asm/hvm/hvm.h
@@ -512,7 +512,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_ENABLED(CONFIG_VIRIDIAN) && \
+ is_hvm_domain(d) && (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 3cf7a71fd309..eae9ac53767b 100644
--- a/xen/arch/x86/include/asm/hvm/vcpu.h
+++ b/xen/arch/x86/include/asm/hvm/vcpu.h
@@ -173,7 +173,9 @@ struct hvm_vcpu {
/* Pending hw/sw interrupt (.vector = -1 means nothing pending). */
struct x86_event inject_event;
+#ifdef CONFIG_VIRIDIAN
struct viridian_vcpu *viridian;
+#endif
};
#endif /* __ASM_X86_HVM_VCPU_H__ */
diff --git a/xen/arch/x86/include/asm/hvm/viridian.h b/xen/arch/x86/include/asm/hvm/viridian.h
index 47c9d13841ac..07ea95d4ae6e 100644
--- a/xen/arch/x86/include/asm/hvm/viridian.h
+++ b/xen/arch/x86/include/asm/hvm/viridian.h
@@ -86,11 +86,26 @@ viridian_hypercall(struct cpu_user_regs *regs);
void viridian_time_domain_freeze(const struct domain *d);
void viridian_time_domain_thaw(const struct domain *d);
+#if defined(CONFIG_VIRIDIAN)
int viridian_vcpu_init(struct vcpu *v);
int viridian_domain_init(struct domain *d);
void viridian_vcpu_deinit(struct vcpu *v);
void viridian_domain_deinit(struct domain *d);
+#else
+static inline int viridian_vcpu_init(struct vcpu *v)
+{
+ return 0;
+}
+
+static inline int viridian_domain_init(struct domain *d)
+{
+ return 0;
+}
+
+static inline void viridian_vcpu_deinit(struct vcpu *v) {}
+static inline void viridian_domain_deinit(struct domain *d) {}
+#endif
void viridian_apic_assist_set(const struct vcpu *v);
bool viridian_apic_assist_completed(const struct vcpu *v);
--
2.34.1On 31.10.2025 17:17, Grygorii Strashko wrote:
> --- 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_VIRIDIAN) += viridian/
With this, what is the point of the additions to viridian_load_{domain,vcpu}_ctxt()?
You're adding dead code there, aren't you?
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4231,8 +4231,9 @@ static int hvm_set_param(struct domain *d, uint32_t index, uint64_t value)
> rc = -EINVAL;
> break;
> case HVM_PARAM_VIRIDIAN:
> - if ( (value & ~HVMPV_feature_mask) ||
> - !(value & HVMPV_base_freq) )
> + if ( !IS_ENABLED(CONFIG_VIRIDIAN) && value )
> + rc = -ENODEV;
> + else if ( (value & ~HVMPV_feature_mask) || !(value & HVMPV_base_freq) )
> rc = -EINVAL;
I find the check for value to be (non-)zero a little dubious here: If any caller
passed in 0, it would get back -EINVAL anyway. Imo -ENODEV would be more suitable
in that case as well. Things would be different if 0 was a valid value to pass in.
Jan
On 06.11.25 13:35, Jan Beulich wrote:
> On 31.10.2025 17:17, Grygorii Strashko wrote:
>> --- 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_VIRIDIAN) += viridian/
>
> With this, what is the point of the additions to viridian_load_{domain,vcpu}_ctxt()?
> You're adding dead code there, aren't you?
Hgrr. And we end up back to v3 regarding this part.
Check in viridian_load_{domain,vcpu}_ctxt() may/may not work depending on toolstack
behavior which is not strictly defined (loading HVM_PARAM_VIRIDIAN before/after
viridian_load_{domain,vcpu}_ctxt()).
So what's the conclusion here - drop this check?
>
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -4231,8 +4231,9 @@ static int hvm_set_param(struct domain *d, uint32_t index, uint64_t value)
>> rc = -EINVAL;
>> break;
>> case HVM_PARAM_VIRIDIAN:
>> - if ( (value & ~HVMPV_feature_mask) ||
>> - !(value & HVMPV_base_freq) )
>> + if ( !IS_ENABLED(CONFIG_VIRIDIAN) && value )
>> + rc = -ENODEV;
>> + else if ( (value & ~HVMPV_feature_mask) || !(value & HVMPV_base_freq) )
>> rc = -EINVAL;
>
> I find the check for value to be (non-)zero a little dubious here: If any caller
> passed in 0, it would get back -EINVAL anyway. Imo -ENODEV would be more suitable
> in that case as well. Things would be different if 0 was a valid value to pass in.
The idea was to distinguish between "Feature enabled, Invalid parameter" and "Feature disabled".
--
Best regards,
-grygorii
On 06.11.2025 14:42, Grygorii Strashko wrote:
> On 06.11.25 13:35, Jan Beulich wrote:
>> On 31.10.2025 17:17, Grygorii Strashko wrote:
>>> --- 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_VIRIDIAN) += viridian/
>>
>> With this, what is the point of the additions to viridian_load_{domain,vcpu}_ctxt()?
>> You're adding dead code there, aren't you?
>
> Hgrr. And we end up back to v3 regarding this part.
> Check in viridian_load_{domain,vcpu}_ctxt() may/may not work depending on toolstack
> behavior which is not strictly defined (loading HVM_PARAM_VIRIDIAN before/after
> viridian_load_{domain,vcpu}_ctxt()).
>
> So what's the conclusion here - drop this check?
Well, Misra wants us to not have any dead code. Hence why would we add new
instances of such?
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -4231,8 +4231,9 @@ static int hvm_set_param(struct domain *d, uint32_t index, uint64_t value)
>>> rc = -EINVAL;
>>> break;
>>> case HVM_PARAM_VIRIDIAN:
>>> - if ( (value & ~HVMPV_feature_mask) ||
>>> - !(value & HVMPV_base_freq) )
>>> + if ( !IS_ENABLED(CONFIG_VIRIDIAN) && value )
>>> + rc = -ENODEV;
>>> + else if ( (value & ~HVMPV_feature_mask) || !(value & HVMPV_base_freq) )
>>> rc = -EINVAL;
>>
>> I find the check for value to be (non-)zero a little dubious here: If any caller
>> passed in 0, it would get back -EINVAL anyway. Imo -ENODEV would be more suitable
>> in that case as well. Things would be different if 0 was a valid value to pass in.
>
> The idea was to distinguish between "Feature enabled, Invalid parameter" and "Feature disabled".
But you don't, or else the addition would need to live after the -EINVAL checks.
I also question the utility of knowing "parameter was invalid" when the feature
isn't available anyway.
Jan
On 06.11.25 15:47, Jan Beulich wrote:
> On 06.11.2025 14:42, Grygorii Strashko wrote:
>> On 06.11.25 13:35, Jan Beulich wrote:
>>> On 31.10.2025 17:17, Grygorii Strashko wrote:
>>>> --- 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_VIRIDIAN) += viridian/
>>>
>>> With this, what is the point of the additions to viridian_load_{domain,vcpu}_ctxt()?
>>> You're adding dead code there, aren't you?
>>
>> Hgrr. And we end up back to v3 regarding this part.
>> Check in viridian_load_{domain,vcpu}_ctxt() may/may not work depending on toolstack
>> behavior which is not strictly defined (loading HVM_PARAM_VIRIDIAN before/after
>> viridian_load_{domain,vcpu}_ctxt()).
>>
>> So what's the conclusion here - drop this check?
>
> Well, Misra wants us to not have any dead code. Hence why would we add new
> instances of such?
>
>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>> @@ -4231,8 +4231,9 @@ static int hvm_set_param(struct domain *d, uint32_t index, uint64_t value)
>>>> rc = -EINVAL;
>>>> break;
>>>> case HVM_PARAM_VIRIDIAN:
>>>> - if ( (value & ~HVMPV_feature_mask) ||
>>>> - !(value & HVMPV_base_freq) )
>>>> + if ( !IS_ENABLED(CONFIG_VIRIDIAN) && value )
>>>> + rc = -ENODEV;
>>>> + else if ( (value & ~HVMPV_feature_mask) || !(value & HVMPV_base_freq) )
>>>> rc = -EINVAL;
>>>
>>> I find the check for value to be (non-)zero a little dubious here: If any caller
>>> passed in 0, it would get back -EINVAL anyway. Imo -ENODEV would be more suitable
>>> in that case as well. Things would be different if 0 was a valid value to pass in.
>>
>> The idea was to distinguish between "Feature enabled, Invalid parameter" and "Feature disabled".
> "
> But you don't, or else the addition would need to live after the -EINVAL checks.
> I also question the utility of knowing "parameter was invalid" when the feature
> isn't available anyway.
My understanding here - I need to drop non-zero "value" check.
will be:
case HVM_PARAM_VIRIDIAN:
if ( !IS_ENABLED(CONFIG_VIRIDIAN) )
rc = -ENODEV;
else if ( (value & ~HVMPV_feature_mask) || !(value & HVMPV_base_freq) )
rc = -EINVAL;
break;
--
Best regards,
-grygorii
On 11.11.2025 19:25, Grygorii Strashko wrote:
> On 06.11.25 15:47, Jan Beulich wrote:
>> On 06.11.2025 14:42, Grygorii Strashko wrote:
>>> On 06.11.25 13:35, Jan Beulich wrote:
>>>> On 31.10.2025 17:17, Grygorii Strashko wrote:
>>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>>> @@ -4231,8 +4231,9 @@ static int hvm_set_param(struct domain *d, uint32_t index, uint64_t value)
>>>>> rc = -EINVAL;
>>>>> break;
>>>>> case HVM_PARAM_VIRIDIAN:
>>>>> - if ( (value & ~HVMPV_feature_mask) ||
>>>>> - !(value & HVMPV_base_freq) )
>>>>> + if ( !IS_ENABLED(CONFIG_VIRIDIAN) && value )
>>>>> + rc = -ENODEV;
>>>>> + else if ( (value & ~HVMPV_feature_mask) || !(value & HVMPV_base_freq) )
>>>>> rc = -EINVAL;
>>>>
>>>> I find the check for value to be (non-)zero a little dubious here: If any caller
>>>> passed in 0, it would get back -EINVAL anyway. Imo -ENODEV would be more suitable
>>>> in that case as well. Things would be different if 0 was a valid value to pass in.
>>>
>>> The idea was to distinguish between "Feature enabled, Invalid parameter" and "Feature disabled".
>> "
>> But you don't, or else the addition would need to live after the -EINVAL checks.
>> I also question the utility of knowing "parameter was invalid" when the feature
>> isn't available anyway.
>
> My understanding here - I need to drop non-zero "value" check.
> will be:
>
> case HVM_PARAM_VIRIDIAN:
> if ( !IS_ENABLED(CONFIG_VIRIDIAN) )
> rc = -ENODEV;
> else if ( (value & ~HVMPV_feature_mask) || !(value & HVMPV_base_freq) )
> rc = -EINVAL;
> break;
Yes, or alternatively
case HVM_PARAM_VIRIDIAN:
if ( (value & ~HVMPV_feature_mask) || !(value & HVMPV_base_freq) )
rc = -EINVAL;
else if ( !IS_ENABLED(CONFIG_VIRIDIAN) )
rc = -ENODEV;
break;
Both have their up- and down-sides.
Jan
On Wed Nov 12, 2025 at 7:40 AM CET, Jan Beulich wrote: > On 11.11.2025 19:25, Grygorii Strashko wrote: >> On 06.11.25 15:47, Jan Beulich wrote: >>> On 06.11.2025 14:42, Grygorii Strashko wrote: >>>> On 06.11.25 13:35, Jan Beulich wrote: >>>>> On 31.10.2025 17:17, Grygorii Strashko wrote: >>>>>> --- a/xen/arch/x86/hvm/hvm.c >>>>>> +++ b/xen/arch/x86/hvm/hvm.c >>>>>> @@ -4231,8 +4231,9 @@ static int hvm_set_param(struct domain *d, uint32_t index, uint64_t value) >>>>>> rc = -EINVAL; >>>>>> break; >>>>>> case HVM_PARAM_VIRIDIAN: >>>>>> - if ( (value & ~HVMPV_feature_mask) || >>>>>> - !(value & HVMPV_base_freq) ) >>>>>> + if ( !IS_ENABLED(CONFIG_VIRIDIAN) && value ) >>>>>> + rc = -ENODEV; >>>>>> + else if ( (value & ~HVMPV_feature_mask) || !(value & HVMPV_base_freq) ) >>>>>> rc = -EINVAL; >>>>> >>>>> I find the check for value to be (non-)zero a little dubious here: If any caller >>>>> passed in 0, it would get back -EINVAL anyway. Imo -ENODEV would be more suitable >>>>> in that case as well. Things would be different if 0 was a valid value to pass in. >>>> >>>> The idea was to distinguish between "Feature enabled, Invalid parameter" and "Feature disabled". >>> " >>> But you don't, or else the addition would need to live after the -EINVAL checks. >>> I also question the utility of knowing "parameter was invalid" when the feature >>> isn't available anyway. >> >> My understanding here - I need to drop non-zero "value" check. >> will be: >> >> case HVM_PARAM_VIRIDIAN: >> if ( !IS_ENABLED(CONFIG_VIRIDIAN) ) >> rc = -ENODEV; >> else if ( (value & ~HVMPV_feature_mask) || !(value & HVMPV_base_freq) ) >> rc = -EINVAL; >> break; > > Yes, or alternatively > > case HVM_PARAM_VIRIDIAN: > if ( (value & ~HVMPV_feature_mask) || !(value & HVMPV_base_freq) ) > rc = -EINVAL; > else if ( !IS_ENABLED(CONFIG_VIRIDIAN) ) > rc = -ENODEV; > break; > > Both have their up- and down-sides. > > Jan We should aim for Grygorii's form, imo. If anything because it eliminates the second else-if on !VIRIDIAN, leading to a marginal binary size reduction. There's no value in validation when viridian support just isn't there. Cheers, Alejandro
On 12.11.25 16:27, Alejandro Vallejo wrote:
> On Wed Nov 12, 2025 at 7:40 AM CET, Jan Beulich wrote:
>> On 11.11.2025 19:25, Grygorii Strashko wrote:
>>> On 06.11.25 15:47, Jan Beulich wrote:
>>>> On 06.11.2025 14:42, Grygorii Strashko wrote:
>>>>> On 06.11.25 13:35, Jan Beulich wrote:
>>>>>> On 31.10.2025 17:17, Grygorii Strashko wrote:
>>>>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>>>>> @@ -4231,8 +4231,9 @@ static int hvm_set_param(struct domain *d, uint32_t index, uint64_t value)
>>>>>>> rc = -EINVAL;
>>>>>>> break;
>>>>>>> case HVM_PARAM_VIRIDIAN:
>>>>>>> - if ( (value & ~HVMPV_feature_mask) ||
>>>>>>> - !(value & HVMPV_base_freq) )
>>>>>>> + if ( !IS_ENABLED(CONFIG_VIRIDIAN) && value )
>>>>>>> + rc = -ENODEV;
>>>>>>> + else if ( (value & ~HVMPV_feature_mask) || !(value & HVMPV_base_freq) )
>>>>>>> rc = -EINVAL;
>>>>>>
>>>>>> I find the check for value to be (non-)zero a little dubious here: If any caller
>>>>>> passed in 0, it would get back -EINVAL anyway. Imo -ENODEV would be more suitable
>>>>>> in that case as well. Things would be different if 0 was a valid value to pass in.
>>>>>
>>>>> The idea was to distinguish between "Feature enabled, Invalid parameter" and "Feature disabled".
>>>> "
>>>> But you don't, or else the addition would need to live after the -EINVAL checks.
>>>> I also question the utility of knowing "parameter was invalid" when the feature
>>>> isn't available anyway.
>>>
>>> My understanding here - I need to drop non-zero "value" check.
>>> will be:
>>>
>>> case HVM_PARAM_VIRIDIAN:
>>> if ( !IS_ENABLED(CONFIG_VIRIDIAN) )
>>> rc = -ENODEV;
>>> else if ( (value & ~HVMPV_feature_mask) || !(value & HVMPV_base_freq) )
>>> rc = -EINVAL;
>>> break;
>>
>> Yes, or alternatively
>>
>> case HVM_PARAM_VIRIDIAN:
>> if ( (value & ~HVMPV_feature_mask) || !(value & HVMPV_base_freq) )
>> rc = -EINVAL;
>> else if ( !IS_ENABLED(CONFIG_VIRIDIAN) )
>> rc = -ENODEV;
>> break;
>>
>> Both have their up- and down-sides.
>>
>> Jan
>
> We should aim for Grygorii's form, imo. If anything because it eliminates
> the second else-if on !VIRIDIAN, leading to a marginal binary size reduction.
>
> There's no value in validation when viridian support just isn't there.
I'm planning to resend with below diff applied:
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index c09fb2ba6873..7299cfa90ad5 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4231,7 +4231,7 @@ static int hvm_set_param(struct domain *d, uint32_t index, uint64_t value)
rc = -EINVAL;
break;
case HVM_PARAM_VIRIDIAN:
- if ( !IS_ENABLED(CONFIG_VIRIDIAN) && value )
+ if ( !IS_ENABLED(CONFIG_VIRIDIAN) )
rc = -ENODEV;
else if ( (value & ~HVMPV_feature_mask) || !(value & HVMPV_base_freq) )
rc = -EINVAL;
diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
index 5fb148b2aa15..90e749ceb581 100644
--- a/xen/arch/x86/hvm/viridian/viridian.c
+++ b/xen/arch/x86/hvm/viridian/viridian.c
@@ -1117,9 +1117,6 @@ static int cf_check viridian_load_domain_ctxt(
struct viridian_domain *vd = d->arch.hvm.viridian;
struct hvm_viridian_domain_context ctxt;
- if ( !IS_ENABLED(CONFIG_VIRIDIAN) )
- return -EILSEQ;
-
if ( hvm_load_entry_zeroextend(VIRIDIAN_DOMAIN, h, &ctxt) != 0 )
return -EINVAL;
@@ -1156,9 +1153,6 @@ static int cf_check viridian_load_vcpu_ctxt(
struct vcpu *v;
struct hvm_viridian_vcpu_context ctxt;
- if ( !IS_ENABLED(CONFIG_VIRIDIAN) )
- return -EILSEQ;
-
if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
{
dprintk(XENLOG_G_ERR, "HVM restore: dom%d has no vcpu%u\n",
PS: Not sure I have enough stamina and enthusiasm to continue post v8 version.
--
Best regards,
-grygorii
On 2025-10-31 12:17, Grygorii Strashko wrote: > From: Sergiy Kibrik <Sergiy_Kibrik@epam.com> > > Add config option 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. > > [grygorii_strashko@epam.com: fixed NULL pointer deref in > viridian_save_domain_ctxt(); stub viridian_vcpu/domain_init/deinit()] > Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com> > Signed-off-by: Grygorii Strashko <grygorii_strashko@epam.com> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
© 2016 - 2025 Red Hat, Inc.