[v5] Support device passthrough when dom0 is PVH on Xen

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

Posted by Jiqian Chen 2 years ago

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@amd.com>
Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
---
 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) )
+        {
+            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) )
+        {
+            rcu_unlock_domain(d);
+            return -EOPNOTSUPP;
+        }
+        rcu_unlock_domain(d);
+
         ret = physdev_unmap_pirq(unmap.domid, unmap.pirq);
         break;
     }
-- 
2.34.1

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

Posted by Stefano Stabellini 1 year, 11 months ago

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@amd.com>
> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> ---
>  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
>

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

Posted by Chen, Jiqian 1 year, 11 months ago

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@amd.com>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>> ---
>>  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.

> 
> 
>> +        {
>> +            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
>>

-- 
Best regards,
Jiqian Chen.

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

Posted by Stefano Stabellini 1 year, 11 months ago

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@amd.com>
> >> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> >> ---
> >>  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@kernel.org>

[RFC XEN PATCH v5 1/5] xen/vpci: Clear all vpci status of device
[RFC XEN PATCH v5 2/5] x86/pvh: Allow (un)map_pirq when dom0 is PVH
[RFC XEN PATCH v5 3/5] x86/pvh: Add PHYSDEVOP_setup_gsi for PVH dom0
[RFC XEN PATCH v5 4/5] libxl: Use gsi instead of irq for mapping pirq
[RFC XEN PATCH v5 5/5] domctl: Add XEN_DOMCTL_gsi_permission to grant gsi