libpciaccess 0.13.1 FreeBSD Patch to fix xorg-server-1.12.4 (and later)

Submitted by John Wehle

Assigned to Xorg Project Team

Link to original bug (#63583)

Description

Created attachment 78051 Patch

On xorg-server-1.11.4 works fine with a Radeon 7850 running with the VESA driver on FreeBSD 9.1. Upgrading to xorg-server-1.12.4 causes the X11 server to crash during startup when int10 / VBE is being initialized.

Tracing through the code shows the problem is cause by libpciaccess missing routines for implementing legacy I/O on FreeBSD. With the enclosed patch applied xorg-server-1.12.4 passes a quick smoke test.

This fix has only be tested on an i386 platform.

Attachment 78051, "Patch":
freebsd_pci.patch

Version: 7.7 (2012.06)

Niclas Zeising submitted a patch:

Patch 78063, "Patch to add legacy IO routines for FreeBSD":
patch-src-freebsd_pci.c

Niclas Zeising said:

My text somehow disappeared when attaching the patch. This patch is a better more complete solution to the above problem. It has been in use for quite some time, and is tested on both i386 and amd64 (x86_64) platform, possibly others.

Niclas Zeising submitted a patch:

Here is an updated patch, which fixes a typo.

Patch 78130, "Implement legacy IO routines for FreeBSD":
patch-src-freebsd_pci.c

Alan Coopersmith @alanc said:

Could you please provide Signed-off-by tags for the authors?

http://www.x.org/wiki/Development/Documentation/SubmittingPatches#Signing_off_and_reviewing

(git format-patch format, with the commit comments and signature tags is preferred, instead of raw diffs, but we can make up a comment if needed.)

Mark Kettenis @kettenis said:

Patch looks ok to me. Not sure if FreeBSD has PCI_MAGIC_IO_RANGE on any of its supported architectures yet.

Reviewed-by: Mark Kettenis kettenis@openbsd.org

Niclas Zeising said:

FreeBSD does not have PCI_MAGIC_IO_RANGE as far as I can tell, but the patch originally comes from the debian/kFreeBSD project, although it has been slightly modified locally by Jung-uk Kim (jkim@FreeBSD.org). I don't know if this should be added to the copyright notice as well. I assumed not, since it was not done when jkim committed the changes. I have send mail to both Jung-un Kim and Robert Millan asking for their review and sign-off.

Jung-uk Kim said:

I don't think my modifications are copyrightable.

Signed-off-by: Jung-uk Kim jkim@FreeBSD.org

Chí-Thanh Christopher Nguyễn said:

I ran into this today on Gentoo amd64-fbsd, the patch from attachment 78130 fixes it. What is the current status of this patch?

Alan Coopersmith @alanc said:

Since a revised patch in "git format-patch" style wasn't provided, I have cobbled together a commit message from the previous bug comments and pushed to git master:

To ssh://git.freedesktop.org/git/xorg/lib/libpciaccess d76fb36d..434cd73d master -> master

Robert Millan said:

Hi,

FWIW, my original patch (http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=669062#55 ) is

Signed-off-by: Robert Millan rmh@freebsd.org

However, I don't understand why the unambigous calls to bus_space_write_* family of functions were replaced by out*().

Please note that using out*() in userland namespace is dangerous, it can lead to undefined I/O behaviour!!

The problem is that different implementations use different argument order. E.g.

FreeBSD code: outw(port, data); Glibc code: outw(data, port);

As GNU/kFreeBSD is an hybrid system, it needs to provide both. In different headers of course.

You can also read about the discussion in:

http://lists.freebsd.org/pipermail/freebsd-arch/2012-March/012470.html

So please, could you use my original patch instead?

Robert Millan submitted a patch:

Patch 80968, "same thing, using bus_space_*":
pciaccess.diff

Niclas Zeising said:

You can also read about the discussion in:

http://lists.freebsd.org/pipermail/freebsd-arch/2012-March/012470.html

I will have to look through this discussion, I was not aware that it existed.

So please, could you use my original patch instead?

I unfortunately know very little about how libpciaccess works, only that it does work. Another FreeBSD developer changed your patch to use in*/out* with the motivation that (from the commit log): "Replace bus_space(9) functions with native CPU instructions. These KPIs should never be used in user space. Also, this simplifies the patch a bit." I'll ask him or someone else with more kernel knowledge, to take another look.

Mark Kettenis @kettenis said:

The "9" in bus_space(9) is a giveaway. This is a kernel-only API. The fact that it works in userland at all is an artifact of the i386/amd64-specific implementation, where bus_space_tag_t is a simple integer. On other systems, such as sparc64 and mips, it is a more complex type and these interfaces cannot possibly work outside the kernel.

The outb/outb/outl functions are pretty much standard in the various BSDs, and are intended to be used outside the kernel at least on OpenBSD and NetBSD. The kernel is supposed to use bus_space(9) after all. Just the header files from where to get them from aren't really standardized (and it seems NetBSD recently changed its mind on where to put them).

I don't see an issue to be solved. If there is an actual problem on GNU/kFreeBSD, it should provide a proper <machine/cpufunc.h>.

mentioned in issue #5 (closed)

closed