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


Groups > linux.kernel > #1446435 > unrolled thread

[PATCH v11 00/10] KVM PCIe/MSI passthrough on ARM/ARM64: kernel part 2/3: msi changes

Started byEric Auger <eric.auger@redhat.com>
First post2016-07-19 15:10 +0200
Last post2016-07-19 15:10 +0200
Articles 15 — 3 participants

Back to article view | Back to linux.kernel


Contents

  [PATCH v11 00/10] KVM PCIe/MSI passthrough on ARM/ARM64: kernel part 2/3: msi changes Eric Auger <eric.auger@redhat.com> - 2016-07-19 15:10 +0200
    [PATCH v11 02/10] genirq/msi: msi_compose wrapper Eric Auger <eric.auger@redhat.com> - 2016-07-19 15:10 +0200
    [PATCH v11 09/10] genirq/msi: map/unmap the MSI doorbells on msi_domain_alloc/free_irqs Eric Auger <eric.auger@redhat.com> - 2016-07-19 15:10 +0200
      Re: [PATCH v11 09/10] genirq/msi: map/unmap the MSI doorbells on  msi_domain_alloc/free_irqs Thomas Gleixner <tglx@linutronix.de> - 2016-07-20 11:10 +0200
        Re: [PATCH v11 09/10] genirq/msi: map/unmap the MSI doorbells on  msi_domain_alloc/free_irqs Auger Eric <eric.auger@redhat.com> - 2016-07-25 18:30 +0200
    [PATCH v11 10/10] genirq/msi: use the MSI doorbell's IOVA when requested Eric Auger <eric.auger@redhat.com> - 2016-07-19 15:10 +0200
      Re: [PATCH v11 10/10] genirq/msi: use the MSI doorbell's IOVA when  requested Thomas Gleixner <tglx@linutronix.de> - 2016-07-20 11:20 +0200
        Re: [PATCH v11 10/10] genirq/msi: use the MSI doorbell's IOVA when  requested Auger Eric <eric.auger@redhat.com> - 2016-07-25 18:40 +0200
    [PATCH v11 08/10] irqchip/gicv3-its: register the MSI global doorbell Eric Auger <eric.auger@redhat.com> - 2016-07-19 15:10 +0200
    [PATCH v11 06/10] genirq/msi-doorbell: msi_doorbell_safe Eric Auger <eric.auger@redhat.com> - 2016-07-19 15:10 +0200
      Re: [PATCH v11 06/10] genirq/msi-doorbell: msi_doorbell_safe Thomas Gleixner <tglx@linutronix.de> - 2016-07-20 10:20 +0200
        Re: [PATCH v11 06/10] genirq/msi-doorbell: msi_doorbell_safe Auger Eric <eric.auger@redhat.com> - 2016-07-21 15:40 +0200
          Re: [PATCH v11 06/10] genirq/msi-doorbell: msi_doorbell_safe Thomas Gleixner <tglx@linutronix.de> - 2016-07-22 14:50 +0200
            Re: [PATCH v11 06/10] genirq/msi-doorbell: msi_doorbell_safe Auger Eric <eric.auger@redhat.com> - 2016-07-22 16:10 +0200
    [PATCH v11 01/10] genirq/msi: export msi_get_domain_info Eric Auger <eric.auger@redhat.com> - 2016-07-19 15:10 +0200

#1446435 — [PATCH v11 00/10] KVM PCIe/MSI passthrough on ARM/ARM64: kernel part 2/3: msi changes

FromEric Auger <eric.auger@redhat.com>
Date2016-07-19 15:10 +0200
Subject[PATCH v11 00/10] KVM PCIe/MSI passthrough on ARM/ARM64: kernel part 2/3: msi changes
Message-ID<rWCWB-4qE-3@gated-at.bofh.it>
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.

Irqchips likely to be downstream to iommus (not bypassing MSIs) are supposed
to register their MSI doorbells. This make possible to retrieve their
characteristics, detect whether MSI assignment is safe and report to the
userspace the size/alignment of the guest PA window to provision for MSI
mapping.

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

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

History:

v10 -> v11:
- restored irq_chip msi_doorbell_info since lookup function introduced
  in v10 (taking the chip_data as parameter) did not work for ITS and
  most probably for other irqchips/
- changed the registration API
- eventually tested with GICv3 ITS

v9 -> v10:
- was forced to introduce important changes on parts that were reviewed
  already :-( I took the initiative to replace the irqchip's
  get_doorbell_info callback by a new API, msi-doorbell).
  the new API makes possible to register, lookup doorbells and also compute
  the total requirements and IRQ safety flag used by VFIO.
