Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1500838 > unrolled thread

Re: [PATCH v14 00/16] KVM PCIe/MSI passthrough on ARM/ARM64

Started byPunit Agrawal <punit.agrawal@arm.com>
First post2016-10-14 13:30 +0200
Last post2016-10-17 17:20 +0200
Articles 3 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v14 00/16] KVM PCIe/MSI passthrough on ARM/ARM64 Punit Agrawal <punit.agrawal@arm.com> - 2016-10-14 13:30 +0200
    Re: [PATCH v14 00/16] KVM PCIe/MSI passthrough on ARM/ARM64 Auger Eric <eric.auger@redhat.com> - 2016-10-17 16:20 +0200
      Re: [PATCH v14 00/16] KVM PCIe/MSI passthrough on ARM/ARM64 Punit Agrawal <punit.agrawal@arm.com> - 2016-10-17 17:20 +0200

#1500838 — Re: [PATCH v14 00/16] KVM PCIe/MSI passthrough on ARM/ARM64

FromPunit Agrawal <punit.agrawal@arm.com>
Date2016-10-14 13:30 +0200
SubjectRe: [PATCH v14 00/16] KVM PCIe/MSI passthrough on ARM/ARM64
Message-ID<ss8Qy-CS-9@gated-at.bofh.it>
Hi Eric,

I am a bit late in joining, but I've tried to familiarise
myself with earlier discussions on the series.

Eric Auger <eric.auger@redhat.com> writes:

> This is the second respin on top of Robin's series [1], addressing Alex' comments.
>
> Major changes are:
> - MSI-doorbell API now is moved to DMA IOMMU API following Alex suggestion
>   to put all API pieces at the same place (so eventually in the IOMMU
>   subsystem)

IMHO, this is headed in the opposite direction, i.e., away from the
owner of the information - the doorbells are the property of the MSI
controller. The MSI controllers know the location, size and interrupt
remapping capability as well. On the consumer side, VFIO needs access to
the doorbells to allow userspace to carve out a region in the IOVA.

I quite liked what you had in v13, though I think you can go further
though. Instead of adding new doorbell API [un]registration calls, how
about adding a callback to the irq_domain_ops? The callback will be
populated for irqdomains registered by MSI controllers.

From VFIO, we can calculate the required aperture reservation by
iterating over the irqdomains (something like irq_domain_for_each). The
same callback can also provide information about support for interrupt
remapping.

For systems where there are no separate MSI controllers, i.e., the IOMMU
has a fixed reservation, no MSI callbacks will be populated - which
tells userspace that no separate MSI reservation is required. IIUC, this
was one of Alex' concerns on the prior version.

Thoughts, opinions?

Punit

> - new iommu_domain_msi_resv struct and accessor through DOMAIN_ATTR_MSI_RESV
>   domain with mirror VFIO capability
> - more robustness I think in the VFIO layer
> - added "iommu/iova: fix __alloc_and_insert_iova_range" since with the current
>   code I failed allocating an IOVA page in a single page domain with upper part
>   reserved
>
> IOVA range exclusion will be handled in a separate series
>
> The priority really is to discuss and freeze the API and especially the MSI
> doorbell's handling. Do we agree to put that in DMA IOMMU?
>
> Note: the size computation does not take into account possible page overlaps
> between doorbells but it would add quite a lot of complexity i think.
>
> Tested on AMD Overdrive (single GICv2m frame) with I350 VF assignment.
>
> dependency:
> the series depends on Robin's generic-v7 branch:
> [1] [PATCH v7 00/22] Generic DT bindings for PCI IOMMUs and ARM SMMU
> http://www.spinics.net/lists/arm-kernel/msg531110.html
>
> Best Regards
>
> Eric
>
> Git: complete series available at
> https://github.com/eauger/linux/tree/generic-v7-pcie-passthru-v14
>
> the above branch includes a temporary patch to work around a ThunderX pci
> bus reset crash (which I think unrelated to this series):
> "vfio: pci: HACK! workaround thunderx pci_try_reset_bus crash"
> Do not take this one for other platforms.
>
>
> Eric Auger (15):
>   iommu/iova: fix __alloc_and_insert_iova_range
>   iommu: Introduce DOMAIN_ATTR_MSI_RESV
>   iommu/dma: MSI doorbell alloc/free
>   iommu/dma: Introduce iommu_calc_msi_resv
>   iommu/arm-smmu: Implement domain_get_attr for DOMAIN_ATTR_MSI_RESV
>   irqchip/gic-v2m: Register the MSI doorbell
>   irqchip/gicv3-its: Register the MSI doorbell
>   vfio: Introduce a vfio_dma type field
>   vfio/type1: vfio_find_dma accepting a type argument
>   vfio/type1: Implement recursive vfio_find_dma_from_node
>   vfio/type1: Handle unmap/unpin and replay for VFIO_IOVA_RESERVED slots
>   vfio: Allow reserved msi iova registration
>   vfio/type1: Check doorbell safety
>   iommu/arm-smmu: Do not advertise IOMMU_CAP_INTR_REMAP
>   vfio/type1: Introduce MSI_RESV capability
>
> Robin Murphy (1):
>   iommu/dma: Allow MSI-only cookies
>
>  drivers/iommu/Kconfig            |   4 +-
>  drivers/iommu/arm-smmu-v3.c      |  10 +-
>  drivers/iommu/arm-smmu.c         |  10 +-
>  drivers/iommu/dma-iommu.c        | 184 ++++++++++++++++++++++++++
>  drivers/iommu/iova.c             |   2 +-
>  drivers/irqchip/irq-gic-v2m.c    |  10 +-
>  drivers/irqchip/irq-gic-v3-its.c |  13 ++
>  drivers/vfio/vfio_iommu_type1.c  | 279 +++++++++++++++++++++++++++++++++++++--
>  include/linux/dma-iommu.h        |  59 +++++++++
>  include/linux/iommu.h            |   8 ++
>  include/uapi/linux/vfio.h        |  30 ++++-
>  11 files changed, 587 insertions(+), 22 deletions(-)

