| Message ID | 20240617090035.839640-3-Jiqian.Chen@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support device passthrough when dom0 is PVH on Xen | expand |
On 17.06.2024 11:00, 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 failed path to unmap pirq. Why "failed path"? Isn't unmapping also part of normal device removal from a guest? > And > add a new check to prevent self map when subject domain has no > PIRQ flag. You still talk of only self mapping, and the code also still does only that. As pointed out before: Why would you allow mapping into a PVH DomU? IOW what purpose do the "d == currd" checks have? > So that domU with PIRQ flag can success to map pirq for > passthrough devices even dom0 has no PIRQ flag. There's still a description problem here. Much like the first sentence, this last one also says that the guest would itself map the pIRQ. In which case there would still not be any reason to expose the sub- functions to Dom0. Jan
On 2024/6/17 22:45, Jan Beulich wrote: > On 17.06.2024 11:00, 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 failed path to unmap pirq. > > Why "failed path"? Isn't unmapping also part of normal device removal > from a guest? Yes, both. I will change to also "allow PHYSDEVOP_unmap_pirq for the device removal path to unmap pirq". > >> And >> add a new check to prevent self map when subject domain has no >> PIRQ flag. > > You still talk of only self mapping, and the code also still does only > that. As pointed out before: Why would you allow mapping into a PVH > DomU? IOW what purpose do the "d == currd" checks have? The checking I added has two purpose, first is I need to allow this case: Dom0(without PIRQ) + DomU(with PIRQ), because the original code just do (!has_pirq(currd)) will cause map_pirq fail in this case. Second I need to disallow self-mapping: DomU(without PIRQ) do map_pirq, the "d==currd" means the currd is the subject domain itself. Emmm, I think I know what's your concern. Do you mean I need to " Prevent map_pirq when currd has no X86_EMU_USE_PIRQ flag " instead of " Prevent self-map when currd has no X86_EMU_USE_PIRQ flag ", so I need to remove "d==currd", right? > >> So that domU with PIRQ flag can success to map pirq for >> passthrough devices even dom0 has no PIRQ flag. > > There's still a description problem here. Much like the first sentence, > this last one also says that the guest would itself map the pIRQ. In > which case there would still not be any reason to expose the sub- > functions to Dom0. If change to " So that the interrupt of a passthrough device can success to be mapped to pirq for domU with PIRQ flag when dom0 is PVH.", Is it OK? > > Jan
On 18.06.2024 08:49, Chen, Jiqian wrote: > On 2024/6/17 22:45, Jan Beulich wrote: >> On 17.06.2024 11:00, 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 failed path to unmap pirq. >> >> Why "failed path"? Isn't unmapping also part of normal device removal >> from a guest? > Yes, both. I will change to also "allow PHYSDEVOP_unmap_pirq for the device removal path to unmap pirq". > >> >>> And >>> add a new check to prevent self map when subject domain has no >>> PIRQ flag. >> >> You still talk of only self mapping, and the code also still does only >> that. As pointed out before: Why would you allow mapping into a PVH >> DomU? IOW what purpose do the "d == currd" checks have? > The checking I added has two purpose, first is I need to allow this case: > Dom0(without PIRQ) + DomU(with PIRQ), because the original code just do (!has_pirq(currd)) will cause map_pirq fail in this case. > Second I need to disallow self-mapping: > DomU(without PIRQ) do map_pirq, the "d==currd" means the currd is the subject domain itself. > > Emmm, I think I know what's your concern. > Do you mean I need to > " Prevent map_pirq when currd has no X86_EMU_USE_PIRQ flag " > instead of > " Prevent self-map when currd has no X86_EMU_USE_PIRQ flag ", No. What I mean is that I continue to fail to see why you mention "currd". IOW it would be more like "prevent mapping when the subject domain has no X86_EMU_USE_PIRQ" (which, as a specific sub-case, includes self-mapping if the caller specifies DOMID_SELF for the subject domain). > so I need to remove "d==currd", right? Removing this check is what I'm after, yes. Yet that's not in sync with either of the two quoted sentences above. >>> So that domU with PIRQ flag can success to map pirq for >>> passthrough devices even dom0 has no PIRQ flag. >> >> There's still a description problem here. Much like the first sentence, >> this last one also says that the guest would itself map the pIRQ. In >> which case there would still not be any reason to expose the sub- >> functions to Dom0. > If change to " So that the interrupt of a passthrough device can success to be mapped to pirq for domU with PIRQ flag when dom0 is PVH.", > Is it OK? Kind of, yes. "can be successfully mapped" is one of the various possibilities of making this read a little more smoothly. Jan
On 2024/6/18 16:38, Jan Beulich wrote: > On 18.06.2024 08:49, Chen, Jiqian wrote: >> On 2024/6/17 22:45, Jan Beulich wrote: >>> On 17.06.2024 11:00, 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 failed path to unmap pirq. >>> >>> Why "failed path"? Isn't unmapping also part of normal device removal >>> from a guest? >> Yes, both. I will change to also "allow PHYSDEVOP_unmap_pirq for the device removal path to unmap pirq". >> >>> >>>> And >>>> add a new check to prevent self map when subject domain has no >>>> PIRQ flag. >>> >>> You still talk of only self mapping, and the code also still does only >>> that. As pointed out before: Why would you allow mapping into a PVH >>> DomU? IOW what purpose do the "d == currd" checks have? >> The checking I added has two purpose, first is I need to allow this case: >> Dom0(without PIRQ) + DomU(with PIRQ), because the original code just do (!has_pirq(currd)) will cause map_pirq fail in this case. >> Second I need to disallow self-mapping: >> DomU(without PIRQ) do map_pirq, the "d==currd" means the currd is the subject domain itself. >> >> Emmm, I think I know what's your concern. >> Do you mean I need to >> " Prevent map_pirq when currd has no X86_EMU_USE_PIRQ flag " >> instead of >> " Prevent self-map when currd has no X86_EMU_USE_PIRQ flag ", > > No. What I mean is that I continue to fail to see why you mention "currd". > IOW it would be more like "prevent mapping when the subject domain has no > X86_EMU_USE_PIRQ" (which, as a specific sub-case, includes self-mapping > if the caller specifies DOMID_SELF for the subject domain). Oh, I see, not only to prevent self-mapping, but if the subject domain has no PIRQs, we should reject, self-mapping is just the one sub case. > >> so I need to remove "d==currd", right? > > Removing this check is what I'm after, yes. Yet that's not in sync with > either of the two quoted sentences above. > >>>> So that domU with PIRQ flag can success to map pirq for >>>> passthrough devices even dom0 has no PIRQ flag. >>> >>> There's still a description problem here. Much like the first sentence, >>> this last one also says that the guest would itself map the pIRQ. In >>> which case there would still not be any reason to expose the sub- >>> functions to Dom0. >> If change to " So that the interrupt of a passthrough device can success to be mapped to pirq for domU with PIRQ flag when dom0 is PVH.", >> Is it OK? > > Kind of, yes. "can be successfully mapped" is one of the various possibilities > of making this read a little more smoothly. OK. > > 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..f38cc22c872e 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 self-map when currd has no X86_EMU_USE_PIRQ flag */ + if ( is_hvm_domain(d) && !has_pirq(d) && d == currd ) + { + 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 self-unmap when currd has no X86_EMU_USE_PIRQ flag */ + if ( is_hvm_domain(d) && !has_pirq(d) && d == currd ) + { + rcu_unlock_domain(d); + return -EOPNOTSUPP; + } + ret = physdev_unmap_pirq(d, unmap.pirq); rcu_unlock_domain(d);