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

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

[Eric Auger]

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

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

[Eric Auger]

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

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

[Eric Auger]

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

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

[Eric Auger]

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

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

[Eric Auger]

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

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

[Eric Auger]

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

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

[Marc Zyngier]

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...

[PATCH v8 6/8] irqchip/gicv2m: implement msi_doorbell_info callback

[Eric Auger]

This patch implements the msi_doorbell_info callback in the
gicv2m driver.

The driver now is able to return its doorbell characteristics
(base, size, prot). A single doorbell is exposed.

This will allow the msi layer to iommu_map this doorbell when
requested.

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

---

v7 -> v8:
- gicv2m_msi_doorbell_info does not return a pointer to const
- remove spurious !v2m check
- add IOMMU_MMIO flag

v7: creation
---
 drivers/irqchip/irq-gic-v2m.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-gic-v2m.c b/drivers/irqchip/irq-gic-v2m.c
index 28f047c..a1ea40c 100644
--- a/drivers/irqchip/irq-gic-v2m.c
+++ b/drivers/irqchip/irq-gic-v2m.c
@@ -24,6 +24,8 @@
 #include <linux/of_pci.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
+#include <linux/percpu.h>
+#include <linux/iommu.h>
 
 /*
 * MSI_TYPER:
@@ -64,6 +66,7 @@ struct v2m_data {
 	u32 nr_spis;		/* The number of SPIs for MSIs */
 	unsigned long *bm;	/* MSI vector bitmap */
 	u32 flags;		/* v2m flags for specific implementation */
+	struct irq_chip_msi_doorbell_info doorbell_info;
 };
 
 static void gicv2m_mask_msi_irq(struct irq_data *d)
@@ -105,6 +108,14 @@ static void gicv2m_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
 		msg->data -= v2m->spi_start;
 }
 
+static inline struct irq_chip_msi_doorbell_info *
+gicv2m_msi_doorbell_info(struct irq_data *data)
+{
+	struct v2m_data *v2m = irq_data_get_irq_chip_data(data);
+
+	return &v2m->doorbell_info;
+}
+
 static struct irq_chip gicv2m_irq_chip = {
 	.name			= "GICv2m",
 	.irq_mask		= irq_chip_mask_parent,
@@ -112,6 +123,7 @@ static struct irq_chip gicv2m_irq_chip = {
 	.irq_eoi		= irq_chip_eoi_parent,
 	.irq_set_affinity	= irq_chip_set_affinity_parent,
 	.irq_compose_msi_msg	= gicv2m_compose_msi_msg,
+	.msi_doorbell_info	= gicv2m_msi_doorbell_info,
 };
 
 static int gicv2m_irq_gic_domain_alloc(struct irq_domain *domain,
@@ -247,6 +259,7 @@ static void gicv2m_teardown(void)
 
 	list_for_each_entry_safe(v2m, tmp, &v2m_nodes, entry) {
 		list_del(&v2m->entry);
+		free_percpu(v2m->doorbell_info.percpu_doorbells);
 		kfree(v2m->bm);
 		iounmap(v2m->base);
 		of_node_put(to_of_node(v2m->fwnode));
@@ -299,6 +312,7 @@ static int __init gicv2m_init_one(struct fwnode_handle *fwnode,
 {
 	int ret;
 	struct v2m_data *v2m;
+	phys_addr_t __percpu *doorbell;
 
 	v2m = kzalloc(sizeof(struct v2m_data), GFP_KERNEL);
 	if (!v2m) {
@@ -311,11 +325,23 @@ static int __init gicv2m_init_one(struct fwnode_handle *fwnode,
 
 	memcpy(&v2m->res, res, sizeof(struct resource));
 
+	v2m->doorbell_info.percpu_doorbells =
+		alloc_percpu(phys_addr_t);
+	if (WARN_ON(!v2m->doorbell_info.percpu_doorbells)) {
+		ret = -ENOMEM;
+		goto err_free_v2m;
+	}
+	doorbell = per_cpu_ptr(v2m->doorbell_info.percpu_doorbells, 0);
+	*doorbell = v2m->res.start;
+	v2m->doorbell_info.size = 4;
+	v2m->doorbell_info.prot = IOMMU_WRITE | IOMMU_MMIO;
+	v2m->doorbell_info.nb_doorbells = 1;
+
 	v2m->base = ioremap(v2m->res.start, resource_size(&v2m->res));
 	if (!v2m->base) {
 		pr_err("Failed to map GICv2m resource\n");
 		ret = -ENOMEM;
-		goto err_free_v2m;
+		goto err_free_v2m_doorbells;
 	}
 
 	if (spi_start && nr_spis) {
@@ -359,6 +385,8 @@ static int __init gicv2m_init_one(struct fwnode_handle *fwnode,
 
 err_iounmap:
 	iounmap(v2m->base);
+err_free_v2m_doorbells:
+	free_percpu(v2m->doorbell_info.percpu_doorbells);
 err_free_v2m:
 	kfree(v2m);
 	return ret;
-- 
1.9.1

[PATCH v8 6/8] irqchip/gicv2m: implement msi_doorbell_info callback

[Marc Zyngier]

On 28/04/16 09:22, Eric Auger wrote:
> This patch implements the msi_doorbell_info callback in the
> gicv2m driver.
> 
> The driver now is able to return its doorbell characteristics
> (base, size, prot). A single doorbell is exposed.
> 
> This will allow the msi layer to iommu_map this doorbell when
> requested.
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> ---
> 
> v7 -> v8:
> - gicv2m_msi_doorbell_info does not return a pointer to const
> - remove spurious !v2m check
> - add IOMMU_MMIO flag
> 
> v7: creation
> ---
>  drivers/irqchip/irq-gic-v2m.c | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v2m.c b/drivers/irqchip/irq-gic-v2m.c
> index 28f047c..a1ea40c 100644
> --- a/drivers/irqchip/irq-gic-v2m.c
> +++ b/drivers/irqchip/irq-gic-v2m.c
> @@ -24,6 +24,8 @@
>  #include <linux/of_pci.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
> +#include <linux/percpu.h>
> +#include <linux/iommu.h>
>  
>  /*
>  * MSI_TYPER:
> @@ -64,6 +66,7 @@ struct v2m_data {
>  	u32 nr_spis;		/* The number of SPIs for MSIs */
>  	unsigned long *bm;	/* MSI vector bitmap */
>  	u32 flags;		/* v2m flags for specific implementation */
> +	struct irq_chip_msi_doorbell_info doorbell_info;
>  };
>  
>  static void gicv2m_mask_msi_irq(struct irq_data *d)
> @@ -105,6 +108,14 @@ static void gicv2m_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
>  		msg->data -= v2m->spi_start;
>  }
>  
> +static inline struct irq_chip_msi_doorbell_info *

You do not need an inline here, specially given that you're assigning
its pointer to a structure just below.

> +gicv2m_msi_doorbell_info(struct irq_data *data)
> +{
> +	struct v2m_data *v2m = irq_data_get_irq_chip_data(data);
> +
> +	return &v2m->doorbell_info;
> +}
> +
>  static struct irq_chip gicv2m_irq_chip = {
>  	.name			= "GICv2m",
>  	.irq_mask		= irq_chip_mask_parent,
> @@ -112,6 +123,7 @@ static struct irq_chip gicv2m_irq_chip = {
>  	.irq_eoi		= irq_chip_eoi_parent,
>  	.irq_set_affinity	= irq_chip_set_affinity_parent,
>  	.irq_compose_msi_msg	= gicv2m_compose_msi_msg,
> +	.msi_doorbell_info	= gicv2m_msi_doorbell_info,
>  };
>  
>  static int gicv2m_irq_gic_domain_alloc(struct irq_domain *domain,
> @@ -247,6 +259,7 @@ static void gicv2m_teardown(void)
>  
>  	list_for_each_entry_safe(v2m, tmp, &v2m_nodes, entry) {
>  		list_del(&v2m->entry);
> +		free_percpu(v2m->doorbell_info.percpu_doorbells);
>  		kfree(v2m->bm);
>  		iounmap(v2m->base);
>  		of_node_put(to_of_node(v2m->fwnode));
> @@ -299,6 +312,7 @@ static int __init gicv2m_init_one(struct fwnode_handle *fwnode,
>  {
>  	int ret;
>  	struct v2m_data *v2m;
> +	phys_addr_t __percpu *doorbell;
>  
>  	v2m = kzalloc(sizeof(struct v2m_data), GFP_KERNEL);
>  	if (!v2m) {
> @@ -311,11 +325,23 @@ static int __init gicv2m_init_one(struct fwnode_handle *fwnode,
>  
>  	memcpy(&v2m->res, res, sizeof(struct resource));
>  
> +	v2m->doorbell_info.percpu_doorbells =
> +		alloc_percpu(phys_addr_t);
> +	if (WARN_ON(!v2m->doorbell_info.percpu_doorbells)) {
> +		ret = -ENOMEM;
> +		goto err_free_v2m;
> +	}
> +	doorbell = per_cpu_ptr(v2m->doorbell_info.percpu_doorbells, 0);
> +	*doorbell = v2m->res.start;
> +	v2m->doorbell_info.size = 4;
> +	v2m->doorbell_info.prot = IOMMU_WRITE | IOMMU_MMIO;
> +	v2m->doorbell_info.nb_doorbells = 1;


So if you take my earlier suggestion, this becomes:

	v2m->doorbell_info.global_doorbell = v2m->res.start;
	v2m->doorbell_info.size = sizeof(u32);
	v2m->doorbell_info.doorbell_is_percpu = false;

a bit nicer.

> +
>  	v2m->base = ioremap(v2m->res.start, resource_size(&v2m->res));
>  	if (!v2m->base) {
>  		pr_err("Failed to map GICv2m resource\n");
>  		ret = -ENOMEM;
> -		goto err_free_v2m;
> +		goto err_free_v2m_doorbells;
>  	}
>  
>  	if (spi_start && nr_spis) {
> @@ -359,6 +385,8 @@ static int __init gicv2m_init_one(struct fwnode_handle *fwnode,
>  
>  err_iounmap:
>  	iounmap(v2m->base);
> +err_free_v2m_doorbells:
> +	free_percpu(v2m->doorbell_info.percpu_doorbells);
>  err_free_v2m:
>  	kfree(v2m);
>  	return ret;
> 

Thanks,

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

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

[Eric Auger]

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

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

[Marc Zyngier]

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...

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

[Eric Auger]

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.
> 

[PATCH v8 8/8] genirq/msi: use the MSI doorbell's IOVA when requested

[Eric Auger]

On MSI message composition we now use the MSI doorbell's IOVA in
place of the doorbell's PA in case the device is upstream to an
IOMMU that requires MSI addresses to be mapped. The doorbell's
allocation and mapping happened on an early stage (pci_enable_msi).

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

---
v7 -> v8:
- use iommu_msi_msg_pa_to_va
- add WARN_ON

v6 -> v7:
- allocation/mapping is done at an earlier stage. We now just perform
  the iova lookup. So it is safe now to be called in a code that cannot
  sleep. iommu_msi_set_doorbell_iova is moved in the dma-reserved-iommu
  API: I think it cleans things up with respect to various #ifdef CONFIGS.

v5:
- use macros to increase the readability
- add comments
- fix a typo that caused a compilation error if CONFIG_IOMMU_API
  is not set
---
 kernel/irq/msi.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index d5f95e6..835938b 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -64,8 +64,16 @@ static int msi_compose(struct irq_data *irq_data,
 
 	if (erase)
 		memset(msg, 0, sizeof(*msg));
-	else
+	else {
+		struct device *dev;
+
 		ret = irq_chip_compose_msi_msg(irq_data, msg);
+		if (ret)
+			return ret;
+
+		dev = msi_desc_to_dev(irq_data_get_msi_desc(irq_data));
+		WARN_ON(iommu_msi_msg_pa_to_va(dev, msg));
+	}
 
 	return ret;
 }
-- 
1.9.1

[PATCH v8 8/8] genirq/msi: use the MSI doorbell's IOVA when requested

[Marc Zyngier]

On 28/04/16 09:22, Eric Auger wrote:
> On MSI message composition we now use the MSI doorbell's IOVA in
> place of the doorbell's PA in case the device is upstream to an
> IOMMU that requires MSI addresses to be mapped. The doorbell's
> allocation and mapping happened on an early stage (pci_enable_msi).
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> ---
> v7 -> v8:
> - use iommu_msi_msg_pa_to_va
> - add WARN_ON
> 
> v6 -> v7:
> - allocation/mapping is done at an earlier stage. We now just perform
>   the iova lookup. So it is safe now to be called in a code that cannot
>   sleep. iommu_msi_set_doorbell_iova is moved in the dma-reserved-iommu
>   API: I think it cleans things up with respect to various #ifdef CONFIGS.
> 
> v5:
> - use macros to increase the readability
> - add comments
> - fix a typo that caused a compilation error if CONFIG_IOMMU_API
>   is not set
> ---
>  kernel/irq/msi.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
> index d5f95e6..835938b 100644
> --- a/kernel/irq/msi.c
> +++ b/kernel/irq/msi.c
> @@ -64,8 +64,16 @@ static int msi_compose(struct irq_data *irq_data,
>  
>  	if (erase)
>  		memset(msg, 0, sizeof(*msg));
> -	else

Nit: Braces on both sides of the 'else'.

> +	else {
> +		struct device *dev;
> +
>  		ret = irq_chip_compose_msi_msg(irq_data, msg);
> +		if (ret)
> +			return ret;
> +
> +		dev = msi_desc_to_dev(irq_data_get_msi_desc(irq_data));
> +		WARN_ON(iommu_msi_msg_pa_to_va(dev, msg));
> +	}
>  
>  	return ret;
>  }
> 

Thanks,

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