[PATCH] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector

[PATCH] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector

[Jane Malalane]

Implement support for the HVMOP_set_evtchn_upcall_vector hypercall in
order to set the per-vCPU event channel vector callback on Linux and
use it in preference of HVM_PARAM_CALLBACK_IRQ.

If the per-VCPU vector setup is successful on BSP, use this method
for the APs. If not, fallback to the global vector-type callback.

Also register callback_irq at per-vCPU event channel setup to trick
toolstack to think the domain is enlightened.

Suggested-by: "Roger Pau Monné" <roger.pau@citrix.com>
Signed-off-by: Jane Malalane <jane.malalane@citrix.com>
---
CC: Juergen Gross <jgross@suse.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@redhat.com>
CC: Borislav Petkov <bp@alien8.de>
CC: Dave Hansen <dave.hansen@linux.intel.com>
CC: x86@kernel.org
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
CC: Jane Malalane <jane.malalane@citrix.com>
CC: Maximilian Heyne <mheyne@amazon.de>
CC: Jan Beulich <jbeulich@suse.com>
CC: Colin Ian King <colin.king@intel.com>
CC: xen-devel@lists.xenproject.org
---
 arch/x86/include/asm/xen/cpuid.h   |  2 ++
 arch/x86/include/asm/xen/events.h  |  1 +
 arch/x86/xen/enlighten_hvm.c       | 19 +++++++++++++++++--
 arch/x86/xen/suspend_hvm.c         | 20 +++++++++++++++++++-
 drivers/xen/events/events_base.c   | 32 ++++++++++++++++++++++++++++----
 include/xen/interface/hvm/hvm_op.h | 15 +++++++++++++++
 6 files changed, 82 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/xen/cpuid.h b/arch/x86/include/asm/xen/cpuid.h
index 78e667a31d6c..6daa9b0c8d11 100644
--- a/arch/x86/include/asm/xen/cpuid.h
+++ b/arch/x86/include/asm/xen/cpuid.h
@@ -107,6 +107,8 @@
  * ID field from 8 to 15 bits, allowing to target APIC IDs up 32768.
  */
 #define XEN_HVM_CPUID_EXT_DEST_ID      (1u << 5)
+/* Per-vCPU event channel upcalls */
+#define XEN_HVM_CPUID_UPCALL_VECTOR    (1u << 6)
 
 /*
  * Leaf 6 (0x40000x05)
diff --git a/arch/x86/include/asm/xen/events.h b/arch/x86/include/asm/xen/events.h
index 068d9b067c83..b2d86c761bf8 100644
--- a/arch/x86/include/asm/xen/events.h
+++ b/arch/x86/include/asm/xen/events.h
@@ -34,4 +34,5 @@ static inline bool xen_support_evtchn_rebind(void)
 	return (!xen_hvm_domain() || xen_have_vector_callback);
 }
 
+extern bool xen_ack_upcall;
 #endif /* _ASM_X86_XEN_EVENTS_H */
diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c
index 8b71b1dd7639..847d1da46ff7 100644
--- a/arch/x86/xen/enlighten_hvm.c
+++ b/arch/x86/xen/enlighten_hvm.c
@@ -7,6 +7,7 @@
 
 #include <xen/features.h>
 #include <xen/events.h>
+#include <xen/interface/hvm/hvm_op.h>
 #include <xen/interface/memory.h>
 
 #include <asm/apic.h>
@@ -30,6 +31,9 @@
 
 static unsigned long shared_info_pfn;
 
+__ro_after_init bool xen_ack_upcall;
+EXPORT_SYMBOL_GPL(xen_ack_upcall);
+
 void xen_hvm_init_shared_info(void)
 {
 	struct xen_add_to_physmap xatp;
@@ -125,6 +129,9 @@ DEFINE_IDTENTRY_SYSVEC(sysvec_xen_hvm_callback)
 {
 	struct pt_regs *old_regs = set_irq_regs(regs);
 
+	if (xen_ack_upcall)
+		ack_APIC_irq();
+
 	inc_irq_stat(irq_hv_callback_count);
 
 	xen_hvm_evtchn_do_upcall();
@@ -168,6 +175,15 @@ static int xen_cpu_up_prepare_hvm(unsigned int cpu)
 	if (!xen_have_vector_callback)
 		return 0;
 
+	if (xen_ack_upcall) {
+		xen_hvm_evtchn_upcall_vector_t op = {
+			.vector = HYPERVISOR_CALLBACK_VECTOR,
+			.vcpu = per_cpu(xen_vcpu_id, cpu),
+		};
+
+		BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector, &op));
+	}
+
 	if (xen_feature(XENFEAT_hvm_safe_pvclock))
 		xen_setup_timer(cpu);
 
@@ -211,8 +227,7 @@ static void __init xen_hvm_guest_init(void)
 
 	xen_panic_handler_init();
 
-	if (!no_vector_callback && xen_feature(XENFEAT_hvm_callback_vector))
-		xen_have_vector_callback = 1;
+	xen_have_vector_callback = !no_vector_callback;
 
 	xen_hvm_smp_init();
 	WARN_ON(xen_cpuhp_setup(xen_cpu_up_prepare_hvm, xen_cpu_dead_hvm));
diff --git a/arch/x86/xen/suspend_hvm.c b/arch/x86/xen/suspend_hvm.c
index 9d548b0c772f..be66e027ef28 100644
--- a/arch/x86/xen/suspend_hvm.c
+++ b/arch/x86/xen/suspend_hvm.c
@@ -5,6 +5,7 @@
 #include <xen/hvm.h>
 #include <xen/features.h>
 #include <xen/interface/features.h>
+#include <xen/events.h>
 
 #include "xen-ops.h"
 
@@ -14,6 +15,23 @@ void xen_hvm_post_suspend(int suspend_cancelled)
 		xen_hvm_init_shared_info();
 		xen_vcpu_restore();
 	}
