Re: [PATCH] x86/hvm: Widen condition for is_hvm_pv_evtchn_vcpu()

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, May 11, 2022 at 04:14:23PM +0100, Jane Malalane wrote:
> Have is_hvm_pv_evtchn_vcpu() 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_vcpu() returning true is a condition for setting up
> physical IRQ to event channel mappings.

I would add something like:

The naming of the CPUID bit is a bit generic about upcall support
being available.  That's done so that the define name doesn't get
overly long like XEN_HVM_CPUID_UPCALL_VECTOR_SUPPORTS_PIRQ or some
such.

Guests that don'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@xxxxxxxxxx>
> ---
> CC: Jan Beulich <jbeulich@xxxxxxxx>
> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> CC: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> ---
>  xen/arch/x86/include/asm/domain.h   | 8 +++++++-
>  xen/arch/x86/traps.c                | 3 +++
>  xen/include/public/arch-x86/cpuid.h | 2 ++
>  3 files changed, 12 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

I think you should remove LAPIC here.  There's no such thing as 'LAPIC
vectors', it's just that the old mechanism was bypassing the lapic
EOI.

> + * 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..2c51faab2c 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1152,6 +1152,9 @@ 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. */

... 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..1760e2c405 100644
> --- a/xen/include/public/arch-x86/cpuid.h
> +++ b/xen/include/public/arch-x86/cpuid.h
> @@ -109,6 +109,8 @@
>   * 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. */

I would maybe expand the message to:

"Per-vCPU event channel upcalls work correctly with physical IRQs bound
to event channels."

Thanks, Roger.