- also added code in GICv3 ITS to register a global doorbell.

v8 -> v9:
- use a union in irq_chip_msi_doorbell_info + boolean telling whether the
  doorbell is percpu
- decouple irq_data parsing from the actual mapping/unmapping in
  msi_handle_doorbell_mappings
- fix misc style issues

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 (10):
  genirq/msi: export msi_get_domain_info
  genirq/msi: msi_compose wrapper
  genirq/irq: introduce msi_doorbell_info
  genirq/msi-doorbell: allow MSI doorbell (un)registration
  genirq/msi-doorbell: msi_doorbell_pages
  genirq/msi-doorbell: msi_doorbell_safe
  irqchip/gicv2m: register the MSI global doorbell
  irqchip/gicv3-its: register the MSI global doorbell
  genirq/msi: map/unmap the MSI doorbells on msi_domain_alloc/free_irqs
  genirq/msi: use the MSI doorbell's IOVA when requested

 drivers/iommu/Kconfig            |   1 +
 drivers/irqchip/irq-gic-v2m.c    |  23 ++++++-
 drivers/irqchip/irq-gic-v3-its.c |  28 ++++++++
 include/linux/irq.h              |  16 ++++-
 include/linux/msi-doorbell.h     |  78 ++++++++++++++++++++++
 kernel/irq/Kconfig               |   4 ++
 kernel/irq/Makefile              |   1 +
 kernel/irq/msi-doorbell.c        | 129 ++++++++++++++++++++++++++++++++++++
 kernel/irq/msi.c                 | 137 +++++++++++++++++++++++++++++++++++----
 9 files changed, 404 insertions(+), 13 deletions(-)
 create mode 100644 include/linux/msi-doorbell.h
 create mode 100644 kernel/irq/msi-doorbell.c

-- 
1.9.1

[toc] | [next] | [standalone]


#1446437 — [PATCH v11 02/10] genirq/msi: msi_compose wrapper

FromEric Auger <eric.auger@redhat.com>
Date2016-07-19 15:10 +0200
Subject[PATCH v11 02/10] genirq/msi: msi_compose wrapper
Message-ID<rWCWC-4qE-21@gated-at.bofh.it>
In reply to#1446435
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@redhat.com>

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


#1446442 — [PATCH v11 09/10] genirq/msi: map/unmap the MSI doorbells on msi_domain_alloc/free_irqs

FromEric Auger <eric.auger@redhat.com>
Date2016-07-19 15:10 +0200
Subject[PATCH v11 09/10] genirq/msi: map/unmap the MSI doorbells on msi_domain_alloc/free_irqs
Message-ID<rWCWC-4qE-35@gated-at.bofh.it>
In reply to#1446435
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@redhat.com>

---

v10 -> v11:
- restore v9 version based on irq_chip msi_doorbell_info

v9 -> v10:
- use irqchip API to lookup for the chip_data's doorbell

v8 -> v9:
- decouple irq_data parsing from the actual mapping/unmapping

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 | 109 +++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 101 insertions(+), 8 deletions(-)

diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index 72bf4d6..69b5b19 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -14,6 +14,9 @@
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
 #include <linux/msi.h>
+#include <linux/msi-iommu.h>
+#include <linux/iommu.h>
+#include <linux/msi-doorbell.h>
 
 /* Temparory solution for building, will be removed later */
 #include <linux/pci.h>
@@ -322,6 +325,74 @@ 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)
+{
+	const struct irq_chip_msi_doorbell_info *dbinfo;
+	struct iommu_domain *domain;
+	struct irq_chip *chip;
+	struct device *dev;
+	dma_addr_t iova;
+	int ret = 0, cpu;
+
+	while (data) {
+		dev = msi_desc_to_dev(irq_data_get_msi_desc(data));
+		domain = iommu_msi_domain(dev);
+		if (domain) {
+			chip = irq_data_get_irq_chip(data);
+			if (chip->msi_doorbell_info)
+				break;
+		}
+		data = data->parent_data;
+	}
+
+	if (!data)
+		return 0;
+
+	dbinfo = chip->msi_doorbell_info(data);
+	if (!dbinfo)
+		return -EINVAL;
+
+	if (!dbinfo->doorbell_is_percpu) {
+		if (!map) {
+			iommu_msi_put_doorbell_iova(domain,
+						    dbinfo->global_doorbell);
+			return 0;
+		}
+		return iommu_msi_get_doorbell_iova(domain,
+						   dbinfo->global_doorbell,
+						   dbinfo->size, dbinfo->prot,
+						   &iova);
+	}
+
+	/* percpu doorbells */
+	for_each_possible_cpu(cpu) {
+		phys_addr_t __percpu *db_addr =
+			per_cpu_ptr(dbinfo->percpu_doorbells, cpu);
+
+		if (!map) {
+			iommu_msi_put_doorbell_iova(domain, *db_addr);
+		} else {
+
+			ret = iommu_msi_get_doorbell_iova(domain, *db_addr,
+							  dbinfo->size,
+							  dbinfo->prot, &iova);
+			if (ret)
+				return ret;
+		}
+	}
+
+	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 +423,29 @@ 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++) {
+			struct irq_data *d = irq_get_irq_data(virq + i);
+
+			ret = msi_handle_doorbell_mappings(d, true);
+			if (ret)
+				break;
+		}
+		if (ret) {
+			for (; i >= 0; i--) {
+				struct irq_data *d = irq_get_irq_data(virq + i);
+
+				msi_handle_doorbell_mappings(d, false);
+			}
+			irq_domain_free_irqs(virq, desc->nvec_used);
+			desc->irq = 0;
+			goto error;
+		}
 	}
 
 	if (ops->msi_finish)
