[RFC v5 6/7] PCI: Add a new bit to pci_bus_flags to indicate interrupt remapping

[RFC v5 6/7] PCI: Add a new bit to pci_bus_flags to indicate interrupt remapping

[Yongji Xie]

I'm trying to find a proper way to indicate
the capability of interrupt remapping on PPC64
because we need this to determine whether it is
safe to mmap MSI-X table in VFIO driver.

There is a existing flag for this in the IOMMU
space:

enum iommu_cap {
	IOMMU_CAP_CACHE_COHERENCY,
--->    IOMMU_CAP_INTR_REMAP,
	IOMMU_CAP_NOEXEC,
};

But it seems to be not a good idea to use
bus_set_iommu() to add this flag on PPC64 which
never set/use iommu_ops.

Eric also posted a patchset [1] to abstract
this capability on MSI controller side for ARM.
But I found we also never use msi_domain_info
on PPC64. We have to rework the MSI code on
PPC64 to support msi_domain_ops if we want to
use msi_domain_info.

Then I noticed that interrupt remapping is
abstracted on PCI host bridge side on PPC64.
So I'm thinking whether we could add a new
bit to pci_bus_flags to indicate this
capability, like this patch does.

Compared with IOMMU_CAP_INTR_REMAP, this
flag is more common. Any arch can set/use this
easily. And it can provide a better granularity
(pci_bus_type -> pci_bus).

[1] http://www.spinics.net/lists/kvm/msg130262.html

Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/powernv/pci-ioda.c |    8 ++++++++
 include/linux/pci.h                       |    1 +
 2 files changed, 9 insertions(+)

diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index f90dc04..9557638 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -3080,6 +3080,12 @@ static void pnv_pci_ioda_fixup(void)
 	pnv_npu_ioda_fixup();
 }
 
+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.
@@ -3364,6 +3370,8 @@ static void __init pnv_pci_init_ioda_phb(struct device_node *np,
 	 */
 	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;
 	else
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 27df4a6..741dcaf 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -193,6 +193,7 @@ typedef unsigned short __bitwise pci_bus_flags_t;
 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_MSI_REMAP = (__force pci_bus_flags_t) 3,
 };
 
 /* These values come from the PCI Express Spec */
-- 
1.7.9.5

[RFC v5 7/7] vfio-pci: Allow to mmap MSI-X table if interrupt remapping is supported

[Yongji Xie]

This patch enables mmapping MSI-X tables if
hardware supports interrupt remapping which
can ensure that a given pci device can only
shoot the MSIs assigned for it.

Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
---
 drivers/vfio/pci/vfio_pci.c         |    9 +++++++--
 drivers/vfio/pci/vfio_pci_private.h |    1 +
 drivers/vfio/pci/vfio_pci_rdwr.c    |    2 +-
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index c60d790..ef02896 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -201,6 +201,10 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev)
 	} else
 		vdev->msix_bar = 0xFF;
 
+	if (iommu_capable(pdev->dev.bus, IOMMU_CAP_INTR_REMAP) ||
+		pdev->bus->bus_flags | PCI_BUS_FLAGS_MSI_REMAP)
+		vdev->msi_remap = true;
+
 	if (!vfio_vga_disabled() && vfio_pci_is_vga(pdev))
 		vdev->has_vga = true;
 