-	xen_setup_callback_vector();
+	if (xen_ack_upcall) {
+		unsigned int cpu;
+
+		for_each_online_cpu(cpu) {
+			xen_hvm_evtchn_upcall_vector_t op = {
+					.vector = HYPERVISOR_CALLBACK_VECTOR,
+					.vcpu = per_cpu(xen_vcpu_id, cpu),
+			};
+
+			BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector,
+						 &op));
+			/* Trick toolstack to think we are enlightened. */
+			if (!cpu)
+				BUG_ON(xen_set_callback_via(1));
+		}
+	} else {
+		xen_setup_callback_vector();
+	}
 	xen_unplug_emulated_devices();
 }
diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 46d9295d9a6e..a2420b66e735 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -48,6 +48,7 @@
 #include <asm/xen/pci.h>
 #endif
 #include <asm/sync_bitops.h>
+#include <asm/xen/cpuid.h>
 #include <asm/xen/hypercall.h>
 #include <asm/xen/hypervisor.h>
 #include <xen/page.h>
@@ -2200,6 +2201,29 @@ void xen_setup_callback_vector(void)
 	}
 }
 
+/* Setup per-vCPU vector-type callbacks and trick toolstack to think
+ * we are enlightened. If this setup is unavailable, fallback to the
+ * global vector-type callback. */
+static __init void xen_setup_upcall_vector(void)
+{
+	xen_hvm_evtchn_upcall_vector_t op = {
+		.vector = HYPERVISOR_CALLBACK_VECTOR,
+		.vcpu = per_cpu(xen_vcpu_id, 0),
+	};
+
+	if (!xen_have_vector_callback)
+		return;
+
+	if ((cpuid_eax(xen_cpuid_base() + 4) & XEN_HVM_CPUID_UPCALL_VECTOR) &&
+	    !HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector, &op) &&
+	    !xen_set_callback_via(1))
+		xen_ack_upcall = true;
+	else if (xen_feature(XENFEAT_hvm_callback_vector))
+		xen_setup_callback_vector();
+	else
+		xen_have_vector_callback = 0;
+}
+
 static __init void xen_alloc_callback_vector(void)
 {
 	if (!xen_have_vector_callback)
@@ -2210,6 +2234,7 @@ static __init void xen_alloc_callback_vector(void)
 }
 #else
 void xen_setup_callback_vector(void) {}
+static inline void xen_setup_upcall_vector(void) {}
 static inline void xen_alloc_callback_vector(void) {}
 #endif
 
@@ -2271,10 +2296,9 @@ void __init xen_init_IRQ(void)
 		if (xen_initial_domain())
 			pci_xen_initial_domain();
 	}
-	if (xen_feature(XENFEAT_hvm_callback_vector)) {
-		xen_setup_callback_vector();
-		xen_alloc_callback_vector();
-	}
+	xen_setup_upcall_vector();
+	xen_alloc_callback_vector();
+
 
 	if (xen_hvm_domain()) {
 		native_init_IRQ();
diff --git a/include/xen/interface/hvm/hvm_op.h b/include/xen/interface/hvm/hvm_op.h
index f3097e79bb03..e714d8b6ef89 100644
--- a/include/xen/interface/hvm/hvm_op.h
+++ b/include/xen/interface/hvm/hvm_op.h
@@ -46,4 +46,19 @@ struct xen_hvm_get_mem_type {
 };
 DEFINE_GUEST_HANDLE_STRUCT(xen_hvm_get_mem_type);
 
+/*
+ * HVMOP_set_evtchn_upcall_vector: Set a <vector> that should be used for event
+ *                                 channel upcalls on the specified <vcpu>. If set,
+ *                                 this vector will be used in preference to the
+ *                                 domain global callback via (see
+ *                                 HVM_PARAM_CALLBACK_IRQ).
+ */
+#define HVMOP_set_evtchn_upcall_vector 23
+struct xen_hvm_evtchn_upcall_vector {
+    uint32_t vcpu;
+    uint8_t vector;
+};
+typedef struct xen_hvm_evtchn_upcall_vector xen_hvm_evtchn_upcall_vector_t;
+DEFINE_GUEST_HANDLE_STRUCT(xen_hvm_evtchn_upcall_vector_t);
+
 #endif /* __XEN_PUBLIC_HVM_HVM_OP_H__ */
-- 
2.11.0

Re: [PATCH] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector

[Boris Ostrovsky]

On 7/11/22 11:22 AM, Jane Malalane wrote:
> --- a/arch/x86/xen/enlighten_hvm.c
> +++ b/arch/x86/xen/enlighten_hvm.c
> @@ -7,6 +7,7 @@
>   
>   #include <xen/features.h>
>   #include <xen/events.h>
> +#include <xen/interface/hvm/hvm_op.h>
>   #include <xen/interface/memory.h>
>   
>   #include <asm/apic.h>
> @@ -30,6 +31,9 @@
>   
>   static unsigned long shared_info_pfn;
>   
> +__ro_after_init bool xen_ack_upcall;
> +EXPORT_SYMBOL_GPL(xen_ack_upcall);


Shouldn't this be called something like xen_percpu_upcall?


> +
>   void xen_hvm_init_shared_info(void)
>   {
>   	struct xen_add_to_physmap xatp;
> @@ -125,6 +129,9 @@ DEFINE_IDTENTRY_SYSVEC(sysvec_xen_hvm_callback)
>   {
>   	struct pt_regs *old_regs = set_irq_regs(regs);
>   
> +	if (xen_ack_upcall)
> +		ack_APIC_irq();
> +
>   	inc_irq_stat(irq_hv_callback_count);
>   
>   	xen_hvm_evtchn_do_upcall();
> @@ -168,6 +175,15 @@ static int xen_cpu_up_prepare_hvm(unsigned int cpu)
>   	if (!xen_have_vector_callback)
>   		return 0;
>   
> +	if (xen_ack_upcall) {
> +		xen_hvm_evtchn_upcall_vector_t op = {
> +			.vector = HYPERVISOR_CALLBACK_VECTOR,
> +			.vcpu = per_cpu(xen_vcpu_id, cpu),
> +		};
> +
> +		BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector, &op));


Does this have to be fatal? Can't we just fail bringing this vcpu up?


> +	}
> +
>   	if (xen_feature(XENFEAT_hvm_safe_pvclock))
>   		xen_setup_timer(cpu);
>   
> @@ -211,8 +227,7 @@ static void __init xen_hvm_guest_init(void)
>   
>   	xen_panic_handler_init();
>   
> -	if (!no_vector_callback && xen_feature(XENFEAT_hvm_callback_vector))
> -		xen_have_vector_callback = 1;
> +	xen_have_vector_callback = !no_vector_callback;


Can we get rid of one of those two variables then?


>   
>   	xen_hvm_smp_init();
>   	WARN_ON(xen_cpuhp_setup(xen_cpu_up_prepare_hvm, xen_cpu_dead_hvm));
> diff --git a/arch/x86/xen/suspend_hvm.c b/arch/x86/xen/suspend_hvm.c
> index 9d548b0c772f..be66e027ef28 100644
> --- a/arch/x86/xen/suspend_hvm.c
> +++ b/arch/x86/xen/suspend_hvm.c
> @@ -5,6 +5,7 @@
>   #include <xen/hvm.h>
>   #include <xen/features.h>
>   #include <xen/interface/features.h>
> +#include <xen/events.h>
>   
>   #include "xen-ops.h"
>   
> @@ -14,6 +15,23 @@ void xen_hvm_post_suspend(int suspend_cancelled)
>   		xen_hvm_init_shared_info();
>   		xen_vcpu_restore();
>   	}
> -	xen_setup_callback_vector();
> +	if (xen_ack_upcall) {
> +		unsigned int cpu;
> +
> +		for_each_online_cpu(cpu) {
> +			xen_hvm_evtchn_upcall_vector_t op = {
> +					.vector = HYPERVISOR_CALLBACK_VECTOR,
> +					.vcpu = per_cpu(xen_vcpu_id, cpu),
> +			};
> +
> +			BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector,
> +						 &op));
> +			/* Trick toolstack to think we are enlightened. */
> +			if (!cpu)
> +				BUG_ON(xen_set_callback_via(1));


