On 06/03/2015 01:06 AM, Peter Maydell wrote:
> On 30 May 2015 at 22:10, Chen Gang <xili_gchen_5...@hotmail.com> 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 <gang.chen.5...@gmail.com>
>> ---
>> +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.
>
OK, thanks.
>> +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?
>
OK, thanks, I will try to test it without pad and unused fields.
For me, I also think they are useless (wrong). But I don't understand
why all the other targets add them (may they also need remove pad or
unused? -- I guess yes).
>> +};
>> +
>> +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.
>
We really need analyze why it still works. I will try to analyze it (it
seems most of the other targets face to the same issue).
>> + 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.
>
OK, thanks.
>> + 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.
>
OK, thanks. I will remove it.
--
Chen Gang
Open, share, and attitude like air, water, and life which God blessed
