Re: libpciaccess x86 backend
Corbin Simpson, le Tue 19 Jan 2010 13:26:47 -0800, a écrit : > It doesn't seem that exotic to want a PCI layer, and wasn't the entire > point of libpciaccess to keep PCI code out of the Xserver? I'm afraid I'm not sure which point you want to make. An exotic kernel would precisely _not_ want to have to fiddle with PCI intrinsics. Also, putting x86 code into libpciaccess in the case there is not PCI layer still keeps the PCI code out of Xserver itself, in a way. Samuel ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: libpciaccess x86 backend
It doesn't seem that exotic to want a PCI layer, and wasn't the entire point of libpciaccess to keep PCI code out of the Xserver? On Tue, Jan 19, 2010 at 12:53 PM, Samuel Thibault wrote: > Dave Airlie, le Wed 20 Jan 2010 06:42:30 +1000, a écrit : >> On Mon, Jan 18, 2010 at 12:37 AM, Samuel Thibault >> wrote: >> > This adds support on x86 for OSes that do not have a PCI interface, >> > tinkering >> > with I/O ports, and makes use of it on GNU/Hurd. >> > >> >> Now this might be a dumb question but how to hurd drivers interact >> with the PCI layer, they all just bang on it directly? > > There is no Hurd PCI driver (yet). > > The only PCI code being used is a temporary glue into GNU Mach. > > Yes, an interface between that and userspace should be designed, etc. > But since it's a temporary glue, it's not really worth doing it right > now. Still people would like to already try Xorg. > >> It just seems lazy to add a direct to hw code again in userspace when >> clearly the kernel should be able for it, > > Depends on the kernel. A kernel could very well not deal with PCI at > all and let userspace agree on using a "PCI server", that could for > instance use libpciaccess. > >> not that I really mind, I'm just hoping other OSes don't get this >> lazy, which marking this code as generic x86 might make them. > > I understand your concern. I still believe it is more helpful to provide > this in the interim than to let yet another barrier from Xorg support > for exotic kernels. > > Samuel > ___ > xorg mailing list > xorg@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/xorg > -- Only fools are easily impressed by what is only barely beyond their reach. ~ Unknown Corbin Simpson ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: libpciaccess x86 backend
Dave Airlie, le Wed 20 Jan 2010 06:42:30 +1000, a écrit : > On Mon, Jan 18, 2010 at 12:37 AM, Samuel Thibault > wrote: > > This adds support on x86 for OSes that do not have a PCI interface, > > tinkering > > with I/O ports, and makes use of it on GNU/Hurd. > > > > Now this might be a dumb question but how to hurd drivers interact > with the PCI layer, they all just bang on it directly? There is no Hurd PCI driver (yet). The only PCI code being used is a temporary glue into GNU Mach. Yes, an interface between that and userspace should be designed, etc. But since it's a temporary glue, it's not really worth doing it right now. Still people would like to already try Xorg. > It just seems lazy to add a direct to hw code again in userspace when > clearly the kernel should be able for it, Depends on the kernel. A kernel could very well not deal with PCI at all and let userspace agree on using a "PCI server", that could for instance use libpciaccess. > not that I really mind, I'm just hoping other OSes don't get this > lazy, which marking this code as generic x86 might make them. I understand your concern. I still believe it is more helpful to provide this in the interim than to let yet another barrier from Xorg support for exotic kernels. Samuel ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: libpciaccess x86 backend
On Mon, Jan 18, 2010 at 12:37 AM, Samuel Thibault wrote: > Hello, > > This adds support on x86 for OSes that do not have a PCI interface, tinkering > with I/O ports, and makes use of it on GNU/Hurd. > Now this might be a dumb question but how to hurd drivers interact with the PCI layer, they all just bang on it directly? It just seems lazy to add a direct to hw code again in userspace when clearly the kernel should be able for it, not that I really mind, I'm just hoping other OSes don't get this lazy, which marking this code as generic x86 might make them. Dave. ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: libpciaccess x86 backend
Hi,
I put some minor comments bellow. Make sure you send your future patches to
xorg-de...@lists.freedesktop.org instead.
On Sun, Jan 17, 2010 at 06:30:00PM +0100, ext Samuel Thibault wrote:
>
> This adds support on x86 for OSes that do not have a PCI interface,
> tinkering with I/O ports, and makes use of it on GNU/Hurd.
>
> diff --git a/configure.ac b/configure.ac
> index ffc1c8d..535d704 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -90,6 +90,9 @@ case $host_os in
> solaris=yes
> PCIACCESS_LIBS="$PCIACCESS_LIBS -ldevinfo"
> ;;
> + gnu*)
> + gnu=yes
> + ;;
> esac
>
> AM_CONDITIONAL(LINUX, [test "x$linux" = xyes])
> @@ -97,6 +100,7 @@ AM_CONDITIONAL(FREEBSD, [test "x$freebsd" = xyes])
> AM_CONDITIONAL(NETBSD, [test "x$netbsd" = xyes])
> AM_CONDITIONAL(OPENBSD, [test "x$openbsd" = xyes])
> AM_CONDITIONAL(SOLARIS, [test "x$solaris" = xyes])
> +AM_CONDITIONAL(GNU, [test "x$gnu" = xyes])
>
> AC_SYS_LARGEFILE
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 4c06c25..8afd53b 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -51,6 +51,10 @@ else
> VGA_ARBITER = common_vgaarb_stub.c
> endif
>
> +if GNU
> +OS_SUPPORT = x86_pci.c
> +endif
change the name for gnuhurd_pci.c makes more sense for me, given all file
there now are OS related.
> libpciaccess_la_SOURCES = common_bridge.c \
> common_iterator.c \
> common_init.c \
> diff --git a/src/common_init.c b/src/common_init.c
> index 6b9b936..89c56ad 100644
> --- a/src/common_init.c
> +++ b/src/common_init.c
> @@ -62,6 +62,8 @@ pci_system_init( void )
> err = pci_system_openbsd_create();
> #elif defined(__sun)
> err = pci_system_solx_devfs_create();
> +#elif defined(__GNU__)
> +err = pci_system_x86_create();
I think the same name idea applies for all functions. So I'd prefer something
like pci_system_gnuhurd_create(), etc.
> #endif
>
> return err;
> diff --git a/src/pciaccess_private.h b/src/pciaccess_private.h
> index 77eb57b..ef0 100644
> --- a/src/pciaccess_private.h
> +++ b/src/pciaccess_private.h
> @@ -167,4 +167,5 @@ extern int pci_system_netbsd_create( void );
> extern int pci_system_openbsd_create( void );
> extern void pci_system_openbsd_init_dev_mem( int );
> extern int pci_system_solx_devfs_create( void );
> +extern int pci_system_x86_create( void );
> extern void pci_io_cleanup( void );
> diff --git a/src/x86_pci.c b/src/x86_pci.c
> new file mode 100644
> index 000..b5725ef
> --- /dev/null
> +++ b/src/x86_pci.c
> @@ -0,0 +1,669 @@
> +/*
> + * Copyright (c) 2009 Samuel Thibault
> + * Heavily inspired from the freebsd, netbsd, and openbsd backends
> + * (C) Copyright Eric Anholt 2006
> + * (C) Copyright IBM Corporation 2006
> + * Copyright (c) 2008 Juan Romero Pardines
> + * Copyright (c) 2008 Mark Kettenis
> + *
> + * Permission to use, copy, modify, and distribute this software for any
> + * purpose with or without fee is hereby granted, provided that the above
> + * copyright notice and this permission notice appear in all copies.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
> +
> +#define _GNU_SOURCE
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "pciaccess.h"
> +#include "pciaccess_private.h"
> +
> +#if defined(__GNU__)
> +
> +#include
> +
> +static int
> +x86_enable_io(void)
> +{
> +if (!ioperm(0, 0x, 1))
> +return 0;
> +return errno;
> +}
> +
> +static int
> +x86_disable_io(void)
> +{
> +if (!ioperm(0, 0x, 0))
> +return 0;
> +return errno;
> +}
> +
> +#elif defined(__GLIBC__)
> +
> +#include
I'd put hearders on the top...
> +
> +static int
> +x86_enable_io(void)
> +{
> +if (!iopl(3))
> +return 0;
> +return errno;
> +}
> +
> +static int
> +x86_disable_io(void)
> +{
> +if (!iopl(0))
> +return 0;
> +return errno;
> +}
> +
> +#else
> +
> +#error How to enable IO ports on this system?
> +
> +#endif
> +
> +#define PCI_VENDOR(reg)((reg) & 0x)
> +#define PCI_VENDOR_INVALID 0x
> +
> +#define PCI_VENDOR_ID 0x00
> +#define PCI_SUB_VENDOR_ID 0x2c
> +#define PCI_VENDOR_ID_COMPAQ 0x0e11
> +#define PCI_VENDOR_ID_INTEL0x8086
> +
> +#define PCI_DEVICE(reg)(((reg) >> 16) & 0x)
> +#define PCI_DEVICE_INVALID 0x
> +
> +#define PCI_CLASS 0x08
> +#define PCI
Re: libpciaccess x86 backend
Here is a fixed patch
Samuel
This adds support on x86 for OSes that do not have a PCI interface,
tinkering with I/O ports, and makes use of it on GNU/Hurd.
diff --git a/configure.ac b/configure.ac
index ffc1c8d..535d704 100644
--- a/configure.ac
+++ b/configure.ac
@@ -90,6 +90,9 @@ case $host_os in
solaris=yes
PCIACCESS_LIBS="$PCIACCESS_LIBS -ldevinfo"
;;
+ gnu*)
+ gnu=yes
+ ;;
esac
AM_CONDITIONAL(LINUX, [test "x$linux" = xyes])
@@ -97,6 +100,7 @@ AM_CONDITIONAL(FREEBSD, [test "x$freebsd" = xyes])
AM_CONDITIONAL(NETBSD, [test "x$netbsd" = xyes])
AM_CONDITIONAL(OPENBSD, [test "x$openbsd" = xyes])
AM_CONDITIONAL(SOLARIS, [test "x$solaris" = xyes])
+AM_CONDITIONAL(GNU, [test "x$gnu" = xyes])
AC_SYS_LARGEFILE
diff --git a/src/Makefile.am b/src/Makefile.am
index 4c06c25..8afd53b 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -51,6 +51,10 @@ else
VGA_ARBITER = common_vgaarb_stub.c
endif
+if GNU
+OS_SUPPORT = x86_pci.c
+endif
+
libpciaccess_la_SOURCES = common_bridge.c \
common_iterator.c \
common_init.c \
diff --git a/src/common_init.c b/src/common_init.c
index 6b9b936..89c56ad 100644
--- a/src/common_init.c
+++ b/src/common_init.c
@@ -62,6 +62,8 @@ pci_system_init( void )
err = pci_system_openbsd_create();
#elif defined(__sun)
err = pci_system_solx_devfs_create();
+#elif defined(__GNU__)
+err = pci_system_x86_create();
#endif
return err;
diff --git a/src/pciaccess_private.h b/src/pciaccess_private.h
index 77eb57b..ef0 100644
--- a/src/pciaccess_private.h
+++ b/src/pciaccess_private.h
@@ -167,4 +167,5 @@ extern int pci_system_netbsd_create( void );
extern int pci_system_openbsd_create( void );
extern void pci_system_openbsd_init_dev_mem( int );
extern int pci_system_solx_devfs_create( void );
+extern int pci_system_x86_create( void );
extern void pci_io_cleanup( void );
diff --git a/src/x86_pci.c b/src/x86_pci.c
new file mode 100644
index 000..b5725ef
--- /dev/null
+++ b/src/x86_pci.c
@@ -0,0 +1,669 @@
+/*
+ * Copyright (c) 2009 Samuel Thibault
+ * Heavily inspired from the freebsd, netbsd, and openbsd backends
+ * (C) Copyright Eric Anholt 2006
+ * (C) Copyright IBM Corporation 2006
+ * Copyright (c) 2008 Juan Romero Pardines
+ * Copyright (c) 2008 Mark Kettenis
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#define _GNU_SOURCE
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "pciaccess.h"
+#include "pciaccess_private.h"
+
+#if defined(__GNU__)
+
+#include
+
+static int
+x86_enable_io(void)
+{
+if (!ioperm(0, 0x, 1))
+return 0;
+return errno;
+}
+
+static int
+x86_disable_io(void)
+{
+if (!ioperm(0, 0x, 0))
+return 0;
+return errno;
+}
+
+#elif defined(__GLIBC__)
+
+#include
+
+static int
+x86_enable_io(void)
+{
+if (!iopl(3))
+return 0;
+return errno;
+}
+
+static int
+x86_disable_io(void)
+{
+if (!iopl(0))
+return 0;
+return errno;
+}
+
+#else
+
+#error How to enable IO ports on this system?
+
+#endif
+
+#define PCI_VENDOR(reg)((reg) & 0x)
+#define PCI_VENDOR_INVALID 0x
+
+#define PCI_VENDOR_ID 0x00
+#define PCI_SUB_VENDOR_ID 0x2c
+#define PCI_VENDOR_ID_COMPAQ 0x0e11
+#define PCI_VENDOR_ID_INTEL0x8086
+
+#define PCI_DEVICE(reg)(((reg) >> 16) & 0x)
+#define PCI_DEVICE_INVALID 0x
+
+#define PCI_CLASS 0x08
+#define PCI_CLASS_DEVICE 0x0a
+#define PCI_CLASS_DISPLAY_VGA 0x0300
+#define PCI_CLASS_BRIDGE_HOST 0x0600
+
+#definePCIC_DISPLAY0x03
+#definePCIS_DISPLAY_VGA0x00
+
+#define PCI_HDRTYPE0x0E
+#define PCI_IRQ0x3C
+
+struct pci_system_x86 {
+struct pci_system system;
+int (*read)(unsigned bus, unsigned dev, unsigned func, pciaddr_t reg, void
*data, unsigned size);
+int (*write)(unsigned bus, unsigned dev, unsigned func, pciaddr_t reg,
const void *data, unsigned size);
+};
+
+static int
+pci_x86_conf1_probe(void)
+{
+unsigned long sav;
+int res = ENODEV;
+
+outb(0x01, 0xCFB);
+sav = inl(0xCF8);
+outl(0x8000, 0xCF8);
+if (inl(0xCF8)
Re: libpciaccess x86 backend
Hello,
Alexander E. Patrakov, le Sun 17 Jan 2010 21:42:38 +0500, a écrit :
> [If this review is stupid, disregard - I know nothing about PCI internals]
Thanks for your review, it's wasn't stupid at all, it's always good to
make sure even cases that are not supposed to happen are handled. I'll
send a fixed patch.
> 17.01.2010 19:37, Samuel Thibault wrote:
> >+sav = inl(0xCF8);
> >+outl(0x8000 | (bus<< 16) | (dev<< 11) | (func<< 8) | (reg&
> >~3), 0xCF8);
> >+switch (size) {
> ...
> >+default:
> >+ret = EIO;
> >+break;
> >+}
> >+outl(sav, 0xCF8);
>
> Here a read or write request that can be rejected right away (wrong
> size) still leads to I/O port access. Is it OK?
The I/O is harmless, but it's better to not even do it indeed.
> >+printf("unknown header type %02x\n", header_type);
>
> printf to stdout in a library? Shouldn't that better go to stderr?
Indeed.
> >+static int
> >+get_test_val_size( uint32_t testval )
>
> I understand that nobody is going to pass 0x8000 here as testval,
> but, should this and the return value be unsigned nevertheless?
Why not indeed.
> >+static int
> >+pci_device_x86_read(struct pci_device *dev, void *data,
>
> Can't it happen that toread == 3 here? Or is this an invalid request?
It's not really supposed to happen, but it possibly could indeed.
> >+if (ret)
> >+return ret;
>
> and stay with io enabled and pci_sys_x86 being non-freed?
Oops.
> >+pci_sys->devices = calloc(ndevs, sizeof(struct pci_device_private));
> >+if (pci_sys->devices == NULL) {
> >+x86_disable_io();
> >+free(pci_sys);
>
> Is this supposed to be free(pci_sys_x86)?
Yes, actually they are the same.
Samuel
___
xorg mailing list
xorg@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/xorg
Re: libpciaccess x86 backend
[If this review is stupid, disregard - I know nothing about PCI internals]
17.01.2010 19:37, Samuel Thibault wrote:
> +sav = inl(0xCF8);
> +outl(0x8000 | (bus<< 16) | (dev<< 11) | (func<< 8) | (reg& ~3),
> 0xCF8);
> +/* NOTE: x86 is already LE */
> +switch (size) {
...
> + default:
> + ret = EIO;
> + break;
> +}
> +outl(sav, 0xCF8);
Here a read or write request that can be rejected right away (wrong
size) still leads to I/O port access. Is it OK?
> + printf("unknown header type %02x\n", header_type);
printf to stdout in a library? Shouldn't that better go to stderr? (btw,
there is another case in src/common_capability.c)
> +/** Returns the size of a region based on the all-ones test value */
> +static int
> +get_test_val_size( uint32_t testval )
> +{
> +int size = 1;
I understand that nobody is going to pass 0x8000 here as testval,
but, should this and the return value be unsigned nevertheless?
> +static int
> +pci_device_x86_read(struct pci_device *dev, void *data,
> +pciaddr_t offset, pciaddr_t size, pciaddr_t *bytes_read)
> +{
> +struct pci_system_x86 *pci_sys_x86 = (struct pci_system_x86 *) pci_sys;
> +int err;
> +
> +*bytes_read = 0;
> +while (size> 0) {
> + int toread = 4;
> + if (toread> size)
> + toread = size;
> + if (toread> 4 - (offset& 0x3))
> + toread = 4 - (offset& 0x3);
Can't it happen that toread == 3 here? Or is this an invalid request?
> +_pci_hidden int
> +pci_system_x86_create(void)
> +{
> +struct pci_device_private *device;
> +int ret, bus, dev, ndevs, func, nfuncs;
> +struct pci_system_x86 *pci_sys_x86;
> +uint32_t reg;
> +
> +ret = x86_enable_io();
> +if (ret)
> + return ret;
> +
> +pci_sys_x86 = calloc(1, sizeof(struct pci_system_x86));
> +if (pci_sys_x86 == NULL) {
> + x86_disable_io();
> + return ENOMEM;
> +}
> +pci_sys =&pci_sys_x86->system;
> +
> +ret = pci_probe(pci_sys_x86);
> +if (ret)
> + return ret;
and stay with io enabled and pci_sys_x86 being non-freed?
> +pci_sys->devices = calloc(ndevs, sizeof(struct pci_device_private));
> +if (pci_sys->devices == NULL) {
> + x86_disable_io();
> + free(pci_sys);
Is this supposed to be free(pci_sys_x86)?
--
Alexander E. Patrakov
___
xorg mailing list
xorg@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/xorg
