diff mbox

x86, pci: Add quirk for unsizeable Broadwell EP bar

Message ID 1452896279-22034-1-git-send-email-andi@firstfloor.org (mailing list archive)
State New, archived
Headers show

Commit Message

Andi Kleen Jan. 15, 2016, 10:17 p.m. UTC
From: Andi Kleen <ak@linux.intel.com>

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 <ak@linux.intel.com>
---
 arch/x86/pci/fixup.c | 12 ++++++++++++
 drivers/pci/probe.c  |  8 +++++---
 2 files changed, 17 insertions(+), 3 deletions(-)

Comments

Bjorn Helgaas Feb. 4, 2016, 5:41 p.m. UTC | #1
Hi Andi,

On Fri, Jan 15, 2016 at 02:17:59PM -0800, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> 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

> 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 <ak@linux.intel.com>
> ---
>  arch/x86/pci/fixup.c | 12 ++++++++++++
>  drivers/pci/probe.c  |  8 +++++---
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
> index e585655..86bbdd6 100644
> --- a/arch/x86/pci/fixup.c
> +++ b/arch/x86/pci/fixup.c
> @@ -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);
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index edb1984..f7926e8 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -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;
>  	}
>  
> -- 
> 2.4.3
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andi Kleen Feb. 4, 2016, 6:54 p.m. UTC | #2
> 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.

> 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?

> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas Feb. 5, 2016, 1:57 a.m. UTC | #3
On Thu, Feb 04, 2016 at 10:54:42AM -0800, Andi Kleen wrote:
> > 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.
> 
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andi Kleen Feb. 5, 2016, 3:36 a.m. UTC | #4
> > 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.
> 
> 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
Bjorn Helgaas Feb. 5, 2016, 4:34 p.m. UTC | #5
On Fri, Feb 05, 2016 at 04:36:17AM +0100, Andi Kleen wrote:
> > > 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.
> > 
> > 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.

> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
index e585655..86bbdd6 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -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);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index edb1984..f7926e8 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -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;
 	}