| Message ID | 20220518132714.5557-1-jane.malalane@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] x86/hvm: Widen condition for is_hvm_pv_evtchn_domain() and report fix in CPUID | expand |
Subject could a little shorter I think: x86/hvm: fix upcall vector usage with PIRQs on event channels On Wed, May 18, 2022 at 02:27:14PM +0100, Jane Malalane wrote: > Have is_hvm_pv_evtchn_domain() return true for vector callbacks for > evtchn delivery set up on a per-vCPU basis via > HVMOP_set_evtchn_upcall_vector. > > is_hvm_pv_evtchn_domain() returning true is a condition for setting up > physical IRQ to event channel mappings. > > Therefore, a CPUID bit is added so that guests know whether the check > in is_hvm_pv_evtchn_domain() will fail when using > HVMOP_set_evtchn_upcall_vector. This matters for guests that route > PIRQs over event channels since is_hvm_pv_evtchn_domain() is a > condition in physdev_map_pirq(). > > The naming of the CPUID bit is quite generic about upcall support > being available. That's done so that the define name doesn't become > overly long like XEN_HVM_CPUID_UPCALL_VECTOR_SUPPORTS_PIRQ or some > such. I think you can drop the "... like XEN_HVM_CPUID_UPCALL_VECTOR_SUPPORTS_PIRQ or some such." That's maybe too informal for a commit message log. > > A guest that doesn't care about physical interrupts routed over event > channels can just test for the availability of the hypercall directly > (HVMOP_set_evtchn_upcall_vector) without checking the CPUID bit. > > Signed-off-by: Jane Malalane <jane.malalane@citrix.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> (I think the above can be fixed on commit if the committer agrees) One thing that worries me is how to differentiate between callbacks setup with HVM_PARAM_CALLBACK_TYPE_VECTOR vs using HVMOP_set_evtchn_upcall_vector in writing. We usually use 'callback vector' to refer to the former and 'upcall vector' to refer to the later. Hope that's clearer enough. Thanks, Roger.
On 18.05.2022 15:27, Jane Malalane wrote: > --- a/xen/arch/x86/include/asm/domain.h > +++ b/xen/arch/x86/include/asm/domain.h > @@ -14,8 +14,14 @@ > > #define has_32bit_shinfo(d) ((d)->arch.has_32bit_shinfo) > > +/* > + * Set to true if either the global vector-type callback or per-vCPU > + * LAPIC vectors are used. Assume all vCPUs will use > + * HVMOP_set_evtchn_upcall_vector as long as the initial vCPU does. > + */ > #define is_hvm_pv_evtchn_domain(d) (is_hvm_domain(d) && \ > - (d)->arch.hvm.irq->callback_via_type == HVMIRQ_callback_vector) > + ((d)->arch.hvm.irq->callback_via_type == HVMIRQ_callback_vector || \ > + (d)->vcpu[0]->arch.hvm.evtchn_upcall_vector)) > #define is_hvm_pv_evtchn_vcpu(v) (is_hvm_pv_evtchn_domain(v->domain)) I continue to think that with the vCPU0 dependency added to is_hvm_pv_evtchn_domain(), is_hvm_pv_evtchn_vcpu() should either be adjusted as well (to check the correct vCPU's field) or be deleted (and the sole caller be replaced). Jan
On Tue, May 24, 2022 at 05:14:12PM +0200, Jan Beulich wrote: > On 18.05.2022 15:27, Jane Malalane wrote: > > --- a/xen/arch/x86/include/asm/domain.h > > +++ b/xen/arch/x86/include/asm/domain.h > > @@ -14,8 +14,14 @@ > > > > #define has_32bit_shinfo(d) ((d)->arch.has_32bit_shinfo) > > > > +/* > > + * Set to true if either the global vector-type callback or per-vCPU > > + * LAPIC vectors are used. Assume all vCPUs will use > > + * HVMOP_set_evtchn_upcall_vector as long as the initial vCPU does. > > + */ > > #define is_hvm_pv_evtchn_domain(d) (is_hvm_domain(d) && \ > > - (d)->arch.hvm.irq->callback_via_type == HVMIRQ_callback_vector) > > + ((d)->arch.hvm.irq->callback_via_type == HVMIRQ_callback_vector || \ > > + (d)->vcpu[0]->arch.hvm.evtchn_upcall_vector)) > > #define is_hvm_pv_evtchn_vcpu(v) (is_hvm_pv_evtchn_domain(v->domain)) > > I continue to think that with the vCPU0 dependency added to > is_hvm_pv_evtchn_domain(), is_hvm_pv_evtchn_vcpu() should either > be adjusted as well (to check the correct vCPU's field) or be > deleted (and the sole caller be replaced). I would be fine with replacing, the sole caller of is_hvm_pv_evtchn_vcpu(v) is never reached if the upcall vector is in use. Thanks, Roger.
On 24/05/2022 16:14, Jan Beulich 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 18.05.2022 15:27, Jane Malalane wrote: >> --- a/xen/arch/x86/include/asm/domain.h >> +++ b/xen/arch/x86/include/asm/domain.h >> @@ -14,8 +14,14 @@ >> >> #define has_32bit_shinfo(d) ((d)->arch.has_32bit_shinfo) >> >> +/* >> + * Set to true if either the global vector-type callback or per-vCPU >> + * LAPIC vectors are used. Assume all vCPUs will use >> + * HVMOP_set_evtchn_upcall_vector as long as the initial vCPU does. >> + */ >> #define is_hvm_pv_evtchn_domain(d) (is_hvm_domain(d) && \ >> - (d)->arch.hvm.irq->callback_via_type == HVMIRQ_callback_vector) >> + ((d)->arch.hvm.irq->callback_via_type == HVMIRQ_callback_vector || \ >> + (d)->vcpu[0]->arch.hvm.evtchn_upcall_vector)) >> #define is_hvm_pv_evtchn_vcpu(v) (is_hvm_pv_evtchn_domain(v->domain)) > > I continue to think that with the vCPU0 dependency added to > is_hvm_pv_evtchn_domain(), is_hvm_pv_evtchn_vcpu() should either > be adjusted as well (to check the correct vCPU's field) or be > deleted (and the sole caller be replaced). > > Jan I will replace it in a newer version of the patch. Thank you. Jane.
diff --git a/xen/arch/x86/include/asm/domain.h b/xen/arch/x86/include/asm/domain.h index 35898d725f..f044e0a492 100644 --- a/xen/arch/x86/include/asm/domain.h +++ b/xen/arch/x86/include/asm/domain.h @@ -14,8 +14,14 @@ #define has_32bit_shinfo(d) ((d)->arch.has_32bit_shinfo) +/* + * Set to true if either the global vector-type callback or per-vCPU + * LAPIC vectors are used. Assume all vCPUs will use + * HVMOP_set_evtchn_upcall_vector as long as the initial vCPU does. + */ #define is_hvm_pv_evtchn_domain(d) (is_hvm_domain(d) && \ - (d)->arch.hvm.irq->callback_via_type == HVMIRQ_callback_vector) + ((d)->arch.hvm.irq->callback_via_type == HVMIRQ_callback_vector || \ + (d)->vcpu[0]->arch.hvm.evtchn_upcall_vector)) #define is_hvm_pv_evtchn_vcpu(v) (is_hvm_pv_evtchn_domain(v->domain)) #define is_domain_direct_mapped(d) ((void)(d), 0) diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c index 25bffe47d7..1a7f9df067 100644 --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -1152,6 +1152,12 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf, res->a |= XEN_HVM_CPUID_DOMID_PRESENT; res->c = d->domain_id; + /* + * Per-vCPU event channel upcalls are implemented and work + * correctly with PIRQs routed over event channels. + */ + res->a |= XEN_HVM_CPUID_UPCALL_VECTOR; + break; case 5: /* PV-specific parameters */ diff --git a/xen/include/public/arch-x86/cpuid.h b/xen/include/public/arch-x86/cpuid.h index f2b2b3632c..c49eefeaf8 100644 --- a/xen/include/public/arch-x86/cpuid.h +++ b/xen/include/public/arch-x86/cpuid.h @@ -109,6 +109,11 @@ * 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 work correctly with physical IRQs + * bound to event channels. + */ +#define XEN_HVM_CPUID_UPCALL_VECTOR (1u << 6) /* * Leaf 6 (0x40000x05)
Have is_hvm_pv_evtchn_domain() return true for vector callbacks for evtchn delivery set up on a per-vCPU basis via HVMOP_set_evtchn_upcall_vector. is_hvm_pv_evtchn_domain() returning true is a condition for setting up physical IRQ to event channel mappings. Therefore, a CPUID bit is added so that guests know whether the check in is_hvm_pv_evtchn_domain() will fail when using HVMOP_set_evtchn_upcall_vector. This matters for guests that route PIRQs over event channels since is_hvm_pv_evtchn_domain() is a condition in physdev_map_pirq(). The naming of the CPUID bit is quite generic about upcall support being available. That's done so that the define name doesn't become overly long like XEN_HVM_CPUID_UPCALL_VECTOR_SUPPORTS_PIRQ or some such. A guest that doesn't care about physical interrupts routed over event channels can just test for the availability of the hypercall directly (HVMOP_set_evtchn_upcall_vector) without checking the CPUID bit. Signed-off-by: Jane Malalane <jane.malalane@citrix.com> --- CC: Jan Beulich <jbeulich@suse.com> CC: Andrew Cooper <andrew.cooper3@citrix.com> CC: "Roger Pau Monné" <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> v3: * Improve commit message and title. v2: * Since the naming of the CPUID bit is quite generic, better explain when it should be checked for, in code comments and commit message. --- xen/arch/x86/include/asm/domain.h | 8 +++++++- xen/arch/x86/traps.c | 6 ++++++ xen/include/public/arch-x86/cpuid.h | 5 +++++ 3 files changed, 18 insertions(+), 1 deletion(-)