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

From: Eric Auger <[email-masked]>
Date: Fri, 8 Apr 2016 11:10:38 +0200

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 <[email-masked]> 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 <[email-masked]>
>>>>> ---
>>>>>    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 [email-masked]
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>