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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <570646AB.8050406@linux.vnet.ibm.com>
Date:	Thu, 7 Apr 2016 19:38:19 +0800
From:	Yongji Xie <xyjxie@...ux.vnet.ibm.com>
To:	Alex Williamson <alex.williamson@...hat.com>
Cc:	kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-pci@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
	linux-doc@...r.kernel.org, bhelgaas@...gle.com, corbet@....net,
	aik@...abs.ru, benh@...nel.crashing.org, paulus@...ba.org,
	mpe@...erman.id.au, warrier@...ux.vnet.ibm.com,
	zhong@...ux.vnet.ibm.com, nikunj@...ux.vnet.ibm.com,
	eric.auger@...aro.org, will.deacon@....com,
	gwshan@...ux.vnet.ibm.com, alistair@...ple.id.au, ruscur@...sell.cc
Subject: Re: [RFC v5 7/7] vfio-pci: Allow to mmap MSI-X table if interrupt
 remapping is supported

On 2016/4/6 22:45, Alex Williamson wrote:
> On Tue,  5 Apr 2016 21:46:44 +0800
> Yongji Xie <xyjxie@...ux.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@...ux.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