Re: PCI resource problems caused by improper address rounding

Linus Torvalds
2007-12-18T18:23:52Z

On Tue, 18 Dec 2007, Chuck Ebbert wrote:
> >
> > So why do you want them to be close, anyway?
>
> Because otherwise some video adapters with 256MB of memory end up with their
> resources allocated above 4GB, and that doesn't work very well.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=425794#c0

That bugzilla entry doesn't even have a dmesg output or anything like
that. I'd really like to see what the

The fact is, that patch is not safe. We very much _want_ to make the PCI
region allocator use a window that is in the *middle* of the gap, and not
close to either end of the gap, and the code literally tries to make the
default start of the PCI allocation gap start be about 1/16th of the
actual gap size in question, so that we don't hit BIOS allocations that
it didn't tell us about by mistake.

But without dmesg and lspci output to see what the actual allocations are,
there's no way to even _guess_ at whether there is a correct fix or not,
just the fix that totally misses the point of having any rounding-up at
all.

That patch might as well just do

pci_mem_start = gapstart;

and get rid of all that rounding code entirely, since the patch just
assumes that it's safe to use memory after gapstart (which is known to not
be true, and is the whole and only reason for that code in the first
place: BIOSes *invariably* get resource allocation wrong, and forget to
tell us about some resource they set up).

Now, it's entirely possible that the only reasonable end result is that we
do have to avoid rounding up that far, but I definitely want to see what
the actual resource situation is - that patch is *not* obviously correct,
and it definitely breaks the whole point of the code.

The *other* patch in the bugzilla entry seems more correct, in that yes,
we should make sure that we don't allocate resources over 4G if the
resource won't fit. That said, I think that patch is wrong too: we should
just fix pcibios_align_resource() to check that case for MEM resouces (the
same way it already knows about the magic rules for IO resources).

So I'd suggest just fixing pcibios_align_resource() instead. Something
like the appended might work (and then you could perhaps quirk it to
always clear the PCI_BASE_ADDRESS_MEM_TYPE_64 thing for VGA controllers,
although I really don't think the kernel is the right place to do that,
and that would be an X server issue!).

NOTE! This patch is an independent issue of the whole "what window do we
use to allocate new resources, and how do we align it" thing.

Linus

---
arch/x86/pci/i386.c | 14 ++++++++++++++
1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/arch/x86/pci/i386.c b/arch/x86/pci/i386.c
index 42ba0e2..abc642b 100644
--- a/arch/x86/pci/i386.c
+++ b/arch/x86/pci/i386.c
@@ -70,6 +70,20 @@ pcibios_align_resource(void *data, struct resource *res,
start = (start + 0x3ff) & ~0x3ff;
res->start = start;
}
+ } else {
+ u64 max;
+ switch (res->flags & PCI_BASE_ADDRESS_MEM_MASK) {
+ case PCI_BASE_ADDRESS_MEM_TYPE_1M:
+ max = 0xfffff;
+ break;
+ case PCI_BASE_ADDRESS_MEM_TYPE_64:
+ max = -1;
+ break;
+ default:
+ max = 0xffffffff;
+ }
+ if (res->start > max)
+ res->start = res->end;
}
}