@@ -377,6 +460,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 +486,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]


#1447117 — Re: [PATCH v11 09/10] genirq/msi: map/unmap the MSI doorbells on msi_domain_alloc/free_irqs

FromThomas Gleixner <tglx@linutronix.de>
Date2016-07-20 11:10 +0200
SubjectRe: [PATCH v11 09/10] genirq/msi: map/unmap the MSI doorbells on msi_domain_alloc/free_irqs
Message-ID<rWVFT-7RJ-15@gated-at.bofh.it>
In reply to#1446442
On Tue, 19 Jul 2016, Eric Auger wrote:
>  /**
> + * 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
> + */


Please run that through the kernel doc generator. It does not work that way.

The format is:

/**
 * function_name - Short function description    
 * @arg1:	Description of arg1
 * @argument2:	Description of argument2
 *
 * Long explanation including documentation of the return values.
 */

> +static int msi_handle_doorbell_mappings(struct irq_data *data, bool map)
> +{
> +	const struct irq_chip_msi_doorbell_info *dbinfo;
> +	struct iommu_domain *domain;
> +	struct irq_chip *chip;
> +	struct device *dev;
> +	dma_addr_t iova;
> +	int ret = 0, cpu;
> +
> +	while (data) {
> +		dev = msi_desc_to_dev(irq_data_get_msi_desc(data));
> +		domain = iommu_msi_domain(dev);
> +		if (domain) {
> +			chip = irq_data_get_irq_chip(data);
> +			if (chip->msi_doorbell_info)
> +				break;
> +		}
> +		data = data->parent_data;
> +	}

Please split that out into a seperate function

struct irq_data *msi_get_doorbell_info(data)
{
	.....
		if (chip->msi_doorbell_info)
			return chip->msi_get_doorbell_info(data);
	}
	return NULL;
}

       info = msi_get_doorbell_info(data);
       .....

> +	if (!data)
> +		return 0;
> +
> +	dbinfo = chip->msi_doorbell_info(data);
> +	if (!dbinfo)
> +		return -EINVAL;
> +
> +	if (!dbinfo->doorbell_is_percpu) {
> +		if (!map) {
> +			iommu_msi_put_doorbell_iova(domain,
> +						    dbinfo->global_doorbell);
> +			return 0;
> +		}
> +		return iommu_msi_get_doorbell_iova(domain,
> +						   dbinfo->global_doorbell,
> +						   dbinfo->size, dbinfo->prot,
> +						   &iova);
> +	}

You can spare an indentation level with a helper function

    	if (!dbinfo->doorbell_is_percpu)
		return msi_map_global_doorbell(domain, dbinfo);

> +
> +	/* percpu doorbells */
> +	for_each_possible_cpu(cpu) {
> +		phys_addr_t __percpu *db_addr =
> +			per_cpu_ptr(dbinfo->percpu_doorbells, cpu);
> +
> +		if (!map) {
> +			iommu_msi_put_doorbell_iova(domain, *db_addr);
> +		} else {
> +
> +			ret = iommu_msi_get_doorbell_iova(domain, *db_addr,
> +							  dbinfo->size,
> +							  dbinfo->prot, &iova);
> +			if (ret)
> +				return ret;
> +		}
> +	}

Same here:

	for_each_possible_cpu(cpu) {
		ret = msi_map_percpu_doorbell(domain, cpu);
		if (ret)
			return ret;
	}
     	return 0;
     
Hmm?

