AMD 8131 and MSI quirk

AMD 8131 and MSI quirk

[Roland Dreier]

The current quirk_amd_8131_ioapic() function sets a global
pci_msi_quirk flag, which disables MSI/MSI-X for all devices in the
system.  This is safe but suboptimal, because there may be devices on
other buses not related to the AMD 8131 bridge, for which MSI would
work fine.  As an example, see the end of this email for a lspci -t
from a real Opteron system that has PCI-X buses coming from an AMD
8131 and PCI Express buses coming from an Nforce4 bridge -- MSI works
fine for the Mellanox InfiniBand adapter on the PCIe bus, if we allow
it to be enabled.

I guess what we really should be doing is setting the dev->no_msi flag
for all devices below the AMD 8131 PCI-X bridge rather than turning
off MSI globally.  Of course this is somewhat tricky, since a device
could be hotplugged onto a bus below the AMD 8131.  Greg, any thoughts
about the proper way to use the driver model infrastructure to handle
this?

The other place that pci_msi_quirk is set is quirk_svw_msi().  I don't
know why MSI is turned off for that Serverworks chipset, but perhaps
the same change could be made, and the global pci_msi_quirk flag
killed off entirely.

Thanks,
  Roland

-+-[80]-+-00.0  nVidia Corporation CK804 Memory Controller
 |      +-01.0  nVidia Corporation CK804 Memory Controller
 |      +-0a.0  nVidia Corporation CK804 Ethernet Controller
 |      \-0e.0-[81]----00.0  Mellanox Technologies MT25208 InfiniHost III Ex (Tavor compatibility mode)
 +-[08]-+-0a.0-[09]--
 |      +-0a.1  Advanced Micro Devices [AMD] AMD-8131 PCI-X APIC
 |      +-0b.0-[0a]--+-04.0  Intel Corporation 82546EB Gigabit Ethernet Controller (Copper)
 |      |            +-04.1  Intel Corporation 82546EB Gigabit Ethernet Controller (Copper)
 |      |            \-09.0  Chelsio Communications Inc: Unknown device 000b
 |      \-0b.1  Advanced Micro Devices [AMD] AMD-8131 PCI-X APIC
 \-[00]-+-00.0  nVidia Corporation CK804 Memory Controller
        +-01.0  nVidia Corporation CK804 ISA Bridge
        +-01.1  nVidia Corporation CK804 SMBus
        +-02.0  nVidia Corporation CK804 USB Controller
        +-02.1  nVidia Corporation CK804 USB Controller
        +-04.0  nVidia Corporation CK804 AC'97 Audio Controller
        +-06.0  nVidia Corporation CK804 IDE
        +-07.0  nVidia Corporation CK804 Serial ATA Controller
        +-08.0  nVidia Corporation CK804 Serial ATA Controller
        +-09.0-[01]----05.0  Texas Instruments TSB43AB22/A IEEE-1394a-2000 Controller (PHY/Link)
        +-0a.0  nVidia Corporation CK804 Ethernet Controller
        +-0e.0-[02]--+-00.0  ATI Technologies Inc RV370 5B60 [Radeon X300 (PCIE)]
        |            \-00.1  ATI Technologies Inc: Unknown device 5b70
        +-18.0  Advanced Micro Devices [AMD] K8 [Athlon64/Opteron] HyperTransport Technology Configuration
        +-18.1  Advanced Micro Devices [AMD] K8 [Athlon64/Opteron] Address Map
        +-18.2  Advanced Micro Devices [AMD] K8 [Athlon64/Opteron] DRAM Controller
        +-18.3  Advanced Micro Devices [AMD] K8 [Athlon64/Opteron] Miscellaneous Control
        +-19.0  Advanced Micro Devices [AMD] K8 [Athlon64/Opteron] HyperTransport Technology Configuration
        +-19.1  Advanced Micro Devices [AMD] K8 [Athlon64/Opteron] Address Map
        +-19.2  Advanced Micro Devices [AMD] K8 [Athlon64/Opteron] DRAM Controller
        \-19.3  Advanced Micro Devices [AMD] K8 [Athlon64/Opteron] Miscellaneous Control

Re: AMD 8131 and MSI quirk

[Matthew Wilcox]

