Re: [Qemu-devel] [PATCH] linux-user: Use correct alignment for long long on i386 guests

[Peter Maydell]

On 28 July 2016 at 21:36, Laurent Vivier <address@hidden> wrote:
>
>
> Le 28/07/2016 à 13:57, Peter Maydell a écrit :
>> For i386, the ABI specifies that 'long long' (8 byte values)
>> need only be 4 aligned, but we were requiring them to be
>> 8-aligned. This meant we were laying out the target_epoll_event
>> structure wrongly. Add a suitable ifdef to abitypes.h to
>> specify the i386-specific alignment requirement.
>
> gdb qemu-i386
> (gdb) p &(((struct target_epoll_event *)0)->data)
> $1 = (target_epoll_data_t *) 0x8

This is wrong; perhaps the debug info doesn't correctly
include alignment requirements for types? (If you
print sizes/offsets in QEMU's C code I believe you get
different answers from what gdb is telling you here.)

> whereas:
>
> gdb qemu-x86_64
> (gdb) p &(((struct target_epoll_event *)0)->data)
> $1 = (target_epoll_data_t *) 0x4
>
> I've checked on real systems x86_64/i386:
> -----
> #include <sys/epoll.h>
>
> int main(void)
> {
>     volatile struct epoll_event e;
>
>     e.events = 0;
> }
> ----
> (gdb) p &(((struct epoll_event *)0)->data)
> $1 = (epoll_data_t *) 0x4
>
> but on ppc64, I have
>
> (gdb) p &(((struct epoll_event *)0)->data)
> $1 = (epoll_data_t *) 0x8

Yep; ppc64 doesn't pack the struct and it has an 8-align
requirement for 64-bit types.

> In fact, the structure should be packed in both cases, something like:
>
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -2562,7 +2562,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

If we have the correctly defined alignment requirement
on the target 64-bit type, then you don't need this.
(See the kernel sources you quote below which only
provide the packed attribute on x86-64, not on i386).

> on my Fedora systems x86_64/i386:
>
> /usr/include/bits/epoll.h
>
> #define __EPOLL_PACKED __attribute__ ((__packed__))
>
> /usr/include/sys/epoll.h
>
> struct epoll_event
> {
>   uint32_t events;      /* Epoll events */
>   epoll_data_t data;    /* User data variable */
> } __EPOLL_PACKED;

These are the libc data structures, which aren't
defined the same way as the kernel ones.

> but I don't understand why in linux source tree we have
>
> #ifdef __x86_64__
> #define EPOLL_PACKED __attribute__((packed))
> #else
> #define EPOLL_PACKED
> #endif
>
> struct epoll_event {
>         __u32 events;
>         __u64 data;
> } EPOLL_PACKED;

This is the kernel structure. x86-64 is a weird outlier
because they wanted to make the structure layout identical
to i386 (to ease the compat-syscall implementation).
Every other arch just lets the struct be naturally laid
out for the types it contains.

thanks
-- PMM