> +
> +	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 +423,29 @@ 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++) {
> +			struct irq_data *d = irq_get_irq_data(virq + i);
> +
> +			ret = msi_handle_doorbell_mappings(d, true);
> +			if (ret)
> +				break;
> +		}
> +		if (ret) {
> +			for (; i >= 0; i--) {
> +				struct irq_data *d = irq_get_irq_data(virq + i);
> +
> +				msi_handle_doorbell_mappings(d, false);
> +			}
> +			irq_domain_free_irqs(virq, desc->nvec_used);
> +			desc->irq = 0;
> +			goto error;

How is that supposed to work? You clear desc->irq and then you call
ops->handle_error.

Why are you adding this extra stuff here? Look at the call sites of
msi_domain_alloc_irqs(). All of them use msi_domain_free_irqs() in case of
error. There is no reason why you can't do the same....

>  /**
> @@ -396,6 +486,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;

Can you please restructure the code so it reads

		if (desc->irq)
			continue;

		msi_handle_doorbell_mappings(irq_get_irq_data(desc->irq),
					     false);	
		irq_domain_free_irqs(desc->irq, desc->nvec_used);
		desc->irq = 0;

Just blindly whacking stuff into the 80 char limit is not helping readability.

Thanks,

	tglx

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


#1449642 — Re: [PATCH v11 09/10] genirq/msi: map/unmap the MSI doorbells on msi_domain_alloc/free_irqs

FromAuger Eric <eric.auger@redhat.com>
Date2016-07-25 18:30 +0200
SubjectRe: [PATCH v11 09/10] genirq/msi: map/unmap the MSI doorbells on msi_domain_alloc/free_irqs
Message-ID<rYQVs-6No-51@gated-at.bofh.it>
In reply to#1447117
Hi Thomas,

On 20/07/2016 11:04, Thomas Gleixner wrote:
> On Tue, 19 Jul 2016, Eric Auger wrote:
>>  /**
>> + * 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
>> + */
> 
> 
> Please run that through the kernel doc generator. It does not work that way.
> 
> The format is:
> 
> /**
>  * function_name - Short function description    
>  * @arg1:	Description of arg1
>  * @argument2:	Description of argument2
>  *
>  * Long explanation including documentation of the return values.
>  */
> 
>> +static int msi_handle_doorbell_mappings(struct irq_data *data, bool map)
>> +{
>> +	const struct irq_chip_msi_doorbell_info *dbinfo;
>> +	struct iommu_domain *domain;
>> +	struct irq_chip *chip;
>> +	struct device *dev;
>> +	dma_addr_t iova;
>> +	int ret = 0, cpu;
>> +
>> +	while (data) {
>> +		dev = msi_desc_to_dev(irq_data_get_msi_desc(data));
>> +		domain = iommu_msi_domain(dev);
>> +		if (domain) {
>> +			chip = irq_data_get_irq_chip(data);
>> +			if (chip->msi_doorbell_info)
>> +				break;
>> +		}
>> +		data = data->parent_data;
>> +	}
> 
> Please split that out into a seperate function
> 
> struct irq_data *msi_get_doorbell_info(data)
> {
> 	.....
> 		if (chip->msi_doorbell_info)
> 			return chip->msi_get_doorbell_info(data);
> 	}
> 	return NULL;
> }
> 
>        info = msi_get_doorbell_info(data);
>        .....
> 
>> +	if (!data)
>> +		return 0;
>> +
>> +	dbinfo = chip->msi_doorbell_info(data);
>> +	if (!dbinfo)
>> +		return -EINVAL;
>> +
>> +	if (!dbinfo->doorbell_is_percpu) {
>> +		if (!map) {
>> +			iommu_msi_put_doorbell_iova(domain,
>> +						    dbinfo->global_doorbell);
>> +			return 0;
>> +		}
>> +		return iommu_msi_get_doorbell_iova(domain,
>> +						   dbinfo->global_doorbell,
>> +						   dbinfo->size, dbinfo->prot,
>> +						   &iova);
>> +	}
> 
> You can spare an indentation level with a helper function
> 
>     	if (!dbinfo->doorbell_is_percpu)
> 		return msi_map_global_doorbell(domain, dbinfo);
> 
>> +
>> +	/* percpu doorbells */
>> +	for_each_possible_cpu(cpu) {
>> +		phys_addr_t __percpu *db_addr =
>> +			per_cpu_ptr(dbinfo->percpu_doorbells, cpu);
>> +
>> +		if (!map) {
>> +			iommu_msi_put_doorbell_iova(domain, *db_addr);
>> +		} else {
>> +
>> +			ret = iommu_msi_get_doorbell_iova(domain, *db_addr,
>> +							  dbinfo->size,
>> +							  dbinfo->prot, &iova);
>> +			if (ret)
>> +				return ret;
>> +		}
>> +	}
> 
> Same here:
> 
> 	for_each_possible_cpu(cpu) {
> 		ret = msi_map_percpu_doorbell(domain, cpu);
> 		if (ret)
> 			return ret;
> 	}
>      	return 0;
>      
> Hmm?
> 
>> +
>> +	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 +423,29 @@ 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++) {
>> +			struct irq_data *d = irq_get_irq_data(virq + i);
>> +
>> +			ret = msi_handle_doorbell_mappings(d, true);
>> +			if (ret)
>> +				break;
>> +		}
>> +		if (ret) {
>> +			for (; i >= 0; i--) {
>> +				struct irq_data *d = irq_get_irq_data(virq + i);
>> +
>> +				msi_handle_doorbell_mappings(d, false);
>> +			}
>> +			irq_domain_free_irqs(virq, desc->nvec_used);
>> +			desc->irq = 0;
>> +			goto error;
> 
> How is that supposed to work? You clear desc->irq and then you call
> ops->handle_error.
if I don't clear the desc->irq I enter an infinite loop in pci_enable_msix_range.
This happens because msix_capability_init and pcie_enable_msix returns 1.

In msix_capability_init, at out_avail: we enumerate the msi_desc which have a non
zero irq, hence the returned value equal to 1.

Currently the only handle_error ops I found, pci_msi_domain_handle_error does not
use irq field so works although questionable.

As for the irq_domain_free_irqs I think I can remove it since handled later.

How do you advise to handle the above situation?

Thanks

Eric

> 
> Why are you adding this extra stuff here? Look at the call sites of
> msi_domain_alloc_irqs(). All of them use msi_domain_free_irqs() in case of
> error. There is no reason why you can't do the same....
> 
>>  /**
>> @@ -396,6 +486,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;
> 
> Can you please restructure the code so it reads
> 
> 		if (desc->irq)
> 			continue;
> 
> 		msi_handle_doorbell_mappings(irq_get_irq_data(desc->irq),
> 					     false);	
> 		irq_domain_free_irqs(desc->irq, desc->nvec_used);
> 		desc->irq = 0;
> 
> Just blindly whacking stuff into the 80 char limit is not helping readability.
> 
> Thanks,
> 
> 	tglx
> 

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


#1446443 — [PATCH v11 10/10] genirq/msi: use the MSI doorbell's IOVA when requested

FromEric Auger <eric.auger@redhat.com>
Date2016-07-19 15:10 +0200
Subject[PATCH v11 10/10] genirq/msi: use the MSI doorbell's IOVA when requested
Message-ID<rWCWC-4qE-31@gated-at.bofh.it>
In reply to#1446435
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@redhat.com>

---
v8 -> v9:
- Braces on both sides of the 'else' in msi_compose

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 | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index 69b5b19..e375544 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -63,10 +63,18 @@ static int msi_compose(struct irq_data *irq_data,
 {
 	int ret = 0;
 
-	if (erase)
+	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

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


#1447120 — Re: [PATCH v11 10/10] genirq/msi: use the MSI doorbell's IOVA when requested

FromThomas Gleixner <tglx@linutronix.de>
Date2016-07-20 11:20 +0200
SubjectRe: [PATCH v11 10/10] genirq/msi: use the MSI doorbell's IOVA when requested
Message-ID<rWVPA-7V0-11@gated-at.bofh.it>
In reply to#1446443
On Tue, 19 Jul 2016, Eric Auger wrote:

First of all - valid for all patches:

Subject: sys/subsys: Sentence starts with an uppercase letter

Now for this particular one:

genirq/msi: use the MSI doorbell's IOVA when requested

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

This changelog is completely useless. At least I cannot figure out what that
patch actually does. And the implementation is not self explaining either.
 
> @@ -63,10 +63,18 @@ static int msi_compose(struct irq_data *irq_data,
>  {
>  	int ret = 0;
>  
> -	if (erase)
> +	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));

What the heck is this call doing? And why is there only a WARN_ON and not a
proper error return code handling?

Thanks,

	tglx

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


#1449644 — Re: [PATCH v11 10/10] genirq/msi: use the MSI doorbell's IOVA when requested

FromAuger Eric <eric.auger@redhat.com>
Date2016-07-25 18:40 +0200
SubjectRe: [PATCH v11 10/10] genirq/msi: use the MSI doorbell's IOVA when requested
Message-ID<rYR58-6Qr-11@gated-at.bofh.it>
In reply to#1447120
Hi Thomas,

On 20/07/2016 11:09, Thomas Gleixner wrote:
> On Tue, 19 Jul 2016, Eric Auger wrote:
> 
> First of all - valid for all patches:
> 
> Subject: sys/subsys: Sentence starts with an uppercase letter
OK understood.
> 
> Now for this particular one:
> 
> genirq/msi: use the MSI doorbell's IOVA when requested
> 
>> 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).
> 
> This changelog is completely useless. At least I cannot figure out what that
> patch actually does. And the implementation is not self explaining either.

>  
>> @@ -63,10 +63,18 @@ static int msi_compose(struct irq_data *irq_data,
>>  {
>>  	int ret = 0;
>>  
>> -	if (erase)
>> +	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));
> 
> What the heck is this call doing? And why is there only a WARN_ON and not a
> proper error return code handling?

iommu_msi_msg_pa_to_va is part of the new iommu-msi API introduced in PART I of
this series. This helper function detects the physical address found in the
MSI message has a corresponding allocated IOVA. This happens if the MSI doorbell
is accessed through an IOMMU and this IOMMU do not bypass the MSI addresses
(ARM case). Allocation of this IOVA was performed in the previous patch.

So, if this is the case, the physical address is swapped with the IOVA
address. That way the PCIe device will send the MSI with this IOVA and
the address will be translated by the IOMMU into the target MSI doorbell PA.

Hope this clarifies

Thanks

Eric 
> 
> Thanks,
> 
> 	tglx
> 

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


#1446448 — [PATCH v11 08/10] irqchip/gicv3-its: register the MSI global doorbell

FromEric Auger <eric.auger@redhat.com>
Date2016-07-19 15:10 +0200
Subject[PATCH v11 08/10] irqchip/gicv3-its: register the MSI global doorbell
Message-ID<rWCWD-4qE-59@gated-at.bofh.it>
In reply to#1446435
This patch adds the registration of the MSI global doorbell in
gicv3-its driver plus the implementation for irq_chip
msi_doorbell_info ops.

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

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

---

v10 -> v11:
- adapt to new doorbell registration API and implement msi_doorbell_info
---
 drivers/irqchip/irq-gic-v3-its.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 5eb1f9e..23102e3 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -29,6 +29,8 @@
 #include <linux/of_platform.h>
 #include <linux/percpu.h>
 #include <linux/slab.h>
+#include <linux/iommu.h>
+#include <linux/msi-doorbell.h>
 
 #include <linux/irqchip.h>
 #include <linux/irqchip/arm-gic-v3.h>
@@ -84,6 +86,7 @@ struct its_node {
 	u32			ite_size;
 	u32			device_ids;
 	int			numa_node;
+	struct irq_chip_msi_doorbell_info *doorbell_info;
 };
 
 #define ITS_ITT_ALIGN		SZ_256
@@ -656,6 +659,16 @@ static void its_irq_compose_msi_msg(struct irq_data *d, struct msi_msg *msg)
 	msg->data		= its_get_event_id(d);
 }
 
+static struct irq_chip_msi_doorbell_info *
+its_msi_doorbell_info(struct irq_data *d)
+{
+	struct its_device *its_dev = irq_data_get_irq_chip_data(d);
+	struct its_node *its = its_dev->its;
+
+	return its->doorbell_info;
+}
+
+
 static struct irq_chip its_irq_chip = {
 	.name			= "ITS",
 	.irq_mask		= its_mask_irq,
@@ -663,6 +676,7 @@ static struct irq_chip its_irq_chip = {
 	.irq_eoi		= irq_chip_eoi_parent,
 	.irq_set_affinity	= its_set_affinity,
 	.irq_compose_msi_msg	= its_irq_compose_msi_msg,
+	.msi_doorbell_info	= its_msi_doorbell_info,
 };
 
 /*
@@ -1607,6 +1621,7 @@ static int __init its_probe(struct device_node *node,
 
 	if (of_property_read_bool(node, "msi-controller")) {
 		struct msi_domain_info *info;
+		phys_addr_t translater;
 
 		info = kzalloc(sizeof(*info), GFP_KERNEL);
 		if (!info) {
@@ -1614,10 +1629,23 @@ static int __init its_probe(struct device_node *node,
 			goto out_free_tables;
 		}
 
+		translater = its->phys_base + GITS_TRANSLATER;
+		its->doorbell_info =
+			msi_doorbell_register_global(translater, sizeof(u32),
+						     IOMMU_WRITE | IOMMU_MMIO,
+						     true);
+
+		if (IS_ERR_OR_NULL(its->doorbell_info))  {
+			kfree(info);
+			goto out_free_tables;
+		}
+
+
 		inner_domain = irq_domain_add_tree(node, &its_domain_ops, its);
 		if (!inner_domain) {
 			err = -ENOMEM;
 			kfree(info);
+			msi_doorbell_unregister_global(its->doorbell_info);
 			goto out_free_tables;
 		}
 
-- 
1.9.1

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


#1446450 — [PATCH v11 06/10] genirq/msi-doorbell: msi_doorbell_safe

FromEric Auger <eric.auger@redhat.com>
Date2016-07-19 15:10 +0200
Subject[PATCH v11 06/10] genirq/msi-doorbell: msi_doorbell_safe
Message-ID<rWCWD-4qE-49@gated-at.bofh.it>
In reply to#1446435
msi_doorbell_safe returns whether all the registered doorbells
implement irq_remapping.

When assigning a PCIe device whose host controller is upstream to
an IOMMU, we currently do not know whether the MSI controller is
upstream or downstream to the IOMMU.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 include/linux/msi-doorbell.h | 12 ++++++++++++
 kernel/irq/msi-doorbell.c    | 15 +++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/include/linux/msi-doorbell.h b/include/linux/msi-doorbell.h
index 146bfbc..9c30141 100644
--- a/include/linux/msi-doorbell.h
+++ b/include/linux/msi-doorbell.h
@@ -43,6 +43,13 @@ void msi_doorbell_unregister_global(struct irq_chip_msi_doorbell_info *db);
  */
 int msi_doorbell_pages(unsigned int order);
 
+/**
+ * msi_doorbell_safe: return whether all registered doorbells
+ * do implement irq_remapping and are safe to assign (coarse safety
+ * assessment)
+ */
+bool msi_doorbell_safe(void);
+
 #else
 
 static inline struct irq_chip_msi_doorbell_info *
@@ -61,6 +68,11 @@ msi_doorbell_pages(unsigned int order)
 	return 0;
 }
 
