[PATCH] Revert "Make sys.c use unaligned access functions provided in compiler."

Matt Turner mattst88 at gmail.com
Mon Dec 28 11:02:02 PST 2009


On Mon, Dec 28, 2009 at 11:00 AM, Tiago Vignatti
<tiago.vignatti at nokia.com> wrote:
> This reverts commit da923d0bc15e99a8ed1986bd6f5df37f7af7284b.
>
> Signed-off-by: Tiago Vignatti <tiago.vignatti at nokia.com>
>
> ---
> Again this Matt's change is breaking my compilation under scratchbox, for arm
> architecture:
>
>    then mv -f ".deps/sys.Tpo" ".deps/sys.Plo"; else rm -f ".deps/sys.Tpo";
> exit 1; fi
>  gcc -DHAVE_CONFIG_H -I. -I../../../../hw/xfree86/x86emu -I../../../include
> -I../../../include -I../../../include -I../../../include -I../../../include
> -I../../../include -I../../../include -I../../../include
> -I../../../../hw/xfree86 -I../../../../hw/xfree86/include
> -I../../../../hw/xfree86/common -I../../../../hw/xfree86/os-support
> -I../../../../hw/xfree86/os-support/bus -I../../../../os -DPRE_RELEASE=0
> -DHAVE_DIX_CONFIG_H -Wall -Wpointer-arith -Wstrict-prototypes
> -Wmissing-prototypes -Wmissing-declarations -Wnested-externs
> -fno-strict-aliasing -Wbad-function-cast -Wold-style-definition
> -Wdeclaration-after-statement -D_BSD_SOURCE -DHAS_FCHOWN -DHAS_STICKY_DIR_BIT
> -DDBUS_API_SUBJECT_TO_CHANGE -I/usr/include/freetype2 -I/usr/include/pixman-1
> -I/usr/include/hal -I/usr/include/dbus-1.0 -I/usr/lib/dbus-1.0/include
> -I../../../../include -I../../../include -I../../../../Xext
> -I../../../../composite -I../../../../damageext -I../../../../xfixes
> -I../../../../Xi -I../../../../mi -I../../../../miext/shadow
> -I../../../../miext/damage -I../../../../render -I../../../../randr
> -I../../../../fb -fvisibility=hidden -DHAVE_XORG_CONFIG_H -fvisibility=hidden
> -DXF86PM -Wall -g -O2 -MT sys.lo -MD -MP -MF .deps/sys.Tpo -c
> ../../../../hw/xfree86/x86emu/sys.c  -fPIC -DPIC -o .libs/sys.o
> ../../../../hw/xfree86/x86emu/sys.c: In function 'rdl':
> ../../../../hw/xfree86/x86emu/sys.c:143: warning: passing argument 1 of
> 'ldl_u' from incompatible pointer type
> ../../../../hw/xfree86/x86emu/sys.c: In function 'wrl':
> ../../../../hw/xfree86/x86emu/sys.c:225: warning: passing argument 2 of
> 'stl_u' from incompatible pointer type
> ../../../../hw/xfree86/x86emu/sys.c: In function 'X86EMU_setupPioFuncs':
> ../../../../hw/xfree86/x86emu/sys.c:376: error: 'X86EMU_pioFuncs' has no
> member named 'xf_outb'
> ../../../../hw/xfree86/x86emu/sys.c:377: error: 'X86EMU_pioFuncs' has no
> member named 'xf_outw'
> ../../../../hw/xfree86/x86emu/sys.c:378: error: 'X86EMU_pioFuncs' has no
> member named 'xf_outl'
>
>
> Sorry to not scream before about it, when the patch came to xorg-devel review.
>
> Now I could try to fix the issue of the patch, but I'm not happy with its
> approach: the patch is using a header from the server inside x86emu. There's
> no conceptual problem on it but it will complicates when we try to remove
> x86emu from the server (my ongoing work - using lrmi).
>
> So I'd be happy to just revert such patch now.
>
>
>  hw/xfree86/x86emu/Makefile.am |    2 +-
>  hw/xfree86/x86emu/sys.c       |  181 ++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 181 insertions(+), 2 deletions(-)
>
> diff --git a/hw/xfree86/x86emu/Makefile.am b/hw/xfree86/x86emu/Makefile.am
> index e7368f5..df96977 100644
> --- a/hw/xfree86/x86emu/Makefile.am
> +++ b/hw/xfree86/x86emu/Makefile.am
> @@ -11,7 +11,7 @@ libx86emu_la_SOURCES = debug.c \
>                       sys.c \
>                       x86emu.h
>
> -INCLUDES = $(XORG_INCS)
> +INCLUDES =
>
>  AM_CFLAGS = $(DIX_CFLAGS) $(XORG_CFLAGS)
>
> diff --git a/hw/xfree86/x86emu/sys.c b/hw/xfree86/x86emu/sys.c
> index 2ebf6f1..e15fb09 100644
> --- a/hw/xfree86/x86emu/sys.c
> +++ b/hw/xfree86/x86emu/sys.c
> @@ -48,13 +48,168 @@
>  #ifndef NO_SYS_HEADERS
>  #include <string.h>
>  #endif
> -#include "compiler.h" /* for unaligned access functions */
>  /*------------------------- Global Variables ------------------------------*/
>
>  X86EMU_sysEnv          _X86EMU_env;            /* Global emulator machine state */
>  X86EMU_intrFuncs       _X86EMU_intrTab[256];
>
>  /*----------------------------- Implementation ----------------------------*/
> +#if defined(__alpha__) || defined(__alpha)
> +/* to cope with broken egcs-1.1.2 :-(((( */
> +
> +#define ALPHA_UALOADS
> +/*
> + * inline functions to do unaligned accesses
> + * from linux/include/asm-alpha/unaligned.h
> + */
> +
> +/*
> + * EGCS 1.1 knows about arbitrary unaligned loads.  Define some
> + * packed structures to talk about such things with.
> + */
> +
> +#if defined(__GNUC__)
> +struct __una_u64 { unsigned long  x __attribute__((packed)); };
> +struct __una_u32 { unsigned int   x __attribute__((packed)); };
> +struct __una_u16 { unsigned short x __attribute__((packed)); };
> +#endif
> +
> +static __inline__ unsigned long ldq_u(unsigned long * r11)
> +{
> +#if defined(__GNUC__)
> +       const struct __una_u64 *ptr = (const struct __una_u64 *) r11;
> +       return ptr->x;
> +#else
> +       unsigned long r1,r2;
> +       __asm__("ldq_u %0,%3\n\t"
> +               "ldq_u %1,%4\n\t"
> +               "extql %0,%2,%0\n\t"
> +               "extqh %1,%2,%1"
> +               :"=&r" (r1), "=&r" (r2)
> +               :"r" (r11),
> +                "m" (*r11),
> +                "m" (*(const unsigned long *)(7+(char *) r11)));
> +       return r1 | r2;
> +#endif
> +}
> +
> +static __inline__ unsigned long ldl_u(unsigned int * r11)
> +{
> +#if defined(__GNUC__)
> +       const struct __una_u32 *ptr = (const struct __una_u32 *) r11;
> +       return ptr->x;
> +#else
> +       unsigned long r1,r2;
> +       __asm__("ldq_u %0,%3\n\t"
> +               "ldq_u %1,%4\n\t"
> +               "extll %0,%2,%0\n\t"
> +               "extlh %1,%2,%1"
> +               :"=&r" (r1), "=&r" (r2)
> +               :"r" (r11),
> +                "m" (*r11),
> +                "m" (*(const unsigned long *)(3+(char *) r11)));
> +       return r1 | r2;
> +#endif
> +}
> +
> +static __inline__ unsigned long ldw_u(unsigned short * r11)
> +{
> +#if defined(__GNUC__)
> +       const struct __una_u16 *ptr = (const struct __una_u16 *) r11;
> +       return ptr->x;
> +#else
> +       unsigned long r1,r2;
> +       __asm__("ldq_u %0,%3\n\t"
> +               "ldq_u %1,%4\n\t"
> +               "extwl %0,%2,%0\n\t"
> +               "extwh %1,%2,%1"
> +               :"=&r" (r1), "=&r" (r2)
> +               :"r" (r11),
> +                "m" (*r11),
> +                "m" (*(const unsigned long *)(1+(char *) r11)));
> +       return r1 | r2;
> +#endif
> +}
> +
> +/*
> + * Elemental unaligned stores
> + */
> +
> +static __inline__ void stq_u(unsigned long r5, unsigned long * r11)
> +{
> +#if defined(__GNUC__)
> +       struct __una_u64 *ptr = (struct __una_u64 *) r11;
> +       ptr->x = r5;
> +#else
> +       unsigned long r1,r2,r3,r4;
> +
> +       __asm__("ldq_u %3,%1\n\t"
> +               "ldq_u %2,%0\n\t"
> +               "insqh %6,%7,%5\n\t"
> +               "insql %6,%7,%4\n\t"
> +               "mskqh %3,%7,%3\n\t"
> +               "mskql %2,%7,%2\n\t"
> +               "bis %3,%5,%3\n\t"
> +               "bis %2,%4,%2\n\t"
> +               "stq_u %3,%1\n\t"
> +               "stq_u %2,%0"
> +               :"=m" (*r11),
> +                "=m" (*(unsigned long *)(7+(char *) r11)),
> +                "=&r" (r1), "=&r" (r2), "=&r" (r3), "=&r" (r4)
> +               :"r" (r5), "r" (r11));
> +#endif
> +}
> +
> +static __inline__ void stl_u(unsigned long r5, unsigned int * r11)
> +{
> +#if defined(__GNUC__)
> +       struct __una_u32 *ptr = (struct __una_u32 *) r11;
> +       ptr->x = r5;
> +#else
> +       unsigned long r1,r2,r3,r4;
> +
> +       __asm__("ldq_u %3,%1\n\t"
> +               "ldq_u %2,%0\n\t"
> +               "inslh %6,%7,%5\n\t"
> +               "insll %6,%7,%4\n\t"
> +               "msklh %3,%7,%3\n\t"
> +               "mskll %2,%7,%2\n\t"
> +               "bis %3,%5,%3\n\t"
> +               "bis %2,%4,%2\n\t"
> +               "stq_u %3,%1\n\t"
> +               "stq_u %2,%0"
> +               :"=m" (*r11),
> +                "=m" (*(unsigned long *)(3+(char *) r11)),
> +                "=&r" (r1), "=&r" (r2), "=&r" (r3), "=&r" (r4)
> +               :"r" (r5), "r" (r11));
> +#endif
> +}
> +
> +static __inline__ void stw_u(unsigned long r5, unsigned short * r11)
> +{
> +#if defined(__GNUC__)
> +       struct __una_u16 *ptr = (struct __una_u16 *) r11;
> +       ptr->x = r5;
> +#else
> +       unsigned long r1,r2,r3,r4;
> +
> +       __asm__("ldq_u %3,%1\n\t"
> +               "ldq_u %2,%0\n\t"
> +               "inswh %6,%7,%5\n\t"
> +               "inswl %6,%7,%4\n\t"
> +               "mskwh %3,%7,%3\n\t"
> +               "mskwl %2,%7,%2\n\t"
> +               "bis %3,%5,%3\n\t"
> +               "bis %2,%4,%2\n\t"
> +               "stq_u %3,%1\n\t"
> +               "stq_u %2,%0"
> +               :"=m" (*r11),
> +                "=m" (*(unsigned long *)(1+(char *) r11)),
> +                "=&r" (r1), "=&r" (r2), "=&r" (r3), "=&r" (r4)
> +               :"r" (r5), "r" (r11));
> +#endif
> +}
> +#endif
>
>  /****************************************************************************
>  PARAMETERS:
> @@ -107,7 +262,13 @@ u16 X86API rdw(
>                }
>        else
>  #endif
> +#if defined(ALPHA_UALOADS)
>                val = ldw_u((u16*)(M.mem_base + addr));
> +#elif  defined(IA64_UALOADS)
> +      val = uldw((u16*)(M.mem_base + addr));
> +#else
> +               val = *(u16*)(M.mem_base + addr);
> +#endif
>                DB(     if (DEBUG_MEM_TRACE())
>                printk("%#08x 2 -> %#x\n", addr, val);)
>     return val;
> @@ -140,7 +301,13 @@ u32 X86API rdl(
>                }
>        else
>  #endif
> +#if defined(ALPHA_UALOADS)
>                val = ldl_u((u32*)(M.mem_base + addr));
> +#elif  defined(IA64_UALOADS)
> +        val = uldl((u32*)(M.mem_base + addr));
> +#else
> +               val = *(u32*)(M.mem_base + addr);
> +#endif
>  DB(    if (DEBUG_MEM_TRACE())
>                printk("%#08x 4 -> %#x\n", addr, val);)
>        return val;
> @@ -192,7 +359,13 @@ DB(        if (DEBUG_MEM_TRACE())
>                }
>        else
>  #endif
> +#if defined(ALPHA_UALOADS)
>         stw_u(val,(u16*)(M.mem_base + addr));
> +#elif defined(IA64_UALOADS)
> +     ustw(val,(u16*)(M.mem_base + addr));
> +#else
> +        *(u16*)(M.mem_base + addr) = val;
> +#endif
>  }
>
>  /****************************************************************************
> @@ -222,7 +395,13 @@ DB(        if (DEBUG_MEM_TRACE())
>                }
>        else
>  #endif
> +#if defined(ALPHA_UALOADS)
>         stl_u(val,(u32*)(M.mem_base + addr));
> +#elif defined(IA64_UALOADS)
> +     ustl(val,(u32*)(M.mem_base + addr));
> +#else
> +        *(u32*)(M.mem_base + addr) = val;
> +#endif
>  }
>
>  /****************************************************************************
> --
> 1.6.3.3

The fix here isn't to add back in a bunch of duplicated Alpha assembly.

It this needs its own unaligned access code, then copy the platform
agnostic code from compiler.h. That is, the code like

static __inline__ uint32_t ldl_u(uint32_t *p)
{
    const struct __una_u32 *ptr = (const struct __una_u32 *) p;
    return ptr->x;
}

But please, do not add back in the Alpha assembly.

I understand that ARM-specific nastiness in compiler.h (lines
compiler.h:1017-1019) causes sys.c to not compile--there is an
appropriate fix and this is not it.

Maybe move ARM away from out{b,w,l} to the new pci access routines ajax wrote?

Matt


More information about the xorg-devel mailing list