x86, pci: Add quirk for unsizeable Broadwell EP bar

PATCH In review Andi Kleen 3 participants 8 messages Jan 15, 2016 – Feb 11, 2016

Subsystems (2)

x86 PCI

lore

Share & tools

lore

Andi Kleen

author

2016-01-15 22:17 UTC #0 lore raw

PATCH 1/1 x86, pci: Add quirk for unsizeable Broadwell EP bar

From: Andi Kleen <[email protected]>

The Home Agent and PCU PCI devices in Broadwell-EP have a BAR that returns a non zero value when read, but is still not sizeable (because it doesn't exist). This causes several [Firmware error] messages at boot. It does not cause any functional problems, as the devices really have no BARs.

Add a PCI quirk to shut off the messages.

Since the message is printed before the normal header fixup add EARLY fixups. This requires changing the PCI probe code to not override the resource flags unconditionally, so that the quirk can set flags.

(I believe that's ok, they should be always zero before, but please double check)

Also don't print the invalid BAR message for FIXED BARs.

Signed-off-by Andi Kleen <[email protected]>

---
 arch/x86/pci/fixup.c | 12 ++++++++++++
 drivers/pci/probe.c  |  8 +++++---
 2 files changed, 17 insertions(+), 3 deletions(-)

arch/x86/pci/fixup.c +12 -0

index e585655..86bbdd6 100644
@@ -540,3 +540,15 @@ static void twinhead_reserve_killing_zone(struct pci_dev *dev)
         }
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x27B9, twinhead_reserve_killing_zone);
+
+/*
+ * Intel Broadwell EP. Prevent reading/updating BAR on Home Agent and PCU devices
+ * which are not real BARs, but still return non-null.
+ * This prevents a harmless warning message at boot.
+ */
+static void pci_bdwep_ha_bar(struct pci_dev *dev)
+{
+	dev->resource[0].flags |= IORESOURCE_PCI_FIXED;
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6fa0, pci_bdwep_ha_bar);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6fc0, pci_bdwep_ha_bar);

drivers/pci/probe.c +5 -3