[toc] | [next] | [standalone]

#1502018

FromAuger Eric <eric.auger@redhat.com>
Date2016-10-17 16:20 +0200
Message-ID<stgVH-4zx-27@gated-at.bofh.it>
In reply to#1500838
Hi Punit,

On 14/10/2016 13:24, Punit Agrawal wrote:
> Hi Eric,
> 
> I am a bit late in joining, but I've tried to familiarise
> myself with earlier discussions on the series.
> 
> Eric Auger <eric.auger@redhat.com> writes:
> 
>> This is the second respin on top of Robin's series [1], addressing Alex' comments.
>>
>> Major changes are:
>> - MSI-doorbell API now is moved to DMA IOMMU API following Alex suggestion
>>   to put all API pieces at the same place (so eventually in the IOMMU
>>   subsystem)
> 
> IMHO, this is headed in the opposite direction, i.e., away from the
> owner of the information - the doorbells are the property of the MSI
> controller. The MSI controllers know the location, size and interrupt
> remapping capability as well. On the consumer side, VFIO needs access to
> the doorbells to allow userspace to carve out a region in the IOVA.
> 
> I quite liked what you had in v13, though I think you can go further
> though. Instead of adding new doorbell API [un]registration calls, how
> about adding a callback to the irq_domain_ops? The callback will be
> populated for irqdomains registered by MSI controllers.

Thank you for jumping into that thread. Any help/feedback is greatly
appreciated.

Regarding your suggestion, the irq_domain looks dedicated to the
translation between linux irq and HW irq. I tend to think adding an ops
to retrieve the MSI doorbell info at that level looks far from the
original goal of the infrastructure. Obviously the fact there is a list
of such domain is tempting but I preferred to add a separate struct and
separate list.

In the v14 release I moved the "doorbell API" in the dma-iommu API since
Alex recommended to offer a unified API where all pieces would be at the
same place.

Anyway I will follow the guidance of maintainers.


> 
> From VFIO, we can calculate the required aperture reservation by
> iterating over the irqdomains (something like irq_domain_for_each). The
> same callback can also provide information about support for interrupt
> remapping.
> 
> For systems where there are no separate MSI controllers, i.e., the IOMMU
> has a fixed reservation, no MSI callbacks will be populated - which
> tells userspace that no separate MSI reservation is required. IIUC, this
> was one of Alex' concerns on the prior version.

I'am working on a separate series to report to the user-space the usable
IOVA range(s).

Thanks

Eric
> 
> Thoughts, opinions?
> 
> Punit
> 
>> - new iommu_domain_msi_resv struct and accessor through DOMAIN_ATTR_MSI_RESV
>>   domain with mirror VFIO capability
>> - more robustness I think in the VFIO layer
>> - added "iommu/iova: fix __alloc_and_insert_iova_range" since with the current
>>   code I failed allocating an IOVA page in a single page domain with upper part
>>   reserved
>>
>> IOVA range exclusion will be handled in a separate series
>>
>> The priority really is to discuss and freeze the API and especially the MSI
>> doorbell's handling. Do we agree to put that in DMA IOMMU?
>>
>> Note: the size computation does not take into account possible page overlaps
>> between doorbells but it would add quite a lot of complexity i think.
>>
>> Tested on AMD Overdrive (single GICv2m frame) with I350 VF assignment.
>>
>> dependency:
>> the series depends on Robin's generic-v7 branch:
>> [1] [PATCH v7 00/22] Generic DT bindings for PCI IOMMUs and ARM SMMU
>> http://www.spinics.net/lists/arm-kernel/msg531110.html
>>
>> Best Regards
>>
>> Eric
>>
>> Git: complete series available at
>> https://github.com/eauger/linux/tree/generic-v7-pcie-passthru-v14
>>
>> the above branch includes a temporary patch to work around a ThunderX pci
>> bus reset crash (which I think unrelated to this series):
>> "vfio: pci: HACK! workaround thunderx pci_try_reset_bus crash"
>> Do not take this one for other platforms.
>>
>>
>> Eric Auger (15):
>>   iommu/iova: fix __alloc_and_insert_iova_range
>>   iommu: Introduce DOMAIN_ATTR_MSI_RESV
>>   iommu/dma: MSI doorbell alloc/free
>>   iommu/dma: Introduce iommu_calc_msi_resv
>>   iommu/arm-smmu: Implement domain_get_attr for DOMAIN_ATTR_MSI_RESV
>>   irqchip/gic-v2m: Register the MSI doorbell
>>   irqchip/gicv3-its: Register the MSI doorbell
>>   vfio: Introduce a vfio_dma type field
>>   vfio/type1: vfio_find_dma accepting a type argument
>>   vfio/type1: Implement recursive vfio_find_dma_from_node
>>   vfio/type1: Handle unmap/unpin and replay for VFIO_IOVA_RESERVED slots
>>   vfio: Allow reserved msi iova registration
>>   vfio/type1: Check doorbell safety
>>   iommu/arm-smmu: Do not advertise IOMMU_CAP_INTR_REMAP
>>   vfio/type1: Introduce MSI_RESV capability
>>
>> Robin Murphy (1):
>>   iommu/dma: Allow MSI-only cookies
>>
>>  drivers/iommu/Kconfig            |   4 +-
>>  drivers/iommu/arm-smmu-v3.c      |  10 +-
>>  drivers/iommu/arm-smmu.c         |  10 +-
>>  drivers/iommu/dma-iommu.c        | 184 ++++++++++++++++++++++++++
>>  drivers/iommu/iova.c             |   2 +-
>>  drivers/irqchip/irq-gic-v2m.c    |  10 +-
>>  drivers/irqchip/irq-gic-v3-its.c |  13 ++
>>  drivers/vfio/vfio_iommu_type1.c  | 279 +++++++++++++++++++++++++++++++++++++--
>>  include/linux/dma-iommu.h        |  59 +++++++++
>>  include/linux/iommu.h            |   8 ++
>>  include/uapi/linux/vfio.h        |  30 ++++-
>>  11 files changed, 587 insertions(+), 22 deletions(-)
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

[toc] | [prev] | [next] | [standalone]

#1502100

FromPunit Agrawal <punit.agrawal@arm.com>
Date2016-10-17 17:20 +0200
Message-ID<sthRM-59n-39@gated-at.bofh.it>
In reply to#1502018
Auger Eric <eric.auger@redhat.com> writes:

> Hi Punit,
>
> On 14/10/2016 13:24, Punit Agrawal wrote:
>> Hi Eric,
>> 
>> I am a bit late in joining, but I've tried to familiarise
>> myself with earlier discussions on the series.
>> 
>> Eric Auger <eric.auger@redhat.com> writes:
>> 
>>> This is the second respin on top of Robin's series [1], addressing Alex' comments.
>>>
>>> Major changes are:
>>> - MSI-doorbell API now is moved to DMA IOMMU API following Alex suggestion
>>>   to put all API pieces at the same place (so eventually in the IOMMU
>>>   subsystem)
>> 
>> IMHO, this is headed in the opposite direction, i.e., away from the
>> owner of the information - the doorbells are the property of the MSI
>> controller. The MSI controllers know the location, size and interrupt
>> remapping capability as well. On the consumer side, VFIO needs access to
>> the doorbells to allow userspace to carve out a region in the IOVA.
>> 
>> I quite liked what you had in v13, though I think you can go further
>> though. Instead of adding new doorbell API [un]registration calls, how
>> about adding a callback to the irq_domain_ops? The callback will be
>> populated for irqdomains registered by MSI controllers.
>
> Thank you for jumping into that thread. Any help/feedback is greatly
> appreciated.
>
> Regarding your suggestion, the irq_domain looks dedicated to the
> translation between linux irq and HW irq. I tend to think adding an ops
> to retrieve the MSI doorbell info at that level looks far from the
> original goal of the infrastructure. Obviously the fact there is a list
> of such domain is tempting but I preferred to add a separate struct and
> separate list.

Guilty as charged - the suggestion was definitely borne out of
convenience. I originally looked at adding this functionality to the MSI
domains but couldn't find an obvious way to enumerate all of them.

Also, the way the domain hierarchy is setup for MSI controllers, the
actual doorbell is not available to the MSI domains.

>
> In the v14 release I moved the "doorbell API" in the dma-iommu API since
> Alex recommended to offer a unified API where all pieces would be at the
> same place.
>
> Anyway I will follow the guidance of maintainers.

Ok, makes sense.

>
>
>> 
>> From VFIO, we can calculate the required aperture reservation by
>> iterating over the irqdomains (something like irq_domain_for_each). The
>> same callback can also provide information about support for interrupt
>> remapping.
>> 
>> For systems where there are no separate MSI controllers, i.e., the IOMMU
>> has a fixed reservation, no MSI callbacks will be populated - which
>> tells userspace that no separate MSI reservation is required. IIUC, this
>> was one of Alex' concerns on the prior version.
>
> I'am working on a separate series to report to the user-space the usable
> IOVA range(s).

Is that the alternative to reporting the aperture size that needs to be
reserved?

>
> Thanks
>
> Eric
>> 
>> Thoughts, opinions?
>> 
>> Punit
>> 
>>> - new iommu_domain_msi_resv struct and accessor through DOMAIN_ATTR_MSI_RESV
>>>   domain with mirror VFIO capability
>>> - more robustness I think in the VFIO layer
>>> - added "iommu/iova: fix __alloc_and_insert_iova_range" since with the current
>>>   code I failed allocating an IOVA page in a single page domain with upper part
>>>   reserved
>>>
>>> IOVA range exclusion will be handled in a separate series
>>>
>>> The priority really is to discuss and freeze the API and especially the MSI
>>> doorbell's handling. Do we agree to put that in DMA IOMMU?
>>>
>>> Note: the size computation does not take into account possible page overlaps
>>> between doorbells but it would add quite a lot of complexity i think.
>>>
>>> Tested on AMD Overdrive (single GICv2m frame) with I350 VF assignment.
>>>
>>> dependency:
>>> the series depends on Robin's generic-v7 branch:
>>> [1] [PATCH v7 00/22] Generic DT bindings for PCI IOMMUs and ARM SMMU
>>> http://www.spinics.net/lists/arm-kernel/msg531110.html
>>>
>>> Best Regards
>>>
>>> Eric
>>>
>>> Git: complete series available at
>>> https://github.com/eauger/linux/tree/generic-v7-pcie-passthru-v14
>>>
>>> the above branch includes a temporary patch to work around a ThunderX pci
>>> bus reset crash (which I think unrelated to this series):
>>> "vfio: pci: HACK! workaround thunderx pci_try_reset_bus crash"
>>> Do not take this one for other platforms.
>>>
>>>
>>> Eric Auger (15):
>>>   iommu/iova: fix __alloc_and_insert_iova_range
>>>   iommu: Introduce DOMAIN_ATTR_MSI_RESV
>>>   iommu/dma: MSI doorbell alloc/free
>>>   iommu/dma: Introduce iommu_calc_msi_resv
>>>   iommu/arm-smmu: Implement domain_get_attr for DOMAIN_ATTR_MSI_RESV
>>>   irqchip/gic-v2m: Register the MSI doorbell
>>>   irqchip/gicv3-its: Register the MSI doorbell
>>>   vfio: Introduce a vfio_dma type field
>>>   vfio/type1: vfio_find_dma accepting a type argument
>>>   vfio/type1: Implement recursive vfio_find_dma_from_node
>>>   vfio/type1: Handle unmap/unpin and replay for VFIO_IOVA_RESERVED slots
>>>   vfio: Allow reserved msi iova registration
>>>   vfio/type1: Check doorbell safety
>>>   iommu/arm-smmu: Do not advertise IOMMU_CAP_INTR_REMAP
>>>   vfio/type1: Introduce MSI_RESV capability
>>>
>>> Robin Murphy (1):
>>>   iommu/dma: Allow MSI-only cookies
>>>
>>>  drivers/iommu/Kconfig            |   4 +-
>>>  drivers/iommu/arm-smmu-v3.c      |  10 +-
>>>  drivers/iommu/arm-smmu.c         |  10 +-
>>>  drivers/iommu/dma-iommu.c        | 184 ++++++++++++++++++++++++++
>>>  drivers/iommu/iova.c             |   2 +-
>>>  drivers/irqchip/irq-gic-v2m.c    |  10 +-
>>>  drivers/irqchip/irq-gic-v3-its.c |  13 ++
>>>  drivers/vfio/vfio_iommu_type1.c  | 279 +++++++++++++++++++++++++++++++++++++--
>>>  include/linux/dma-iommu.h        |  59 +++++++++
>>>  include/linux/iommu.h            |   8 ++
>>>  include/uapi/linux/vfio.h        |  30 ++++-
>>>  11 files changed, 587 insertions(+), 22 deletions(-)
>> 
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>> 
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web