All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v7 0/8] KVM PCIe/MSI passthrough on ARM/ARM64: kernel part 2/3: msi changes
@ 2016-04-19 17:13 ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-19 17:13 UTC (permalink / raw)
  To: eric.auger, eric.auger, robin.murphy, alex.williamson,
	will.deacon, joro, tglx, jason, marc.zyngier, christoffer.dall,
	linux-arm-kernel
  Cc: patches, linux-kernel, Bharat.Bhushan, pranav.sawargaonkar,
	p.fedin, iommu, Jean-Philippe.Brucker, julien.grall

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

^ permalink raw reply	[flat|nested] 64+ messages in thread

* [PATCH v7 0/8] KVM PCIe/MSI passthrough on ARM/ARM64: kernel part 2/3: msi changes
@ 2016-04-19 17:13 ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-19 17:13 UTC (permalink / raw)
  To: eric.auger-qxv4g6HH51o, eric.auger-QSEj5FYQhm4dnm+yROfE0A,
	robin.murphy-5wv7dgnIgG8, alex.williamson-H+wXaHxf7aLQT0dZR+AlfA,
	will.deacon-5wv7dgnIgG8, joro-zLv9SwRftAIdnm+yROfE0A,
	tglx-hfZtesqFncYOwBW4kG4KsQ, jason-NLaQJdtUoK4Be96aLqz0jA,
	marc.zyngier-5wv7dgnIgG8,
	christoffer.dall-QSEj5FYQhm4dnm+yROfE0A,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
  Cc: julien.grall-5wv7dgnIgG8, patches-QSEj5FYQhm4dnm+yROfE0A,
	p.fedin-Sze3O3UU22JBDgjK7y7TUQ,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	pranav.sawargaonkar-Re5JQEeQqe8AvxtiuMwx3w

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

^ permalink raw reply	[flat|nested] 64+ messages in thread

* [PATCH v7 0/8] KVM PCIe/MSI passthrough on ARM/ARM64: kernel part 2/3: msi changes
@ 2016-04-19 17:13 ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-19 17:13 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 64+ messages in thread

* [PATCH v7 1/8] genirq/msi: Add a new MSI_FLAG_IRQ_REMAPPING flag
@ 2016-04-19 17:13   ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-19 17:13 UTC (permalink / raw)
  To: eric.auger, eric.auger, robin.murphy, alex.williamson,
	will.deacon, joro, tglx, jason, marc.zyngier, christoffer.dall,
	linux-arm-kernel
  Cc: patches, linux-kernel, Bharat.Bhushan, pranav.sawargaonkar,
	p.fedin, iommu, Jean-Philippe.Brucker, julien.grall

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

^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v7 1/8] genirq/msi: Add a new MSI_FLAG_IRQ_REMAPPING flag
@ 2016-04-19 17:13   ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-19 17:13 UTC (permalink / raw)
  To: eric.auger-qxv4g6HH51o, eric.auger-QSEj5FYQhm4dnm+yROfE0A,
	robin.murphy-5wv7dgnIgG8, alex.williamson-H+wXaHxf7aLQT0dZR+AlfA,
	will.deacon-5wv7dgnIgG8, joro-zLv9SwRftAIdnm+yROfE0A,
	tglx-hfZtesqFncYOwBW4kG4KsQ, jason-NLaQJdtUoK4Be96aLqz0jA,
	marc.zyngier-5wv7dgnIgG8,
	christoffer.dall-QSEj5FYQhm4dnm+yROfE0A,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
  Cc: julien.grall-5wv7dgnIgG8, patches-QSEj5FYQhm4dnm+yROfE0A,
	p.fedin-Sze3O3UU22JBDgjK7y7TUQ,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	pranav.sawargaonkar-Re5JQEeQqe8AvxtiuMwx3w

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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

^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v7 1/8] genirq/msi: Add a new MSI_FLAG_IRQ_REMAPPING flag
@ 2016-04-19 17:13   ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-19 17:13 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v7 2/8] irqchip/gic-v3-its: ITS advertises MSI_FLAG_IRQ_REMAPPING
@ 2016-04-19 17:13   ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-19 17:13 UTC (permalink / raw)
  To: eric.auger, eric.auger, robin.murphy, alex.williamson,
	will.deacon, joro, tglx, jason, marc.zyngier, christoffer.dall,
	linux-arm-kernel
  Cc: patches, linux-kernel, Bharat.Bhushan, pranav.sawargaonkar,
	p.fedin, iommu, Jean-Philippe.Brucker, julien.grall

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

^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v7 2/8] irqchip/gic-v3-its: ITS advertises MSI_FLAG_IRQ_REMAPPING
@ 2016-04-19 17:13   ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-19 17:13 UTC (permalink / raw)
  To: eric.auger-qxv4g6HH51o, eric.auger-QSEj5FYQhm4dnm+yROfE0A,
	robin.murphy-5wv7dgnIgG8, alex.williamson-H+wXaHxf7aLQT0dZR+AlfA,
	will.deacon-5wv7dgnIgG8, joro-zLv9SwRftAIdnm+yROfE0A,
	tglx-hfZtesqFncYOwBW4kG4KsQ, jason-NLaQJdtUoK4Be96aLqz0jA,
	marc.zyngier-5wv7dgnIgG8,
	christoffer.dall-QSEj5FYQhm4dnm+yROfE0A,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
  Cc: julien.grall-5wv7dgnIgG8, patches-QSEj5FYQhm4dnm+yROfE0A,
	p.fedin-Sze3O3UU22JBDgjK7y7TUQ,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	pranav.sawargaonkar-Re5JQEeQqe8AvxtiuMwx3w

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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

^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v7 2/8] irqchip/gic-v3-its: ITS advertises MSI_FLAG_IRQ_REMAPPING
@ 2016-04-19 17:13   ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-19 17:13 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v7 3/8] genirq/msi: export msi_get_domain_info
@ 2016-04-19 17:13   ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-19 17:13 UTC (permalink / raw)
  To: eric.auger, eric.auger, robin.murphy, alex.williamson,
	will.deacon, joro, tglx, jason, marc.zyngier, christoffer.dall,
	linux-arm-kernel
  Cc: patches, linux-kernel, Bharat.Bhushan, pranav.sawargaonkar,
	p.fedin, iommu, Jean-Philippe.Brucker, julien.grall

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

^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v7 3/8] genirq/msi: export msi_get_domain_info
@ 2016-04-19 17:13   ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-19 17:13 UTC (permalink / raw)
  To: eric.auger-qxv4g6HH51o, eric.auger-QSEj5FYQhm4dnm+yROfE0A,
	robin.murphy-5wv7dgnIgG8, alex.williamson-H+wXaHxf7aLQT0dZR+AlfA,
	will.deacon-5wv7dgnIgG8, joro-zLv9SwRftAIdnm+yROfE0A,
	tglx-hfZtesqFncYOwBW4kG4KsQ, jason-NLaQJdtUoK4Be96aLqz0jA,
	marc.zyngier-5wv7dgnIgG8,
	christoffer.dall-QSEj5FYQhm4dnm+yROfE0A,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
  Cc: julien.grall-5wv7dgnIgG8, patches-QSEj5FYQhm4dnm+yROfE0A,
	p.fedin-Sze3O3UU22JBDgjK7y7TUQ,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	pranav.sawargaonkar-Re5JQEeQqe8AvxtiuMwx3w

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

Signed-off-by: Eric Auger <eric.auger-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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

^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v7 3/8] genirq/msi: export msi_get_domain_info
@ 2016-04-19 17:13   ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-19 17:13 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v7 4/8] genirq/msi: msi_compose wrapper
@ 2016-04-19 17:13   ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-19 17:13 UTC (permalink / raw)
  To: eric.auger, eric.auger, robin.murphy, alex.williamson,
	will.deacon, joro, tglx, jason, marc.zyngier, christoffer.dall,
	linux-arm-kernel
  Cc: patches, linux-kernel, Bharat.Bhushan, pranav.sawargaonkar,
	p.fedin, iommu, Jean-Philippe.Brucker, julien.grall

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

^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v7 4/8] genirq/msi: msi_compose wrapper
@ 2016-04-19 17:13   ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-19 17:13 UTC (permalink / raw)
  To: eric.auger-qxv4g6HH51o, eric.auger-QSEj5FYQhm4dnm+yROfE0A,
	robin.murphy-5wv7dgnIgG8, alex.williamson-H+wXaHxf7aLQT0dZR+AlfA,
	will.deacon-5wv7dgnIgG8, joro-zLv9SwRftAIdnm+yROfE0A,
	tglx-hfZtesqFncYOwBW4kG4KsQ, jason-NLaQJdtUoK4Be96aLqz0jA,
	marc.zyngier-5wv7dgnIgG8,
	christoffer.dall-QSEj5FYQhm4dnm+yROfE0A,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
  Cc: julien.grall-5wv7dgnIgG8, patches-QSEj5FYQhm4dnm+yROfE0A,
	p.fedin-Sze3O3UU22JBDgjK7y7TUQ,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	pranav.sawargaonkar-Re5JQEeQqe8AvxtiuMwx3w

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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

^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v7 4/8] genirq/msi: msi_compose wrapper
@ 2016-04-19 17:13   ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-19 17:13 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v7 5/8] genirq/irq: introduce msi_doorbell's structs and related callback
@ 2016-04-19 17:13   ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-19 17:13 UTC (permalink / raw)
  To: eric.auger, eric.auger, robin.murphy, alex.williamson,
	will.deacon, joro, tglx, jason, marc.zyngier, christoffer.dall,
	linux-arm-kernel
  Cc: patches, linux-kernel, Bharat.Bhushan, pranav.sawargaonkar,
	p.fedin, iommu, Jean-Philippe.Brucker, julien.grall

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.
The number of doorbells for the whole irqchip is stored in nb_doorbells.

A doorbell region is characterized by its physical address base, size and
IOMMU protection flag.

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

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

---

v7: creation
---
 include/linux/irq.h | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index c4de623..fdad8c1 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -312,9 +312,25 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
 	return d->hwirq;
 }
 
