[PATCH kernel v4 0/6] vfio-pci: Add support for mmapping MSI-X table

[PATCH kernel v4 0/6] vfio-pci: Add support for mmapping MSI-X table

[Alexey Kardashevskiy]

Here is a patchset which Yongji was working on before
leaving IBM LTC. Since we still want to have this functionality
in the kernel (DPDK is the first user), here is a rebase
on the current upstream.


Current vfio-pci implementation disallows to mmap the page
containing MSI-X table in case that users can write directly
to MSI-X table and generate an incorrect MSIs.

However, this will cause some performance issue when there
are some critical device registers in the same page as the
MSI-X table. We have to handle the mmio access to these
registers in QEMU emulation rather than in guest.

To solve this issue, this series allows to expose MSI-X table
to userspace when hardware enables the capability of interrupt
remapping which can ensure that a given PCI device can only
shoot the MSIs assigned for it. And we introduce a new bus_flags
PCI_BUS_FLAGS_MSI_REMAP to test this capability on PCI side
for different archs.


This is based on sha1
3c2bfbaadff6 Linus Torvalds "Merge branch 'fixes' of git://git.armlinux.org.uk/~rmk/linux-arm".

Please comment. Thanks.

Changes:
v4:
* rebased on recent upstream
* got all 6 patches from v2 (v3 was missing some)


Alexey Kardashevskiy (1):
  PCI: Set PCI_BUS_FLAGS_MSI_REMAP if MSI controller enables IRQ
    remapping

Yongji Xie (5):
  PCI: Add a new PCI_BUS_FLAGS_MSI_REMAP flag
  PCI: Set PCI_BUS_FLAGS_MSI_REMAP if IOMMU have capability of IRQ
    remapping
  iommu: Set PCI_BUS_FLAGS_MSI_REMAP on iommu driver initialization
  pci-ioda: Set PCI_BUS_FLAGS_MSI_REMAP for IODA host bridge
  vfio-pci: Allow to expose MSI-X table to userspace if interrupt
    remapping is enabled

 include/linux/pci.h                       |  1 +
 arch/powerpc/platforms/powernv/pci-ioda.c |  7 +++++++
 drivers/iommu/iommu.c                     |  8 ++++++++
 drivers/pci/probe.c                       |  8 ++++++++
 drivers/vfio/pci/vfio_pci.c               | 17 ++++++++++++++---
 drivers/vfio/pci/vfio_pci_rdwr.c          |  3 ++-
 6 files changed, 40 insertions(+), 4 deletions(-)

-- 
2.11.0

[PATCH kernel v4 1/6] PCI: Add a new PCI_BUS_FLAGS_MSI_REMAP flag

[Alexey Kardashevskiy]

From: Yongji Xie <elohimes@gmail.com>

We introduce a new pci_bus_flags, PCI_BUS_FLAGS_MSI_REMAP
which indicates interrupts of all devices on the bus are
managed by the hardware enabling IRQ remapping (intel naming).
When the capability is enabled, a given PCI device can only
shoot the MSIs assigned for it. In other words, the hardware
can protect system from invalid MSIs of the device by checking
the target address and data when there is something wrong
with MSI part in device or device driver.

The new flag will be set by when the kernel decides that it is safe to
do so. With this flag enabled, we can easily know whether it's safe
to expose MSI-X tables of PCI BARs to userspace. Some usespace
drivers such as VFIO may benefit from this.

Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 include/linux/pci.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8039f9f0ca05..2c6dbb3dd0da 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -200,6 +200,7 @@ enum pci_bus_flags {
 	PCI_BUS_FLAGS_NO_MSI	= (__force pci_bus_flags_t) 1,
 	PCI_BUS_FLAGS_NO_MMRBC	= (__force pci_bus_flags_t) 2,
 	PCI_BUS_FLAGS_NO_AERSID	= (__force pci_bus_flags_t) 4,
+	PCI_BUS_FLAGS_MSI_REMAP = (__force pci_bus_flags_t) 8,
 };
 
 /* These values come from the PCI Express Spec */
-- 
2.11.0

[PATCH kernel v4 2/6] PCI: Set PCI_BUS_FLAGS_MSI_REMAP if MSI controller enables IRQ remapping

[Alexey Kardashevskiy]

This sets PCI_BUS_FLAGS_MSI_REMAP to a bus if MSI remapping is an IRQ
domain is capable of MSI remapping.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v4:
* dropped pci_bus_msi_isolated() which looked for (never pushed out)
MSI_FLAG_IRQ_REMAPPING; used irq_domain_hierarchical_is_msi_remap() instead
---
 drivers/pci/probe.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 19c8950c6c38..3529ae17b70e 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -716,6 +716,10 @@ static void pci_set_bus_msi_domain(struct pci_bus *bus)
 	if (!d)
 		d = pci_host_bridge_msi_domain(b);
 
+	if (d && irq_domain_is_msi(d) &&
+			irq_domain_hierarchical_is_msi_remap(d))
+		bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP;
+
 	dev_set_msi_domain(&bus->dev, d);
 }
 
-- 
2.11.0

[PATCH kernel v4 3/6] PCI: Set PCI_BUS_FLAGS_MSI_REMAP if IOMMU have capability of IRQ remapping

[Alexey Kardashevskiy]

From: Yongji Xie <elohimes@gmail.com>

The capability of IRQ remapping is abstracted on IOMMU side on
some archs. There is a existing flag IOMMU_CAP_INTR_REMAP for this.

To have a universal flag to test this capability for different
archs on PCI side, we set PCI_BUS_FLAGS_MSI_REMAP for PCI buses
when IOMMU_CAP_INTR_REMAP is set.

Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 drivers/pci/probe.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 3529ae17b70e..f2393b7d7ebf 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -775,6 +775,9 @@ int pci_register_host_bridge(struct pci_host_bridge *bridge)
 	pci_set_bus_of_node(bus);
 	pci_set_bus_msi_domain(bus);
 
+	if (iommu_capable(&pci_bus_type, IOMMU_CAP_INTR_REMAP))
+		b->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP;
+
 	if (!parent)
 		set_dev_node(bus->bridge, pcibus_to_node(bus));
 
-- 
2.11.0

[PATCH kernel v4 4/6] iommu: Set PCI_BUS_FLAGS_MSI_REMAP on iommu driver initialization

[Alexey Kardashevskiy]

From: Yongji Xie <elohimes@gmail.com>

Some iommu drivers would be initialized after PCI device
enumeration. So PCI_BUS_FLAGS_MSI_REMAP would not be set
when probing PCI devices although IOMMU enables capability
of IRQ remapping. This patch tests this capability and
set the flag when iommu driver is initialized.

Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 drivers/iommu/iommu.c | 8 ++++++++
 drivers/pci/probe.c   | 1 +
 2 files changed, 9 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index cf7ca7e70777..0b5881ddca09 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1063,6 +1063,14 @@ static int add_iommu_group(struct device *dev, void *data)
 	const struct iommu_ops *ops = cb->ops;
 	int ret;
 
+	/*
+	 * Set PCI_BUS_FLAGS_MSI_REMAP for all PCI buses when IOMMU
+	 * have capability of IRQ remapping.
+	 */
+	if (dev_is_pci(dev) && ops->capable &&
+			ops->capable(IOMMU_CAP_INTR_REMAP))
+		to_pci_dev(dev)->bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP;
+
 	if (!ops->add_device)
 		return 0;
 
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index f2393b7d7ebf..14aac9df3d17 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -17,6 +17,7 @@
 #include <linux/acpi.h>
 #include <linux/irqdomain.h>
 #include <linux/pm_runtime.h>
+#include <linux/iommu.h>
 #include "pci.h"
 
 #define CARDBUS_LATENCY_TIMER	176	/* secondary latency timer */
-- 
2.11.0

[PATCH kernel v4 5/6] pci-ioda: Set PCI_BUS_FLAGS_MSI_REMAP for IODA host bridge

[Alexey Kardashevskiy]

From: Yongji Xie <elohimes@gmail.com>

Any IODA host bridge have the capability of IRQ remapping.
So we set PCI_BUS_FLAGS_MSI_REMAP when this kind of host birdge
is detected.

Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 arch/powerpc/platforms/powernv/pci-ioda.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index 283caf1070c9..8456e07cfc68 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -3177,6 +3177,12 @@ static void pnv_pci_ioda_fixup(void)
 #endif
 }
 
