| Message ID | 1489408896-25039-4-git-send-email-deathsimple@vodafone.de |
|---|---|
| State | Changes Requested |
| Headers | show |
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).
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
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 >
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.
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.
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
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.
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 --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);