diff mbox

[3/4] x86/PCI: Enable a 64bit BAR on AMD Family 15h (Models 30h-3fh) Processors

Message ID 1489408896-25039-4-git-send-email-deathsimple@vodafone.de (mailing list archive)
State New, archived
Headers show

Commit Message

Christian König March 13, 2017, 12:41 p.m. UTC
From: Christian König <christian.koenig@amd.com>

Most BIOS don't enable this because of compatibility reasons.

Manually enable a 64bit BAR of 64GB size so that we have
enough room for PCI devices.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 arch/x86/pci/fixup.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

Comments

Andy Shevchenko March 13, 2017, 4:49 p.m. UTC | #1
On Mon, Mar 13, 2017 at 2:41 PM, Christian König
<deathsimple@vodafone.de> wrote:

> Most BIOS don't enable this because of compatibility reasons.
>
> Manually enable a 64bit BAR of 64GB size so that we have
> enough room for PCI devices.

> +static void pci_amd_enable_64bit_bar(struct pci_dev *dev)
> +{
> +       const uint64_t size = 64ULL * 1024 * 1024 * 1024;

Perhaps extend <linux/sizes.h> and use SZ_64G here?

It would be nice to do, since some of the drivers already are using
sizes like 4GB and alike.

> +       uint32_t base, limit, high;
> +       struct resource *res;
> +       unsigned i;
> +       int r;
> +

> +       for (i = 0; i < 8; ++i) {

> +

Redundant empty line.

> +               pci_read_config_dword(dev, 0x80 + i * 0x8, &base);
> +               pci_read_config_dword(dev, 0x180 + i * 0x4, &high);
> +
> +               /* Is this slot free? */
> +               if ((base & 0x3) == 0x0)
> +                       break;
> +
> +               base >>= 8;
> +               base |= high << 24;
> +
> +               /* Abort if a slot already configures a 64bit BAR. */
> +               if (base > 0x10000)
> +                       return;

> +

Ditto.

> +       }

> +

Ditto.

> +       if (i == 8)
> +               return;
> +
> +       res = kzalloc(sizeof(*res), GFP_KERNEL);
> +       res->flags = IORESOURCE_MEM | IORESOURCE_PREFETCH | IORESOURCE_MEM_64 |
> +               IORESOURCE_WINDOW;
> +       res->name = dev->bus->name;
> +       r = allocate_resource(&iomem_resource, res, size, 0x100000000,
> +                             0xfd00000000, size, NULL, NULL);
> +       if (r) {
> +               kfree(res);
> +               return;
> +       }
> +
> +       base = ((res->start >> 8) & 0xffffff00) | 0x3;
> +       limit = ((res->end + 1) >> 8) & 0xffffff00;
> +       high = ((res->start >> 40) & 0xff) |
> +               ((((res->end + 1) >> 40) & 0xff) << 16);

Perhaps some of constants can be replaced by defines (I think some of
them are already defined in ioport.h or somewhere else).
kernel test robot March 14, 2017, 9:25 a.m. UTC | #2
Hi Christian,

[auto build test WARNING on pci/next]
[also build test WARNING on v4.11-rc2 next-20170310]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Christian-K-nig/PCI-add-resizeable-BAR-infrastructure-v3/20170314-163334
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: i386-randconfig-s0-201711 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   arch/x86/pci/fixup.c: In function 'pci_amd_enable_64bit_bar':
>> arch/x86/pci/fixup.c:608:52: warning: large integer implicitly truncated to unsigned type [-Woverflow]
     r = allocate_resource(&iomem_resource, res, size, 0x100000000,
                                                       ^~~~~~~~~~~
   arch/x86/pci/fixup.c:609:10: warning: large integer implicitly truncated to unsigned type [-Woverflow]
             0xfd00000000, size, NULL, NULL);
             ^~~~~~~~~~~~
>> arch/x86/pci/fixup.c:617:22: warning: right shift count >= width of type [-Wshift-count-overflow]
     high = ((res->start >> 40) & 0xff) |
                         ^~
   arch/x86/pci/fixup.c:618:21: warning: right shift count >= width of type [-Wshift-count-overflow]
      ((((res->end + 1) >> 40) & 0xff) << 16);
                        ^~

vim +608 arch/x86/pci/fixup.c

   602			return;
   603	
   604		res = kzalloc(sizeof(*res), GFP_KERNEL);
   605		res->flags = IORESOURCE_MEM | IORESOURCE_PREFETCH | IORESOURCE_MEM_64 |
   606			IORESOURCE_WINDOW;
   607		res->name = dev->bus->name;
 > 608		r = allocate_resource(&iomem_resource, res, size, 0x100000000,
   609				      0xfd00000000, size, NULL, NULL);
   610		if (r) {
   611			kfree(res);
   612			return;
   613		}
   614	
   615		base = ((res->start >> 8) & 0xffffff00) | 0x3;
   616		limit = ((res->end + 1) >> 8) & 0xffffff00;
 > 617		high = ((res->start >> 40) & 0xff) |
   618			((((res->end + 1) >> 40) & 0xff) << 16);
   619	
   620		pci_write_config_dword(dev, 0x180 + i * 0x4, high);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Bjorn Helgaas March 24, 2017, 3:47 p.m. UTC | #3
On Mon, Mar 13, 2017 at 01:41:35PM +0100, Christian König wrote:
> From: Christian König <christian.koenig@amd.com>
> 
> Most BIOS don't enable this because of compatibility reasons.

Can you give any more details here?  Without more hints, it's hard to
know whether any of the compatibility reasons might apply to Linux as
well.

> Manually enable a 64bit BAR of 64GB size so that we have
> enough room for PCI devices.

From the context, I'm guessing this is enabling a new 64GB window
through the PCI host bridge?  That might be documented as a "BAR", but
it's not anything the Linux PCI core would recognize as a BAR.

I think the specs would envision this being done via an ACPI _SRS
method on the PNP0A03 host bridge device.  That would be a more
generic path that would work on any host bridge.  Did you explore that
possibility?  I would prefer to avoid adding device-specific code if
that's possible.

> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  arch/x86/pci/fixup.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
> index 6d52b94..bff5242 100644
> --- a/arch/x86/pci/fixup.c
> +++ b/arch/x86/pci/fixup.c
> @@ -571,3 +571,56 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2fc0, pci_invalid_bar);
>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6f60, pci_invalid_bar);
>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6fa0, pci_invalid_bar);
>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6fc0, pci_invalid_bar);
> +
> +static void pci_amd_enable_64bit_bar(struct pci_dev *dev)
> +{
> +	const uint64_t size = 64ULL * 1024 * 1024 * 1024;
> +	uint32_t base, limit, high;
> +	struct resource *res;
> +	unsigned i;
> +	int r;
> +
> +	for (i = 0; i < 8; ++i) {
> +
> +		pci_read_config_dword(dev, 0x80 + i * 0x8, &base);
> +		pci_read_config_dword(dev, 0x180 + i * 0x4, &high);
> +
> +		/* Is this slot free? */
> +		if ((base & 0x3) == 0x0)
> +			break;
> +
> +		base >>= 8;
> +		base |= high << 24;
> +
> +		/* Abort if a slot already configures a 64bit BAR. */
> +		if (base > 0x10000)
> +			return;
> +
> +	}
> +
> +	if (i == 8)
> +		return;
> +
> +	res = kzalloc(sizeof(*res), GFP_KERNEL);
> +	res->flags = IORESOURCE_MEM | IORESOURCE_PREFETCH | IORESOURCE_MEM_64 |
> +		IORESOURCE_WINDOW;
> +	res->name = dev->bus->name;
> +	r = allocate_resource(&iomem_resource, res, size, 0x100000000,
> +			      0xfd00000000, size, NULL, NULL);
> +	if (r) {
> +		kfree(res);
> +		return;
> +	}
> +
> +	base = ((res->start >> 8) & 0xffffff00) | 0x3;
> +	limit = ((res->end + 1) >> 8) & 0xffffff00;
> +	high = ((res->start >> 40) & 0xff) |
> +		((((res->end + 1) >> 40) & 0xff) << 16);
> +
> +	pci_write_config_dword(dev, 0x180 + i * 0x4, high);
> +	pci_write_config_dword(dev, 0x84 + i * 0x8, limit);
> +	pci_write_config_dword(dev, 0x80 + i * 0x8, base);
> +
> +	pci_bus_add_resource(dev->bus, res, 0);

We would need some sort of printk here to explain how this new window
magically appeared.

> +}
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_AMD, 0x141b, pci_amd_enable_64bit_bar);
> -- 
> 2.7.4
>
Christian König April 11, 2017, 9:21 a.m. UTC | #4
Am 13.03.2017 um 17:49 schrieb Andy Shevchenko:
> On Mon, Mar 13, 2017 at 2:41 PM, Christian König
> <deathsimple@vodafone.de> wrote:
>
>> Most BIOS don't enable this because of compatibility reasons.
>>
>> Manually enable a 64bit BAR of 64GB size so that we have
>> enough room for PCI devices.
>> +static void pci_amd_enable_64bit_bar(struct pci_dev *dev)
>> +{
>> +       const uint64_t size = 64ULL * 1024 * 1024 * 1024;
> Perhaps extend <linux/sizes.h> and use SZ_64G here?
>
> It would be nice to do, since some of the drivers already are using
> sizes like 4GB and alike.

Actually using 64GB here was just for testing and to get some initial 
feedback.

I think we want to use all the remaining address space for PCIe, but for 
this we would need a new function in the resource management I think.

Going to take a deeper look when I'm sure we actually want this.

>> +       if (i == 8)
>> +               return;
>> +
>> +       res = kzalloc(sizeof(*res), GFP_KERNEL);
>> +       res->flags = IORESOURCE_MEM | IORESOURCE_PREFETCH | IORESOURCE_MEM_64 |
>> +               IORESOURCE_WINDOW;
>> +       res->name = dev->bus->name;
>> +       r = allocate_resource(&iomem_resource, res, size, 0x100000000,
>> +                             0xfd00000000, size, NULL, NULL);
>> +       if (r) {
>> +               kfree(res);
>> +               return;
>> +       }
>> +
>> +       base = ((res->start >> 8) & 0xffffff00) | 0x3;
>> +       limit = ((res->end + 1) >> 8) & 0xffffff00;
>> +       high = ((res->start >> 40) & 0xff) |
>> +               ((((res->end + 1) >> 40) & 0xff) << 16);
> Perhaps some of constants can be replaced by defines (I think some of
> them are already defined in ioport.h or somewhere else).

Yeah, good idea. But that stuff is purely AMD CPU specific, so won't 
belong into ioport.h or similar common code.

Does anybody have any idea where I could put this?

Regards,

Christian.
Christian König April 11, 2017, 3:48 p.m. UTC | #5
Am 24.03.2017 um 16:47 schrieb Bjorn Helgaas:
> On Mon, Mar 13, 2017 at 01:41:35PM +0100, Christian König wrote:
>> From: Christian König <christian.koenig@amd.com>
>>
>> Most BIOS don't enable this because of compatibility reasons.
> Can you give any more details here?  Without more hints, it's hard to
> know whether any of the compatibility reasons might apply to Linux as
> well.

Unfortunately not, I could try to ask a few more people at AMD if they 
know the background.

I was told that there are a few boards which offers that as a BIOS 
option, but so far I haven't found any (and I have quite a few here).

My best guess is that older windows versions have a problem with that.

>> Manually enable a 64bit BAR of 64GB size so that we have
>> enough room for PCI devices.
>  From the context, I'm guessing this is enabling a new 64GB window
> through the PCI host bridge?

Yes, exactly. Sorry for the confusion.

> That might be documented as a "BAR", but
> it's not anything the Linux PCI core would recognize as a BAR.

At least the AMD NB documentation calls this the host BARs. But I'm 
perfectly fine with any terminology.

How about calling it host bridge window instead?

> I think the specs would envision this being done via an ACPI _SRS
> method on the PNP0A03 host bridge device.  That would be a more
> generic path that would work on any host bridge.  Did you explore that
> possibility?  I would prefer to avoid adding device-specific code if
> that's possible.

I've checked quite a few boards, but none of them actually implements it 
this way.

M$ is working on a new ACPI table to enable this vendor neutral, but I 
guess that will still take a while.

I want to support this for all AMD CPU released in the past 5 years or 
so, so we are going to deal with a bunch of older boards as well.


>> +	pci_bus_add_resource(dev->bus, res, 0);
> We would need some sort of printk here to explain how this new window
> magically appeared.

Good point, consider this done.

But is this actually the right place of doing it? Or would you prefer 
something to be added to the probing code?

I think those fixups are applied a bit later, aren't they?

Best regards,
Christian.
Bjorn Helgaas April 12, 2017, 4:55 p.m. UTC | #6
On Tue, Apr 11, 2017 at 05:48:25PM +0200, Christian König wrote:
> Am 24.03.2017 um 16:47 schrieb Bjorn Helgaas:
> >On Mon, Mar 13, 2017 at 01:41:35PM +0100, Christian König wrote:
> >>From: Christian König <christian.koenig@amd.com>
> >>
> >>Most BIOS don't enable this because of compatibility reasons.
> >Can you give any more details here?  Without more hints, it's hard to
> >know whether any of the compatibility reasons might apply to Linux as
> >well.
> 
> Unfortunately not, I could try to ask a few more people at AMD if
> they know the background.
> 
> I was told that there are a few boards which offers that as a BIOS
> option, but so far I haven't found any (and I have quite a few
> here).
> 
> My best guess is that older windows versions have a problem with that.
> 
> >>Manually enable a 64bit BAR of 64GB size so that we have
> >>enough room for PCI devices.
> > From the context, I'm guessing this is enabling a new 64GB window
> >through the PCI host bridge?
> 
> Yes, exactly. Sorry for the confusion.
> 
> >That might be documented as a "BAR", but
> >it's not anything the Linux PCI core would recognize as a BAR.
> 
> At least the AMD NB documentation calls this the host BARs. But I'm
> perfectly fine with any terminology.
> 
> How about calling it host bridge window instead?

