Re: [RFC XEN PATCH v5 2/5] x86/pvh: Allow (un)map_pirq when dom0 is PVH

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 23 Feb 2024, Chen, Jiqian wrote:
> On 2024/2/23 08:40, Stefano Stabellini wrote:
> > On Fri, 12 Jan 2024, Jiqian Chen wrote:
> >> If run Xen with PVH dom0 and hvm domU, hvm will map a pirq for
> >> a passthrough device by using gsi, see
> >> xen_pt_realize->xc_physdev_map_pirq and
> >> pci_add_dm_done->xc_physdev_map_pirq. Then xc_physdev_map_pirq
> >> will call into Xen, but in hvm_physdev_op, PHYSDEVOP_map_pirq
> >> is not allowed because currd is PVH dom0 and PVH has no
> >> X86_EMU_USE_PIRQ flag, it will fail at has_pirq check.
> >>
> >> So, allow PHYSDEVOP_map_pirq when dom0 is PVH and also allow
> >> PHYSDEVOP_unmap_pirq for the failed path to unmap pirq. And
> >> add a new check to prevent self map when caller has no PIRQ
> >> flag.
> >>
> >> Co-developed-by: Huang Rui <ray.huang@xxxxxxx>
> >> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
> >> ---
> >>  xen/arch/x86/hvm/hypercall.c |  2 ++
> >>  xen/arch/x86/physdev.c       | 22 ++++++++++++++++++++++
> >>  2 files changed, 24 insertions(+)
> >>
> >> diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
> >> index 6ad5b4d5f11f..493998b42ec5 100644
> >> --- a/xen/arch/x86/hvm/hypercall.c
> >> +++ b/xen/arch/x86/hvm/hypercall.c
> >> @@ -74,6 +74,8 @@ long hvm_physdev_op(int cmd, 
> >> XEN_GUEST_HANDLE_PARAM(void) arg)
> >>      {
> >>      case PHYSDEVOP_map_pirq:
> >>      case PHYSDEVOP_unmap_pirq:
> >> +        break;
> >> +
> >>      case PHYSDEVOP_eoi:
> >>      case PHYSDEVOP_irq_status_query:
> >>      case PHYSDEVOP_get_free_pirq:
> >> diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
> >> index 47c4da0af7e1..7f2422c2a483 100644
> >> --- a/xen/arch/x86/physdev.c
> >> +++ b/xen/arch/x86/physdev.c
> >> @@ -303,11 +303,22 @@ ret_t do_physdev_op(int cmd, 
> >> XEN_GUEST_HANDLE_PARAM(void) arg)
> >>      case PHYSDEVOP_map_pirq: {
> >>          physdev_map_pirq_t map;
> >>          struct msi_info msi;
> >> +        struct domain *d;
> >>  
> >>          ret = -EFAULT;
> >>          if ( copy_from_guest(&map, arg, 1) != 0 )
> >>              break;
> >>  
> >> +        d = rcu_lock_domain_by_any_id(map.domid);
> >> +        if ( d == NULL )
> >> +            return -ESRCH;
> >> +        if ( !is_pv_domain(d) && !has_pirq(d) )
> > 
> > I think this could just be:
> > 
> >     if ( !has_pirq(d) )
> > 
> > Right?
> No. In the beginning, I only set this condition here, but it destroyed PV 
> dom0.
> Because  PV has no pirq flag too, it can match this condition and return 
> -EOPNOTSUPP, PHYSDEVOP_map_pirq will fail.

Yes I get it now. Thanks for the explanation. I think "has_pirq" is
misnamed and should probably be hvm_has_pirq or something like that.

I am not asking to fix it, but maybe an in-code comment here would help,
like:

/* if it is an HVM guest, check if it has PIRQs */

in any case:

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>