What are you trying to make the toolstack aware of? That we have *a* callback (either global or percpu)?


BTW, you can take it out the loop. And maybe @op definition too, except for .vcpu assignment.


> +		}
> +	} else {
> +		xen_setup_callback_vector();
> +	}
>   	xen_unplug_emulated_devices();
>   }


-boris

Re: [PATCH] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector

[Jane Malalane]

On 14/07/2022 00:27, Boris Ostrovsky wrote:
> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open 
> attachments unless you have verified the sender and know the content is 
> safe.
> 
> On 7/11/22 11:22 AM, Jane Malalane wrote:
>> --- a/arch/x86/xen/enlighten_hvm.c
>> +++ b/arch/x86/xen/enlighten_hvm.c
>> @@ -7,6 +7,7 @@
>>   #include <xen/features.h>
>>   #include <xen/events.h>
>> +#include <xen/interface/hvm/hvm_op.h>
>>   #include <xen/interface/memory.h>
>>   #include <asm/apic.h>
>> @@ -30,6 +31,9 @@
>>   static unsigned long shared_info_pfn;
>> +__ro_after_init bool xen_ack_upcall;
>> +EXPORT_SYMBOL_GPL(xen_ack_upcall);
> 
> 
> Shouldn't this be called something like xen_percpu_upcall?
Sure.
> 
> 
>> +
>>   void xen_hvm_init_shared_info(void)
>>   {
>>       struct xen_add_to_physmap xatp;
>> @@ -125,6 +129,9 @@ DEFINE_IDTENTRY_SYSVEC(sysvec_xen_hvm_callback)
>>   {
>>       struct pt_regs *old_regs = set_irq_regs(regs);
>> +    if (xen_ack_upcall)
>> +        ack_APIC_irq();
>> +
>>       inc_irq_stat(irq_hv_callback_count);
>>       xen_hvm_evtchn_do_upcall();
>> @@ -168,6 +175,15 @@ static int xen_cpu_up_prepare_hvm(unsigned int cpu)
>>       if (!xen_have_vector_callback)
>>           return 0;
>> +    if (xen_ack_upcall) {
>> +        xen_hvm_evtchn_upcall_vector_t op = {
>> +            .vector = HYPERVISOR_CALLBACK_VECTOR,
>> +            .vcpu = per_cpu(xen_vcpu_id, cpu),
>> +        };
>> +
>> +        BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector, &op));
> 
> 
> Does this have to be fatal? Can't we just fail bringing this vcpu up?
Yes, will amend.
> 
> 
>> +    }
>> +
>>       if (xen_feature(XENFEAT_hvm_safe_pvclock))
>>           xen_setup_timer(cpu);
>> @@ -211,8 +227,7 @@ static void __init xen_hvm_guest_init(void)
>>       xen_panic_handler_init();
>> -    if (!no_vector_callback && xen_feature(XENFEAT_hvm_callback_vector))
>> -        xen_have_vector_callback = 1;
>> +    xen_have_vector_callback = !no_vector_callback;
> 
> 
> Can we get rid of one of those two variables then?
I'll remove no_vector_callback for less code changes.
> 
> 
>>       xen_hvm_smp_init();
>>       WARN_ON(xen_cpuhp_setup(xen_cpu_up_prepare_hvm, xen_cpu_dead_hvm));
>> diff --git a/arch/x86/xen/suspend_hvm.c b/arch/x86/xen/suspend_hvm.c
>> index 9d548b0c772f..be66e027ef28 100644
>> --- a/arch/x86/xen/suspend_hvm.c
>> +++ b/arch/x86/xen/suspend_hvm.c
>> @@ -5,6 +5,7 @@
>>   #include <xen/hvm.h>
>>   #include <xen/features.h>
>>   #include <xen/interface/features.h>
>> +#include <xen/events.h>
>>   #include "xen-ops.h"
>> @@ -14,6 +15,23 @@ void xen_hvm_post_suspend(int suspend_cancelled)
>>           xen_hvm_init_shared_info();
>>           xen_vcpu_restore();
>>       }
>> -    xen_setup_callback_vector();
>> +    if (xen_ack_upcall) {
>> +        unsigned int cpu;
>> +
>> +        for_each_online_cpu(cpu) {
>> +            xen_hvm_evtchn_upcall_vector_t op = {
>> +                    .vector = HYPERVISOR_CALLBACK_VECTOR,
>> +                    .vcpu = per_cpu(xen_vcpu_id, cpu),
>> +            };
>> +
>> +            BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector,
>> +                         &op));
>> +            /* Trick toolstack to think we are enlightened. */
>> +            if (!cpu)
>> +                BUG_ON(xen_set_callback_via(1));
> 
> 
> What are you trying to make the toolstack aware of? That we have *a* 
> callback (either global or percpu)?
Yes, specifically for the check in libxl__domain_pvcontrol_available.
> 
> 
> BTW, you can take it out the loop. And maybe @op definition too, except 
> for .vcpu assignment.
> 
> 
>> +        }
>> +    } else {
>> +        xen_setup_callback_vector();
>> +    }
>>       xen_unplug_emulated_devices();
>>   }
> 
> 
> -boris
> 
> 

Thank you,

Jane.

Re: [PATCH] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector

[Andrew Cooper]

On 15/07/2022 09:18, Jane Malalane wrote:
> On 14/07/2022 00:27, Boris Ostrovsky wrote:
>>>       xen_hvm_smp_init();
>>>       WARN_ON(xen_cpuhp_setup(xen_cpu_up_prepare_hvm, xen_cpu_dead_hvm));
>>> diff --git a/arch/x86/xen/suspend_hvm.c b/arch/x86/xen/suspend_hvm.c
>>> index 9d548b0c772f..be66e027ef28 100644
>>> --- a/arch/x86/xen/suspend_hvm.c
>>> +++ b/arch/x86/xen/suspend_hvm.c
>>> @@ -5,6 +5,7 @@
>>>   #include <xen/hvm.h>
>>>   #include <xen/features.h>
>>>   #include <xen/interface/features.h>
>>> +#include <xen/events.h>
>>>   #include "xen-ops.h"
>>> @@ -14,6 +15,23 @@ void xen_hvm_post_suspend(int suspend_cancelled)
>>>           xen_hvm_init_shared_info();
>>>           xen_vcpu_restore();
>>>       }
>>> -    xen_setup_callback_vector();
>>> +    if (xen_ack_upcall) {
>>> +        unsigned int cpu;
>>> +
>>> +        for_each_online_cpu(cpu) {
>>> +            xen_hvm_evtchn_upcall_vector_t op = {
>>> +                    .vector = HYPERVISOR_CALLBACK_VECTOR,
>>> +                    .vcpu = per_cpu(xen_vcpu_id, cpu),
>>> +            };
>>> +
>>> +            BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector,
>>> +                         &op));
>>> +            /* Trick toolstack to think we are enlightened. */
>>> +            if (!cpu)
>>> +                BUG_ON(xen_set_callback_via(1));
>>
>> What are you trying to make the toolstack aware of? That we have *a* 
>> callback (either global or percpu)?
> Yes, specifically for the check in libxl__domain_pvcontrol_available.

And others.

This is all a giant bodge, but basically a lot of tooling uses the
non-zero-ness of the CALLBACK_VIA param to determine whether the VM has
Xen-aware drivers loaded or not.

The value 1 is a CALLBACK_VIA value which encodes GSI 1, and the only
reason this doesn't explode everywhere is because the
evtchn_upcall_vector registration takes priority over GSI delivery.

This is decades of tech debt piled on top of tech debt.

~Andrew

Re: [PATCH] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector

[Boris Ostrovsky]

On 7/15/22 5:50 AM, Andrew Cooper wrote:
> On 15/07/2022 09:18, Jane Malalane wrote:
>> On 14/07/2022 00:27, Boris Ostrovsky wrote:
>>>>        xen_hvm_smp_init();
>>>>        WARN_ON(xen_cpuhp_setup(xen_cpu_up_prepare_hvm, xen_cpu_dead_hvm));
>>>> diff --git a/arch/x86/xen/suspend_hvm.c b/arch/x86/xen/suspend_hvm.c
>>>> index 9d548b0c772f..be66e027ef28 100644
>>>> --- a/arch/x86/xen/suspend_hvm.c
>>>> +++ b/arch/x86/xen/suspend_hvm.c
>>>> @@ -5,6 +5,7 @@
>>>>    #include <xen/hvm.h>
>>>>    #include <xen/features.h>
>>>>    #include <xen/interface/features.h>
>>>> +#include <xen/events.h>
>>>>    #include "xen-ops.h"
>>>> @@ -14,6 +15,23 @@ void xen_hvm_post_suspend(int suspend_cancelled)
>>>>            xen_hvm_init_shared_info();
>>>>            xen_vcpu_restore();
>>>>        }
>>>> -    xen_setup_callback_vector();
>>>> +    if (xen_ack_upcall) {
>>>> +        unsigned int cpu;
>>>> +
>>>> +        for_each_online_cpu(cpu) {
>>>> +            xen_hvm_evtchn_upcall_vector_t op = {
>>>> +                    .vector = HYPERVISOR_CALLBACK_VECTOR,
>>>> +                    .vcpu = per_cpu(xen_vcpu_id, cpu),
>>>> +            };
>>>> +
>>>> +            BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector,
>>>> +                         &op));
>>>> +            /* Trick toolstack to think we are enlightened. */
>>>> +            if (!cpu)
>>>> +                BUG_ON(xen_set_callback_via(1));
>>> What are you trying to make the toolstack aware of? That we have *a*
>>> callback (either global or percpu)?
>> Yes, specifically for the check in libxl__domain_pvcontrol_available.
> And others.
>
> This is all a giant bodge, but basically a lot of tooling uses the
> non-zero-ness of the CALLBACK_VIA param to determine whether the VM has
> Xen-aware drivers loaded or not.
>
> The value 1 is a CALLBACK_VIA value which encodes GSI 1, and the only
> reason this doesn't explode everywhere is because the
> evtchn_upcall_vector registration takes priority over GSI delivery.
>
> This is decades of tech debt piled on top of tech debt.


Feels like it (setting the callback parameter) is something that the hypervisor should do --- no need to expose guests to this.


-boris

Re: [PATCH] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector

[Andrew Cooper]

