> From: Adam Jackson <a...@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 <a...@redhat.com> > > > Date: Wed, 18 Nov 2009 14:28:57 -0500 > > > > > > Signed-off-by: Adam Jackson <a...@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! _______________________________________________ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel