[PATCH 0/3] PCI/x86: Interface for testing multivector MSI support

[PATCH 0/3] PCI/x86: Interface for testing multivector MSI support

[Alex Williamson]

I'd like to make vfio-pci capable of manipulating the device exposed
to the user such that if the host can only support a single MSI
vector then we hide the fact that the device itself may actually be
able to support more.  When we virtualize PCI config space and
interrupt setup there's no PCI protocol for the device failing to
allocate the number of vectors that it said were available.  If the
userspace driver is a guest operating system, it certainly doesn't
expect this to fail.  I don't think we can ever guarantee that a
multi-vector request will succeed, but we can certainly guarantee
that it will fail if the platform doesn't support it.

An example device is the Atheros AR93xxx running in a Windows 7 VM.
Both the device and the guest OS support multiple MSI vectors.  With
interrupt remapping, such that the host supports multivector, the
device works well in the guest.  With interrupt remapping disabled,
the device is far less reliable because of the mismatch in MSI
programming vs driver configuration and often fails.  If vfio-pci
can test whether multiple vectors are supported, then we can make it
work reliably in both cases by adjusting the exposed MSI capability,
like in this patch that would follow this series:

https://github.com/awilliam/linux-vfio/commit/9ace67515680

With this series, only x86 w/ interrupt remapping will advertise
support for multiple MSI vectors.  In surveying the code, I couldn't
find any other archs that allowed it, but I'll take corrections if
that's untrue.  Thanks,

Alex

---

Alex Williamson (3):
      PCI: Extend and export pci_msi_supported() for multivector MSI
      PCI/x86: Add arch_supports_multivector_msi() hook
      PCI/MSI: Initial hook for archs to declare multivector MSI support


 arch/x86/include/asm/x86_init.h |    1 +
 arch/x86/kernel/x86_init.c      |    6 ++++++
 drivers/iommu/irq_remapping.c   |    6 ++++++
 drivers/pci/msi.c               |   20 +++++++++++++++++---
 include/linux/msi.h             |    1 +
 include/linux/pci.h             |    3 +++
 6 files changed, 34 insertions(+), 3 deletions(-)

[PATCH 1/3] PCI/MSI: Initial hook for archs to declare multivector MSI support

[Alex Williamson]

For the most part multivector MSI is not supported and drivers and
hardware wanting multiple vectors opt for MSI-X instead.  It seems
though that having the ability to query the arch/platform code to
determine whether allocating multiple MSI vectors will ever succeed
is a useful thing.  For instance, vfio-pci can use this to determine
whether to expose multiple MSI vectors to the user.  If we know we
cannot ever support more than one vector, we have a better shot at
the userspace driver working, especially if it's a guest OS, if we
only expose one vector as being available in the interface.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 drivers/pci/msi.c   |    5 +++++
 include/linux/msi.h |    1 +
 2 files changed, 6 insertions(+)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 9fab30a..36b503a 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -79,6 +79,11 @@ int __weak arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 	return 0;
 }
 
