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

Groups > linux.kernel > #1389862 > unrolled thread

[PATCH v8 0/8] KVM PCIe/MSI passthrough on ARM/ARM64: kernel part 2/3: msi changes

Back to article view | Back to linux.kernel

Contents

  [PATCH v8 0/8] KVM PCIe/MSI passthrough on ARM/ARM64: kernel part 2/3: msi changes Eric Auger <eric.auger@linaro.org> - 2016-04-28 10:30 +0200
    [PATCH v8 4/8] genirq/msi: msi_compose wrapper Eric Auger <eric.auger@linaro.org> - 2016-04-28 10:30 +0200
    [PATCH v8 5/8] genirq/irq: introduce msi_doorbell_info Eric Auger <eric.auger@linaro.org> - 2016-04-28 10:30 +0200
      Re: [PATCH v8 5/8] genirq/irq: introduce msi_doorbell_info Marc Zyngier <marc.zyngier@arm.com> - 2016-05-04 15:00 +0200
    [PATCH v8 1/8] genirq/msi: Add a new MSI_FLAG_IRQ_REMAPPING flag Eric Auger <eric.auger@linaro.org> - 2016-04-28 10:30 +0200
    [PATCH v8 3/8] genirq/msi: export msi_get_domain_info Eric Auger <eric.auger@linaro.org> - 2016-04-28 10:30 +0200
    [PATCH v8 7/8] genirq/msi: map/unmap the MSI doorbells on msi_domain_alloc/free_irqs Eric Auger <eric.auger@linaro.org> - 2016-04-28 10:30 +0200
      Re: [PATCH v8 7/8] genirq/msi: map/unmap the MSI doorbells on  msi_domain_alloc/free_irqs Marc Zyngier <marc.zyngier@arm.com> - 2016-05-04 15:30 +0200
        Re: [PATCH v8 7/8] genirq/msi: map/unmap the MSI doorbells on  msi_domain_alloc/free_irqs Eric Auger <eric.auger@linaro.org> - 2016-05-04 17:30 +0200
    [PATCH v8 2/8] irqchip/gic-v3-its: ITS advertises MSI_FLAG_IRQ_REMAPPING Eric Auger <eric.auger@linaro.org> - 2016-04-28 10:30 +0200

#1389862 — [PATCH v8 0/8] KVM PCIe/MSI passthrough on ARM/ARM64: kernel part 2/3: msi changes

This series implements the MSI address mapping/unmapping in the MSI layer.
IOMMU binding happens on pci_enable_msi since this function can sleep and
return errors. On msi_domain_set_affinity, msi_domain_(de)activate, which
are not allowed to sleep, we simply look for the already existing binding.

A new MSI domain info flag value is introduced to report whether the msi
domain implements IRQ remapping. GIC v3 ITS is the first MSI controller
advertising it. This flag value will be used by VFIO subsystem to
determine whether MSI forwarding is safe.

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

Best Regards

Eric

Applies on top of PART 1/3. Also depends on
[PATCH 1/3] iommu: Add MMIO mapping type,
http://comments.gmane.org/gmane.linux.kernel.iommu/12869

Git: complete series available at
https://git.linaro.org/people/eric.auger/linux.git/shortlog/refs/heads/v4.6-rc5-pcie-passthrough-v8

History:

v7 -> v8:
take into account Marc's comments:
- use iommu_msi_msg_pa_to_va with new proto
- change in irq_chip_msi_doorbell_info struct definition:
  prot and size became shared between all doorbells and phys_addr_t __percpu
- cleanups in v2m irqchip
- eventually did not touch MSI_FLAG_IRQ_REMAPPING naming
- On msi_handle_doorbell_mappings, stop on the first irqchip where doorbells
  can be found
- fix resource deallocation on mapping failure in msi_domain_alloc_irqs

v6 -> v7:
- do alloc/map handling on pci_enable_msi and search on msi_(de)domain_activate
- add msi_doorbell_info callback in irq-chip to retrieve the characteristics
  of doorbells

RFC v5 -> patch v6:
- split to ease the review process
- rebase on default iommu domain code (irq_data_to_msi_mapping_domain
  checks IOMMU_DOMAIN_DMA type)
- fix unmap sequence on msi_domain_set_affinity (reported by Marc):
  unmap the previous doorbell when the new one has been mapped & written to
  the device, ie. irq_chip_write_msi_msg.