+int pnv_pci_ioda_root_bridge_prepare(struct pci_host_bridge *bridge)
+{
+	bridge->bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP;
+	return 0;
+}
+
 /*
  * Returns the alignment for I/O or memory windows for P2P
  * bridges. That actually depends on how PEs are segmented.
@@ -3860,6 +3866,7 @@ static void __init pnv_pci_init_ioda_phb(struct device_node *np,
 	 * the child P2P bridges) can form individual PE.
 	 */
 	ppc_md.pcibios_fixup = pnv_pci_ioda_fixup;
+	ppc_md.pcibios_root_bridge_prepare = pnv_pci_ioda_root_bridge_prepare;
 
 	if (phb->type == PNV_PHB_NPU) {
 		hose->controller_ops = pnv_npu_ioda_controller_ops;
-- 
2.11.0

[PATCH kernel v4 6/6] vfio-pci: Allow to expose MSI-X table to userspace if interrupt remapping is enabled

[Alexey Kardashevskiy]

From: Yongji Xie <elohimes@gmail.com>

This patch tries to expose MSI-X tables to userspace if hardware
enables interrupt remapping. This capability can ensure that a
given PCI device can only shoot the MSIs assigned for it. That
means a userspace driver could never hurt other devices or system
by writing to the exposed MSI-X table directly.

Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 drivers/vfio/pci/vfio_pci.c      | 17 ++++++++++++++---
 drivers/vfio/pci/vfio_pci_rdwr.c |  3 ++-
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 324c52e3a1a4..0c616048ad80 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -564,8 +564,12 @@ static int msix_sparse_mmap_cap(struct vfio_pci_device *vdev,
 
 	end = pci_resource_len(vdev->pdev, vdev->msix_bar);
 
-	/* If MSI-X table is aligned to the start or end, only one area */
-	if (((vdev->msix_offset & PAGE_MASK) == 0) ||
+	/*
+	 * If MSI-X table is allowed to mmap because of the capability
+	 * of IRQ remapping or aligned to the start or end, only one area
+	 */
+	if ((vdev->pdev->bus->bus_flags & PCI_BUS_FLAGS_MSI_REMAP) ||
+	    ((vdev->msix_offset & PAGE_MASK) == 0) ||
 	    (PAGE_ALIGN(vdev->msix_offset + vdev->msix_size) >= end))
 		nr_areas = 1;
 
@@ -577,6 +581,12 @@ static int msix_sparse_mmap_cap(struct vfio_pci_device *vdev,
 
 	sparse->nr_areas = nr_areas;
 
+	if (vdev->pdev->bus->bus_flags & PCI_BUS_FLAGS_MSI_REMAP) {
+		sparse->areas[i].offset = 0;
+		sparse->areas[i].size = end;
+		return 0;
+	}
+
 	if (vdev->msix_offset & PAGE_MASK) {
 		sparse->areas[i].offset = 0;
 		sparse->areas[i].size = vdev->msix_offset & PAGE_MASK;
@@ -1115,7 +1125,8 @@ static int vfio_pci_mmap(void *device_data, struct vm_area_struct *vma)
 	if (req_start + req_len > phys_len)
 		return -EINVAL;
 
-	if (index == vdev->msix_bar) {
+	if (index == vdev->msix_bar &&
+		!(pdev->bus->bus_flags & PCI_BUS_FLAGS_MSI_REMAP)) {
 		/*
 		 * Disallow mmaps overlapping the MSI-X table; users don't
 		 * get to touch this directly.  We could find somewhere
diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
index 357243d76f10..5378f2c3ac8e 100644
--- a/drivers/vfio/pci/vfio_pci_rdwr.c
+++ b/drivers/vfio/pci/vfio_pci_rdwr.c
@@ -164,7 +164,8 @@ ssize_t vfio_pci_bar_rw(struct vfio_pci_device *vdev, char __user *buf,
 	} else
 		io = vdev->barmap[bar];
 
-	if (bar == vdev->msix_bar) {
+	if (bar == vdev->msix_bar &&
+		!(pdev->bus->bus_flags & PCI_BUS_FLAGS_MSI_REMAP)) {
 		x_start = vdev->msix_offset;
 		x_end = vdev->msix_offset + vdev->msix_size;
 	}
-- 
2.11.0

Re: [PATCH kernel v4 3/6] PCI: Set PCI_BUS_FLAGS_MSI_REMAP if IOMMU have capability of IRQ remapping

[kbuild test robot]

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

Hi Yongji,

[auto build test ERROR on pci/next]
[also build test ERROR on v4.12-rc7 next-20170630]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Alexey-Kardashevskiy/PCI-Add-a-new-PCI_BUS_FLAGS_MSI_REMAP-flag/20170701-171815
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: x86_64-randconfig-x010-201726 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

Note: the linux-review/Alexey-Kardashevskiy/PCI-Add-a-new-PCI_BUS_FLAGS_MSI_REMAP-flag/20170701-171815 HEAD 315f74455dce23dfb8bb1f5195c8c94ed62743a4 builds fine.
      It only hurts bisectibility.

All error/warnings (new ones prefixed by >>):

   In file included from include/linux/linkage.h:4:0,
                    from include/linux/kernel.h:6,
                    from drivers/pci/probe.c:5:
   drivers/pci/probe.c: In function 'pci_register_host_bridge':
>> drivers/pci/probe.c:805:6: error: implicit declaration of function 'iommu_capable' [-Werror=implicit-function-declaration]
     if (iommu_capable(&pci_bus_type, IOMMU_CAP_INTR_REMAP))
         ^
   include/linux/compiler.h:160:30: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                 ^~~~
>> drivers/pci/probe.c:805:2: note: in expansion of macro 'if'
     if (iommu_capable(&pci_bus_type, IOMMU_CAP_INTR_REMAP))
     ^~
>> drivers/pci/probe.c:805:35: error: 'IOMMU_CAP_INTR_REMAP' undeclared (first use in this function)
     if (iommu_capable(&pci_bus_type, IOMMU_CAP_INTR_REMAP))
                                      ^
   include/linux/compiler.h:160:30: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                 ^~~~
>> drivers/pci/probe.c:805:2: note: in expansion of macro 'if'
     if (iommu_capable(&pci_bus_type, IOMMU_CAP_INTR_REMAP))
     ^~
   drivers/pci/probe.c:805:35: note: each undeclared identifier is reported only once for each function it appears in
     if (iommu_capable(&pci_bus_type, IOMMU_CAP_INTR_REMAP))
                                      ^
   include/linux/compiler.h:160:30: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                 ^~~~
>> drivers/pci/probe.c:805:2: note: in expansion of macro 'if'
     if (iommu_capable(&pci_bus_type, IOMMU_CAP_INTR_REMAP))
     ^~
   cc1: some warnings being treated as errors

vim +/iommu_capable +805 drivers/pci/probe.c

   799	
   800		bus->bridge = get_device(&bridge->dev);
   801		device_enable_async_suspend(bus->bridge);
   802		pci_set_bus_of_node(bus);
   803		pci_set_bus_msi_domain(bus);
   804	
 > 805		if (iommu_capable(&pci_bus_type, IOMMU_CAP_INTR_REMAP))
   806			b->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP;
   807	
   808		if (!parent)

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31398 bytes --]

Re: [PATCH kernel v4 0/6] vfio-pci: Add support for mmapping MSI-X table

[Alexey Kardashevskiy]

On 30/06/17 15:24, Alexey Kardashevskiy wrote:
> Here is a patchset which Yongji was working on before
> leaving IBM LTC. Since we still want to have this functionality
> in the kernel (DPDK is the first user), here is a rebase
> on the current upstream.
> 
> 
> Current vfio-pci implementation disallows to mmap the page
> containing MSI-X table in case that users can write directly
> to MSI-X table and generate an incorrect MSIs.
> 
> However, this will cause some performance issue when there
> are some critical device registers in the same page as the
> MSI-X table. We have to handle the mmio access to these
> registers in QEMU emulation rather than in guest.
> 
> To solve this issue, this series allows to expose MSI-X table
> to userspace when hardware enables the capability of interrupt
> remapping which can ensure that a given PCI device can only
> shoot the MSIs assigned for it. And we introduce a new bus_flags
> PCI_BUS_FLAGS_MSI_REMAP to test this capability on PCI side
> for different archs.
> 
> 
> This is based on sha1
> 3c2bfbaadff6 Linus Torvalds "Merge branch 'fixes' of git://git.armlinux.org.uk/~rmk/linux-arm".
> 
> Please comment. Thanks.


Other than that compile error, is there anything else wrong/missing in this
patchset? Thanks.



> 
> Changes:
> v4:
> * rebased on recent upstream
> * got all 6 patches from v2 (v3 was missing some)
> 
> 
> Alexey Kardashevskiy (1):
>   PCI: Set PCI_BUS_FLAGS_MSI_REMAP if MSI controller enables IRQ
>     remapping
> 
> Yongji Xie (5):
>   PCI: Add a new PCI_BUS_FLAGS_MSI_REMAP flag
>   PCI: Set PCI_BUS_FLAGS_MSI_REMAP if IOMMU have capability of IRQ
>     remapping
>   iommu: Set PCI_BUS_FLAGS_MSI_REMAP on iommu driver initialization
>   pci-ioda: Set PCI_BUS_FLAGS_MSI_REMAP for IODA host bridge
>   vfio-pci: Allow to expose MSI-X table to userspace if interrupt
>     remapping is enabled
> 
>  include/linux/pci.h                       |  1 +
>  arch/powerpc/platforms/powernv/pci-ioda.c |  7 +++++++
>  drivers/iommu/iommu.c                     |  8 ++++++++
>  drivers/pci/probe.c                       |  8 ++++++++
>  drivers/vfio/pci/vfio_pci.c               | 17 ++++++++++++++---
>  drivers/vfio/pci/vfio_pci_rdwr.c          |  3 ++-
>  6 files changed, 40 insertions(+), 4 deletions(-)
> 


-- 
Alexey

Re: [PATCH kernel v4 0/6] vfio-pci: Add support for mmapping MSI-X table

[Bjorn Helgaas]

[+cc linux-pci]

On Fri, Jun 30, 2017 at 12:24 AM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>
> Here is a patchset which Yongji was working on before
> leaving IBM LTC. Since we still want to have this functionality
> in the kernel (DPDK is the first user), here is a rebase
> on the current upstream.
>
>
> Current vfio-pci implementation disallows to mmap the page
> containing MSI-X table in case that users can write directly
> to MSI-X table and generate an incorrect MSIs.
>
> However, this will cause some performance issue when there
> are some critical device registers in the same page as the
> MSI-X table. We have to handle the mmio access to these
> registers in QEMU emulation rather than in guest.
>
> To solve this issue, this series allows to expose MSI-X table
> to userspace when hardware enables the capability of interrupt
> remapping which can ensure that a given PCI device can only
> shoot the MSIs assigned for it. And we introduce a new bus_flags
> PCI_BUS_FLAGS_MSI_REMAP to test this capability on PCI side
> for different archs.
>
>
> This is based on sha1
> 3c2bfbaadff6 Linus Torvalds "Merge branch 'fixes' of git://git.armlinux.org.uk/~rmk/linux-arm".
>
> Please comment. Thanks.
>
> Changes:
> v4:
> * rebased on recent upstream
> * got all 6 patches from v2 (v3 was missing some)
>
>
> Alexey Kardashevskiy (1):
>   PCI: Set PCI_BUS_FLAGS_MSI_REMAP if MSI controller enables IRQ
>     remapping
>
> Yongji Xie (5):
>   PCI: Add a new PCI_BUS_FLAGS_MSI_REMAP flag
>   PCI: Set PCI_BUS_FLAGS_MSI_REMAP if IOMMU have capability of IRQ
>     remapping
>   iommu: Set PCI_BUS_FLAGS_MSI_REMAP on iommu driver initialization
>   pci-ioda: Set PCI_BUS_FLAGS_MSI_REMAP for IODA host bridge
>   vfio-pci: Allow to expose MSI-X table to userspace if interrupt
>     remapping is enabled
>
>  include/linux/pci.h                       |  1 +
>  arch/powerpc/platforms/powernv/pci-ioda.c |  7 +++++++
>  drivers/iommu/iommu.c                     |  8 ++++++++
>  drivers/pci/probe.c                       |  8 ++++++++
>  drivers/vfio/pci/vfio_pci.c               | 17 ++++++++++++++---
>  drivers/vfio/pci/vfio_pci_rdwr.c          |  3 ++-
>  6 files changed, 40 insertions(+), 4 deletions(-)

Since this modifies drivers/pci/probe.c, it should be posted to
linux-pci@vger.kernel.org.

Re: [PATCH kernel v4 1/6] PCI: Add a new PCI_BUS_FLAGS_MSI_REMAP flag

[Bjorn Helgaas]

[+cc linux-pci]

On Fri, Jun 30, 2017 at 12:24 AM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> From: Yongji Xie <elohimes@gmail.com>
>
> We introduce a new pci_bus_flags, PCI_BUS_FLAGS_MSI_REMAP
> which indicates interrupts of all devices on the bus are
> managed by the hardware enabling IRQ remapping (intel naming).
> When the capability is enabled, a given PCI device can only
> shoot the MSIs assigned for it. In other words, the hardware
> can protect system from invalid MSIs of the device by checking
> the target address and data when there is something wrong
> with MSI part in device or device driver.

This needs a reference to the spec that describes how this MSI protection works.

> The new flag will be set by when the kernel decides that it is safe to
> do so.

We also need some clue about how the kernel makes this decision.  That
clue probably doesn't belong in *this* patch, but I didn't see it
anywhere.

> With this flag enabled, we can easily know whether it's safe
> to expose MSI-X tables of PCI BARs to userspace. Some usespace
> drivers such as VFIO may benefit from this.
>
> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  include/linux/pci.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 8039f9f0ca05..2c6dbb3dd0da 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -200,6 +200,7 @@ enum pci_bus_flags {
>         PCI_BUS_FLAGS_NO_MSI    = (__force pci_bus_flags_t) 1,
>         PCI_BUS_FLAGS_NO_MMRBC  = (__force pci_bus_flags_t) 2,
>         PCI_BUS_FLAGS_NO_AERSID = (__force pci_bus_flags_t) 4,
> +       PCI_BUS_FLAGS_MSI_REMAP = (__force pci_bus_flags_t) 8,
>  };
>
>  /* These values come from the PCI Express Spec */
> --
> 2.11.0
>

Re: [PATCH kernel v4 4/6] iommu: Set PCI_BUS_FLAGS_MSI_REMAP on iommu driver initialization

[Bjorn Helgaas via iommu]

[+cc Joerg, iommu]

On Fri, Jun 30, 2017 at 12:24 AM, Alexey Kardashevskiy <aik-sLpHqDYs0B2HXe+LvDLADg@public.gmane.org> wrote:
> From: Yongji Xie <elohimes-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> Some iommu drivers would be initialized after PCI device
> enumeration. So PCI_BUS_FLAGS_MSI_REMAP would not be set
> when probing PCI devices although IOMMU enables capability
> of IRQ remapping. This patch tests this capability and
> set the flag when iommu driver is initialized.
>
> Signed-off-by: Yongji Xie <xyjxie-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> Signed-off-by: Alexey Kardashevskiy <aik-sLpHqDYs0B2HXe+LvDLADg@public.gmane.org>
> ---
>  drivers/iommu/iommu.c | 8 ++++++++
>  drivers/pci/probe.c   | 1 +
>  2 files changed, 9 insertions(+)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index cf7ca7e70777..0b5881ddca09 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1063,6 +1063,14 @@ static int add_iommu_group(struct device *dev, void *data)
>         const struct iommu_ops *ops = cb->ops;
>         int ret;
>
> +       /*
> +        * Set PCI_BUS_FLAGS_MSI_REMAP for all PCI buses when IOMMU
> +        * have capability of IRQ remapping.
> +        */
> +       if (dev_is_pci(dev) && ops->capable &&
> +                       ops->capable(IOMMU_CAP_INTR_REMAP))
> +               to_pci_dev(dev)->bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP;

This isn't my area, but this addition is really ugly.  It doesn't look
anything like the rest of the code in add_iommu_group(), so it just
feels like a wart.

>         if (!ops->add_device)
>                 return 0;
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index f2393b7d7ebf..14aac9df3d17 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -17,6 +17,7 @@
>  #include <linux/acpi.h>
>  #include <linux/irqdomain.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/iommu.h>

This obviously belongs in another patch, as the compile error showed.

>  #include "pci.h"
>
>  #define CARDBUS_LATENCY_TIMER  176     /* secondary latency timer */
> --
> 2.11.0
>

Re: [PATCH kernel v4 4/6] iommu: Set PCI_BUS_FLAGS_MSI_REMAP on iommu driver initialization

[Bjorn Helgaas via iommu]

[+cc Joerg, iommu]

On Fri, Jun 30, 2017 at 12:24 AM, Alexey Kardashevskiy <aik-sLpHqDYs0B2HXe+LvDLADg@public.gmane.org> wrote:
> From: Yongji Xie <elohimes-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> Some iommu drivers would be initialized after PCI device
> enumeration. So PCI_BUS_FLAGS_MSI_REMAP would not be set
> when probing PCI devices although IOMMU enables capability
> of IRQ remapping. This patch tests this capability and
> set the flag when iommu driver is initialized.
>
> Signed-off-by: Yongji Xie <xyjxie-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> Signed-off-by: Alexey Kardashevskiy <aik-sLpHqDYs0B2HXe+LvDLADg@public.gmane.org>
> ---
>  drivers/iommu/iommu.c | 8 ++++++++
>  drivers/pci/probe.c   | 1 +
>  2 files changed, 9 insertions(+)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index cf7ca7e70777..0b5881ddca09 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1063,6 +1063,14 @@ static int add_iommu_group(struct device *dev, void *data)
>         const struct iommu_ops *ops = cb->ops;
>         int ret;
>
> +       /*
> +        * Set PCI_BUS_FLAGS_MSI_REMAP for all PCI buses when IOMMU
> +        * have capability of IRQ remapping.
> +        */
> +       if (dev_is_pci(dev) && ops->capable &&
> +                       ops->capable(IOMMU_CAP_INTR_REMAP))
> +               to_pci_dev(dev)->bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP;

This isn't my area, but this addition is really ugly.  It doesn't look
anything like the rest of the code in add_iommu_group(), so it just
feels like a wart.

>         if (!ops->add_device)
>                 return 0;
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index f2393b7d7ebf..14aac9df3d17 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -17,6 +17,7 @@
>  #include <linux/acpi.h>
>  #include <linux/irqdomain.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/iommu.h>

This obviously belongs in another patch, as the compile error showed.

>  #include "pci.h"
>
>  #define CARDBUS_LATENCY_TIMER  176     /* secondary latency timer */
> --
> 2.11.0
>

Re: [PATCH kernel v4 1/6] PCI: Add a new PCI_BUS_FLAGS_MSI_REMAP flag

[Alexey Kardashevskiy]

On 11/07/17 05:20, Bjorn Helgaas wrote:
> [+cc linux-pci]
> 
> On Fri, Jun 30, 2017 at 12:24 AM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>> From: Yongji Xie <elohimes@gmail.com>
>>
>> We introduce a new pci_bus_flags, PCI_BUS_FLAGS_MSI_REMAP
>> which indicates interrupts of all devices on the bus are
>> managed by the hardware enabling IRQ remapping (intel naming).
>> When the capability is enabled, a given PCI device can only
>> shoot the MSIs assigned for it. In other words, the hardware
>> can protect system from invalid MSIs of the device by checking
>> the target address and data when there is something wrong
>> with MSI part in device or device driver.
> 
> This needs a reference to the spec that describes how this MSI protection works.


How much detail is needed here? For example, what to add to the below?

===
On Intel VT-d [1], there is an Interrupt Remapping Table, one entry per
interrupt, has a source-id (i.e. BDFN) of allowed device.

On AMD IOMMU [2], there is a Device Table, each entry is indexed by
DevideID which is BDFN.

On IBM POWERPC (POWER8) [3], PCI host bridge maintains BFDN-to-PE
translation (PE stands for "partitionable endpoint"), and PE index is used
to look at Interrupt Vector Table (IVT) to identify the interrupt server.
Actually this is not remapping, rather filtering but it still works for the
purpose of this flag.

[1] 9.10 Interrupt Remapping Table Entry (IRTE) for Remapped Interrupts
https://www.intel.com/content/dam/www/public/us/en/documents/product-specifications/vt-directed-io-spec.pdf

[2] "2.2 Data Structures" and "2.2.5 Interrupt Remapping Tables"
https://support.amd.com/TechDocs/48882_IOMMU.pdf

[3] 3.2.4. MSI Design
http://openpowerfoundation.org/wp-content/uploads/resources/IODA2Spec/IODA2WGSpec-1.0.0-20160217.pdf
===

And I did not comment about ARM, should I?

If anyone could check the correctness of the above, that would be great
(thanks in advance :) )


>> The new flag will be set by when the kernel decides that it is safe to
>> do so.
> 
> We also need some clue about how the kernel makes this decision.  That
> clue probably doesn't belong in *this* patch, but I didn't see it
> anywhere.

There are 3 sources:

1. IOMMU driver advertises MSI remapping capability, done by [3/6] and
[4/6], they could make a single patch but I believe they are separated as
they are touching different maintainership areas;

2. MSI controller advertises MSI remapping capability, this is from ARM and
their MSI domains, I am not really familiar with this.

3. Platform code can just know that it is capable of filtering, like PPC64
powernv for IODA2/IODA3 PHB types.

Could be more, probably.


And thanks for the review.

> 
>> With this flag enabled, we can easily know whether it's safe
>> to expose MSI-X tables of PCI BARs to userspace. Some usespace
>> drivers such as VFIO may benefit from this.
>>
>> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>  include/linux/pci.h | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 8039f9f0ca05..2c6dbb3dd0da 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -200,6 +200,7 @@ enum pci_bus_flags {
>>         PCI_BUS_FLAGS_NO_MSI    = (__force pci_bus_flags_t) 1,
>>         PCI_BUS_FLAGS_NO_MMRBC  = (__force pci_bus_flags_t) 2,
>>         PCI_BUS_FLAGS_NO_AERSID = (__force pci_bus_flags_t) 4,
>> +       PCI_BUS_FLAGS_MSI_REMAP = (__force pci_bus_flags_t) 8,
>>  };
>>
>>  /* These values come from the PCI Express Spec */
>> --
>> 2.11.0
>>


-- 
Alexey

Re: [PATCH kernel v4 4/6] iommu: Set PCI_BUS_FLAGS_MSI_REMAP on iommu driver initialization

[Robin Murphy]

On 10/07/17 20:23, Bjorn Helgaas via iommu wrote:
> [+cc Joerg, iommu]
> 
> On Fri, Jun 30, 2017 at 12:24 AM, Alexey Kardashevskiy <aik-sLpHqDYs0B2HXe+LvDLADg@public.gmane.org> wrote:
>> From: Yongji Xie <elohimes-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
>> Some iommu drivers would be initialized after PCI device
>> enumeration. So PCI_BUS_FLAGS_MSI_REMAP would not be set
>> when probing PCI devices although IOMMU enables capability
>> of IRQ remapping. This patch tests this capability and
>> set the flag when iommu driver is initialized.
>>
>> Signed-off-by: Yongji Xie <xyjxie-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>> Signed-off-by: Alexey Kardashevskiy <aik-sLpHqDYs0B2HXe+LvDLADg@public.gmane.org>
>> ---
>>  drivers/iommu/iommu.c | 8 ++++++++
>>  drivers/pci/probe.c   | 1 +
>>  2 files changed, 9 insertions(+)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index cf7ca7e70777..0b5881ddca09 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -1063,6 +1063,14 @@ static int add_iommu_group(struct device *dev, void *data)
>>         const struct iommu_ops *ops = cb->ops;
>>         int ret;
>>
>> +       /*
>> +        * Set PCI_BUS_FLAGS_MSI_REMAP for all PCI buses when IOMMU
>> +        * have capability of IRQ remapping.
>> +        */
>> +       if (dev_is_pci(dev) && ops->capable &&
>> +                       ops->capable(IOMMU_CAP_INTR_REMAP))
>> +               to_pci_dev(dev)->bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP;
> 
> This isn't my area, but this addition is really ugly.  It doesn't look
> anything like the rest of the code in add_iommu_group(), so it just
> feels like a wart.

I have no idea what the context is here, but this flag looks wrong
generally. IRQ remapping is a property of the irqchip and has nothing to
do with PCI, so pretending it's a property of PCI buses looks like a
massive hack around... something.

Also, iommu_capable() is a fundamentally broken and unworkable interface
anyway. The existing IRQ remapping code really wants updating to
advertise IRQ_DOMAIN_FLAG_MSI_REMAP on the relevant MSI domains so that
IOMMU_CAP_INTR_REMAP can go away for good. That way, all consumers who
actually care about whether IRQ remapping is implemented (see e.g. VFIO)
can use irq_domain_check_msi_remap() or check specific devices in a sane
and scalable manner.

Robin.

>>         if (!ops->add_device)
>>                 return 0;
>>
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index f2393b7d7ebf..14aac9df3d17 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -17,6 +17,7 @@
>>  #include <linux/acpi.h>
>>  #include <linux/irqdomain.h>
>>  #include <linux/pm_runtime.h>
>> +#include <linux/iommu.h>
> 
> This obviously belongs in another patch, as the compile error showed.
> 
>>  #include "pci.h"
>>
>>  #define CARDBUS_LATENCY_TIMER  176     /* secondary latency timer */
>> --
>> 2.11.0
>>
> _______________________________________________
> iommu mailing list
> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
> 

Re: [PATCH kernel v4 4/6] iommu: Set PCI_BUS_FLAGS_MSI_REMAP on iommu driver initialization

[Robin Murphy]

On 10/07/17 20:23, Bjorn Helgaas via iommu wrote:
> [+cc Joerg, iommu]
> 
> On Fri, Jun 30, 2017 at 12:24 AM, Alexey Kardashevskiy <aik-sLpHqDYs0B2HXe+LvDLADg@public.gmane.org> wrote:
>> From: Yongji Xie <elohimes-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
>> Some iommu drivers would be initialized after PCI device
>> enumeration. So PCI_BUS_FLAGS_MSI_REMAP would not be set
>> when probing PCI devices although IOMMU enables capability
>> of IRQ remapping. This patch tests this capability and
>> set the flag when iommu driver is initialized.
>>
>> Signed-off-by: Yongji Xie <xyjxie-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>> Signed-off-by: Alexey Kardashevskiy <aik-sLpHqDYs0B2HXe+LvDLADg@public.gmane.org>
>> ---
>>  drivers/iommu/iommu.c | 8 ++++++++
>>  drivers/pci/probe.c   | 1 +
>>  2 files changed, 9 insertions(+)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index cf7ca7e70777..0b5881ddca09 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -1063,6 +1063,14 @@ static int add_iommu_group(struct device *dev, void *data)
>>         const struct iommu_ops *ops = cb->ops;
>>         int ret;
>>
>> +       /*
>> +        * Set PCI_BUS_FLAGS_MSI_REMAP for all PCI buses when IOMMU
>> +        * have capability of IRQ remapping.
>> +        */
>> +       if (dev_is_pci(dev) && ops->capable &&
>> +                       ops->capable(IOMMU_CAP_INTR_REMAP))
>> +               to_pci_dev(dev)->bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP;
> 
> This isn't my area, but this addition is really ugly.  It doesn't look
> anything like the rest of the code in add_iommu_group(), so it just
> feels like a wart.

I have no idea what the context is here, but this flag looks wrong
generally. IRQ remapping is a property of the irqchip and has nothing to
do with PCI, so pretending it's a property of PCI buses looks like a
massive hack around... something.

Also, iommu_capable() is a fundamentally broken and unworkable interface
anyway. The existing IRQ remapping code really wants updating to
advertise IRQ_DOMAIN_FLAG_MSI_REMAP on the relevant MSI domains so that
IOMMU_CAP_INTR_REMAP can go away for good. That way, all consumers who
actually care about whether IRQ remapping is implemented (see e.g. VFIO)
can use irq_domain_check_msi_remap() or check specific devices in a sane
and scalable manner.

Robin.

>>         if (!ops->add_device)
>>                 return 0;
>>
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index f2393b7d7ebf..14aac9df3d17 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -17,6 +17,7 @@
>>  #include <linux/acpi.h>
>>  #include <linux/irqdomain.h>
>>  #include <linux/pm_runtime.h>
>> +#include <linux/iommu.h>
> 
> This obviously belongs in another patch, as the compile error showed.
> 
>>  #include "pci.h"
>>
>>  #define CARDBUS_LATENCY_TIMER  176     /* secondary latency timer */
>> --
>> 2.11.0
>>
> _______________________________________________
> iommu mailing list
> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
> 

Re: [PATCH kernel v4 4/6] iommu: Set PCI_BUS_FLAGS_MSI_REMAP on iommu driver initialization

[Alexey Kardashevskiy]

On 11/07/17 20:39, Robin Murphy wrote:
> On 10/07/17 20:23, Bjorn Helgaas via iommu wrote:
>> [+cc Joerg, iommu]
>>
>> On Fri, Jun 30, 2017 at 12:24 AM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>> From: Yongji Xie <elohimes@gmail.com>
>>>
>>> Some iommu drivers would be initialized after PCI device
>>> enumeration. So PCI_BUS_FLAGS_MSI_REMAP would not be set
>>> when probing PCI devices although IOMMU enables capability
>>> of IRQ remapping. This patch tests this capability and
>>> set the flag when iommu driver is initialized.
>>>
>>> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>>  drivers/iommu/iommu.c | 8 ++++++++
>>>  drivers/pci/probe.c   | 1 +
>>>  2 files changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>> index cf7ca7e70777..0b5881ddca09 100644
>>> --- a/drivers/iommu/iommu.c
>>> +++ b/drivers/iommu/iommu.c
>>> @@ -1063,6 +1063,14 @@ static int add_iommu_group(struct device *dev, void *data)
>>>         const struct iommu_ops *ops = cb->ops;
>>>         int ret;
>>>
>>> +       /*
>>> +        * Set PCI_BUS_FLAGS_MSI_REMAP for all PCI buses when IOMMU
>>> +        * have capability of IRQ remapping.
>>> +        */
>>> +       if (dev_is_pci(dev) && ops->capable &&
>>> +                       ops->capable(IOMMU_CAP_INTR_REMAP))
>>> +               to_pci_dev(dev)->bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP;
>>
>> This isn't my area, but this addition is really ugly.  It doesn't look
>> anything like the rest of the code in add_iommu_group(), so it just
>> feels like a wart.
> 
> I have no idea what the context is here, but this flag looks wrong
> generally. IRQ remapping is a property of the irqchip and has nothing to
> do with PCI, so pretending it's a property of PCI buses looks like a
> massive hack around... something.

The context here - should we allow mapping of MSIX BAR to a guest or not.
Now it is disabled as the guest can write there anything and make device
send MSIX messages to random addresses. However if a PCI bridge can somehow
filter these writes, then it is safe.

So it is not really remapping what we care about in this patchset, it is
filtering/censoring.

And the reason to allow MSIX mapping is that it may be adjacent to
frequently used MMIO and also reside within same 64K page which happens to
be the default page size on PPC64.

So even though remapping/filtering is not a property of any PCI bus but it
is a part on an IOMMU which is usually (always?) combined with the PCI host
bridge adapter so the PCI bus flag makes sense. How else would we pass that
flag from IOMMU to the actual device we want to mmap to the userspace?


> Also, iommu_capable() is a fundamentally broken and unworkable interface
> anyway. The existing IRQ remapping code really wants updating to
> advertise IRQ_DOMAIN_FLAG_MSI_REMAP on the relevant MSI domains so that
> IOMMU_CAP_INTR_REMAP can go away for good.

This IRQ_DOMAIN_FLAG_MSI_REMAP is used only on ARM, is there a plan/desire
to use it in other places instead of capable(IOMMU_CAP_INTR_REMAP)?

> That way, all consumers who
> actually care about whether IRQ remapping is implemented (see e.g. VFIO)
> can use irq_domain_check_msi_remap() or check specific devices in a sane
> and scalable manner.

What specific devices are you talking about here? Actual PCI controllers do
not have control over MSIX remapping/filtering...




> 
> Robin.
> 
>>>         if (!ops->add_device)
>>>                 return 0;
>>>
>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>>> index f2393b7d7ebf..14aac9df3d17 100644
>>> --- a/drivers/pci/probe.c
>>> +++ b/drivers/pci/probe.c
>>> @@ -17,6 +17,7 @@
>>>  #include <linux/acpi.h>
>>>  #include <linux/irqdomain.h>
>>>  #include <linux/pm_runtime.h>
>>> +#include <linux/iommu.h>
>>
>> This obviously belongs in another patch, as the compile error showed.
>>
>>>  #include "pci.h"
>>>
>>>  #define CARDBUS_LATENCY_TIMER  176     /* secondary latency timer */
>>> --
>>> 2.11.0
>>>
>> _______________________________________________
>> iommu mailing list
>> iommu@lists.linux-foundation.org
>> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>>
> 


-- 
Alexey

Re: [PATCH kernel v4 4/6] iommu: Set PCI_BUS_FLAGS_MSI_REMAP on iommu driver initialization

[Jike Song]

On 06/30/2017 01:24 PM, Alexey Kardashevskiy wrote:
> From: Yongji Xie <elohimes@gmail.com>
> 
> Some iommu drivers would be initialized after PCI device
> enumeration. So PCI_BUS_FLAGS_MSI_REMAP would not be set
> when probing PCI devices although IOMMU enables capability
> of IRQ remapping. This patch tests this capability and
> set the flag when iommu driver is initialized.
> 
> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  drivers/iommu/iommu.c | 8 ++++++++
>  drivers/pci/probe.c   | 1 +
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index cf7ca7e70777..0b5881ddca09 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1063,6 +1063,14 @@ static int add_iommu_group(struct device *dev, void *data)
>  	const struct iommu_ops *ops = cb->ops;
>  	int ret;
>  
> +	/*
> +	 * Set PCI_BUS_FLAGS_MSI_REMAP for all PCI buses when IOMMU
> +	 * have capability of IRQ remapping.
> +	 */
> +	if (dev_is_pci(dev) && ops->capable &&
> +			ops->capable(IOMMU_CAP_INTR_REMAP))
> +		to_pci_dev(dev)->bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP;

[+Kevin]


Hi Alexey,

Just a reminder, you might want to check the proposed patch to intel-iommu
by Alex, at:

	https://patchwork.kernel.org/patch/9092511/

Without that being the prerequisite, this patch will probably introduce
security issues on x86.

--
Thanks,
Jike

> +
>  	if (!ops->add_device)
>  		return 0;
>  
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index f2393b7d7ebf..14aac9df3d17 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -17,6 +17,7 @@
>  #include <linux/acpi.h>
>  #include <linux/irqdomain.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/iommu.h>
>  #include "pci.h"
>  
>  #define CARDBUS_LATENCY_TIMER	176	/* secondary latency timer */
> 

Re: [PATCH kernel v4 4/6] iommu: Set PCI_BUS_FLAGS_MSI_REMAP on iommu driver initialization

[Alexey Kardashevskiy]

On 12/07/17 17:04, Jike Song wrote:
> On 06/30/2017 01:24 PM, Alexey Kardashevskiy wrote:
>> From: Yongji Xie <elohimes@gmail.com>
>>
>> Some iommu drivers would be initialized after PCI device
>> enumeration. So PCI_BUS_FLAGS_MSI_REMAP would not be set
>> when probing PCI devices although IOMMU enables capability
>> of IRQ remapping. This patch tests this capability and
>> set the flag when iommu driver is initialized.
>>
>> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>  drivers/iommu/iommu.c | 8 ++++++++
>>  drivers/pci/probe.c   | 1 +
>>  2 files changed, 9 insertions(+)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index cf7ca7e70777..0b5881ddca09 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -1063,6 +1063,14 @@ static int add_iommu_group(struct device *dev, void *data)
>>  	const struct iommu_ops *ops = cb->ops;
>>  	int ret;
>>  
>> +	/*
>> +	 * Set PCI_BUS_FLAGS_MSI_REMAP for all PCI buses when IOMMU
>> +	 * have capability of IRQ remapping.
>> +	 */
>> +	if (dev_is_pci(dev) && ops->capable &&
>> +			ops->capable(IOMMU_CAP_INTR_REMAP))
>> +		to_pci_dev(dev)->bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP;
> 
> [+Kevin]
> 
> 
> Hi Alexey,
> 
> Just a reminder, you might want to check the proposed patch to intel-iommu
> by Alex, at:
> 
> 	https://patchwork.kernel.org/patch/9092511/
> 
> Without that being the prerequisite, this patch will probably introduce
> security issues on x86.



Thanks, noted. Anything like this for AMD/s390?




> 
> --
> Thanks,
> Jike
> 
>> +
>>  	if (!ops->add_device)
>>  		return 0;
>>  
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index f2393b7d7ebf..14aac9df3d17 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -17,6 +17,7 @@
>>  #include <linux/acpi.h>
>>  #include <linux/irqdomain.h>
>>  #include <linux/pm_runtime.h>
>> +#include <linux/iommu.h>
>>  #include "pci.h"
>>  
>>  #define CARDBUS_LATENCY_TIMER	176	/* secondary latency timer */
>>


-- 
Alexey

Re: [PATCH kernel v4 4/6] iommu: Set PCI_BUS_FLAGS_MSI_REMAP on iommu driver initialization

[Benjamin Herrenschmidt]

On Tue, 2017-07-11 at 11:39 +0100, Robin Murphy wrote:
> I have no idea what the context is here, but this flag looks wrong
> generally. IRQ remapping is a property of the irqchip and has nothing to
> do with PCI, so pretending it's a property of PCI buses looks like a
> massive hack around... something.
> 
> Also, iommu_capable() is a fundamentally broken and unworkable interface
> anyway. The existing IRQ remapping code really wants updating to
> advertise IRQ_DOMAIN_FLAG_MSI_REMAP on the relevant MSI domains so that
> IOMMU_CAP_INTR_REMAP can go away for good. That way, all consumers who
> actually care about whether IRQ remapping is implemented (see e.g. VFIO)
> can use irq_domain_check_msi_remap() or check specific devices in a sane
> and scalable manner.

As you rightfully said, you have no idea what the context is :-)

This is not an interrupt domain.

On powerpc we have a single global unified domains that contains all
our MSIs, IPIs, internally interrupts and what not, because of the way
our interrupts infrastructure works.

So there is no such thing as "a property of the MSI domain".

The way the HW works is that the PCI Host Bridge has the ability
to filter which device can access which MSIs. This is done by the IOMMU
portion of the bridge.

Thus it's a filtering capability, not a remapping capability per-se,
and it's a property of the IOMMU domain.

Sicne this is also a paravitualized interface, the "remapping" part
is handled by the HV calls done by the guest to configure the MSIs.

The HW filtering ensures that an evil guest cannot do bad things if
it goes manually whack the MSI table.

Now this issue have been discussed and patches floated around for
*YEARS* now and there is still no upstream solution for what is a
completely trivial problem: Don't bloody bloock MSI BAR table access on
pseries platforms. It kills performance on a number of device due to
our 64K pages.

A 1-liner fix in qemu would have done it YEARS ago. But instead we have
now painted about 1000 bike sheds and going without anything that
actually works. Yay.

Ben.

Re: [PATCH kernel v4 4/6] iommu: Set PCI_BUS_FLAGS_MSI_REMAP on iommu driver initialization

[Alexey Kardashevskiy]

On 11/07/17 05:23, Bjorn Helgaas wrote:
> [+cc Joerg, iommu]
> 
> On Fri, Jun 30, 2017 at 12:24 AM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>> From: Yongji Xie <elohimes@gmail.com>
>>
>> Some iommu drivers would be initialized after PCI device
>> enumeration. So PCI_BUS_FLAGS_MSI_REMAP would not be set
>> when probing PCI devices although IOMMU enables capability
>> of IRQ remapping. This patch tests this capability and
>> set the flag when iommu driver is initialized.
>>
>> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>  drivers/iommu/iommu.c | 8 ++++++++
>>  drivers/pci/probe.c   | 1 +
>>  2 files changed, 9 insertions(+)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index cf7ca7e70777..0b5881ddca09 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -1063,6 +1063,14 @@ static int add_iommu_group(struct device *dev, void *data)
>>         const struct iommu_ops *ops = cb->ops;
>>         int ret;
>>
>> +       /*
>> +        * Set PCI_BUS_FLAGS_MSI_REMAP for all PCI buses when IOMMU
>> +        * have capability of IRQ remapping.
>> +        */
>> +       if (dev_is_pci(dev) && ops->capable &&
>> +                       ops->capable(IOMMU_CAP_INTR_REMAP))
>> +               to_pci_dev(dev)->bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP;
> 
> This isn't my area, but this addition is really ugly.  It doesn't look
> anything like the rest of the code in add_iommu_group(), so it just
> feels like a wart.


It does look like a wart, however it did not cause immediate rejection
after first couple of revisions of this before I took it over...

So. Here is the problem - deliver an MSIX isolation flag from IOMMU to
VFIO-PCI driver. The options are:

1. PCI_BUS_FLAGS_MSI_REMAP on a PCI bus - MSIX isolation is not really a
PCI bus property and it is "like a wart".

2. Introduce "flags" in iommu_group and define IOMMU_GROUP_MSIX_ISOLATED
and set it when an IOMMU group is created; VFIO-PCI has ways to get to the
group and read the flag.

3. Create IOMMU_DOMAIN_UNMANAGED IOMMU domains for PPC64/powernv IOMMU
groups and only define capable() hook to report IOMMU_CAP_INTR_REMAP;
others already use these IOMMU domains. VFIO-PCI's mmap() hook could then
check the capability via iommu_capable(bus). The problems is as Robin said:
"iommu_capable() is a fundamentally broken and unworkable interface
anyway"; however it is still not clear to me why it is unworkable in this
particular case of isolation checking.

I am missing knowledge about ARM, is there a good overview of these MSIX
domains vs. IOMMU on ARM (as MSIX remapping seems not to be an IOMMU property)?


Which one to pick? Thanks.



> 
>>         if (!ops->add_device)
>>                 return 0;
>>
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index f2393b7d7ebf..14aac9df3d17 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -17,6 +17,7 @@
>>  #include <linux/acpi.h>
>>  #include <linux/irqdomain.h>
>>  #include <linux/pm_runtime.h>
>> +#include <linux/iommu.h>
> 
> This obviously belongs in another patch, as the compile error showed.
> 
>>  #include "pci.h"
>>
>>  #define CARDBUS_LATENCY_TIMER  176     /* secondary latency timer */
>> --
>> 2.11.0
>>


-- 
Alexey

Re: [PATCH kernel v4 4/6] iommu: Set PCI_BUS_FLAGS_MSI_REMAP on iommu driver initialization

[Alexey Kardashevskiy]

On 19/07/17 20:02, Alexey Kardashevskiy wrote:
> On 11/07/17 05:23, Bjorn Helgaas wrote:
>> [+cc Joerg, iommu]
>>
>> On Fri, Jun 30, 2017 at 12:24 AM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>> From: Yongji Xie <elohimes@gmail.com>
>>>
>>> Some iommu drivers would be initialized after PCI device
>>> enumeration. So PCI_BUS_FLAGS_MSI_REMAP would not be set
>>> when probing PCI devices although IOMMU enables capability
>>> of IRQ remapping. This patch tests this capability and
>>> set the flag when iommu driver is initialized.
>>>
>>> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>>  drivers/iommu/iommu.c | 8 ++++++++
>>>  drivers/pci/probe.c   | 1 +
>>>  2 files changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>> index cf7ca7e70777..0b5881ddca09 100644
>>> --- a/drivers/iommu/iommu.c
>>> +++ b/drivers/iommu/iommu.c
>>> @@ -1063,6 +1063,14 @@ static int add_iommu_group(struct device *dev, void *data)
>>>         const struct iommu_ops *ops = cb->ops;
>>>         int ret;
>>>
>>> +       /*
>>> +        * Set PCI_BUS_FLAGS_MSI_REMAP for all PCI buses when IOMMU
>>> +        * have capability of IRQ remapping.
>>> +        */
>>> +       if (dev_is_pci(dev) && ops->capable &&
>>> +                       ops->capable(IOMMU_CAP_INTR_REMAP))
>>> +               to_pci_dev(dev)->bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP;
>>
>> This isn't my area, but this addition is really ugly.  It doesn't look
>> anything like the rest of the code in add_iommu_group(), so it just
>> feels like a wart.
> 
> 
> It does look like a wart, however it did not cause immediate rejection
> after first couple of revisions of this before I took it over...
> 
> So. Here is the problem - deliver an MSIX isolation flag from IOMMU to
> VFIO-PCI driver. The options are:
> 
> 1. PCI_BUS_FLAGS_MSI_REMAP on a PCI bus - MSIX isolation is not really a
> PCI bus property and it is "like a wart".
> 
> 2. Introduce "flags" in iommu_group and define IOMMU_GROUP_MSIX_ISOLATED
> and set it when an IOMMU group is created; VFIO-PCI has ways to get to the
> group and read the flag.
> 
> 3. Create IOMMU_DOMAIN_UNMANAGED IOMMU domains for PPC64/powernv IOMMU
> groups and only define capable() hook to report IOMMU_CAP_INTR_REMAP;
> others already use these IOMMU domains. VFIO-PCI's mmap() hook could then
> check the capability via iommu_capable(bus). The problems is as Robin said:
> "iommu_capable() is a fundamentally broken and unworkable interface
> anyway"; however it is still not clear to me why it is unworkable in this
> particular case of isolation checking.
> 
> I am missing knowledge about ARM, is there a good overview of these MSIX
> domains vs. IOMMU on ARM (as MSIX remapping seems not to be an IOMMU property)?
> 
> 
> Which one to pick? Thanks.



Anyone, please?



> 
> 
> 
>>
>>>         if (!ops->add_device)
>>>                 return 0;
>>>
>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>>> index f2393b7d7ebf..14aac9df3d17 100644
>>> --- a/drivers/pci/probe.c
>>> +++ b/drivers/pci/probe.c
>>> @@ -17,6 +17,7 @@
>>>  #include <linux/acpi.h>
>>>  #include <linux/irqdomain.h>
>>>  #include <linux/pm_runtime.h>
>>> +#include <linux/iommu.h>
>>
>> This obviously belongs in another patch, as the compile error showed.
>>
>>>  #include "pci.h"
>>>
>>>  #define CARDBUS_LATENCY_TIMER  176     /* secondary latency timer */
>>> --
>>> 2.11.0
>>>
> 
> 


-- 
Alexey

Re: [PATCH kernel v4 4/6] iommu: Set PCI_BUS_FLAGS_MSI_REMAP on iommu driver initialization

[Joerg Roedel]

On Wed, Jul 19, 2017 at 08:02:04PM +1000, Alexey Kardashevskiy wrote:
> On 11/07/17 05:23, Bjorn Helgaas wrote:
> >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> >> index cf7ca7e70777..0b5881ddca09 100644
> >> --- a/drivers/iommu/iommu.c
> >> +++ b/drivers/iommu/iommu.c
> >> @@ -1063,6 +1063,14 @@ static int add_iommu_group(struct device *dev, void *data)
> >>         const struct iommu_ops *ops = cb->ops;
> >>         int ret;
> >>
> >> +       /*
> >> +        * Set PCI_BUS_FLAGS_MSI_REMAP for all PCI buses when IOMMU
> >> +        * have capability of IRQ remapping.
> >> +        */
> >> +       if (dev_is_pci(dev) && ops->capable &&
> >> +                       ops->capable(IOMMU_CAP_INTR_REMAP))
> >> +               to_pci_dev(dev)->bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP;

We avoid bus-specific hacks in generic iommu code. This has to be done
in bus-specific iommu-group setup code.

> 1. PCI_BUS_FLAGS_MSI_REMAP on a PCI bus - MSIX isolation is not really a
> PCI bus property and it is "like a wart".

This one is at least debatable. It could be a property of the bus.

> 2. Introduce "flags" in iommu_group and define IOMMU_GROUP_MSIX_ISOLATED
> and set it when an IOMMU group is created; VFIO-PCI has ways to get to the
> group and read the flag.

That's the best option I see here.

> 3. Create IOMMU_DOMAIN_UNMANAGED IOMMU domains for PPC64/powernv IOMMU
> groups and only define capable() hook to report IOMMU_CAP_INTR_REMAP;
> others already use these IOMMU domains. VFIO-PCI's mmap() hook could then
> check the capability via iommu_capable(bus). The problems is as Robin said:
> "iommu_capable() is a fundamentally broken and unworkable interface
> anyway"; however it is still not clear to me why it is unworkable in this
> particular case of isolation checking.

That one is wrong, IRQ remapping is not a property of a domain. A domain
is an abstraction for a device address space. Attaching IRQ information
there is just wrong.



	Joerg

Re: [PATCH kernel v4 4/6] iommu: Set PCI_BUS_FLAGS_MSI_REMAP on iommu driver initialization

[Joerg Roedel]

On Wed, Jul 19, 2017 at 08:02:04PM +1000, Alexey Kardashevskiy wrote:
> On 11/07/17 05:23, Bjorn Helgaas wrote:
> >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> >> index cf7ca7e70777..0b5881ddca09 100644
> >> --- a/drivers/iommu/iommu.c
> >> +++ b/drivers/iommu/iommu.c
> >> @@ -1063,6 +1063,14 @@ static int add_iommu_group(struct device *dev, void *data)
> >>         const struct iommu_ops *ops = cb->ops;
> >>         int ret;
> >>
> >> +       /*
> >> +        * Set PCI_BUS_FLAGS_MSI_REMAP for all PCI buses when IOMMU
> >> +        * have capability of IRQ remapping.
> >> +        */
> >> +       if (dev_is_pci(dev) && ops->capable &&
> >> +                       ops->capable(IOMMU_CAP_INTR_REMAP))
> >> +               to_pci_dev(dev)->bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP;

We avoid bus-specific hacks in generic iommu code. This has to be done
in bus-specific iommu-group setup code.

> 1. PCI_BUS_FLAGS_MSI_REMAP on a PCI bus - MSIX isolation is not really a
> PCI bus property and it is "like a wart".

This one is at least debatable. It could be a property of the bus.

> 2. Introduce "flags" in iommu_group and define IOMMU_GROUP_MSIX_ISOLATED
> and set it when an IOMMU group is created; VFIO-PCI has ways to get to the
> group and read the flag.

That's the best option I see here.

> 3. Create IOMMU_DOMAIN_UNMANAGED IOMMU domains for PPC64/powernv IOMMU
> groups and only define capable() hook to report IOMMU_CAP_INTR_REMAP;
> others already use these IOMMU domains. VFIO-PCI's mmap() hook could then
> check the capability via iommu_capable(bus). The problems is as Robin said:
> "iommu_capable() is a fundamentally broken and unworkable interface
> anyway"; however it is still not clear to me why it is unworkable in this
> particular case of isolation checking.

That one is wrong, IRQ remapping is not a property of a domain. A domain
is an abstraction for a device address space. Attaching IRQ information
there is just wrong.



	Joerg

Re: [PATCH kernel v4 4/6] iommu: Set PCI_BUS_FLAGS_MSI_REMAP on iommu driver initialization

[Joerg Roedel]

On Wed, Jul 19, 2017 at 08:02:04PM +1000, Alexey Kardashevskiy wrote:
> On 11/07/17 05:23, Bjorn Helgaas wrote:
> >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> >> index cf7ca7e70777..0b5881ddca09 100644
> >> --- a/drivers/iommu/iommu.c
> >> +++ b/drivers/iommu/iommu.c
> >> @@ -1063,6 +1063,14 @@ static int add_iommu_group(struct device *dev, void *data)
> >>         const struct iommu_ops *ops = cb->ops;
> >>         int ret;
> >>
> >> +       /*
> >> +        * Set PCI_BUS_FLAGS_MSI_REMAP for all PCI buses when IOMMU
> >> +        * have capability of IRQ remapping.
> >> +        */
> >> +       if (dev_is_pci(dev) && ops->capable &&
> >> +                       ops->capable(IOMMU_CAP_INTR_REMAP))
> >> +               to_pci_dev(dev)->bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP;

We avoid bus-specific hacks in generic iommu code. This has to be done
in bus-specific iommu-group setup code.

> 1. PCI_BUS_FLAGS_MSI_REMAP on a PCI bus - MSIX isolation is not really a
> PCI bus property and it is "like a wart".

This one is at least debatable. It could be a property of the bus.

> 2. Introduce "flags" in iommu_group and define IOMMU_GROUP_MSIX_ISOLATED
> and set it when an IOMMU group is created; VFIO-PCI has ways to get to the
> group and read the flag.

That's the best option I see here.

> 3. Create IOMMU_DOMAIN_UNMANAGED IOMMU domains for PPC64/powernv IOMMU
> groups and only define capable() hook to report IOMMU_CAP_INTR_REMAP;
> others already use these IOMMU domains. VFIO-PCI's mmap() hook could then
> check the capability via iommu_capable(bus). The problems is as Robin said:
> "iommu_capable() is a fundamentally broken and unworkable interface
> anyway"; however it is still not clear to me why it is unworkable in this
> particular case of isolation checking.

That one is wrong, IRQ remapping is not a property of a domain. A domain
is an abstraction for a device address space. Attaching IRQ information
there is just wrong.



	Joerg

Re: [PATCH kernel v4 4/6] iommu: Set PCI_BUS_FLAGS_MSI_REMAP on iommu driver initialization

[Benjamin Herrenschmidt]

On Wed, 2017-07-26 at 11:50 +0200, Joerg Roedel wrote:
> > 3. Create IOMMU_DOMAIN_UNMANAGED IOMMU domains for PPC64/powernv IOMMU
> > groups and only define capable() hook to report IOMMU_CAP_INTR_REMAP;
> > others already use these IOMMU domains. VFIO-PCI's mmap() hook could then
> > check the capability via iommu_capable(bus). The problems is as Robin said:
> > "iommu_capable() is a fundamentally broken and unworkable interface
> > anyway"; however it is still not clear to me why it is unworkable in this
> > particular case of isolation checking.
> 
> That one is wrong, IRQ remapping is not a property of a domain. A domain
> is an abstraction for a device address space. Attaching IRQ information
> there is just wrong.

Except it somewhat is ... an MSI is a store in the device address
space, the way MSIs are handled and/or filtered can be considered a
property of the domain. In our case, it's the exact same piece of HW
that defines domains and which MSIs they are allowed to generate.

Cheers,
Ben.

Re: [PATCH kernel v4 4/6] iommu: Set PCI_BUS_FLAGS_MSI_REMAP on iommu driver initialization

[Benjamin Herrenschmidt]

On Wed, 2017-07-26 at 11:50 +0200, Joerg Roedel wrote:
> > 3. Create IOMMU_DOMAIN_UNMANAGED IOMMU domains for PPC64/powernv IOMMU
> > groups and only define capable() hook to report IOMMU_CAP_INTR_REMAP;
> > others already use these IOMMU domains. VFIO-PCI's mmap() hook could then
> > check the capability via iommu_capable(bus). The problems is as Robin said:
> > "iommu_capable() is a fundamentally broken and unworkable interface
> > anyway"; however it is still not clear to me why it is unworkable in this
> > particular case of isolation checking.
> 
> That one is wrong, IRQ remapping is not a property of a domain. A domain
> is an abstraction for a device address space. Attaching IRQ information
> there is just wrong.

Except it somewhat is ... an MSI is a store in the device address
space, the way MSIs are handled and/or filtered can be considered a
property of the domain. In our case, it's the exact same piece of HW
that defines domains and which MSIs they are allowed to generate.

Cheers,
Ben.

Re: [PATCH kernel v4 4/6] iommu: Set PCI_BUS_FLAGS_MSI_REMAP on iommu driver initialization

[Benjamin Herrenschmidt]

On Wed, 2017-07-26 at 11:50 +0200, Joerg Roedel wrote:
> > 3. Create IOMMU_DOMAIN_UNMANAGED IOMMU domains for PPC64/powernv IOMMU
> > groups and only define capable() hook to report IOMMU_CAP_INTR_REMAP;
> > others already use these IOMMU domains. VFIO-PCI's mmap() hook could then
> > check the capability via iommu_capable(bus). The problems is as Robin said:
> > "iommu_capable() is a fundamentally broken and unworkable interface
> > anyway"; however it is still not clear to me why it is unworkable in this
> > particular case of isolation checking.
> 
> That one is wrong, IRQ remapping is not a property of a domain. A domain
> is an abstraction for a device address space. Attaching IRQ information
> there is just wrong.

Except it somewhat is ... an MSI is a store in the device address
space, the way MSIs are handled and/or filtered can be considered a
property of the domain. In our case, it's the exact same piece of HW
that defines domains and which MSIs they are allowed to generate.

Cheers,
Ben.