| Message ID | 20240630123344.20623-3-Jiqian.Chen@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Support device passthrough when dom0 is PVH on Xen | expand |
On 30.06.2024 14:33, 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 qemu code > xen_pt_realize->xc_physdev_map_pirq and libxl code > 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 removal device path to unmap pirq. > And add a new check to prevent (un)map when the subject domain > has no X86_EMU_USE_PIRQ flag. > > So that the interrupt of a passthrough device can be > successfully mapped to pirq for domU with X86_EMU_USE_PIRQ flag > when dom0 is PVH > > Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> > Signed-off-by: Huang Rui <ray.huang@amd.com> > Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> You keep carrying this R-b, despite making functional changes. This can't be quite right. While functionally I'm now okay with the change, I still have a code structure concern: > --- a/xen/arch/x86/physdev.c > +++ b/xen/arch/x86/physdev.c > @@ -323,6 +323,13 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > if ( !d ) > break; > > + /* Prevent mapping when the subject domain has no X86_EMU_USE_PIRQ */ > + if ( is_hvm_domain(d) && !has_pirq(d) ) > + { > + rcu_unlock_domain(d); > + return -EOPNOTSUPP; > + } > + > ret = physdev_map_pirq(d, map.type, &map.index, &map.pirq, &msi); > > rcu_unlock_domain(d); > @@ -346,6 +353,13 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > if ( !d ) > break; > > + /* Prevent unmapping when the subject domain has no X86_EMU_USE_PIRQ */ > + if ( is_hvm_domain(d) && !has_pirq(d) ) > + { > + rcu_unlock_domain(d); > + return -EOPNOTSUPP; > + } > + > ret = physdev_unmap_pirq(d, unmap.pirq); > > rcu_unlock_domain(d); If you did go look, you will have noticed that we use "return" in the middle of this function only very sparingly (when alternatives would result in more complicated code elsewhere). I think you want to avoid "return" here, too, and probably go even further and avoid the extra rcu_unlock_domain() as well. That's easily possible to arrange for (taking the latter case as example): /* Prevent unmapping when the subject domain has no X86_EMU_USE_PIRQ */ if ( !is_hvm_domain(d) || has_pirq(d) ) ret = physdev_unmap_pirq(d, unmap.pirq); else ret = -EOPNOTSUPP; rcu_unlock_domain(d); Personally I would even use a conditional operator here, but I believe others might dislike its use in situations like this one. The re-arrangement make a little more noticeable though that the comment isn't quite right either: PV domains necessarily have no X86_EMU_USE_PIRQ. Maybe "... has no notion of pIRQ"? Jan
On 2024/7/1 15:44, Jan Beulich wrote: > On 30.06.2024 14:33, 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 qemu code >> xen_pt_realize->xc_physdev_map_pirq and libxl code >> 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 removal device path to unmap pirq. >> And add a new check to prevent (un)map when the subject domain >> has no X86_EMU_USE_PIRQ flag. >> >> So that the interrupt of a passthrough device can be >> successfully mapped to pirq for domU with X86_EMU_USE_PIRQ flag >> when dom0 is PVH >> >> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> >> Signed-off-by: Huang Rui <ray.huang@amd.com> >> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> >> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > > You keep carrying this R-b, despite making functional changes. This can't be > quite right. Will remove in next version. > > While functionally I'm now okay with the change, I still have a code structure > concern: > >> --- a/xen/arch/x86/physdev.c >> +++ b/xen/arch/x86/physdev.c >> @@ -323,6 +323,13 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >> if ( !d ) >> break; >> >> + /* Prevent mapping when the subject domain has no X86_EMU_USE_PIRQ */ >> + if ( is_hvm_domain(d) && !has_pirq(d) ) >> + { >> + rcu_unlock_domain(d); >> + return -EOPNOTSUPP; >> + } >> + >> ret = physdev_map_pirq(d, map.type, &map.index, &map.pirq, &msi); >> >> rcu_unlock_domain(d); >> @@ -346,6 +353,13 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >> if ( !d ) >> break; >> >> + /* Prevent unmapping when the subject domain has no X86_EMU_USE_PIRQ */ >> + if ( is_hvm_domain(d) && !has_pirq(d) ) >> + { >> + rcu_unlock_domain(d); >> + return -EOPNOTSUPP; >> + } >> + >> ret = physdev_unmap_pirq(d, unmap.pirq); >> >> rcu_unlock_domain(d); > > If you did go look, you will have noticed that we use "return" in the middle > of this function only very sparingly (when alternatives would result in more > complicated code elsewhere). I think you want to avoid "return" here, too, > and probably go even further and avoid the extra rcu_unlock_domain() as well. > That's easily possible to arrange for (taking the latter case as example): > > /* Prevent unmapping when the subject domain has no X86_EMU_USE_PIRQ */ > if ( !is_hvm_domain(d) || has_pirq(d) ) > ret = physdev_unmap_pirq(d, unmap.pirq); > else > ret = -EOPNOTSUPP; > > rcu_unlock_domain(d); > > Personally I would even use a conditional operator here, but I believe > others might dislike its use in situations like this one. > > The re-arrangement make a little more noticeable though that the comment > isn't quite right either: PV domains necessarily have no > X86_EMU_USE_PIRQ. Maybe "... has no notion of pIRQ"? Or just like below? /* * Prevent unmapping when the subject hvm domain has no * X86_EMU_USE_PIRQ */ if ( is_hvm_domain(d) && !has_pirq(d) ) ret = -EOPNOTSUPP; else ret = physdev_unmap_pirq(d, unmap.pirq); > > Jan
On 02.07.2024 05:15, Chen, Jiqian wrote: > On 2024/7/1 15:44, Jan Beulich wrote: >> On 30.06.2024 14:33, 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 qemu code >>> xen_pt_realize->xc_physdev_map_pirq and libxl code >>> 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 removal device path to unmap pirq. >>> And add a new check to prevent (un)map when the subject domain >>> has no X86_EMU_USE_PIRQ flag. >>> >>> So that the interrupt of a passthrough device can be >>> successfully mapped to pirq for domU with X86_EMU_USE_PIRQ flag >>> when dom0 is PVH >>> >>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> >>> Signed-off-by: Huang Rui <ray.huang@amd.com> >>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> >>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> >> >> You keep carrying this R-b, despite making functional changes. This can't be >> quite right. > Will remove in next version. > >> >> While functionally I'm now okay with the change, I still have a code structure >> concern: >> >>> --- a/xen/arch/x86/physdev.c >>> +++ b/xen/arch/x86/physdev.c >>> @@ -323,6 +323,13 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >>> if ( !d ) >>> break; >>> >>> + /* Prevent mapping when the subject domain has no X86_EMU_USE_PIRQ */ >>> + if ( is_hvm_domain(d) && !has_pirq(d) ) >>> + { >>> + rcu_unlock_domain(d); >>> + return -EOPNOTSUPP; >>> + } >>> + >>> ret = physdev_map_pirq(d, map.type, &map.index, &map.pirq, &msi); >>> >>> rcu_unlock_domain(d); >>> @@ -346,6 +353,13 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >>> if ( !d ) >>> break; >>> >>> + /* Prevent unmapping when the subject domain has no X86_EMU_USE_PIRQ */ >>> + if ( is_hvm_domain(d) && !has_pirq(d) ) >>> + { >>> + rcu_unlock_domain(d); >>> + return -EOPNOTSUPP; >>> + } >>> + >>> ret = physdev_unmap_pirq(d, unmap.pirq); >>> >>> rcu_unlock_domain(d); >> >> If you did go look, you will have noticed that we use "return" in the middle >> of this function only very sparingly (when alternatives would result in more >> complicated code elsewhere). I think you want to avoid "return" here, too, >> and probably go even further and avoid the extra rcu_unlock_domain() as well. >> That's easily possible to arrange for (taking the latter case as example): >> >> /* Prevent unmapping when the subject domain has no X86_EMU_USE_PIRQ */ >> if ( !is_hvm_domain(d) || has_pirq(d) ) >> ret = physdev_unmap_pirq(d, unmap.pirq); >> else >> ret = -EOPNOTSUPP; >> >> rcu_unlock_domain(d); >> >> Personally I would even use a conditional operator here, but I believe >> others might dislike its use in situations like this one. >> >> The re-arrangement make a little more noticeable though that the comment >> isn't quite right either: PV domains necessarily have no >> X86_EMU_USE_PIRQ. Maybe "... has no notion of pIRQ"? > > Or just like below? > > /* > * Prevent unmapping when the subject hvm domain has no > * X86_EMU_USE_PIRQ > */ > if ( is_hvm_domain(d) && !has_pirq(d) ) > ret = -EOPNOTSUPP; > else > ret = physdev_unmap_pirq(d, unmap.pirq); No objection to the slightly changed comment. The code alternative you present is of course functionally identical, yet personally I prefer to have the "good" case on the "if" branch and the "bad" one following "else". I wouldn't insist, though. Jan
diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c index 0fab670a4871..03ada3c880bd 100644 --- a/xen/arch/x86/hvm/hypercall.c +++ b/xen/arch/x86/hvm/hypercall.c @@ -71,8 +71,14 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) switch ( cmd ) { + /* + * Only being permitted for management of other domains. + * Further restrictions are enforced in do_physdev_op. + */ 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 d6dd622952a9..a165f68225c1 100644 --- a/xen/arch/x86/physdev.c +++ b/xen/arch/x86/physdev.c @@ -323,6 +323,13 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) if ( !d ) break; + /* Prevent mapping when the subject domain has no X86_EMU_USE_PIRQ */ + if ( is_hvm_domain(d) && !has_pirq(d) ) + { + rcu_unlock_domain(d); + return -EOPNOTSUPP; + } + ret = physdev_map_pirq(d, map.type, &map.index, &map.pirq, &msi); rcu_unlock_domain(d); @@ -346,6 +353,13 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) if ( !d ) break; + /* Prevent unmapping when the subject domain has no X86_EMU_USE_PIRQ */ + if ( is_hvm_domain(d) && !has_pirq(d) ) + { + rcu_unlock_domain(d); + return -EOPNOTSUPP; + } + ret = physdev_unmap_pirq(d, unmap.pirq); rcu_unlock_domain(d);