- "msi: msi_compose wrapper removed" following change above
- add size parameter to iommu_get_reserved_iova API following Marc's request

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):
  genirq/msi: Add a new MSI_FLAG_IRQ_REMAPPING flag
  irqchip/gic-v3-its: ITS advertises MSI_FLAG_IRQ_REMAPPING
  genirq/msi: export msi_get_domain_info
  genirq/msi: msi_compose wrapper
  genirq/irq: introduce msi_doorbell_info
  irqchip/gicv2m: implement msi_doorbell_info callback
  genirq/msi: map/unmap the MSI doorbells on msi_domain_alloc/free_irqs
  genirq/msi: use the MSI doorbell's IOVA when requested

 drivers/irqchip/irq-gic-v2m.c                 |  30 ++++++-
 drivers/irqchip/irq-gic-v3-its-pci-msi.c      |   3 +-
 drivers/irqchip/irq-gic-v3-its-platform-msi.c |   3 +-
 include/linux/irq.h                           |  17 +++-
 include/linux/msi.h                           |   2 +
 kernel/irq/msi.c                              | 115 +++++++++++++++++++++++---
 6 files changed, 155 insertions(+), 15 deletions(-)

-- 
1.9.1

[toc] | [next] | [standalone]

#1389863 — [PATCH v8 4/8] genirq/msi: msi_compose wrapper

Currently the MSI message is composed by directly calling
irq_chip_compose_msi_msg and erased by setting the memory to zero.

On some platforms, we will need to complexify this composition to
properly handle MSI emission through IOMMU. Also we will need to track
when the MSI message is erased.

We propose to introduce a common wrapper for actual composition and
erasure, msi_compose.

Signed-off-by: Eric Auger <eric.auger@linaro.org>

---
v4 -> v5:
- just introduce the msi-compose wrapper without adding new
  functionalities

v3 -> v4:
- that code was formely in irq-gic-common.c
  "irqchip/gicv2m/v3-its-pci-msi: IOMMU map the MSI frame when needed"
  also the [un]mapping was done in irq_write_msi_msg; now done on compose

v2 -> v3:
- protect iova/addr manipulation with CONFIG_ARCH_DMA_ADDR_T_64BIT and
  CONFIG_PHYS_ADDR_T_64BIT
- only expose gic_pci_msi_domain_write_msg in case CONFIG_IOMMU_API &
  CONFIG_PCI_MSI_IRQ_DOMAIN are set.
- gic_set/unset_msi_addr duly become static
---
 kernel/irq/msi.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index 9b0ba4a..72bf4d6 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -55,6 +55,19 @@ static inline void irq_chip_write_msi_msg(struct irq_data *data,
 	data->chip->irq_write_msi_msg(data, msg);
 }
 
