Re: [RFC XEN PATCH v5 2/5] x86/pvh: Allow (un)map_pirq when dom0 is PVH
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
> >> Signed-off-by: Jiqian Chen
> >> ---
> >> 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(, 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
Re: [RFC XEN PATCH v5 2/5] x86/pvh: Allow (un)map_pirq when dom0 is PVH
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
>> Signed-off-by: Jiqian Chen
>> ---
>> 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(, 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(, 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
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
> Signed-off-by: Jiqian Chen
> ---
> 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(, 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(, 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
>
[RFC XEN PATCH v5 2/5] x86/pvh: Allow (un)map_pirq when dom0 is PVH
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
Signed-off-by: Jiqian Chen
---
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(, 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(, 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