+static inline bool
+msi_doorbell_safe(void)
+{
+	return true;
+}
 #endif /* CONFIG_MSI_DOORBELL */
 
 #endif
diff --git a/kernel/irq/msi-doorbell.c b/kernel/irq/msi-doorbell.c
index a5bde37..fa5b429 100644
--- a/kernel/irq/msi-doorbell.c
+++ b/kernel/irq/msi-doorbell.c
@@ -112,3 +112,18 @@ int msi_doorbell_pages(unsigned int order)
 	return ret;
 }
 EXPORT_SYMBOL_GPL(msi_doorbell_pages);
+
+bool msi_doorbell_safe(void)
+{
+	struct irqchip_doorbell *db;
+	bool irq_remapping = true;
+
+	mutex_lock(&irqchip_doorbell_mutex);
+	list_for_each_entry(db, &irqchip_doorbell_list, next) {
+		irq_remapping &= db->info.irq_remapping;
+	}
+	mutex_unlock(&irqchip_doorbell_mutex);
+
+	return irq_remapping;
+}
+EXPORT_SYMBOL_GPL(msi_doorbell_safe);
\ No newline at end of file
-- 
1.9.1

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


#1447076 — Re: [PATCH v11 06/10] genirq/msi-doorbell: msi_doorbell_safe

