Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

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

46 views
Skip to first unread message

Christian König

unread,
Mar 13, 2017, 8:50:05 AM3/13/17
to
From: Christian König <christia...@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 <christia...@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);
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_AMD, 0x141b, pci_amd_enable_64bit_bar);
--
2.7.4

Christian König

unread,
Mar 13, 2017, 8:50:06 AM3/13/17
to
Hi Bjorn and others on the lists,

it's over a year that I initially came up with that, but now I
finally have the time to finish it up.

This set of patches allows device drivers to resize and most likely also
relocate the PCI BAR of devices they manage to allow the CPU to access
all of the device local memory at once.

This is very useful for GFX device drivers where the default PCI BAR is only
about 256MB in size for compatibility reasons, but the device easily have
multiple gigabyte of local memory.

Additional to the pure resize functionality this set also adds a quirk to
enable a 64bit bar above 4GB on AMD Family 15h CPUs/APUs. Doing so is
actually trivial and I plan to extend this to all recent AMD CPUs/APUs.

Open questions:

1. Is this the right location to implement the 64bit BAR above 4GB for AMD CPUs?
I guess that this needs a bit more cleanup.

2. Any idea how to better handle intermediate bridges? Reprogramming them isn't
possible after drivers are loaded, but at least for GFX drivers we need to load
them first to know how large we actually want our BAR to be (and to kick our
vesafb/efifb).

Thanks in advance,
Christian.

Christian König

unread,
Mar 13, 2017, 8:50:08 AM3/13/17
to
From: Christian König <christia...@amd.com>

Try to resize BAR0 to let CPU access all of VRAM.

Signed-off-by: Christian König <christia...@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 +
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 29 +++++++++++++++++++++++++++++
drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c | 8 +++++---
drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c | 8 +++++---
4 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 3b81ded..905ded9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1719,6 +1719,7 @@ uint64_t amdgpu_ttm_tt_pte_flags(struct amdgpu_device *adev, struct ttm_tt *ttm,
struct ttm_mem_reg *mem);
void amdgpu_vram_location(struct amdgpu_device *adev, struct amdgpu_mc *mc, u64 base);
void amdgpu_gtt_location(struct amdgpu_device *adev, struct amdgpu_mc *mc);
+void amdgpu_resize_bar0(struct amdgpu_device *adev);
void amdgpu_ttm_set_active_vram_size(struct amdgpu_device *adev, u64 size);
int amdgpu_ttm_init(struct amdgpu_device *adev);
void amdgpu_ttm_fini(struct amdgpu_device *adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 118f4e6..92955fe 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -692,6 +692,35 @@ void amdgpu_gtt_location(struct amdgpu_device *adev, struct amdgpu_mc *mc)
mc->gtt_size >> 20, mc->gtt_start, mc->gtt_end);
}

+/**
+ * amdgpu_resize_bar0 - try to resize BAR0
+ *
+ * @adev: amdgpu_device pointer
+ *
+ * Try to resize BAR0 to make all VRAM CPU accessible.
+ */
+void amdgpu_resize_bar0(struct amdgpu_device *adev)
+{
+ u32 size = max(ilog2(adev->mc.real_vram_size - 1) + 1, 20) - 20;
+ int r;
+
+ r = pci_resize_resource(adev->pdev, 0, size);
+
+ if (r == -ENOTSUPP) {
+ /* The hardware don't support the extension. */
+ return;
+
+ } else if (r == -ENOSPC) {
+ DRM_INFO("Not enoigh PCI address space for a large BAR.");
+ } else if (r) {
+ DRM_ERROR("Problem resizing BAR0 (%d).", r);
+ }
+
+ /* Reinit the doorbell mapping, it is most likely moved as well */
+ amdgpu_doorbell_fini(adev);
+ BUG_ON(amdgpu_doorbell_init(adev));
+}
+
/*
* GPU helpers function.
*/
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
index dc9b6d6..36a7aa5 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
@@ -367,13 +367,15 @@ static int gmc_v7_0_mc_init(struct amdgpu_device *adev)
break;
}
adev->mc.vram_width = numchan * chansize;
- /* Could aper size report 0 ? */
- adev->mc.aper_base = pci_resource_start(adev->pdev, 0);
- adev->mc.aper_size = pci_resource_len(adev->pdev, 0);
/* size in MB on si */
adev->mc.mc_vram_size = RREG32(mmCONFIG_MEMSIZE) * 1024ULL * 1024ULL;
adev->mc.real_vram_size = RREG32(mmCONFIG_MEMSIZE) * 1024ULL * 1024ULL;

