[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] linux-user: Use correct alignment for long long
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [PATCH] linux-user: Use correct alignment for long long on i386 guests |
Date: |
Thu, 28 Jul 2016 21:43:00 +0100 |
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