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

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Jane Malalane <jane.malalane@citrix.com>
Cc: Xen-devel <xen-devel@lists.xenproject.org>,
	Jan Beulich <jbeulich@suse.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>, Wei Liu <wl@xen.org>
Subject: Re: [PATCH v3] x86/hvm: Widen condition for is_hvm_pv_evtchn_domain() and report fix in CPUID
Date: Tue, 24 May 2022 13:22:34 +0200	[thread overview]
Message-ID: <Yoy/+t6M4JCxzUAk@Air-de-Roger> (raw)
In-Reply-To: <20220518132714.5557-1-jane.malalane@citrix.com>

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.