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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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?


> +        {
> +            rcu_unlock_domain(d);
> +            return -EOPNOTSUPP;
> +        }
> +        rcu_unlock_domain(d);
> +
>          switch ( map.type )
>          {
>          case MAP_PIRQ_TYPE_MSI_SEG:
> @@ -341,11 +352,22 @@ ret_t do_physdev_op(int cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>  
>      case PHYSDEVOP_unmap_pirq: {
>          struct physdev_unmap_pirq unmap;
> +        struct domain *d;
>  
>          ret = -EFAULT;
>          if ( copy_from_guest(&unmap, arg, 1) != 0 )
>              break;
>  
> +        d = rcu_lock_domain_by_any_id(unmap.domid);
> +        if ( d == NULL )
> +            return -ESRCH;
> +        if ( !is_pv_domain(d) && !has_pirq(d) )

same here


Other than that, everything looks fine to me


> +        {
> +            rcu_unlock_domain(d);
> +            return -EOPNOTSUPP;
> +        }
> +        rcu_unlock_domain(d);
> +
>          ret = physdev_unmap_pirq(unmap.domid, unmap.pirq);
>          break;
>      }
> -- 
> 2.34.1
>