-/**
- * struct irq_chip - hardware interrupt chip descriptor
- *
+/* MSI doorbell region */
+struct irq_chip_msi_doorbell {
+	phys_addr_t base;
+	size_t size;
+	int prot; /* iommu protection flag */
+};
+
+/*
+ * 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 {
+	struct irq_chip_msi_doorbell __percpu *percpu_doorbells;
+	int nb_doorbells; /* overall number of doorbells */
+};
+
+/** * struct irq_chip - hardware interrupt chip descriptor *
  * @name:		name for /proc/interrupts
  * @irq_startup:	start up the interrupt (defaults to ->enable if NULL)
  * @irq_shutdown:	shut down the interrupt (defaults to ->disable if NULL)
@@ -349,6 +365,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 +411,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);
-
+	const 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

^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v7 5/8] genirq/irq: introduce msi_doorbell's structs and related callback
@ 2016-04-19 17:13   ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-19 17:13 UTC (permalink / raw)
  To: eric.auger-qxv4g6HH51o, eric.auger-QSEj5FYQhm4dnm+yROfE0A,
	robin.murphy-5wv7dgnIgG8, alex.williamson-H+wXaHxf7aLQT0dZR+AlfA,
	will.deacon-5wv7dgnIgG8, joro-zLv9SwRftAIdnm+yROfE0A,
	tglx-hfZtesqFncYOwBW4kG4KsQ, jason-NLaQJdtUoK4Be96aLqz0jA,
	marc.zyngier-5wv7dgnIgG8,
	christoffer.dall-QSEj5FYQhm4dnm+yROfE0A,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
  Cc: julien.grall-5wv7dgnIgG8, patches-QSEj5FYQhm4dnm+yROfE0A,
	p.fedin-Sze3O3UU22JBDgjK7y7TUQ,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	pranav.sawargaonkar-Re5JQEeQqe8AvxtiuMwx3w

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.
The number of doorbells for the whole irqchip is stored in nb_doorbells.

A doorbell region is characterized by its physical address base, size and
IOMMU protection flag.

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

Signed-off-by: Eric Auger <eric.auger-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

---

v7: creation
---
 include/linux/irq.h | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index c4de623..fdad8c1 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -312,9 +312,25 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
 	return d->hwirq;
 }
 
-/**
- * struct irq_chip - hardware interrupt chip descriptor
- *
+/* MSI doorbell region */
+struct irq_chip_msi_doorbell {
+	phys_addr_t base;
+	size_t size;
+	int prot; /* iommu protection flag */
+};
+
+/*
+ * 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 {
+	struct irq_chip_msi_doorbell __percpu *percpu_doorbells;
+	int nb_doorbells; /* overall number of doorbells */
+};
+
+/** * struct irq_chip - hardware interrupt chip descriptor *
  * @name:		name for /proc/interrupts
  * @irq_startup:	start up the interrupt (defaults to ->enable if NULL)
  * @irq_shutdown:	shut down the interrupt (defaults to ->disable if NULL)
@@ -349,6 +365,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 +411,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);
-
+	const 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

^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v7 5/8] genirq/irq: introduce msi_doorbell's structs and related callback
@ 2016-04-19 17:13   ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-19 17:13 UTC (permalink / raw)
  To: linux-arm-kernel

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.
The number of doorbells for the whole irqchip is stored in nb_doorbells.

A doorbell region is characterized by its physical address base, size and
IOMMU protection flag.

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

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

---

v7: creation
---
 include/linux/irq.h | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index c4de623..fdad8c1 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -312,9 +312,25 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
 	return d->hwirq;
 }
 
-/**
- * struct irq_chip - hardware interrupt chip descriptor
- *
+/* MSI doorbell region */
+struct irq_chip_msi_doorbell {
+	phys_addr_t base;
+	size_t size;
+	int prot; /* iommu protection flag */
+};
+
+/*
+ * 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 {
+	struct irq_chip_msi_doorbell __percpu *percpu_doorbells;
+	int nb_doorbells; /* overall number of doorbells */
+};
+
+/** * struct irq_chip - hardware interrupt chip descriptor *
  * @name:		name for /proc/interrupts
  * @irq_startup:	start up the interrupt (defaults to ->enable if NULL)
  * @irq_shutdown:	shut down the interrupt (defaults to ->disable if NULL)
@@ -349,6 +365,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 +411,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);
-
+	const 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

^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v7 6/8] irqchip/gicv2m: implement msi_doorbell_info callback
@ 2016-04-19 17:13   ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-19 17:13 UTC (permalink / raw)
  To: eric.auger, eric.auger, robin.murphy, alex.williamson,
	will.deacon, joro, tglx, jason, marc.zyngier, christoffer.dall,
	linux-arm-kernel
  Cc: patches, linux-kernel, Bharat.Bhushan, pranav.sawargaonkar,
	p.fedin, iommu, Jean-Philippe.Brucker, julien.grall

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

^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v7 6/8] irqchip/gicv2m: implement msi_doorbell_info callback
@ 2016-04-19 17:13   ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-19 17:13 UTC (permalink / raw)
  To: eric.auger-qxv4g6HH51o, eric.auger-QSEj5FYQhm4dnm+yROfE0A,
	robin.murphy-5wv7dgnIgG8, alex.williamson-H+wXaHxf7aLQT0dZR+AlfA,
	will.deacon-5wv7dgnIgG8, joro-zLv9SwRftAIdnm+yROfE0A,
	tglx-hfZtesqFncYOwBW4kG4KsQ, jason-NLaQJdtUoK4Be96aLqz0jA,
	marc.zyngier-5wv7dgnIgG8,
	christoffer.dall-QSEj5FYQhm4dnm+yROfE0A,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
  Cc: julien.grall-5wv7dgnIgG8, patches-QSEj5FYQhm4dnm+yROfE0A,
	p.fedin-Sze3O3UU22JBDgjK7y7TUQ,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	pranav.sawargaonkar-Re5JQEeQqe8AvxtiuMwx3w

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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

^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v7 6/8] irqchip/gicv2m: implement msi_doorbell_info callback
@ 2016-04-19 17:13   ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-19 17:13 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v7 7/8] genirq/msi: map/unmap the MSI doorbells on msi_domain_alloc/free_irqs
@ 2016-04-19 17:13   ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-19 17:13 UTC (permalink / raw)
  To: eric.auger, eric.auger, robin.murphy, alex.williamson,
	will.deacon, joro, tglx, jason, marc.zyngier, christoffer.dall,
	linux-arm-kernel
  Cc: patches, linux-kernel, Bharat.Bhushan, pranav.sawargaonkar,
	p.fedin, iommu, Jean-Philippe.Brucker, julien.grall

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: creation
---
 kernel/irq/msi.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 85 insertions(+), 9 deletions(-)

diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index 72bf4d6..e45907e 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/dma-reserved-iommu.h>
+#include <linux/iommu.h>
 
 /* Temparory solution for building, will be removed later */
 #include <linux/pci.h>
@@ -322,6 +324,59 @@ int msi_domain_populate_irqs(struct irq_domain *domain, struct device *dev,
 }
 
 /**
+ * msi_handle_doorbell_mapping: 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) {
+		const struct irq_chip_msi_doorbell_info *dbinfo;
+		struct iommu_domain *domain;
+		struct msi_desc *desc;
+		struct irq_chip *chip;
+		dma_addr_t iova;
+		int ret = 0, i;
+
+		chip = irq_data_get_irq_chip(data);
+		desc = irq_data_get_msi_desc(data);
+
+		domain = iommu_msi_mapping_desc_to_domain(desc);
+		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++) {
+			struct irq_chip_msi_doorbell __percpu *db;
+
+			db = per_cpu_ptr(dbinfo->percpu_doorbells, i);
+			if (map) {
+				ret = iommu_get_reserved_iova(domain,
+							      db->base,
+							      db->size,
+							      db->prot,
+							      &iova);
+				if (ret)
+					return ret;
+			} else
+				iommu_put_reserved_iova(domain, db->base);
+		}
+	}
+	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 +407,25 @@ 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++)
+		for (i = 0; i < desc->nvec_used; i++) {
 			irq_set_msi_desc_off(virq, i, desc);
+			ret = msi_handle_doorbell_mappings(
+				irq_get_irq_data(virq + i), true);
+			if (ret)
+				break;
+		}
+
+		if (ret) {
+			for (; i >= 0; i--) {
+				ret = msi_handle_doorbell_mappings(
+					irq_get_irq_data(virq + i), false);
+				irq_set_msi_desc_off(virq, i, NULL);
+			}
+			goto error;
+		}
 	}
 
 	if (ops->msi_finish)
@@ -377,6 +440,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 +466,12 @@ 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) {
+			int ret;
+
+			ret = msi_handle_doorbell_mappings(
+				irq_get_irq_data(desc->irq),
+				false);
+			WARN_ON(ret);
 			irq_domain_free_irqs(desc->irq, desc->nvec_used);
 			desc->irq = 0;
 		}
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v7 7/8] genirq/msi: map/unmap the MSI doorbells on msi_domain_alloc/free_irqs
@ 2016-04-19 17:13   ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-19 17:13 UTC (permalink / raw)
  To: eric.auger-qxv4g6HH51o, eric.auger-QSEj5FYQhm4dnm+yROfE0A,
	robin.murphy-5wv7dgnIgG8, alex.williamson-H+wXaHxf7aLQT0dZR+AlfA,
	will.deacon-5wv7dgnIgG8, joro-zLv9SwRftAIdnm+yROfE0A,
	tglx-hfZtesqFncYOwBW4kG4KsQ, jason-NLaQJdtUoK4Be96aLqz0jA,
	marc.zyngier-5wv7dgnIgG8,
	christoffer.dall-QSEj5FYQhm4dnm+yROfE0A,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
  Cc: julien.grall-5wv7dgnIgG8, patches-QSEj5FYQhm4dnm+yROfE0A,
	p.fedin-Sze3O3UU22JBDgjK7y7TUQ,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	pranav.sawargaonkar-Re5JQEeQqe8AvxtiuMwx3w

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

---

v7: creation
---
 kernel/irq/msi.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 85 insertions(+), 9 deletions(-)

diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index 72bf4d6..e45907e 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/dma-reserved-iommu.h>
+#include <linux/iommu.h>
 
 /* Temparory solution for building, will be removed later */
 #include <linux/pci.h>
@@ -322,6 +324,59 @@ int msi_domain_populate_irqs(struct irq_domain *domain, struct device *dev,
 }
 
 /**
+ * msi_handle_doorbell_mapping: 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) {
+		const struct irq_chip_msi_doorbell_info *dbinfo;
+		struct iommu_domain *domain;
+		struct msi_desc *desc;
+		struct irq_chip *chip;
+		dma_addr_t iova;
+		int ret = 0, i;
+
+		chip = irq_data_get_irq_chip(data);
+		desc = irq_data_get_msi_desc(data);
+
+		domain = iommu_msi_mapping_desc_to_domain(desc);
+		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++) {
+			struct irq_chip_msi_doorbell __percpu *db;
+
+			db = per_cpu_ptr(dbinfo->percpu_doorbells, i);
+			if (map) {
+				ret = iommu_get_reserved_iova(domain,
+							      db->base,
+							      db->size,
+							      db->prot,
+							      &iova);
+				if (ret)
+					return ret;
+			} else
+				iommu_put_reserved_iova(domain, db->base);
+		}
+	}
+	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 +407,25 @@ 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++)
+		for (i = 0; i < desc->nvec_used; i++) {
 			irq_set_msi_desc_off(virq, i, desc);
+			ret = msi_handle_doorbell_mappings(
+				irq_get_irq_data(virq + i), true);
+			if (ret)
+				break;
+		}
+
+		if (ret) {
+			for (; i >= 0; i--) {
+				ret = msi_handle_doorbell_mappings(
+					irq_get_irq_data(virq + i), false);
+				irq_set_msi_desc_off(virq, i, NULL);
+			}
+			goto error;
+		}
 	}
 
 	if (ops->msi_finish)
@@ -377,6 +440,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 +466,12 @@ 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) {
+			int ret;
+
+			ret = msi_handle_doorbell_mappings(
+				irq_get_irq_data(desc->irq),
+				false);
+			WARN_ON(ret);
 			irq_domain_free_irqs(desc->irq, desc->nvec_used);
 			desc->irq = 0;
 		}
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v7 7/8] genirq/msi: map/unmap the MSI doorbells on msi_domain_alloc/free_irqs
@ 2016-04-19 17:13   ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-19 17:13 UTC (permalink / raw)
  To: linux-arm-kernel

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: creation
---
 kernel/irq/msi.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 85 insertions(+), 9 deletions(-)

diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index 72bf4d6..e45907e 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/dma-reserved-iommu.h>
+#include <linux/iommu.h>
 
 /* Temparory solution for building, will be removed later */
 #include <linux/pci.h>
@@ -322,6 +324,59 @@ int msi_domain_populate_irqs(struct irq_domain *domain, struct device *dev,
 }
 
 /**
+ * msi_handle_doorbell_mapping: 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) {
+		const struct irq_chip_msi_doorbell_info *dbinfo;
+		struct iommu_domain *domain;
+		struct msi_desc *desc;
+		struct irq_chip *chip;
+		dma_addr_t iova;
+		int ret = 0, i;
+
+		chip = irq_data_get_irq_chip(data);
+		desc = irq_data_get_msi_desc(data);
+
+		domain = iommu_msi_mapping_desc_to_domain(desc);
+		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++) {
+			struct irq_chip_msi_doorbell __percpu *db;
+
+			db = per_cpu_ptr(dbinfo->percpu_doorbells, i);
+			if (map) {
+				ret = iommu_get_reserved_iova(domain,
+							      db->base,
+							      db->size,
+							      db->prot,
+							      &iova);
+				if (ret)
+					return ret;
+			} else
+				iommu_put_reserved_iova(domain, db->base);
+		}
+	}
+	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 +407,25 @@ 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++)
+		for (i = 0; i < desc->nvec_used; i++) {
 			irq_set_msi_desc_off(virq, i, desc);
+			ret = msi_handle_doorbell_mappings(
+				irq_get_irq_data(virq + i), true);
+			if (ret)
+				break;
+		}
+
+		if (ret) {
+			for (; i >= 0; i--) {
+				ret = msi_handle_doorbell_mappings(
+					irq_get_irq_data(virq + i), false);
+				irq_set_msi_desc_off(virq, i, NULL);
+			}
+			goto error;
+		}
 	}
 
 	if (ops->msi_finish)
@@ -377,6 +440,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 +466,12 @@ 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) {
+			int ret;
+
+			ret = msi_handle_doorbell_mappings(
+				irq_get_irq_data(desc->irq),
+				false);
+			WARN_ON(ret);
 			irq_domain_free_irqs(desc->irq, desc->nvec_used);
 			desc->irq = 0;
 		}
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v7 8/8] genirq/msi: use the MSI doorbell's IOVA when requested
  2016-04-19 17:13 ` Eric Auger
@ 2016-04-19 17:13   ` Eric Auger
  -1 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-19 17:13 UTC (permalink / raw)
  To: eric.auger, eric.auger, robin.murphy, alex.williamson,
	will.deacon, joro, tglx, jason, marc.zyngier, christoffer.dall,
	linux-arm-kernel
  Cc: patches, linux-kernel, Bharat.Bhushan, pranav.sawargaonkar,
	p.fedin, iommu, Jean-Philippe.Brucker, julien.grall

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>

---
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 | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index e45907e..0ebb2d8 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -19,6 +19,7 @@
 
 /* Temparory solution for building, will be removed later */
 #include <linux/pci.h>