That works for me.

> >I think the specs would envision this being done via an ACPI _SRS
> >method on the PNP0A03 host bridge device.  That would be a more
> >generic path that would work on any host bridge.  Did you explore that
> >possibility?  I would prefer to avoid adding device-specific code if
> >that's possible.
> 
> I've checked quite a few boards, but none of them actually
> implements it this way.
> 
> M$ is working on a new ACPI table to enable this vendor neutral, but
> I guess that will still take a while.
> 
> I want to support this for all AMD CPU released in the past 5 years
> or so, so we are going to deal with a bunch of older boards as well.

I've never seen _SRS for host bridges either.  I'm curious about what
sort of new table will be proposed.  It seems like the existing ACPI
resource framework could manage it, but I certainly don't know all the
issues.

> >>+	pci_bus_add_resource(dev->bus, res, 0);
> >We would need some sort of printk here to explain how this new window
> >magically appeared.
> 
> Good point, consider this done.
> 
> But is this actually the right place of doing it? Or would you
> prefer something to be added to the probing code?
> 
> I think those fixups are applied a bit later, aren't they?

Logically, this should be done before we enumerate the PCI devices
below the host bridge, so a PCI device fixup is not the ideal place
for it, but it might be the most practical.

I could imagine some sort of quirk like the ones in
drivers/pnp/quirks.c that could add the window to the host bridge _CRS
and program the bridge to open it.  But the PCI host bridges aren't
handled through the path that applies those fixups, and it would be
messy to identify your bridges (you currently use PCI vendor/device
IDs, which are only available after enumerating the device).  So this
doesn't seem like a viable strategy.

Bjorn
Christian König April 25, 2017, 1:01 p.m. UTC | #7
Am 12.04.2017 um 18:55 schrieb Bjorn Helgaas:
> [SNIP]
>>> I think the specs would envision this being done via an ACPI _SRS
>>> method on the PNP0A03 host bridge device.  That would be a more
>>> generic path that would work on any host bridge.  Did you explore that
>>> possibility?  I would prefer to avoid adding device-specific code if
>>> that's possible.
>> I've checked quite a few boards, but none of them actually
>> implements it this way.
>>
>> M$ is working on a new ACPI table to enable this vendor neutral, but
>> I guess that will still take a while.
>>
>> I want to support this for all AMD CPU released in the past 5 years
>> or so, so we are going to deal with a bunch of older boards as well.
> I've never seen _SRS for host bridges either.  I'm curious about what
> sort of new table will be proposed.  It seems like the existing ACPI
> resource framework could manage it, but I certainly don't know all the
> issues.

No idea either since I'm not involved into that. My job is to get it 
working on the existing hw generations and that alone is enough work :)

My best guess is that MS is going to either make _SRS on the host bridge 
or a pre-configured 64bit window mandatory for the BIOS.

>>>> +	pci_bus_add_resource(dev->bus, res, 0);
>>> We would need some sort of printk here to explain how this new window
>>> magically appeared.
>> Good point, consider this done.
>>
>> But is this actually the right place of doing it? Or would you
>> prefer something to be added to the probing code?
>>
>> I think those fixups are applied a bit later, aren't they?
> Logically, this should be done before we enumerate the PCI devices
> below the host bridge, so a PCI device fixup is not the ideal place
> for it, but it might be the most practical.

Since the modification must be done on a device connected to the root 
bus I run into quite a chicken and egg problem if I try to do it before 
the enumeration.

> I could imagine some sort of quirk like the ones in
> drivers/pnp/quirks.c that could add the window to the host bridge _CRS
> and program the bridge to open it.  But the PCI host bridges aren't
> handled through the path that applies those fixups, and it would be
> messy to identify your bridges (you currently use PCI vendor/device
> IDs, which are only available after enumerating the device).  So this
> doesn't seem like a viable strategy.

I've tried that, but gave up rather quickly. Looks like the current 
approach indeed work find even with "pci=realloc", so I'm going to stick 
with that.

Regards,
Christian.
Bjorn Helgaas May 17, 2017, 9:36 p.m. UTC | #8
On Tue, Apr 25, 2017 at 03:01:35PM +0200, Christian König wrote:
> Am 12.04.2017 um 18:55 schrieb Bjorn Helgaas:
> >[SNIP]
> >>>I think the specs would envision this being done via an ACPI _SRS
> >>>method on the PNP0A03 host bridge device.  That would be a more
> >>>generic path that would work on any host bridge.  Did you explore that
> >>>possibility?  I would prefer to avoid adding device-specific code if
> >>>that's possible.
> >>I've checked quite a few boards, but none of them actually
> >>implements it this way.
> >>
> >>M$ is working on a new ACPI table to enable this vendor neutral, but
> >>I guess that will still take a while.
> >>
> >>I want to support this for all AMD CPU released in the past 5 years
> >>or so, so we are going to deal with a bunch of older boards as well.
> >I've never seen _SRS for host bridges either.  I'm curious about what
> >sort of new table will be proposed.  It seems like the existing ACPI
> >resource framework could manage it, but I certainly don't know all the
> >issues.
> 
> No idea either since I'm not involved into that. My job is to get it
> working on the existing hw generations and that alone is enough work
> :)
> 
> My best guess is that MS is going to either make _SRS on the host
> bridge or a pre-configured 64bit window mandatory for the BIOS.