+bool __weak arch_supports_multivector_msi(struct pci_dev *dev)
+{
+	return false;
+}
+
 /*
  * We have a default implementation available as a separate non-weak
  * function, as it is used by the Xen x86 PCI code
diff --git a/include/linux/msi.h b/include/linux/msi.h
index 44f4746..2365c64 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -58,6 +58,7 @@ void arch_teardown_msi_irq(unsigned int irq);
 int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type);
 void arch_teardown_msi_irqs(struct pci_dev *dev);
 void arch_restore_msi_irqs(struct pci_dev *dev);
+bool arch_supports_multivector_msi(struct pci_dev *dev);
 
 void default_teardown_msi_irqs(struct pci_dev *dev);
 void default_restore_msi_irqs(struct pci_dev *dev);

Re: [PATCH 1/3] PCI/MSI: Initial hook for archs to declare multivector MSI support

[Thomas Gleixner]

On Fri, 21 Nov 2014, Alex Williamson wrote:
> For the most part multivector MSI is not supported and drivers and
> hardware wanting multiple vectors opt for MSI-X instead.  It seems
> though that having the ability to query the arch/platform code to
> determine whether allocating multiple MSI vectors will ever succeed
> is a useful thing.  For instance, vfio-pci can use this to determine
> whether to expose multiple MSI vectors to the user.  If we know we
> cannot ever support more than one vector, we have a better shot at
> the userspace driver working, especially if it's a guest OS, if we
> only expose one vector as being available in the interface.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> 
>  drivers/pci/msi.c   |    5 +++++
>  include/linux/msi.h |    1 +
>  2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 9fab30a..36b503a 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -79,6 +79,11 @@ int __weak arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
>  	return 0;
>  }
>  
> +bool __weak arch_supports_multivector_msi(struct pci_dev *dev)

Please not another weak arch function. We are in the process to reduce
them not to extend them.

arch_supports is pretty much wrong here anyway. We are moving away
from arch MSI dependencies simply because it's not a arch property per
se.

Multi MSI is a property of the underlying interrupt controllers and
there might be several MSI interrupt domains on a given system. They
can or cannot support multi MSI.

The current x86 implementation is a tangled maze and Jiang is in the
process to distangle it completely. Until thats done x86 is not going
to add new ad hoc interfaces.

Once we converted everything over to the new hierarchical irqdomains
we can add such an interface to the code, but for now I'm not
accepting anything like that into x86 msi related code.

Thanks,

	tglx

Re: [PATCH 1/3] PCI/MSI: Initial hook for archs to declare multivector MSI support

[Alex Williamson]

On Sun, 2014-11-23 at 21:20 +0100, Thomas Gleixner wrote:
> On Fri, 21 Nov 2014, Alex Williamson wrote:
> > For the most part multivector MSI is not supported and drivers and
> > hardware wanting multiple vectors opt for MSI-X instead.  It seems
> > though that having the ability to query the arch/platform code to
> > determine whether allocating multiple MSI vectors will ever succeed
> > is a useful thing.  For instance, vfio-pci can use this to determine
> > whether to expose multiple MSI vectors to the user.  If we know we
> > cannot ever support more than one vector, we have a better shot at
> > the userspace driver working, especially if it's a guest OS, if we
> > only expose one vector as being available in the interface.
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> > 
> >  drivers/pci/msi.c   |    5 +++++
> >  include/linux/msi.h |    1 +
> >  2 files changed, 6 insertions(+)
> > 
> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> > index 9fab30a..36b503a 100644
> > --- a/drivers/pci/msi.c
> > +++ b/drivers/pci/msi.c
> > @@ -79,6 +79,11 @@ int __weak arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
> >  	return 0;
> >  }
> >  
> > +bool __weak arch_supports_multivector_msi(struct pci_dev *dev)
> 
> Please not another weak arch function. We are in the process to reduce
> them not to extend them.
> 
> arch_supports is pretty much wrong here anyway. We are moving away
> from arch MSI dependencies simply because it's not a arch property per
> se.
> 
> Multi MSI is a property of the underlying interrupt controllers and
> there might be several MSI interrupt domains on a given system. They
> can or cannot support multi MSI.
> 
> The current x86 implementation is a tangled maze and Jiang is in the
> process to distangle it completely. Until thats done x86 is not going
> to add new ad hoc interfaces.
> 
> Once we converted everything over to the new hierarchical irqdomains
> we can add such an interface to the code, but for now I'm not
> accepting anything like that into x86 msi related code.

Ok, I guess I can do some ugliness with associating the IOMMU IRQ
remapping capability with multivector MSI support within an #ifdef x86
block.  Gross, but I think that's as accurate as I can get w/o a hook
through the MSI code.  Is there any target for the refactoring you
mention?  Thanks,

Alex

Re: [PATCH 1/3] PCI/MSI: Initial hook for archs to declare multivector MSI support

[Jiang Liu]

On 2014/11/25 5:45, Alex Williamson wrote:
> On Sun, 2014-11-23 at 21:20 +0100, Thomas Gleixner wrote:
>> On Fri, 21 Nov 2014, Alex Williamson wrote:
>>> For the most part multivector MSI is not supported and drivers and
>>> hardware wanting multiple vectors opt for MSI-X instead.  It seems
>>> though that having the ability to query the arch/platform code to
>>> determine whether allocating multiple MSI vectors will ever succeed
>>> is a useful thing.  For instance, vfio-pci can use this to determine
>>> whether to expose multiple MSI vectors to the user.  If we know we
>>> cannot ever support more than one vector, we have a better shot at
>>> the userspace driver working, especially if it's a guest OS, if we
>>> only expose one vector as being available in the interface.
>>>
>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>>> ---
>>>
>>>  drivers/pci/msi.c   |    5 +++++
>>>  include/linux/msi.h |    1 +
>>>  2 files changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
>>> index 9fab30a..36b503a 100644
>>> --- a/drivers/pci/msi.c
>>> +++ b/drivers/pci/msi.c
>>> @@ -79,6 +79,11 @@ int __weak arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
>>>  	return 0;
>>>  }
>>>  
>>> +bool __weak arch_supports_multivector_msi(struct pci_dev *dev)
>>
>> Please not another weak arch function. We are in the process to reduce
>> them not to extend them.
>>
>> arch_supports is pretty much wrong here anyway. We are moving away
>> from arch MSI dependencies simply because it's not a arch property per
>> se.
>>
>> Multi MSI is a property of the underlying interrupt controllers and
>> there might be several MSI interrupt domains on a given system. They
>> can or cannot support multi MSI.
>>
>> The current x86 implementation is a tangled maze and Jiang is in the
>> process to distangle it completely. Until thats done x86 is not going
>> to add new ad hoc interfaces.
>>
>> Once we converted everything over to the new hierarchical irqdomains
>> we can add such an interface to the code, but for now I'm not
>> accepting anything like that into x86 msi related code.
> 
> Ok, I guess I can do some ugliness with associating the IOMMU IRQ
> remapping capability with multivector MSI support within an #ifdef x86
> block.  Gross, but I think that's as accurate as I can get w/o a hook
> through the MSI code.  Is there any target for the refactoring you
> mention?  Thanks,
Hi Alex,
	The hierarchy irqdomain framework and some preparation work
for PCI MSI refactor will be merged into v3.19, there are already in
tip tree. If thing goes smoothly, we may finish PCI MSI refactoring
on x86 platform for the merging window after 3.19, not sure 3.20 or
4.0:)
Regards!
Gerry
> 
> Alex
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

[PATCH 2/3] PCI/x86: Add arch_supports_multivector_msi() hook

[Alex Williamson]

x86 does support multivector MSI, but only when using interrupt
remapping.  Extend x86_msi_ops to add an optional callback for
this and assume unsupported when not present.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 arch/x86/include/asm/x86_init.h |    1 +
 arch/x86/kernel/x86_init.c      |    6 ++++++
 drivers/iommu/irq_remapping.c   |    6 ++++++
 3 files changed, 13 insertions(+)

diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index e45e4da..28951c6 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -185,6 +185,7 @@ struct x86_msi_ops {
 	int  (*setup_hpet_msi)(unsigned int irq, unsigned int id);
 	u32 (*msi_mask_irq)(struct msi_desc *desc, u32 mask, u32 flag);
 	u32 (*msix_mask_irq)(struct msi_desc *desc, u32 flag);
+	bool (*supports_multivector)(struct pci_dev *dev);
 };
 
 struct IO_APIC_route_entry;
diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
index e48b674..d917410 100644
--- a/arch/x86/kernel/x86_init.c
+++ b/arch/x86/kernel/x86_init.c
@@ -148,6 +148,12 @@ u32 arch_msix_mask_irq(struct msi_desc *desc, u32 flag)
 {
 	return x86_msi.msix_mask_irq(desc, flag);
 }
+bool arch_supports_multivector_msi(struct pci_dev *dev)
+{
+	if (x86_msi.supports_multivector)
+		return x86_msi.supports_multivector(dev);
+	return false;
+}
 #endif
 
 struct x86_io_apic_ops x86_io_apic_ops = {
diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c
index 74a1767..4310bfb 100644
--- a/drivers/iommu/irq_remapping.c
+++ b/drivers/iommu/irq_remapping.c
@@ -160,6 +160,11 @@ static void eoi_ioapic_pin_remapped(int apic, int pin, int vector)
 	io_apic_eoi(apic, pin);
 }
 
+static bool remapped_supports_multivector(struct pci_dev *dev)
+{
+	return true;
+}
+
 static void __init irq_remapping_modify_x86_ops(void)
 {
 	x86_io_apic_ops.disable		= irq_remapping_disable_io_apic;
@@ -169,6 +174,7 @@ static void __init irq_remapping_modify_x86_ops(void)
 	x86_msi.setup_msi_irqs		= irq_remapping_setup_msi_irqs;
 	x86_msi.setup_hpet_msi		= setup_hpet_msi_remapped;
 	x86_msi.compose_msi_msg		= compose_remapped_msi_msg;
+	x86_msi.supports_multivector	= remapped_supports_multivector;
 }
 
 static __init int setup_nointremap(char *str)

[PATCH 3/3] PCI: Extend and export pci_msi_supported() for multivector MSI

[Alex Williamson]

Embrace and extend this existing function as an interface to test
for multivector MSI support.  Drivers can now call this function
with nvec >1 and type PCI_CAP_ID_MSI to verify whether the platform
is capable of supporting multiple MSI vectors.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 drivers/pci/msi.c   |   15 ++++++++++++---
 include/linux/pci.h |    3 +++
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 36b503a..da3c709 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -793,12 +793,13 @@ out_free:
  * pci_msi_supported - check whether MSI may be enabled on a device
  * @dev: pointer to the pci_dev data structure of MSI device function
  * @nvec: how many MSIs have been requested ?
+ * @type: PCI_CAP_ID_MSI or PCI_CAP_ID_MSIX
  *
  * Look at global flags, the device itself, and its parent buses
  * to determine if MSI/-X are supported for the device. If MSI/-X is
  * supported return 1, else return 0.
  **/
