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

Groups > linux.kernel > #1382731 > unrolled thread

[PATCH v7 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 v7 0/8] KVM PCIe/MSI passthrough on ARM/ARM64: kernel part 2/3: msi changes Eric Auger <eric.auger@linaro.org> - 2016-04-19 19:20 +0200
    [PATCH v7 3/8] genirq/msi: export msi_get_domain_info Eric Auger <eric.auger@linaro.org> - 2016-04-19 19:20 +0200
    [PATCH v7 4/8] genirq/msi: msi_compose wrapper Eric Auger <eric.auger@linaro.org> - 2016-04-19 19:20 +0200
    [PATCH v7 6/8] irqchip/gicv2m: implement msi_doorbell_info callback Eric Auger <eric.auger@linaro.org> - 2016-04-19 19:20 +0200
      Re: [PATCH v7 6/8] irqchip/gicv2m: implement msi_doorbell_info  callback Marc Zyngier <marc.zyngier@arm.com> - 2016-04-20 11:30 +0200
        Re: [PATCH v7 6/8] irqchip/gicv2m: implement msi_doorbell_info  callback Eric Auger <eric.auger@linaro.org> - 2016-04-20 14:40 +0200
          Re: [PATCH v7 6/8] irqchip/gicv2m: implement msi_doorbell_info  callback Marc Zyngier <marc.zyngier@arm.com> - 2016-04-20 20:00 +0200
            Re: [PATCH v7 6/8] irqchip/gicv2m: implement msi_doorbell_info  callback Eric Auger <eric.auger@linaro.org> - 2016-04-20 20:20 +0200

#1382731 — [PATCH v7 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.

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

History:

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's structs and related callback
  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                 |  32 ++++++-
 drivers/irqchip/irq-gic-v3-its-pci-msi.c      |   3 +-
 drivers/irqchip/irq-gic-v3-its-platform-msi.c |   3 +-
 include/linux/irq.h                           |  26 +++++-
 include/linux/msi.h                           |   2 +
 kernel/irq/msi.c                              | 119 +++++++++++++++++++++++---
 6 files changed, 166 insertions(+), 19 deletions(-)

-- 
1.9.1

[toc] | [next] | [standalone]

#1382732 — [PATCH v7 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]

#1382734 — [PATCH v7 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]

#1382736 — [PATCH v7 6/8] irqchip/gicv2m: implement msi_doorbell_info callback

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: creation
---
 drivers/irqchip/irq-gic-v2m.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-gic-v2m.c b/drivers/irqchip/irq-gic-v2m.c
index 28f047c..54690b9 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,16 @@ static void gicv2m_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
 		msg->data -= v2m->spi_start;
 }
 