On 15/07/2022 14:10, Boris Ostrovsky wrote:
>
> On 7/15/22 5:50 AM, Andrew Cooper wrote:
>> On 15/07/2022 09:18, Jane Malalane wrote:
>>> On 14/07/2022 00:27, Boris Ostrovsky wrote:
>>>>>        xen_hvm_smp_init();
>>>>>        WARN_ON(xen_cpuhp_setup(xen_cpu_up_prepare_hvm,
>>>>> xen_cpu_dead_hvm));
>>>>> diff --git a/arch/x86/xen/suspend_hvm.c b/arch/x86/xen/suspend_hvm.c
>>>>> index 9d548b0c772f..be66e027ef28 100644
>>>>> --- a/arch/x86/xen/suspend_hvm.c
>>>>> +++ b/arch/x86/xen/suspend_hvm.c
>>>>> @@ -5,6 +5,7 @@
>>>>>    #include <xen/hvm.h>
>>>>>    #include <xen/features.h>
>>>>>    #include <xen/interface/features.h>
>>>>> +#include <xen/events.h>
>>>>>    #include "xen-ops.h"
>>>>> @@ -14,6 +15,23 @@ void xen_hvm_post_suspend(int suspend_cancelled)
>>>>>            xen_hvm_init_shared_info();
>>>>>            xen_vcpu_restore();
>>>>>        }
>>>>> -    xen_setup_callback_vector();
>>>>> +    if (xen_ack_upcall) {
>>>>> +        unsigned int cpu;
>>>>> +
>>>>> +        for_each_online_cpu(cpu) {
>>>>> +            xen_hvm_evtchn_upcall_vector_t op = {
>>>>> +                    .vector = HYPERVISOR_CALLBACK_VECTOR,
>>>>> +                    .vcpu = per_cpu(xen_vcpu_id, cpu),
>>>>> +            };
>>>>> +
>>>>> +            BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector,
>>>>> +                         &op));
>>>>> +            /* Trick toolstack to think we are enlightened. */
>>>>> +            if (!cpu)
>>>>> +                BUG_ON(xen_set_callback_via(1));
>>>> What are you trying to make the toolstack aware of? That we have *a*
>>>> callback (either global or percpu)?
>>> Yes, specifically for the check in libxl__domain_pvcontrol_available.
>> And others.
>>
>> This is all a giant bodge, but basically a lot of tooling uses the
>> non-zero-ness of the CALLBACK_VIA param to determine whether the VM has
>> Xen-aware drivers loaded or not.
>>
>> The value 1 is a CALLBACK_VIA value which encodes GSI 1, and the only
>> reason this doesn't explode everywhere is because the
>> evtchn_upcall_vector registration takes priority over GSI delivery.
>>
>> This is decades of tech debt piled on top of tech debt.
>
>
> Feels like it (setting the callback parameter) is something that the
> hypervisor should do --- no need to expose guests to this.

Sensible or not, it is the ABI.

Linux still needs to work (nicely) with older Xen's in the world, and we
can't just retrofit a change in the hypervisor which says "btw, this ABI
we've just changed now has a side effect of modifying a field that you
also logically own".

~Andrew

Re: [PATCH] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector

[Boris Ostrovsky]

On 7/18/22 4:56 AM, Andrew Cooper wrote:
> On 15/07/2022 14:10, Boris Ostrovsky wrote:
>> On 7/15/22 5:50 AM, Andrew Cooper wrote:
>>> On 15/07/2022 09:18, Jane Malalane wrote:
>>>> On 14/07/2022 00:27, Boris Ostrovsky wrote:
>>>>>>         xen_hvm_smp_init();
>>>>>>         WARN_ON(xen_cpuhp_setup(xen_cpu_up_prepare_hvm,
>>>>>> xen_cpu_dead_hvm));
>>>>>> diff --git a/arch/x86/xen/suspend_hvm.c b/arch/x86/xen/suspend_hvm.c
>>>>>> index 9d548b0c772f..be66e027ef28 100644
>>>>>> --- a/arch/x86/xen/suspend_hvm.c
>>>>>> +++ b/arch/x86/xen/suspend_hvm.c
>>>>>> @@ -5,6 +5,7 @@
>>>>>>     #include <xen/hvm.h>
>>>>>>     #include <xen/features.h>
>>>>>>     #include <xen/interface/features.h>
>>>>>> +#include <xen/events.h>
>>>>>>     #include "xen-ops.h"
>>>>>> @@ -14,6 +15,23 @@ void xen_hvm_post_suspend(int suspend_cancelled)
>>>>>>             xen_hvm_init_shared_info();
>>>>>>             xen_vcpu_restore();
>>>>>>         }
>>>>>> -    xen_setup_callback_vector();
>>>>>> +    if (xen_ack_upcall) {
>>>>>> +        unsigned int cpu;
>>>>>> +
>>>>>> +        for_each_online_cpu(cpu) {
>>>>>> +            xen_hvm_evtchn_upcall_vector_t op = {
>>>>>> +                    .vector = HYPERVISOR_CALLBACK_VECTOR,
>>>>>> +                    .vcpu = per_cpu(xen_vcpu_id, cpu),
>>>>>> +            };
>>>>>> +
>>>>>> +            BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector,
>>>>>> +                         &op));
>>>>>> +            /* Trick toolstack to think we are enlightened. */
>>>>>> +            if (!cpu)
>>>>>> +                BUG_ON(xen_set_callback_via(1));
>>>>> What are you trying to make the toolstack aware of? That we have *a*
>>>>> callback (either global or percpu)?
>>>> Yes, specifically for the check in libxl__domain_pvcontrol_available.
>>> And others.
>>>
>>> This is all a giant bodge, but basically a lot of tooling uses the
>>> non-zero-ness of the CALLBACK_VIA param to determine whether the VM has
>>> Xen-aware drivers loaded or not.
>>>
>>> The value 1 is a CALLBACK_VIA value which encodes GSI 1, and the only
>>> reason this doesn't explode everywhere is because the
>>> evtchn_upcall_vector registration takes priority over GSI delivery.
>>>
>>> This is decades of tech debt piled on top of tech debt.
>>
>> Feels like it (setting the callback parameter) is something that the
>> hypervisor should do --- no need to expose guests to this.
> Sensible or not, it is the ABI.
>
> Linux still needs to work (nicely) with older Xen's in the world, and we
> can't just retrofit a change in the hypervisor which says "btw, this ABI
> we've just changed now has a side effect of modifying a field that you
> also logically own".


