Re: [PATCH 2/2] I/O port access routines
> From: Adam Jackson > Date: Wed, 18 Nov 2009 14:28:57 -0500 > > Signed-off-by: Adam Jackson > --- > 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. 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. ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/2] I/O port access routines
On Wed, Nov 18, 2009 at 02:28:57PM -0500, Adam Jackson wrote:
> Signed-off-by: Adam Jackson
> ---
> 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
>
> diff --git a/include/pciaccess.h b/include/pciaccess.h
> index 8128656..b4a431a 100644
> --- a/include/pciaccess.h
> +++ b/include/pciaccess.h
> @@ -1,5 +1,6 @@
> /*
> * (C) Copyright IBM Corporation 2006
> + * Copyright 2009 Red Hat, Inc.
> * All Rights Reserved.
> *
> * Permission is hereby granted, free of charge, to any person obtaining a
> @@ -507,4 +508,17 @@ int pci_device_vgaarb_unlock (void);
> /* return the current device count + resource decodes for the device */
> int pci_device_vgaarb_get_info (struct pci_device *dev, int
> *vga_count, int *rsrc_decodes);
>
> +/*
> + * I/O space access.
> + */
> +void *pci_device_open_io(struct pci_device *dev, int bar);
> +void *pci_legacy_open_io(struct pci_device *dev);
> +void pci_device_close_io(struct pci_device *dev, void *handle);
> +uint32_t pci_io_inl(void *handle, uint16_t reg);
> +uint16_t pci_io_inw(void *handle, uint16_t reg);
> +uint8_t pci_io_inb(void *handle, uint16_t reg);
> +void pci_io_outl(void *handle, uint16_t reg, uint32_t data);
> +void pci_io_outw(void *handle, uint16_t reg, uint16_t data);
> +void pci_io_outb(void *handle, uint16_t reg, uint8_t data);
> +
> #endif /* PCIACCESS_H */
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 4dd7a5f..4c06c25 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -55,6 +55,7 @@ libpciaccess_la_SOURCES = common_bridge.c \
> common_iterator.c \
> common_init.c \
> common_interface.c \
> + common_io.c \
> common_capability.c \
> common_device_name.c \
> common_map.c \
> diff --git a/src/common_io.c b/src/common_io.c
> new file mode 100644
> index 000..9828af4
> --- /dev/null
> +++ b/src/common_io.c
> @@ -0,0 +1,95 @@
> +/*
> + * Copyright 2009 Red Hat, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software")
> + * to deal in the software without restriction, including without limitation
> + * on the rights to use, copy, modify, merge, publish, distribute, sub
> + * license, and/or sell copies of the Software, and to permit persons to whom
> + * them Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTIBILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS BE LIABLE FOR ANY CLAIM, DAMAGES, OR OTHER LIABILITY, WHETHER
> + * IN AN ACTION OF CONTRACT, TORT, OR OTHERWISE, ARISING FROM, OUT OF OR IN
> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * Author:
> + * Adam Jackson
> + */
> +
> +#include
> +#include "pciaccess.h"
> +#include "pciaccess_private.h"
> +
> +void *
> +pci_device_open_io(struct pci_device *dev, int bar)
> +{
> +if (!pci_sys->methods->open_device_io)
> + return NULL;
> +
> +if (bar < 0 || bar > 6)
> + return NULL;
> +
> +if (!dev->regions[bar].is_IO)
> + return NULL;
> +
> +return pci_sys->methods->open_device_io(dev, bar);
> +}
> +
> +void *
> +pci_legacy_open_io(struct pci_device *dev)
> +{
> +if (!pci_sys->methods->open_legacy_io)
> + return NULL;
> +
> +return pci_sys->methods->open_legacy_io(dev);
> +}
> +
> +void
> +pci_device_close_io(struct pci_device *dev, void *handle)
> +{
> +if (dev && handle && pci_sys->methods->close_io)
> + pci_sys->methods->close_io(dev, handle);
> +}
> +
> +uint32_t
> +pci_io_inl(void *handle, uint16_t reg)
> +{
> +return pci_sys->methods->inl(handle, reg);
> +}
> +
> +uint16_t
> +pci_io_inw(void *handle, uint16_t reg)
> +{
> +return pci_sys->methods->inw(handle, reg);
> +}
> +
> +uint8_t
> +pci_io_inb(void *handle, uint16_t reg)
> +{
> +return pci_sys->methods->inb(handle, reg);
> +}
> +
> +void
> +pci_io_outl(void *handle, uint16_t reg, uint32_t data)
> +{
> +pci_sys->methods->outl(handle, reg, data);
> +}
> +
> +void
> +pci_io_outw(void *handle, uint16_t reg, uint16_t data)
> +{
> +pci_sys->methods->outw(handle, reg, data);
> +}
> +
> +void
> +pci_io_outb(void *handle, uint16_t reg, uint8_t data)
> +{
> +pci_sys->methods->outb(handle, reg, data);
> +}
> diff --git a/src/linux_sysfs.c b/src/linux
Re: [PATCH 2/2] I/O port access routines
On Thu, Nov 19, 2009 at 12:37:10AM +0100, Luc Verhaegen wrote: > So how would we go about using any of this then? Do we get to have > another huge API/ABI breakage like when libpciaccess was introduced just > because the normal in/out routines should no longer be used? It's not even an ABI break, let alone API. If you read the patch, you'll note that new calls are being added, and nothing modified; drivers which choose to make use of these calls can make use of them. If you want to be compatible back to 6.9, you can ifdef it, or make a wrapper if you really care that much. > Oh, and that's before i start wondering about gratuitous void pointers > and goto. There's absolutely nothing wrong with void pointers (what do you recommend instead?), and goto is used in one place, as an error path to unwind, which is a very common coding practice (cf. the kernel) which avoids an entire class of errors related to failing to free things on an error return, mainly because you have the same unwind code duplicated in 17 different places. pgpbdr7v6s39B.pgp Description: PGP signature ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/2] I/O port access routines
On Thu, Nov 19, 2009 at 11:21:40AM +1100, Daniel Stone wrote: > On Thu, Nov 19, 2009 at 12:37:10AM +0100, Luc Verhaegen wrote: > > So how would we go about using any of this then? Do we get to have > > another huge API/ABI breakage like when libpciaccess was introduced just > > because the normal in/out routines should no longer be used? > > It's not even an ABI break, let alone API. If you read the patch, > you'll note that new calls are being added, and nothing modified; > drivers which choose to make use of these calls can make use of them. > If you want to be compatible back to 6.9, you can ifdef it, or make a > wrapper if you really care that much. > > > Oh, and that's before i start wondering about gratuitous void pointers > > and goto. > > There's absolutely nothing wrong with void pointers (what do you > recommend instead?), and goto is used in one place, as an error path to > unwind, which is a very common coding practice (cf. the kernel) which > avoids an entire class of errors related to failing to free things on > an error return, mainly because you have the same unwind code duplicated > in 17 different places. Well, that one goto is rather gratuitous. But the main point of this mail was ignored. Why did existing xorg pci infrastructure have to be reinvented like that instead of adding a new backend and fixing up the bad patches? Why did RAC get thrown away like that? Why does this NIH have to keep on repeating itself? Luc Verhaegen. ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/2] I/O port access routines
On Wed, Nov 18, 2009 at 2:28 PM, Adam Jackson wrote: > Signed-off-by: Adam Jackson > --- > 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 > > diff --git a/include/pciaccess.h b/include/pciaccess.h > index 8128656..b4a431a 100644 > --- a/include/pciaccess.h > +++ b/include/pciaccess.h > @@ -1,5 +1,6 @@ > /* > * (C) Copyright IBM Corporation 2006 > + * Copyright 2009 Red Hat, Inc. > * All Rights Reserved. > * > * Permission is hereby granted, free of charge, to any person obtaining a > @@ -507,4 +508,17 @@ int pci_device_vgaarb_unlock (void); > /* return the current device count + resource decodes for the device */ > int pci_device_vgaarb_get_info (struct pci_device *dev, int *vga_count, > int *rsrc_decodes); > > +/* > + * I/O space access. > + */ > +void *pci_device_open_io(struct pci_device *dev, int bar); I'd make the returned void pointer a struct pci_device_io pointer instead. We don't have to actually define the struct anywhere, and it gives us typesafety and make the API more self-documenting. cheers, Kristian ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/2] I/O port access routines
On Thu, 2009-11-19 at 00:13 +0100, Mark Kettenis wrote:
> > From: Adam Jackson
> > Date: Wed, 18 Nov 2009 14:28:57 -0500
> >
> > Signed-off-by: Adam Jackson
> > ---
> > 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
signature.asc
Description: This is a digitally signed message part
___
xorg-devel mailing list
xorg-devel@lists.x.org
http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/2] I/O port access routines
On Thu, Nov 19, 2009 at 01:45:57AM +0100, ext Luc Verhaegen wrote: > > But the main point of this mail was ignored. Why did existing xorg pci > infrastructure have to be reinvented like that instead of adding a new > backend and fixing up the bad patches? Why did RAC get thrown away like > that? Why does this NIH have to keep on repeating itself? > The main reason is because we were needing a PCI resource broker for the entire system and not for one process only. And X was touching the PCI resources directly with RAC.. oops! We don't need and our 21st century's kernels do it pretty well for us :) Tiago ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/2] I/O port access routines
On Thu, Nov 19, 2009 at 06:06:47PM +0200, Tiago Vignatti wrote: > On Thu, Nov 19, 2009 at 01:45:57AM +0100, ext Luc Verhaegen wrote: > > > > But the main point of this mail was ignored. Why did existing xorg pci > > infrastructure have to be reinvented like that instead of adding a new > > backend and fixing up the bad patches? Why did RAC get thrown away like > > that? Why does this NIH have to keep on repeating itself? > > > > The main reason is because we were needing a PCI resource broker for the > entire system and not for one process only. And X was touching the PCI > resources directly with RAC.. oops! We don't need and our 21st century's > kernels do it pretty well for us :) > > > Tiago You're talking RAC backend here. Once the RAC was initialised to know what resources this hardware needed, it handled everything for the driver without having to care for anything. This for memory, io and vga (a subclass of io). What i see now is that _everything_ got reinvented, instead of having written up a backend for a modern operating system, and maybe adjusting initialisation on the driver level (as part of this info really can be retrieved from the os now). Re-inventing everything is exactly the criticism that libpciaccess got, apart from it having not been tested on any worthwhile subset of hardware. Yet people still seem unable or unwilling to learn from their mistakes. Luc Verhaegen. ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/2] I/O port access routines
On Thu, Nov 19, 2009 at 05:20:41PM +0100, Luc Verhaegen wrote: > On Thu, Nov 19, 2009 at 06:06:47PM +0200, Tiago Vignatti wrote: > > On Thu, Nov 19, 2009 at 01:45:57AM +0100, ext Luc Verhaegen wrote: > > > > > > But the main point of this mail was ignored. Why did existing xorg pci > > > infrastructure have to be reinvented like that instead of adding a new > > > backend and fixing up the bad patches? Why did RAC get thrown away like > > > that? Why does this NIH have to keep on repeating itself? > > > > > > > The main reason is because we were needing a PCI resource broker for the > > entire system and not for one process only. And X was touching the PCI > > resources directly with RAC.. oops! We don't need and our 21st century's > > kernels do it pretty well for us :) > > > > You're talking RAC backend here. Once the RAC was initialised to know > what resources this hardware needed, it handled everything for the > driver without having to care for anything. This for memory, io and vga > (a subclass of io). > > What i see now is that _everything_ got reinvented, instead of having > written up a backend for a modern operating system, and maybe adjusting > initialisation on the driver level (as part of this info really can be > retrieved from the os now). > > Re-inventing everything is exactly the criticism that libpciaccess got, > apart from it having not been tested on any worthwhile subset of > hardware. Yet people still seem unable or unwilling to learn from their > mistakes. > PCI code was removed (well, it's being) from X and needed be put somewhere. libpciaccess was the name of the place. I can imagine how tied and coupled such code was with the rest of the server and it was more easy to just reimplement some parts. So, it was a decision that someone made. But the removal of PCI from X was not all the problem; there was also RAC which, as I said to you, should be visible from other apps. Given, recently kernels are already doing a lot of RAC's job, we decided to just call another name - VGA arbiter - and implement only the necessary parts. I don't see why you're complaining so much... Tiago ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/2] I/O port access routines
On Thu, 2009-11-19 at 00:37 +0100, Luc Verhaegen wrote:
> And i just keep on wondering... Just like with the VGA arbitration...
> Didn't the RAC used wrap all this crap for us _outside_ calls to driver
> code so that we could just access IO and memory directly, without
> having to care at all?
RAC did not handle this correctly when multiple X servers were running;
VGA arb does. The I/O port routines in the server - not strictly part
of RAC - leave the onus of domain safety on the driver, and are a
wretched pile of ifdef (though admittedly a working pile).
The deeper assumption with RAC was that the OS could not be trusted to
give you a working PCI memory map, and that it was okay to rewrite it.
No no no no no.
> How hard would it have been to have written up a
> backend for the RAC for the linux pci code that popped up during the
> active life of RAC? My guess is, much easier than the stuff that has to
> be pulled now, all it took was that someone was willing to go into
> existing code and understand how it work(s/ed).
The Linux PCI code did use RAC. I don't know why you think it didn't?
As someone who's actually had to fix bugs in the PCI code in 6.8:
while:
- the transition to the new library was ungraceful
- the motivation for switching was a complete MacGuffin
- the internal conversion to pciaccess is still incomplete, and
- the completion of VGA arbitration took embarassingly long
I can honestly and without reservation say that the current code is
comprehensible and pleasant to work with, and the old PCI layer has
literally woken me from sleep sweating in terror.
> Oh, and that's before i start wondering about gratuitous void pointers
> and goto.
The handle needs to be opaque to the user, because it's intrinsically
platform-dependent. Kristian's opaque struct name suggestion is
probably what I'll go with.
In this particular case, yeah, the goto should reasonably be thus:
/* First check if there's a legacy io method for the device */
while (dev) {
snprintf(name, PATH_MAX, "/sys/class/pci_bus/%04x:%02x/legacy_io",
dev->domain, dev->bus);
ret = open(name, O_RDWR);
if (ret >= 0)
break;
dev = pci_device_get_parent_bridge(dev);
}
/* If not, /dev/port is the best we can do */
if (!dev)
ret = open("/dev/port", O_RDWR);
if (ret == 0) {
int new = dup(ret);
close(ret);
ret = new;
}
- ajax
signature.asc
Description: This is a digitally signed message part
___
xorg-devel mailing list
xorg-devel@lists.x.org
http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/2] I/O port access routines
On Thu, Nov 19, 2009 at 11:53:16AM -0500, Adam Jackson wrote: > On Thu, 2009-11-19 at 00:37 +0100, Luc Verhaegen wrote: > > > And i just keep on wondering... Just like with the VGA arbitration... > > Didn't the RAC used wrap all this crap for us _outside_ calls to driver > > code so that we could just access IO and memory directly, without > > having to care at all? > > RAC did not handle this correctly when multiple X servers were running; > VGA arb does. The I/O port routines in the server - not strictly part > of RAC - leave the onus of domain safety on the driver, and are a > wretched pile of ifdef (though admittedly a working pile). > > The deeper assumption with RAC was that the OS could not be trusted to > give you a working PCI memory map, and that it was okay to rewrite it. > No no no no no. This was not just RAC, this was just the situation of that time. Now, my understanding of RAC, from a driver point of view, was that you provided it with some information as to what resources your hardware needed, and it would then magically take care of everything for you, before you entered any driver specific call, so you never had to worry about it then. Afaik, instead of fixing up the backend, this nice infrastructure was entirely removed. Because this is really what i care about, as long as it is all behind the scenes, and it has no consequences for me as a driver developer, and it works (without making the mistakes that old solutions fixed), i am happy. Look at this code, it is so close to RAC like behaviour already. There is code to enable and disable io access. Why can we not use IN/OUT[BWL] macros in between like before? Why can enable/disable not wrap driver calls like RAC before? The baby was thrown out with the bathwater with libpciaccess and vga arbitration, and now suddenly the plan seems to be to put these new enable/disable hooks all over the driver, together with slightly more involved IN/OUT calls. Unacceptable. > > How hard would it have been to have written up a > > backend for the RAC for the linux pci code that popped up during the > > active life of RAC? My guess is, much easier than the stuff that has to > > be pulled now, all it took was that someone was willing to go into > > existing code and understand how it work(s/ed). > > The Linux PCI code did use RAC. I don't know why you think it didn't? > > As someone who's actually had to fix bugs in the PCI code in 6.8: > while: > > - the transition to the new library was ungraceful > - the motivation for switching was a complete MacGuffin > - the internal conversion to pciaccess is still incomplete, and > - the completion of VGA arbitration took embarassingly long > > I can honestly and without reservation say that the current code is > comprehensible and pleasant to work with, and the old PCI layer has > literally woken me from sleep sweating in terror. Of course it is, today we can depend on operating systems have useful pci support. Not so then. But there is nothing that stopped people from keeping the upper level infrastructure in place while having reworked the backend to work on top of the now existing nice pci support in the kernel. The only reason to replace the whole thing was rampant NIH. And this rampant NIH is the core cause for the big nasty interface change and RAC being #ifdeffed and commented out, without a replacement, for several years. > > Oh, and that's before i start wondering about gratuitous void pointers > > and goto. > > The handle needs to be opaque to the user, because it's intrinsically > platform-dependent. Kristian's opaque struct name suggestion is > probably what I'll go with. Ah, good, typechecking will still happen then. Thanks. Luc Verhaegen. ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/2] I/O port access routines
On Thu, Nov 19, 2009 at 06:36:15PM +0200, Tiago Vignatti wrote: > > PCI code was removed (well, it's being) from X and needed be put somewhere. > libpciaccess was the name of the place. I can imagine how tied and coupled > such code was with the rest of the server and it was more easy to just > reimplement some parts. So, it was a decision that someone made. > > But the removal of PCI from X was not all the problem; there was also RAC > which, as I said to you, should be visible from other apps. Given, recently > kernels are already doing a lot of RAC's job, we decided to just call another > name - VGA arbiter - and implement only the necessary parts. > > > I don't see why you're complaining so much... > > > Tiago Oh man... This whole logic, when looked at it by a driver developer who just wants to have his driver work, is soo interdependent. Current situation: * We have to wrap io calls in the driver. * Why? * Because we threw out RAC completely. * Why? * Because we no longer had VGA arbitration. * Why? * Because we daftly threw out the old xserver pci code completely and reinvented it, and didn't bother to keep existing APIs nor give RAC support for this new backend code. * Why did we throw out old pci code? * Because the lowlevel code was involved and was not using the operating system infrastructure that popped up "recently". * Why do it that way? * Because it's just how we did it, ok? now stfu. Now i am sure that i am not the only person who frowns upon the logic applied in there, when it is, finally, laid out like this. And Tiago, it was not coupled that tightly. It supported just about every operating system out there at the time (in as far as it supported operating systems itself), and the driver side interface was not fundamentally different from what we have now. For reference, you can go and check any driver that has the compat code. Code is mostly different for the sake of being different. NIH. Luc Verhaegen. ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/2] I/O port access routines
On Thu, Nov 19, 2009 at 06:36:12PM +0100, Luc Verhaegen wrote: > > And Tiago, it was not coupled that tightly. Well, just look at the head of xf86pciBus.c (before RAC be removed and when the old PCI code was in place): #ifdef HAVE_XORG_CONFIG_H #include #endif #include #include #include #include #include "os.h" #include "Pci.h" #include "xf86.h" #include "xf86Priv.h" #include "xf86Resources.h" /* Bus-specific headers */ #include "xf86PciData.h" #include "xf86Bus.h" #define XF86_OS_PRIVS #define NEED_OS_RAC_PROTOS #include "xf86_OSproc.h" #include "xf86RAC.h" It was mix of RAC + PCI + BUS + high level screen informations + X protocol + real-mode things and etc, and you're saying that that code was not coupled? So I _really_ don't know what you mean about tightly, coupled and confusing code... Tiago ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/2] I/O port access routines
On Thu, 2009-11-19 at 18:25 +0100, Luc Verhaegen wrote: > The baby was thrown out with the bathwater with libpciaccess and vga > arbitration, and now suddenly the plan seems to be to put these new > enable/disable hooks all over the driver, together with slightly more > involved IN/OUT calls. The enable/disable thing, no. That wrapping is in the server. You don't have to do it from your driver. You seem to be putting words in my mouth with the in/out stuff though. I haven't actually said anything (yet) about whether the drivers may continue to use the old in/out calls. All I've done here is add API to the hardware access library to let me get at the third of the three address spaces the hardware exposes. Now, drivers _ought_ not use the old API. And I'm entirely willing to fix the drivers to use the new API conditional on its presence. But if you really want the freedom to make your driver continue to not work with multiple PCI domains, then I guess that's a choice you can make. > But there is nothing that stopped people from keeping the upper level > infrastructure in place while having reworked the backend to work on top > of the now existing nice pci support in the kernel. The only reason > to replace the whole thing was rampant NIH. > > And this rampant NIH is the core cause for the big nasty interface > change and RAC being #ifdeffed and commented out, without a replacement, > for several years. You're using "NIH" as something of a scarlet letter here. That's a thing you can do, although it's sort of comical. It might even be a true description of the motivation, for all it matters. You're also conflating the RAC and PCI layers a bit more than is deserved. And I should point out (again) that I do _not_ think the transition was handled well. But, in the end, I think you're mistaken about the practicality of "reworking the backend". Taking off and nuking the site from orbit really was the right move. All of which is lovely parlour conversation, but doesn't really address the patch. I have this feature in mind. I wrote some code to implement it. I think it would make life better. Are you suggesting that it would make life worse? - ajax signature.asc Description: This is a digitally signed message part ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/2] I/O port access routines
> From: Adam Jackson > Date: Thu, 19 Nov 2009 13:58:52 -0500 > > On Thu, 2009-11-19 at 18:25 +0100, Luc Verhaegen wrote: > > > The baby was thrown out with the bathwater with libpciaccess and vga > > arbitration, and now suddenly the plan seems to be to put these new > > enable/disable hooks all over the driver, together with slightly more > > involved IN/OUT calls. > > The enable/disable thing, no. That wrapping is in the server. You > don't have to do it from your driver. > > You seem to be putting words in my mouth with the in/out stuff though. > I haven't actually said anything (yet) about whether the drivers may > continue to use the old in/out calls. All I've done here is add API to > the hardware access library to let me get at the third of the three > address spaces the hardware exposes. While I'm sympathetic too many of the things Luc is saying, Adam is right here. The new interfaces will allow you to access PCI I/O BARs (and only that particular I/O BAR) and do so in a way that also works right for machines that have multiple PCI domains. That is something the old code simply didn't support. This patch is (almost) exactly the functionality I need to make the i128 driver work on OpenBSD/sparc64. ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/2] I/O port access routines
> From: Adam Jackson
> Date: Thu, 19 Nov 2009 10:23:49 -0500
>
> On Thu, 2009-11-19 at 00:13 +0100, Mark Kettenis wrote:
> > > From: Adam Jackson
> > > Date: Wed, 18 Nov 2009 14:28:57 -0500
> > >
> > > Signed-off-by: Adam Jackson
> > > ---
> > > 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
Re: [PATCH 2/2] I/O port access routines
On Thu, Nov 19, 2009 at 01:58:52PM -0500, Adam Jackson wrote: > On Thu, 2009-11-19 at 18:25 +0100, Luc Verhaegen wrote: > > > The baby was thrown out with the bathwater with libpciaccess and vga > > arbitration, and now suddenly the plan seems to be to put these new > > enable/disable hooks all over the driver, together with slightly more > > involved IN/OUT calls. > > The enable/disable thing, no. That wrapping is in the server. You > don't have to do it from your driver. > > You seem to be putting words in my mouth with the in/out stuff though. > I haven't actually said anything (yet) about whether the drivers may > continue to use the old in/out calls. All I've done here is add API to > the hardware access library to let me get at the third of the three > address spaces the hardware exposes. Oh, so now you _are_ threatening to impose some new API on drivers? Hah. Let me quote part of another mail: > * Why do it that way? > * Because it's just how we did it, ok? now stfu. > Now, drivers _ought_ not use the old API. And I'm entirely willing to > fix the drivers to use the new API conditional on its presence. But if > you really want the freedom to make your driver continue to not work > with multiple PCI domains, then I guess that's a choice you can make. Well, my out of tree driver has long stopped using direct IO, so i can take away your enjoyment there already. > You're using "NIH" as something of a scarlet letter here. That's a > thing you can do, although it's sort of comical. It might even be a > true description of the motivation, for all it matters. You're also > conflating the RAC and PCI layers a bit more than is deserved. And I > should point out (again) that I do _not_ think the transition was > handled well. > > But, in the end, I think you're mistaken about the practicality of > "reworking the backend". Taking off and nuking the site from orbit > really was the right move. > > All of which is lovely parlour conversation, but doesn't really address > the patch. I have this feature in mind. I wrote some code to implement > it. I think it would make life better. Are you suggesting that it > would make life worse? > > - ajax Oh, i have no doubt that your patch is making _current_ life better, but it once again exposes the huge shortsighted mistake made several years ago. Way more problems were created than were fixed. Maybe I should blame myself for all of it. If i had been more lazy, then the ati probe code would still be a mess. And then maybe no-one would've accepted libpciaccess in that shape until finally someone else came round with big enough guts and clue to go and fix up the ati wrapper like that. But don't expect me to shut up about NIH. It describes quite a lot of the behaviour of parts of the X.org community. But i of course do not expect you to acknowledge that. Luc Verhaegen. ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