+static const 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);
+
+	if (!v2m)
+		return NULL;
+	return (const struct irq_chip_msi_doorbell_info *)(&v2m->doorbell_info);
+}
+
 static struct irq_chip gicv2m_irq_chip = {
 	.name			= "GICv2m",
 	.irq_mask		= irq_chip_mask_parent,
@@ -112,6 +125,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 +261,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 +314,7 @@ static int __init gicv2m_init_one(struct fwnode_handle *fwnode,
 {
 	int ret;
 	struct v2m_data *v2m;
+	struct irq_chip_msi_doorbell __percpu *doorbell;
 
 	v2m = kzalloc(sizeof(struct v2m_data), GFP_KERNEL);
 	if (!v2m) {
@@ -311,11 +327,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(struct irq_chip_msi_doorbell);
+	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->base = v2m->res.start;
+	doorbell->size = 4;
+	doorbell->prot = IOMMU_WRITE;
+	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 +387,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

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

#1383196 — Re: [PATCH v7 6/8] irqchip/gicv2m: implement msi_doorbell_info callback

On 19/04/16 18:13, 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: creation
> ---
>  drivers/irqchip/irq-gic-v2m.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v2m.c b/drivers/irqchip/irq-gic-v2m.c
> index 28f047c..54690b9 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,16 @@ static void gicv2m_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
>  		msg->data -= v2m->spi_start;
>  }
>  
> +static const 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);
> +
> +	if (!v2m)
> +		return NULL;

How can this ever be NULL? I think you can drop that test.

> +	return (const struct irq_chip_msi_doorbell_info *)(&v2m->doorbell_info);

Please don't do that. Use "const" in the functions that are using
irq_chip_msi_doorbell_info, but do not make this "const" here.

> +}
> +
>  static struct irq_chip gicv2m_irq_chip = {
>  	.name			= "GICv2m",
>  	.irq_mask		= irq_chip_mask_parent,
> @@ -112,6 +125,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 +261,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 +314,7 @@ static int __init gicv2m_init_one(struct fwnode_handle *fwnode,
>  {
>  	int ret;
>  	struct v2m_data *v2m;
> +	struct irq_chip_msi_doorbell __percpu *doorbell;
>  
>  	v2m = kzalloc(sizeof(struct v2m_data), GFP_KERNEL);
>  	if (!v2m) {
> @@ -311,11 +327,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(struct irq_chip_msi_doorbell);
> +	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->base = v2m->res.start;
> +	doorbell->size = 4;
> +	doorbell->prot = IOMMU_WRITE;

You probably need to also have something that says IOMMU_DEVICE or
something similar, because I'm afraid you're getting a memory mapping
here. I've had a quick look at the two other series, but couldn't find
anything that would force the memory attributes.

> +	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 +387,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...

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

#1383345 — Re: [PATCH v7 6/8] irqchip/gicv2m: implement msi_doorbell_info callback

Marc,
On 04/20/2016 11:27 AM, Marc Zyngier wrote:
> On 19/04/16 18:13, 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: creation
>> ---
>>  drivers/irqchip/irq-gic-v2m.c | 32 +++++++++++++++++++++++++++++++-
>>  1 file changed, 31 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v2m.c b/drivers/irqchip/irq-gic-v2m.c
>> index 28f047c..54690b9 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,16 @@ static void gicv2m_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
>>  		msg->data -= v2m->spi_start;
>>  }
>>  
>> +static const 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);
>> +
>> +	if (!v2m)
>> +		return NULL;
> 
> How can this ever be NULL? I think you can drop that test.
OK
> 
>> +	return (const struct irq_chip_msi_doorbell_info *)(&v2m->doorbell_info);
> 
> Please don't do that. Use "const" in the functions that are using
> irq_chip_msi_doorbell_info, but do not make this "const" here.
It definitively compiles without casting so obviously this is not needed
but is there any other wrong thing I don't see?
we still want this function to return a pointer to a const?
> 
>> +}
>> +
>>  static struct irq_chip gicv2m_irq_chip = {
>>  	.name			= "GICv2m",
>>  	.irq_mask		= irq_chip_mask_parent,
>> @@ -112,6 +125,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 +261,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 +314,7 @@ static int __init gicv2m_init_one(struct fwnode_handle *fwnode,
>>  {
>>  	int ret;
>>  	struct v2m_data *v2m;
>> +	struct irq_chip_msi_doorbell __percpu *doorbell;
>>  
>>  	v2m = kzalloc(sizeof(struct v2m_data), GFP_KERNEL);
>>  	if (!v2m) {
>> @@ -311,11 +327,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(struct irq_chip_msi_doorbell);
>> +	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->base = v2m->res.start;
>> +	doorbell->size = 4;
>> +	doorbell->prot = IOMMU_WRITE;
> 
> You probably need to also have something that says IOMMU_DEVICE or
> something similar, because I'm afraid you're getting a memory mapping
> here. I've had a quick look at the two other series, but couldn't find
> anything that would force the memory attributes.
Yes you're right I currently just enforce the direction (which is
checked against what VFIO user registered). Do you refer to IOMMU_MMIO,
latterly proposed on the ML. In the positive, yes I intend to add it
once it gets upstreamed.

Thanks

Eric
> 
>> +	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 +387,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.
> 

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

#1383592 — Re: [PATCH v7 6/8] irqchip/gicv2m: implement msi_doorbell_info callback

On Wed, 20 Apr 2016 14:33:17 +0200
Eric Auger <eric.auger@linaro.org> wrote:

> Marc,
> On 04/20/2016 11:27 AM, Marc Zyngier wrote:
> > On 19/04/16 18:13, 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: creation
> >> ---
> >>  drivers/irqchip/irq-gic-v2m.c | 32 +++++++++++++++++++++++++++++++-
> >>  1 file changed, 31 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/irqchip/irq-gic-v2m.c b/drivers/irqchip/irq-gic-v2m.c
> >> index 28f047c..54690b9 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,16 @@ static void gicv2m_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
> >>  		msg->data -= v2m->spi_start;
> >>  }
> >>  
> >> +static const 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);
> >> +
> >> +	if (!v2m)
> >> +		return NULL;
> > 
> > How can this ever be NULL? I think you can drop that test.
> OK
> > 
> >> +	return (const struct irq_chip_msi_doorbell_info *)(&v2m->doorbell_info);
> > 
> > Please don't do that. Use "const" in the functions that are using
> > irq_chip_msi_doorbell_info, but do not make this "const" here.
> It definitively compiles without casting so obviously this is not needed
> but is there any other wrong thing I don't see?
> we still want this function to return a pointer to a const?

I don't think we can return a const pointer, because it is obviously
not (the memory has been kmalloc'ed, and you've written to it, so it is
not really "read-only").

Maybe I'm being overly zealous, but I've seen compilers taking amazing
shortcuts when offered a const qualifier...

> > 
> >> +}
> >> +
> >>  static struct irq_chip gicv2m_irq_chip = {
> >>  	.name			= "GICv2m",
> >>  	.irq_mask		= irq_chip_mask_parent,
> >> @@ -112,6 +125,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 +261,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 +314,7 @@ static int __init gicv2m_init_one(struct fwnode_handle *fwnode,
> >>  {
> >>  	int ret;
> >>  	struct v2m_data *v2m;
> >> +	struct irq_chip_msi_doorbell __percpu *doorbell;
> >>  
> >>  	v2m = kzalloc(sizeof(struct v2m_data), GFP_KERNEL);
> >>  	if (!v2m) {
> >> @@ -311,11 +327,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(struct irq_chip_msi_doorbell);
> >> +	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->base = v2m->res.start;
> >> +	doorbell->size = 4;
> >> +	doorbell->prot = IOMMU_WRITE;
> > 
> > You probably need to also have something that says IOMMU_DEVICE or
> > something similar, because I'm afraid you're getting a memory mapping
> > here. I've had a quick look at the two other series, but couldn't find
> > anything that would force the memory attributes.
> Yes you're right I currently just enforce the direction (which is
> checked against what VFIO user registered). Do you refer to IOMMU_MMIO,
> latterly proposed on the ML. In the positive, yes I intend to add it
> once it gets upstreamed.

Yeah, Robin's patches should become a dependency here, because there is
absolutely no guarantee that the device write to the doorbell won't be
treated a normal cacheable memory, with disastrous effects.

Thanks,

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

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

#1383620 — Re: [PATCH v7 6/8] irqchip/gicv2m: implement msi_doorbell_info callback

Marc,
On 04/20/2016 07:56 PM, Marc Zyngier wrote:
> On Wed, 20 Apr 2016 14:33:17 +0200
> Eric Auger <eric.auger@linaro.org> wrote:
> 
>> Marc,
>> On 04/20/2016 11:27 AM, Marc Zyngier wrote:
>>> On 19/04/16 18:13, 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: creation
>>>> ---
>>>>  drivers/irqchip/irq-gic-v2m.c | 32 +++++++++++++++++++++++++++++++-
>>>>  1 file changed, 31 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/irqchip/irq-gic-v2m.c b/drivers/irqchip/irq-gic-v2m.c
>>>> index 28f047c..54690b9 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,16 @@ static void gicv2m_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
>>>>  		msg->data -= v2m->spi_start;
>>>>  }
>>>>  
>>>> +static const 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);
>>>> +
>>>> +	if (!v2m)
>>>> +		return NULL;
>>>
>>> How can this ever be NULL? I think you can drop that test.
>> OK
>>>
>>>> +	return (const struct irq_chip_msi_doorbell_info *)(&v2m->doorbell_info);
>>>
>>> Please don't do that. Use "const" in the functions that are using
>>> irq_chip_msi_doorbell_info, but do not make this "const" here.
>> It definitively compiles without casting so obviously this is not needed
>> but is there any other wrong thing I don't see?
>> we still want this function to return a pointer to a const?
> 
> I don't think we can return a const pointer, because it is obviously
> not (the memory has been kmalloc'ed, and you've written to it, so it is
> not really "read-only").
I see what you are afraid of now and I will remove it; will ask some
compiler guys ;-)

Have a nice evening

Eric

> 
> Maybe I'm being overly zealous, but I've seen compilers taking amazing
> shortcuts when offered a const qualifier...
> 
>>>
>>>> +}
>>>> +
>>>>  static struct irq_chip gicv2m_irq_chip = {
>>>>  	.name			= "GICv2m",
>>>>  	.irq_mask		= irq_chip_mask_parent,
>>>> @@ -112,6 +125,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 +261,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 +314,7 @@ static int __init gicv2m_init_one(struct fwnode_handle *fwnode,
>>>>  {
>>>>  	int ret;
>>>>  	struct v2m_data *v2m;
>>>> +	struct irq_chip_msi_doorbell __percpu *doorbell;
>>>>  
>>>>  	v2m = kzalloc(sizeof(struct v2m_data), GFP_KERNEL);
>>>>  	if (!v2m) {
>>>> @@ -311,11 +327,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(struct irq_chip_msi_doorbell);
>>>> +	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->base = v2m->res.start;
>>>> +	doorbell->size = 4;
>>>> +	doorbell->prot = IOMMU_WRITE;
>>>
>>> You probably need to also have something that says IOMMU_DEVICE or
>>> something similar, because I'm afraid you're getting a memory mapping
>>> here. I've had a quick look at the two other series, but couldn't find
>>> anything that would force the memory attributes.
>> Yes you're right I currently just enforce the direction (which is
>> checked against what VFIO user registered). Do you refer to IOMMU_MMIO,
>> latterly proposed on the ML. In the positive, yes I intend to add it
>> once it gets upstreamed.
> 
> Yeah, Robin's patches should become a dependency here, because there is
> absolutely no guarantee that the device write to the doorbell won't be
> treated a normal cacheable memory, with disastrous effects.
> 
> Thanks,
> 
> 	M.
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web