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(-)
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
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
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.
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
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
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
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
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
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.
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
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.
© 2016 - 2024 Red Hat, Inc.