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

[Adam Jackson]

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);

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.

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.

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.

So dropping ranges from the open routines seemed attractively simple.
It makes the port arguments unambiguous (the offset from the handle's
base), it matches the known use cases, and it means the handle can be
just the naked file descriptor.  I could probably be talked into putting
ranges back in, but if we do so I don't think the legacy API should
prevent you from shooting yourself in the foot.

I should add some internal bookkeeping anyway though, because
pci_system_cleanup() will not close I/O handles.  Oops.

> 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.

- ajax
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
Url : http://lists.x.org/archives/xorg-devel/attachments/20091119/5b7bcff5/attachment.pgp