+static int msi_compose(struct irq_data *irq_data,
+		       struct msi_msg *msg, bool erase)
+{
+	int ret = 0;
+
+	if (erase)
+		memset(msg, 0, sizeof(*msg));
+	else
+		ret = irq_chip_compose_msi_msg(irq_data, msg);
+
+	return ret;
+}
+
 /**
  * msi_domain_set_affinity - Generic affinity setter function for MSI domains
  * @irq_data:	The irq data associated to the interrupt
@@ -73,7 +86,7 @@ int msi_domain_set_affinity(struct irq_data *irq_data,
 
 	ret = parent->chip->irq_set_affinity(parent, mask, force);
 	if (ret >= 0 && ret != IRQ_SET_MASK_OK_DONE) {
-		BUG_ON(irq_chip_compose_msi_msg(irq_data, &msg));
+		BUG_ON(msi_compose(irq_data, &msg, false));
 		irq_chip_write_msi_msg(irq_data, &msg);
 	}
 
@@ -85,7 +98,7 @@ static void msi_domain_activate(struct irq_domain *domain,
 {
 	struct msi_msg msg;
 
-	BUG_ON(irq_chip_compose_msi_msg(irq_data, &msg));
+	BUG_ON(msi_compose(irq_data, &msg, false));
 	irq_chip_write_msi_msg(irq_data, &msg);
 }
 
@@ -94,7 +107,7 @@ static void msi_domain_deactivate(struct irq_domain *domain,
 {
 	struct msi_msg msg;
 
-	memset(&msg, 0, sizeof(msg));
+	msi_compose(irq_data, &msg, true);
 	irq_chip_write_msi_msg(irq_data, &msg);
 }
 
-- 
1.9.1

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

#1389865 — [PATCH v8 5/8] genirq/irq: introduce msi_doorbell_info

The purpose is to be able to retrieve the MSI doorbells of an irqchip.
This is now needed since on some platforms those doorbells must be
iommu mapped (in case the MSIs transit through an IOMMU that do not
bypass those transactions).

The assumption is there is a maximum of one doorbell region per cpu.

A doorbell region is characterized by its physical address base, size and
IOMMU protection flag. Those 2 last characteristics are shared among all
doorbells.

irq_chip msi_doorbell_info callback enables to retrieve the doorbells of
the irqchip.

Signed-off-by: Eric Auger <eric.auger@linaro.org>

---
v7 -> v8:
- size and prot now are shared among all doorbells
- doorbells now directly points to a percpu phys_addr_t

v7: creation
---
 include/linux/irq.h | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index c4de623..5dbb26d 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -312,6 +312,19 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
 	return d->hwirq;
 }
 
+/*
+ * Describe all the MSI doorbell regions for an irqchip.
+ * A single doorbell region per cpu is assumed.
+ * In case a single doorbell is supported for the whole irqchip,
+ * the region is described in as cpu #0's one
+ */
+struct irq_chip_msi_doorbell_info {
+	phys_addr_t __percpu *percpu_doorbells; /* per cpu base address */
+	size_t size;				/* size of a each doorbell */
+	int prot;				/* iommu protection flag */
+	int nb_doorbells;
+};
+
 /**
  * struct irq_chip - hardware interrupt chip descriptor
  *
@@ -349,6 +362,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
  * @irq_get_irqchip_state:	return the internal state of an interrupt
  * @irq_set_irqchip_state:	set the internal state of a interrupt
  * @irq_set_vcpu_affinity:	optional to target a vCPU in a virtual machine
+ * @msi_doorbell_info:	return the MSI doorbell info
  * @ipi_send_single:	send a single IPI to destination cpus
  * @ipi_send_mask:	send an IPI to destination cpus in cpumask
  * @flags:		chip specific flags
@@ -394,7 +408,8 @@ struct irq_chip {
 	int		(*irq_set_irqchip_state)(struct irq_data *data, enum irqchip_irq_state which, bool state);
 
 	int		(*irq_set_vcpu_affinity)(struct irq_data *data, void *vcpu_info);
-
+	struct irq_chip_msi_doorbell_info *(*msi_doorbell_info)(
+							struct irq_data *data);
 	void		(*ipi_send_single)(struct irq_data *data, unsigned int cpu);
 	void		(*ipi_send_mask)(struct irq_data *data, const struct cpumask *dest);
 
-- 
1.9.1

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

#1394263 — Re: [PATCH v8 5/8] genirq/irq: introduce msi_doorbell_info

On 28/04/16 09:22, Eric Auger wrote:
> The purpose is to be able to retrieve the MSI doorbells of an irqchip.
> This is now needed since on some platforms those doorbells must be
> iommu mapped (in case the MSIs transit through an IOMMU that do not
> bypass those transactions).
> 
> The assumption is there is a maximum of one doorbell region per cpu.
> 
> A doorbell region is characterized by its physical address base, size and
> IOMMU protection flag. Those 2 last characteristics are shared among all
> doorbells.
> 
> irq_chip msi_doorbell_info callback enables to retrieve the doorbells of
> the irqchip.
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> ---
> v7 -> v8:
> - size and prot now are shared among all doorbells
> - doorbells now directly points to a percpu phys_addr_t
> 
> v7: creation
> ---
>  include/linux/irq.h | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index c4de623..5dbb26d 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -312,6 +312,19 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>  	return d->hwirq;
>  }
>  
> +/*
> + * Describe all the MSI doorbell regions for an irqchip.
> + * A single doorbell region per cpu is assumed.
> + * In case a single doorbell is supported for the whole irqchip,
> + * the region is described in as cpu #0's one
> + */
> +struct irq_chip_msi_doorbell_info {
> +	phys_addr_t __percpu *percpu_doorbells; /* per cpu base address */

Here's an idea: you could turn this field into a union:

	union {
		phys_addr_t __percpu *percpu_doorbells;
		phys_addr_t global_doorbell;
	};

> +	size_t size;				/* size of a each doorbell */
> +	int prot;				/* iommu protection flag */
> +	int nb_doorbells;

And this can be turned into a boolean:

	bool doorbell_is_percpu;

This allows you to avoid allocating a bunch of percpu doorbells for a
MSI controller that only has a global one. And the number of doorbells
is always either 1 or the number of CPUs anyway.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

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

#1389873 — [PATCH v8 1/8] genirq/msi: Add a new MSI_FLAG_IRQ_REMAPPING flag

Let's introduce a new msi_domain_info flag value, MSI_FLAG_IRQ_REMAPPING
meant to tell the domain supports IRQ REMAPPING, also known as Interrupt
Translation Service. On Intel HW this IRQ remapping capability is
abstracted on IOMMU side while on ARM it is abstracted on MSI controller
side. This flag will be used to know whether the MSI passthrough is
safe.

Signed-off-by: Eric Auger <eric.auger@linaro.org>

---

v4 -> v5:
- seperate flag introduction from first user addition (ITS)
---
 include/linux/msi.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/msi.h b/include/linux/msi.h
index 8b425c6..08441b1 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -270,6 +270,8 @@ enum {
 	MSI_FLAG_MULTI_PCI_MSI		= (1 << 3),
 	/* Support PCI MSIX interrupts */
 	MSI_FLAG_PCI_MSIX		= (1 << 4),
+	/* Support MSI IRQ remapping service */
+	MSI_FLAG_IRQ_REMAPPING		= (1 << 5),
 };
 
 int msi_domain_set_affinity(struct irq_data *data, const struct cpumask *mask,
-- 
1.9.1

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

#1389874 — [PATCH v8 3/8] genirq/msi: export msi_get_domain_info

We plan to use msi_get_domain_info in VFIO module so let's export it.

Signed-off-by: Eric Auger <eric.auger@linaro.org>

---

v2 -> v3:
- remove static implementation in case CONFIG_PCI_MSI_IRQ_DOMAIN is not set
---
 kernel/irq/msi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index 38e89ce..9b0ba4a 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -400,5 +400,6 @@ struct msi_domain_info *msi_get_domain_info(struct irq_domain *domain)
 {
 	return (struct msi_domain_info *)domain->host_data;
 }
+EXPORT_SYMBOL_GPL(msi_get_domain_info);
 
 #endif /* CONFIG_GENERIC_MSI_IRQ_DOMAIN */
-- 
1.9.1

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

#1389876 — [PATCH v8 7/8] genirq/msi: map/unmap the MSI doorbells on msi_domain_alloc/free_irqs

This patch handles the iommu mapping of MSI doorbells that require to
be mapped in an iommu domain. This happens on msi_domain_alloc/free_irqs
since this is called in code that can sleep (pci_enable/disable_msi):
iommu_map/unmap is not stated as atomic. On msi_domain_(de)activate and
msi_domain_set_affinity, which must be atomic, we just lookup for this
pre-allocated/mapped IOVA.

Signed-off-by: Eric Auger <eric.auger@linaro.org>

---
v7 -> v8:
- new percpu pointer type
- exit from the irq domain hierarchy parsing on first map/unmap success
- reset desc->irq to 0 on mapping failure

v7: creation
---
 kernel/irq/msi.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 79 insertions(+), 8 deletions(-)

diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index 72bf4d6..d5f95e6 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -14,6 +14,8 @@
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
 #include <linux/msi.h>
+#include <linux/msi-iommu.h>
+#include <linux/iommu.h>
 
 /* Temparory solution for building, will be removed later */
 #include <linux/pci.h>
@@ -322,6 +324,56 @@ int msi_domain_populate_irqs(struct irq_domain *domain, struct device *dev,
 }
 
 /**
+ * msi_handle_doorbell_mappings: in case the irq data corresponds to an
+ * MSI that requires iommu mapping, traverse the irq domain hierarchy
+ * to retrieve the doorbells to handle and iommu_map/unmap them according
+ * to @map boolean.
+ *
+ * @data: irq data handle
+ * @map: mapping if true, unmapping if false
+ */
+static int msi_handle_doorbell_mappings(struct irq_data *data, bool map)
+{
+	for (; data; data = data->parent_data) {
+		struct device *dev =
+			msi_desc_to_dev(irq_data_get_msi_desc(data));
+		struct irq_chip *chip = irq_data_get_irq_chip(data);
+		const struct irq_chip_msi_doorbell_info *dbinfo;
+		struct iommu_domain *domain;
+		phys_addr_t __percpu *db_addr;
+		dma_addr_t iova;
+		int ret = 0, i;
+
+		domain = iommu_msi_domain(dev);
+		if (!domain)
+			continue;
+
+		if (!chip->msi_doorbell_info)
+			continue;
+
+		dbinfo = chip->msi_doorbell_info(data);
+		if (!dbinfo)
+			return -EINVAL;
+
+		for (i = 0; i < dbinfo->nb_doorbells; i++) {
+			db_addr = per_cpu_ptr(dbinfo->percpu_doorbells, i);
+			if (map) {
+				ret = iommu_msi_get_doorbell_iova(domain,
+								  *db_addr,
+								  dbinfo->size,
+								  dbinfo->prot,
+								  &iova);
+				if (ret)
+					return ret;
+			} else
+				iommu_msi_put_doorbell_iova(domain, *db_addr);
+		}
+		break;
+	}
+	return 0;
+}
+
+/**
  * msi_domain_alloc_irqs - Allocate interrupts from a MSI interrupt domain
  * @domain:	The domain to allocate from
  * @dev:	Pointer to device struct of the device for which the interrupts
@@ -352,17 +404,26 @@ int msi_domain_alloc_irqs(struct irq_domain *domain, struct device *dev,
 
 		virq = __irq_domain_alloc_irqs(domain, virq, desc->nvec_used,
 					       dev_to_node(dev), &arg, false);
-		if (virq < 0) {
-			ret = -ENOSPC;
-			if (ops->handle_error)
-				ret = ops->handle_error(domain, desc, ret);
-			if (ops->msi_finish)
-				ops->msi_finish(&arg, ret);
-			return ret;
-		}
+		if (virq < 0)
+			goto error;
 
 		for (i = 0; i < desc->nvec_used; i++)
 			irq_set_msi_desc_off(virq, i, desc);
+
+		for (i = 0; i < desc->nvec_used; i++) {
+			ret = msi_handle_doorbell_mappings(
+				irq_get_irq_data(virq + i), true);
+			if (ret)
+				break;
+		}
+		if (ret) {
+			for (; i >= 0; i--)
+				msi_handle_doorbell_mappings(
+					irq_get_irq_data(virq + i), false);
+			irq_domain_free_irqs(virq, desc->nvec_used);
+			desc->irq = 0;
+			goto error;
+		}
 	}
 
 	if (ops->msi_finish)
@@ -377,6 +438,13 @@ int msi_domain_alloc_irqs(struct irq_domain *domain, struct device *dev,
 	}
 
 	return 0;
+error:
+	ret = -ENOSPC;
+	if (ops->handle_error)
+		ret = ops->handle_error(domain, desc, ret);
+	if (ops->msi_finish)
+		ops->msi_finish(&arg, ret);
+	return ret;
 }
 
 /**
@@ -396,6 +464,9 @@ void msi_domain_free_irqs(struct irq_domain *domain, struct device *dev)
 		 * entry. If that's the case, don't do anything.
 		 */
 		if (desc->irq) {
+			msi_handle_doorbell_mappings(
+				irq_get_irq_data(desc->irq),
+				false);
 			irq_domain_free_irqs(desc->irq, desc->nvec_used);
 			desc->irq = 0;
 		}
-- 
1.9.1

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

#1394284 — Re: [PATCH v8 7/8] genirq/msi: map/unmap the MSI doorbells on msi_domain_alloc/free_irqs

On 28/04/16 09:22, Eric Auger wrote:
> This patch handles the iommu mapping of MSI doorbells that require to
> be mapped in an iommu domain. This happens on msi_domain_alloc/free_irqs
> since this is called in code that can sleep (pci_enable/disable_msi):
> iommu_map/unmap is not stated as atomic. On msi_domain_(de)activate and
> msi_domain_set_affinity, which must be atomic, we just lookup for this
> pre-allocated/mapped IOVA.
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> ---
> v7 -> v8:
> - new percpu pointer type
> - exit from the irq domain hierarchy parsing on first map/unmap success
> - reset desc->irq to 0 on mapping failure
> 
> v7: creation
> ---
>  kernel/irq/msi.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 79 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
> index 72bf4d6..d5f95e6 100644
> --- a/kernel/irq/msi.c
> +++ b/kernel/irq/msi.c
> @@ -14,6 +14,8 @@
>  #include <linux/irq.h>
>  #include <linux/irqdomain.h>
>  #include <linux/msi.h>
> +#include <linux/msi-iommu.h>
> +#include <linux/iommu.h>
>  
>  /* Temparory solution for building, will be removed later */
>  #include <linux/pci.h>
> @@ -322,6 +324,56 @@ int msi_domain_populate_irqs(struct irq_domain *domain, struct device *dev,
>  }
>  
>  /**
> + * msi_handle_doorbell_mappings: in case the irq data corresponds to an
> + * MSI that requires iommu mapping, traverse the irq domain hierarchy
> + * to retrieve the doorbells to handle and iommu_map/unmap them according
> + * to @map boolean.
> + *
> + * @data: irq data handle
> + * @map: mapping if true, unmapping if false
> + */
> +static int msi_handle_doorbell_mappings(struct irq_data *data, bool map)
> +{
> +	for (; data; data = data->parent_data) {
> +		struct device *dev =
> +			msi_desc_to_dev(irq_data_get_msi_desc(data));
> +		struct irq_chip *chip = irq_data_get_irq_chip(data);
> +		const struct irq_chip_msi_doorbell_info *dbinfo;
> +		struct iommu_domain *domain;
> +		phys_addr_t __percpu *db_addr;
> +		dma_addr_t iova;
> +		int ret = 0, i;
> +
> +		domain = iommu_msi_domain(dev);
> +		if (!domain)
> +			continue;
> +
> +		if (!chip->msi_doorbell_info)
> +			continue;
> +
> +		dbinfo = chip->msi_doorbell_info(data);
> +		if (!dbinfo)
> +			return -EINVAL;
> +
> +		for (i = 0; i < dbinfo->nb_doorbells; i++) {
> +			db_addr = per_cpu_ptr(dbinfo->percpu_doorbells, i);
> +			if (map) {
> +				ret = iommu_msi_get_doorbell_iova(domain,
> +								  *db_addr,
> +								  dbinfo->size,
> +								  dbinfo->prot,
> +								  &iova);
> +				if (ret)
> +					return ret;
> +			} else
> +				iommu_msi_put_doorbell_iova(domain, *db_addr);
> +		}
> +		break;
> +	}
> +	return 0;
> +}

I'm really not fond of this whole loop. Could you try to decouple the
irq_data parsing (looking for a msi_doorbell_info method) from the
actual mapping/unmapping? This would make it a lot more readable.
Something along the lines of:

	struct device *dev;
	struct irq_chip *chip;
	struct iommu_domain *domain;
	const struct irq_chip_msi_doorbell_info *dbinfo;

	while (data) {
		dev = msi_desc_to_dev(irq_data_get_msi_desc(data));
		domain = iommu_msi_domain(dev);
		if (!domain)
			continue;

		chip = irq_data_get_irq_chip(data);
		if (chip->msi_doorbell_info)
			break;

		data = data->parent;
	}

	if (!data)
		return 0;

	dbinfo = chip->msi_doorbell_info(data);
	if (!dbinfo)
		return -EINVAL;

	[... handle mapping/unmapping here ...]

> +
> +/**
>   * msi_domain_alloc_irqs - Allocate interrupts from a MSI interrupt domain
>   * @domain:	The domain to allocate from
>   * @dev:	Pointer to device struct of the device for which the interrupts
> @@ -352,17 +404,26 @@ int msi_domain_alloc_irqs(struct irq_domain *domain, struct device *dev,
>  
>  		virq = __irq_domain_alloc_irqs(domain, virq, desc->nvec_used,
>  					       dev_to_node(dev), &arg, false);
> -		if (virq < 0) {
> -			ret = -ENOSPC;
> -			if (ops->handle_error)
> -				ret = ops->handle_error(domain, desc, ret);
> -			if (ops->msi_finish)
> -				ops->msi_finish(&arg, ret);
> -			return ret;
> -		}
> +		if (virq < 0)
> +			goto error;
>  
>  		for (i = 0; i < desc->nvec_used; i++)
>  			irq_set_msi_desc_off(virq, i, desc);
> +
> +		for (i = 0; i < desc->nvec_used; i++) {
> +			ret = msi_handle_doorbell_mappings(
> +				irq_get_irq_data(virq + i), true);

Do not be afraid of longer lines. Or if you are, create an intermediate
variable. But this kind of construct makes my brain work harder, and I
hate the feeling... ;-)

> +			if (ret)
> +				break;
> +		}
> +		if (ret) {
> +			for (; i >= 0; i--)
> +				msi_handle_doorbell_mappings(
> +					irq_get_irq_data(virq + i), false);
> +			irq_domain_free_irqs(virq, desc->nvec_used);
> +			desc->irq = 0;
> +			goto error;
> +		}
>  	}
>  
>  	if (ops->msi_finish)
> @@ -377,6 +438,13 @@ int msi_domain_alloc_irqs(struct irq_domain *domain, struct device *dev,
>  	}
>  
>  	return 0;
> +error:
> +	ret = -ENOSPC;
> +	if (ops->handle_error)
> +		ret = ops->handle_error(domain, desc, ret);
> +	if (ops->msi_finish)
> +		ops->msi_finish(&arg, ret);
> +	return ret;
>  }
>  
>  /**
> @@ -396,6 +464,9 @@ void msi_domain_free_irqs(struct irq_domain *domain, struct device *dev)
>  		 * entry. If that's the case, don't do anything.
>  		 */
>  		if (desc->irq) {
> +			msi_handle_doorbell_mappings(
> +				irq_get_irq_data(desc->irq),
> +				false);
>  			irq_domain_free_irqs(desc->irq, desc->nvec_used);
>  			desc->irq = 0;
>  		}
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

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

#1394456 — Re: [PATCH v8 7/8] genirq/msi: map/unmap the MSI doorbells on msi_domain_alloc/free_irqs

Hi Marc,
On 05/04/2016 03:21 PM, Marc Zyngier wrote:
> On 28/04/16 09:22, Eric Auger wrote:
>> This patch handles the iommu mapping of MSI doorbells that require to
>> be mapped in an iommu domain. This happens on msi_domain_alloc/free_irqs
>> since this is called in code that can sleep (pci_enable/disable_msi):
>> iommu_map/unmap is not stated as atomic. On msi_domain_(de)activate and
>> msi_domain_set_affinity, which must be atomic, we just lookup for this
>> pre-allocated/mapped IOVA.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>> v7 -> v8:
>> - new percpu pointer type
>> - exit from the irq domain hierarchy parsing on first map/unmap success
>> - reset desc->irq to 0 on mapping failure
>>
>> v7: creation
>> ---
>>  kernel/irq/msi.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++------
>>  1 file changed, 79 insertions(+), 8 deletions(-)
>>
>> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
>> index 72bf4d6..d5f95e6 100644
>> --- a/kernel/irq/msi.c
>> +++ b/kernel/irq/msi.c
>> @@ -14,6 +14,8 @@
>>  #include <linux/irq.h>
>>  #include <linux/irqdomain.h>
>>  #include <linux/msi.h>
>> +#include <linux/msi-iommu.h>
>> +#include <linux/iommu.h>
>>  
>>  /* Temparory solution for building, will be removed later */
>>  #include <linux/pci.h>
>> @@ -322,6 +324,56 @@ int msi_domain_populate_irqs(struct irq_domain *domain, struct device *dev,
>>  }
>>  
>>  /**
>> + * msi_handle_doorbell_mappings: in case the irq data corresponds to an
>> + * MSI that requires iommu mapping, traverse the irq domain hierarchy
>> + * to retrieve the doorbells to handle and iommu_map/unmap them according
>> + * to @map boolean.
>> + *
>> + * @data: irq data handle
>> + * @map: mapping if true, unmapping if false
>> + */
>> +static int msi_handle_doorbell_mappings(struct irq_data *data, bool map)
>> +{
>> +	for (; data; data = data->parent_data) {
>> +		struct device *dev =
>> +			msi_desc_to_dev(irq_data_get_msi_desc(data));
>> +		struct irq_chip *chip = irq_data_get_irq_chip(data);
>> +		const struct irq_chip_msi_doorbell_info *dbinfo;
>> +		struct iommu_domain *domain;
>> +		phys_addr_t __percpu *db_addr;
>> +		dma_addr_t iova;
>> +		int ret = 0, i;
>> +
>> +		domain = iommu_msi_domain(dev);
>> +		if (!domain)
>> +			continue;
>> +
>> +		if (!chip->msi_doorbell_info)
>> +			continue;
>> +
>> +		dbinfo = chip->msi_doorbell_info(data);
>> +		if (!dbinfo)
>> +			return -EINVAL;
>> +
>> +		for (i = 0; i < dbinfo->nb_doorbells; i++) {
>> +			db_addr = per_cpu_ptr(dbinfo->percpu_doorbells, i);
>> +			if (map) {
>> +				ret = iommu_msi_get_doorbell_iova(domain,
>> +								  *db_addr,
>> +								  dbinfo->size,
>> +								  dbinfo->prot,
>> +								  &iova);
>> +				if (ret)
>> +					return ret;
>> +			} else
>> +				iommu_msi_put_doorbell_iova(domain, *db_addr);
>> +		}
>> +		break;
>> +	}
>> +	return 0;
>> +}
> 
> I'm really not fond of this whole loop. Could you try to decouple the
> irq_data parsing (looking for a msi_doorbell_info method) from the
> actual mapping/unmapping? This would make it a lot more readable.
> Something along the lines of:
Just sent v9 where I addressed all your comments. Please let me know
whether this looks better.
> 
> 	struct device *dev;
> 	struct irq_chip *chip;
> 	struct iommu_domain *domain;
> 	const struct irq_chip_msi_doorbell_info *dbinfo;
> 
> 	while (data) {
> 		dev = msi_desc_to_dev(irq_data_get_msi_desc(data));
> 		domain = iommu_msi_domain(dev);
> 		if (!domain)
> 			continue;
> 
> 		chip = irq_data_get_irq_chip(data);
> 		if (chip->msi_doorbell_info)
> 			break;
> 
> 		data = data->parent;
> 	}
> 
> 	if (!data)
> 		return 0;
> 
> 	dbinfo = chip->msi_doorbell_info(data);
> 	if (!dbinfo)
> 		return -EINVAL;
> 
> 	[... handle mapping/unmapping here ...]
> 
>> +
>> +/**
>>   * msi_domain_alloc_irqs - Allocate interrupts from a MSI interrupt domain
>>   * @domain:	The domain to allocate from
>>   * @dev:	Pointer to device struct of the device for which the interrupts
>> @@ -352,17 +404,26 @@ int msi_domain_alloc_irqs(struct irq_domain *domain, struct device *dev,
>>  
>>  		virq = __irq_domain_alloc_irqs(domain, virq, desc->nvec_used,
>>  					       dev_to_node(dev), &arg, false);
>> -		if (virq < 0) {
>> -			ret = -ENOSPC;
>> -			if (ops->handle_error)
>> -				ret = ops->handle_error(domain, desc, ret);
>> -			if (ops->msi_finish)
>> -				ops->msi_finish(&arg, ret);
>> -			return ret;
>> -		}
>> +		if (virq < 0)
>> +			goto error;
>>  
>>  		for (i = 0; i < desc->nvec_used; i++)
>>  			irq_set_msi_desc_off(virq, i, desc);
>> +
>> +		for (i = 0; i < desc->nvec_used; i++) {
>> +			ret = msi_handle_doorbell_mappings(
>> +				irq_get_irq_data(virq + i), true);
> 
> Do not be afraid of longer lines. Or if you are, create an intermediate
> variable. But this kind of construct makes my brain work harder, and I
> hate the feeling... ;-)
Yes I am afraid of checkpatch and I do my utmost to abide by its law ;-)
Well, let me know if the v9 is of any relief for your brain ;-)

Thanks for your time!

Eric
> 
>> +			if (ret)
>> +				break;
>> +		}
>> +		if (ret) {
>> +			for (; i >= 0; i--)
>> +				msi_handle_doorbell_mappings(
>> +					irq_get_irq_data(virq + i), false);
>> +			irq_domain_free_irqs(virq, desc->nvec_used);
>> +			desc->irq = 0;
>> +			goto error;
>> +		}
>>  	}
>>  
>>  	if (ops->msi_finish)
>> @@ -377,6 +438,13 @@ int msi_domain_alloc_irqs(struct irq_domain *domain, struct device *dev,
>>  	}
>>  
>>  	return 0;
>> +error:
>> +	ret = -ENOSPC;
>> +	if (ops->handle_error)
>> +		ret = ops->handle_error(domain, desc, ret);
>> +	if (ops->msi_finish)
>> +		ops->msi_finish(&arg, ret);
>> +	return ret;
>>  }
>>  
>>  /**
>> @@ -396,6 +464,9 @@ void msi_domain_free_irqs(struct irq_domain *domain, struct device *dev)
>>  		 * entry. If that's the case, don't do anything.
>>  		 */
>>  		if (desc->irq) {
>> +			msi_handle_doorbell_mappings(
>> +				irq_get_irq_data(desc->irq),
>> +				false);
>>  			irq_domain_free_irqs(desc->irq, desc->nvec_used);
>>  			desc->irq = 0;
>>  		}
>>
> 
> Thanks,
> 
> 	M.
> 

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

#1389877 — [PATCH v8 2/8] irqchip/gic-v3-its: ITS advertises MSI_FLAG_IRQ_REMAPPING

The ITS is the first ARM MSI controller advertising the new
MSI_FLAG_IRQ_REMAPPING flag. It does so because it supports
interrupt translation service. This HW support offers isolation
of MSIs, feature used when using KVM device passthrough.

Signed-off-by: Eric Auger <eric.auger@linaro.org>

---

v5: new
---
 drivers/irqchip/irq-gic-v3-its-pci-msi.c      | 3 ++-
 drivers/irqchip/irq-gic-v3-its-platform-msi.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
index aee60ed..8223765 100644
--- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
+++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
@@ -96,7 +96,8 @@ static struct msi_domain_ops its_pci_msi_ops = {
 
 static struct msi_domain_info its_pci_msi_domain_info = {
 	.flags	= (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
-		   MSI_FLAG_MULTI_PCI_MSI | MSI_FLAG_PCI_MSIX),
+		   MSI_FLAG_MULTI_PCI_MSI | MSI_FLAG_PCI_MSIX |
+		   MSI_FLAG_IRQ_REMAPPING),
 	.ops	= &its_pci_msi_ops,
 	.chip	= &its_msi_irq_chip,
 };
diff --git a/drivers/irqchip/irq-gic-v3-its-platform-msi.c b/drivers/irqchip/irq-gic-v3-its-platform-msi.c
index 470b4aa..8c0d69d 100644
--- a/drivers/irqchip/irq-gic-v3-its-platform-msi.c
+++ b/drivers/irqchip/irq-gic-v3-its-platform-msi.c
@@ -63,7 +63,8 @@ static struct msi_domain_ops its_pmsi_ops = {
 };
 
 static struct msi_domain_info its_pmsi_domain_info = {
-	.flags	= (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS),
+	.flags	= (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
+		   MSI_FLAG_IRQ_REMAPPING),
 	.ops	= &its_pmsi_ops,
 	.chip	= &its_pmsi_irq_chip,
 };
-- 
1.9.1

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web