Re: [XEN PATCH v12 2/7] x86/pvh: Allow (un)map_pirq when dom0 is PVH

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

On Wed, Jul 31, 2024 at 10:40:46AM +0200, Jan Beulich wrote:
> On 31.07.2024 10:24, Roger Pau Monné wrote:
> > On Wed, Jul 31, 2024 at 09:58:28AM +0200, Jan Beulich wrote:
> >> On 31.07.2024 09:50, Roger Pau Monné wrote:
> >>> On Mon, Jul 08, 2024 at 07:41:19PM +0800, Jiqian Chen wrote:
> >>>> --- a/xen/arch/x86/physdev.c
> >>>> +++ b/xen/arch/x86/physdev.c
> >>>> @@ -323,7 +323,11 @@ ret_t do_physdev_op(int cmd, 
> >>>> XEN_GUEST_HANDLE_PARAM(void) arg)
> >>>>          if ( !d )
> >>>>              break;
> >>>>  
> >>>> -        ret = physdev_map_pirq(d, map.type, &map.index, &map.pirq, 
> >>>> &msi);
> >>>> +        /* Only mapping when the subject domain has a notion of PIRQ */
> >>>> +        if ( !is_hvm_domain(d) || has_pirq(d) )
> >>>
> >>> I'm afraid this is not true.  It's fine to map interrupts to HVM
> >>> domains that don't have XENFEAT_hvm_pirqs enabled.  has_pirq() simply
> >>> allow HVM domains to route interrupts from devices (either emulated or
> >>> passed through) over event channels.
> >>>
> >>> It might have worked in the past (when using a version of Xen < 4.19)
> >>> because XENFEAT_hvm_pirqs was enabled by default for HVM guests.
> >>>
> >>> physdev_map_pirq() will work fine when used against domains that don't
> >>> have XENFEAT_hvm_pirqs enabled, and it needs to be kept this way.
> >>>
> >>> I think you want to allow PHYSDEVOP_{,un}map_pirq for HVM domains, but
> >>> keep the code in do_physdev_op() as-is.  You will have to check
> >>> whether the current paths in do_physdev_op() are not making
> >>> assumptions about XENFEAT_hvm_pirqs being enabled when the calling
> >>> domain is of HVM type.  I don't think that's the case, but better
> >>> check.
> >>
> >> Yet the goal is to disallow mapping into PVH domains. The use of
> >> has_pirq() was aiming at that. If that predicate can't be used (anymore)
> >> for this purpose, which one is appropriate now?
> > 
> > Why do you want to add such restriction now, when it's not currently
> > present?
> > 
> > It was already the case that a PV dom0 could issue
> > PHYSDEVOP_{,un}map_pirq operations against a PVH domU, whatever the
> > result of such operation be.
> 
> Because (a) that was wrong and (b) we'd suddenly permit a PVH DomU to
> issue such for itself.

Regarding (b) a PVH domU issuing such operations would fail at the
xsm_map_domain_pirq() check in physdev_map_pirq().

I agree with (a), but I don't think enabling PVH dom0 usage of the
hypercalls should be gated on this.  As said a PV dom0 is already
capable of issuing PHYSDEVOP_{,un}map_pirq operations against a PVH
domU.

Thanks, Roger.