PCI rework

[Ian Romanick]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Mark Kettenis wrote:
>>From: Jesse Barnes <jbarnes at virtuousgeek.org>
>>Date: Sun, 30 Apr 2006 11:06:50 -0700
>>
>>On Sunday, April 30, 2006 5:22 am, Mark Kettenis wrote:
>>
>>>Matthieu Herb, asked me to have a look at libpciaccess some time ago.
>>>My conclusion then was that it is too tightly bound to Linux.  It
>>>takes the Linux sysfs pci access interface as a model instead of the
>>>actual pci hardware.  I especially feel the support to map memory
>>>associated with the PCI BAR's does not belong in the library.  It is
>>>not impossible to implement the libpciaccess backend for other
>>>operating systems, but it would be rather awkward.
>>
>>X already assumes it can mmap memory BARs, do you think it needs to 
>>change too?  It seems to me that the interface provided by libpciaccess 
>>is fairly flexible, easy to use, and easily ported to new platforms (it 
>>should be, it's based on the Linux internal PCI interfaces which have 
>>prooven highly portable).  Do you have other examples where you think 
>>Linux specific implementation details have crept into the interface?
> 
> I'm talking specifically about pci_device_map_region() and
> pci_device_unmap_region().  These interfaces are clearly there because
> the Linux sysfs provides these "regions" as files which you can open
> and then mmap.  Other operating systems don't support this view, and

On those systems (including older Linux kernels that do not have sysfs),
pci_device_map_region internally does an mmap on /dev/mem of the proper
offset.

Note that this is currently broken in X on some platforms.  IBM pSeries
hardware always maps BARs above 0xffffffff.  If the X-server is built as
a 32-bit app (which is the default), it silently drops the upper
32-bits.  This problem is propagate to each and every driver.  By
isoalting it in a single library, the amount of code that is exposed to
this issue is limited.

> there's a good reason not to do it: Many PCI devices put mappable
> memory addresses in config space outside the standard PCI BARs.  What
> you really need is an interface to read from PCI config space and an
> interface to map physicall addresses on the pci bus into memory.  Some
> standard helper functions to decode the standard BARs is certainly
> desirable.

Excluding legacy VGA memory, can you name any?

>>>Looking at your patch I see that changes to the drivers are mostly
>>>changes like:
>>>
>>>-    *pucByte = pciReadByte(pMga->PciTag,ulOffset);
>>>+    pci_device_cfg_read_u8( pMga->PciInfo, pucByte, ulOffset );
>>>
>>>So basically this is just replacing the current interfaces with the
>>>equivalent libpciaccess interfaces.  Why not keep the current current
>>>interfaces and reimplement them to use libpciaccess in the
>>>Linux-specific Xorg code?
>>
>>Note that the prototype is different in that it takes a full pci info 
>>structure rather than just a tag.  This gives arch specific 
>>implementations more flexibility and eases porting.
> 
> But a PCITAG is already opaque; there's no reason why you could extend
> it to include the additional information you might need.

PCITAG is *not* opaque.  The previous attempt to extend it (for PCI
domains) is ugly and evil.

>>>I really don't see any major flaws in the 
>>>current xf86Pci.h interfaces, but I agree that some of the
>>>implementation could use some cleanup.
>>
>>I disagree with this, the xf86Pci interface is pretty screwy:
>>  o X does things with PCI devices it has no business doing (e.g.
>>    remapping BARs)
> 
> Remapping as in writing different addresses into them?  X might need
> to do that if the firmware doesn't properly initialize them.  I've
> seen many, many buggy firmware implementations (ok they're mostly
> BIOSes) that don't do this properly.

Again, this is a job that the kernel should do.  PERIOD.  On platforms
where the kernel is that broken (seriously, are there any still?), the
code that is currently in the X-server could be put it that platform's
backend in libpciaccess.  Again, this limits the exposure of
per-platform issues.

>>  o the distinction between mapping domain and regular PCI memory is
>>    arbitrary and should be removed
> 
> I'm not quite sure what is actually meant by a "domain".  I'm
> presuming it's similar to what the ACPI specification calls a
> "segment": a completely seperate PCI bus hierarchy.  Yes, the way the
> current interface handles that is awkward, but it should be easy to
> fix this if you add the domain to the PCITAG.

The domain is in the PCITAG, and that is one of the sources of the
awkwardness.

>>  o the PCI device discovery code needed by drivers is unnecessarily
>>    complex
> 
> I'm not so sure about that.  Some amount of complexity will be needed
> to deal with badly designed or buggy hardware and firmware.  Most of
> these issues will be specific to particular PCI hardware.  Shoving
> those into the domain of a seperate library, or the further down into
> the operating system isn't a solution.

The burden of dealing with buggy devices / firmware shouldn't be forced
onto all drivers.  There should be an easy interface for the 95% case,
and a fallback interface for the rest.  That is, by the way, what I have
implemented (PciProbe vs. Probe).

>>  o ROM mapping is hard to port and buggy in some cases
> 
> By its very nature ROM's are unportable.  I really can't see how
> libpciaccess would alleviate that situation.  The X hardware drivers
> really should try to avoid depending on ROMs.

Quite a few drivers need to access the ROM to detect hardware features.
 For example, the *ONLY* way to determine the existance of some features
on MGA hardware is via bits stored in the option ROM.  libpciaccess can
do its best to make the ROM available (again, by working around
per-platform issues).

If the ROM can't be made available, pci_device_read_rom returns an
error, and the driver has to deal with that.  On truly broken platforms,
the pci_device_read_rom handler can just be hardcoded to always return NULL.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2.2 (GNU/Linux)

iD8DBQFEVlbGX1gOwKyEAw8RAicrAJwJDNeu2k/rIv4OkIMxeNVQtY5SPQCgjUip
TyI8GQ2QuZxGA45lQZLQiQ8=
=aue5
-----END PGP SIGNATURE-----