Re: [Qemu-devel] ?==?utf-8?q? ?==?utf-8?q? [PATCH 3/3]?==?utf-8?q? linux-user: Fix structure target_semid64_ds definition for Mips

[Aleksandar Markovic]

-------- Original Message --------
Subject: Re: [Qemu-devel] [PATCH 3/3] linux-user: Fix structure 
target_semid64_ds definition for Mips
Date: Monday, August 29, 2016 23:41 CEST
From: Peter Maydell <address@hidden>
To: Aleksandar Markovic <address@hidden>
CC: Riku Voipio <address@hidden>, QEMU Developers <address@hidden>, Petar 
Jovanovic <address@hidden>, Aleksandar Markovic <address@hidden>, 
address@hidden, Miodrag Dinic <address@hidden>, Leon Alrae <address@hidden>, 
Aurelien Jarno <address@hidden>
References: <address@hidden> <address@hidden>


 On 29 August 2016 at 17:07, Aleksandar Markovic
<address@hidden> wrote:
> From: Aleksandar Markovic <address@hidden>
>
> This patch corrects target_semid64_ds structure definition for Mips.
>
> See, for example definition of semid64_ds for Mips in Linux kernel:
> arch/mips/include/uapi/asm/sembuf.h#L13.
>
> This patch will also fix certain semaphore-related LTP tests for Mips,
> if they are executed in Qemu user mode for any Mips platform.
>
> Signed-off-by: Miodrag Dinic <address@hidden>
> ---
> linux-user/mips/target_structs.h | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/linux-user/mips/target_structs.h 
> b/linux-user/mips/target_structs.h
> index fbd9955..6875506 100644
> --- a/linux-user/mips/target_structs.h
> +++ b/linux-user/mips/target_structs.h
> @@ -45,4 +45,23 @@ struct target_shmid_ds {
> abi_ulong __unused2;
> };
>
> +#define TARGET_SEMID64_DS
> +/*
> + * 
> http://lxr.free-electrons.com/source/arch/mips/include/uapi/asm/sembuf.h#L13
> + *
> + * The semid64_ds structure for the MIPS architecture.
> + * Note extra padding because this structure is passed back and forth
> + * between kernel and user space.
> + *
> + * MIPS uses the same structure layout for both 32bit and 64bit variants.
> + */

Isn't it specifically *not* using the same structure layout for
32 and 64 bit? The struct below uses abi_ulong, which has a
different size for the two cases, so the structure layout will
differ. The 'asm generic' version of the struct (which QEMU defines
in syscall.c) is the one which is the same for both 32 and 64 bit
because it adds the explicit padding for the TARGET_ABI_BITS=32 case.

> +struct target_semid64_ds {
> + struct target_ipc_perm sem_perm;
> + abi_ulong sem_otime;
> + abi_ulong sem_ctime;
> + abi_ulong sem_nsems;
> + abi_ulong __unused3;
> + abi_ulong __unused4;
> +};

This does seem to be what the kernel does, though, so
Reviewed-by: Peter Maydell <address@hidden>
if you clarify the comment.

thanks
-- PMMYes, good point - in v2, I am going to remove that sentence altogether 
from the comment. I plan to remove also the link from the comment, since it is 
uncommon to insert links in the comments, and the kernel source location for 
Mips' semid64_ds is mentioned in the commit message anyway.

Thanks!