[PATCH 2/2] I/O port access routines

[Mark Kettenis]

> From: Adam Jackson <ajax at redhat.com>
> Date: Thu, 19 Nov 2009 10:23:49 -0500
> 
> On Thu, 2009-11-19 at 00:13 +0100, Mark Kettenis wrote:
> > > From: Adam Jackson <ajax at redhat.com>
> > > Date: Wed, 18 Nov 2009 14:28:57 -0500
> > > 
> > > Signed-off-by: Adam Jackson <ajax at redhat.com>
> > > ---
> > >  include/pciaccess.h     |   14 ++++
> > >  src/Makefile.am         |    1 +
> > >  src/common_io.c         |   95 ++++++++++++++++++++++++
> > >  src/linux_sysfs.c       |  184 ++++++++++++++++++++++++++++++++++++----------
> > >  src/pciaccess_private.h |    9 +++
> > >  5 files changed, 263 insertions(+), 40 deletions(-)
> > >  create mode 100644 src/common_io.c
> > 
> > Oh, you actually went ahead and implemented this.  Cool!
> > 
> > I feel a bit guilty now, but I do have a few comments.  I think the
> > interface to "open" io should actually allow one to specify a range,
> > much in the same way as pci_device_map_range() does.  Perhaps the
> > right way to do this is simply to make pci_device_map_range() accept a
> > PCI_DEV_MAP_FLAG_IO flag.  You'd still need to use the pci_io_xxx
> > functions on the returned address to handle x86 machines properly of
> > course.
> 
> I admit I was seduced by the ability to make the I/O handle just the
> file descriptor.  When I first started typing, the prototype read:
> 
> void *pci_legacy_open_io(struct pci_device *dev, uint16_t base, uint16_t
> range);

uint16_t?  PCI provides a ful 32-bit address space for I/O space.
Probably quite a bit of hardware out there that doesn't really support
that, but still...

> Mostly for the reasons I'd said before, legacy port I/O should
> reasonably prevent you from poking at config space registers and ports
> claimed by BARs.  But the asymmetry with pci_device_open_io() made the
> in/out routines ambiguous.  To what would their port argument refer?
> The address in the domain, or the offset from the handle's base?
> Implementing either one would require carrying around more state in the
> region handle.

Not sure I understand completely what you're saying here.  However you
are asking the right question.  I'd say the answer is obvious: the
"port" argument to the in/out routines should be the offset relative
to base of the handle.

> The other way to fix the asymmetry would have been to extend
> pci_device_open_io() to also take a base/range pair.  But I was
> hard-pressed to come up with a use case where they would ever not be the
> whole BAR.  pci_device_map_range() makes sense because sometimes you
> need different caching policies on different bits of the BAR, but I/O
> (thankfully) never got posting policy control.

Well, I do think it would be superior.  For one thing, BAR numbering
is ambiguous in the presence of 64-bit BARs.  Also, there are devices
where certain register blocks are moved around between different
generations of a device.  In that case you could either specify the
right offset when you "map" things, or specify an offset for every
in/out call.  Guess what I'd prefer.

> So then I thought about the implications of keeping base/range for
> legacy I/O.  The major user I have in mind is libx86, where you need the
> entire legacy range opened because the VBIOS you're executing might in
> fact touch _any_ I/O port, and there's no real way of predicting that a
> priori.  So if the pci_io routines refused to let you touch some ports,
> libx86 would have to create multiple legacy I/O handles around them in
> all the negative space, and then walk its own internal map from
> base/range to handle.  That seems like it creates work rather than
> solves problems.  Especially if the blacklist in libpciaccess changed
> over time.

Ah, well, I'm thinking about the drivers proper that will be mapping
(parts) of an I/O BAR.  And, I don't see your problem here.  The
kernel should enforce restrictions, not libpciaccess.

> > Regarding those pci_io_xxx functions, would it be better to encode the
> > access size a bit more explicit, like the config space access functions?
> > 
> > pci_io_read_u8()
> > pci_io_read_u16()
> > pci_io_read_u32()
> > 
> > pci_io_write_u8()
> > pci_io_write_u16()
> > pci_io_write_u32()
> > 
> > The 'b' in inb/outb is unambiguous, but the 'w' and the 'l' already
> > didn't make sense for 32-bit machines, let alone now that we have
> > 64-bit machines.
> 
> I don't feel strongly either way.  It's not ambiguous if you remember
> that the 8086 instructions were {in,out}{b,w,l} and 8086 compatibility
> is the whole point of the I/O space, but, not needing to remember 8086
> arcana is the whole reason we have abstraction layers in the first
> place.

Exactly!