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

[Icenowy Zheng]

22.07.2016, 23:06, "Peter Maydell" <address@hidden>:
> 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

Congratulations! Your patch made epoll running correctly.
I'm now running LTP syscall tests to check for hurt syscalls.

Thanks
Icenowy Zheng