2006-10-19 10:01:28

by eiichiro.oiwa.nm

[permalink] [raw]
Subject: Re[2]: pci_fixup_video change blows up on sparc64

>> If an expansion ROM exists on ATI Radeon or ATY128 card, pci_map_rom returns
>> the expansion ROM base address instead of 0xC0000 because fixup_video checks
>> the VGA Enable bit in the Bridge Control register.
>
>It is not valid to expect the bridge control register to return
>anything meaningful on PCI "host bridge". The Radeon card here sits
>on the root, just under the PCI Host Controller. The code in
>fixup_video appears to assume that every bus up to the root from
>the VGA device is a PCI-PCI bridge, which is not a valid assumption.
>There can be a PCI host bridge at the root.

Have you ever read the PCI-to-PCI Bridge Architecture Specification?
The default of VGA Enable bit is 0. This mean video ROM doesn't forward
system RAM at 0xC0000.

There is your VGA card under 0001:00:00.0 Host bridge. The VGA Enable bit
in this host bridge will return 0 and IORESOURCE_ROM_SHADOW won't set.

>Also, and more importantly, you cannot use the 0xc0000 address in a
>raw way like this. There are multiple PCI domains possible in a
>given system, and the 0xc0000 address you wish to use must be relative
>to that PCI domain.
>
>Therefore, in the presence of multiple PCI domains:
>
> x = ioremap(0xc0000, ...);
>
>doesn't make any sense, is extremely non-portable, and will crash
>on many non-x86 systems.

It's impossible that multiple VGA cards, which have not the expansion
ROM, exist in a system regardless of multiple PCI domain system.

>All of this pci_fixup_video code was perfectly fine when it was only
>used on x86, where assumptions like this happened to work, but it is
>not possible to continue making these assumptions if this code will
>now run on every single architecture.

pci_fixup_video is also perfectly fine on IA64. And VGA is historical
device of x86 platform.


2006-10-19 11:18:16

by Alan

[permalink] [raw]
Subject: Re: Re[2]: pci_fixup_video change blows up on sparc64

Ar Iau, 2006-10-19 am 19:01 +0900, ysgrifennodd
[email protected]:
> It's impossible that multiple VGA cards, which have not the expansion
> ROM, exist in a system regardless of multiple PCI domain system.

Strange. I've worked on several machines where it could.

The 0xC0000 is a physical address only as far as the bridge is concerned
(if the bridge even implements it - not all do). The PCI bus or busses
may not even be the root busses of the system. On such systems you can
happily have multiple PCI root bridges each in their own address space
and each with their own idea of where 0xC0000 maps if anywhere.


2006-10-19 18:02:49

by Jesse Barnes

[permalink] [raw]
Subject: Re: pci_fixup_video change blows up on sparc64

On Thursday, October 19, 2006 3:01 am, [email protected]
wrote:
> >> If an expansion ROM exists on ATI Radeon or ATY128 card,
> >> pci_map_rom returns the expansion ROM base address instead of
> >> 0xC0000 because fixup_video checks the VGA Enable bit in the
> >> Bridge Control register.
> >
> >It is not valid to expect the bridge control register to return
> >anything meaningful on PCI "host bridge". The Radeon card here sits
> >on the root, just under the PCI Host Controller. The code in
> >fixup_video appears to assume that every bus up to the root from
> >the VGA device is a PCI-PCI bridge, which is not a valid assumption.
> >There can be a PCI host bridge at the root.
>
> Have you ever read the PCI-to-PCI Bridge Architecture Specification?
> The default of VGA Enable bit is 0. This mean video ROM doesn't
> forward system RAM at 0xC0000.
>
> There is your VGA card under 0001:00:00.0 Host bridge. The VGA Enable
> bit in this host bridge will return 0 and IORESOURCE_ROM_SHADOW won't
> set.

I don't think that applies to host->pci bridges though, all bets are off
as to how their bits are defined. One check that might make this
feature a bit more robust is to look for a real PCI ROM on the device.
If present, we probably don't need to bother with the system copy
(which probably won't be there anyway).

We should probably also check whether the parent bridge of the device to
be fixed up is a real pci->pci bridge (if possible). That would remove
some ambiguity that's likely to cause problems with other platforms
too.

Jesse

2006-10-19 22:58:44

by David Miller

[permalink] [raw]
Subject: Re: pci_fixup_video change blows up on sparc64

From: Jesse Barnes <[email protected]>
Date: Thu, 19 Oct 2006 11:03:11 -0700

> On Thursday, October 19, 2006 3:01 am, [email protected]
> wrote:
> > >> If an expansion ROM exists on ATI Radeon or ATY128 card,
> > >> pci_map_rom returns the expansion ROM base address instead of
> > >> 0xC0000 because fixup_video checks the VGA Enable bit in the
> > >> Bridge Control register.
> > >
> > >It is not valid to expect the bridge control register to return
> > >anything meaningful on PCI "host bridge". The Radeon card here sits
> > >on the root, just under the PCI Host Controller. The code in
> > >fixup_video appears to assume that every bus up to the root from
> > >the VGA device is a PCI-PCI bridge, which is not a valid assumption.
> > >There can be a PCI host bridge at the root.
> >
> > Have you ever read the PCI-to-PCI Bridge Architecture Specification?
> > The default of VGA Enable bit is 0. This mean video ROM doesn't
> > forward system RAM at 0xC0000.
> >
> > There is your VGA card under 0001:00:00.0 Host bridge. The VGA Enable
> > bit in this host bridge will return 0 and IORESOURCE_ROM_SHADOW won't
> > set.
>
> I don't think that applies to host->pci bridges though, all bets are off
> as to how their bits are defined.

Yep, that's exactly what I was trying to say.

This is also why the rest of the kernel PCI code, and even things
like "lspci" from PCI utils do not print out the bridge control
register for host->pci bridges. It's contents are %100 undefined
for host->pci bridges, yet this pci_fixup_video() code is testing
those bits without checking what kind of bridge it is looking at.

> One check that might make this feature a bit more robust is to look
> for a real PCI ROM on the device. If present, we probably don't
> need to bother with the system copy (which probably won't be there
> anyway).
>
> We should probably also check whether the parent bridge of the device to
> be fixed up is a real pci->pci bridge (if possible). That would remove
> some ambiguity that's likely to cause problems with other platforms
> too.

At the core of this is the "definition" that 0xc0000 is a location in
physical RAM that the video ROM might be stored. While this might
be a %100 valid definition on IA64, x86 and x86_64, it simply is not
on other platforms such as sparc64.

In fact even the existence of any RAM at all at that address is
not guarenteed. I have a few systems where physical RAM starts
at some high physical address, such as 0x80000000.

Even if presence were guarenteed, you can't access this memory using
ioremap() and things like readl() and friends, which is exactly what
callers of pci_map_rom() are doing. Accesses using readl() use
non-cacheable transactions which result in bus errors, and furthermore
the first argument to ioremap() is not a physical address, it's an
architecture-defined "address cookie" that must be setup by platforms
specific code.