+#include <linux/dma-reserved-iommu.h>
 
 struct msi_desc *alloc_msi_entry(struct device *dev)
 {
@@ -64,8 +65,12 @@ static int msi_compose(struct irq_data *irq_data,
 
 	if (erase)
 		memset(msg, 0, sizeof(*msg));
-	else
+	else {
 		ret = irq_chip_compose_msi_msg(irq_data, msg);
+		if (ret)
+			return ret;
+		iommu_msi_mapping_translate_msg(irq_data, msg);
+	}
 
 	return ret;
 }
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v7 8/8] genirq/msi: use the MSI doorbell's IOVA when requested
@ 2016-04-19 17:13   ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-19 17:13 UTC (permalink / raw)
  To: linux-arm-kernel

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>

---
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 | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index e45907e..0ebb2d8 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -19,6 +19,7 @@
 
 /* Temparory solution for building, will be removed later */
 #include <linux/pci.h>
+#include <linux/dma-reserved-iommu.h>
 
 struct msi_desc *alloc_msi_entry(struct device *dev)
 {
@@ -64,8 +65,12 @@ static int msi_compose(struct irq_data *irq_data,
 
 	if (erase)
 		memset(msg, 0, sizeof(*msg));
-	else
+	else {
 		ret = irq_chip_compose_msi_msg(irq_data, msg);
+		if (ret)
+			return ret;
+		iommu_msi_mapping_translate_msg(irq_data, msg);
+	}
 
 	return ret;
 }
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 64+ messages in thread

* Re: [PATCH v7 7/8] genirq/msi: map/unmap the MSI doorbells on msi_domain_alloc/free_irqs
  2016-04-19 17:13   ` Eric Auger
  (?)
@ 2016-04-19 18:04     ` kbuild test robot
  -1 siblings, 0 replies; 64+ messages in thread
From: kbuild test robot @ 2016-04-19 18:04 UTC (permalink / raw)
  To: Eric Auger
  Cc: kbuild-all, eric.auger, eric.auger, robin.murphy,
	alex.williamson, will.deacon, joro, tglx, jason, marc.zyngier,
	christoffer.dall, linux-arm-kernel, patches, linux-kernel,
	Bharat.Bhushan, pranav.sawargaonkar, p.fedin, iommu,
	Jean-Philippe.Brucker, julien.grall

[-- Attachment #1: Type: text/plain, Size: 1232 bytes --]

Hi,

[auto build test ERROR on tip/irq/core]
[also build test ERROR on v4.6-rc4 next-20160419]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Eric-Auger/KVM-PCIe-MSI-passthrough-on-ARM-ARM64-kernel-part-2-3-msi-changes/20160420-011957
config: x86_64-lkp (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

>> kernel/irq/msi.c:17:38: fatal error: linux/dma-reserved-iommu.h: No such file or directory
    #include <linux/dma-reserved-iommu.h>
                                         ^
   compilation terminated.

vim +17 kernel/irq/msi.c

    11	 */
    12	#include <linux/types.h>
    13	#include <linux/device.h>
    14	#include <linux/irq.h>
    15	#include <linux/irqdomain.h>
    16	#include <linux/msi.h>
  > 17	#include <linux/dma-reserved-iommu.h>
    18	#include <linux/iommu.h>
    19	
    20	/* Temparory solution for building, will be removed later */

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 22417 bytes --]

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v7 7/8] genirq/msi: map/unmap the MSI doorbells on msi_domain_alloc/free_irqs
@ 2016-04-19 18:04     ` kbuild test robot
  0 siblings, 0 replies; 64+ messages in thread
From: kbuild test robot @ 2016-04-19 18:04 UTC (permalink / raw)
  Cc: kbuild-all, eric.auger, eric.auger, robin.murphy,
	alex.williamson, will.deacon, joro, tglx, jason, marc.zyngier,
	christoffer.dall, linux-arm-kernel, patches, linux-kernel,
	Bharat.Bhushan, pranav.sawargaonkar, p.fedin, iommu,
	Jean-Philippe.Brucker, julien.grall

[-- Attachment #1: Type: text/plain, Size: 1232 bytes --]

Hi,

[auto build test ERROR on tip/irq/core]
[also build test ERROR on v4.6-rc4 next-20160419]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Eric-Auger/KVM-PCIe-MSI-passthrough-on-ARM-ARM64-kernel-part-2-3-msi-changes/20160420-011957
config: x86_64-lkp (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

>> kernel/irq/msi.c:17:38: fatal error: linux/dma-reserved-iommu.h: No such file or directory
    #include <linux/dma-reserved-iommu.h>
                                         ^
   compilation terminated.

vim +17 kernel/irq/msi.c

    11	 */
    12	#include <linux/types.h>
    13	#include <linux/device.h>
    14	#include <linux/irq.h>
    15	#include <linux/irqdomain.h>
    16	#include <linux/msi.h>
  > 17	#include <linux/dma-reserved-iommu.h>
    18	#include <linux/iommu.h>
    19	
    20	/* Temparory solution for building, will be removed later */

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 22417 bytes --]

^ permalink raw reply	[flat|nested] 64+ messages in thread

* [PATCH v7 7/8] genirq/msi: map/unmap the MSI doorbells on msi_domain_alloc/free_irqs
@ 2016-04-19 18:04     ` kbuild test robot
  0 siblings, 0 replies; 64+ messages in thread
From: kbuild test robot @ 2016-04-19 18:04 UTC (permalink / raw)
  To: linux-arm-kernel

Hi,

[auto build test ERROR on tip/irq/core]
[also build test ERROR on v4.6-rc4 next-20160419]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Eric-Auger/KVM-PCIe-MSI-passthrough-on-ARM-ARM64-kernel-part-2-3-msi-changes/20160420-011957
config: x86_64-lkp (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

>> kernel/irq/msi.c:17:38: fatal error: linux/dma-reserved-iommu.h: No such file or directory
    #include <linux/dma-reserved-iommu.h>
                                         ^
   compilation terminated.

vim +17 kernel/irq/msi.c

    11	 */
    12	#include <linux/types.h>
    13	#include <linux/device.h>
    14	#include <linux/irq.h>
    15	#include <linux/irqdomain.h>
    16	#include <linux/msi.h>
  > 17	#include <linux/dma-reserved-iommu.h>
    18	#include <linux/iommu.h>
    19	
    20	/* Temparory solution for building, will be removed later */

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/octet-stream
Size: 22417 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160420/f6b60d8e/attachment-0001.obj>

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v7 7/8] genirq/msi: map/unmap the MSI doorbells on msi_domain_alloc/free_irqs
@ 2016-04-20  7:47       ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-20  7:47 UTC (permalink / raw)
  To: kbuild test robot
  Cc: kbuild-all, eric.auger, robin.murphy, alex.williamson,
	will.deacon, joro, tglx, jason, marc.zyngier, christoffer.dall,
	linux-arm-kernel, patches, linux-kernel, Bharat.Bhushan,
	pranav.sawargaonkar, p.fedin, iommu, Jean-Philippe.Brucker,
	julien.grall

Hi,

Both reported errors related to this series are due to the fact part n
has dependency on part n-1.

Does anyone know how to let the 0-day CI robot know about such
dependency between series?

If it is an inconvenience I can put all the patches back into the same
big series, specifying tentative patch split according to sub-systems in
the cover-letter?

Thank you in advance

Best Regards

Eric

04/19/2016 08:04 PM, kbuild test robot wrote:
> Hi,
> 
> [auto build test ERROR on tip/irq/core]
> [also build test ERROR on v4.6-rc4 next-20160419]
> [if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Eric-Auger/KVM-PCIe-MSI-passthrough-on-ARM-ARM64-kernel-part-2-3-msi-changes/20160420-011957
> config: x86_64-lkp (attached as .config)
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=x86_64 
> 
> All errors (new ones prefixed by >>):
> 
>>> kernel/irq/msi.c:17:38: fatal error: linux/dma-reserved-iommu.h: No such file or directory
>     #include <linux/dma-reserved-iommu.h>
>                                          ^
>    compilation terminated.
> 
> vim +17 kernel/irq/msi.c
> 
>     11	 */
>     12	#include <linux/types.h>
>     13	#include <linux/device.h>
>     14	#include <linux/irq.h>
>     15	#include <linux/irqdomain.h>
>     16	#include <linux/msi.h>
>   > 17	#include <linux/dma-reserved-iommu.h>
>     18	#include <linux/iommu.h>
>     19	
>     20	/* Temparory solution for building, will be removed later */
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
> 

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v7 7/8] genirq/msi: map/unmap the MSI doorbells on msi_domain_alloc/free_irqs
@ 2016-04-20  7:47       ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-20  7:47 UTC (permalink / raw)
  To: kbuild test robot
  Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	julien.grall-5wv7dgnIgG8, eric.auger-qxv4g6HH51o,
	jason-NLaQJdtUoK4Be96aLqz0jA, patches-QSEj5FYQhm4dnm+yROfE0A,
	marc.zyngier-5wv7dgnIgG8, p.fedin-Sze3O3UU22JBDgjK7y7TUQ,
	will.deacon-5wv7dgnIgG8, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	pranav.sawargaonkar-Re5JQEeQqe8AvxtiuMwx3w,
	kbuild-all-JC7UmRfGjtg, tglx-hfZtesqFncYOwBW4kG4KsQ,
	christoffer.dall-QSEj5FYQhm4dnm+yROfE0A

Hi,

Both reported errors related to this series are due to the fact part n
has dependency on part n-1.

Does anyone know how to let the 0-day CI robot know about such
dependency between series?

If it is an inconvenience I can put all the patches back into the same
big series, specifying tentative patch split according to sub-systems in
the cover-letter?

Thank you in advance

Best Regards

Eric

04/19/2016 08:04 PM, kbuild test robot wrote:
> Hi,
> 
> [auto build test ERROR on tip/irq/core]
> [also build test ERROR on v4.6-rc4 next-20160419]
> [if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Eric-Auger/KVM-PCIe-MSI-passthrough-on-ARM-ARM64-kernel-part-2-3-msi-changes/20160420-011957
> config: x86_64-lkp (attached as .config)
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=x86_64 
> 
> All errors (new ones prefixed by >>):
> 
>>> kernel/irq/msi.c:17:38: fatal error: linux/dma-reserved-iommu.h: No such file or directory
>     #include <linux/dma-reserved-iommu.h>
>                                          ^
>    compilation terminated.
> 
> vim +17 kernel/irq/msi.c
> 
>     11	 */
>     12	#include <linux/types.h>
>     13	#include <linux/device.h>
>     14	#include <linux/irq.h>
>     15	#include <linux/irqdomain.h>
>     16	#include <linux/msi.h>
>   > 17	#include <linux/dma-reserved-iommu.h>
>     18	#include <linux/iommu.h>
>     19	
>     20	/* Temparory solution for building, will be removed later */
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
> 

^ permalink raw reply	[flat|nested] 64+ messages in thread

* [PATCH v7 7/8] genirq/msi: map/unmap the MSI doorbells on msi_domain_alloc/free_irqs
@ 2016-04-20  7:47       ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-20  7:47 UTC (permalink / raw)
  To: linux-arm-kernel

Hi,

Both reported errors related to this series are due to the fact part n
has dependency on part n-1.

Does anyone know how to let the 0-day CI robot know about such
dependency between series?

If it is an inconvenience I can put all the patches back into the same
big series, specifying tentative patch split according to sub-systems in
the cover-letter?

Thank you in advance

Best Regards

Eric

04/19/2016 08:04 PM, kbuild test robot wrote:
> Hi,
> 
> [auto build test ERROR on tip/irq/core]
> [also build test ERROR on v4.6-rc4 next-20160419]
> [if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Eric-Auger/KVM-PCIe-MSI-passthrough-on-ARM-ARM64-kernel-part-2-3-msi-changes/20160420-011957
> config: x86_64-lkp (attached as .config)
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=x86_64 
> 
> All errors (new ones prefixed by >>):
> 
>>> kernel/irq/msi.c:17:38: fatal error: linux/dma-reserved-iommu.h: No such file or directory
>     #include <linux/dma-reserved-iommu.h>
>                                          ^
>    compilation terminated.
> 
> vim +17 kernel/irq/msi.c
> 
>     11	 */
>     12	#include <linux/types.h>
>     13	#include <linux/device.h>
>     14	#include <linux/irq.h>
>     15	#include <linux/irqdomain.h>
>     16	#include <linux/msi.h>
>   > 17	#include <linux/dma-reserved-iommu.h>
>     18	#include <linux/iommu.h>
>     19	
>     20	/* Temparory solution for building, will be removed later */
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
> 

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v7 5/8] genirq/irq: introduce msi_doorbell's structs and related callback
@ 2016-04-20  9:16     ` Marc Zyngier
  0 siblings, 0 replies; 64+ messages in thread
From: Marc Zyngier @ 2016-04-20  9:16 UTC (permalink / raw)
  To: Eric Auger, eric.auger, robin.murphy, alex.williamson,
	will.deacon, joro, tglx, jason, christoffer.dall,
	linux-arm-kernel
  Cc: patches, linux-kernel, Bharat.Bhushan, pranav.sawargaonkar,
	p.fedin, iommu, Jean-Philippe.Brucker, julien.grall

On 19/04/16 18:13, 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.
> The number of doorbells for the whole irqchip is stored in nb_doorbells.
> 
> A doorbell region is characterized by its physical address base, size and
> IOMMU protection flag.
> 
> irq_chip msi_doorbell_info callback enables to retrieve the doorbells of
> the irqchip.
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> ---
> 
> v7: creation
> ---
>  include/linux/irq.h | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index c4de623..fdad8c1 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -312,9 +312,25 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>  	return d->hwirq;
>  }
>  
> -/**
> - * struct irq_chip - hardware interrupt chip descriptor
> - *
> +/* MSI doorbell region */
> +struct irq_chip_msi_doorbell {
> +	phys_addr_t base;
> +	size_t size;
> +	int prot; /* iommu protection flag */

I find this one a bit scary. "int" is a probably not the right type if
it is a set of flags (it should describe both the protection and the
memory attributes - in this case, probably something like Device +
Writeable). You should probably use the same type the IOMMU code uses
(and if it is actually an int, then I'll shut up...).

> +};
> +
> +/*
> + * 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 {
> +	struct irq_chip_msi_doorbell __percpu *percpu_doorbells;
> +	int nb_doorbells; /* overall number of doorbells */
> +};

How can size and prot be different from one CPU to another? It really
feels like they should be common. Can I suggest something like this?

struct irq_chip_msi_doorbell_info {
	phys_addr_t __percpu	*doorbells;
	size_t			size;
	u32			prot;
};

and get rid of struct irq_chip_msi_doorbell altogether?

> +
> +/** * struct irq_chip - hardware interrupt chip descriptor *
>   * @name:		name for /proc/interrupts
>   * @irq_startup:	start up the interrupt (defaults to ->enable if NULL)
>   * @irq_shutdown:	shut down the interrupt (defaults to ->disable if NULL)
> @@ -349,6 +365,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 +411,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);
> -
> +	const 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);
>  
> 

Thanks,

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

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v7 5/8] genirq/irq: introduce msi_doorbell's structs and related callback
@ 2016-04-20  9:16     ` Marc Zyngier
  0 siblings, 0 replies; 64+ messages in thread
From: Marc Zyngier @ 2016-04-20  9:16 UTC (permalink / raw)
  To: Eric Auger, eric.auger-qxv4g6HH51o, robin.murphy-5wv7dgnIgG8,
	alex.williamson-H+wXaHxf7aLQT0dZR+AlfA, will.deacon-5wv7dgnIgG8,
	joro-zLv9SwRftAIdnm+yROfE0A, tglx-hfZtesqFncYOwBW4kG4KsQ,
	jason-NLaQJdtUoK4Be96aLqz0jA,
	christoffer.dall-QSEj5FYQhm4dnm+yROfE0A,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
  Cc: julien.grall-5wv7dgnIgG8, patches-QSEj5FYQhm4dnm+yROfE0A,
	p.fedin-Sze3O3UU22JBDgjK7y7TUQ,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	pranav.sawargaonkar-Re5JQEeQqe8AvxtiuMwx3w

On 19/04/16 18:13, 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.
> The number of doorbells for the whole irqchip is stored in nb_doorbells.
> 
> A doorbell region is characterized by its physical address base, size and
> IOMMU protection flag.
> 
> irq_chip msi_doorbell_info callback enables to retrieve the doorbells of
> the irqchip.
> 
> Signed-off-by: Eric Auger <eric.auger-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> 
> ---
> 
> v7: creation
> ---
>  include/linux/irq.h | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index c4de623..fdad8c1 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -312,9 +312,25 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>  	return d->hwirq;
>  }
>  
> -/**
> - * struct irq_chip - hardware interrupt chip descriptor
> - *
> +/* MSI doorbell region */
> +struct irq_chip_msi_doorbell {
> +	phys_addr_t base;
> +	size_t size;
> +	int prot; /* iommu protection flag */

I find this one a bit scary. "int" is a probably not the right type if
it is a set of flags (it should describe both the protection and the
memory attributes - in this case, probably something like Device +
Writeable). You should probably use the same type the IOMMU code uses
(and if it is actually an int, then I'll shut up...).

> +};
> +
> +/*
> + * 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 {
> +	struct irq_chip_msi_doorbell __percpu *percpu_doorbells;
> +	int nb_doorbells; /* overall number of doorbells */
> +};

How can size and prot be different from one CPU to another? It really
feels like they should be common. Can I suggest something like this?

struct irq_chip_msi_doorbell_info {
	phys_addr_t __percpu	*doorbells;
	size_t			size;
	u32			prot;
};

and get rid of struct irq_chip_msi_doorbell altogether?

> +
> +/** * struct irq_chip - hardware interrupt chip descriptor *
>   * @name:		name for /proc/interrupts
>   * @irq_startup:	start up the interrupt (defaults to ->enable if NULL)
>   * @irq_shutdown:	shut down the interrupt (defaults to ->disable if NULL)
> @@ -349,6 +365,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 +411,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);
> -
> +	const 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);
>  
> 

Thanks,

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

^ permalink raw reply	[flat|nested] 64+ messages in thread

* [PATCH v7 5/8] genirq/irq: introduce msi_doorbell's structs and related callback
@ 2016-04-20  9:16     ` Marc Zyngier
  0 siblings, 0 replies; 64+ messages in thread
From: Marc Zyngier @ 2016-04-20  9:16 UTC (permalink / raw)
  To: linux-arm-kernel

On 19/04/16 18:13, 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.
> The number of doorbells for the whole irqchip is stored in nb_doorbells.
> 
> A doorbell region is characterized by its physical address base, size and
> IOMMU protection flag.
> 
> irq_chip msi_doorbell_info callback enables to retrieve the doorbells of
> the irqchip.
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> ---
> 
> v7: creation
> ---
>  include/linux/irq.h | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index c4de623..fdad8c1 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -312,9 +312,25 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>  	return d->hwirq;
>  }
>  
> -/**
> - * struct irq_chip - hardware interrupt chip descriptor
> - *
> +/* MSI doorbell region */
> +struct irq_chip_msi_doorbell {
> +	phys_addr_t base;
> +	size_t size;
> +	int prot; /* iommu protection flag */

I find this one a bit scary. "int" is a probably not the right type if
it is a set of flags (it should describe both the protection and the
memory attributes - in this case, probably something like Device +
Writeable). You should probably use the same type the IOMMU code uses
(and if it is actually an int, then I'll shut up...).

> +};
> +
> +/*
> + * 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 {
> +	struct irq_chip_msi_doorbell __percpu *percpu_doorbells;
> +	int nb_doorbells; /* overall number of doorbells */
> +};

How can size and prot be different from one CPU to another? It really
feels like they should be common. Can I suggest something like this?

struct irq_chip_msi_doorbell_info {
	phys_addr_t __percpu	*doorbells;
	size_t			size;
	u32			prot;
};

and get rid of struct irq_chip_msi_doorbell altogether?

> +
> +/** * struct irq_chip - hardware interrupt chip descriptor *
>   * @name:		name for /proc/interrupts
>   * @irq_startup:	start up the interrupt (defaults to ->enable if NULL)
>   * @irq_shutdown:	shut down the interrupt (defaults to ->disable if NULL)
> @@ -349,6 +365,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 +411,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);
> -
> +	const 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);
>  
> 

Thanks,

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

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v7 6/8] irqchip/gicv2m: implement msi_doorbell_info callback
@ 2016-04-20  9:27     ` Marc Zyngier
  0 siblings, 0 replies; 64+ messages in thread
From: Marc Zyngier @ 2016-04-20  9:27 UTC (permalink / raw)
  To: Eric Auger, eric.auger, robin.murphy, alex.williamson,
	will.deacon, joro, tglx, jason, christoffer.dall,
	linux-arm-kernel
  Cc: patches, linux-kernel, Bharat.Bhushan, pranav.sawargaonkar,
	p.fedin, iommu, Jean-Philippe.Brucker, julien.grall

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

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v7 6/8] irqchip/gicv2m: implement msi_doorbell_info callback
@ 2016-04-20  9:27     ` Marc Zyngier
  0 siblings, 0 replies; 64+ messages in thread
From: Marc Zyngier @ 2016-04-20  9:27 UTC (permalink / raw)
  To: Eric Auger, eric.auger-qxv4g6HH51o, robin.murphy-5wv7dgnIgG8,
	alex.williamson-H+wXaHxf7aLQT0dZR+AlfA, will.deacon-5wv7dgnIgG8,
	joro-zLv9SwRftAIdnm+yROfE0A, tglx-hfZtesqFncYOwBW4kG4KsQ,
	jason-NLaQJdtUoK4Be96aLqz0jA,
	christoffer.dall-QSEj5FYQhm4dnm+yROfE0A,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
  Cc: julien.grall-5wv7dgnIgG8, patches-QSEj5FYQhm4dnm+yROfE0A,
	p.fedin-Sze3O3UU22JBDgjK7y7TUQ,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	pranav.sawargaonkar-Re5JQEeQqe8AvxtiuMwx3w

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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...

^ permalink raw reply	[flat|nested] 64+ messages in thread

* [PATCH v7 6/8] irqchip/gicv2m: implement msi_doorbell_info callback
@ 2016-04-20  9:27     ` Marc Zyngier
  0 siblings, 0 replies; 64+ messages in thread
From: Marc Zyngier @ 2016-04-20  9:27 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v7 5/8] genirq/irq: introduce msi_doorbell's structs and related callback
@ 2016-04-20  9:36       ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-20  9:36 UTC (permalink / raw)
  To: Marc Zyngier, eric.auger, robin.murphy, alex.williamson,
	will.deacon, joro, tglx, jason, christoffer.dall,
	linux-arm-kernel
  Cc: patches, linux-kernel, Bharat.Bhushan, pranav.sawargaonkar,
	p.fedin, iommu, Jean-Philippe.Brucker, julien.grall

Hi Marc,
On 04/20/2016 11:16 AM, Marc Zyngier wrote:
> On 19/04/16 18:13, 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.
>> The number of doorbells for the whole irqchip is stored in nb_doorbells.
>>
>> A doorbell region is characterized by its physical address base, size and
>> IOMMU protection flag.
>>
>> irq_chip msi_doorbell_info callback enables to retrieve the doorbells of
>> the irqchip.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>>
>> v7: creation
>> ---
>>  include/linux/irq.h | 26 ++++++++++++++++++++++----
>>  1 file changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/linux/irq.h b/include/linux/irq.h
>> index c4de623..fdad8c1 100644
>> --- a/include/linux/irq.h
>> +++ b/include/linux/irq.h
>> @@ -312,9 +312,25 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>>  	return d->hwirq;
>>  }
>>  
>> -/**
>> - * struct irq_chip - hardware interrupt chip descriptor
>> - *
>> +/* MSI doorbell region */
>> +struct irq_chip_msi_doorbell {
>> +	phys_addr_t base;
>> +	size_t size;
>> +	int prot; /* iommu protection flag */
> 
> I find this one a bit scary. "int" is a probably not the right type if
> it is a set of flags (it should describe both the protection and the
> memory attributes - in this case, probably something like Device +
> Writeable). You should probably use the same type the IOMMU code uses
> (and if it is actually an int, then I'll shut up...).
Hum yes iommu also uses an int ;-)
> 
>> +};
>> +
>> +/*
>> + * 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 {
>> +	struct irq_chip_msi_doorbell __percpu *percpu_doorbells;
>> +	int nb_doorbells; /* overall number of doorbells */
>> +};
> 
> How can size and prot be different from one CPU to another? It really
> feels like they should be common. Can I suggest something like this?
> 
> struct irq_chip_msi_doorbell_info {
> 	phys_addr_t __percpu	*doorbells;
> 	size_t			size;
> 	u32			prot;
> };
> 
> and get rid of struct irq_chip_msi_doorbell altogether?
I'am definitively fine with your proposal.

