| Message ID | 20240105070920.350113-3-Jiqian.Chen@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support device passthrough when dom0 is PVH on Xen | expand |
On Fri, 5 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 domid of the caller is not > DOMID_SELF no matter whether currd has X86_EMU_USE_PIRQ flag > and also allow PHYSDEVOP_unmap_pirq for the failed path to > unmap pirq. > > Co-developed-by: Huang Rui <ray.huang@amd.com> > Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> > --- > xen/arch/x86/hvm/hypercall.c | 27 +++++++++++++++++++++++++-- > 1 file changed, 25 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c > index 6ad5b4d5f11f..632a68be3cc4 100644 > --- a/xen/arch/x86/hvm/hypercall.c > +++ b/xen/arch/x86/hvm/hypercall.c > @@ -10,6 +10,7 @@ > #include <xen/hypercall.h> > #include <xen/ioreq.h> > #include <xen/nospec.h> > +#include <xen/guest_access.h> > > #include <asm/hvm/emulate.h> > #include <asm/hvm/support.h> > @@ -72,8 +73,30 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > > switch ( cmd ) > { > - case PHYSDEVOP_map_pirq: > - case PHYSDEVOP_unmap_pirq: > + case PHYSDEVOP_map_pirq: { > + physdev_map_pirq_t map; > + > + if ( copy_from_guest(&map, arg, 1) != 0 ) > + return -EFAULT; > + > + if ( !has_pirq(currd) && map.domid == DOMID_SELF ) > + return -ENOSYS; This looks OK to me although there is already another copy_from_guest in do_physdev_op, but I don't see an easy way to make it better. Also, you could write this check like this: d = rcu_lock_domain_by_any_id(map.domid); if ( d == NULL ) return -ESRCH; if ( !has_pirq(d) ) return -ENOSYS; rcu_unlock_domain(d); This way it is a bit more generic and not special-cased to DOMID_SELF. I'll let the x86 maintainers comment on the way the prefer it. > + break; > + } > + > + case PHYSDEVOP_unmap_pirq: { > + physdev_unmap_pirq_t unmap; > + > + if ( copy_from_guest(&unmap, arg, 1) != 0 ) > + return -EFAULT; > + > + if ( !has_pirq(currd) && unmap.domid == DOMID_SELF ) > + return -ENOSYS; > + > + break; > + } > + > case PHYSDEVOP_eoi: > case PHYSDEVOP_irq_status_query: > case PHYSDEVOP_get_free_pirq: > -- > 2.34.1 >
On 06.01.2024 01:46, Stefano Stabellini wrote: > On Fri, 5 Jan 2024, Jiqian Chen wrote: >> @@ -72,8 +73,30 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >> >> switch ( cmd ) >> { >> - case PHYSDEVOP_map_pirq: >> - case PHYSDEVOP_unmap_pirq: >> + case PHYSDEVOP_map_pirq: { >> + physdev_map_pirq_t map; >> + >> + if ( copy_from_guest(&map, arg, 1) != 0 ) >> + return -EFAULT; >> + >> + if ( !has_pirq(currd) && map.domid == DOMID_SELF ) >> + return -ENOSYS; > > This looks OK to me although there is already another copy_from_guest in > do_physdev_op, but I don't see an easy way to make it better. How can double reads of hypercall args ever be okay? The new check clearly needs to be inserted in the code path where the structure is being read already anyway. Jan
On 2024/1/8 16:47, Jan Beulich wrote: > On 06.01.2024 01:46, Stefano Stabellini wrote: >> On Fri, 5 Jan 2024, Jiqian Chen wrote: >>> @@ -72,8 +73,30 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >>> >>> switch ( cmd ) >>> { >>> - case PHYSDEVOP_map_pirq: >>> - case PHYSDEVOP_unmap_pirq: >>> + case PHYSDEVOP_map_pirq: { >>> + physdev_map_pirq_t map; >>> + >>> + if ( copy_from_guest(&map, arg, 1) != 0 ) >>> + return -EFAULT; >>> + >>> + if ( !has_pirq(currd) && map.domid == DOMID_SELF ) >>> + return -ENOSYS; >> >> This looks OK to me although there is already another copy_from_guest in >> do_physdev_op, but I don't see an easy way to make it better. > > How can double reads of hypercall args ever be okay? The new check clearly > needs to be inserted in the code path where the structure is being read > already anyway. I also tried to add this check in PHYSDEVOP_map_pirq in physdev.c, but pv has no flag X86_EMU_USE_PIRQ too. If want to add it into physdev.c and combine Stefano's opinions, this check may be like: diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c index 47c4da0af7e1..c38d4d405726 100644 --- a/xen/arch/x86/physdev.c +++ b/xen/arch/x86/physdev.c @@ -303,11 +303,19 @@ 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) ) + return -ENOSYS; + rcu_unlock_domain(d); + switch ( map.type ) { case MAP_PIRQ_TYPE_MSI_SEG: > > Jan
On 08.01.2024 10:15, Chen, Jiqian wrote: > On 2024/1/8 16:47, Jan Beulich wrote: >> On 06.01.2024 01:46, Stefano Stabellini wrote: >>> On Fri, 5 Jan 2024, Jiqian Chen wrote: >>>> @@ -72,8 +73,30 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >>>> >>>> switch ( cmd ) >>>> { >>>> - case PHYSDEVOP_map_pirq: >>>> - case PHYSDEVOP_unmap_pirq: >>>> + case PHYSDEVOP_map_pirq: { >>>> + physdev_map_pirq_t map; >>>> + >>>> + if ( copy_from_guest(&map, arg, 1) != 0 ) >>>> + return -EFAULT; >>>> + >>>> + if ( !has_pirq(currd) && map.domid == DOMID_SELF ) >>>> + return -ENOSYS; >>> >>> This looks OK to me although there is already another copy_from_guest in >>> do_physdev_op, but I don't see an easy way to make it better. >> >> How can double reads of hypercall args ever be okay? The new check clearly >> needs to be inserted in the code path where the structure is being read >> already anyway. > I also tried to add this check in PHYSDEVOP_map_pirq in physdev.c, but pv has no flag X86_EMU_USE_PIRQ too. > If want to add it into physdev.c and combine Stefano's opinions, this check may be like: > > diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c > index 47c4da0af7e1..c38d4d405726 100644 > --- a/xen/arch/x86/physdev.c > +++ b/xen/arch/x86/physdev.c > @@ -303,11 +303,19 @@ 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) ) > + return -ENOSYS; > + rcu_unlock_domain(d); > + > switch ( map.type ) > { > case MAP_PIRQ_TYPE_MSI_SEG: Well, yes, perhaps kind of like that, but with rcu_unlock_domain() called on the error 2nd return path as well, and without abusing ENOSYS. Jan
On 2024/1/8 17:25, Jan Beulich wrote: > On 08.01.2024 10:15, Chen, Jiqian wrote: >> On 2024/1/8 16:47, Jan Beulich wrote: >>> On 06.01.2024 01:46, Stefano Stabellini wrote: >>>> On Fri, 5 Jan 2024, Jiqian Chen wrote: >>>>> @@ -72,8 +73,30 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >>>>> >>>>> switch ( cmd ) >>>>> { >>>>> - case PHYSDEVOP_map_pirq: >>>>> - case PHYSDEVOP_unmap_pirq: >>>>> + case PHYSDEVOP_map_pirq: { >>>>> + physdev_map_pirq_t map; >>>>> + >>>>> + if ( copy_from_guest(&map, arg, 1) != 0 ) >>>>> + return -EFAULT; >>>>> + >>>>> + if ( !has_pirq(currd) && map.domid == DOMID_SELF ) >>>>> + return -ENOSYS; >>>> >>>> This looks OK to me although there is already another copy_from_guest in >>>> do_physdev_op, but I don't see an easy way to make it better. >>> >>> How can double reads of hypercall args ever be okay? The new check clearly >>> needs to be inserted in the code path where the structure is being read >>> already anyway. >> I also tried to add this check in PHYSDEVOP_map_pirq in physdev.c, but pv has no flag X86_EMU_USE_PIRQ too. >> If want to add it into physdev.c and combine Stefano's opinions, this check may be like: >> >> diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c >> index 47c4da0af7e1..c38d4d405726 100644 >> --- a/xen/arch/x86/physdev.c >> +++ b/xen/arch/x86/physdev.c >> @@ -303,11 +303,19 @@ 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) ) >> + return -ENOSYS; >> + rcu_unlock_domain(d); >> + >> switch ( map.type ) >> { >> case MAP_PIRQ_TYPE_MSI_SEG: > > Well, yes, perhaps kind of like that, but with rcu_unlock_domain() called > on the error 2nd return path as well, and without abusing ENOSYS. Ok, will call rcu_unlock_domain on 2nd error path, and change ENOSYS to EOPNOTSUPP. > > Jan
diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c index 6ad5b4d5f11f..632a68be3cc4 100644 --- a/xen/arch/x86/hvm/hypercall.c +++ b/xen/arch/x86/hvm/hypercall.c @@ -10,6 +10,7 @@ #include <xen/hypercall.h> #include <xen/ioreq.h> #include <xen/nospec.h> +#include <xen/guest_access.h> #include <asm/hvm/emulate.h> #include <asm/hvm/support.h> @@ -72,8 +73,30 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) switch ( cmd ) { - case PHYSDEVOP_map_pirq: - case PHYSDEVOP_unmap_pirq: + case PHYSDEVOP_map_pirq: { + physdev_map_pirq_t map; + + if ( copy_from_guest(&map, arg, 1) != 0 ) + return -EFAULT; + + if ( !has_pirq(currd) && map.domid == DOMID_SELF ) + return -ENOSYS; + + break; + } + + case PHYSDEVOP_unmap_pirq: { + physdev_unmap_pirq_t unmap; + + if ( copy_from_guest(&unmap, arg, 1) != 0 ) + return -EFAULT; + + if ( !has_pirq(currd) && unmap.domid == DOMID_SELF ) + return -ENOSYS; + + break; + } + case PHYSDEVOP_eoi: case PHYSDEVOP_irq_status_query: case PHYSDEVOP_get_free_pirq:
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 domid of the caller is not DOMID_SELF no matter whether currd has X86_EMU_USE_PIRQ flag and also allow PHYSDEVOP_unmap_pirq for the failed path to unmap pirq. Co-developed-by: Huang Rui <ray.huang@amd.com> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> --- xen/arch/x86/hvm/hypercall.c | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-)