The hypercall has been around for a while so I understand ABI concerns there but XEN_HVM_CPUID_UPCALL_VECTOR was introduced only a month ago. Why not tie presence of this bit to no longer having to explicitly set the callback field?


-boris

Re: [PATCH] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector

[Jane Malalane]

On 18/07/2022 14:59, Boris Ostrovsky wrote:
> 
> On 7/18/22 4:56 AM, Andrew Cooper wrote:
>> On 15/07/2022 14:10, Boris Ostrovsky wrote:
>>> On 7/15/22 5:50 AM, Andrew Cooper wrote:
>>>> On 15/07/2022 09:18, Jane Malalane wrote:
>>>>> On 14/07/2022 00:27, Boris Ostrovsky wrote:
>>>>>>>         xen_hvm_smp_init();
>>>>>>>         WARN_ON(xen_cpuhp_setup(xen_cpu_up_prepare_hvm,
>>>>>>> xen_cpu_dead_hvm));
>>>>>>> diff --git a/arch/x86/xen/suspend_hvm.c b/arch/x86/xen/suspend_hvm.c
>>>>>>> index 9d548b0c772f..be66e027ef28 100644
>>>>>>> --- a/arch/x86/xen/suspend_hvm.c
>>>>>>> +++ b/arch/x86/xen/suspend_hvm.c
>>>>>>> @@ -5,6 +5,7 @@
>>>>>>>     #include <xen/hvm.h>
>>>>>>>     #include <xen/features.h>
>>>>>>>     #include <xen/interface/features.h>
>>>>>>> +#include <xen/events.h>
>>>>>>>     #include "xen-ops.h"
>>>>>>> @@ -14,6 +15,23 @@ void xen_hvm_post_suspend(int suspend_cancelled)
>>>>>>>             xen_hvm_init_shared_info();
>>>>>>>             xen_vcpu_restore();
>>>>>>>         }
>>>>>>> -    xen_setup_callback_vector();
>>>>>>> +    if (xen_ack_upcall) {
>>>>>>> +        unsigned int cpu;
>>>>>>> +
>>>>>>> +        for_each_online_cpu(cpu) {
>>>>>>> +            xen_hvm_evtchn_upcall_vector_t op = {
>>>>>>> +                    .vector = HYPERVISOR_CALLBACK_VECTOR,
>>>>>>> +                    .vcpu = per_cpu(xen_vcpu_id, cpu),
>>>>>>> +            };
>>>>>>> +
>>>>>>> +            
>>>>>>> BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector,
>>>>>>> +                         &op));
>>>>>>> +            /* Trick toolstack to think we are enlightened. */
>>>>>>> +            if (!cpu)
>>>>>>> +                BUG_ON(xen_set_callback_via(1));
>>>>>> What are you trying to make the toolstack aware of? That we have *a*
>>>>>> callback (either global or percpu)?
>>>>> Yes, specifically for the check in libxl__domain_pvcontrol_available.
>>>> And others.
>>>>
>>>> This is all a giant bodge, but basically a lot of tooling uses the
>>>> non-zero-ness of the CALLBACK_VIA param to determine whether the VM has
>>>> Xen-aware drivers loaded or not.
>>>>
>>>> The value 1 is a CALLBACK_VIA value which encodes GSI 1, and the only
>>>> reason this doesn't explode everywhere is because the
>>>> evtchn_upcall_vector registration takes priority over GSI delivery.
>>>>
>>>> This is decades of tech debt piled on top of tech debt.
>>>
>>> Feels like it (setting the callback parameter) is something that the
>>> hypervisor should do --- no need to expose guests to this.
>> Sensible or not, it is the ABI.
>>
>> Linux still needs to work (nicely) with older Xen's in the world, and we
>> can't just retrofit a change in the hypervisor which says "btw, this ABI
>> we've just changed now has a side effect of modifying a field that you
>> also logically own".
> 
> 
> The hypercall has been around for a while so I understand ABI concerns 
> there but XEN_HVM_CPUID_UPCALL_VECTOR was introduced only a month ago. 
> Why not tie presence of this bit to no longer having to explicitly set 
> the callback field?
> 
Any other opinions on this?

(i.e., calling xen_set_callback_via(1) after 
HVMOP_set_evtchn_upcall_vector OR not exposing this to guests and 
instead having Xen call this function (in hvmop_set_evtchn_upcall_vector 
maybe) and tieing its presense to XEN_HVM_CPUID_UPCALL_VECTOR which was 
recently added)

Thank you,

Jane.

Re: [PATCH] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector

[Andrew Cooper]

