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.c...@amd.com> >>> Signed-off-by: Huang Rui <ray.hu...@amd.com> >>> Signed-off-by: Jiqian Chen <jiqian.c...@amd.com> >>> Reviewed-by: Stefano Stabellini <sstabell...@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