On Sat, Oct 22, 2005 at 03:14:34PM -0700, Roland Dreier wrote:
> The current quirk_amd_8131_ioapic() function sets a global
> pci_msi_quirk flag, which disables MSI/MSI-X for all devices in the
> system.  This is safe but suboptimal, because there may be devices on
> other buses not related to the AMD 8131 bridge, for which MSI would
> work fine.  As an example, see the end of this email for a lspci -t
> from a real Opteron system that has PCI-X buses coming from an AMD
> 8131 and PCI Express buses coming from an Nforce4 bridge -- MSI works
> fine for the Mellanox InfiniBand adapter on the PCIe bus, if we allow
> it to be enabled.
> 
> I guess what we really should be doing is setting the dev->no_msi flag
> for all devices below the AMD 8131 PCI-X bridge rather than turning
> off MSI globally.  Of course this is somewhat tricky, since a device
> could be hotplugged onto a bus below the AMD 8131.  Greg, any thoughts
> about the proper way to use the driver model infrastructure to handle
> this?

Perhaps the right thing to do is to change pad2 (in struct pci_bus) to
bus_flags and make bit 0 PCI_BRIDGE_FLAGS_NO_MSI ?

Re: AMD 8131 and MSI quirk

[Greg KH]

On Sat, Oct 22, 2005 at 05:32:20PM -0600, Matthew Wilcox wrote:
> On Sat, Oct 22, 2005 at 03:14:34PM -0700, Roland Dreier wrote:
> > The current quirk_amd_8131_ioapic() function sets a global
> > pci_msi_quirk flag, which disables MSI/MSI-X for all devices in the
> > system.  This is safe but suboptimal, because there may be devices on
> > other buses not related to the AMD 8131 bridge, for which MSI would
> > work fine.  As an example, see the end of this email for a lspci -t
> > from a real Opteron system that has PCI-X buses coming from an AMD
> > 8131 and PCI Express buses coming from an Nforce4 bridge -- MSI works
> > fine for the Mellanox InfiniBand adapter on the PCIe bus, if we allow
> > it to be enabled.
> > 
> > I guess what we really should be doing is setting the dev->no_msi flag
> > for all devices below the AMD 8131 PCI-X bridge rather than turning
> > off MSI globally.  Of course this is somewhat tricky, since a device
> > could be hotplugged onto a bus below the AMD 8131.  Greg, any thoughts
> > about the proper way to use the driver model infrastructure to handle
> > this?
> 
> Perhaps the right thing to do is to change pad2 (in struct pci_bus) to
> bus_flags and make bit 0 PCI_BRIDGE_FLAGS_NO_MSI ?

Yeah, I can't think of any way to use the device tree to do this, so
this sounds as good a way as any.

thanks,

greg k-h

Re: AMD 8131 and MSI quirk

[Ivan Kokshaysky]

On Wed, Oct 26, 2005 at 03:51:40PM -0700, Greg KH wrote:
> On Sat, Oct 22, 2005 at 05:32:20PM -0600, Matthew Wilcox wrote:
> > Perhaps the right thing to do is to change pad2 (in struct pci_bus) to
> > bus_flags and make bit 0 PCI_BRIDGE_FLAGS_NO_MSI ?

It would be better to make it a bit field as in struct pci_dev.

-	unsigned short pad2;
+	unsigned short no_msi:1;

> Yeah, I can't think of any way to use the device tree to do this, so
> this sounds as good a way as any.

Other pci_bus flags might be useful as well, for instance I thought
of bus->hotplug_bridge or something like that.

Ivan.

Re: AMD 8131 and MSI quirk

[Roland Dreier]

    Matthew> Perhaps the right thing to do is to change pad2 (in
    Matthew> struct pci_bus) to bus_flags and make bit 0
    Matthew> PCI_BRIDGE_FLAGS_NO_MSI ?

Seems reasonable, but I'm still not sure how to implement this.  Where
does this bit get set and propagated to secondary buses?

To give a somewhat pathological real-world example, Mellanox PCI-X
adapters have a PCI bridge in them; in other words, a single adapter
looks like:

	0000:03:01.0 PCI bridge: Mellanox Technologies MT23108 PCI Bridge (rev a1) (prog-if 00 [Normal decode])
		Flags: bus master, 66MHz, medium devsel, latency 64
		Bus: primary=03, secondary=04, subordinate=04, sec-latency=68
		Memory behind bridge: e8200000-e82fffff
		Prefetchable memory behind bridge: 00000000ea800000-00000000f7f00000
		Capabilities: [70] PCI-X bridge device.
	
	0000:04:00.0 InfiniBand: Mellanox Technologies MT23108 InfiniHost (rev a1)
		Subsystem: Mellanox Technologies MT23108 InfiniHost
		Flags: bus master, fast Back2Back, 66MHz, medium devsel, latency 64, IRQ 185
		Memory at e8200000 (64-bit, non-prefetchable) [size=1M]
		Memory at ea800000 (64-bit, prefetchable) [size=8M]
		Memory at f0000000 (64-bit, prefetchable) [size=128M]
		Capabilities: [40] #11 [001f]
		Capabilities: [50] Vital Product Data
		Capabilities: [60] Message Signalled Interrupts: 64bit+ Queue=0/5 Enable-
		Capabilities: [70] PCI-X non-bridge device.