Thanks

Eric
> 
>> +
>> +/** * struct irq_chip - hardware interrupt chip descriptor *
>>   * @name:		name for /proc/interrupts
>>   * @irq_startup:	start up the interrupt (defaults to ->enable if NULL)
>>   * @irq_shutdown:	shut down the interrupt (defaults to ->disable if NULL)
>> @@ -349,6 +365,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 +411,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);
>> -
>> +	const 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);
>>  
>>
> 
> Thanks,
> 
> 	M.
> 

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v7 5/8] genirq/irq: introduce msi_doorbell's structs and related callback
@ 2016-04-20  9:36       ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-20  9:36 UTC (permalink / raw)
  To: Marc Zyngier, eric.auger-qxv4g6HH51o, robin.murphy-5wv7dgnIgG8,
	alex.williamson-H+wXaHxf7aLQT0dZR+AlfA, will.deacon-5wv7dgnIgG8,
	joro-zLv9SwRftAIdnm+yROfE0A, tglx-hfZtesqFncYOwBW4kG4KsQ,
	jason-NLaQJdtUoK4Be96aLqz0jA,
	christoffer.dall-QSEj5FYQhm4dnm+yROfE0A,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
  Cc: julien.grall-5wv7dgnIgG8, patches-QSEj5FYQhm4dnm+yROfE0A,
	p.fedin-Sze3O3UU22JBDgjK7y7TUQ,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	pranav.sawargaonkar-Re5JQEeQqe8AvxtiuMwx3w

Hi Marc,
On 04/20/2016 11:16 AM, Marc Zyngier wrote:
> On 19/04/16 18:13, 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.
>> The number of doorbells for the whole irqchip is stored in nb_doorbells.
>>
>> A doorbell region is characterized by its physical address base, size and
>> IOMMU protection flag.
>>
>> irq_chip msi_doorbell_info callback enables to retrieve the doorbells of
>> the irqchip.
>>
>> Signed-off-by: Eric Auger <eric.auger-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>
>> ---
>>
>> v7: creation
>> ---
>>  include/linux/irq.h | 26 ++++++++++++++++++++++----
>>  1 file changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/linux/irq.h b/include/linux/irq.h
>> index c4de623..fdad8c1 100644
>> --- a/include/linux/irq.h
>> +++ b/include/linux/irq.h
>> @@ -312,9 +312,25 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>>  	return d->hwirq;
>>  }
>>  
>> -/**
>> - * struct irq_chip - hardware interrupt chip descriptor
>> - *
>> +/* MSI doorbell region */
>> +struct irq_chip_msi_doorbell {
>> +	phys_addr_t base;
>> +	size_t size;
>> +	int prot; /* iommu protection flag */
> 
> I find this one a bit scary. "int" is a probably not the right type if
> it is a set of flags (it should describe both the protection and the
> memory attributes - in this case, probably something like Device +
> Writeable). You should probably use the same type the IOMMU code uses
> (and if it is actually an int, then I'll shut up...).
Hum yes iommu also uses an int ;-)
> 
>> +};
>> +
>> +/*
>> + * 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 {
>> +	struct irq_chip_msi_doorbell __percpu *percpu_doorbells;
>> +	int nb_doorbells; /* overall number of doorbells */
>> +};
> 
> How can size and prot be different from one CPU to another? It really
> feels like they should be common. Can I suggest something like this?
> 
> struct irq_chip_msi_doorbell_info {
> 	phys_addr_t __percpu	*doorbells;
> 	size_t			size;
> 	u32			prot;
> };
> 
> and get rid of struct irq_chip_msi_doorbell altogether?
I'am definitively fine with your proposal.