-static int pci_msi_supported(struct pci_dev *dev, int nvec)
+int pci_msi_supported(struct pci_dev *dev, int nvec, int type)
 {
 	struct pci_bus *bus;
 
@@ -809,6 +810,9 @@ static int pci_msi_supported(struct pci_dev *dev, int nvec)
 	if (!dev || dev->no_msi || dev->current_state != PCI_D0)
 		return 0;
 
+	if (type != PCI_CAP_ID_MSI && type != PCI_CAP_ID_MSIX)
+		return 0;
+
 	/*
 	 * You can't ask to have 0 or less MSIs configured.
 	 *  a) it's stupid ..
@@ -828,8 +832,13 @@ static int pci_msi_supported(struct pci_dev *dev, int nvec)
 		if (bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
 			return 0;
 
+	if (type == PCI_CAP_ID_MSI && nvec > 1 &&
+	    !arch_supports_multivector_msi(dev))
+		return 0;
+
 	return 1;
 }
+EXPORT_SYMBOL(pci_msi_supported);
 
 /**
  * pci_msi_vec_count - Return the number of MSI vectors a device can send
@@ -930,7 +939,7 @@ int pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int nvec)
 	int nr_entries;
 	int i, j;
 
-	if (!pci_msi_supported(dev, nvec))
+	if (!pci_msi_supported(dev, nvec, PCI_CAP_ID_MSIX))
 		return -EINVAL;
 
 	if (!entries)
@@ -1041,7 +1050,7 @@ int pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec)
 	int nvec;
 	int rc;
 
-	if (!pci_msi_supported(dev, minvec))
+	if (!pci_msi_supported(dev, minvec, PCI_CAP_ID_MSI))
 		return -EINVAL;
 
 	WARN_ON(!!dev->msi_enabled);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index c9a8904..f20605e 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1197,6 +1197,7 @@ struct msix_entry {
 
 
 #ifdef CONFIG_PCI_MSI
+int pci_msi_supported(struct pci_dev *dev, int nvec, int type);
 int pci_msi_vec_count(struct pci_dev *dev);
 void pci_msi_shutdown(struct pci_dev *dev);
 void pci_disable_msi(struct pci_dev *dev);
@@ -1225,6 +1226,8 @@ static inline int pci_enable_msix_exact(struct pci_dev *dev,
 	return 0;
 }
 #else
+static inline int pci_msi_supported(struct pci_dev *dev, int nvec, int type)
+{ return 0; }
 static inline int pci_msi_vec_count(struct pci_dev *dev) { return -ENOSYS; }
 static inline void pci_msi_shutdown(struct pci_dev *dev) { }
 static inline void pci_disable_msi(struct pci_dev *dev) { }

Re: [PATCH 0/3] PCI/x86: Interface for testing multivector MSI support

[Bjorn Helgaas]

On Fri, Nov 21, 2014 at 03:08:27PM -0700, Alex Williamson wrote:
> I'd like to make vfio-pci capable of manipulating the device exposed
> to the user such that if the host can only support a single MSI
> vector then we hide the fact that the device itself may actually be
> able to support more.  When we virtualize PCI config space and
> interrupt setup there's no PCI protocol for the device failing to
> allocate the number of vectors that it said were available.  If the
> userspace driver is a guest operating system, it certainly doesn't
> expect this to fail.  I don't think we can ever guarantee that a
> multi-vector request will succeed, but we can certainly guarantee
> that it will fail if the platform doesn't support it.
> 
> An example device is the Atheros AR93xxx running in a Windows 7 VM.
> Both the device and the guest OS support multiple MSI vectors.  With
> interrupt remapping, such that the host supports multivector, the
> device works well in the guest.  With interrupt remapping disabled,
> the device is far less reliable because of the mismatch in MSI
> programming vs driver configuration and often fails.  If vfio-pci
> can test whether multiple vectors are supported, then we can make it
> work reliably in both cases by adjusting the exposed MSI capability,
> like in this patch that would follow this series:
> 
> https://github.com/awilliam/linux-vfio/commit/9ace67515680
> 
> With this series, only x86 w/ interrupt remapping will advertise
> support for multiple MSI vectors.  In surveying the code, I couldn't
> find any other archs that allowed it, but I'll take corrections if
> that's untrue.  Thanks,

Per Thomas' comments and your possible workaround if we don't have
pci_msi_supported(), I'm going to ignore these for now.  Let me know if
you disagree.

Bjorn

> ---
> 
> Alex Williamson (3):
>       PCI: Extend and export pci_msi_supported() for multivector MSI
>       PCI/x86: Add arch_supports_multivector_msi() hook
>       PCI/MSI: Initial hook for archs to declare multivector MSI support
> 
> 
>  arch/x86/include/asm/x86_init.h |    1 +
>  arch/x86/kernel/x86_init.c      |    6 ++++++
>  drivers/iommu/irq_remapping.c   |    6 ++++++
>  drivers/pci/msi.c               |   20 +++++++++++++++++---
>  include/linux/msi.h             |    1 +
>  include/linux/pci.h             |    3 +++
>  6 files changed, 34 insertions(+), 3 deletions(-)

Re: [PATCH 0/3] PCI/x86: Interface for testing multivector MSI support

[Alex Williamson]

On Thu, 2015-01-08 at 09:15 -0700, Bjorn Helgaas wrote:
> On Fri, Nov 21, 2014 at 03:08:27PM -0700, Alex Williamson wrote:
> > I'd like to make vfio-pci capable of manipulating the device exposed
> > to the user such that if the host can only support a single MSI
> > vector then we hide the fact that the device itself may actually be
> > able to support more.  When we virtualize PCI config space and
> > interrupt setup there's no PCI protocol for the device failing to
> > allocate the number of vectors that it said were available.  If the
> > userspace driver is a guest operating system, it certainly doesn't
> > expect this to fail.  I don't think we can ever guarantee that a
> > multi-vector request will succeed, but we can certainly guarantee
> > that it will fail if the platform doesn't support it.
> > 
> > An example device is the Atheros AR93xxx running in a Windows 7 VM.
> > Both the device and the guest OS support multiple MSI vectors.  With
> > interrupt remapping, such that the host supports multivector, the
> > device works well in the guest.  With interrupt remapping disabled,
> > the device is far less reliable because of the mismatch in MSI
> > programming vs driver configuration and often fails.  If vfio-pci
> > can test whether multiple vectors are supported, then we can make it
> > work reliably in both cases by adjusting the exposed MSI capability,
> > like in this patch that would follow this series:
> > 
> > https://github.com/awilliam/linux-vfio/commit/9ace67515680
> > 
> > With this series, only x86 w/ interrupt remapping will advertise
> > support for multiple MSI vectors.  In surveying the code, I couldn't
> > find any other archs that allowed it, but I'll take corrections if
> > that's untrue.  Thanks,
> 
> Per Thomas' comments and your possible workaround if we don't have
> pci_msi_supported(), I'm going to ignore these for now.  Let me know if
> you disagree.

Yep, that's fine.  I'll either forget about this for a while or kludge
something in vfio to know that only x86 with interrupt remapping, which
I can test from the IOMMU API, has multivector MSI support.  Thanks,

Alex