On 18/07/2022 14:59, Boris Ostrovsky wrote:
>
> On 7/18/22 4:56 AM, Andrew Cooper wrote:
>> On 15/07/2022 14:10, Boris Ostrovsky wrote:
>>> On 7/15/22 5:50 AM, Andrew Cooper wrote:
>>>> On 15/07/2022 09:18, Jane Malalane wrote:
>>>>> On 14/07/2022 00:27, Boris Ostrovsky wrote:
>>>>>>>         xen_hvm_smp_init();
>>>>>>>         WARN_ON(xen_cpuhp_setup(xen_cpu_up_prepare_hvm,
>>>>>>> xen_cpu_dead_hvm));
>>>>>>> diff --git a/arch/x86/xen/suspend_hvm.c
>>>>>>> b/arch/x86/xen/suspend_hvm.c
>>>>>>> index 9d548b0c772f..be66e027ef28 100644
>>>>>>> --- a/arch/x86/xen/suspend_hvm.c
>>>>>>> +++ b/arch/x86/xen/suspend_hvm.c
>>>>>>> @@ -5,6 +5,7 @@
>>>>>>>     #include <xen/hvm.h>
>>>>>>>     #include <xen/features.h>
>>>>>>>     #include <xen/interface/features.h>
>>>>>>> +#include <xen/events.h>
>>>>>>>     #include "xen-ops.h"
>>>>>>> @@ -14,6 +15,23 @@ void xen_hvm_post_suspend(int suspend_cancelled)
>>>>>>>             xen_hvm_init_shared_info();
>>>>>>>             xen_vcpu_restore();
>>>>>>>         }
>>>>>>> -    xen_setup_callback_vector();
>>>>>>> +    if (xen_ack_upcall) {
>>>>>>> +        unsigned int cpu;
>>>>>>> +
>>>>>>> +        for_each_online_cpu(cpu) {
>>>>>>> +            xen_hvm_evtchn_upcall_vector_t op = {
>>>>>>> +                    .vector = HYPERVISOR_CALLBACK_VECTOR,
>>>>>>> +                    .vcpu = per_cpu(xen_vcpu_id, cpu),
>>>>>>> +            };
>>>>>>> +
>>>>>>> +           
>>>>>>> BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector,
>>>>>>> +                         &op));
>>>>>>> +            /* Trick toolstack to think we are enlightened. */
>>>>>>> +            if (!cpu)
>>>>>>> +                BUG_ON(xen_set_callback_via(1));
>>>>>> What are you trying to make the toolstack aware of? That we have *a*
>>>>>> callback (either global or percpu)?
>>>>> Yes, specifically for the check in libxl__domain_pvcontrol_available.
>>>> And others.
>>>>
>>>> This is all a giant bodge, but basically a lot of tooling uses the
>>>> non-zero-ness of the CALLBACK_VIA param to determine whether the VM
>>>> has
>>>> Xen-aware drivers loaded or not.
>>>>
>>>> The value 1 is a CALLBACK_VIA value which encodes GSI 1, and the only
>>>> reason this doesn't explode everywhere is because the
>>>> evtchn_upcall_vector registration takes priority over GSI delivery.
>>>>
>>>> This is decades of tech debt piled on top of tech debt.
>>>
>>> Feels like it (setting the callback parameter) is something that the
>>> hypervisor should do --- no need to expose guests to this.
>> Sensible or not, it is the ABI.
>>
>> Linux still needs to work (nicely) with older Xen's in the world, and we
>> can't just retrofit a change in the hypervisor which says "btw, this ABI
>> we've just changed now has a side effect of modifying a field that you
>> also logically own".
>
>
> The hypercall has been around for a while so I understand ABI concerns
> there but XEN_HVM_CPUID_UPCALL_VECTOR was introduced only a month ago.
> Why not tie presence of this bit to no longer having to explicitly set
> the callback field?

Because that's a breaking change for ~5 years of windows drivers.

~Andrew

Re: [PATCH] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector

[Boris Ostrovsky]

On 7/25/22 6:03 AM, Jane Malalane wrote:
> On 18/07/2022 14:59, Boris Ostrovsky wrote:
>> On 7/18/22 4:56 AM, Andrew Cooper wrote:
>>> On 15/07/2022 14:10, Boris Ostrovsky wrote:
>>>> On 7/15/22 5:50 AM, Andrew Cooper wrote:
>>>>> On 15/07/2022 09:18, Jane Malalane wrote:
>>>>>> On 14/07/2022 00:27, Boris Ostrovsky wrote:
>>>>>>>>          xen_hvm_smp_init();
>>>>>>>>          WARN_ON(xen_cpuhp_setup(xen_cpu_up_prepare_hvm,
>>>>>>>> xen_cpu_dead_hvm));
>>>>>>>> diff --git a/arch/x86/xen/suspend_hvm.c b/arch/x86/xen/suspend_hvm.c
>>>>>>>> index 9d548b0c772f..be66e027ef28 100644
>>>>>>>> --- a/arch/x86/xen/suspend_hvm.c
>>>>>>>> +++ b/arch/x86/xen/suspend_hvm.c
>>>>>>>> @@ -5,6 +5,7 @@
>>>>>>>>      #include <xen/hvm.h>
>>>>>>>>      #include <xen/features.h>
>>>>>>>>      #include <xen/interface/features.h>
>>>>>>>> +#include <xen/events.h>
>>>>>>>>      #include "xen-ops.h"
>>>>>>>> @@ -14,6 +15,23 @@ void xen_hvm_post_suspend(int suspend_cancelled)
>>>>>>>>              xen_hvm_init_shared_info();
>>>>>>>>              xen_vcpu_restore();
>>>>>>>>          }
>>>>>>>> -    xen_setup_callback_vector();
>>>>>>>> +    if (xen_ack_upcall) {
>>>>>>>> +        unsigned int cpu;
>>>>>>>> +
>>>>>>>> +        for_each_online_cpu(cpu) {
>>>>>>>> +            xen_hvm_evtchn_upcall_vector_t op = {
>>>>>>>> +                    .vector = HYPERVISOR_CALLBACK_VECTOR,
>>>>>>>> +                    .vcpu = per_cpu(xen_vcpu_id, cpu),
>>>>>>>> +            };
>>>>>>>> +
>>>>>>>> +
>>>>>>>> BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector,
>>>>>>>> +                         &op));
>>>>>>>> +            /* Trick toolstack to think we are enlightened. */
>>>>>>>> +            if (!cpu)
>>>>>>>> +                BUG_ON(xen_set_callback_via(1));
>>>>>>> What are you trying to make the toolstack aware of? That we have *a*
>>>>>>> callback (either global or percpu)?
>>>>>> Yes, specifically for the check in libxl__domain_pvcontrol_available.
>>>>> And others.
>>>>>
>>>>> This is all a giant bodge, but basically a lot of tooling uses the
>>>>> non-zero-ness of the CALLBACK_VIA param to determine whether the VM has
>>>>> Xen-aware drivers loaded or not.
>>>>>
>>>>> The value 1 is a CALLBACK_VIA value which encodes GSI 1, and the only
>>>>> reason this doesn't explode everywhere is because the
>>>>> evtchn_upcall_vector registration takes priority over GSI delivery.
>>>>>
>>>>> This is decades of tech debt piled on top of tech debt.
>>>> Feels like it (setting the callback parameter) is something that the
>>>> hypervisor should do --- no need to expose guests to this.
>>> Sensible or not, it is the ABI.
>>>
>>> Linux still needs to work (nicely) with older Xen's in the world, and we
>>> can't just retrofit a change in the hypervisor which says "btw, this ABI
>>> we've just changed now has a side effect of modifying a field that you
>>> also logically own".
>>
>> The hypercall has been around for a while so I understand ABI concerns
>> there but XEN_HVM_CPUID_UPCALL_VECTOR was introduced only a month ago.
>> Why not tie presence of this bit to no longer having to explicitly set
>> the callback field?
>>
> Any other opinions on this?
>
> (i.e., calling xen_set_callback_via(1) after
> HVMOP_set_evtchn_upcall_vector OR not exposing this to guests and
> instead having Xen call this function (in hvmop_set_evtchn_upcall_vector
> maybe) and tieing its presense to XEN_HVM_CPUID_UPCALL_VECTOR which was
> recently added)


