Re: [Qemu-devel] [PATCH] linux-user: correctly pack target_epoll_event for i386 target

[Peter Maydell]

On 22 July 2016 at 03:30, Icenowy Zheng <address@hidden> wrote:
> According to comments in /usr/include/linux/eventpoll.h, x86_64 have
> the same memory layout of struct target_epoll_event as i386. So on a
> aligned host, if x86_64 should be packed, i386 will also need.
>
> This has been tested with a i386 guest on an arm host: without the
> patch, wineserver crashes (core).
>
> Signed-off-by: Icenowy Zheng <address@hidden>
> ---
>  linux-user/syscall_defs.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> index b43966e..7380bf5 100644
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -2547,7 +2547,7 @@ struct target_mq_attr {
>  #define FUTEX_CMD_MASK          ~(FUTEX_PRIVATE_FLAG | FUTEX_CLOCK_REALTIME)
>
>  #ifdef CONFIG_EPOLL
> -#if defined(TARGET_X86_64)
> +#if defined(TARGET_X86_64) || defined(TARGET_I386)
>  #define TARGET_EPOLL_PACKED QEMU_PACKED
>  #else
>  #define TARGET_EPOLL_PACKED

We do indeed not get the right arrangement for this struct for
i386, but I don't think this is the right way to fix it. The
kernel headers only special case this for x86-64, not i386,
and so we should not need to special case i386 either.

The underlying problem is that we get the alignment of the
'unsigned long long' type for i386 wrong, treating it as 8-aligned
when it should be 4-aligned. This can be fixed by

diff --git a/include/exec/user/abitypes.h b/include/exec/user/abitypes.h
index a09d6c6..ba18860 100644
--- a/include/exec/user/abitypes.h
+++ b/include/exec/user/abitypes.h
@@ -15,6 +15,10 @@
 #define ABI_LLONG_ALIGNMENT 2
 #endif

+#if defined(TARGET_I386) && !defined(TARGET_X86_64)
+#define ABI_LLONG_ALIGNMENT 4
+#endif
+
 #ifndef ABI_SHORT_ALIGNMENT
 #define ABI_SHORT_ALIGNMENT 2
 #endif

and if you do that I don't think you need to change the
handling of the target_epoll_event struct.

(and that might in turn fix a bunch of other bugs, or if we're
unlucky introduce some new ones by breaking any lurking
workarounds for the previous misalignment...)

thanks
-- PMM