+ if (!(adev->flags & AMD_IS_APU))
+ amdgpu_resize_bar0(adev);
+ adev->mc.aper_base = pci_resource_start(adev->pdev, 0);
+ adev->mc.aper_size = pci_resource_len(adev->pdev, 0);
+
#ifdef CONFIG_X86_64
if (adev->flags & AMD_IS_APU) {
adev->mc.aper_base = ((u64)RREG32(mmMC_VM_FB_OFFSET)) << 22;
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
index c087b00..7761ad3 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
@@ -459,13 +459,15 @@ static int gmc_v8_0_mc_init(struct amdgpu_device *adev)
break;
}
adev->mc.vram_width = numchan * chansize;
- /* Could aper size report 0 ? */
- adev->mc.aper_base = pci_resource_start(adev->pdev, 0);
- adev->mc.aper_size = pci_resource_len(adev->pdev, 0);
/* size in MB on si */
adev->mc.mc_vram_size = RREG32(mmCONFIG_MEMSIZE) * 1024ULL * 1024ULL;
adev->mc.real_vram_size = RREG32(mmCONFIG_MEMSIZE) * 1024ULL * 1024ULL;

+ if (!(adev->flags & AMD_IS_APU))
+ amdgpu_resize_bar0(adev);
+ adev->mc.aper_base = pci_resource_start(adev->pdev, 0);
+ adev->mc.aper_size = pci_resource_len(adev->pdev, 0);
+
#ifdef CONFIG_X86_64
if (adev->flags & AMD_IS_APU) {
adev->mc.aper_base = ((u64)RREG32(mmMC_VM_FB_OFFSET)) << 22;
--
2.7.4

Andy Shevchenko

unread,
Mar 13, 2017, 12:50:10 PM3/13/17
to
On Mon, Mar 13, 2017 at 2:41 PM, Christian König
<death...@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).

--
With Best Regards,
Andy Shevchenko

Andy Shevchenko

unread,
Mar 13, 2017, 1:00:06 PM3/13/17
to
On Mon, Mar 13, 2017 at 2:41 PM, Christian König
<death...@vodafone.de> wrote:
> From: Christian König <christia...@amd.com>
>
> Try to resize BAR0 to let CPU access all of VRAM.

