pci rework not quite ready yet?

[Jesse Barnes]

On Tuesday, August 28, 2007 8:39:02 pm Keith Packard wrote:
> On Tue, 2007-08-28 at 20:13 -0700, Ian Romanick wrote:
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> >
> > Keith Packard wrote:
> > > After converting the intel driver to the pciaccess API, I've discovered
> > > a couple of shortcomings that may need to be addressed before this code
> > > is really ready for use.
> > >
> > > The first problem was that we continue to have a legacy code in the
> > > server which passes bus addresses around, like xf86MapVidMem.
> >
> > A big part of the reason I started on this project is that on some
> > platforms, using bus addresses in this way is nonsensical.  The only
> > reason that xf86MapVidMem still lives is for ISA, sbus, and other
> > non-PCI buses.
>
> Mapping memory by address as xf86MapVidMem does makes sense if you know
> what device (or PCI domain) the address is related to. Adding a new API
> that includes a reference to the device would enable the address to be
> appropriately based.

Then the address in the BAR is relegated to "cookie" status.  This would be 
fine, but the code is (or at least used to be) somewhat confused in this 
regard.  At times the address was used as a 'real' address, resulting in bugs 
and BAR corruption on various platforms.  Also, the code didn't properly tie 
a given BAR value to a specific bus or PCI domain, resulting in duplicates 
and other bugs.  I'd be much happier if we just got away from using bus 
addresses entirely.  There's really no way we can use them reliably short of 
growing sophisticated bus drivers in the X server...

> libpciaccess must be the right place -- only it knows the relationship
> between the bus address of the device and how that maps to a CPU address
> which is programmed into the MTRR.

Yeah, the MTRR API for Linux at least isn't that great.  It takes a CPU 
physical address as a base argument, which means we have to program it with 
the value from the Linux pci_dev->resource fields, rather than the BAR 
address since these can be different on some platforms.

> > One reason for the current design of the mapping interface was to keep
> > tracking the mappings easy.  It doesn't get much easier than a 1-to-1
> > correlation between mappings and BARs.  Would something like this be
> > suitable?
> >
> > int pci_device_map_range(struct pci_device *dev, int bar,
> >     pciaddr_t offset, size_t size, unsigned flags, int *map);
>
> No, as described above there are devices which have mappings outside of
> any BAR (!). Or so anholt claims. I'm not sure what the BAR would be for
> in this case anyway; the only information needed here is which PCI
> domain the device is in anyway. Tracking a list of mappings by address
> seems easy enough; that's what the existing xf86Map routines do.

VGA legacy resources (VGA I/O ports and memory) for example fall outside this 
range; devices may respond to cycles on those addresses but not list them in 
their BARs.  I'm not sure about other ranges.  But libpciaccess shouldn't 
really be dealing with those regions I think.

> Given that we can track by address, simply using the physical address as
> the tag for unmap should suffice. It can be an error to map the same
> address twice.

Per the above, I think it would be best to avoid this...

Also, adding this new interface seems like a lot of churn for just dealing 
with cacheability attributes.  Maybe it would be best to have a separate, 
madvise-like call to set the attributes instead?  The user would be required 
to pass in a range or subrange already mapped by a previous mapping call.  
E.g.

  error_t pci_device_set_range_attributes(struct pci_device *dev,
					  pciaddr_t offset, size_t size,
					  unsigned flags)

This has the added bonus of making error checking easy, e.g. if you try to set 
WC on a range and it fails, you'll still have the mapping but you can warn 
the user that it won't be very performant.

> Also, let's work to transition the existing xf86 PCI apis to
> libpciaccess as well; the needn't be efficient, they just need to work.
> Just pulling apart the PCITAG given to these APIs should make it
> possible for them to operate as they used to. Most of them are not used
> in performance sensitive code anyway.

Ick, yeah, getting rid of PCITAG entirely would be nice.  But any cleanups (I 
think there are many more we could pursue) should be put off until things are 
working well.

Jesse