@@ -635,7 +639,8 @@ static long vfio_pci_ioctl(void *device_data,
 				     VFIO_REGION_INFO_FLAG_WRITE;
 			if (vdev->bar_mmap_supported[info.index]) {
 				info.flags |= VFIO_REGION_INFO_FLAG_MMAP;
-				if (info.index == vdev->msix_bar) {
+				if (info.index == vdev->msix_bar &&
+						!vdev->msi_remap) {
 					ret = msix_sparse_mmap_cap(vdev, &caps);
 					if (ret)
 						return ret;
@@ -1067,7 +1072,7 @@ 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 && !vdev->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_private.h b/drivers/vfio/pci/vfio_pci_private.h
index 0ea4c62..4f20963 100644
--- a/drivers/vfio/pci/vfio_pci_private.h
+++ b/drivers/vfio/pci/vfio_pci_private.h
@@ -78,6 +78,7 @@ struct vfio_pci_device {
 	int			irq_type;
 	int			num_regions;
 	struct vfio_pci_region	*region;
+	bool			msi_remap;
 	u8			msi_qmax;
 	u8			msix_bar;
 	u16			msix_size;
diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
index 5ffd1d9..606ee3c 100644
--- a/drivers/vfio/pci/vfio_pci_rdwr.c
+++ b/drivers/vfio/pci/vfio_pci_rdwr.c
@@ -164,7 +164,7 @@ 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 && !vdev->msi_remap) {
 		x_start = vdev->msix_offset;
 		x_end = vdev->msix_offset + vdev->msix_size;
 	}
-- 
1.7.9.5

Re: [RFC v5 7/7] vfio-pci: Allow to mmap MSI-X table if interrupt remapping is supported

[Gavin Shan]

On Tue, Apr 05, 2016 at 09:46:44PM +0800, Yongji Xie wrote:
>This patch enables mmapping MSI-X tables if
>hardware supports interrupt remapping which
>can ensure that a given pci device can only
>shoot the MSIs assigned for it.
>
>Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
>---
> drivers/vfio/pci/vfio_pci.c         |    9 +++++++--
> drivers/vfio/pci/vfio_pci_private.h |    1 +
> drivers/vfio/pci/vfio_pci_rdwr.c    |    2 +-
> 3 files changed, 9 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
>index c60d790..ef02896 100644
>--- a/drivers/vfio/pci/vfio_pci.c
>+++ b/drivers/vfio/pci/vfio_pci.c
>@@ -201,6 +201,10 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev)
> 	} else
> 		vdev->msix_bar = 0xFF;
>
>+	if (iommu_capable(pdev->dev.bus, IOMMU_CAP_INTR_REMAP) ||
>+		pdev->bus->bus_flags | PCI_BUS_FLAGS_MSI_REMAP)
>+		vdev->msi_remap = true;
>+

I guess you probably need a "&" here. Otherwise, the condition
is always true.

> 	if (!vfio_vga_disabled() && vfio_pci_is_vga(pdev))
> 		vdev->has_vga = true;
>
>@@ -635,7 +639,8 @@ static long vfio_pci_ioctl(void *device_data,
> 				     VFIO_REGION_INFO_FLAG_WRITE;
> 			if (vdev->bar_mmap_supported[info.index]) {
> 				info.flags |= VFIO_REGION_INFO_FLAG_MMAP;
>-				if (info.index == vdev->msix_bar) {
>+				if (info.index == vdev->msix_bar &&
>+						!vdev->msi_remap) {
> 					ret = msix_sparse_mmap_cap(vdev, &caps);
> 					if (ret)
> 						return ret;
>@@ -1067,7 +1072,7 @@ 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 && !vdev->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_private.h b/drivers/vfio/pci/vfio_pci_private.h
>index 0ea4c62..4f20963 100644
>--- a/drivers/vfio/pci/vfio_pci_private.h
>+++ b/drivers/vfio/pci/vfio_pci_private.h
>@@ -78,6 +78,7 @@ struct vfio_pci_device {
> 	int			irq_type;
> 	int			num_regions;
> 	struct vfio_pci_region	*region;
>+	bool			msi_remap;
> 	u8			msi_qmax;
> 	u8			msix_bar;
> 	u16			msix_size;
>diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
>index 5ffd1d9..606ee3c 100644
>--- a/drivers/vfio/pci/vfio_pci_rdwr.c
>+++ b/drivers/vfio/pci/vfio_pci_rdwr.c
>@@ -164,7 +164,7 @@ 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 && !vdev->msi_remap) {
> 		x_start = vdev->msix_offset;
> 		x_end = vdev->msix_offset + vdev->msix_size;
> 	}

When PCI_BUS_FLAGS_MSI_REMAP is set, the MSIx table can be accessed by
read/write interface except mmap(). The commit log doesn't mention it.
It would be better if you have some words about it.

Thanks,
Gavin


>-- 
>1.7.9.5
>

Re: [RFC v5 6/7] PCI: Add a new bit to pci_bus_flags to indicate interrupt remapping

[Gavin Shan]

On Tue, Apr 05, 2016 at 09:46:43PM +0800, Yongji Xie wrote:
>I'm trying to find a proper way to indicate
>the capability of interrupt remapping on PPC64
>because we need this to determine whether it is
>safe to mmap MSI-X table in VFIO driver.
>
>There is a existing flag for this in the IOMMU
>space:
>
>enum iommu_cap {
>	IOMMU_CAP_CACHE_COHERENCY,
>--->    IOMMU_CAP_INTR_REMAP,
>	IOMMU_CAP_NOEXEC,
>};
>
>But it seems to be not a good idea to use
>bus_set_iommu() to add this flag on PPC64 which
>never set/use iommu_ops.
>
>Eric also posted a patchset [1] to abstract
>this capability on MSI controller side for ARM.
>But I found we also never use msi_domain_info
>on PPC64. We have to rework the MSI code on
>PPC64 to support msi_domain_ops if we want to
>use msi_domain_info.
>
>Then I noticed that interrupt remapping is
>abstracted on PCI host bridge side on PPC64.
>So I'm thinking whether we could add a new
>bit to pci_bus_flags to indicate this
>capability, like this patch does.
>
>Compared with IOMMU_CAP_INTR_REMAP, this
>flag is more common. Any arch can set/use this
>easily. And it can provide a better granularity
>(pci_bus_type -> pci_bus).
>
>[1] http://www.spinics.net/lists/kvm/msg130262.html
>
>Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
>---
> arch/powerpc/platforms/powernv/pci-ioda.c |    8 ++++++++
> include/linux/pci.h                       |    1 +
> 2 files changed, 9 insertions(+)
>
>diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
>index f90dc04..9557638 100644
>--- a/arch/powerpc/platforms/powernv/pci-ioda.c
>+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
>@@ -3080,6 +3080,12 @@ static void pnv_pci_ioda_fixup(void)
> 	pnv_npu_ioda_fixup();
> }
>
>+int pnv_pci_ioda_root_bridge_prepare(struct pci_host_bridge *bridge)
>+{
>+	bridge->bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP;
>+	return 0;
>+}
>+

A irelevent question: Here we set the flag unconditionally. Can the
MSIx table be exposed to user space unconditionally? Do we have issues
if the MSIx related BAR isn't page aligned or overlapped with others?

> /*
>  * Returns the alignment for I/O or memory windows for P2P
>  * bridges. That actually depends on how PEs are segmented.
>@@ -3364,6 +3370,8 @@ static void __init pnv_pci_init_ioda_phb(struct device_node *np,
> 	 */
> 	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;
> 	else
>diff --git a/include/linux/pci.h b/include/linux/pci.h
>index 27df4a6..741dcaf 100644
>--- a/include/linux/pci.h
>+++ b/include/linux/pci.h
>@@ -193,6 +193,7 @@ typedef unsigned short __bitwise pci_bus_flags_t;
> 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_MSI_REMAP = (__force pci_bus_flags_t) 3,

Yongji, I guess the corresponding value to PCI_BUS_FLAGS_MSI_REMAP
would be 4 other than 3. Otherwise, two of the flags cannot exist
at the same time.

Thanks,
Gavin

> };
>
> /* These values come from the PCI Express Spec */
>-- 
>1.7.9.5
>

Re: [RFC v5 6/7] PCI: Add a new bit to pci_bus_flags to indicate interrupt remapping

[Yongji Xie]

On 2016/4/6 8:11, Gavin Shan wrote:
> On Tue, Apr 05, 2016 at 09:46:43PM +0800, Yongji Xie wrote:
>> I'm trying to find a proper way to indicate
>> the capability of interrupt remapping on PPC64
>> because we need this to determine whether it is
>> safe to mmap MSI-X table in VFIO driver.
>>
>> There is a existing flag for this in the IOMMU
>> space:
>>
>> enum iommu_cap {
>> 	IOMMU_CAP_CACHE_COHERENCY,
>> --->    IOMMU_CAP_INTR_REMAP,
>> 	IOMMU_CAP_NOEXEC,
>> };
>>
>> But it seems to be not a good idea to use
>> bus_set_iommu() to add this flag on PPC64 which
>> never set/use iommu_ops.
>>
>> Eric also posted a patchset [1] to abstract
>> this capability on MSI controller side for ARM.
>> But I found we also never use msi_domain_info
>> on PPC64. We have to rework the MSI code on
>> PPC64 to support msi_domain_ops if we want to
>> use msi_domain_info.
>>
>> Then I noticed that interrupt remapping is
>> abstracted on PCI host bridge side on PPC64.
>> So I'm thinking whether we could add a new
>> bit to pci_bus_flags to indicate this
>> capability, like this patch does.
>>
>> Compared with IOMMU_CAP_INTR_REMAP, this
>> flag is more common. Any arch can set/use this
>> easily. And it can provide a better granularity
>> (pci_bus_type -> pci_bus).
>>
>> [1] http://www.spinics.net/lists/kvm/msg130262.html
>>
>> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
>> ---
>> arch/powerpc/platforms/powernv/pci-ioda.c |    8 ++++++++
>> include/linux/pci.h                       |    1 +
>> 2 files changed, 9 insertions(+)
>>
>> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
>> index f90dc04..9557638 100644
>> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
>> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
>> @@ -3080,6 +3080,12 @@ static void pnv_pci_ioda_fixup(void)
>> 	pnv_npu_ioda_fixup();
>> }
>>
>> +int pnv_pci_ioda_root_bridge_prepare(struct pci_host_bridge *bridge)
>> +{
>> +	bridge->bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP;
>> +	return 0;
>> +}
>> +
> A irelevent question: Here we set the flag unconditionally. Can the
> MSIx table be exposed to user space unconditionally? Do we have issues
> if the MSIx related BAR isn't page aligned or overlapped with others?

This flag is just used to indicate the capability of MSI
remapping which means PCI devices on this bus
can only shoot the MSIs assigned for them. It does
not mean MSIx table can be exposed to user space
unconditionally.

>> /*
>>   * Returns the alignment for I/O or memory windows for P2P
>>   * bridges. That actually depends on how PEs are segmented.
>> @@ -3364,6 +3370,8 @@ static void __init pnv_pci_init_ioda_phb(struct device_node *np,
>> 	 */
>> 	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;
>> 	else
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 27df4a6..741dcaf 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -193,6 +193,7 @@ typedef unsigned short __bitwise pci_bus_flags_t;
>> 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_MSI_REMAP = (__force pci_bus_flags_t) 3,
> Yongji, I guess the corresponding value to PCI_BUS_FLAGS_MSI_REMAP
> would be 4 other than 3. Otherwise, two of the flags cannot exist
> at the same time.
>
> Thanks,
> Gavin

Yes, you are right. It should be 4 (1 << 2).

Thanks,
Yongji

>> };
>>
>> /* These values come from the PCI Express Spec */
>> -- 
>> 1.7.9.5
>>

Re: [RFC v5 7/7] vfio-pci: Allow to mmap MSI-X table if interrupt remapping is supported

[Yongji Xie]

On 2016/4/6 8:00, Gavin Shan wrote:
> On Tue, Apr 05, 2016 at 09:46:44PM +0800, Yongji Xie wrote:
>> This patch enables mmapping MSI-X tables if
>> hardware supports interrupt remapping which
>> can ensure that a given pci device can only
>> shoot the MSIs assigned for it.
>>
>> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
>> ---
>> drivers/vfio/pci/vfio_pci.c         |    9 +++++++--
>> drivers/vfio/pci/vfio_pci_private.h |    1 +
>> drivers/vfio/pci/vfio_pci_rdwr.c    |    2 +-
>> 3 files changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
>> index c60d790..ef02896 100644
>> --- a/drivers/vfio/pci/vfio_pci.c
>> +++ b/drivers/vfio/pci/vfio_pci.c
>> @@ -201,6 +201,10 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev)
>> 	} else
>> 		vdev->msix_bar = 0xFF;
>>
>> +	if (iommu_capable(pdev->dev.bus, IOMMU_CAP_INTR_REMAP) ||
>> +		pdev->bus->bus_flags | PCI_BUS_FLAGS_MSI_REMAP)
>> +		vdev->msi_remap = true;
>> +
> I guess you probably need a "&" here. Otherwise, the condition
> is always true.