> +void amdgpu_resize_bar0(struct amdgpu_device *adev)
> +{
> + u32 size = max(ilog2(adev->mc.real_vram_size - 1) + 1, 20) - 20;
> + int r;
> +
> + r = pci_resize_resource(adev->pdev, 0, size);

> +

Redundant.

> + if (r == -ENOTSUPP) {
> + /* The hardware don't support the extension. */
> + return;

> +

Ditto.

> + } else if (r == -ENOSPC) {

Useless use of else. And thus of curly braces.

> + DRM_INFO("Not enoigh PCI address space for a large BAR.");
> + } else if (r) {
> + DRM_ERROR("Problem resizing BAR0 (%d).", r);
> + }
> +
> + /* Reinit the doorbell mapping, it is most likely moved as well */
> + amdgpu_doorbell_fini(adev);

> + BUG_ON(amdgpu_doorbell_init(adev));

Comment why it's used here.

Ayyappa Ch

unread,
Mar 15, 2017, 3:30:05 AM3/15/17
to
Is it possible on Carrizo asics? Or only supports on newer asics?
> _______________________________________________
> dri-devel mailing list
> dri-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Christian König

unread,
Mar 15, 2017, 3:40:05 AM3/15/17
to
Carizzo is an APU and resizing BARs isn't needed nor supported there.
The CPU can access the full stolen VRAM directly on that hardware.

As far as I know ASICs with support for this are Tonga, Fiji and all
Polaris variants.

Christian.

Zhou, David(ChunMing)

unread,
Mar 15, 2017, 4:30:05 AM3/15/17
to
Does that means we don't need invisible vram later?

David

Christian König

unread,
Mar 15, 2017, 5:30:05 AM3/15/17
to
Yes, exactly that.

Christian.
> _______________________________________________
> amd-gfx mailing list
> amd...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Ayyappa Ch

unread,
Mar 15, 2017, 6:50:07 AM3/15/17
to
It also needs any support from VBIOS side ? I mean PCIe large bar support?

Thanks,
Ayyappa.

On Wed, Mar 15, 2017 at 1:07 PM, Christian König

Christian König

unread,
Mar 15, 2017, 7:10:06 AM3/15/17
to
No, we resize the BAR on the fly during driver load without help from
the BIOS or VBIOS.

Christian.

Deucher, Alexander

unread,
Mar 15, 2017, 12:20:09 PM3/15/17
to
> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx...@lists.freedesktop.org] On Behalf
> Of Christian König
> Sent: Wednesday, March 15, 2017 3:38 AM
> To: Ayyappa Ch
> Cc: linu...@vger.kernel.org; linux-...@vger.kernel.org; amd-
> g...@lists.freedesktop.org; platform-...@vger.kernel.org;
> hel...@kernel.org; dri-...@lists.freedesktop.org
> Subject: Re: [PATCH 4/4] drm/amdgpu: resize VRAM BAR for CPU access
>
> Carizzo is an APU and resizing BARs isn't needed nor supported there.
> The CPU can access the full stolen VRAM directly on that hardware.
>
> As far as I know ASICs with support for this are Tonga, Fiji and all
> Polaris variants.

I think resizable BARs are supported as far back as evergreen or NI.

Alex

Zhang, Jerry

unread,
Mar 15, 2017, 10:20:05 PM3/15/17
to
> -----Original Message-----
> From: dri-devel [mailto:dri-deve...@lists.freedesktop.org] On Behalf Of
> Christian K?nig
> Sent: Wednesday, March 15, 2017 17:29
> To: Zhou, David(ChunMing); Ayyappa Ch
> Cc: linu...@vger.kernel.org; linux-...@vger.kernel.org; amd-
> g...@lists.freedesktop.org; platform-...@vger.kernel.org;
> hel...@kernel.org; dri-...@lists.freedesktop.org
> Subject: Re: [PATCH 4/4] drm/amdgpu: resize VRAM BAR for CPU access
>
> Yes, exactly that.

