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

[Chen Gang]

On 06/03/2015 01:06 AM, Peter Maydell wrote:
> 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.
> 

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