Bug 1201877

Summary: xorg-x11-drv-qxl is FTBFS on aarch64
Product: [Fedora] Fedora Reporter: Peter Robinson <pbrobinson>
Component: xorg-x11-serverAssignee: X/OpenGL Maintenance List <xgl-maint>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: rawhideCC: alon, cfergeau, crobinso, hdegoede, marcandre.lureau, mjuszkie, pbrobinson, pschindl, robatino, sandmann, virt-maint, xgl-maint
Target Milestone: ---   
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard: RejectedBlocker
Fixed In Version: xorg-x11-drv-qxl-0.1.4-2.fc23 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2015-05-15 15:36:03 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 922257, 1221328    
Attachments:
Description Flags
spec+patch
none
test case
none
hack for xserver package to add in/out stubs for AArch64
none
patch + spec changeset none

Description Peter Robinson 2015-03-13 17:33:21 UTC
It's previously built, but we're not getting the following errors:

http://arm.koji.fedoraproject.org/koji/taskinfo?taskID=2919445

libtool: compile:  gcc -DHAVE_CONFIG_H -I. -I.. -I/usr/include/spice-1 -fvisibility=hidden -I/usr/include/xorg -I/usr/include/pixman-1 -I/usr/include/libdrm -I/usr/include/X11/dri -Wall -Wpointer-arith -Wmissing-declarations -Wformat=2 -Wstrict-prototypes -Wmissing-prototypes -Wnested-externs -Wbad-function-cast -Wold-style-definition -Wdeclaration-after-statement -Wunused -Wuninitialized -Wshadow -Wmissing-noreturn -Wmissing-format-attribute -Wredundant-decls -Wlogical-op -Werror=implicit -Werror=nonnull -Werror=init-self -Werror=main -Werror=missing-braces -Werror=sequence-point -Werror=return-type -Werror=trigraphs -Werror=array-bounds -Werror=write-strings -Werror=address -Werror=int-to-pointer-cast -Werror=pointer-to-int-cast -fno-strict-aliasing -I/usr/include/libdrm -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -c qxl_driver.c  -fPIC -DPIC -o .libs/qxl_driver.o
In file included from qxl_driver.c:45:0:
qxl.h: In function 'ioport_write':
qxl.h:637:5: error: implicit declaration of function 'outb' [-Werror=implicit-function-declaration]
     outb(qxl->io_base + port, val);
     ^
