Re: [Qemu-devel] [PATCH 01/10 v11] linux-user: tilegx: Firstly add architecture related features

[Peter Maydell]

On 30 May 2015 at 22:10, Chen Gang <address@hidden> wrote:
> They are based on Linux kernel tilegx architecture for 64 bit binary,
> and also based on tilegx ABI reference document, and also reference from
> other targets implementations.
>
> Signed-off-by: Chen Gang <address@hidden>
> ---
> +typedef struct target_sigaltstack {
> +    abi_ulong ss_sp;
> +    abi_int ss_flags;
> +    abi_int dummy;
> +    abi_ulong ss_size;
> +} target_stack_t;

Drop the 'dummy' field; you don't need it. The point of the abi_*
types is to have the same struct layout requirements as the target,
and the target doesn't have a 'dummy' field.

> +struct target_ipc_perm {
> +    abi_int __key;                      /* Key.  */
> +    abi_uint uid;                       /* Owner's user ID.  */
> +    abi_uint gid;                       /* Owner's group ID.  */
> +    abi_uint cuid;                      /* Creator's user ID.  */
> +    abi_uint cgid;                      /* Creator's group ID.  */
> +    abi_uint mode;                      /* Read/write permission.  */
> +    abi_ushort __seq;                   /* Sequence number.  */
> +    abi_ushort __pad2;
> +    abi_ulong __unused1;
> +    abi_ulong __unused2;

I still think these pad and unused fields are wrong; they're not
in the kernel. Do you have a test program that is incorrectly
handled if we don't have these fields in the QEMU struct?

> +};
> +
> +struct target_shmid_ds {
> +    struct target_ipc_perm shm_perm;    /* operation permission struct */

...in particular the way this struct is embedded means that
if we have the wrong size for target_ipc_perm then we will
have wrong offsets for all these following fields.

> +    abi_long shm_segsz;                 /* size of segment in bytes */
> +    abi_ulong shm_atime;                /* time of last shmat() */
> +    abi_ulong shm_dtime;                /* time of last shmdt() */
> +    abi_ulong shm_ctime;                /* time of last change by shmctl() */
> +    abi_int shm_cpid;                   /* pid of creator */
> +    abi_int shm_lpid;                   /* pid of last shmop */
> +    abi_ulong shm_nattch;               /* number of current attaches */

The kernel has an unsigned short here, with a following
unsigned short shm_unused for padding.

> +    abi_ulong __unused4;
> +    abi_ulong __unused5;

> +};
> +
> +#endif
> diff --git a/linux-user/tilegx/termbits.h b/linux-user/tilegx/termbits.h
> new file mode 100644
> index 0000000..39bc8ac
> --- /dev/null
> +++ b/linux-user/tilegx/termbits.h
> @@ -0,0 +1,285 @@
> +#ifndef TILEGX_TERMBITS_H
> +#define TILEGX_TERMBITS_H
> +
> +/* From asm-generic/termbits.h, which is used by tilegx */
> +
> +#define TARGET_NCCS 19
> +struct target_termios {
> +    unsigned int c_iflag;             /* input mode flags */
> +    unsigned int c_oflag;             /* output mode flags */
> +    unsigned int c_cflag;             /* control mode flags */
> +    unsigned int c_lflag;             /* local mode flags */
> +    unsigned char c_line;             /* line discipline */
> +    unsigned char c_cc[TARGET_NCCS];  /* control characters */
> +};
> +
> +struct target_termios2 {
> +    unsigned int c_iflag;             /* input mode flags */
> +    unsigned int c_oflag;             /* output mode flags */
> +    unsigned int c_cflag;             /* control mode flags */
> +    unsigned int c_lflag;             /* local mode flags */
> +    unsigned char c_line;             /* line discipline */
> +    unsigned char c_cc[TARGET_NCCS];  /* control characters */
> +    unsigned int c_ispeed;            /* input speed */
> +    unsigned int c_ospeed;            /* output speed */
> +};
> +
> +struct target_ktermios {
> +    unsigned int c_iflag;             /* input mode flags */
> +    unsigned int c_oflag;             /* output mode flags */
> +    unsigned int c_cflag;             /* control mode flags */
> +    unsigned int c_lflag;             /* local mode flags */
> +    unsigned char c_line;             /* line discipline */
> +    unsigned char c_cc[TARGET_NCCS];  /* control characters */
> +    unsigned int c_ispeed;            /* input speed */
> +    unsigned int c_ospeed;            /* output speed */
> +};

Why have you defined target_ktermios? It's not used anywhere in QEMU.

thanks
-- PMM