index edb1984..f7926e8 100644
@@ -214,7 +214,7 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 		l = 0;
 
 	if (type == pci_bar_unknown) {
-		res->flags = decode_bar(dev, l);
+		res->flags |= decode_bar(dev, l);
 		res->flags |= IORESOURCE_SIZEALIGN;
 		if (res->flags & IORESOURCE_IO) {
 			l64 = l & PCI_BASE_ADDRESS_IO_MASK;

@@ -251,8 +251,10 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 
 	sz64 = pci_size(l64, sz64, mask64);
 	if (!sz64) {
-		dev_info(&dev->dev, FW_BUG "reg 0x%x: invalid BAR (can't size)\n",
-			 pos);
+		/* Don't print this message for a fixed BAR */
+		if (!(res->flags & IORESOURCE_PCI_FIXED))
+			dev_info(&dev->dev, FW_BUG "reg 0x%x: invalid BAR (can't size)\n",
+				 pos);
 		goto fail;
 	}

Signature · 1 line

2.4.3

Bjorn Helgaas

2016-02-04 17:42 UTC #1 lore raw

Hi Andi,

Quoted 6 lines · Andi Kleen

From: Andi Kleen <[email protected]>
The Home Agent and PCU PCI devices in Broadwell-EP have a BAR that returns a non zero value when read, but is still not sizeable (because it doesn't exist). This causes several [Firmware error] messages at boot. It does not cause any functional problems, as the devices really have no BARs.

Wow, this is a pretty egregious PCI spec violation. I'm sure there's some reason behind it, but it does irk me to have to put code in the core to work around such an obvious problem.

It sounds like these devices have some device-specific register where BAR 0 is supposed to be? Setting IORESOURCE_PCI_FIXED doesn't seem like the right solution to me. Even if we set that, the core still believes this resource corresponds to some address space consumed by the device. I think we will still try to size the BAR and decode its type. I think it will still show up via lspci. That's all meaningless.

How do you deal with this on Windows?

I think you need to replace the config accessor with a special one that knows that this register is not a BAR, and they can return zero. Or maybe the accessor should hide these devices completely, i.e., return 0xffffffff for the vendor/device ID. Or maybe you even have a switch the BIOS can use to hide them from the OS.

Bjorn

Quoted 66 lines

Add a PCI quirk to shut off the messages.

(I believe that's ok, they should be always zero before, but please double check)

Also don't print the invalid BAR message for FIXED BARs.

Signed-off-by Andi Kleen <[email protected]>

---
 arch/x86/pci/fixup.c | 12 ++++++++++++
 drivers/pci/probe.c  |  8 +++++---
 2 files changed, 17 insertions(+), 3 deletions(-)

arch/x86/pci/fixup.c +12 -0

index e585655..86bbdd6 100644
@@ -540,3 +540,15 @@ static void twinhead_reserve_killing_zone(struct pci_dev *dev)
         }
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x27B9, twinhead_reserve_killing_zone);
+
+/*
+ * Intel Broadwell EP. Prevent reading/updating BAR on Home Agent and PCU devices
+ * which are not real BARs, but still return non-null.
+ * This prevents a harmless warning message at boot.
+ */
+static void pci_bdwep_ha_bar(struct pci_dev *dev)
+{
+	dev->resource[0].flags |= IORESOURCE_PCI_FIXED;
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6fa0, pci_bdwep_ha_bar);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6fc0, pci_bdwep_ha_bar);

drivers/pci/probe.c +5 -3

index edb1984..f7926e8 100644
@@ -214,7 +214,7 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 		l = 0;
 
 	if (type == pci_bar_unknown) {
-		res->flags = decode_bar(dev, l);
+		res->flags |= decode_bar(dev, l);
 		res->flags |= IORESOURCE_SIZEALIGN;
 		if (res->flags & IORESOURCE_IO) {
 			l64 = l & PCI_BASE_ADDRESS_IO_MASK;

@@ -251,8 +251,10 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 
 	sz64 = pci_size(l64, sz64, mask64);
 	if (!sz64) {
-		dev_info(&dev->dev, FW_BUG "reg 0x%x: invalid BAR (can't size)\n",
-			 pos);
+		/* Don't print this message for a fixed BAR */
+		if (!(res->flags & IORESOURCE_PCI_FIXED))
+			dev_info(&dev->dev, FW_BUG "reg 0x%x: invalid BAR (can't size)\n",
+				 pos);
 		goto fail;
 	}

Signature · 1 line

2.4.3

Andi Kleen

2016-02-04 18:54 UTC #2 lore raw

Quoted 3 lines

It sounds like these devices have some device-specific register where BAR 0 is supposed to be? Setting IORESOURCE_PCI_FIXED doesn't seem like the right solution to me. Even if we set that, the core still

There is no actually functional register on these locations that has any side effects.

Quoted 4 lines

believes this resource corresponds to some address space consumed by the device. I think we will still try to size the BAR and decode its type. I think it will still show up via lspci. That's all meaningless.

But would actually anything use it?

Quoted 7 lines

How do you deal with this on Windows?
I think you need to replace the config accessor with a special one that knows that this register is not a BAR, and they can return zero. Or maybe the accessor should hide these devices completely, i.e., return 0xffffffff for the vendor/device ID. Or maybe you even have a switch the BIOS can use to hide them from the OS.

In some cases we need the devices.

-Andi

Bjorn Helgaas

2016-02-05 01:57 UTC #3 lore raw

Quoted 7 lines · Andi Kleen

Quoted 3 lines
It sounds like these devices have some device-specific register where BAR 0 is supposed to be? Setting IORESOURCE_PCI_FIXED doesn't seem like the right solution to me. Even if we set that, the core still
There is no actually functional register on these locations that has any side effects.

Quoted 6 lines

Quoted 4 lines
believes this resource corresponds to some address space consumed by the device. I think we will still try to size the BAR and decode its type. I think it will still show up via lspci. That's all meaningless.
But would actually anything use it?

You mean, would anything actually use the lspci output? I don't know, but why would we want it to print garbage?

And the kernel certainly uses the struct resource. Setting IORESOURCE_PCI_FIXED is not a way of saying "please ignore this resource."

Bjorn

Andi Kleen

author

2016-02-05 03:36 UTC #4 lore raw

Quoted 4 lines

Quoted 1 line
But would actually anything use it?
You mean, would anything actually use the lspci output? I don't know, but why would we want it to print garbage?

In he kernel. I don't think lspci is that interesting.

Quoted 4 lines

And the kernel certainly uses the struct resource. Setting IORESOURCE_PCI_FIXED is not a way of saying "please ignore this resource."

There is already another quirk that uses the same technique to handle a bad bar. I also didn't notice any bad side effects. Again what would it be used for?

I looked into the new device ops you asked for, but it is fairly complicated as the ops are not per device but per bus, and there can be many copies of this device, and the pci_dev is not passed, so it needs complicated pattern matching on the devfn.

Doing it like the existing quirk is much simpler, and seems to work just fine.

-Andi

Signature · 1 line

[email protected] -- Speaking for myself only.

Bjorn Helgaas

2016-02-05 16:34 UTC #5 lore raw

Quoted 7 lines · Andi Kleen

Quoted 4 lines
Quoted 1 line
But would actually anything use it?
You mean, would anything actually use the lspci output? I don't know, but why would we want it to print garbage?
In he kernel. I don't think lspci is that interesting.
Quoted 1 line

Quoted 7 lines

Quoted 3 lines
And the kernel certainly uses the struct resource. Setting IORESOURCE_PCI_FIXED is not a way of saying "please ignore this resource."
There is already another quirk that uses the same technique to handle a bad bar. I also didn't notice any bad side effects. Again what would it be used for?

I suppose you mean pci_siemens_interrupt_controller(), added by 73a74ed3a6f8 ("PCI: i386: fixup for Siemens Nixdorf AG FSC Multiprocessor Interrupt Controllers")?

Here are the problems I see:

- We still try to size the register as though it were a real BAR, which means we write 0xffffffff to it. This register isn't a BAR, so we have no idea what effect this will have on the device. In your case, you might know it's safe, but it's not safe in general

- We read from the register and interpret the low bits as a BAR type. We have no idea what data we'll get from a non-BAR register, so we're essentially putting junk in the struct resource flags.

- We try to figure out the size of the BAR. Depending on what data we get, we may compute some arbitrary size. You might know that we happen to compute a zero size or something else safe, but that depends on your device and is not safe in general. I don't want to have to worry about your device settings in that register being magically compatible with the pci_size() computation.

- I think it's possible that the data we read from this register would lead us to conclude this is a BAR that consumes memory space at some adddress. Or I/O space at some address. We rely on this information to perform PCI address space allocation.

Quoted 7 lines

I looked into the new device ops you asked for, but it is fairly complicated as the ops are not per device but per bus, and there can be many copies of this device, and the pci_dev is not passed, so it needs complicated pattern matching on the devfn.
Doing it like the existing quirk is much simpler, and seems to work just fine.

It's simple and seems to work in this case, but it doesn't lead to a consistent model of the system.

It may be a hassle to work around this via pci_ops, but, well, if you [Intel] want to take advantage of all the generic PCI code, you sort of have to play within the constraints of the generic PCI model.

Maybe there's some other way to do a quirk that doesn't involve using IORESOURCE_PCI_FIXED. I just object to the fact that we run so much code that thinks this is a real BAR, and we have no idea what data we're getting back, so we don't know what that struct resource will end up looking like. What I *want* is for that resource to remain zeroed out, just like any other resource corresponding to an unimplemented BAR.

Bjorn

Andi Kleen

author

2016-02-10 22:23 UTC #6 lore raw

Quoted 16 lines · Bjorn Helgaas

Quoted 7 lines · Andi Kleen
Quoted 4 lines
Quoted 1 line
But would actually anything use it?
You mean, would anything actually use the lspci output? I don't know, but why would we want it to print garbage?
In he kernel. I don't think lspci is that interesting.
Quoted 1 line
Quoted 7 lines
Quoted 3 lines
And the kernel certainly uses the struct resource. Setting IORESOURCE_PCI_FIXED is not a way of saying "please ignore this resource."
There is already another quirk that uses the same technique to handle a bad bar. I also didn't notice any bad side effects. Again what would it be used for?

Quoted 5 lines

I suppose you mean pci_siemens_interrupt_controller(), added by 73a74ed3a6f8 ("PCI: i386: fixup for Siemens Nixdorf AG FSC Multiprocessor Interrupt Controllers")?
Here are the problems I see:

All good points. I changed the patch to use a new flag IORESOURCE_PCI_IGNORE that skips the whole bar process (except for the BAR size, which are ok in my case at least).

Also fixed to handle all BARs.

Does that look better?

------
x86, pci: Add quirk for unsizeable Broadwell EP bar

Add a PCI quirk to shut off the messages.

(I believe that's ok, they should be always zero before, but please double check)

I used a new resource flag that causes the BAR to be ignored

v2: Handle all BARs, not just BAR0 (Chaohong Guo) Switch over to using IORESOURCE_PCI_IGNORE over FIXED

Signed-off-by Andi Kleen <[email protected]>

arch/x86/pci/fixup.c +15 -0

index e585655..837acb7 100644
@@ -540,3 +540,18 @@ static void twinhead_reserve_killing_zone(struct pci_dev *dev)
         }
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x27B9, twinhead_reserve_killing_zone);
+
+/*
+ * Intel Broadwell EP. Prevent reading/updating BARs on Home Agent and PCU devices
+ * which are not real BARs, but still return non-null.
+ * This prevents a harmless warning message at boot.
+ */
+static void pci_bdwep_ha_bar(struct pci_dev *dev)
+{
+	int i;
+
+	for (i = 0; i < 5; i++)
+		dev->resource[i].flags |= IORESOURCE_PCI_IGNORE;
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6fa0, pci_bdwep_ha_bar);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6fc0, pci_bdwep_ha_bar);

drivers/pci/probe.c +3 -1

index 6d7ab9b..7c12e6d 100644
@@ -214,7 +214,9 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 		l = 0;
 
 	if (type == pci_bar_unknown) {
-		res->flags = decode_bar(dev, l);
+		res->flags |= decode_bar(dev, l);
+		if (res->flags & IORESOURCE_PCI_IGNORE)
+			goto out;
 		res->flags |= IORESOURCE_SIZEALIGN;
 		if (res->flags & IORESOURCE_IO) {
 			l64 = l & PCI_BASE_ADDRESS_IO_MASK;

include/linux/ioport.h +1 -1

index 24bea08..5262102 100644
@@ -104,7 +104,7 @@ struct resource {
 
 /* PCI control bits.  Shares IORESOURCE_BITS with above PCI ROM.  */
 #define IORESOURCE_PCI_FIXED		(1<<4)	/* Do not move resource */
-
+#define IORESOURCE_PCI_IGNORE		(1<<5)	/* Ignore resource */
 
 /* helpers to define resources */
 #define DEFINE_RES_NAMED(_start, _size, _name, _flags)			\

Bjorn Helgaas

2016-02-11 14:20 UTC #7 lore raw

Quoted 82 lines · Andi Kleen

Quoted 16 lines · Bjorn Helgaas
Quoted 7 lines · Andi Kleen
Quoted 4 lines
Quoted 1 line
But would actually anything use it?
You mean, would anything actually use the lspci output? I don't know, but why would we want it to print garbage?
In he kernel. I don't think lspci is that interesting.
Quoted 1 line
Quoted 7 lines
Quoted 3 lines
And the kernel certainly uses the struct resource. Setting IORESOURCE_PCI_FIXED is not a way of saying "please ignore this resource."
There is already another quirk that uses the same technique to handle a bad bar. I also didn't notice any bad side effects. Again what would it be used for?
Quoted 5 lines
I suppose you mean pci_siemens_interrupt_controller(), added by 73a74ed3a6f8 ("PCI: i386: fixup for Siemens Nixdorf AG FSC Multiprocessor Interrupt Controllers")?
Here are the problems I see:
All good points. I changed the patch to use a new flag IORESOURCE_PCI_IGNORE that skips the whole bar process (except for the BAR size, which are ok in my case at least).
Also fixed to handle all BARs.
Does that look better?
------
x86, pci: Add quirk for unsizeable Broadwell EP bar
The Home Agent and PCU PCI devices in Broadwell-EP have a BAR that returns a non zero value when read, but is still not sizeable (because it doesn't exist). This causes several [Firmware error] messages at boot. It does not cause any functional problems, as the devices really have no BARs.
Add a PCI quirk to shut off the messages.
Since the message is printed before the normal header fixup add EARLY fixups. This requires changing the PCI probe code to not override the resource flags unconditionally, so that the quirk can set flags.
(I believe that's ok, they should be always zero before, but please double check)
I used a new resource flag that causes the BAR to be ignored
v2: Handle all BARs, not just BAR0 (Chaohong Guo) Switch over to using IORESOURCE_PCI_IGNORE over FIXED
Signed-off-by Andi Kleen <[email protected]>
arch/x86/pci/fixup.c +15 -0
index e585655..837acb7 100644
@@ -540,3 +540,18 @@ static void twinhead_reserve_killing_zone(struct pci_dev *dev)
         }
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x27B9, twinhead_reserve_killing_zone);
+
+/*
+ * Intel Broadwell EP. Prevent reading/updating BARs on Home Agent and PCU devices
+ * which are not real BARs, but still return non-null.
+ * This prevents a harmless warning message at boot.
+ */
+static void pci_bdwep_ha_bar(struct pci_dev *dev)
+{
+	int i;
+
+	for (i = 0; i < 5; i++)
+		dev->resource[i].flags |= IORESOURCE_PCI_IGNORE;
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6fa0, pci_bdwep_ha_bar);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6fc0, pci_bdwep_ha_bar);
drivers/pci/probe.c +0 -0
index 6d7ab9b..7c12e6d 100644

drivers/pci/probe.c int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type, · line 219

Quoted 7 lines

drivers/pci/probe.c +3 -1

@@ -214,7 +214,9 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 		l = 0;
 
 	if (type == pci_bar_unknown) {
-		res->flags = decode_bar(dev, l);
+		res->flags |= decode_bar(dev, l);
+		if (res->flags & IORESOURCE_PCI_IGNORE)
+			goto out;

Sorry, I really don't like this either. For one thing, if these registers are not BARs, I don't want to even read them. I don't want the possible side-effects from reading the register, and I don't want to worry about what happens when we feed garbage to decode_bar().

But more importantly, I don't want to define a whole new IORESOURCE_* flag just for this, and I don't want places that use resources to have to check for the new flag.

There's lots of code that assumes "res->flags == 0" means "this resource does not correspond to a BAR". For examples, look at the output of this:

git grep -e 'if (!r.*->flags'

These Broadwell-EP resources don't correspond to valid BARs, so they should look just like the resources we have for unimplemented BARs. I think the cleanest way to accomplish this is to make the registers look like unimplemented BARs, i.e., by making pci_read_config_dword() read zero.

I think it should be pretty easy to define your own pci_ops that special-cases these registers on these devices and falls back to the existing raw_pci_read() for everything else, and you could probably install those ops with an early quirk.

We already do something very similar for quirk_pcie_aspm_ops.

Quoted 16 lines

drivers/pci/probe.c +0 -0

 		res->flags |= IORESOURCE_SIZEALIGN;
 		if (res->flags & IORESOURCE_IO) {
 			l64 = l & PCI_BASE_ADDRESS_IO_MASK;

include/linux/ioport.h +1 -1

index 24bea08..5262102 100644
@@ -104,7 +104,7 @@ struct resource {
 
 /* PCI control bits.  Shares IORESOURCE_BITS with above PCI ROM.  */
 #define IORESOURCE_PCI_FIXED		(1<<4)	/* Do not move resource */
-
+#define IORESOURCE_PCI_IGNORE		(1<<5)	/* Ignore resource */
 
 /* helpers to define resources */
 #define DEFINE_RES_NAMED(_start, _size, _name, _flags)			\