That means the NO_MSI flag still needs to get propagated from the 8131
bridge to the Mellanox bridge, and that needs to cause no_msi to get
set on the actual device.

Also, if someone hot-plugged such an adapter into a bus below an AMD
8131 host bridge (I believe eg Sun V40Zs have hot-pluggable slots like
that), then the NO_MSI flag still needs to get propagated from the
8131 bridge to the Mellanox bridge and set no_msi on the final device.

Where in the PCI driver code is the right place to handle all this (I
hope by writing the code only once)?

 - R.

Re: AMD 8131 and MSI quirk

[Matthew Wilcox]

On Thu, Oct 27, 2005 at 08:08:45AM -0700, Roland Dreier wrote:
>     Matthew> Perhaps the right thing to do is to change pad2 (in
>     Matthew> struct pci_bus) to bus_flags and make bit 0
>     Matthew> PCI_BRIDGE_FLAGS_NO_MSI ?
> 
> Seems reasonable, but I'm still not sure how to implement this.  Where
> does this bit get set and propagated to secondary buses?

We can propagate it to secondary busses in pci_alloc_child_bus().
We inherit parent->ops and parent->sysdata at this point, we can also
inherit parent->whatever_flags_we_like.

Setting it from the quirk is a bit more yucky.  I *think* we're going
to have to walk the PCI tree, given that it's a FIXUP_FINAL.  Maybe it
needs to not be a FIXUP_FINAL ... a FIXUP_HEADER might get it set early
enough for it to propagate through that mechanism.

Re: AMD 8131 and MSI quirk

[Grant Grundler]

On Thu, Oct 27, 2005 at 08:08:45AM -0700, Roland Dreier wrote:
>     Matthew> Perhaps the right thing to do is to change pad2 (in
>     Matthew> struct pci_bus) to bus_flags and make bit 0
>     Matthew> PCI_BRIDGE_FLAGS_NO_MSI ?
> 
> Seems reasonable, but I'm still not sure how to implement this.  Where
> does this bit get set and propagated to secondary buses?

Does it have to be propagate to secondary busses?
Can't the MSI init code walk up the tree until it hits a root node?


> To give a somewhat pathological real-world example, Mellanox PCI-X
> adapters have a PCI bridge in them; in other words, a single adapter
> looks like:
...
> Also, if someone hot-plugged such an adapter into a bus below an AMD
> 8131 host bridge (I believe eg Sun V40Zs have hot-pluggable slots like
> that), then the NO_MSI flag still needs to get propagated from the
> 8131 bridge to the Mellanox bridge and set no_msi on the final device.
> 
> Where in the PCI driver code is the right place to handle all this (I
> hope by writing the code only once)?

I expect this could be contained in msi.c.
ie changes to msi_init(), pci_enable_msi(), msi_capability_init().

The flag would have to be set by whatever code claims the AMD 8131
chip.

hth,
grant

Re: AMD 8131 and MSI quirk

[Michael S. Tsirkin]

Quoting r. Roland Dreier <rolandd@cisco.com>:
> Subject: AMD 8131 and MSI quirk
> 
> The current quirk_amd_8131_ioapic() function sets a global
> pci_msi_quirk flag, which disables MSI/MSI-X for all devices in the
> system.  This is safe but suboptimal, because there may be devices on
> other buses not related to the AMD 8131 bridge, for which MSI would
> work fine.  As an example, see the end of this email for a lspci -t
> from a real Opteron system that has PCI-X buses coming from an AMD
> 8131 and PCI Express buses coming from an Nforce4 bridge -- MSI works
> fine for the Mellanox InfiniBand adapter on the PCIe bus, if we allow
> it to be enabled.

The following should do this IMO. Roland, could you test this patch please?

---

It turns out AMD 8131 quirk only affects MSI for devices behind the 8131 bridge.
Handle this by adding a flags field in pci_bus, inherited from parent to child.

Signed-off-by: Michael S. Tsirkin <mst@mellanox.co.il>