(I'm not familiar with PCI too much.)
Is there any restrict for PCI device?
I'm concerning if any PCI couldn't support it on some motherboard.

Alex Deucher

unread,
Mar 15, 2017, 10:30:05 PM3/15/17
to
On Wed, Mar 15, 2017 at 10:19 PM, Zhang, Jerry <Jerry...@amd.com> wrote:
>> -----Original Message-----
>> From: dri-devel [mailto:dri-deve...@lists.freedesktop.org] On Behalf Of
>> Christian K?nig
>> Sent: Wednesday, March 15, 2017 17:29
>> To: Zhou, David(ChunMing); Ayyappa Ch
>> Cc: linu...@vger.kernel.org; linux-...@vger.kernel.org; amd-
>> g...@lists.freedesktop.org; platform-...@vger.kernel.org;
>> hel...@kernel.org; dri-...@lists.freedesktop.org
>> Subject: Re: [PATCH 4/4] drm/amdgpu: resize VRAM BAR for CPU access
>>
>> Yes, exactly that.
>
> (I'm not familiar with PCI too much.)
> Is there any restrict for PCI device?
> I'm concerning if any PCI couldn't support it on some motherboard.

It depends on the PCI root bridge. This patch set only implements
support for AMD root bridges. Intel and other vendors would need
similar code.

Alex

Zhang, Jerry

unread,
Mar 15, 2017, 10:50:05 PM3/15/17
to
Thanks for your info.
I see.

Regards,
Jerry (Junwei Zhang)

Linux Base Graphics
SRDC Software Development
_____________________________________

Sagalovitch, Serguei

unread,
Mar 23, 2017, 10:40:07 AM3/23/17
to
Christian,

- Are we going to support resizing BAR when kernel
modesetting is not enabled and we are running in console
under VBIOS control (VESA/VGA)?

- Should we restore PCI configuration if amdgpu
will be unloaded?

- In function amdgpu_resize_bar0():
If resizing for "max" size failed should we try other
sizes? What do you think?


Sincerely yours,
Serguei Sagalovitch

Christian König

unread,
Mar 23, 2017, 12:00:07 PM3/23/17
to
> - Are we going to support resizing BAR when kernel
> modesetting is not enabled and we are running in console
> under VBIOS control (VESA/VGA)?
No, initial I've tried to resize the PCI BAR during probing without the
help of the driver at all. But the VESA/EFI/VBIOS don't seem to be able
to handle addresses above 4GB for some reason.

So the approach is to let the driver kick the VESA/EFI drivers out and
then resize when we know that nobody is accessing the BAR.

That's the only approach I've found without either blacklisting VESA/EFI
drivers or crashing the system during the resize.

> - Should we restore PCI configuration if amdgpu
> will be unloaded?
Yeah, thought about the as well. I'm just not sure how to do it.

There is a lot of stuff we need to save and reset when the driver
unloads for not much gain.

> - In function amdgpu_resize_bar0():
> If resizing for "max" size failed should we try other
> sizes? What do you think?
Probably not worth it. If we get the BAR moved to a 64bit address we
should have enough address space in almost all cases, so setting it to
the maximum should succeed.

But I think we could add another parameter to allow limiting the resized
size for all corner cases and for testing.

Regards,
Christian.

Bjorn Helgaas

unread,
Mar 24, 2017, 11:50:07 AM3/24/17
to
On Mon, Mar 13, 2017 at 01:41:35PM +0100, Christian König wrote:
> From: Christian König <christia...@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.
We would need some sort of printk here to explain how this new window
magically appeared.

Bjorn Helgaas

unread,
Mar 24, 2017, 5:50:05 PM3/24/17
to
s/enoigh/enough/

> + } else if (r) {
> + DRM_ERROR("Problem resizing BAR0 (%d).", r);
> + }
> +
> + /* Reinit the doorbell mapping, it is most likely moved as well */

I think you should assume all BARs moved (I don't know how many you have;
maybe this already covers all of them).

> + amdgpu_doorbell_fini(adev);
> + BUG_ON(amdgpu_doorbell_init(adev));

I think things inside BUG_ON() tend to get overlooked, so I avoid things
that have side-effects. But that's just my personal preference.

Christian König

unread,
Apr 11, 2017, 5:30:05 AM4/11/17
to
Am 13.03.2017 um 17:49 schrieb Andy Shevchenko:
> On Mon, Mar 13, 2017 at 2:41 PM, Christian König
> <death...@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

unread,
Apr 11, 2017, 11:50:05 AM4/11/17
to
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 <christia...@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

unread,
Apr 12, 2017, 1:00:05 PM4/12/17
to
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 <christia...@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

unread,
Apr 25, 2017, 9:10:10 AM4/25/17
to
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

unread,
May 17, 2017, 5:40:05 PM5/17/17
to
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
0 new messages