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@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
> > ---
> >   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