Thanks

Eric
> 
>> +
>> +/** * struct irq_chip - hardware interrupt chip descriptor *
>>   * @name:		name for /proc/interrupts
>>   * @irq_startup:	start up the interrupt (defaults to ->enable if NULL)
>>   * @irq_shutdown:	shut down the interrupt (defaults to ->disable if NULL)
>> @@ -349,6 +365,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 +411,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);
>> -
>> +	const 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);
>>  
>>
> 
> Thanks,
> 
> 	M.
> 

^ permalink raw reply	[flat|nested] 64+ messages in thread

* [PATCH v7 5/8] genirq/irq: introduce msi_doorbell's structs and related callback
@ 2016-04-20  9:36       ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-20  9:36 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Marc,
On 04/20/2016 11:16 AM, Marc Zyngier wrote:
> On 19/04/16 18:13, 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.
>> The number of doorbells for the whole irqchip is stored in nb_doorbells.
>>
>> A doorbell region is characterized by its physical address base, size and
>> IOMMU protection flag.
>>
>> irq_chip msi_doorbell_info callback enables to retrieve the doorbells of
>> the irqchip.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>>
>> v7: creation
>> ---
>>  include/linux/irq.h | 26 ++++++++++++++++++++++----
>>  1 file changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/linux/irq.h b/include/linux/irq.h
>> index c4de623..fdad8c1 100644
>> --- a/include/linux/irq.h
>> +++ b/include/linux/irq.h
>> @@ -312,9 +312,25 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>>  	return d->hwirq;
>>  }
>>  
>> -/**
>> - * struct irq_chip - hardware interrupt chip descriptor
>> - *
>> +/* MSI doorbell region */
>> +struct irq_chip_msi_doorbell {
>> +	phys_addr_t base;
>> +	size_t size;
>> +	int prot; /* iommu protection flag */
> 
> I find this one a bit scary. "int" is a probably not the right type if
> it is a set of flags (it should describe both the protection and the
> memory attributes - in this case, probably something like Device +
> Writeable). You should probably use the same type the IOMMU code uses
> (and if it is actually an int, then I'll shut up...).
Hum yes iommu also uses an int ;-)
> 
>> +};
>> +
>> +/*
>> + * 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 {
>> +	struct irq_chip_msi_doorbell __percpu *percpu_doorbells;
>> +	int nb_doorbells; /* overall number of doorbells */
>> +};
> 
> How can size and prot be different from one CPU to another? It really
> feels like they should be common. Can I suggest something like this?
> 
> struct irq_chip_msi_doorbell_info {
> 	phys_addr_t __percpu	*doorbells;
> 	size_t			size;
> 	u32			prot;
> };
> 
> and get rid of struct irq_chip_msi_doorbell altogether?
I'am definitively fine with your proposal.

Thanks

Eric
> 
>> +
>> +/** * struct irq_chip - hardware interrupt chip descriptor *
>>   * @name:		name for /proc/interrupts
>>   * @irq_startup:	start up the interrupt (defaults to ->enable if NULL)
>>   * @irq_shutdown:	shut down the interrupt (defaults to ->disable if NULL)
>> @@ -349,6 +365,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 +411,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);
>> -
>> +	const 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);
>>  
>>
> 
> Thanks,
> 
> 	M.
> 

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v7 8/8] genirq/msi: use the MSI doorbell's IOVA when requested
  2016-04-19 17:13   ` Eric Auger
@ 2016-04-20  9:44     ` Marc Zyngier
  -1 siblings, 0 replies; 64+ messages in thread
From: Marc Zyngier @ 2016-04-20  9:44 UTC (permalink / raw)
  To: Eric Auger, eric.auger, robin.murphy, alex.williamson,
	will.deacon, joro, tglx, jason, christoffer.dall,
	linux-arm-kernel
  Cc: patches, linux-kernel, Bharat.Bhushan, pranav.sawargaonkar,
	p.fedin, iommu, Jean-Philippe.Brucker, julien.grall

On 19/04/16 18:13, 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>
> 
> ---
> 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 | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
> index e45907e..0ebb2d8 100644
> --- a/kernel/irq/msi.c
> +++ b/kernel/irq/msi.c
> @@ -19,6 +19,7 @@
>  
>  /* Temparory solution for building, will be removed later */
>  #include <linux/pci.h>
> +#include <linux/dma-reserved-iommu.h>
>  
>  struct msi_desc *alloc_msi_entry(struct device *dev)
>  {
> @@ -64,8 +65,12 @@ static int msi_compose(struct irq_data *irq_data,
>  
>  	if (erase)
>  		memset(msg, 0, sizeof(*msg));
> -	else
> +	else {
>  		ret = irq_chip_compose_msi_msg(irq_data, msg);
> +		if (ret)
> +			return ret;
> +		iommu_msi_mapping_translate_msg(irq_data, msg);

I've just commented on this function in its respective series. I really
think it deals with the wrong data structure. Id rather see something like:

struct device *dev = msi_desc_to_dev(irq_data_get_msi_desc(irq_data));
iommu_msi_msg_pa_to_va(dev, msg);

Thanks,

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

^ permalink raw reply	[flat|nested] 64+ messages in thread

* [PATCH v7 8/8] genirq/msi: use the MSI doorbell's IOVA when requested
@ 2016-04-20  9:44     ` Marc Zyngier
  0 siblings, 0 replies; 64+ messages in thread
From: Marc Zyngier @ 2016-04-20  9:44 UTC (permalink / raw)
  To: linux-arm-kernel

On 19/04/16 18:13, 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>
> 
> ---
> 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 | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
> index e45907e..0ebb2d8 100644
> --- a/kernel/irq/msi.c
> +++ b/kernel/irq/msi.c
> @@ -19,6 +19,7 @@
>  
>  /* Temparory solution for building, will be removed later */
>  #include <linux/pci.h>
> +#include <linux/dma-reserved-iommu.h>
>  
>  struct msi_desc *alloc_msi_entry(struct device *dev)
>  {
> @@ -64,8 +65,12 @@ static int msi_compose(struct irq_data *irq_data,
>  
>  	if (erase)
>  		memset(msg, 0, sizeof(*msg));
> -	else
> +	else {
>  		ret = irq_chip_compose_msi_msg(irq_data, msg);
> +		if (ret)
> +			return ret;
> +		iommu_msi_mapping_translate_msg(irq_data, msg);

I've just commented on this function in its respective series. I really
think it deals with the wrong data structure. Id rather see something like:

struct device *dev = msi_desc_to_dev(irq_data_get_msi_desc(irq_data));
iommu_msi_msg_pa_to_va(dev, msg);

Thanks,

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

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v7 6/8] irqchip/gicv2m: implement msi_doorbell_info callback
@ 2016-04-20 12:33       ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-20 12:33 UTC (permalink / raw)
  To: Marc Zyngier, eric.auger, robin.murphy, alex.williamson,
	will.deacon, joro, tglx, jason, christoffer.dall,
	linux-arm-kernel
  Cc: patches, linux-kernel, Bharat.Bhushan, pranav.sawargaonkar,
	p.fedin, iommu, Jean-Philippe.Brucker, julien.grall

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

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v7 6/8] irqchip/gicv2m: implement msi_doorbell_info callback
@ 2016-04-20 12:33       ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-20 12:33 UTC (permalink / raw)
  To: Marc Zyngier, eric.auger-qxv4g6HH51o, robin.murphy-5wv7dgnIgG8,
	alex.williamson-H+wXaHxf7aLQT0dZR+AlfA, will.deacon-5wv7dgnIgG8,
	joro-zLv9SwRftAIdnm+yROfE0A, tglx-hfZtesqFncYOwBW4kG4KsQ,
	jason-NLaQJdtUoK4Be96aLqz0jA,
	christoffer.dall-QSEj5FYQhm4dnm+yROfE0A,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
  Cc: julien.grall-5wv7dgnIgG8, patches-QSEj5FYQhm4dnm+yROfE0A,
	p.fedin-Sze3O3UU22JBDgjK7y7TUQ,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	pranav.sawargaonkar-Re5JQEeQqe8AvxtiuMwx3w

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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.
> 

^ permalink raw reply	[flat|nested] 64+ messages in thread

* [PATCH v7 6/8] irqchip/gicv2m: implement msi_doorbell_info callback
@ 2016-04-20 12:33       ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-20 12:33 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v7 6/8] irqchip/gicv2m: implement msi_doorbell_info callback
@ 2016-04-20 17:56         ` Marc Zyngier
  0 siblings, 0 replies; 64+ messages in thread
From: Marc Zyngier @ 2016-04-20 17:56 UTC (permalink / raw)
  To: Eric Auger
  Cc: eric.auger, robin.murphy, alex.williamson, will.deacon, joro,
	tglx, jason, christoffer.dall, linux-arm-kernel, patches,
	linux-kernel, Bharat.Bhushan, pranav.sawargaonkar, p.fedin,
	iommu, Jean-Philippe.Brucker, julien.grall

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.

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v7 6/8] irqchip/gicv2m: implement msi_doorbell_info callback
@ 2016-04-20 17:56         ` Marc Zyngier
  0 siblings, 0 replies; 64+ messages in thread
From: Marc Zyngier @ 2016-04-20 17:56 UTC (permalink / raw)
  To: Eric Auger
  Cc: julien.grall-5wv7dgnIgG8, eric.auger-qxv4g6HH51o,
	jason-NLaQJdtUoK4Be96aLqz0jA, patches-QSEj5FYQhm4dnm+yROfE0A,
	p.fedin-Sze3O3UU22JBDgjK7y7TUQ, will.deacon-5wv7dgnIgG8,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	pranav.sawargaonkar-Re5JQEeQqe8AvxtiuMwx3w,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	tglx-hfZtesqFncYOwBW4kG4KsQ,
	christoffer.dall-QSEj5FYQhm4dnm+yROfE0A

On Wed, 20 Apr 2016 14:33:17 +0200
Eric Auger <eric.auger-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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.

^ permalink raw reply	[flat|nested] 64+ messages in thread

* [PATCH v7 6/8] irqchip/gicv2m: implement msi_doorbell_info callback
@ 2016-04-20 17:56         ` Marc Zyngier
  0 siblings, 0 replies; 64+ messages in thread
From: Marc Zyngier @ 2016-04-20 17:56 UTC (permalink / raw)
  To: linux-arm-kernel

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.

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v7 6/8] irqchip/gicv2m: implement msi_doorbell_info callback
@ 2016-04-20 18:16           ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-20 18:16 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: eric.auger, robin.murphy, alex.williamson, will.deacon, joro,
	tglx, jason, christoffer.dall, linux-arm-kernel, patches,
	linux-kernel, Bharat.Bhushan, pranav.sawargaonkar, p.fedin,
	iommu, Jean-Philippe.Brucker, julien.grall

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

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v7 6/8] irqchip/gicv2m: implement msi_doorbell_info callback
@ 2016-04-20 18:16           ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-20 18:16 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: julien.grall-5wv7dgnIgG8, eric.auger-qxv4g6HH51o,
	jason-NLaQJdtUoK4Be96aLqz0jA, patches-QSEj5FYQhm4dnm+yROfE0A,
	p.fedin-Sze3O3UU22JBDgjK7y7TUQ, will.deacon-5wv7dgnIgG8,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	pranav.sawargaonkar-Re5JQEeQqe8AvxtiuMwx3w,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	tglx-hfZtesqFncYOwBW4kG4KsQ,
	christoffer.dall-QSEj5FYQhm4dnm+yROfE0A

