[PATCH v3] x86/hvm: Widen condition for is_hvm_pv_evtchn_domain() and report fix in CPUID

[PATCH v3] x86/hvm: Widen condition for is_hvm_pv_evtchn_domain() and report fix in CPUID

[Jane Malalane]

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(-)

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)
-- 
2.11.0

Re: [PATCH v3] x86/hvm: Widen condition for is_hvm_pv_evtchn_domain() and report fix in CPUID

[Roger Pau Monné]

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.

Re: [PATCH v3] x86/hvm: Widen condition for is_hvm_pv_evtchn_domain() and report fix in CPUID

[Jan Beulich]

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

Re: [PATCH v3] x86/hvm: Widen condition for is_hvm_pv_evtchn_domain() and report fix in CPUID

[Roger Pau Monné]

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.

Re: [PATCH v3] x86/hvm: Widen condition for is_hvm_pv_evtchn_domain() and report fix in CPUID

[Jane Malalane]

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.