FromThomas Gleixner <tglx@linutronix.de>
Date2016-07-20 10:20 +0200
SubjectRe: [PATCH v11 06/10] genirq/msi-doorbell: msi_doorbell_safe
Message-ID<rWUTw-7mO-21@gated-at.bofh.it>
In reply to#1446450
On Tue, 19 Jul 2016, Eric Auger wrote:
> +bool msi_doorbell_safe(void)
> +{
> +	struct irqchip_doorbell *db;
> +	bool irq_remapping = true;
> +
> +	mutex_lock(&irqchip_doorbell_mutex);
> +	list_for_each_entry(db, &irqchip_doorbell_list, next) {
> +		irq_remapping &= db->info.irq_remapping;

db->info.irq_remapping is set in msi_doorbell_register(). So you can keep book
about that there. No need to iterate here.

Thanks,

	tglx

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


#1447888 — Re: [PATCH v11 06/10] genirq/msi-doorbell: msi_doorbell_safe

FromAuger Eric <eric.auger@redhat.com>
Date2016-07-21 15:40 +0200
SubjectRe: [PATCH v11 06/10] genirq/msi-doorbell: msi_doorbell_safe
Message-ID<rXmmJ-81c-25@gated-at.bofh.it>
In reply to#1447076
Hi,

On 20/07/2016 10:12, Thomas Gleixner wrote:
> On Tue, 19 Jul 2016, Eric Auger wrote:
>> +bool msi_doorbell_safe(void)
>> +{
>> +	struct irqchip_doorbell *db;
>> +	bool irq_remapping = true;
>> +
>> +	mutex_lock(&irqchip_doorbell_mutex);
>> +	list_for_each_entry(db, &irqchip_doorbell_list, next) {
>> +		irq_remapping &= db->info.irq_remapping;
> 
> db->info.irq_remapping is set in msi_doorbell_register(). So you can keep book
> about that there. No need to iterate here.
Yes makes sense to store the info at registration time. Currently this
function is not in any fast path but that's cleaner from a general
perspective. I will need to do such iteration at un-registration though.

Thanks

Eric
> 
> Thanks,
> 
> 	tglx
> 
> 

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


#1448566 — Re: [PATCH v11 06/10] genirq/msi-doorbell: msi_doorbell_safe

FromThomas Gleixner <tglx@linutronix.de>
Date2016-07-22 14:50 +0200
SubjectRe: [PATCH v11 06/10] genirq/msi-doorbell: msi_doorbell_safe
Message-ID<rXI3U-5Un-21@gated-at.bofh.it>
In reply to#1447888
On Thu, 21 Jul 2016, Auger Eric wrote:
> On 20/07/2016 10:12, Thomas Gleixner wrote:
> > On Tue, 19 Jul 2016, Eric Auger wrote:
> >> +bool msi_doorbell_safe(void)
> >> +{
> >> +	struct irqchip_doorbell *db;
> >> +	bool irq_remapping = true;
> >> +
> >> +	mutex_lock(&irqchip_doorbell_mutex);
> >> +	list_for_each_entry(db, &irqchip_doorbell_list, next) {
> >> +		irq_remapping &= db->info.irq_remapping;
> > 
> > db->info.irq_remapping is set in msi_doorbell_register(). So you can keep book
> > about that there. No need to iterate here.
> Yes makes sense to store the info at registration time. Currently this
> function is not in any fast path but that's cleaner from a general
> perspective. I will need to do such iteration at un-registration though.

Two simple counter should be sufficient.

  nr_registered_bells;
  nr_remapped_bells;

....

Thanks,

	tglx

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


#1448609 — Re: [PATCH v11 06/10] genirq/msi-doorbell: msi_doorbell_safe

FromAuger Eric <eric.auger@redhat.com>
Date2016-07-22 16:10 +0200
SubjectRe: [PATCH v11 06/10] genirq/msi-doorbell: msi_doorbell_safe
Message-ID<rXJjk-6SA-13@gated-at.bofh.it>
In reply to#1448566
Hi Thomas,
On 22/07/2016 14:44, Thomas Gleixner wrote:
> On Thu, 21 Jul 2016, Auger Eric wrote:
>> On 20/07/2016 10:12, Thomas Gleixner wrote:
>>> On Tue, 19 Jul 2016, Eric Auger wrote:
>>>> +bool msi_doorbell_safe(void)
>>>> +{
>>>> +	struct irqchip_doorbell *db;
>>>> +	bool irq_remapping = true;
>>>> +
>>>> +	mutex_lock(&irqchip_doorbell_mutex);
>>>> +	list_for_each_entry(db, &irqchip_doorbell_list, next) {
>>>> +		irq_remapping &= db->info.irq_remapping;
>>>
>>> db->info.irq_remapping is set in msi_doorbell_register(). So you can keep book
>>> about that there. No need to iterate here.
>> Yes makes sense to store the info at registration time. Currently this
>> function is not in any fast path but that's cleaner from a general
>> perspective. I will need to do such iteration at un-registration though.
> 
> Two simple counter should be sufficient.
> 
>   nr_registered_bells;
>   nr_remapped_bells;

Yes definitively smarter to use counters! mental viscosity.

Thanks

Eric
> 
> ....
> 
> Thanks,
> 
> 	tglx
> 

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


#1446452 — [PATCH v11 01/10] genirq/msi: export msi_get_domain_info

FromEric Auger <eric.auger@redhat.com>
Date2016-07-19 15:10 +0200
Subject[PATCH v11 01/10] genirq/msi: export msi_get_domain_info
Message-ID<rWCWD-4qE-57@gated-at.bofh.it>
In reply to#1446435
We plan to use msi_get_domain_info in VFIO module so let's export it.

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

---

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] | [standalone]


Back to top | Article view | linux.kernel


csiph-web