Yes, you are right. I'll fix it.

>> 	if (!vfio_vga_disabled() && vfio_pci_is_vga(pdev))
>> 		vdev->has_vga = true;
>>
>> @@ -635,7 +639,8 @@ static long vfio_pci_ioctl(void *device_data,
>> 				     VFIO_REGION_INFO_FLAG_WRITE;
>> 			if (vdev->bar_mmap_supported[info.index]) {
>> 				info.flags |= VFIO_REGION_INFO_FLAG_MMAP;
>> -				if (info.index == vdev->msix_bar) {
>> +				if (info.index == vdev->msix_bar &&
>> +						!vdev->msi_remap) {
>> 					ret = msix_sparse_mmap_cap(vdev, &caps);
>> 					if (ret)
>> 						return ret;
>> @@ -1067,7 +1072,7 @@ 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 && !vdev->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_private.h b/drivers/vfio/pci/vfio_pci_private.h
>> index 0ea4c62..4f20963 100644
>> --- a/drivers/vfio/pci/vfio_pci_private.h
>> +++ b/drivers/vfio/pci/vfio_pci_private.h
>> @@ -78,6 +78,7 @@ struct vfio_pci_device {
>> 	int			irq_type;
>> 	int			num_regions;
>> 	struct vfio_pci_region	*region;
>> +	bool			msi_remap;
>> 	u8			msi_qmax;
>> 	u8			msix_bar;
>> 	u16			msix_size;
>> diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
>> index 5ffd1d9..606ee3c 100644
>> --- a/drivers/vfio/pci/vfio_pci_rdwr.c
>> +++ b/drivers/vfio/pci/vfio_pci_rdwr.c
>> @@ -164,7 +164,7 @@ 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 && !vdev->msi_remap) {
>> 		x_start = vdev->msix_offset;
>> 		x_end = vdev->msix_offset + vdev->msix_size;
>> 	}
> When PCI_BUS_FLAGS_MSI_REMAP is set, the MSIx table can be accessed by
> read/write interface except mmap(). The commit log doesn't mention it.
> It would be better if you have some words about it.
>
> Thanks,
> Gavin
>

OK. I will mention it in commit log.

Thanks,
Yongji

>> -- 
>> 1.7.9.5
>>

Re: [RFC v5 7/7] vfio-pci: Allow to mmap MSI-X table if interrupt remapping is supported

[Alex Williamson]

On Tue,  5 Apr 2016 21:46:44 +0800
Yongji Xie <xyjxie@linux.vnet.ibm.com> wrote:

> This patch enables mmapping MSI-X tables if
> hardware supports interrupt remapping which
> can ensure that a given pci device can only
> shoot the MSIs assigned for it.
> 
> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
> ---
>  drivers/vfio/pci/vfio_pci.c         |    9 +++++++--
>  drivers/vfio/pci/vfio_pci_private.h |    1 +
>  drivers/vfio/pci/vfio_pci_rdwr.c    |    2 +-
>  3 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index c60d790..ef02896 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -201,6 +201,10 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev)
>  	} else
>  		vdev->msix_bar = 0xFF;
>  
> +	if (iommu_capable(pdev->dev.bus, IOMMU_CAP_INTR_REMAP) ||

This doesn't address the issue I raised earlier where ARM SMMU sets
this capability, but doesn't really provide per vector isolation.  ARM
either needs to be fixed or we need to consider the whole capability
tainted for this application and standardize around the bus flags.
It's not very desirable to have two different ways to test this anyway.

> +		pdev->bus->bus_flags | PCI_BUS_FLAGS_MSI_REMAP)

Perhaps some sort of wrapper for testing these flags would help avoid
this kind of coding error (| vs &)

> +		vdev->msi_remap = true;
> +
>  	if (!vfio_vga_disabled() && vfio_pci_is_vga(pdev))
>  		vdev->has_vga = true;
>  
> @@ -635,7 +639,8 @@ static long vfio_pci_ioctl(void *device_data,
>  				     VFIO_REGION_INFO_FLAG_WRITE;
>  			if (vdev->bar_mmap_supported[info.index]) {
>  				info.flags |= VFIO_REGION_INFO_FLAG_MMAP;
> -				if (info.index == vdev->msix_bar) {
> +				if (info.index == vdev->msix_bar &&
> +						!vdev->msi_remap) {
>  					ret = msix_sparse_mmap_cap(vdev, &caps);
>  					if (ret)
>  						return ret;
> @@ -1067,7 +1072,7 @@ 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 && !vdev->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_private.h b/drivers/vfio/pci/vfio_pci_private.h
> index 0ea4c62..4f20963 100644
> --- a/drivers/vfio/pci/vfio_pci_private.h
> +++ b/drivers/vfio/pci/vfio_pci_private.h
> @@ -78,6 +78,7 @@ struct vfio_pci_device {
>  	int			irq_type;
>  	int			num_regions;
>  	struct vfio_pci_region	*region;
> +	bool			msi_remap;
>  	u8			msi_qmax;
>  	u8			msix_bar;
>  	u16			msix_size;
> diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
> index 5ffd1d9..606ee3c 100644
> --- a/drivers/vfio/pci/vfio_pci_rdwr.c
> +++ b/drivers/vfio/pci/vfio_pci_rdwr.c
> @@ -164,7 +164,7 @@ 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 && !vdev->msi_remap) {
>  		x_start = vdev->msix_offset;
>  		x_end = vdev->msix_offset + vdev->msix_size;
>  	}

Re: [RFC v5 7/7] vfio-pci: Allow to mmap MSI-X table if interrupt remapping is supported

[Yongji Xie]

On 2016/4/6 22:45, Alex Williamson wrote:
> On Tue,  5 Apr 2016 21:46:44 +0800
> Yongji Xie <xyjxie@linux.vnet.ibm.com> wrote:
>
>> This patch enables mmapping MSI-X tables if
>> hardware supports interrupt remapping which
>> can ensure that a given pci device can only
>> shoot the MSIs assigned for it.
>>
>> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
>> ---
>>   drivers/vfio/pci/vfio_pci.c         |    9 +++++++--
>>   drivers/vfio/pci/vfio_pci_private.h |    1 +
>>   drivers/vfio/pci/vfio_pci_rdwr.c    |    2 +-
>>   3 files changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
>> index c60d790..ef02896 100644
>> --- a/drivers/vfio/pci/vfio_pci.c
>> +++ b/drivers/vfio/pci/vfio_pci.c
>> @@ -201,6 +201,10 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev)
>>   	} else
>>   		vdev->msix_bar = 0xFF;
>>   
>> +	if (iommu_capable(pdev->dev.bus, IOMMU_CAP_INTR_REMAP) ||
> This doesn't address the issue I raised earlier where ARM SMMU sets
> this capability, but doesn't really provide per vector isolation.  ARM
> either needs to be fixed or we need to consider the whole capability
> tainted for this application and standardize around the bus flags.
> It's not very desirable to have two different ways to test this anyway.

I saw Eric posted a patchset [1] which introduce a flag
MSI_FLAG_IRQ_REMAPPING to indicate the capability
for ARM SMMU. With this patchset applied, it would
be  workable to use bus_flags to test the capability
of ARM SMMU:

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index a080f44..b2d1756 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -1134,6 +1134,21 @@ void *msi_desc_to_pci_sysdata(struct msi_desc *desc)
  }
  EXPORT_SYMBOL_GPL(msi_desc_to_pci_sysdata);

+void pci_check_msi_remapping(struct pci_bus *bus)
+{
+#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
+        struct irq_domain *domain;
+        struct msi_domain_info *info;
+
+        domain = dev_get_msi_domain(&bus->dev);
+        if (domain) {
+                info = msi_get_domain_info(domain);
+                if (info->flags & MSI_FLAG_IRQ_REMAPPING)
+                        pdev->bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP;
+        }
+#endif
+}
+
  #ifdef CONFIG_PCI_MSI_IRQ_DOMAIN
  /**
   * pci_msi_domain_write_msg - Helper to write MSI message to PCI 
config space
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 6d7ab9b..24e9606 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2115,6 +2115,7 @@ struct pci_bus *pci_create_root_bus(struct device 
*parent, int bus,
         device_enable_async_suspend(b->bridge);
         pci_set_bus_of_node(b);
         pci_set_bus_msi_domain(b);
+       pci_check_msi_remapping(b);

         if (!parent)
                 set_dev_node(b->bridge, pcibus_to_node(b));
diff --git a/include/linux/msi.h b/include/linux/msi.h
index a2a0068..fe8ce7b 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -15,6 +15,7 @@ extern int pci_msi_ignore_mask;
  struct irq_data;
  struct msi_desc;
  struct pci_dev;
+struct pci_bus;
  struct platform_msi_priv_data;
  void __get_cached_msi_msg(struct msi_desc *entry, struct msi_msg *msg);
  void get_cached_msi_msg(unsigned int irq, struct msi_msg *msg);
@@ -155,6 +156,8 @@ void arch_restore_msi_irqs(struct pci_dev *dev);
  void default_teardown_msi_irqs(struct pci_dev *dev);
  void default_restore_msi_irqs(struct pci_dev *dev);

+void pci_check_msi_remapping(struct pci_bus *bus);
+
  struct msi_controller {
         struct module *owner;
         struct device *dev;

Next we just need to find a proper way to make
bus_flags compatible with IOMMU_CAP_INTR_REMAP, right?

I think a good place to do that is add_iommu_group().
But I'm not sure whether iommu drivers must be
initialized after PCI enumeration.  Do you have any comment?

[1] http://www.spinics.net/lists/kvm/msg130256.html

>> +		pdev->bus->bus_flags | PCI_BUS_FLAGS_MSI_REMAP)
> Perhaps some sort of wrapper for testing these flags would help avoid
> this kind of coding error (| vs &)

Thank you.  I'll try not to make the same mistake again.

Regards,
Yongji

Re: [RFC v5 7/7] vfio-pci: Allow to mmap MSI-X table if interrupt remapping is supported

[Eric Auger]

Hi Yongji,
On 04/07/2016 01:38 PM, Yongji Xie wrote:
> On 2016/4/6 22:45, Alex Williamson wrote:
>> On Tue,  5 Apr 2016 21:46:44 +0800
>> Yongji Xie <xyjxie@linux.vnet.ibm.com> wrote:
>>
>>> This patch enables mmapping MSI-X tables if
>>> hardware supports interrupt remapping which
>>> can ensure that a given pci device can only
>>> shoot the MSIs assigned for it.
>>>
>>> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
>>> ---
>>>   drivers/vfio/pci/vfio_pci.c         |    9 +++++++--
>>>   drivers/vfio/pci/vfio_pci_private.h |    1 +
>>>   drivers/vfio/pci/vfio_pci_rdwr.c    |    2 +-
>>>   3 files changed, 9 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
>>> index c60d790..ef02896 100644
>>> --- a/drivers/vfio/pci/vfio_pci.c
>>> +++ b/drivers/vfio/pci/vfio_pci.c
>>> @@ -201,6 +201,10 @@ static int vfio_pci_enable(struct
>>> vfio_pci_device *vdev)
>>>       } else
>>>           vdev->msix_bar = 0xFF;
>>>   +    if (iommu_capable(pdev->dev.bus, IOMMU_CAP_INTR_REMAP) ||
>> This doesn't address the issue I raised earlier where ARM SMMU sets
>> this capability, but doesn't really provide per vector isolation.  ARM
>> either needs to be fixed or we need to consider the whole capability
>> tainted for this application and standardize around the bus flags.
>> It's not very desirable to have two different ways to test this anyway.
> 
> I saw Eric posted a patchset [1] which introduce a flag
> MSI_FLAG_IRQ_REMAPPING to indicate the capability
> for ARM SMMU. With this patchset applied, it would
> be  workable to use bus_flags to test the capability
> of ARM SMMU:

My purpose was to remove the advertising of IOMMU_CAP_INTR_REMAP from
arm-smmu.c, "fix" mentionned by Alex (by the way I also need to do the
same in v3 code) and to advertise the functionality on MSI controller
instead (since the IRQ REMAPPING functionality is abstracted in GICv3
ITS MSI controller)

On top of that, on ARM we have platform (non PCI) MSI controllers so my
understanding is the capability advertising should be possible beyond
the PCI bus?

Best Regards

Eric
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index a080f44..b2d1756 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1134,6 +1134,21 @@ void *msi_desc_to_pci_sysdata(struct msi_desc *desc)
>  }
>  EXPORT_SYMBOL_GPL(msi_desc_to_pci_sysdata);
> 
> +void pci_check_msi_remapping(struct pci_bus *bus)
> +{
> +#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
> +        struct irq_domain *domain;
> +        struct msi_domain_info *info;
> +
> +        domain = dev_get_msi_domain(&bus->dev);
> +        if (domain) {
> +                info = msi_get_domain_info(domain);
> +                if (info->flags & MSI_FLAG_IRQ_REMAPPING)
> +                        pdev->bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP;
> +        }
> +#endif
> +}
> +
>  #ifdef CONFIG_PCI_MSI_IRQ_DOMAIN
>  /**
>   * pci_msi_domain_write_msg - Helper to write MSI message to PCI config
> space
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 6d7ab9b..24e9606 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2115,6 +2115,7 @@ struct pci_bus *pci_create_root_bus(struct device
> *parent, int bus,
>         device_enable_async_suspend(b->bridge);
>         pci_set_bus_of_node(b);
>         pci_set_bus_msi_domain(b);
> +       pci_check_msi_remapping(b);
> 
>         if (!parent)
>                 set_dev_node(b->bridge, pcibus_to_node(b));
> diff --git a/include/linux/msi.h b/include/linux/msi.h
> index a2a0068..fe8ce7b 100644
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -15,6 +15,7 @@ extern int pci_msi_ignore_mask;
>  struct irq_data;
>  struct msi_desc;
>  struct pci_dev;
> +struct pci_bus;
>  struct platform_msi_priv_data;
>  void __get_cached_msi_msg(struct msi_desc *entry, struct msi_msg *msg);
>  void get_cached_msi_msg(unsigned int irq, struct msi_msg *msg);
> @@ -155,6 +156,8 @@ void arch_restore_msi_irqs(struct pci_dev *dev);
>  void default_teardown_msi_irqs(struct pci_dev *dev);
>  void default_restore_msi_irqs(struct pci_dev *dev);
> 
> +void pci_check_msi_remapping(struct pci_bus *bus);
> +
>  struct msi_controller {
>         struct module *owner;
>         struct device *dev;
> 
> Next we just need to find a proper way to make
> bus_flags compatible with IOMMU_CAP_INTR_REMAP, right?
> 
> I think a good place to do that is add_iommu_group().
> But I'm not sure whether iommu drivers must be
> initialized after PCI enumeration.  Do you have any comment?
> 
> [1] http://www.spinics.net/lists/kvm/msg130256.html
> 
>>> +        pdev->bus->bus_flags | PCI_BUS_FLAGS_MSI_REMAP)
>> Perhaps some sort of wrapper for testing these flags would help avoid
>> this kind of coding error (| vs &)
> 
> Thank you.  I'll try not to make the same mistake again.
> 
> Regards,
> Yongji
> 

Re: [RFC v5 7/7] vfio-pci: Allow to mmap MSI-X table if interrupt remapping is supported

[Yongji Xie]

Hi Eric,
On 2016/4/7 22:23, Eric Auger wrote:
> Hi Yongji,
> On 04/07/2016 01:38 PM, Yongji Xie wrote:
>> On 2016/4/6 22:45, Alex Williamson wrote:
>>> On Tue,  5 Apr 2016 21:46:44 +0800
>>> Yongji Xie <xyjxie@linux.vnet.ibm.com> wrote:
>>>
>>>> This patch enables mmapping MSI-X tables if
>>>> hardware supports interrupt remapping which
>>>> can ensure that a given pci device can only
>>>> shoot the MSIs assigned for it.
>>>>
>>>> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
>>>> ---
>>>>    drivers/vfio/pci/vfio_pci.c         |    9 +++++++--
>>>>    drivers/vfio/pci/vfio_pci_private.h |    1 +
>>>>    drivers/vfio/pci/vfio_pci_rdwr.c    |    2 +-
>>>>    3 files changed, 9 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
>>>> index c60d790..ef02896 100644
>>>> --- a/drivers/vfio/pci/vfio_pci.c
>>>> +++ b/drivers/vfio/pci/vfio_pci.c
>>>> @@ -201,6 +201,10 @@ static int vfio_pci_enable(struct
>>>> vfio_pci_device *vdev)
>>>>        } else
>>>>            vdev->msix_bar = 0xFF;
>>>>    +    if (iommu_capable(pdev->dev.bus, IOMMU_CAP_INTR_REMAP) ||
>>> This doesn't address the issue I raised earlier where ARM SMMU sets
>>> this capability, but doesn't really provide per vector isolation.  ARM
>>> either needs to be fixed or we need to consider the whole capability
>>> tainted for this application and standardize around the bus flags.
>>> It's not very desirable to have two different ways to test this anyway.
>> I saw Eric posted a patchset [1] which introduce a flag
>> MSI_FLAG_IRQ_REMAPPING to indicate the capability
>> for ARM SMMU. With this patchset applied, it would
>> be  workable to use bus_flags to test the capability
>> of ARM SMMU:
> My purpose was to remove the advertising of IOMMU_CAP_INTR_REMAP from
> arm-smmu.c, "fix" mentionned by Alex (by the way I also need to do the
> same in v3 code) and to advertise the functionality on MSI controller
> instead (since the IRQ REMAPPING functionality is abstracted in GICv3
> ITS MSI controller)

Thank you for your explanation.  Now we have three
flags to test this capability with your and my patches
applied.  We need to test something like
IOMMU_CAP_INTR_REMAP || MSI_FLAG_IRQ_REMAPPING ||
PCI_BUS_FLAGS_MSI_REMAP if we want to mmap
MSI-X table. It's not very desirable if I understood
Alex correctly. So I'm thinking whether we can make
bus_flags compatible with other two flags and only
test bus_flags here.

> On top of that, on ARM we have platform (non PCI) MSI controllers so my
> understanding is the capability advertising should be possible beyond
> the PCI bus?

Actually, we just need one flag which can standardize
the capability on PCI side. With this flag set, we can
easily know hardware supports the capability of
interrupt remapping and it's safe to mmap MSI-X
tables of PCI BARs in any userspace driver.

Of course, we can also achieve that by testing all the
three flags. But I'm not sure whether it is good enough.

Regards,
Yongji

> Best Regards
>
> Eric
>> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
>> index a080f44..b2d1756 100644
>> --- a/drivers/pci/msi.c
>> +++ b/drivers/pci/msi.c
>> @@ -1134,6 +1134,21 @@ void *msi_desc_to_pci_sysdata(struct msi_desc *desc)
>>   }
>>   EXPORT_SYMBOL_GPL(msi_desc_to_pci_sysdata);
>>
>> +void pci_check_msi_remapping(struct pci_bus *bus)
>> +{
>> +#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
>> +        struct irq_domain *domain;
>> +        struct msi_domain_info *info;
>> +
>> +        domain = dev_get_msi_domain(&bus->dev);
>> +        if (domain) {
>> +                info = msi_get_domain_info(domain);
>> +                if (info->flags & MSI_FLAG_IRQ_REMAPPING)
>> +                        pdev->bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP;
>> +        }
>> +#endif
>> +}
>> +
>>   #ifdef CONFIG_PCI_MSI_IRQ_DOMAIN
>>   /**
>>    * pci_msi_domain_write_msg - Helper to write MSI message to PCI config
>> space
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index 6d7ab9b..24e9606 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -2115,6 +2115,7 @@ struct pci_bus *pci_create_root_bus(struct device
>> *parent, int bus,
>>          device_enable_async_suspend(b->bridge);
>>          pci_set_bus_of_node(b);
>>          pci_set_bus_msi_domain(b);
>> +       pci_check_msi_remapping(b);
>>
>>          if (!parent)
>>                  set_dev_node(b->bridge, pcibus_to_node(b));
>> diff --git a/include/linux/msi.h b/include/linux/msi.h
>> index a2a0068..fe8ce7b 100644
>> --- a/include/linux/msi.h
>> +++ b/include/linux/msi.h
>> @@ -15,6 +15,7 @@ extern int pci_msi_ignore_mask;
>>   struct irq_data;
>>   struct msi_desc;
>>   struct pci_dev;
>> +struct pci_bus;
>>   struct platform_msi_priv_data;
>>   void __get_cached_msi_msg(struct msi_desc *entry, struct msi_msg *msg);
>>   void get_cached_msi_msg(unsigned int irq, struct msi_msg *msg);
>> @@ -155,6 +156,8 @@ void arch_restore_msi_irqs(struct pci_dev *dev);
>>   void default_teardown_msi_irqs(struct pci_dev *dev);
>>   void default_restore_msi_irqs(struct pci_dev *dev);
>>
>> +void pci_check_msi_remapping(struct pci_bus *bus);
>> +
>>   struct msi_controller {
>>          struct module *owner;
>>          struct device *dev;
>>
>> Next we just need to find a proper way to make
>> bus_flags compatible with IOMMU_CAP_INTR_REMAP, right?
>>
>> I think a good place to do that is add_iommu_group().
>> But I'm not sure whether iommu drivers must be
>> initialized after PCI enumeration.  Do you have any comment?
>>
>> [1] http://www.spinics.net/lists/kvm/msg130256.html
>>
>>>> +        pdev->bus->bus_flags | PCI_BUS_FLAGS_MSI_REMAP)
>>> Perhaps some sort of wrapper for testing these flags would help avoid
>>> this kind of coding error (| vs &)
>> Thank you.  I'll try not to make the same mistake again.
>>
>> Regards,
>> Yongji
>>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [RFC v5 7/7] vfio-pci: Allow to mmap MSI-X table if interrupt remapping is supported

[Eric Auger]

Hi Yongji,
On 04/08/2016 10:14 AM, Yongji Xie wrote:
> Hi Eric,
> On 2016/4/7 22:23, Eric Auger wrote:
>> Hi Yongji,
>> On 04/07/2016 01:38 PM, Yongji Xie wrote:
>>> On 2016/4/6 22:45, Alex Williamson wrote:
>>>> On Tue,  5 Apr 2016 21:46:44 +0800
>>>> Yongji Xie <xyjxie@linux.vnet.ibm.com> wrote:
>>>>
>>>>> This patch enables mmapping MSI-X tables if
>>>>> hardware supports interrupt remapping which
>>>>> can ensure that a given pci device can only
>>>>> shoot the MSIs assigned for it.
>>>>>
>>>>> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
>>>>> ---
>>>>>    drivers/vfio/pci/vfio_pci.c         |    9 +++++++--
>>>>>    drivers/vfio/pci/vfio_pci_private.h |    1 +
>>>>>    drivers/vfio/pci/vfio_pci_rdwr.c    |    2 +-
>>>>>    3 files changed, 9 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
>>>>> index c60d790..ef02896 100644
>>>>> --- a/drivers/vfio/pci/vfio_pci.c
>>>>> +++ b/drivers/vfio/pci/vfio_pci.c
>>>>> @@ -201,6 +201,10 @@ static int vfio_pci_enable(struct
>>>>> vfio_pci_device *vdev)
>>>>>        } else
>>>>>            vdev->msix_bar = 0xFF;
>>>>>    +    if (iommu_capable(pdev->dev.bus, IOMMU_CAP_INTR_REMAP) ||
>>>> This doesn't address the issue I raised earlier where ARM SMMU sets
>>>> this capability, but doesn't really provide per vector isolation.  ARM
>>>> either needs to be fixed or we need to consider the whole capability
>>>> tainted for this application and standardize around the bus flags.
>>>> It's not very desirable to have two different ways to test this anyway.
>>> I saw Eric posted a patchset [1] which introduce a flag
>>> MSI_FLAG_IRQ_REMAPPING to indicate the capability
>>> for ARM SMMU. With this patchset applied, it would
>>> be  workable to use bus_flags to test the capability
>>> of ARM SMMU:
>> My purpose was to remove the advertising of IOMMU_CAP_INTR_REMAP from
>> arm-smmu.c, "fix" mentionned by Alex (by the way I also need to do the
>> same in v3 code) and to advertise the functionality on MSI controller
>> instead (since the IRQ REMAPPING functionality is abstracted in GICv3
>> ITS MSI controller)
> 
> Thank you for your explanation.  Now we have three
> flags to test this capability with your and my patches
> applied.  We need to test something like
> IOMMU_CAP_INTR_REMAP || MSI_FLAG_IRQ_REMAPPING ||
> PCI_BUS_FLAGS_MSI_REMAP if we want to mmap
> MSI-X table. It's not very desirable if I understood
> Alex correctly. So I'm thinking whether we can make
> bus_flags compatible with other two flags and only
> test bus_flags here.
> 
>> On top of that, on ARM we have platform (non PCI) MSI controllers so my
>> understanding is the capability advertising should be possible beyond
>> the PCI bus?
> 
> Actually, we just need one flag which can standardize
> the capability on PCI side. With this flag set, we can
> easily know hardware supports the capability of
> interrupt remapping and it's safe to mmap MSI-X
> tables of PCI BARs in any userspace driver.
I agree with you on the fact storing the info at a single place looks
better. However my question was: if my understanding is correct, you
plan to store the info in pci_bus flags. What about platform_bus? Don't
we need to advertise the IRQ remapping capability also with a platform
bus topology? We can have platform devices writing to a platform MSI
controller that supports irq remapping. Assignment of such devices is
not considered yet though and maybe not feasible. I don't know if the
capability is used in other use cases.

Best Regards

Eric
> 
> Of course, we can also achieve that by testing all the
> three flags. But I'm not sure whether it is good enough.
> 
> Regards,
> Yongji
> 
>> Best Regards
>>
>> Eric
>>> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
>>> index a080f44..b2d1756 100644
>>> --- a/drivers/pci/msi.c
>>> +++ b/drivers/pci/msi.c
>>> @@ -1134,6 +1134,21 @@ void *msi_desc_to_pci_sysdata(struct msi_desc
>>> *desc)
>>>   }
>>>   EXPORT_SYMBOL_GPL(msi_desc_to_pci_sysdata);
>>>
>>> +void pci_check_msi_remapping(struct pci_bus *bus)
>>> +{
>>> +#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
>>> +        struct irq_domain *domain;
>>> +        struct msi_domain_info *info;
>>> +
>>> +        domain = dev_get_msi_domain(&bus->dev);
>>> +        if (domain) {
>>> +                info = msi_get_domain_info(domain);
>>> +                if (info->flags & MSI_FLAG_IRQ_REMAPPING)
>>> +                        pdev->bus->bus_flags |=
>>> PCI_BUS_FLAGS_MSI_REMAP;
>>> +        }
>>> +#endif
>>> +}
>>> +
>>>   #ifdef CONFIG_PCI_MSI_IRQ_DOMAIN
>>>   /**
>>>    * pci_msi_domain_write_msg - Helper to write MSI message to PCI
>>> config
>>> space
>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>>> index 6d7ab9b..24e9606 100644
>>> --- a/drivers/pci/probe.c
>>> +++ b/drivers/pci/probe.c
>>> @@ -2115,6 +2115,7 @@ struct pci_bus *pci_create_root_bus(struct device
>>> *parent, int bus,
>>>          device_enable_async_suspend(b->bridge);
>>>          pci_set_bus_of_node(b);
>>>          pci_set_bus_msi_domain(b);
>>> +       pci_check_msi_remapping(b);
>>>
>>>          if (!parent)
>>>                  set_dev_node(b->bridge, pcibus_to_node(b));
>>> diff --git a/include/linux/msi.h b/include/linux/msi.h
>>> index a2a0068..fe8ce7b 100644
>>> --- a/include/linux/msi.h
>>> +++ b/include/linux/msi.h
>>> @@ -15,6 +15,7 @@ extern int pci_msi_ignore_mask;
>>>   struct irq_data;
>>>   struct msi_desc;
>>>   struct pci_dev;
>>> +struct pci_bus;
>>>   struct platform_msi_priv_data;
>>>   void __get_cached_msi_msg(struct msi_desc *entry, struct msi_msg
>>> *msg);
>>>   void get_cached_msi_msg(unsigned int irq, struct msi_msg *msg);
>>> @@ -155,6 +156,8 @@ void arch_restore_msi_irqs(struct pci_dev *dev);
>>>   void default_teardown_msi_irqs(struct pci_dev *dev);
>>>   void default_restore_msi_irqs(struct pci_dev *dev);
>>>
>>> +void pci_check_msi_remapping(struct pci_bus *bus);
>>> +
>>>   struct msi_controller {
>>>          struct module *owner;
>>>          struct device *dev;
>>>
>>> Next we just need to find a proper way to make
>>> bus_flags compatible with IOMMU_CAP_INTR_REMAP, right?
>>>
>>> I think a good place to do that is add_iommu_group().
>>> But I'm not sure whether iommu drivers must be
>>> initialized after PCI enumeration.  Do you have any comment?
>>>
>>> [1] http://www.spinics.net/lists/kvm/msg130256.html
>>>
>>>>> +        pdev->bus->bus_flags | PCI_BUS_FLAGS_MSI_REMAP)
>>>> Perhaps some sort of wrapper for testing these flags would help avoid
>>>> this kind of coding error (| vs &)
>>> Thank you.  I'll try not to make the same mistake again.
>>>
>>> Regards,
>>> Yongji
>>>
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 

Re: [RFC v5 7/7] vfio-pci: Allow to mmap MSI-X table if interrupt remapping is supported

[Yongji Xie]

Hi Eric,
On 2016/4/8 17:10, Eric Auger wrote:
> Hi Yongji,
> On 04/08/2016 10:14 AM, Yongji Xie wrote:
>> Hi Eric,
>> On 2016/4/7 22:23, Eric Auger wrote:
>>> Hi Yongji,
>>> On 04/07/2016 01:38 PM, Yongji Xie wrote:
>>>> On 2016/4/6 22:45, Alex Williamson wrote:
>>>>> On Tue,  5 Apr 2016 21:46:44 +0800
>>>>> Yongji Xie <xyjxie@linux.vnet.ibm.com> wrote:
>>>>>
>>>>>> This patch enables mmapping MSI-X tables if
>>>>>> hardware supports interrupt remapping which
>>>>>> can ensure that a given pci device can only
>>>>>> shoot the MSIs assigned for it.
>>>>>>
>>>>>> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
>>>>>> ---
>>>>>>     drivers/vfio/pci/vfio_pci.c         |    9 +++++++--
>>>>>>     drivers/vfio/pci/vfio_pci_private.h |    1 +
>>>>>>     drivers/vfio/pci/vfio_pci_rdwr.c    |    2 +-
>>>>>>     3 files changed, 9 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
>>>>>> index c60d790..ef02896 100644
>>>>>> --- a/drivers/vfio/pci/vfio_pci.c
>>>>>> +++ b/drivers/vfio/pci/vfio_pci.c
>>>>>> @@ -201,6 +201,10 @@ static int vfio_pci_enable(struct
>>>>>> vfio_pci_device *vdev)
>>>>>>         } else
>>>>>>             vdev->msix_bar = 0xFF;
>>>>>>     +    if (iommu_capable(pdev->dev.bus, IOMMU_CAP_INTR_REMAP) ||
>>>>> This doesn't address the issue I raised earlier where ARM SMMU sets
>>>>> this capability, but doesn't really provide per vector isolation.  ARM
>>>>> either needs to be fixed or we need to consider the whole capability
>>>>> tainted for this application and standardize around the bus flags.
>>>>> It's not very desirable to have two different ways to test this anyway.
>>>> I saw Eric posted a patchset [1] which introduce a flag
>>>> MSI_FLAG_IRQ_REMAPPING to indicate the capability
>>>> for ARM SMMU. With this patchset applied, it would
>>>> be  workable to use bus_flags to test the capability
>>>> of ARM SMMU:
>>> My purpose was to remove the advertising of IOMMU_CAP_INTR_REMAP from
>>> arm-smmu.c, "fix" mentionned by Alex (by the way I also need to do the
>>> same in v3 code) and to advertise the functionality on MSI controller
>>> instead (since the IRQ REMAPPING functionality is abstracted in GICv3
>>> ITS MSI controller)
>> Thank you for your explanation.  Now we have three
>> flags to test this capability with your and my patches
>> applied.  We need to test something like
>> IOMMU_CAP_INTR_REMAP || MSI_FLAG_IRQ_REMAPPING ||
>> PCI_BUS_FLAGS_MSI_REMAP if we want to mmap
>> MSI-X table. It's not very desirable if I understood
>> Alex correctly. So I'm thinking whether we can make
>> bus_flags compatible with other two flags and only
>> test bus_flags here.
>>
>>> On top of that, on ARM we have platform (non PCI) MSI controllers so my
>>> understanding is the capability advertising should be possible beyond
>>> the PCI bus?
>> Actually, we just need one flag which can standardize
>> the capability on PCI side. With this flag set, we can
>> easily know hardware supports the capability of
>> interrupt remapping and it's safe to mmap MSI-X
>> tables of PCI BARs in any userspace driver.
> I agree with you on the fact storing the info at a single place looks
> better. However my question was: if my understanding is correct, you
> plan to store the info in pci_bus flags. What about platform_bus? Don't
> we need to advertise the IRQ remapping capability also with a platform
> bus topology? We can have platform devices writing to a platform MSI
> controller that supports irq remapping. Assignment of such devices is
> not considered yet though and maybe not feasible. I don't know if the
> capability is used in other use cases.

My purpose is to make bus_flags compatible with other
two flags so that we can only test bus_flags when mmapping
MSI-X table. We would not remove the flag
MSI_FLAG_IRQ_REMAPPING and IOMMU_CAP_INTR_REMAP.
So we still can test these two flags if we have platform
devices writing to a platform MSI controller.

Of course, it would be better to have a flag which can
advertise the IRQ remapping capability for both PCI
bus and platform bus.  But now I don't find a proper
way to achieve that...

Regards,
Yongji

> Best Regards
>
> Eric
>> Of course, we can also achieve that by testing all the
>> three flags. But I'm not sure whether it is good enough.
>>
>> Regards,
>> Yongji
>>
>>> Best Regards
>>>
>>> Eric
>>>> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
>>>> index a080f44..b2d1756 100644
>>>> --- a/drivers/pci/msi.c
>>>> +++ b/drivers/pci/msi.c
>>>> @@ -1134,6 +1134,21 @@ void *msi_desc_to_pci_sysdata(struct msi_desc
>>>> *desc)
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(msi_desc_to_pci_sysdata);
>>>>
>>>> +void pci_check_msi_remapping(struct pci_bus *bus)
>>>> +{
>>>> +#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
>>>> +        struct irq_domain *domain;
>>>> +        struct msi_domain_info *info;
>>>> +
>>>> +        domain = dev_get_msi_domain(&bus->dev);
>>>> +        if (domain) {
>>>> +                info = msi_get_domain_info(domain);
>>>> +                if (info->flags & MSI_FLAG_IRQ_REMAPPING)
>>>> +                        pdev->bus->bus_flags |=
>>>> PCI_BUS_FLAGS_MSI_REMAP;
>>>> +        }
>>>> +#endif
>>>> +}
>>>> +
>>>>    #ifdef CONFIG_PCI_MSI_IRQ_DOMAIN
>>>>    /**
>>>>     * pci_msi_domain_write_msg - Helper to write MSI message to PCI
>>>> config
>>>> space
>>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>>>> index 6d7ab9b..24e9606 100644
>>>> --- a/drivers/pci/probe.c
>>>> +++ b/drivers/pci/probe.c
>>>> @@ -2115,6 +2115,7 @@ struct pci_bus *pci_create_root_bus(struct device
>>>> *parent, int bus,
>>>>           device_enable_async_suspend(b->bridge);
>>>>           pci_set_bus_of_node(b);
>>>>           pci_set_bus_msi_domain(b);
>>>> +       pci_check_msi_remapping(b);
>>>>
>>>>           if (!parent)
>>>>                   set_dev_node(b->bridge, pcibus_to_node(b));
>>>> diff --git a/include/linux/msi.h b/include/linux/msi.h
>>>> index a2a0068..fe8ce7b 100644
>>>> --- a/include/linux/msi.h
>>>> +++ b/include/linux/msi.h
>>>> @@ -15,6 +15,7 @@ extern int pci_msi_ignore_mask;
>>>>    struct irq_data;
>>>>    struct msi_desc;
>>>>    struct pci_dev;
>>>> +struct pci_bus;
>>>>    struct platform_msi_priv_data;
>>>>    void __get_cached_msi_msg(struct msi_desc *entry, struct msi_msg
>>>> *msg);
>>>>    void get_cached_msi_msg(unsigned int irq, struct msi_msg *msg);
>>>> @@ -155,6 +156,8 @@ void arch_restore_msi_irqs(struct pci_dev *dev);
>>>>    void default_teardown_msi_irqs(struct pci_dev *dev);
>>>>    void default_restore_msi_irqs(struct pci_dev *dev);
>>>>
>>>> +void pci_check_msi_remapping(struct pci_bus *bus);
>>>> +
>>>>    struct msi_controller {
>>>>           struct module *owner;
>>>>           struct device *dev;
>>>>
>>>> Next we just need to find a proper way to make
>>>> bus_flags compatible with IOMMU_CAP_INTR_REMAP, right?
>>>>
>>>> I think a good place to do that is add_iommu_group().
>>>> But I'm not sure whether iommu drivers must be
>>>> initialized after PCI enumeration.  Do you have any comment?
>>>>
>>>> [1] http://www.spinics.net/lists/kvm/msg130256.html
>>>>
>>>>>> +        pdev->bus->bus_flags | PCI_BUS_FLAGS_MSI_REMAP)
>>>>> Perhaps some sort of wrapper for testing these flags would help avoid
>>>>> this kind of coding error (| vs &)
>>>> Thank you.  I'll try not to make the same mistake again.
>>>>
>>>> Regards,
>>>> Yongji
>>>>
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>