CPUID won't help here, I wasn't thinking clearly.


Can we wrap the HVMOP_set_evtchn_upcall_vector hypercall in a function that will decide whether or not to also do xen_set_callback_via(1)?


-boris

Re: [PATCH] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector

[Jane Malalane]

On 25/07/2022 21:46, Boris Ostrovsky wrote:
> 
> On 7/25/22 6:03 AM, Jane Malalane wrote:
>> On 18/07/2022 14:59, Boris Ostrovsky wrote:
>>> On 7/18/22 4:56 AM, Andrew Cooper wrote:
>>>> On 15/07/2022 14:10, Boris Ostrovsky wrote:
>>>>> On 7/15/22 5:50 AM, Andrew Cooper wrote:
>>>>>> On 15/07/2022 09:18, Jane Malalane wrote:
>>>>>>> On 14/07/2022 00:27, Boris Ostrovsky wrote:
>>>>>>>>>          xen_hvm_smp_init();
>>>>>>>>>          WARN_ON(xen_cpuhp_setup(xen_cpu_up_prepare_hvm,
>>>>>>>>> xen_cpu_dead_hvm));
>>>>>>>>> diff --git a/arch/x86/xen/suspend_hvm.c 
>>>>>>>>> b/arch/x86/xen/suspend_hvm.c
>>>>>>>>> index 9d548b0c772f..be66e027ef28 100644
>>>>>>>>> --- a/arch/x86/xen/suspend_hvm.c
>>>>>>>>> +++ b/arch/x86/xen/suspend_hvm.c
>>>>>>>>> @@ -5,6 +5,7 @@
>>>>>>>>>      #include <xen/hvm.h>
>>>>>>>>>      #include <xen/features.h>
>>>>>>>>>      #include <xen/interface/features.h>
>>>>>>>>> +#include <xen/events.h>
>>>>>>>>>      #include "xen-ops.h"
>>>>>>>>> @@ -14,6 +15,23 @@ void xen_hvm_post_suspend(int 
>>>>>>>>> suspend_cancelled)
>>>>>>>>>              xen_hvm_init_shared_info();
>>>>>>>>>              xen_vcpu_restore();
>>>>>>>>>          }
>>>>>>>>> -    xen_setup_callback_vector();
>>>>>>>>> +    if (xen_ack_upcall) {
>>>>>>>>> +        unsigned int cpu;
>>>>>>>>> +
>>>>>>>>> +        for_each_online_cpu(cpu) {
>>>>>>>>> +            xen_hvm_evtchn_upcall_vector_t op = {
>>>>>>>>> +                    .vector = HYPERVISOR_CALLBACK_VECTOR,
>>>>>>>>> +                    .vcpu = per_cpu(xen_vcpu_id, cpu),
>>>>>>>>> +            };
>>>>>>>>> +
>>>>>>>>> +
>>>>>>>>> BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector,
>>>>>>>>> +                         &op));
>>>>>>>>> +            /* Trick toolstack to think we are enlightened. */
>>>>>>>>> +            if (!cpu)
>>>>>>>>> +                BUG_ON(xen_set_callback_via(1));
>>>>>>>> What are you trying to make the toolstack aware of? That we have 
>>>>>>>> *a*
>>>>>>>> callback (either global or percpu)?
>>>>>>> Yes, specifically for the check in 
>>>>>>> libxl__domain_pvcontrol_available.
>>>>>> And others.
>>>>>>
>>>>>> This is all a giant bodge, but basically a lot of tooling uses the
>>>>>> non-zero-ness of the CALLBACK_VIA param to determine whether the 
>>>>>> VM has
>>>>>> Xen-aware drivers loaded or not.
>>>>>>
>>>>>> The value 1 is a CALLBACK_VIA value which encodes GSI 1, and the only
>>>>>> reason this doesn't explode everywhere is because the
>>>>>> evtchn_upcall_vector registration takes priority over GSI delivery.
>>>>>>
>>>>>> This is decades of tech debt piled on top of tech debt.
>>>>> Feels like it (setting the callback parameter) is something that the
>>>>> hypervisor should do --- no need to expose guests to this.
>>>> Sensible or not, it is the ABI.
>>>>
>>>> Linux still needs to work (nicely) with older Xen's in the world, 
>>>> and we
>>>> can't just retrofit a change in the hypervisor which says "btw, this 
>>>> ABI
>>>> we've just changed now has a side effect of modifying a field that you
>>>> also logically own".
>>>
>>> The hypercall has been around for a while so I understand ABI concerns
>>> there but XEN_HVM_CPUID_UPCALL_VECTOR was introduced only a month ago.
>>> Why not tie presence of this bit to no longer having to explicitly set
>>> the callback field?
>>>
>> Any other opinions on this?
>>
>> (i.e., calling xen_set_callback_via(1) after
>> HVMOP_set_evtchn_upcall_vector OR not exposing this to guests and
>> instead having Xen call this function (in hvmop_set_evtchn_upcall_vector
>> maybe) and tieing its presense to XEN_HVM_CPUID_UPCALL_VECTOR which was
>> recently added)
> 
> 
> CPUID won't help here, I wasn't thinking clearly.
> 
> 
> Can we wrap the HVMOP_set_evtchn_upcall_vector hypercall in a function 
> that will decide whether or not to also do xen_set_callback_via(1)?
> 
Okay. Will do this in a v2.

Thanks,

Jane.