Marc,
On 04/20/2016 07:56 PM, Marc Zyngier wrote:
> On Wed, 20 Apr 2016 14:33:17 +0200
> Eric Auger <eric.auger-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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.
> 

^ permalink raw reply	[flat|nested] 64+ messages in thread

* [PATCH v7 6/8] irqchip/gicv2m: implement msi_doorbell_info callback
@ 2016-04-20 18:16           ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-20 18:16 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v7 1/8] genirq/msi: Add a new MSI_FLAG_IRQ_REMAPPING flag
  2016-04-19 17:13   ` Eric Auger
@ 2016-04-22 11:02     ` Robin Murphy
  -1 siblings, 0 replies; 64+ messages in thread
From: Robin Murphy @ 2016-04-22 11:02 UTC (permalink / raw)
  To: Eric Auger, eric.auger, alex.williamson, will.deacon, joro, tglx,
	jason, marc.zyngier, christoffer.dall, linux-arm-kernel
  Cc: patches, linux-kernel, Bharat.Bhushan, pranav.sawargaonkar,
	p.fedin, iommu, Jean-Philippe.Brucker, julien.grall

Hi Eric,

On 19/04/16 18:13, Eric Auger wrote:
> 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.

Perhaps a nitpick, but given the earlier confusion about what the IOMMU 
flag actually meant this prompts me to wonder if it's worth adjusting 
the general terminology before we propagate it further. What I think we 
actually care about is that one thing or the other "provides MSI 
isolation" rather than "supports MSI remapping", since the latter is all 
to easy to misinterpret the way we did in the SMMU drivers.

Robin.

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

^ permalink raw reply	[flat|nested] 64+ messages in thread

* [PATCH v7 1/8] genirq/msi: Add a new MSI_FLAG_IRQ_REMAPPING flag
@ 2016-04-22 11:02     ` Robin Murphy
  0 siblings, 0 replies; 64+ messages in thread
From: Robin Murphy @ 2016-04-22 11:02 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Eric,

On 19/04/16 18:13, Eric Auger wrote:
> 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.

Perhaps a nitpick, but given the earlier confusion about what the IOMMU 
flag actually meant this prompts me to wonder if it's worth adjusting 
the general terminology before we propagate it further. What I think we 
actually care about is that one thing or the other "provides MSI 
isolation" rather than "supports MSI remapping", since the latter is all 
to easy to misinterpret the way we did in the SMMU drivers.

Robin.

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

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v7 1/8] genirq/msi: Add a new MSI_FLAG_IRQ_REMAPPING flag
@ 2016-04-22 12:25       ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-22 12:25 UTC (permalink / raw)
  To: Robin Murphy, eric.auger, alex.williamson, will.deacon, joro,
	tglx, jason, marc.zyngier, christoffer.dall, linux-arm-kernel
  Cc: patches, linux-kernel, Bharat.Bhushan, pranav.sawargaonkar,
	p.fedin, iommu, Jean-Philippe.Brucker, julien.grall

Robin,
On 04/22/2016 01:02 PM, Robin Murphy wrote:
> Hi Eric,
> 
> On 19/04/16 18:13, Eric Auger wrote:
>> 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.
> 
> Perhaps a nitpick, but given the earlier confusion about what the IOMMU
> flag actually meant this prompts me to wonder if it's worth adjusting
> the general terminology before we propagate it further. What I think we
> actually care about is that one thing or the other "provides MSI
> isolation" rather than "supports MSI remapping", since the latter is all
> to easy to misinterpret the way we did in the SMMU drivers.

The only concern I have is https://lkml.org/lkml/2016/4/18/283 attempts
to define a PCI bus flag dubbed PCI_BUS_FLAGS_MSI_REMAP combining the
iommu & msi layer info. In that sense x86 people may not be keen of
having different terminaologies. Anyway I will follow the consensus, if any.

Best Regards

Eric


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

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v7 1/8] genirq/msi: Add a new MSI_FLAG_IRQ_REMAPPING flag
@ 2016-04-22 12:25       ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-22 12:25 UTC (permalink / raw)
  To: Robin Murphy, eric.auger-qxv4g6HH51o,
	alex.williamson-H+wXaHxf7aLQT0dZR+AlfA, will.deacon-5wv7dgnIgG8,
	joro-zLv9SwRftAIdnm+yROfE0A, tglx-hfZtesqFncYOwBW4kG4KsQ,
	jason-NLaQJdtUoK4Be96aLqz0jA, marc.zyngier-5wv7dgnIgG8,
	christoffer.dall-QSEj5FYQhm4dnm+yROfE0A,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
  Cc: julien.grall-5wv7dgnIgG8, patches-QSEj5FYQhm4dnm+yROfE0A,
	p.fedin-Sze3O3UU22JBDgjK7y7TUQ,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	pranav.sawargaonkar-Re5JQEeQqe8AvxtiuMwx3w

Robin,
On 04/22/2016 01:02 PM, Robin Murphy wrote:
> Hi Eric,
> 
> On 19/04/16 18:13, Eric Auger wrote:
>> 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.
> 
> Perhaps a nitpick, but given the earlier confusion about what the IOMMU
> flag actually meant this prompts me to wonder if it's worth adjusting
> the general terminology before we propagate it further. What I think we
> actually care about is that one thing or the other "provides MSI
> isolation" rather than "supports MSI remapping", since the latter is all
> to easy to misinterpret the way we did in the SMMU drivers.

The only concern I have is https://lkml.org/lkml/2016/4/18/283 attempts
to define a PCI bus flag dubbed PCI_BUS_FLAGS_MSI_REMAP combining the
iommu & msi layer info. In that sense x86 people may not be keen of
having different terminaologies. Anyway I will follow the consensus, if any.

Best Regards

Eric


> 
> Robin.
> 
>> Signed-off-by: Eric Auger <eric.auger-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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,
>>
> 

^ permalink raw reply	[flat|nested] 64+ messages in thread

* [PATCH v7 1/8] genirq/msi: Add a new MSI_FLAG_IRQ_REMAPPING flag
@ 2016-04-22 12:25       ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-22 12:25 UTC (permalink / raw)
  To: linux-arm-kernel

Robin,
On 04/22/2016 01:02 PM, Robin Murphy wrote:
> Hi Eric,
> 
> On 19/04/16 18:13, Eric Auger wrote:
>> 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.
> 
> Perhaps a nitpick, but given the earlier confusion about what the IOMMU
> flag actually meant this prompts me to wonder if it's worth adjusting
> the general terminology before we propagate it further. What I think we
> actually care about is that one thing or the other "provides MSI
> isolation" rather than "supports MSI remapping", since the latter is all
> to easy to misinterpret the way we did in the SMMU drivers.

The only concern I have is https://lkml.org/lkml/2016/4/18/283 attempts
to define a PCI bus flag dubbed PCI_BUS_FLAGS_MSI_REMAP combining the
iommu & msi layer info. In that sense x86 people may not be keen of
having different terminaologies. Anyway I will follow the consensus, if any.

Best Regards

Eric


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

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v7 1/8] genirq/msi: Add a new MSI_FLAG_IRQ_REMAPPING flag
  2016-04-22 12:25       ` Eric Auger
@ 2016-04-22 15:39         ` Thomas Gleixner
  -1 siblings, 0 replies; 64+ messages in thread
From: Thomas Gleixner @ 2016-04-22 15:39 UTC (permalink / raw)
  To: Eric Auger
  Cc: Robin Murphy, eric.auger, alex.williamson, will.deacon, joro,
	jason, marc.zyngier, christoffer.dall, linux-arm-kernel, patches,
	linux-kernel, Bharat.Bhushan, pranav.sawargaonkar, p.fedin,
	iommu, Jean-Philippe.Brucker, julien.grall

On Fri, 22 Apr 2016, Eric Auger wrote:
> Robin,
> On 04/22/2016 01:02 PM, Robin Murphy wrote:
> > Hi Eric,
> > 
> > On 19/04/16 18:13, Eric Auger wrote:
> >> 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.
> > 
> > Perhaps a nitpick, but given the earlier confusion about what the IOMMU
> > flag actually meant this prompts me to wonder if it's worth adjusting
> > the general terminology before we propagate it further. What I think we
> > actually care about is that one thing or the other "provides MSI
> > isolation" rather than "supports MSI remapping", since the latter is all
> > to easy to misinterpret the way we did in the SMMU drivers.
> 
> The only concern I have is https://lkml.org/lkml/2016/4/18/283 attempts
> to define a PCI bus flag dubbed PCI_BUS_FLAGS_MSI_REMAP combining the
> iommu & msi layer info. In that sense x86 people may not be keen of
> having different terminaologies. Anyway I will follow the consensus, if any.

Yes, please keep that consistent. It makes 'grep' much more conveniant.

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 64+ messages in thread

* [PATCH v7 1/8] genirq/msi: Add a new MSI_FLAG_IRQ_REMAPPING flag
@ 2016-04-22 15:39         ` Thomas Gleixner
  0 siblings, 0 replies; 64+ messages in thread
From: Thomas Gleixner @ 2016-04-22 15:39 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, 22 Apr 2016, Eric Auger wrote:
> Robin,
> On 04/22/2016 01:02 PM, Robin Murphy wrote:
> > Hi Eric,
> > 
> > On 19/04/16 18:13, Eric Auger wrote:
> >> 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.
> > 
> > Perhaps a nitpick, but given the earlier confusion about what the IOMMU
> > flag actually meant this prompts me to wonder if it's worth adjusting
> > the general terminology before we propagate it further. What I think we
> > actually care about is that one thing or the other "provides MSI
> > isolation" rather than "supports MSI remapping", since the latter is all
> > to easy to misinterpret the way we did in the SMMU drivers.
> 
> The only concern I have is https://lkml.org/lkml/2016/4/18/283 attempts
> to define a PCI bus flag dubbed PCI_BUS_FLAGS_MSI_REMAP combining the
> iommu & msi layer info. In that sense x86 people may not be keen of
> having different terminaologies. Anyway I will follow the consensus, if any.

Yes, please keep that consistent. It makes 'grep' much more conveniant.

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [lkp] [PATCH v7 7/8] genirq/msi: map/unmap the MSI doorbells on msi_domain_alloc/free_irqs
  2016-04-20  7:47       ` Eric Auger
@ 2016-04-26  1:24         ` Ye Xiaolong
  -1 siblings, 0 replies; 64+ messages in thread
