| Message ID | 20240112061317.418658-3-Jiqian.Chen@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Support device passthrough when dom0 is PVH on Xen | expand |
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 >
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 >>
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>
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; }
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(+)