* Re: AMD 8131 and MSI quirk
2006-02-14 16:52 ` Michael S. Tsirkin
@ 2006-02-14 16:56 ` Matthew Wilcox
2006-02-14 17:17 ` Roland Dreier
2006-02-14 17:19 ` Roland Dreier
` (3 subsequent siblings)
4 siblings, 1 reply; 23+ messages in thread
From: Matthew Wilcox @ 2006-02-14 16:56 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Roland Dreier, gregkh, linux-kernel, linux-pci
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;
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: AMD 8131 and MSI quirk
2006-02-14 16:56 ` Matthew Wilcox
@ 2006-02-14 17:17 ` Roland Dreier
0 siblings, 0 replies; 23+ messages in thread
From: Roland Dreier @ 2006-02-14 17:17 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Michael S. Tsirkin, Roland Dreier, gregkh, linux-kernel,
linux-pci
> 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.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: AMD 8131 and MSI quirk
2006-02-14 16:52 ` Michael S. Tsirkin
2006-02-14 16:56 ` Matthew Wilcox
@ 2006-02-14 17:19 ` Roland Dreier
2006-02-14 18:03 ` Kristen Accardi
` (2 subsequent siblings)
4 siblings, 0 replies; 23+ messages in thread
From: Roland Dreier @ 2006-02-14 17:19 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Roland Dreier, gregkh, linux-kernel, linux-pci
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.
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: AMD 8131 and MSI quirk
2006-02-14 16:52 ` Michael S. Tsirkin
2006-02-14 16:56 ` Matthew Wilcox
2006-02-14 17:19 ` Roland Dreier
@ 2006-02-14 18:03 ` Kristen Accardi
2006-02-14 21:21 ` Michael S. Tsirkin
2006-02-14 18:27 ` Roland Dreier
2006-02-17 0:09 ` Greg KH
4 siblings, 1 reply; 23+ messages in thread
From: Kristen Accardi @ 2006-02-14 18:03 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Roland Dreier, gregkh, linux-kernel, linux-pci
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 */
>
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: AMD 8131 and MSI quirk
2006-02-14 18:03 ` Kristen Accardi
@ 2006-02-14 21:21 ` Michael S. Tsirkin
2006-02-14 22:06 ` Kristen Accardi
0 siblings, 1 reply; 23+ messages in thread
From: Michael S. Tsirkin @ 2006-02-14 21:21 UTC (permalink / raw)
To: Kristen Accardi; +Cc: Roland Dreier, gregkh, linux-kernel, linux-pci
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: AMD 8131 and MSI quirk
2006-02-14 21:21 ` Michael S. Tsirkin
@ 2006-02-14 22:06 ` Kristen Accardi
2006-02-14 22:10 ` Michael S. Tsirkin
0 siblings, 1 reply; 23+ messages in thread
From: Kristen Accardi @ 2006-02-14 22:06 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Roland Dreier, gregkh, linux-kernel, linux-pci
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;
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: AMD 8131 and MSI quirk
2006-02-14 22:06 ` Kristen Accardi
@ 2006-02-14 22:10 ` Michael S. Tsirkin
2006-02-14 22:52 ` Kristen Accardi
0 siblings, 1 reply; 23+ messages in thread
From: Michael S. Tsirkin @ 2006-02-14 22:10 UTC (permalink / raw)
To: Kristen Accardi; +Cc: Roland Dreier, gregkh, linux-kernel, linux-pci
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: AMD 8131 and MSI quirk
2006-02-14 22:10 ` Michael S. Tsirkin
@ 2006-02-14 22:52 ` Kristen Accardi
0 siblings, 0 replies; 23+ messages in thread
From: Kristen Accardi @ 2006-02-14 22:52 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Roland Dreier, gregkh, linux-kernel, linux-pci
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: AMD 8131 and MSI quirk
2006-02-14 16:52 ` Michael S. Tsirkin
` (2 preceding siblings ...)
2006-02-14 18:03 ` Kristen Accardi
@ 2006-02-14 18:27 ` Roland Dreier
2006-02-14 20:03 ` Matthew Wilcox
2006-02-14 21:11 ` Michael S. Tsirkin
2006-02-17 0:09 ` Greg KH
4 siblings, 2 replies; 23+ messages in thread
From: Roland Dreier @ 2006-02-14 18:27 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Roland Dreier, gregkh, linux-kernel, linux-pci
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.
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: AMD 8131 and MSI quirk
2006-02-14 18:27 ` Roland Dreier
@ 2006-02-14 20:03 ` Matthew Wilcox
2006-02-14 21:24 ` Roland Dreier
2006-02-14 21:11 ` Michael S. Tsirkin
1 sibling, 1 reply; 23+ messages in thread
From: Matthew Wilcox @ 2006-02-14 20:03 UTC (permalink / raw)
To: Roland Dreier
Cc: Michael S. Tsirkin, Roland Dreier, gregkh, linux-kernel,
linux-pci
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;
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: AMD 8131 and MSI quirk
2006-02-14 20:03 ` Matthew Wilcox
@ 2006-02-14 21:24 ` Roland Dreier
0 siblings, 0 replies; 23+ messages in thread
From: Roland Dreier @ 2006-02-14 21:24 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Michael S. Tsirkin, Roland Dreier, gregkh, linux-kernel,
linux-pci
> 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.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: AMD 8131 and MSI quirk
2006-02-14 18:27 ` Roland Dreier
2006-02-14 20:03 ` Matthew Wilcox
@ 2006-02-14 21:11 ` Michael S. Tsirkin
2006-02-14 21:24 ` Roland Dreier
1 sibling, 1 reply; 23+ messages in thread
From: Michael S. Tsirkin @ 2006-02-14 21:11 UTC (permalink / raw)
To: Roland Dreier; +Cc: gregkh, linux-kernel, linux-pci
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: AMD 8131 and MSI quirk
2006-02-14 16:52 ` Michael S. Tsirkin
` (3 preceding siblings ...)
2006-02-14 18:27 ` Roland Dreier
@ 2006-02-17 0:09 ` Greg KH
2006-02-17 0:16 ` Roland Dreier
4 siblings, 1 reply; 23+ messages in thread
From: Greg KH @ 2006-02-17 0:09 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Roland Dreier, gregkh, linux-kernel, linux-pci
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
^ permalink raw reply [flat|nested] 23+ messages in thread