While researching something else, I noticed that the PCI Firmware Spec, rev
3.2, does indeed call out _PRS and _SRS as the mechanism for the OS to
configure host bridge windows.  See sections 4.1.3, 4.3.2.1, and 4.6.5.

Sec 4.6.5 also includes an implementation note that might be a clue about
the "compatibility issues" that prevent the BIOS from enabling the window
in the first place.

I'd like to incorporate some of this info into these changes, probably in a
code comment and changelog, so we can encourage a more generic approach in
the future, even if we can't use it in all existing cases.

Bjorn
diff mbox

Patch

diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
index 6d52b94..bff5242 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -571,3 +571,56 @@  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2fc0, pci_invalid_bar);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6f60, pci_invalid_bar);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6fa0, pci_invalid_bar);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6fc0, pci_invalid_bar);
+
+static void pci_amd_enable_64bit_bar(struct pci_dev *dev)
+{
+	const uint64_t size = 64ULL * 1024 * 1024 * 1024;
+	uint32_t base, limit, high;
+	struct resource *res;
+	unsigned i;
+	int r;
+
+	for (i = 0; i < 8; ++i) {
+
+		pci_read_config_dword(dev, 0x80 + i * 0x8, &base);
+		pci_read_config_dword(dev, 0x180 + i * 0x4, &high);
+
+		/* Is this slot free? */
+		if ((base & 0x3) == 0x0)
+			break;
+
+		base >>= 8;
+		base |= high << 24;
+
+		/* Abort if a slot already configures a 64bit BAR. */
+		if (base > 0x10000)
+			return;
+
+	}
+
+	if (i == 8)
+		return;
+
+	res = kzalloc(sizeof(*res), GFP_KERNEL);
+	res->flags = IORESOURCE_MEM | IORESOURCE_PREFETCH | IORESOURCE_MEM_64 |
+		IORESOURCE_WINDOW;
+	res->name = dev->bus->name;
+	r = allocate_resource(&iomem_resource, res, size, 0x100000000,
+			      0xfd00000000, size, NULL, NULL);
+	if (r) {
+		kfree(res);
+		return;
+	}
+
+	base = ((res->start >> 8) & 0xffffff00) | 0x3;
+	limit = ((res->end + 1) >> 8) & 0xffffff00;
+	high = ((res->start >> 40) & 0xff) |
+		((((res->end + 1) >> 40) & 0xff) << 16);
+
+	pci_write_config_dword(dev, 0x180 + i * 0x4, high);
+	pci_write_config_dword(dev, 0x84 + i * 0x8, limit);
+	pci_write_config_dword(dev, 0x80 + i * 0x8, base);
+
+	pci_bus_add_resource(dev->bus, res, 0);
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_AMD, 0x141b, pci_amd_enable_64bit_bar);