qxl.h:637:5: warning: nested extern declaration of 'outb' [-Wnested-externs]
qxl_driver.c: In function 'qxl_map_memory':
qxl_driver.c:386:36: warning: format '%d' expects argument of type 'int', but argument 5 has type 'uint32_t {aka unsigned int}' [-Wformat=]
     xf86DrvMsg (scrnIndex, X_INFO, "framebuffer at %p (%d KB)\n",
                                    ^
qxl_driver.c:389:36: warning: format '%d' expects argument of type 'int', but argument 5 has type 'uint32_t {aka unsigned int}' [-Wformat=]
     xf86DrvMsg (scrnIndex, X_INFO, "command ram at %p (%d KB)\n",
                                    ^
qxl_driver.c: In function 'qxl_screen_init':
qxl_driver.c:715:13: warning: format '%d' expects argument of type 'int', but argument 2 has type 'uint32_t {aka unsigned int}' [-Wformat=]
     printf ("ram_header at %d\n", qxl->rom->ram_header_offset);
             ^
qxl_driver.c:716:13: warning: format '%d' expects argument of type 'int', but argument 2 has type 'uint32_t {aka unsigned int}' [-Wformat=]
     printf ("surf0 size: %d\n", qxl->rom->surface0_area_size);
             ^
qxl_driver.c: In function 'print_modes':
qxl_driver.c:931:14: warning: format '%d' expects argument of type 'int', but argument 4 has type 'uint32_t {aka unsigned int}' [-Wformat=]
              "%d: %dx%d, %d bits, stride %d, %dmm x %dmm, orientation %d\n",
              ^
qxl_driver.c:931:14: warning: format '%d' expects argument of type 'int', but argument 5 has type 'uint32_t {aka unsigned int}' [-Wformat=]
qxl_driver.c:931:14: warning: format '%d' expects argument of type 'int', but argument 6 has type 'uint32_t {aka unsigned int}' [-Wformat=]
qxl_driver.c:931:14: warning: format '%d' expects argument of type 'int', but argument 7 has type 'uint32_t {aka unsigned int}' [-Wformat=]
qxl_driver.c:931:14: warning: format '%d' expects argument of type 'int', but argument 8 has type 'uint32_t {aka unsigned int}' [-Wformat=]
qxl_driver.c:931:14: warning: format '%d' expects argument of type 'int', but argument 9 has type 'uint32_t {aka unsigned int}' [-Wformat=]
qxl_driver.c:931:14: warning: format '%d' expects argument of type 'int', but argument 10 has type 'uint32_t {aka unsigned int}' [-Wformat=]
qxl_driver.c:931:14: warning: format '%d' expects argument of type 'int', but argument 11 has type 'uint32_t {aka unsigned int}' [-Wformat=]
qxl_driver.c: In function 'qxl_check_device':
qxl_driver.c:953:36: warning: format '%d' expects argument of type 'int', but argument 4 has type 'uint32_t {aka unsigned int}' [-Wformat=]
     xf86DrvMsg (scrnIndex, X_INFO, "Device version %d.%d\n",
                                    ^
qxl_driver.c:953:36: warning: format '%d' expects argument of type 'int', but argument 5 has type 'uint32_t {aka unsigned int}' [-Wformat=]
qxl_driver.c:956:36: warning: format '%d' expects argument of type 'int', but argument 4 has type 'uint32_t {aka unsigned int}' [-Wformat=]
     xf86DrvMsg (scrnIndex, X_INFO, "Compression level %d, log level %d\n",
                                    ^
qxl_driver.c:956:36: warning: format '%d' expects argument of type 'int', but argument 5 has type 'uint32_t {aka unsigned int}' [-Wformat=]
qxl_driver.c:960:36: warning: format '%d' expects argument of type 'int', but argument 4 has type 'uint32_t {aka unsigned int}' [-Wformat=]
     xf86DrvMsg (scrnIndex, X_INFO, "%d io pages at 0x%lx\n",
                                    ^
qxl_driver.c: In function 'qxl_pre_init_common':
qxl_driver.c:1007:39: warning: format '%d' expects argument of type 'int', but argument 4 has type 'uint32_t {aka unsigned int}' [-Wformat=]
         xf86DrvMsg(scrnIndex, X_INFO, "Deferred FPS: %d\n", qxl->deferred_fps);
                                       ^
qxl_driver.c: In function 'qxl_pre_init':
qxl_driver.c:1114:36: warning: format '%d' expects argument of type 'int', but argument 4 has type 'uint32_t {aka unsigned int}' [-Wformat=]
     xf86DrvMsg (scrnIndex, X_INFO, "%d surfaces\n", qxl->rom->n_surfaces);
                                    ^
cc1: some warnings being treated as errors
Makefile:731: recipe for target 'qxl_driver.lo' failed

Comment 1 Christophe Fergeau 2015-03-16 09:36:23 UTC
0.1.3 did build in the past:
http://arm.koji.fedoraproject.org//packages/xorg-x11-drv-qxl/0.1.3/1.fc22/data/logs/aarch64/build.log
There were few enough patches to the package since that build that this looks like it is caused by some external changes (?)

Comment 2 Marcin Juszkiewicz 2015-03-18 09:59:24 UTC
Created attachment 1003118 [details]
spec+patch

Build failed due to lack of outb() so I checked X11 headers. Attached patch is a hack which allows to build that package but I am not quite sure is it the proper way of doing it.

Last built version was with xorg-xserver 1.16 so maybe some headers cleanup happened in meantime which changed situation for aarch64.

Comment 3 Christophe Fergeau 2015-03-18 10:18:58 UTC
Created attachment 1003124 [details]
test case

Is this testcase failing to build on ARM? (when xorg-x11-server-devel is installed). If yes, this is something which should be fixed on the xorg side in my opinion.

Comment 4 Christophe Fergeau 2015-03-18 10:24:26 UTC
compiler.h has:

#elif defined(__arm__) && defined(__linux__)

/* for Linux on ARM, we use the LIBC inx/outx routines */
/* note that the appropriate setup via "ioperm" needs to be done */
/*  *before* any inx/outx is done. */

I don't know if __arm__ is defined for aarch64 nor if the same code path should be used for aarch64?

Comment 5 Peter Robinson 2015-03-18 10:39:20 UTC
> I don't know if __arm__ is defined for aarch64 nor if the same code path
> should be used for aarch64?

It's not, and whether the same code path should be used is it depends, some things are the same, some are not

Comment 6 Marcin Juszkiewicz 2015-03-18 11:13:49 UTC
(In reply to Christophe Fergeau from comment #3)

> Is this testcase failing to build on ARM? (when xorg-x11-server-devel is
> installed). If yes, this is something which should be fixed on the xorg side
> in my opinion.

fails:

12:09 hrw@pinkiepie-rawhide:del$ gcc -Wall $(pkg-config --cflags --libs xorg-server) outb.c
outb.c: In function ‘main’:
outb.c:7:9: warning: implicit declaration of function ‘outb’ [-Wimplicit-function-declaration]
         outb(0, 0);
         ^
/tmp/cccvCtz8.o: In function `main':
outb.c:(.text+0x18): undefined reference to `outb'
collect2: error: ld returned 1 exit status


(In reply to Christophe Fergeau from comment #4)
> compiler.h has:
> 
> #elif defined(__arm__) && defined(__linux__)
> 
> /* for Linux on ARM, we use the LIBC inx/outx routines */

> I don't know if __arm__ is defined for aarch64 nor if the same code path
> should be used for aarch64?

AArch64 does not have sys/io.h header.

Comment 7 Christophe Fergeau 2015-03-18 13:46:21 UTC
Looking again at the initial report, appending -Wno-error to the build cflags in the .spec (make %{?_smp_mflags} CFLAGS="-Wno-error") would probably work around that issue too.

Fixing this bug for good would mean fixing the xserver headers, reassigning the bug there.

Comment 8 Marcin Juszkiewicz 2015-03-19 11:48:32 UTC
Created attachment 1003815 [details]
hack for xserver package to add in/out stubs for AArch64

Checked changes in hw/xfree86/common/compiler.h header.

There was set of cleanups done. One commit removed FAKEIT variable and set of stubs for in/out functions:

http://cgit.freedesktop.org/xorg/xserver/commit/hw/xfree86/common/compiler.h?id=80446086b9cfcc5e23a400d7fa38ec773fae68fc

xfree86: Undef FAKEIT
I guess this is meant to stub out all I/O port calls?  Whatever, it's
not been defined by the buildsystem at least as far back as monolith
6.8.2.

Reviewed-by: Julien Cristau <jcristau>
Signed-off-by: Adam Jackson <ajax>
Signed-off-by: Keith Packard <keithp>

But looks like AArch64 used those. I am now running build with stubs added for aarch64.

Comment 9 Marcin Juszkiewicz 2015-03-19 11:56:36 UTC
xorg-x11-drv-qxl built fine with patched xserver devel package.

Comment 10 Marcin Juszkiewicz 2015-03-19 12:07:50 UTC
test code also builds

Comment 11 Peter Robinson 2015-04-19 08:27:54 UTC
Can this be reviewed please? It's causing us issues composing aarch64

Comment 12 Fedora Blocker Bugs Application 2015-04-19 08:30:43 UTC
Proposed as a Blocker for 22-final by Fedora user pbrobinson using the blocker tracking app because:

 Regression in xorg, causing issues with composes which require kludgy workarounds

Comment 13 Petr Schindler 2015-04-20 19:00:53 UTC
Discussed at today's blocker review meeting [1].

This bug was rejected as Final Blocker - this appears to be an issue in a secondary architecture, and cannot possibly block the release.

[1] http://meetbot.fedoraproject.org/fedora-blocker-review/2015-04-20/

Comment 14 Adam Jackson 2015-04-23 14:55:58 UTC
When the FAKEIT code was there all the I/O port calls would compile to nothing, which means this driver probably couldn't have worked at all on arm64.  So this can't possibly be a blocker.

Making this actually work on arm64 probably requires a patch along the lines of:

====
diff --git a/src/qxl.h b/src/qxl.h
index f46bc58..73c5378 100644
--- a/src/qxl.h
+++ b/src/qxl.h
@@ -273,6 +273,7 @@ struct _qxl_screen_t
 #ifndef XSPICE
 #ifdef XSERVER_LIBPCIACCESS
     struct pci_device *                pci;
+    struct pci_io_handle *     io;
 #else
     pciVideoPtr                        pci;
     PCITAG                     pci_tag;
@@ -631,7 +632,7 @@ void ioport_write(qxl_screen_t *qxl, uint32_t io_port, uint32_t val);
 #else
 static inline void ioport_write(qxl_screen_t *qxl, int port, int val)
 {
-    outb(qxl->io_base + port, val);
+    pci_io_write8(qxl->io, port, val)
 }
 #endif
 
diff --git a/src/qxl_driver.c b/src/qxl_driver.c
index 942067f..d6c33f0 100644
--- a/src/qxl_driver.c
+++ b/src/qxl_driver.c
@@ -219,6 +219,8 @@ unmap_memory_helper (qxl_screen_t *qxl)
        pci_device_unmap_range (qxl->pci, qxl->vram, qxl->pci->regions[1].size);
     if (qxl->rom)
        pci_device_unmap_range (qxl->pci, qxl->rom, qxl->pci->regions[2].size);
+    if (qxl->io)
+       pci_device_close_io (qxl->pci, qxl->io);
 #else
     if (qxl->ram)
        xf86UnMapVidMem (scrnIndex, qxl->ram, (1 << qxl->pci->size[0]));
@@ -251,6 +253,9 @@ map_memory_helper (qxl_screen_t *qxl)
                           qxl->pci->regions[2].size, 0,
                           (void **)&qxl->rom);
     
+    qxl->io = pci_device_open_io(qxl->pci,
+                                qxl->pci->regions[3].base_addr,
+                                qxl->pci->regions[3].size);
     qxl->io_base = qxl->pci->regions[3].base_addr;
 #else
     qxl->ram = xf86MapPciMem (scrnIndex, VIDMEM_FRAMEBUFFER,
====

I can't test easily without hardware though.

Comment 15 Christophe Fergeau 2015-04-24 11:21:07 UTC
Ah, thanks for the patch suggestion!

(In reply to Adam Jackson from comment #14)
>  static inline void ioport_write(qxl_screen_t *qxl, int port, int val)
>  {
> -    outb(qxl->io_base + port, val);
> +    pci_io_write8(qxl->io, port, val)
>  }

Is this going to do the right thing on arches which use outb, or do we need one outb codepath, and one aarch64 pci_io_write() codepath?

Comment 16 Adam Jackson 2015-04-27 19:35:11 UTC
(In reply to Christophe Fergeau from comment #15)
> Ah, thanks for the patch suggestion!
> 
> (In reply to Adam Jackson from comment #14)
> >  static inline void ioport_write(qxl_screen_t *qxl, int port, int val)
> >  {
> > -    outb(qxl->io_base + port, val);
> > +    pci_io_write8(qxl->io, port, val)
> >  }
> 
> Is this going to do the right thing on arches which use outb, or do we need
> one outb codepath, and one aarch64 pci_io_write() codepath?

On linux pci_io_* try to open the sysfs map file corresponding to the I/O port range, which the kernel translates arch-appropriately.  If there is no such file then it'll try to use port instructions if that's a thing the architecture has.  So you only need one path, pciaccess exists to get the portability right for you.

Comment 17 Marcin Juszkiewicz 2015-04-28 15:03:07 UTC
Created attachment 1019726 [details]
patch + spec changeset

Integrated patch from comment 14 and now it builds. 

Do not know how to check does it work properly (have hw).

Comment 18 Cole Robinson 2015-04-28 15:40:50 UTC
Given that qxl is virt only, and aarch64 VMs don't support video or PCI yet, I don't know if there _is_ any way to test it...

Comment 19 Christophe Fergeau 2015-04-28 16:04:52 UTC
I tested a build with this patch on an x86 guest/host and this seems to be working fine there.

Comment 21 Christophe Fergeau 2015-05-15 15:36:03 UTC
I pushed your .spec patch Marcin with the patch which was committed upstream (the one from comment #20 ). So build should work in rawhide even though it's probably not possible to use this code.