Re: [Qemu-devel] [PATCH] target-tilegx: Execute _start and reach to __libc_start_main successfully

[Chris Metcalf]

On 2/24/2015 2:53 AM, Chen Gang S wrote:
>   typedef struct CPUTLState {
> +    uint64_t regs[TILEGX_R_COUNT]; /* Common used registers by outside */
> +    uint64_t zero;                 /* Zero register */
> +    uint64_t pc;                   /* Current pc */
>       CPU_COMMON
>   } CPUTLState;

I skimmed through this and my only comment is that I was surprised to see 
"zero" as part of the state, since it's always 0.  :-)  No doubt there is some 
reason that this makes sense.

> +#define TILEGX_GEN_CHK_BEGIN(x) \
> +    if ((x) == TILEGX_R_ZERO) {
> +#define TILEGX_GEN_CHK_END(x) \
> +        return 0; \
> +    } \
> +    if ((x) >= TILEGX_R_COUNT) { \
> +        return -1; \
> +    }

This macro pattern seems potentially a little confusing and I do wonder if 
there is some way to avoid having to explicitly check the zero register every 
time; for example, maybe you make it a legitimate part of the state and declare 
that there are 64 registers, and then just always finish any register-update 
phase by re-zeroing that register?  It might yield a smaller code footprint and 
probably be just as fast, as long as it was easy to know where registers were 
updated.

Also, note that you should model accesses to registers 56..62 as causing an 
interrupt to be raised, rather than returning -1.  But you might choose to just 
put this on your TODO list and continue making forward progress for the time 
being.  But a FIXME comment here would be appropriate.

> +    case 0x3800000000000000ULL:

There are a lot of constants defined in the tile <arch/opcode.h> header, and 
presumably you could synthesize these constant values from them.  Perhaps it would 
make sense to import that header into qemu and then use symbolic values for all of 
this kind of thing?

I can't really comment on most of the rest of the code, and I only skimmed it 
quickly, but generally: good work getting as far as you have!

--
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com