Index: linux-2.6.16-rc2/drivers/pci/msi.c
===================================================================
--- linux-2.6.16-rc2.orig/drivers/pci/msi.c	2006-02-14 17:09:23.000000000 +0200
+++ linux-2.6.16-rc2/drivers/pci/msi.c	2006-02-14 18:14:00.000000000 +0200
@@ -699,6 +699,9 @@ int pci_enable_msi(struct pci_dev* dev)
 	if (dev->no_msi)
 		return status;
 
+	if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
+		return -EINVAL;
+
 	temp = dev->irq;
 
 	if ((status = msi_init()) < 0)
@@ -924,6 +927,9 @@ int pci_enable_msix(struct pci_dev* dev,
 	if (!pci_msi_enable || !dev || !entries)
  		return -EINVAL;
 
+	if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
+		return -EINVAL;
+
 	if ((status = msi_init()) < 0)
 		return status;
 
Index: linux-2.6.16-rc2/drivers/pci/probe.c
===================================================================
--- linux-2.6.16-rc2.orig/drivers/pci/probe.c	2006-02-14 17:09:23.000000000 +0200
+++ linux-2.6.16-rc2/drivers/pci/probe.c	2006-02-14 17:58:17.000000000 +0200
@@ -347,6 +347,7 @@ pci_alloc_child_bus(struct pci_bus *pare
 	child->parent = parent;
 	child->ops = parent->ops;
 	child->sysdata = parent->sysdata;
+	child->bus_flags = parent->bus_flags;
 	child->bridge = get_device(&bridge->dev);
 
 	child->class_dev.class = &pcibus_class;
Index: linux-2.6.16-rc2/drivers/pci/quirks.c
===================================================================
--- linux-2.6.16-rc2.orig/drivers/pci/quirks.c	2006-02-14 17:09:23.000000000 +0200
+++ linux-2.6.16-rc2/drivers/pci/quirks.c	2006-02-14 17:58:17.000000000 +0200
@@ -575,8 +575,11 @@ static void __init quirk_amd_8131_ioapic
 { 
         unsigned char revid, tmp;
         
-	pci_msi_quirk = 1;
-	printk(KERN_WARNING "PCI: MSI quirk detected. pci_msi_quirk set.\n");
+	if (dev->subordinate) {
+		printk(KERN_WARNING "PCI: MSI quirk detected. "
+		       "PCI_BUS_FLAGS_NO_MSI set for subordinate bus.\n");
+		dev->subordinate->bus_flags |= PCI_BUS_FLAGS_NO_MSI;
+	}
 
         if (nr_ioapics == 0) 
                 return;
Index: linux-2.6.16-rc2/include/linux/pci.h
===================================================================
--- linux-2.6.16-rc2.orig/include/linux/pci.h	2006-02-14 17:09:24.000000000 +0200
+++ linux-2.6.16-rc2/include/linux/pci.h	2006-02-14 17:58:17.000000000 +0200
@@ -95,6 +95,11 @@ enum pci_channel_state {
 	pci_channel_io_perm_failure = (__force pci_channel_state_t) 3,
 };
 
+typedef unsigned short __bitwise pci_bus_flags_t;
+enum pci_bus_flags {
+	PCI_BUS_FLAGS_NO_MSI = (pci_bus_flags_t) 1,
+};
+
 /*
  * The pci_dev structure is used to describe PCI devices.
  */
@@ -203,7 +208,7 @@ struct pci_bus {
 	char		name[48];
 
 	unsigned short  bridge_ctl;	/* manage NO_ISA/FBB/et al behaviors */
-	unsigned short  pad2;
+	pci_bus_flags_t bus_flags;	/* Inherited by child busses */
 	struct device		*bridge;
 	struct class_device	class_dev;
 	struct bin_attribute	*legacy_io; /* legacy I/O for this bus */



-- 
Michael S. Tsirkin
Staff Engineer, Mellanox Technologies

Re: AMD 8131 and MSI quirk

[Matthew Wilcox]

On Tue, Feb 14, 2006 at 06:52:22PM +0200, Michael S. Tsirkin wrote:
> The following should do this IMO. Roland, could you test this patch please?

Going a bit overboard on the type safety.  Please, leave bus_flags as an
unsigned short so as not to bloat the pci_bus structure unnecessarily.

> +typedef unsigned short __bitwise pci_bus_flags_t;
> +enum pci_bus_flags {
> +	PCI_BUS_FLAGS_NO_MSI = (pci_bus_flags_t) 1,
> +};
> +
>  /*
>   * The pci_dev structure is used to describe PCI devices.
>   */
> @@ -203,7 +208,7 @@ struct pci_bus {
>  	char		name[48];
>  
>  	unsigned short  bridge_ctl;	/* manage NO_ISA/FBB/et al behaviors */
> -	unsigned short  pad2;
> +	pci_bus_flags_t bus_flags;	/* Inherited by child busses */
>  	struct device		*bridge;

Re: AMD 8131 and MSI quirk

[Roland Dreier]

 > Going a bit overboard on the type safety.  Please, leave bus_flags as an
 > unsigned short so as not to bloat the pci_bus structure unnecessarily.

Hmm:

> +typedef unsigned short __bitwise pci_bus_flags_t;

and:

> -	unsigned short  pad2;
> +	pci_bus_flags_t bus_flags;	/* Inherited by child busses */

This does make pci_bus_flags_t a short -- it just lets sparse catch
misuses of the enum values.

It's debatable whether it's worth the source obfuscation to let sparse
check this, since it seems rather unlikely that someone will screw it
up.  But I don't see how it makes any difference in the generated
code; certainly there's no bloat (beyond the extra source code)

 - R.

Re: AMD 8131 and MSI quirk

[Roland Dreier]

    Michael> The following should do this IMO. Roland, could you test
    Michael> this patch please?

I'll need to find a system with the required setup; unfortunately I
don't have one myself.

What's required is an Opteron motherboard with both an AMD 8131 PCI-X
bridge and a PCI Express slot (typically Nvidia Nforce4), and an
MSI-capable device (such as a Mellanox HCA) on the PCIe bus.  I know
that eg Tyan makes such a motherboard.  Does anyone reading this have
a setup like that?

 - R.

Re: AMD 8131 and MSI quirk

[Kristen Accardi]

On Tue, 2006-02-14 at 18:52 +0200, Michael S. Tsirkin wrote:
> Quoting r. Roland Dreier <rolandd@cisco.com>:
> > Subject: AMD 8131 and MSI quirk
> > 
> > The current quirk_amd_8131_ioapic() function sets a global
> > pci_msi_quirk flag, which disables MSI/MSI-X for all devices in the
> > system.  This is safe but suboptimal, because there may be devices on
> > other buses not related to the AMD 8131 bridge, for which MSI would
> > work fine.  As an example, see the end of this email for a lspci -t
> > from a real Opteron system that has PCI-X buses coming from an AMD
> > 8131 and PCI Express buses coming from an Nforce4 bridge -- MSI works
> > fine for the Mellanox InfiniBand adapter on the PCIe bus, if we allow
> > it to be enabled.
> 
> The following should do this IMO. Roland, could you test this patch please?
> 
> ---
> 
> It turns out AMD 8131 quirk only affects MSI for devices behind the 8131 bridge.
> Handle this by adding a flags field in pci_bus, inherited from parent to child.

It seems like we have a way to turn of msi already (the no_msi bit in
the pci_dev structure).  Does it make sense to just have the child bus
pci_dev structure inherit the no_msi bit from the parent's pci_dev
structure when doing an allocation, or does that unnecessarily remove
the msi capability for devices that may not need it?

Kristen


> 
> Signed-off-by: Michael S. Tsirkin <mst@mellanox.co.il>
> 
> Index: linux-2.6.16-rc2/drivers/pci/msi.c
> ===================================================================
> --- linux-2.6.16-rc2.orig/drivers/pci/msi.c	2006-02-14 17:09:23.000000000 +0200
> +++ linux-2.6.16-rc2/drivers/pci/msi.c	2006-02-14 18:14:00.000000000 +0200
> @@ -699,6 +699,9 @@ int pci_enable_msi(struct pci_dev* dev)
>  	if (dev->no_msi)
>  		return status;
>  
> +	if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
> +		return -EINVAL;
> +
>  	temp = dev->irq;
>  
>  	if ((status = msi_init()) < 0)
> @@ -924,6 +927,9 @@ int pci_enable_msix(struct pci_dev* dev,
>  	if (!pci_msi_enable || !dev || !entries)
>   		return -EINVAL;
>  
> +	if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
> +		return -EINVAL;
> +
>  	if ((status = msi_init()) < 0)
>  		return status;
>  
> Index: linux-2.6.16-rc2/drivers/pci/probe.c
> ===================================================================
> --- linux-2.6.16-rc2.orig/drivers/pci/probe.c	2006-02-14 17:09:23.000000000 +0200
> +++ linux-2.6.16-rc2/drivers/pci/probe.c	2006-02-14 17:58:17.000000000 +0200
> @@ -347,6 +347,7 @@ pci_alloc_child_bus(struct pci_bus *pare
>  	child->parent = parent;
>  	child->ops = parent->ops;
>  	child->sysdata = parent->sysdata;
> +	child->bus_flags = parent->bus_flags;
>  	child->bridge = get_device(&bridge->dev);
>  
>  	child->class_dev.class = &pcibus_class;
> Index: linux-2.6.16-rc2/drivers/pci/quirks.c
> ===================================================================
> --- linux-2.6.16-rc2.orig/drivers/pci/quirks.c	2006-02-14 17:09:23.000000000 +0200
> +++ linux-2.6.16-rc2/drivers/pci/quirks.c	2006-02-14 17:58:17.000000000 +0200
> @@ -575,8 +575,11 @@ static void __init quirk_amd_8131_ioapic
>  { 
>          unsigned char revid, tmp;
>          
> -	pci_msi_quirk = 1;
> -	printk(KERN_WARNING "PCI: MSI quirk detected. pci_msi_quirk set.\n");
> +	if (dev->subordinate) {
> +		printk(KERN_WARNING "PCI: MSI quirk detected. "
> +		       "PCI_BUS_FLAGS_NO_MSI set for subordinate bus.\n");
> +		dev->subordinate->bus_flags |= PCI_BUS_FLAGS_NO_MSI;
> +	}
>  
>          if (nr_ioapics == 0) 
>                  return;
> Index: linux-2.6.16-rc2/include/linux/pci.h
> ===================================================================
> --- linux-2.6.16-rc2.orig/include/linux/pci.h	2006-02-14 17:09:24.000000000 +0200
> +++ linux-2.6.16-rc2/include/linux/pci.h	2006-02-14 17:58:17.000000000 +0200
> @@ -95,6 +95,11 @@ enum pci_channel_state {
>  	pci_channel_io_perm_failure = (__force pci_channel_state_t) 3,
>  };
>  
> +typedef unsigned short __bitwise pci_bus_flags_t;
> +enum pci_bus_flags {
> +	PCI_BUS_FLAGS_NO_MSI = (pci_bus_flags_t) 1,
> +};
> +
>  /*
>   * The pci_dev structure is used to describe PCI devices.
>   */
> @@ -203,7 +208,7 @@ struct pci_bus {
>  	char		name[48];
>  
>  	unsigned short  bridge_ctl;	/* manage NO_ISA/FBB/et al behaviors */
> -	unsigned short  pad2;
> +	pci_bus_flags_t bus_flags;	/* Inherited by child busses */
>  	struct device		*bridge;
>  	struct class_device	class_dev;
>  	struct bin_attribute	*legacy_io; /* legacy I/O for this bus */
> 
> 
> 

Re: AMD 8131 and MSI quirk

[Roland Dreier]

Michael, now I'm not sure whether this will work for devices like the
Mellanox PCI-X HCA, where the HCA device sits below a virtual PCI
bridge.  In that case we need to propagate the NO_MSI flag from the
8131 bridge to the Tavor bridge, right?  And it has to work for
systems like Sun V40Z where the PCI-X slots are hot-swappable (so the
HCA and its bridge could be added later).

 - R.

Re: AMD 8131 and MSI quirk

[Matthew Wilcox]

On Tue, Feb 14, 2006 at 10:27:52AM -0800, Roland Dreier wrote:
> Michael, now I'm not sure whether this will work for devices like the
> Mellanox PCI-X HCA, where the HCA device sits below a virtual PCI
> bridge.  In that case we need to propagate the NO_MSI flag from the
> 8131 bridge to the Tavor bridge, right?  And it has to work for
> systems like Sun V40Z where the PCI-X slots are hot-swappable (so the
> HCA and its bridge could be added later).

Michael's patch does this:

@@ -347,6 +347,7 @@ pci_alloc_child_bus(struct pci_bus *pare
        child->parent = parent;
        child->ops = parent->ops;
        child->sysdata = parent->sysdata;
+       child->bus_flags = parent->bus_flags;
        child->bridge = get_device(&bridge->dev);

        child->class_dev.class = &pcibus_class;

Re: AMD 8131 and MSI quirk

[Michael S. Tsirkin]

Quoting r. Roland Dreier <rdreier@cisco.com>:
> Subject: Re: AMD 8131 and MSI quirk
> 
> Michael, now I'm not sure whether this will work for devices like the
> Mellanox PCI-X HCA, where the HCA device sits below a virtual PCI
> bridge.  In that case we need to propagate the NO_MSI flag from the
> 8131 bridge to the Tavor bridge, right?

Yes, I tested on that system.

> And it has to work for
> systems like Sun V40Z where the PCI-X slots are hot-swappable (so the
> HCA and its bridge could be added later).

I expect it will work on hot swappable systems too: bus_flags are inherited
from parent to child bus.

-- 
Michael S. Tsirkin
Staff Engineer, Mellanox Technologies

Re: AMD 8131 and MSI quirk

[Michael S. Tsirkin]

Quoting Kristen Accardi <kristen.c.accardi@intel.com>:
> > It turns out AMD 8131 quirk only affects MSI for devices behind the 8131
> > bridge.  Handle this by adding a flags field in pci_bus, inherited from
> > parent to child.
> 
> It seems like we have a way to turn of msi already (the no_msi bit in
> the pci_dev structure).  Does it make sense to just have the child bus
> pci_dev structure inherit the no_msi bit from the parent's pci_dev
> structure when doing an allocation, or does that unnecessarily remove
> the msi capability for devices that may not need it?
> 
> Kristen

This bit is already used to mean that msi is disabled for a specific device,
which appears to be a PCI Express to PCI bridge (PCI_DEVICE_ID_INTEL_PXH).  So
it seems that disabling MSI for child devices as well might break things (i.e.
disable msi unnecessarily).
Working for Intel, I guess you would know about the PCI_DEVICE_ID_INTEL_PXH
best: what do you say?

In my opinion, it is cleaner to separate the two concepts: suppress msi
for child devices versus suppress it for the specific device.

Right?

-- 
Michael S. Tsirkin
Staff Engineer, Mellanox Technologies

Re: AMD 8131 and MSI quirk

[Roland Dreier]

 > Michael's patch does this:
 > 
 > @@ -347,6 +347,7 @@ pci_alloc_child_bus(struct pci_bus *pare
 >         child->parent = parent;
 >         child->ops = parent->ops;
 >         child->sysdata = parent->sysdata;
 > +       child->bus_flags = parent->bus_flags;
 >         child->bridge = get_device(&bridge->dev);
 > 
 >         child->class_dev.class = &pcibus_class;

Sorry, I missed that.  Yes, that should work.

 - R.

Re: AMD 8131 and MSI quirk

[Roland Dreier]

    Michael> Yes, I tested on that system.

OK, sorry I missed the bits that made that work.

 - R.

Re: AMD 8131 and MSI quirk

[Kristen Accardi]

On Tue, 2006-02-14 at 23:21 +0200, Michael S. Tsirkin wrote:
> Quoting Kristen Accardi <kristen.c.accardi@intel.com>:
> > > It turns out AMD 8131 quirk only affects MSI for devices behind the 8131
> > > bridge.  Handle this by adding a flags field in pci_bus, inherited from
> > > parent to child.
> > 
> > It seems like we have a way to turn of msi already (the no_msi bit in
> > the pci_dev structure).  Does it make sense to just have the child bus
> > pci_dev structure inherit the no_msi bit from the parent's pci_dev
> > structure when doing an allocation, or does that unnecessarily remove
> > the msi capability for devices that may not need it?
> > 
> > Kristen
> 
> This bit is already used to mean that msi is disabled for a specific device,
> which appears to be a PCI Express to PCI bridge (PCI_DEVICE_ID_INTEL_PXH).  So
> it seems that disabling MSI for child devices as well might break things (i.e.
> disable msi unnecessarily).
> Working for Intel, I guess you would know about the PCI_DEVICE_ID_INTEL_PXH
> best: what do you say?
> 
> In my opinion, it is cleaner to separate the two concepts: suppress msi
> for child devices versus suppress it for the specific device.
> 
> Right?
> 

I was thinking something along these lines might work for you.  I think
it does the same thing as the other patch, without needing to add extra
flags to pci_bus.  I guess the assumption I made was that if msi is
turned off for a bridge, then all devices under the bridge may not use
msi.   

---
 drivers/pci/msi.c   |    2 +-
 drivers/pci/probe.c |    1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

--- linux-dock-mm.orig/drivers/pci/msi.c
+++ linux-dock-mm/drivers/pci/msi.c
@@ -701,7 +701,7 @@ int pci_enable_msi(struct pci_dev* dev)
 	if (!pci_msi_enable || !dev)
  		return status;
 
-	if (dev->no_msi)
+	if (dev->no_msi || dev->bus->self->no_msi)
 		return status;
 
 	temp = dev->irq;
--- linux-dock-mm.orig/drivers/pci/probe.c
+++ linux-dock-mm/drivers/pci/probe.c
@@ -344,6 +344,7 @@ pci_alloc_child_bus(struct pci_bus *pare
 		return NULL;
 
 	child->self = bridge;
+	child->self->no_msi = parent->self->no_msi;
 	child->parent = parent;
 	child->ops = parent->ops;
 	child->sysdata = parent->sysdata;

Re: AMD 8131 and MSI quirk

[Michael S. Tsirkin]

Quoting r. Kristen Accardi <kristen.c.accardi@intel.com>:
> I guess the assumption I made was that if msi is
> turned off for a bridge, then all devices under the bridge may not use
> msi.   

Well, all this is just quirks, so no real rules apply, thats why I thought
having 2 bits gives us maximum flexibility.

Specifically for PCXH I see this in code:
/*
 * It's possible for the MSI to get corrupted if shpc and acpi
 * are used together on certain PXH-based systems.
 * */

So it seems the issue is device-specific - only affects the bridge itself.

What the code currently does is disable msi for bridge itself but not for
the devices behind it. I cant inherit dev->no_msi from parent to child
without changing this, and I just assumed this is by design.

Are you saying this is a bug and should be changed?

-- 
Michael S. Tsirkin
Staff Engineer, Mellanox Technologies

Re: AMD 8131 and MSI quirk

[Kristen Accardi]

On Wed, 2006-02-15 at 00:10 +0200, Michael S. Tsirkin wrote:
> Quoting r. Kristen Accardi <kristen.c.accardi@intel.com>:
> > I guess the assumption I made was that if msi is
> > turned off for a bridge, then all devices under the bridge may not use
> > msi.   
> 
> Well, all this is just quirks, so no real rules apply, thats why I thought
> having 2 bits gives us maximum flexibility.
> 
> Specifically for PCXH I see this in code:
> /*
>  * It's possible for the MSI to get corrupted if shpc and acpi
>  * are used together on certain PXH-based systems.
>  * */
> 
> So it seems the issue is device-specific - only affects the bridge itself.
> 
> What the code currently does is disable msi for bridge itself but not for
> the devices behind it. I cant inherit dev->no_msi from parent to child
> without changing this, and I just assumed this is by design.
> 
> Are you saying this is a bug and should be changed?
> 

Ok, I went back and read the errata that this patch was attempting to
solve, and you are right, it would not be correct to have children
inherit the no_msi flag from the parent in this case.  I can clearly see
why we need to have a flag associated with the bus rather than the
device for your case.

Kristen

Re: AMD 8131 and MSI quirk

[Greg KH]

On Tue, Feb 14, 2006 at 06:52:22PM +0200, Michael S. Tsirkin wrote:
> Quoting r. Roland Dreier <rolandd@cisco.com>:
> > Subject: AMD 8131 and MSI quirk
> > 
> > The current quirk_amd_8131_ioapic() function sets a global
> > pci_msi_quirk flag, which disables MSI/MSI-X for all devices in the
> > system.  This is safe but suboptimal, because there may be devices on
> > other buses not related to the AMD 8131 bridge, for which MSI would
> > work fine.  As an example, see the end of this email for a lspci -t
> > from a real Opteron system that has PCI-X buses coming from an AMD
> > 8131 and PCI Express buses coming from an Nforce4 bridge -- MSI works
> > fine for the Mellanox InfiniBand adapter on the PCIe bus, if we allow
> > it to be enabled.
> 
> The following should do this IMO. Roland, could you test this patch please?
> 
> ---
> 
> It turns out AMD 8131 quirk only affects MSI for devices behind the 8131 bridge.
> Handle this by adding a flags field in pci_bus, inherited from parent to child.
> 
> Signed-off-by: Michael S. Tsirkin <mst@mellanox.co.il>

Roland, did you ever verify that this patch worked or not for you?

thanks,

greg k-h

Re: AMD 8131 and MSI quirk

[Roland Dreier]

    Greg> Roland, did you ever verify that this patch worked or not for you?

Unfortunately as I said I don't have such a system (with AMD 8131 and
also a host bus controller that _does_ support MSI) myself.

Michael has tested on systems with AMD 8131, so I don't think the
patch could hurt.

 - R.