From: Ye Xiaolong @ 2016-04-26  1:24 UTC (permalink / raw)
  To: Eric Auger
  Cc: kbuild test robot, kbuild-all, eric.auger, robin.murphy,
	alex.williamson, will.deacon, joro, tglx, jason, marc.zyngier,
	christoffer.dall, linux-arm-kernel, patches, linux-kernel,
	Bharat.Bhushan, pranav.sawargaonkar, p.fedin, iommu,
	Jean-Philippe.Brucker, julien.grall

On Wed, Apr 20, 2016 at 09:47:19AM +0200, Eric Auger wrote:
>Hi,
>
Hi, Eric
>Both reported errors related to this series are due to the fact part n
>has dependency on part n-1.
>

If I understand correctly, what you meant is that you send patch series
(let's say B), and B has its dependency patch series A which is in-flight
and hasn't been merged, so the commit history is like:

        ---P---A---B

where P is the well-known commit in public linux tree. In this case,
0day just applied B based on P without A, thus caused the build error.

>Does anyone know how to let the 0-day CI robot know about such
>dependency between series?
>

Currently, we have proposed to add a new '--base' option for git-format-patch
to git community[1], developers could use this option to record the base tree
information which could help 0day to identify the state the patch series
applies to, thus 0day could avoid false report like this case.

For example, imagine that on top of the public commit P, you applied well-known
patches X, Y and Z from somebody else or yourself, and then built your
three-patch series A, B, C, the history would be like:

................................................
---P---X---Y---Z---A---B---C
................................................

With `git format-patch --base=P -3 C` (or variants thereof, e.g. with
`--cover-letter` of using `Z..C` instead of `-3 C` to specify the
range), the base tree information block is shown at the end of the
first message the command outputs (either the first patch, or the
cover letter), like this:

------------
base-commit: P
prerequisite-patch-id: X
prerequisite-patch-id: Y
prerequisite-patch-id: Z
------------

Now this feature is still under review in git community[2], hope it could be
merged in next git release.


[1] http://thread.gmane.org/gmane.comp.version-control.git/286873
[2] http://thread.gmane.org/gmane.comp.version-control.git/292168

Thanks,
Xiaolong

>If it is an inconvenience I can put all the patches back into the same
>big series, specifying tentative patch split according to sub-systems in
>the cover-letter?
>
>Thank you in advance
>
>Best Regards
>
>Eric
>
>04/19/2016 08:04 PM, kbuild test robot wrote:
>> Hi,
>> 
>> [auto build test ERROR on tip/irq/core]
>> [also build test ERROR on v4.6-rc4 next-20160419]
>> [if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
>> 
>> url:    https://github.com/0day-ci/linux/commits/Eric-Auger/KVM-PCIe-MSI-passthrough-on-ARM-ARM64-kernel-part-2-3-msi-changes/20160420-011957
>> config: x86_64-lkp (attached as .config)
>> reproduce:
>>         # save the attached .config to linux build tree
>>         make ARCH=x86_64 
>> 
>> All errors (new ones prefixed by >>):
>> 
>>>> kernel/irq/msi.c:17:38: fatal error: linux/dma-reserved-iommu.h: No such file or directory
>>     #include <linux/dma-reserved-iommu.h>
>>                                          ^
>>    compilation terminated.
>> 
>> vim +17 kernel/irq/msi.c
>> 
>>     11	 */
>>     12	#include <linux/types.h>
>>     13	#include <linux/device.h>
>>     14	#include <linux/irq.h>
>>     15	#include <linux/irqdomain.h>
>>     16	#include <linux/msi.h>
>>   > 17	#include <linux/dma-reserved-iommu.h>
>>     18	#include <linux/iommu.h>
>>     19	
>>     20	/* Temparory solution for building, will be removed later */
>> 
>> ---
>> 0-DAY kernel test infrastructure                Open Source Technology Center
>> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
>> 
>

^ permalink raw reply	[flat|nested] 64+ messages in thread

* [lkp] [PATCH v7 7/8] genirq/msi: map/unmap the MSI doorbells on msi_domain_alloc/free_irqs
@ 2016-04-26  1:24         ` Ye Xiaolong
  0 siblings, 0 replies; 64+ messages in thread
From: Ye Xiaolong @ 2016-04-26  1:24 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Apr 20, 2016 at 09:47:19AM +0200, Eric Auger wrote:
>Hi,
>
Hi, Eric
>Both reported errors related to this series are due to the fact part n
>has dependency on part n-1.
>

If I understand correctly, what you meant is that you send patch series
(let's say B), and B has its dependency patch series A which is in-flight
and hasn't been merged, so the commit history is like:

        ---P---A---B

where P is the well-known commit in public linux tree. In this case,
0day just applied B based on P without A, thus caused the build error.

>Does anyone know how to let the 0-day CI robot know about such
>dependency between series?
>

Currently, we have proposed to add a new '--base' option for git-format-patch
to git community[1], developers could use this option to record the base tree
information which could help 0day to identify the state the patch series
applies to, thus 0day could avoid false report like this case.

For example, imagine that on top of the public commit P, you applied well-known
patches X, Y and Z from somebody else or yourself, and then built your
three-patch series A, B, C, the history would be like:

................................................
---P---X---Y---Z---A---B---C
................................................

With `git format-patch --base=P -3 C` (or variants thereof, e.g. with
`--cover-letter` of using `Z..C` instead of `-3 C` to specify the
range), the base tree information block is shown at the end of the
first message the command outputs (either the first patch, or the
cover letter), like this:

------------
base-commit: P
prerequisite-patch-id: X
prerequisite-patch-id: Y
prerequisite-patch-id: Z
------------

Now this feature is still under review in git community[2], hope it could be
merged in next git release.


[1] http://thread.gmane.org/gmane.comp.version-control.git/286873
[2] http://thread.gmane.org/gmane.comp.version-control.git/292168

Thanks,
Xiaolong

>If it is an inconvenience I can put all the patches back into the same
>big series, specifying tentative patch split according to sub-systems in
>the cover-letter?
>
>Thank you in advance
>
>Best Regards
>
>Eric
>
>04/19/2016 08:04 PM, kbuild test robot wrote:
>> Hi,
>> 
>> [auto build test ERROR on tip/irq/core]
>> [also build test ERROR on v4.6-rc4 next-20160419]
>> [if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
>> 
>> url:    https://github.com/0day-ci/linux/commits/Eric-Auger/KVM-PCIe-MSI-passthrough-on-ARM-ARM64-kernel-part-2-3-msi-changes/20160420-011957
>> config: x86_64-lkp (attached as .config)
>> reproduce:
>>         # save the attached .config to linux build tree
>>         make ARCH=x86_64 
>> 
>> All errors (new ones prefixed by >>):
>> 
>>>> kernel/irq/msi.c:17:38: fatal error: linux/dma-reserved-iommu.h: No such file or directory
>>     #include <linux/dma-reserved-iommu.h>
>>                                          ^
>>    compilation terminated.
>> 
>> vim +17 kernel/irq/msi.c
>> 
>>     11	 */
>>     12	#include <linux/types.h>
>>     13	#include <linux/device.h>
>>     14	#include <linux/irq.h>
>>     15	#include <linux/irqdomain.h>
>>     16	#include <linux/msi.h>
>>   > 17	#include <linux/dma-reserved-iommu.h>
>>     18	#include <linux/iommu.h>
>>     19	
>>     20	/* Temparory solution for building, will be removed later */
>> 
>> ---
>> 0-DAY kernel test infrastructure                Open Source Technology Center
>> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
>> 
>

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [lkp] [PATCH v7 7/8] genirq/msi: map/unmap the MSI doorbells on msi_domain_alloc/free_irqs
  2016-04-26  1:24         ` Ye Xiaolong
  (?)
@ 2016-04-26 16:43           ` Eric Auger
  -1 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-26 16:43 UTC (permalink / raw)
  To: Ye Xiaolong
  Cc: kbuild test robot, kbuild-all, eric.auger, robin.murphy,
	alex.williamson, will.deacon, joro, tglx, jason, marc.zyngier,
	christoffer.dall, linux-arm-kernel, patches, linux-kernel,
	Bharat.Bhushan, pranav.sawargaonkar, p.fedin, iommu,
	Jean-Philippe.Brucker, julien.grall

Hi Xiaolong
On 04/26/2016 03:24 AM, Ye Xiaolong wrote:
> On Wed, Apr 20, 2016 at 09:47:19AM +0200, Eric Auger wrote:
>> Hi,
>>
> Hi, Eric
>> Both reported errors related to this series are due to the fact part n
>> has dependency on part n-1.
>>
> 
> If I understand correctly, what you meant is that you send patch series
> (let's say B), and B has its dependency patch series A which is in-flight
> and hasn't been merged, so the commit history is like:
> 
>         ---P---A---B
> 
> where P is the well-known commit in public linux tree. In this case,
> 0day just applied B based on P without A, thus caused the build error.

yes that's it.
> 
>> Does anyone know how to let the 0-day CI robot know about such
>> dependency between series?
>>
> 
> Currently, we have proposed to add a new '--base' option for git-format-patch
> to git community[1], developers could use this option to record the base tree
> information which could help 0day to identify the state the patch series
> applies to, thus 0day could avoid false report like this case.
> 
> For example, imagine that on top of the public commit P, you applied well-known
> patches X, Y and Z from somebody else or yourself, and then built your
> three-patch series A, B, C, the history would be like:
> 
> ................................................
> ---P---X---Y---Z---A---B---C
> ................................................
> 
> With `git format-patch --base=P -3 C` (or variants thereof, e.g. with
> `--cover-letter` of using `Z..C` instead of `-3 C` to specify the
> range), the base tree information block is shown at the end of the
> first message the command outputs (either the first patch, or the
> cover letter), like this:
> 
> ------------
> base-commit: P
> prerequisite-patch-id: X
> prerequisite-patch-id: Y
> prerequisite-patch-id: Z
> ------------
> 
> Now this feature is still under review in git community[2], hope it could be
> merged in next git release.

OK. That's good to know. Thank you very much for the info. I will follow
the progress then.

Best Regards

Eric
> 
> 
> [1] http://thread.gmane.org/gmane.comp.version-control.git/286873
> [2] http://thread.gmane.org/gmane.comp.version-control.git/292168
> 
> Thanks,
> Xiaolong
> 
>> If it is an inconvenience I can put all the patches back into the same
>> big series, specifying tentative patch split according to sub-systems in
>> the cover-letter?
>>
>> Thank you in advance
>>
>> Best Regards
>>
>> Eric
>>
>> 04/19/2016 08:04 PM, kbuild test robot wrote:
>>> Hi,
>>>
>>> [auto build test ERROR on tip/irq/core]
>>> [also build test ERROR on v4.6-rc4 next-20160419]
>>> [if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
>>>
>>> url:    https://github.com/0day-ci/linux/commits/Eric-Auger/KVM-PCIe-MSI-passthrough-on-ARM-ARM64-kernel-part-2-3-msi-changes/20160420-011957
>>> config: x86_64-lkp (attached as .config)
>>> reproduce:
>>>         # save the attached .config to linux build tree
>>>         make ARCH=x86_64 
>>>
>>> All errors (new ones prefixed by >>):
>>>
>>>>> kernel/irq/msi.c:17:38: fatal error: linux/dma-reserved-iommu.h: No such file or directory
>>>     #include <linux/dma-reserved-iommu.h>
>>>                                          ^
>>>    compilation terminated.
>>>
>>> vim +17 kernel/irq/msi.c
>>>
>>>     11	 */
>>>     12	#include <linux/types.h>
>>>     13	#include <linux/device.h>
>>>     14	#include <linux/irq.h>
>>>     15	#include <linux/irqdomain.h>
>>>     16	#include <linux/msi.h>
>>>   > 17	#include <linux/dma-reserved-iommu.h>
>>>     18	#include <linux/iommu.h>
>>>     19	
>>>     20	/* Temparory solution for building, will be removed later */
>>>
>>> ---
>>> 0-DAY kernel test infrastructure                Open Source Technology Center
>>> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
>>>
>>

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [lkp] [PATCH v7 7/8] genirq/msi: map/unmap the MSI doorbells on msi_domain_alloc/free_irqs
@ 2016-04-26 16:43           ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-26 16:43 UTC (permalink / raw)
  To: Ye Xiaolong
  Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	julien.grall-5wv7dgnIgG8, eric.auger-qxv4g6HH51o,
	kbuild test robot, patches-QSEj5FYQhm4dnm+yROfE0A,
	marc.zyngier-5wv7dgnIgG8, p.fedin-Sze3O3UU22JBDgjK7y7TUQ,
	will.deacon-5wv7dgnIgG8, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	pranav.sawargaonkar-Re5JQEeQqe8AvxtiuMwx3w,
	kbuild-all-JC7UmRfGjtg, tglx-hfZtesqFncYOwBW4kG4KsQ,
	christoffer.dall-QSEj5FYQhm4dnm+yROfE0A,
	jason-NLaQJdtUoK4Be96aLqz0jA

Hi Xiaolong
On 04/26/2016 03:24 AM, Ye Xiaolong wrote:
> On Wed, Apr 20, 2016 at 09:47:19AM +0200, Eric Auger wrote:
>> Hi,
>>
> Hi, Eric
>> Both reported errors related to this series are due to the fact part n
>> has dependency on part n-1.
>>
> 
> If I understand correctly, what you meant is that you send patch series
> (let's say B), and B has its dependency patch series A which is in-flight
> and hasn't been merged, so the commit history is like:
> 
>         ---P---A---B
> 
> where P is the well-known commit in public linux tree. In this case,
> 0day just applied B based on P without A, thus caused the build error.

yes that's it.
> 
>> Does anyone know how to let the 0-day CI robot know about such
>> dependency between series?
>>
> 
> Currently, we have proposed to add a new '--base' option for git-format-patch
> to git community[1], developers could use this option to record the base tree
> information which could help 0day to identify the state the patch series
> applies to, thus 0day could avoid false report like this case.
> 
> For example, imagine that on top of the public commit P, you applied well-known
> patches X, Y and Z from somebody else or yourself, and then built your
> three-patch series A, B, C, the history would be like:
> 
> ................................................
> ---P---X---Y---Z---A---B---C
> ................................................
> 
> With `git format-patch --base=P -3 C` (or variants thereof, e.g. with
> `--cover-letter` of using `Z..C` instead of `-3 C` to specify the
> range), the base tree information block is shown at the end of the
> first message the command outputs (either the first patch, or the
> cover letter), like this:
> 
> ------------
> base-commit: P
> prerequisite-patch-id: X
> prerequisite-patch-id: Y
> prerequisite-patch-id: Z
> ------------
> 
> Now this feature is still under review in git community[2], hope it could be
> merged in next git release.

OK. That's good to know. Thank you very much for the info. I will follow
the progress then.

Best Regards

Eric
> 
> 
> [1] http://thread.gmane.org/gmane.comp.version-control.git/286873
> [2] http://thread.gmane.org/gmane.comp.version-control.git/292168
> 
> Thanks,
> Xiaolong
> 
>> If it is an inconvenience I can put all the patches back into the same
>> big series, specifying tentative patch split according to sub-systems in
>> the cover-letter?
>>
>> Thank you in advance
>>
>> Best Regards
>>
>> Eric
>>
>> 04/19/2016 08:04 PM, kbuild test robot wrote:
>>> Hi,
>>>
>>> [auto build test ERROR on tip/irq/core]
>>> [also build test ERROR on v4.6-rc4 next-20160419]
>>> [if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
>>>
>>> url:    https://github.com/0day-ci/linux/commits/Eric-Auger/KVM-PCIe-MSI-passthrough-on-ARM-ARM64-kernel-part-2-3-msi-changes/20160420-011957
>>> config: x86_64-lkp (attached as .config)
>>> reproduce:
>>>         # save the attached .config to linux build tree
>>>         make ARCH=x86_64 
>>>
>>> All errors (new ones prefixed by >>):
>>>
>>>>> kernel/irq/msi.c:17:38: fatal error: linux/dma-reserved-iommu.h: No such file or directory
>>>     #include <linux/dma-reserved-iommu.h>
>>>                                          ^
>>>    compilation terminated.
>>>
>>> vim +17 kernel/irq/msi.c
>>>
>>>     11	 */
>>>     12	#include <linux/types.h>
>>>     13	#include <linux/device.h>
>>>     14	#include <linux/irq.h>
>>>     15	#include <linux/irqdomain.h>
>>>     16	#include <linux/msi.h>
>>>   > 17	#include <linux/dma-reserved-iommu.h>
>>>     18	#include <linux/iommu.h>
>>>     19	
>>>     20	/* Temparory solution for building, will be removed later */
>>>
>>> ---
>>> 0-DAY kernel test infrastructure                Open Source Technology Center
>>> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
>>>
>>

^ permalink raw reply	[flat|nested] 64+ messages in thread

* [lkp] [PATCH v7 7/8] genirq/msi: map/unmap the MSI doorbells on msi_domain_alloc/free_irqs
@ 2016-04-26 16:43           ` Eric Auger
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Auger @ 2016-04-26 16:43 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Xiaolong
On 04/26/2016 03:24 AM, Ye Xiaolong wrote:
> On Wed, Apr 20, 2016 at 09:47:19AM +0200, Eric Auger wrote:
>> Hi,
>>
> Hi, Eric
>> Both reported errors related to this series are due to the fact part n
>> has dependency on part n-1.
>>
> 
> If I understand correctly, what you meant is that you send patch series
> (let's say B), and B has its dependency patch series A which is in-flight
> and hasn't been merged, so the commit history is like:
> 
>         ---P---A---B
> 
> where P is the well-known commit in public linux tree. In this case,
> 0day just applied B based on P without A, thus caused the build error.

yes that's it.
> 
>> Does anyone know how to let the 0-day CI robot know about such
>> dependency between series?
>>
> 
> Currently, we have proposed to add a new '--base' option for git-format-patch
> to git community[1], developers could use this option to record the base tree
> information which could help 0day to identify the state the patch series
> applies to, thus 0day could avoid false report like this case.
> 
> For example, imagine that on top of the public commit P, you applied well-known
> patches X, Y and Z from somebody else or yourself, and then built your
> three-patch series A, B, C, the history would be like:
> 
> ................................................
> ---P---X---Y---Z---A---B---C
> ................................................
> 
> With `git format-patch --base=P -3 C` (or variants thereof, e.g. with
> `--cover-letter` of using `Z..C` instead of `-3 C` to specify the
> range), the base tree information block is shown at the end of the
> first message the command outputs (either the first patch, or the
> cover letter), like this:
> 
> ------------
> base-commit: P
> prerequisite-patch-id: X
> prerequisite-patch-id: Y
> prerequisite-patch-id: Z
> ------------
> 
> Now this feature is still under review in git community[2], hope it could be
> merged in next git release.

OK. That's good to know. Thank you very much for the info. I will follow
the progress then.

Best Regards

Eric
> 
> 
> [1] http://thread.gmane.org/gmane.comp.version-control.git/286873
> [2] http://thread.gmane.org/gmane.comp.version-control.git/292168
> 
> Thanks,
> Xiaolong
> 
>> If it is an inconvenience I can put all the patches back into the same
>> big series, specifying tentative patch split according to sub-systems in
>> the cover-letter?
>>
>> Thank you in advance
>>
>> Best Regards
>>
>> Eric
>>
>> 04/19/2016 08:04 PM, kbuild test robot wrote:
>>> Hi,
>>>
>>> [auto build test ERROR on tip/irq/core]
>>> [also build test ERROR on v4.6-rc4 next-20160419]
>>> [if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
>>>
>>> url:    https://github.com/0day-ci/linux/commits/Eric-Auger/KVM-PCIe-MSI-passthrough-on-ARM-ARM64-kernel-part-2-3-msi-changes/20160420-011957
>>> config: x86_64-lkp (attached as .config)
>>> reproduce:
>>>         # save the attached .config to linux build tree
>>>         make ARCH=x86_64 
>>>
>>> All errors (new ones prefixed by >>):
>>>
>>>>> kernel/irq/msi.c:17:38: fatal error: linux/dma-reserved-iommu.h: No such file or directory
>>>     #include <linux/dma-reserved-iommu.h>
>>>                                          ^
>>>    compilation terminated.
>>>
>>> vim +17 kernel/irq/msi.c
>>>
>>>     11	 */
>>>     12	#include <linux/types.h>
>>>     13	#include <linux/device.h>
>>>     14	#include <linux/irq.h>
>>>     15	#include <linux/irqdomain.h>
>>>     16	#include <linux/msi.h>
>>>   > 17	#include <linux/dma-reserved-iommu.h>
>>>     18	#include <linux/iommu.h>
>>>     19	
>>>     20	/* Temparory solution for building, will be removed later */
>>>
>>> ---
>>> 0-DAY kernel test infrastructure                Open Source Technology Center
>>> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
>>>
>>

^ permalink raw reply	[flat|nested] 64+ messages in thread

end of thread, other threads:[~2016-04-26 16:45 UTC | newest]

Thread overview: 64+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-19 17:13 [PATCH v7 0/8] KVM PCIe/MSI passthrough on ARM/ARM64: kernel part 2/3: msi changes Eric Auger
2016-04-19 17:13 ` Eric Auger
2016-04-19 17:13 ` Eric Auger
2016-04-19 17:13 ` [PATCH v7 1/8] genirq/msi: Add a new MSI_FLAG_IRQ_REMAPPING flag Eric Auger
2016-04-19 17:13   ` Eric Auger
2016-04-19 17:13   ` Eric Auger
2016-04-22 11:02   ` Robin Murphy
2016-04-22 11:02     ` Robin Murphy
2016-04-22 12:25     ` Eric Auger
2016-04-22 12:25       ` Eric Auger
2016-04-22 12:25       ` Eric Auger
2016-04-22 15:39       ` Thomas Gleixner
2016-04-22 15:39         ` Thomas Gleixner
2016-04-19 17:13 ` [PATCH v7 2/8] irqchip/gic-v3-its: ITS advertises MSI_FLAG_IRQ_REMAPPING Eric Auger
2016-04-19 17:13   ` Eric Auger
2016-04-19 17:13   ` Eric Auger
2016-04-19 17:13 ` [PATCH v7 3/8] genirq/msi: export msi_get_domain_info Eric Auger
2016-04-19 17:13   ` Eric Auger
2016-04-19 17:13   ` Eric Auger
2016-04-19 17:13 ` [PATCH v7 4/8] genirq/msi: msi_compose wrapper Eric Auger
2016-04-19 17:13   ` Eric Auger
2016-04-19 17:13   ` Eric Auger
2016-04-19 17:13 ` [PATCH v7 5/8] genirq/irq: introduce msi_doorbell's structs and related callback Eric Auger
2016-04-19 17:13   ` Eric Auger
2016-04-19 17:13   ` Eric Auger
2016-04-20  9:16   ` Marc Zyngier
2016-04-20  9:16     ` Marc Zyngier
2016-04-20  9:16     ` Marc Zyngier
2016-04-20  9:36     ` Eric Auger
2016-04-20  9:36       ` Eric Auger
2016-04-20  9:36       ` Eric Auger
2016-04-19 17:13 ` [PATCH v7 6/8] irqchip/gicv2m: implement msi_doorbell_info callback Eric Auger
2016-04-19 17:13   ` Eric Auger
2016-04-19 17:13   ` Eric Auger
2016-04-20  9:27   ` Marc Zyngier
2016-04-20  9:27     ` Marc Zyngier
2016-04-20  9:27     ` Marc Zyngier
2016-04-20 12:33     ` Eric Auger
2016-04-20 12:33       ` Eric Auger
2016-04-20 12:33       ` Eric Auger
2016-04-20 17:56       ` Marc Zyngier
2016-04-20 17:56         ` Marc Zyngier
2016-04-20 17:56         ` Marc Zyngier
2016-04-20 18:16         ` Eric Auger
2016-04-20 18:16           ` Eric Auger
2016-04-20 18:16           ` Eric Auger
2016-04-19 17:13 ` [PATCH v7 7/8] genirq/msi: map/unmap the MSI doorbells on msi_domain_alloc/free_irqs Eric Auger
2016-04-19 17:13   ` Eric Auger
2016-04-19 17:13   ` Eric Auger
2016-04-19 18:04   ` kbuild test robot
2016-04-19 18:04     ` kbuild test robot
2016-04-19 18:04     ` kbuild test robot
2016-04-20  7:47     ` Eric Auger
2016-04-20  7:47       ` Eric Auger
2016-04-20  7:47       ` Eric Auger
2016-04-26  1:24       ` [lkp] " Ye Xiaolong
2016-04-26  1:24         ` Ye Xiaolong
2016-04-26 16:43         ` Eric Auger
2016-04-26 16:43           ` Eric Auger
2016-04-26 16:43           ` Eric Auger
2016-04-19 17:13 ` [PATCH v7 8/8] genirq/msi: use the MSI doorbell's IOVA when requested Eric Auger
2016-04-19 17:13   ` Eric Auger
2016-04-20  9:44   ` Marc Zyngier
2016-04-20  9:44     ` Marc Zyngier

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.