| Message ID | 20240816110820.75672-3-Jiqian.Chen@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support device passthrough when dom0 is PVH on Xen | expand |
On 16.08.2024 13:08, 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 > iPHYSDEVOP_unmap_pirq for the removal device path to unmap pirq. > So that the interrupt of a passthrough device can be successfully > mapped to pirq for domU with a notion of PIRQ when dom0 is PVH. > > To exposing the functionality to wider than (presently) necessary > audience(like PVH domU), so it doesn't add any futher restrictions. The code change is fine, but I'm struggling with this sentence. I can't really derive what you're trying to say. > And there already are some senarios for domains without > X86_EMU_USE_PIRQ to use these functions. Are there? If so, pointing out an example may help. Jan
On 2024/8/19 17:08, Jan Beulich wrote: > On 16.08.2024 13:08, 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 >> iPHYSDEVOP_unmap_pirq for the removal device path to unmap pirq. >> So that the interrupt of a passthrough device can be successfully >> mapped to pirq for domU with a notion of PIRQ when dom0 is PVH. >> >> To exposing the functionality to wider than (presently) necessary >> audience(like PVH domU), so it doesn't add any futher restrictions. > > The code change is fine, but I'm struggling with this sentence. I can't > really derive what you're trying to say. Ah, I wanted to explain why this path not add any further restrictions, then used your comments of last version. How do I need to change this explanation? > >> And there already are some senarios for domains without >> X86_EMU_USE_PIRQ to use these functions. > > Are there? If so, pointing out an example may help. If I understand correctly, Roger mentioned that PIRQs is disable by default for HVM guest("hvm_pirq=0") and passthrough device to guest. In this scene, guest doesn't have PIRQs, but it still needs this hypercall. > > Jan
On 20.08.2024 08:12, Chen, Jiqian wrote: > On 2024/8/19 17:08, Jan Beulich wrote: >> On 16.08.2024 13:08, 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 >>> iPHYSDEVOP_unmap_pirq for the removal device path to unmap pirq. >>> So that the interrupt of a passthrough device can be successfully >>> mapped to pirq for domU with a notion of PIRQ when dom0 is PVH. >>> >>> To exposing the functionality to wider than (presently) necessary >>> audience(like PVH domU), so it doesn't add any futher restrictions. >> >> The code change is fine, but I'm struggling with this sentence. I can't >> really derive what you're trying to say. > Ah, I wanted to explain why this path not add any further restrictions, then used your comments of last version. > How do I need to change this explanation? I think you want to take Roger's earlier comments (when he requested the relaxation) as basis to re-write (combine) both of the latter two paragraphs above (or maybe even all three of them). It's odd to first talk about Dom0, as if the operations were to be exposed just there, and only then add DomU-s. >>> And there already are some senarios for domains without >>> X86_EMU_USE_PIRQ to use these functions. >> >> Are there? If so, pointing out an example may help. > If I understand correctly, Roger mentioned that PIRQs is disable by default for HVM guest("hvm_pirq=0") and passthrough device to guest. > In this scene, guest doesn't have PIRQs, but it still needs this hypercall. In which case please say so in order to be concrete, not vague. Jan
On 2024/8/20 15:07, Jan Beulich wrote: > On 20.08.2024 08:12, Chen, Jiqian wrote: >> On 2024/8/19 17:08, Jan Beulich wrote: >>> On 16.08.2024 13:08, 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 >>>> iPHYSDEVOP_unmap_pirq for the removal device path to unmap pirq. >>>> So that the interrupt of a passthrough device can be successfully >>>> mapped to pirq for domU with a notion of PIRQ when dom0 is PVH. >>>> >>>> To exposing the functionality to wider than (presently) necessary >>>> audience(like PVH domU), so it doesn't add any futher restrictions. >>> >>> The code change is fine, but I'm struggling with this sentence. I can't >>> really derive what you're trying to say. >> Ah, I wanted to explain why this path not add any further restrictions, then used your comments of last version. >> How do I need to change this explanation? > > I think you want to take Roger's earlier comments (when he requested > the relaxation) as basis to re-write (combine) both of the latter two > paragraphs above (or maybe even all three of them). It's odd to first > talk about Dom0, as if the operations were to be exposed just there, > and only then add DomU-s. I tried to understand and summarize Roger's previous comments and changed commit message to the following. Do you think it is fine? x86/pvh: Allow (un)map_pirq when dom0 is PVH When dom0 is PVH type and passthrough a device to HVM domU, Qemu code xen_pt_realize->xc_physdev_map_pirq and libxl code pci_add_dm_done-> xc_physdev_map_pirq map a pirq for passthrough devices. In xc_physdev_map_pirq call stack, function hvm_physdev_op has a check has_pirq(currd), but currd is PVH dom0, PVH has no X86_EMU_USE_PIRQ flag, so it fails, PHYSDEVOP_map_pirq is not allowed for PVH dom0 in current codes. But it is fine to map interrupts through pirq to a HVM domain whose XENFEAT_hvm_pirqs is not enabled. Because pirq field is used as a way to reference interrupts and it is just the way for the device model to identify which interrupt should be mapped to which domain, however has_pirq() is just to check if HVM domains route interrupts from devices(emulated or passthrough) through event channel, so, the has_pirq() check should not be applied to the PHYSDEVOP_map_pirq issued by dom0. And the PVH domU which use vpci trying to issue a map_pirq will fail at the xsm_map_domain_pirq() check in physdev_map_pirq() . So, allow PHYSDEVOP_map_pirq when dom0 is PVH and also allow PHYSDEVOP_unmap_pirq for the removal device path to unmap pirq. Then the interrupt of a passthrough device can be successfully mapped to pirq for domU. > >>>> And there already are some senarios for domains without >>>> X86_EMU_USE_PIRQ to use these functions. >>> >>> Are there? If so, pointing out an example may help. >> If I understand correctly, Roger mentioned that PIRQs is disable by default for HVM guest("hvm_pirq=0") and passthrough device to guest. >> In this scene, guest doesn't have PIRQs, but it still needs this hypercall. > > In which case please say so in order to be concrete, not vague. > > Jan
On 03.09.2024 06:01, Chen, Jiqian wrote: > On 2024/8/20 15:07, Jan Beulich wrote: >> On 20.08.2024 08:12, Chen, Jiqian wrote: >>> On 2024/8/19 17:08, Jan Beulich wrote: >>>> On 16.08.2024 13:08, 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 >>>>> iPHYSDEVOP_unmap_pirq for the removal device path to unmap pirq. >>>>> So that the interrupt of a passthrough device can be successfully >>>>> mapped to pirq for domU with a notion of PIRQ when dom0 is PVH. >>>>> >>>>> To exposing the functionality to wider than (presently) necessary >>>>> audience(like PVH domU), so it doesn't add any futher restrictions. >>>> >>>> The code change is fine, but I'm struggling with this sentence. I can't >>>> really derive what you're trying to say. >>> Ah, I wanted to explain why this path not add any further restrictions, then used your comments of last version. >>> How do I need to change this explanation? >> >> I think you want to take Roger's earlier comments (when he requested >> the relaxation) as basis to re-write (combine) both of the latter two >> paragraphs above (or maybe even all three of them). It's odd to first >> talk about Dom0, as if the operations were to be exposed just there, >> and only then add DomU-s. > > I tried to understand and summarize Roger's previous comments and changed commit message to the following. Do you think it is fine? What are we talking about here? The patch was committed over a month ago? Jan > x86/pvh: Allow (un)map_pirq when dom0 is PVH > > When dom0 is PVH type and passthrough a device to HVM domU, Qemu code > xen_pt_realize->xc_physdev_map_pirq and libxl code pci_add_dm_done-> > xc_physdev_map_pirq map a pirq for passthrough devices. > In xc_physdev_map_pirq call stack, function hvm_physdev_op has a check > has_pirq(currd), but currd is PVH dom0, PVH has no X86_EMU_USE_PIRQ flag, > so it fails, PHYSDEVOP_map_pirq is not allowed for PVH dom0 in current > codes. > > But it is fine to map interrupts through pirq to a HVM domain whose > XENFEAT_hvm_pirqs is not enabled. Because pirq field is used as a way to > reference interrupts and it is just the way for the device model to > identify which interrupt should be mapped to which domain, however > has_pirq() is just to check if HVM domains route interrupts from > devices(emulated or passthrough) through event channel, so, the has_pirq() > check should not be applied to the PHYSDEVOP_map_pirq issued by dom0. > > And the PVH domU which use vpci trying to issue a map_pirq will fail at the > xsm_map_domain_pirq() check in physdev_map_pirq() . > > So, allow PHYSDEVOP_map_pirq when dom0 is PVH and also allow > PHYSDEVOP_unmap_pirq for the removal device path to unmap pirq. Then the > interrupt of a passthrough device can be successfully mapped to pirq for domU. > >> >>>>> And there already are some senarios for domains without >>>>> X86_EMU_USE_PIRQ to use these functions. >>>> >>>> Are there? If so, pointing out an example may help. >>> If I understand correctly, Roger mentioned that PIRQs is disable by default for HVM guest("hvm_pirq=0") and passthrough device to guest. >>> In this scene, guest doesn't have PIRQs, but it still needs this hypercall. >> >> In which case please say so in order to be concrete, not vague. >> >> Jan >
On 2024/9/3 14:09, Jan Beulich wrote: > On 03.09.2024 06:01, Chen, Jiqian wrote: >> On 2024/8/20 15:07, Jan Beulich wrote: >>> On 20.08.2024 08:12, Chen, Jiqian wrote: >>>> On 2024/8/19 17:08, Jan Beulich wrote: >>>>> On 16.08.2024 13:08, 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 >>>>>> iPHYSDEVOP_unmap_pirq for the removal device path to unmap pirq. >>>>>> So that the interrupt of a passthrough device can be successfully >>>>>> mapped to pirq for domU with a notion of PIRQ when dom0 is PVH. >>>>>> >>>>>> To exposing the functionality to wider than (presently) necessary >>>>>> audience(like PVH domU), so it doesn't add any futher restrictions. >>>>> >>>>> The code change is fine, but I'm struggling with this sentence. I can't >>>>> really derive what you're trying to say. >>>> Ah, I wanted to explain why this path not add any further restrictions, then used your comments of last version. >>>> How do I need to change this explanation? >>> >>> I think you want to take Roger's earlier comments (when he requested >>> the relaxation) as basis to re-write (combine) both of the latter two >>> paragraphs above (or maybe even all three of them). It's odd to first >>> talk about Dom0, as if the operations were to be exposed just there, >>> and only then add DomU-s. >> >> I tried to understand and summarize Roger's previous comments and changed commit message to the following. Do you think it is fine? > > What are we talking about here? You had some concern about the description of commit message of this patch. So I send a draft below to get your opinion. If you forgot, I will directly send a new version(v14) later today. > The patch was committed over a month ago? Yes, I sent this v13 in Aug 16, and sorry to reply late. > > Jan > >> x86/pvh: Allow (un)map_pirq when dom0 is PVH >> >> When dom0 is PVH type and passthrough a device to HVM domU, Qemu code >> xen_pt_realize->xc_physdev_map_pirq and libxl code pci_add_dm_done-> >> xc_physdev_map_pirq map a pirq for passthrough devices. >> In xc_physdev_map_pirq call stack, function hvm_physdev_op has a check >> has_pirq(currd), but currd is PVH dom0, PVH has no X86_EMU_USE_PIRQ flag, >> so it fails, PHYSDEVOP_map_pirq is not allowed for PVH dom0 in current >> codes. >> >> But it is fine to map interrupts through pirq to a HVM domain whose >> XENFEAT_hvm_pirqs is not enabled. Because pirq field is used as a way to >> reference interrupts and it is just the way for the device model to >> identify which interrupt should be mapped to which domain, however >> has_pirq() is just to check if HVM domains route interrupts from >> devices(emulated or passthrough) through event channel, so, the has_pirq() >> check should not be applied to the PHYSDEVOP_map_pirq issued by dom0. >> >> And the PVH domU which use vpci trying to issue a map_pirq will fail at the >> xsm_map_domain_pirq() check in physdev_map_pirq() . >> >> So, allow PHYSDEVOP_map_pirq when dom0 is PVH and also allow >> PHYSDEVOP_unmap_pirq for the removal device path to unmap pirq. Then the >> interrupt of a passthrough device can be successfully mapped to pirq for domU. >> >>> >>>>>> And there already are some senarios for domains without >>>>>> X86_EMU_USE_PIRQ to use these functions. >>>>> >>>>> Are there? If so, pointing out an example may help. >>>> If I understand correctly, Roger mentioned that PIRQs is disable by default for HVM guest("hvm_pirq=0") and passthrough device to guest. >>>> In this scene, guest doesn't have PIRQs, but it still needs this hypercall. >>> >>> In which case please say so in order to be concrete, not vague. >>> >>> Jan >> >
On 03.09.2024 08:20, Chen, Jiqian wrote: > On 2024/9/3 14:09, Jan Beulich wrote: >> On 03.09.2024 06:01, Chen, Jiqian wrote: >>> On 2024/8/20 15:07, Jan Beulich wrote: >>>> On 20.08.2024 08:12, Chen, Jiqian wrote: >>>>> On 2024/8/19 17:08, Jan Beulich wrote: >>>>>> On 16.08.2024 13:08, 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 >>>>>>> iPHYSDEVOP_unmap_pirq for the removal device path to unmap pirq. >>>>>>> So that the interrupt of a passthrough device can be successfully >>>>>>> mapped to pirq for domU with a notion of PIRQ when dom0 is PVH. >>>>>>> >>>>>>> To exposing the functionality to wider than (presently) necessary >>>>>>> audience(like PVH domU), so it doesn't add any futher restrictions. >>>>>> >>>>>> The code change is fine, but I'm struggling with this sentence. I can't >>>>>> really derive what you're trying to say. >>>>> Ah, I wanted to explain why this path not add any further restrictions, then used your comments of last version. >>>>> How do I need to change this explanation? >>>> >>>> I think you want to take Roger's earlier comments (when he requested >>>> the relaxation) as basis to re-write (combine) both of the latter two >>>> paragraphs above (or maybe even all three of them). It's odd to first >>>> talk about Dom0, as if the operations were to be exposed just there, >>>> and only then add DomU-s. >>> >>> I tried to understand and summarize Roger's previous comments and changed commit message to the following. Do you think it is fine? >> >> What are we talking about here? > You had some concern about the description of commit message of this patch. > So I send a draft below to get your opinion. > If you forgot, I will directly send a new version(v14) later today. I still don't get it. Yes, the patch could have done with a better description, but ... >> The patch was committed over a month ago? > Yes, I sent this v13 in Aug 16, and sorry to reply late. ... it's simply too late now. Jan
diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c index 68815b03eb25..0b7fc060b4e2 100644 --- a/xen/arch/x86/hvm/hypercall.c +++ b/xen/arch/x86/hvm/hypercall.c @@ -73,6 +73,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: