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

Groups > linux.kernel > #1455193 > unrolled thread

[PATCH v12 0/8] KVM PCIe/MSI passthrough on ARM/ARM64: kernel part 3/3: vfio changes

Back to article view | Back to linux.kernel

Contents

  [PATCH v12 0/8] KVM PCIe/MSI passthrough on ARM/ARM64: kernel part 3/3: vfio changes Eric Auger <eric.auger@redhat.com> - 2016-08-02 19:50 +0200
    [PATCH v12 3/8] vfio/type1: implement recursive vfio_find_dma_from_node Eric Auger <eric.auger@redhat.com> - 2016-08-02 19:50 +0200
    [PATCH v12 5/8] vfio: allow reserved msi iova registration Eric Auger <eric.auger@redhat.com> - 2016-08-02 19:50 +0200
    Re: [PATCH v12 0/8] KVM PCIe/MSI passthrough on ARM/ARM64: kernel  part 3/3: vfio changes Diana Madalina Craciun <diana.craciun@nxp.com> - 2016-08-05 19:30 +0200
      Re: [PATCH v12 0/8] KVM PCIe/MSI passthrough on ARM/ARM64: kernel  part 3/3: vfio changes Auger Eric <eric.auger@redhat.com> - 2016-08-07 16:10 +0200

#1455193 — [PATCH v12 0/8] KVM PCIe/MSI passthrough on ARM/ARM64: kernel part 3/3: vfio changes

This series allows the user-space to register a reserved IOVA domain.
This completes the kernel integration of the whole functionality on top
of the 2 previous parts (v11).

We reuse the VFIO DMA MAP ioctl with a new flag to bridge to the
msi-iommu API. The need for provisioning such MSI IOVA range is reported
through capability chain, using VFIO_IOMMU_TYPE1_INFO_CAP_MSI_GEOMETRY.

vfio_iommu_type1 checks if the MSI mapping is safe when attaching the
vfio group to the container (allow_unsafe_interrupts modality). This is
done in a very coarse way, looking at all the registered doorbells and
returning assignment is safe wrt IRQ if all the doorbells are safe.

More details & context can be found at:
http://www.linaro.org/blog/core-dump/kvm-pciemsi-passthrough-armarm64/

Best Regards

Eric

Testing:
- functional on ARM64 AMD Overdrive HW (single GICv2m frame) with
  Intel I350T2 (SR-IOV capable, igb/igbvf) and Intel 82574L (e1000e)
- functional on Cavium ThunderX (ARM GICv3 ITS) with Intel 82574L (e1000e)

References:
[1] [RFC 0/2] VFIO: Add virtual MSI doorbell support
    (https://lkml.org/lkml/2015/7/24/135)
[2] [RFC PATCH 0/6] vfio: Add interface to map MSI pages
    (https://lists.cs.columbia.edu/pipermail/kvmarm/2015-September/016607.html)
[3] [PATCH v2 0/3] Introduce MSI hardware mapping for VFIO
    (http://permalink.gmane.org/gmane.comp.emulators.kvm.arm.devel/3858)

Git: complete series available at
https://github.com/eauger/linux/tree/v4.7-rc7-passthrough-v12
previous: https://github.com/eauger/linux/tree/v4.7-rc7-passthrough-v11

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.

History:
v11 -> v12:
- no functional change. Only adapt to renamings done in PART II

v10 -> v11:
no change to this series, just incremented for consistency

v9 -> v10:
Took into account Alex' comments:
- split "vfio/type1: vfio_find_dma accepting a type argument" into 2 patches
- properly implement replay of MSI DMA slots (by setting the aperture on the
  new domain); fixes the assignment of several devices
- rework user api for vfio_iommu_type1_info capability chains (cap_offset
  at the end of the struct and fix padding issues)
- VFIO_IOVA_RESERVED renamed into VFIO_IOVA_MSI_RESERVED
- explicit dma->type setting to VFIO_IOVA_USER

v8 -> v9:
- report MSI geometry through capability chain (last patch only);
  with the current limitation that an arbitrary number of 16 page
  requirement is reported. To be improved later on.


v7 -> v8:
- use renamed msi-iommu API
- VFIO only responsible for setting the IOVA aperture
- use new DOMAIN_ATTR_MSI_GEOMETRY iommu domain attribute

v6 -> v7:
- vfio_find_dma now accepts a dma_type argument.
- should have recovered the capability to unmap the whole user IOVA range
- remove computation of nb IOVA pages -> will post a separate RFC for that
  while respinning the QEMU part

RFC v5 -> patch v6:
- split to ease the review process

RFC v4 -> RFC v5:
- take into account Thomas' comments on MSI related patches
  - split "msi: IOMMU map the doorbell address when needed"
  - increase readability and add comments
  - fix style issues
 - split "iommu: Add DOMAIN_ATTR_MSI_MAPPING attribute"
 - platform ITS now advertises IOMMU_CAP_INTR_REMAP
 - fix compilation issue with CONFIG_IOMMU API unset
 - arm-smmu-v3 now advertises DOMAIN_ATTR_MSI_MAPPING

RFC v3 -> v4:
- Move doorbell mapping/unmapping in msi.c
- fix ref count issue on set_affinity: in case of a change in the address
  the previous address is decremented
- doorbell map/unmap now is done on msi composition. Should allow the use
  case for platform MSI controllers
- create dma-reserved-iommu.h/c exposing/implementing a new API dedicated
  to reserved IOVA management (looking like dma-iommu glue)
- series reordering to ease the review:
  - first part is related to IOMMU
  - second related to MSI sub-system
  - third related to VFIO (except arm-smmu IOMMU_CAP_INTR_REMAP removal)
- expose the number of requested IOVA pages through VFIO_IOMMU_GET_INFO
  [this partially addresses Marc's comments on iommu_get/put_single_reserved
   size/alignment problematic - which I did not ignore - but I don't know
   how much I can do at the moment]

RFC v2 -> RFC v3:
- should fix wrong handling of some CONFIG combinations:
  CONFIG_IOVA, CONFIG_IOMMU_API, CONFIG_PCI_MSI_IRQ_DOMAIN
- fix MSI_FLAG_IRQ_REMAPPING setting in GICv3 ITS (although not tested)

PATCH v1 -> RFC v2:
- reverted to RFC since it looks more reasonable ;-) the code is split
  between VFIO, IOMMU, MSI controller and I am not sure I did the right
  choices. Also API need to be further discussed.
- iova API usage in arm-smmu.c.
- MSI controller natively programs the MSI addr with either the PA or IOVA.
  This is not done anymore in vfio-pci driver as suggested by Alex.
- check irq remapping capability of the group

RFC v1 [2] -> PATCH v1:
- use the existing dma map/unmap ioctl interface with a flag to register a
  reserved IOVA range. Use the legacy Rb to store this special vfio_dma.
- a single reserved IOVA contiguous region now is allowed
- use of an RB tree indexed by PA to store allocated reserved slots
- use of a vfio_domain iova_domain to manage iova allocation within the
  window provided by the userspace
- vfio alloc_map/unmap_free take a vfio_group handle
- vfio_group handle is cached in vfio_pci_device
- add ref counting to bindings
- user modality enabled at the end of the series


Eric Auger (8):
  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: return MSI geometry through VFIO_IOMMU_GET_INFO capability
    chains

 drivers/iommu/arm-smmu-v3.c     |   3 +-
 drivers/iommu/arm-smmu.c        |   3 +-
 drivers/vfio/vfio_iommu_type1.c | 246 +++++++++++++++++++++++++++++++++++++---
 include/uapi/linux/vfio.h       |  42 ++++++-
 4 files changed, 276 insertions(+), 18 deletions(-)

-- 
1.9.1

[toc] | [next] | [standalone]

#1455201 — [PATCH v12 3/8] vfio/type1: implement recursive vfio_find_dma_from_node

This patch handles the case where a node is encountered, matching
@start and @size arguments but not matching the @type argument.
In that case, we need to skip that node and pursue the search in the
node's leaves. In case @start is inferior to the node's base, we
resume the search on the left leaf. If the recursive search on the left
leaves did not produce any match, we search the right leaves recursively.

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

v10: creation
---
 drivers/vfio/vfio_iommu_type1.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index cb7267a..65a4038 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -125,7 +125,17 @@ static struct vfio_dma *vfio_find_dma_from_node(struct rb_node *top,
 	if (type == VFIO_IOVA_ANY || dma->type == type)
 		return dma;
 
-	return NULL;
+	/* restart 2 searches skipping the current node */
+	if (start < dma->iova) {
+		dma = vfio_find_dma_from_node(node->rb_left, start,
+					      size, type);
+		if (dma)
+			return dma;
+	}
+	if (start + size > dma->iova + dma->size)
+		dma = vfio_find_dma_from_node(node->rb_right, start,
+					      size, type);
+	return dma;
 }
 
 /**
-- 
1.9.1

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

#1455205 — [PATCH v12 5/8] vfio: allow reserved msi iova registration

The user is allowed to register a reserved MSI IOVA range by using the
DMA MAP API and setting the new flag: VFIO_DMA_MAP_FLAG_MSI_RESERVED_IOVA.
This region is stored in the vfio_dma rb tree. At that point the iova
range is not mapped to any target address yet. The host kernel will use
those iova when needed, typically when MSIs are allocated.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>

---
v9 -> v10
- use VFIO_IOVA_RESERVED_MSI enum value

v7 -> v8:
- use iommu_msi_set_aperture function. There is no notion of
  unregistration anymore since the reserved msi slot remains
  until the container gets closed.

v6 -> v7:
- use iommu_free_reserved_iova_domain
- convey prot attributes downto dma-reserved-iommu iova domain creation
- reserved bindings teardown now performed on iommu domain destruction
- rename VFIO_DMA_MAP_FLAG_MSI_RESERVED_IOVA into
         VFIO_DMA_MAP_FLAG_RESERVED_MSI_IOVA
- change title
- pass the protection attribute to dma-reserved-iommu API

v3 -> v4:
- use iommu_alloc/free_reserved_iova_domain exported by dma-reserved-iommu
- protect vfio_register_reserved_iova_range implementation with
  CONFIG_IOMMU_DMA_RESERVED
- handle unregistration by user-space and on vfio_iommu_type1 release

v1 -> v2:
- set returned value according to alloc_reserved_iova_domain result
- free the iova domains in case any error occurs

RFC v1 -> v1:
- takes into account Alex comments, based on
  [RFC PATCH 1/6] vfio: Add interface for add/del reserved iova region:
- use the existing dma map/unmap ioctl interface with a flag to register
  a reserved IOVA range. A single reserved iova region is allowed.
---
 drivers/vfio/vfio_iommu_type1.c | 77 ++++++++++++++++++++++++++++++++++++++++-
 include/uapi/linux/vfio.h       | 10 +++++-
 2 files changed, 85 insertions(+), 2 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 69c775e..14dc9ef 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -442,6 +442,20 @@ static void vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma)
 	vfio_lock_acct(-unlocked);
 }
 
+static int vfio_set_msi_aperture(struct vfio_iommu *iommu,
+				dma_addr_t iova, size_t size)
+{
+	struct vfio_domain *d;
+	int ret = 0;
+
+	list_for_each_entry(d, &iommu->domain_list, next) {
+		ret = iommu_msi_set_aperture(d->domain, iova, iova + size - 1);
+		if (ret)
+			break;
+	}
+	return ret;
+}
+
 static void vfio_remove_dma(struct vfio_iommu *iommu, struct vfio_dma *dma)
 {
 	vfio_unmap_unpin(iommu, dma);
@@ -691,6 +705,63 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
 	return ret;
 }
 
+static int vfio_register_msi_range(struct vfio_iommu *iommu,
+				   struct vfio_iommu_type1_dma_map *map)
+{
+	dma_addr_t iova = map->iova;
+	size_t size = map->size;
+	int ret = 0;
+	struct vfio_dma *dma;
+	unsigned long order;
+	uint64_t mask;
+
+	/* Verify that none of our __u64 fields overflow */
+	if (map->size != size || map->iova != iova)
+		return -EINVAL;
+
+	order =  __ffs(vfio_pgsize_bitmap(iommu));
+	mask = ((uint64_t)1 << order) - 1;
+
+	WARN_ON(mask & PAGE_MASK);
+
+	if (!size || (size | iova) & mask)
+		return -EINVAL;
+
+	/* Don't allow IOVA address wrap */
+	if (iova + size - 1 < iova)
+		return -EINVAL;
+
+	mutex_lock(&iommu->lock);
+
+	if (vfio_find_dma(iommu, iova, size, VFIO_IOVA_ANY)) {
+		ret =  -EEXIST;
+		goto unlock;
+	}
+
+	dma = kzalloc(sizeof(*dma), GFP_KERNEL);
+	if (!dma) {
+		ret = -ENOMEM;
+		goto unlock;
+	}
+
+	dma->iova = iova;
+	dma->size = size;
+	dma->type = VFIO_IOVA_RESERVED_MSI;
+
+	ret = vfio_set_msi_aperture(iommu, iova, size);
+	if (ret)
+		goto free_unlock;
+
+	vfio_link_dma(iommu, dma);
+	goto unlock;
+
+free_unlock:
+	kfree(dma);
+unlock:
+	mutex_unlock(&iommu->lock);
+	return ret;
+}
+
 static int vfio_bus_type(struct device *dev, void *data)
 {
 	struct bus_type **bus = data;
@@ -1065,7 +1136,8 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
 	} else if (cmd == VFIO_IOMMU_MAP_DMA) {
 		struct vfio_iommu_type1_dma_map map;
 		uint32_t mask = VFIO_DMA_MAP_FLAG_READ |
-				VFIO_DMA_MAP_FLAG_WRITE;
+				VFIO_DMA_MAP_FLAG_WRITE |
+				VFIO_DMA_MAP_FLAG_RESERVED_MSI_IOVA;
 
 		minsz = offsetofend(struct vfio_iommu_type1_dma_map, size);
 
@@ -1075,6 +1147,9 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
 		if (map.argsz < minsz || map.flags & ~mask)
 			return -EINVAL;
 
+		if (map.flags & VFIO_DMA_MAP_FLAG_RESERVED_MSI_IOVA)
+			return vfio_register_msi_range(iommu, &map);
+
 		return vfio_dma_do_map(iommu, &map);
 
 	} else if (cmd == VFIO_IOMMU_UNMAP_DMA) {
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 255a211..4a9dbc2 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -498,12 +498,19 @@ struct vfio_iommu_type1_info {
  *
  * Map process virtual addresses to IO virtual addresses using the
  * provided struct vfio_dma_map. Caller sets argsz. READ &/ WRITE required.
+ *
+ * In case RESERVED_MSI_IOVA flag is set, the API only aims at registering an
+ * IOVA region that will be used on some platforms to map the host MSI frames.
+ * In that specific case, vaddr is ignored. Once registered, an MSI reserved
+ * IOVA region stays until the container is closed.
  */
 struct vfio_iommu_type1_dma_map {
 	__u32	argsz;
 	__u32	flags;
 #define VFIO_DMA_MAP_FLAG_READ (1 << 0)		/* readable from device */
 #define VFIO_DMA_MAP_FLAG_WRITE (1 << 1)	/* writable from device */
+/* reserved iova for MSI vectors*/
+#define VFIO_DMA_MAP_FLAG_RESERVED_MSI_IOVA (1 << 2)
 	__u64	vaddr;				/* Process virtual address */
 	__u64	iova;				/* IO virtual address */
 	__u64	size;				/* Size of mapping (bytes) */
@@ -519,7 +526,8 @@ struct vfio_iommu_type1_dma_map {
  * Caller sets argsz.  The actual unmapped size is returned in the size
  * field.  No guarantee is made to the user that arbitrary unmaps of iova
  * or size different from those used in the original mapping call will
- * succeed.
+ * succeed. Once registered, an MSI region cannot be unmapped and stays
+ * until the container is closed.
  */
 struct vfio_iommu_type1_dma_unmap {
 	__u32	argsz;
-- 
1.9.1

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

#1457220 — Re: [PATCH v12 0/8] KVM PCIe/MSI passthrough on ARM/ARM64: kernel part 3/3: vfio changes

Hi Eric,

I have tested these patches in a VFIO PCI scenario (using the ITS
emulation) on a NXP LS2080 board. It worked fine with one e1000 card
assigned to the guest. However, when I tried to assign two cards to the
guest I got a crash. I narrowed down the problem to this code:

drivers/vfio/vfio_iommu_type1.c, vfio_iommu_type1_attach_group function

   /*
     * Try to match an existing compatible domain.  We don't want to
     * preclude an IOMMU driver supporting multiple bus_types and being
     * able to include different bus_types in the same IOMMU domain, so
     * we test whether the domains use the same iommu_ops rather than
     * testing if they're on the same bus_type.
     */
    list_for_each_entry(d, &iommu->domain_list, next) {
        if (d->domain->ops == domain->domain->ops &&
            d->prot == domain->prot) {
            iommu_detach_group(domain->domain, iommu_group);
            if (!iommu_attach_group(d->domain, iommu_group)) {
                list_add(&group->next, &d->group_list);
                iommu_domain_free(domain->domain);
                kfree(domain);
                mutex_unlock(&iommu->lock);
                return 0;
            }

            ret = iommu_attach_group(domain->domain, iommu_group);
            if (ret)
                goto out_domain;
        }
    }

The iommu_domain_free function eventually calls iommu_put_dma_cookie
which calls put_iova_domain. This function (put_iova_domain) tries to
acquire some spinlocks which were not yet initialized. They will be
initialized later when the first DMA mapping will be performed. Because
the spinlock was not yet initialized, it crashes when attempting to
acquire it.

With the following fix I was able to successfully assign two cards to
the guest:

diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index ba764a0..b43e34f 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -457,6 +457,9 @@ void put_iova_domain(struct iova_domain *iovad)
        struct rb_node *node;
        unsigned long flags;
 
+       if (!iovad->start_pfn)
+               return;
+
        free_iova_rcaches(iovad);
        spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
        node = rb_first(&iovad->rbroot);

However, I am not sure if this is the right fix.

Thanks,

Diana


On 08/02/2016 08:30 PM, Eric Auger wrote:
> This series allows the user-space to register a reserved IOVA domain.
> This completes the kernel integration of the whole functionality on top
> of the 2 previous parts (v11).
>
> We reuse the VFIO DMA MAP ioctl with a new flag to bridge to the
> msi-iommu API. The need for provisioning such MSI IOVA range is reported
> through capability chain, using VFIO_IOMMU_TYPE1_INFO_CAP_MSI_GEOMETRY.
>
> vfio_iommu_type1 checks if the MSI mapping is safe when attaching the
> vfio group to the container (allow_unsafe_interrupts modality). This is
> done in a very coarse way, looking at all the registered doorbells and
> returning assignment is safe wrt IRQ if all the doorbells are safe.
>
> More details & context can be found at:
> http://www.linaro.org/blog/core-dump/kvm-pciemsi-passthrough-armarm64/
>
> Best Regards
>
> Eric
>
> Testing:
> - functional on ARM64 AMD Overdrive HW (single GICv2m frame) with
>   Intel I350T2 (SR-IOV capable, igb/igbvf) and Intel 82574L (e1000e)
> - functional on Cavium ThunderX (ARM GICv3 ITS) with Intel 82574L (e1000e)
>
> References:
> [1] [RFC 0/2] VFIO: Add virtual MSI doorbell support
>     (https://lkml.org/lkml/2015/7/24/135)
> [2] [RFC PATCH 0/6] vfio: Add interface to map MSI pages
>     (https://lists.cs.columbia.edu/pipermail/kvmarm/2015-September/016607.html)
> [3] [PATCH v2 0/3] Introduce MSI hardware mapping for VFIO
>     (http://permalink.gmane.org/gmane.comp.emulators.kvm.arm.devel/3858)
>
> Git: complete series available at
> https://github.com/eauger/linux/tree/v4.7-rc7-passthrough-v12
> previous: https://github.com/eauger/linux/tree/v4.7-rc7-passthrough-v11
>
> 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.
>
> History:
> v11 -> v12:
> - no functional change. Only adapt to renamings done in PART II
>
> v10 -> v11:
> no change to this series, just incremented for consistency
>
> v9 -> v10:
> Took into account Alex' comments:
> - split "vfio/type1: vfio_find_dma accepting a type argument" into 2 patches
> - properly implement replay of MSI DMA slots (by setting the aperture on the
>   new domain); fixes the assignment of several devices
> - rework user api for vfio_iommu_type1_info capability chains (cap_offset
>   at the end of the struct and fix padding issues)
> - VFIO_IOVA_RESERVED renamed into VFIO_IOVA_MSI_RESERVED
> - explicit dma->type setting to VFIO_IOVA_USER
>
> v8 -> v9:
> - report MSI geometry through capability chain (last patch only);
>   with the current limitation that an arbitrary number of 16 page
>   requirement is reported. To be improved later on.
>
>
> v7 -> v8:
> - use renamed msi-iommu API
> - VFIO only responsible for setting the IOVA aperture
> - use new DOMAIN_ATTR_MSI_GEOMETRY iommu domain attribute
>
> v6 -> v7:
> - vfio_find_dma now accepts a dma_type argument.
> - should have recovered the capability to unmap the whole user IOVA range
> - remove computation of nb IOVA pages -> will post a separate RFC for that
>   while respinning the QEMU part
>
> RFC v5 -> patch v6:
> - split to ease the review process
>
> RFC v4 -> RFC v5:
> - take into account Thomas' comments on MSI related patches
>   - split "msi: IOMMU map the doorbell address when needed"
>   - increase readability and add comments
>   - fix style issues
>  - split "iommu: Add DOMAIN_ATTR_MSI_MAPPING attribute"
>  - platform ITS now advertises IOMMU_CAP_INTR_REMAP
>  - fix compilation issue with CONFIG_IOMMU API unset
>  - arm-smmu-v3 now advertises DOMAIN_ATTR_MSI_MAPPING
>
> RFC v3 -> v4:
> - Move doorbell mapping/unmapping in msi.c
> - fix ref count issue on set_affinity: in case of a change in the address
>   the previous address is decremented
> - doorbell map/unmap now is done on msi composition. Should allow the use
>   case for platform MSI controllers
> - create dma-reserved-iommu.h/c exposing/implementing a new API dedicated
>   to reserved IOVA management (looking like dma-iommu glue)
> - series reordering to ease the review:
>   - first part is related to IOMMU
>   - second related to MSI sub-system
>   - third related to VFIO (except arm-smmu IOMMU_CAP_INTR_REMAP removal)
> - expose the number of requested IOVA pages through VFIO_IOMMU_GET_INFO
>   [this partially addresses Marc's comments on iommu_get/put_single_reserved
>    size/alignment problematic - which I did not ignore - but I don't know
>    how much I can do at the moment]
>
> RFC v2 -> RFC v3:
> - should fix wrong handling of some CONFIG combinations:
>   CONFIG_IOVA, CONFIG_IOMMU_API, CONFIG_PCI_MSI_IRQ_DOMAIN
> - fix MSI_FLAG_IRQ_REMAPPING setting in GICv3 ITS (although not tested)
>
> PATCH v1 -> RFC v2:
> - reverted to RFC since it looks more reasonable ;-) the code is split
>   between VFIO, IOMMU, MSI controller and I am not sure I did the right
>   choices. Also API need to be further discussed.
> - iova API usage in arm-smmu.c.
> - MSI controller natively programs the MSI addr with either the PA or IOVA.
>   This is not done anymore in vfio-pci driver as suggested by Alex.
> - check irq remapping capability of the group
>
> RFC v1 [2] -> PATCH v1:
> - use the existing dma map/unmap ioctl interface with a flag to register a
>   reserved IOVA range. Use the legacy Rb to store this special vfio_dma.
> - a single reserved IOVA contiguous region now is allowed
> - use of an RB tree indexed by PA to store allocated reserved slots
> - use of a vfio_domain iova_domain to manage iova allocation within the
>   window provided by the userspace
> - vfio alloc_map/unmap_free take a vfio_group handle
> - vfio_group handle is cached in vfio_pci_device
> - add ref counting to bindings
> - user modality enabled at the end of the series
>
>
> Eric Auger (8):
>   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: return MSI geometry through VFIO_IOMMU_GET_INFO capability
>     chains
>
>  drivers/iommu/arm-smmu-v3.c     |   3 +-
>  drivers/iommu/arm-smmu.c        |   3 +-
>  drivers/vfio/vfio_iommu_type1.c | 246 +++++++++++++++++++++++++++++++++++++---
>  include/uapi/linux/vfio.h       |  42 ++++++-
>  4 files changed, 276 insertions(+), 18 deletions(-)
>

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

#1457457 — Re: [PATCH v12 0/8] KVM PCIe/MSI passthrough on ARM/ARM64: kernel part 3/3: vfio changes

Hi Diana,

On 05/08/2016 16:46, Diana Madalina Craciun wrote:
> Hi Eric,
> 
> I have tested these patches in a VFIO PCI scenario (using the ITS
> emulation) on a NXP LS2080 board. It worked fine with one e1000 card
> assigned to the guest. However, when I tried to assign two cards to the
> guest I got a crash. I narrowed down the problem to this code:
> 
> drivers/vfio/vfio_iommu_type1.c, vfio_iommu_type1_attach_group function
> 
>    /*
>      * Try to match an existing compatible domain.  We don't want to
>      * preclude an IOMMU driver supporting multiple bus_types and being
>      * able to include different bus_types in the same IOMMU domain, so
>      * we test whether the domains use the same iommu_ops rather than
>      * testing if they're on the same bus_type.
>      */
>     list_for_each_entry(d, &iommu->domain_list, next) {
>         if (d->domain->ops == domain->domain->ops &&
>             d->prot == domain->prot) {
>             iommu_detach_group(domain->domain, iommu_group);
>             if (!iommu_attach_group(d->domain, iommu_group)) {
>                 list_add(&group->next, &d->group_list);
>                 iommu_domain_free(domain->domain);
>                 kfree(domain);
>                 mutex_unlock(&iommu->lock);
>                 return 0;
>             }
> 
>             ret = iommu_attach_group(domain->domain, iommu_group);
>             if (ret)
>                 goto out_domain;
>         }
>     }
> 
> The iommu_domain_free function eventually calls iommu_put_dma_cookie
> which calls put_iova_domain. This function (put_iova_domain) tries to
> acquire some spinlocks which were not yet initialized. They will be
> initialized later when the first DMA mapping will be performed. Because
> the spinlock was not yet initialized, it crashes when attempting to
> acquire it.
> 
> With the following fix I was able to successfully assign two cards to
> the guest:
> 
> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
> index ba764a0..b43e34f 100644
> --- a/drivers/iommu/iova.c
> +++ b/drivers/iommu/iova.c
> @@ -457,6 +457,9 @@ void put_iova_domain(struct iova_domain *iovad)
>         struct rb_node *node;
>         unsigned long flags;
>  
> +       if (!iovad->start_pfn)
> +               return;
> +
>         free_iova_rcaches(iovad);
>         spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
>         node = rb_first(&iovad->rbroot);
> 
> However, I am not sure if this is the right fix.

Thanks for testing. Sorry for the time you spent narrowing the above
issue. The problem is known and fixed by Robin's patch sent on iommu
list. The patch can be found on my branch - similar fix as the one you
did ;-) -:

iommu/dma: Don't put uninitialised IOVA domains

Thanks!

Eric
> 
> Thanks,
> 
> Diana
> 
> 
> On 08/02/2016 08:30 PM, Eric Auger wrote:
>> This series allows the user-space to register a reserved IOVA domain.
>> This completes the kernel integration of the whole functionality on top
>> of the 2 previous parts (v11).
>>
>> We reuse the VFIO DMA MAP ioctl with a new flag to bridge to the
>> msi-iommu API. The need for provisioning such MSI IOVA range is reported
>> through capability chain, using VFIO_IOMMU_TYPE1_INFO_CAP_MSI_GEOMETRY.
>>
>> vfio_iommu_type1 checks if the MSI mapping is safe when attaching the
>> vfio group to the container (allow_unsafe_interrupts modality). This is
>> done in a very coarse way, looking at all the registered doorbells and
>> returning assignment is safe wrt IRQ if all the doorbells are safe.
>>
>> More details & context can be found at:
>> http://www.linaro.org/blog/core-dump/kvm-pciemsi-passthrough-armarm64/
>>
>> Best Regards
>>
>> Eric
>>
>> Testing:
>> - functional on ARM64 AMD Overdrive HW (single GICv2m frame) with
>>   Intel I350T2 (SR-IOV capable, igb/igbvf) and Intel 82574L (e1000e)
>> - functional on Cavium ThunderX (ARM GICv3 ITS) with Intel 82574L (e1000e)
>>
>> References:
>> [1] [RFC 0/2] VFIO: Add virtual MSI doorbell support
>>     (https://lkml.org/lkml/2015/7/24/135)
>> [2] [RFC PATCH 0/6] vfio: Add interface to map MSI pages
>>     (https://lists.cs.columbia.edu/pipermail/kvmarm/2015-September/016607.html)
>> [3] [PATCH v2 0/3] Introduce MSI hardware mapping for VFIO
>>     (http://permalink.gmane.org/gmane.comp.emulators.kvm.arm.devel/3858)
>>
>> Git: complete series available at
>> https://github.com/eauger/linux/tree/v4.7-rc7-passthrough-v12
>> previous: https://github.com/eauger/linux/tree/v4.7-rc7-passthrough-v11
>>
>> 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.
>>
>> History:
>> v11 -> v12:
>> - no functional change. Only adapt to renamings done in PART II
>>
>> v10 -> v11:
>> no change to this series, just incremented for consistency
>>
>> v9 -> v10:
>> Took into account Alex' comments:
>> - split "vfio/type1: vfio_find_dma accepting a type argument" into 2 patches
>> - properly implement replay of MSI DMA slots (by setting the aperture on the
>>   new domain); fixes the assignment of several devices
>> - rework user api for vfio_iommu_type1_info capability chains (cap_offset
>>   at the end of the struct and fix padding issues)
>> - VFIO_IOVA_RESERVED renamed into VFIO_IOVA_MSI_RESERVED
>> - explicit dma->type setting to VFIO_IOVA_USER
>>
>> v8 -> v9:
>> - report MSI geometry through capability chain (last patch only);
>>   with the current limitation that an arbitrary number of 16 page
>>   requirement is reported. To be improved later on.
>>
>>
>> v7 -> v8:
>> - use renamed msi-iommu API
>> - VFIO only responsible for setting the IOVA aperture
>> - use new DOMAIN_ATTR_MSI_GEOMETRY iommu domain attribute
>>
>> v6 -> v7:
>> - vfio_find_dma now accepts a dma_type argument.
>> - should have recovered the capability to unmap the whole user IOVA range
>> - remove computation of nb IOVA pages -> will post a separate RFC for that
>>   while respinning the QEMU part
>>
>> RFC v5 -> patch v6:
>> - split to ease the review process
>>
>> RFC v4 -> RFC v5:
>> - take into account Thomas' comments on MSI related patches
>>   - split "msi: IOMMU map the doorbell address when needed"
>>   - increase readability and add comments
>>   - fix style issues
>>  - split "iommu: Add DOMAIN_ATTR_MSI_MAPPING attribute"
>>  - platform ITS now advertises IOMMU_CAP_INTR_REMAP
>>  - fix compilation issue with CONFIG_IOMMU API unset
>>  - arm-smmu-v3 now advertises DOMAIN_ATTR_MSI_MAPPING
>>
>> RFC v3 -> v4:
>> - Move doorbell mapping/unmapping in msi.c
>> - fix ref count issue on set_affinity: in case of a change in the address
>>   the previous address is decremented
>> - doorbell map/unmap now is done on msi composition. Should allow the use
>>   case for platform MSI controllers
>> - create dma-reserved-iommu.h/c exposing/implementing a new API dedicated
>>   to reserved IOVA management (looking like dma-iommu glue)
>> - series reordering to ease the review:
>>   - first part is related to IOMMU
>>   - second related to MSI sub-system
>>   - third related to VFIO (except arm-smmu IOMMU_CAP_INTR_REMAP removal)
>> - expose the number of requested IOVA pages through VFIO_IOMMU_GET_INFO
>>   [this partially addresses Marc's comments on iommu_get/put_single_reserved
>>    size/alignment problematic - which I did not ignore - but I don't know
>>    how much I can do at the moment]
>>
>> RFC v2 -> RFC v3:
>> - should fix wrong handling of some CONFIG combinations:
>>   CONFIG_IOVA, CONFIG_IOMMU_API, CONFIG_PCI_MSI_IRQ_DOMAIN
>> - fix MSI_FLAG_IRQ_REMAPPING setting in GICv3 ITS (although not tested)
>>
>> PATCH v1 -> RFC v2:
>> - reverted to RFC since it looks more reasonable ;-) the code is split
>>   between VFIO, IOMMU, MSI controller and I am not sure I did the right
>>   choices. Also API need to be further discussed.
>> - iova API usage in arm-smmu.c.
>> - MSI controller natively programs the MSI addr with either the PA or IOVA.
>>   This is not done anymore in vfio-pci driver as suggested by Alex.
>> - check irq remapping capability of the group
>>
>> RFC v1 [2] -> PATCH v1:
>> - use the existing dma map/unmap ioctl interface with a flag to register a
>>   reserved IOVA range. Use the legacy Rb to store this special vfio_dma.
>> - a single reserved IOVA contiguous region now is allowed
>> - use of an RB tree indexed by PA to store allocated reserved slots
>> - use of a vfio_domain iova_domain to manage iova allocation within the
>>   window provided by the userspace
>> - vfio alloc_map/unmap_free take a vfio_group handle
>> - vfio_group handle is cached in vfio_pci_device
>> - add ref counting to bindings
>> - user modality enabled at the end of the series
>>
>>
>> Eric Auger (8):
>>   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: return MSI geometry through VFIO_IOMMU_GET_INFO capability
>>     chains
>>
>>  drivers/iommu/arm-smmu-v3.c     |   3 +-
>>  drivers/iommu/arm-smmu.c        |   3 +-
>>  drivers/vfio/vfio_iommu_type1.c | 246 +++++++++++++++++++++++++++++++++++++---
>>  include/uapi/linux/vfio.h       |  42 ++++++-
>>  4 files changed, 276 insertions(+), 18 deletions(-)
>>
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web