Re: [Qemu-devel] [PATCH 02/10 v11] linux-user: Support tilegx architecture in linux-user

[Chen Gang]

On 06/03/2015 01:40 AM, Peter Maydell wrote:
> On 30 May 2015 at 22:10, Chen Gang <address@hidden> wrote:
>>
>> +#ifdef TARGET_TILEGX
>> +
>> +static uint64_t get_regval(CPUTLGState *env, uint8_t reg)
>> +{
>> +    if (likely(reg < TILEGX_R_COUNT)) {
>> +        return env->regs[reg];
>> +    } else if (reg != TILEGX_R_ZERO) {
>> +        fprintf(stderr, "invalid register r%d for reading.\n", reg);
>> +        g_assert_not_reached();
> 
> You don't appear to be guaranteeing that the register value
> is < TILEGX_R_COUNT anywhere: get_SrcA_X1() and friends
> mask with 0x3f, but that only means you're guaranteed the
> value is between 0 and 63, wherease TILEGX_R_COUNT is 56.
> What does real hardware do if the encoded register value
> is 56..63 ?
>

At present, it will g_assert_not_reached() too. 56..62 are hidden to
outside. So I did not implement them, either. Need we still implement
them?

For 63, it is zero register, we need do nothing for it.


> Also, if (something) {
>           g_assert_not_reached();
>       }
> 
> is an awkward way to write
>     g_assert(!something);
> 

OK, thanks. The code above is fine to me.

>> +
>> +/*
>> + * Compare the 8-byte contents of the CmpValue SPR with the 8-byte value in
>> + * memory at the address held in the first source register. If the values 
>> are
>> + * not equal, then no memory operation is performed. If the values are 
>> equal,
>> + * the 8-byte quantity from the second source register is written into 
>> memory
>> + * at the address held in the first source register. In either case, the 
>> result
>> + * of the instruc- tion is the value read from memory. The compare and 
>> write to
> 
> stray "- ".
> 

OK, thanks.

>> + * memory are atomic and thus can be used for synchronization purposes. This
>> + * instruction only operates for addresses aligned to a 8-byte boundary.
>> + * Unaligned memory access causes an Unaligned Data Reference interrupt.
>> + *
>> + * Functional Description (64-bit)
>> + *       uint64_t memVal = memoryReadDoubleWord (rf[SrcA]);
>> + *       rf[Dest] = memVal;
>> + *       if (memVal == SPR[CmpValueSPR])
>> + *           memoryWriteDoubleWord (rf[SrcA], rf[SrcB]);
>> + *
>> + * Functional Description (32-bit)
>> + *       uint64_t memVal = signExtend32 (memoryReadWord (rf[SrcA]));
>> + *       rf[Dest] = memVal;
>> + *       if (memVal == signExtend32 (SPR[CmpValueSPR]))
>> + *           memoryWriteWord (rf[SrcA], rf[SrcB]);
>> + *
>> + *
>> + * For exch(4), will no cmp spr.
> 
> Not sure what this sentence means?
> 

This function also process exch and exch4 which need not process SPR.

I guess, the comments needs to be improved (provide more details).

>> + */
>> +static void do_exch(CPUTLGState *env, int8_t quad, int8_t cmp)
> 
> quad and cmp are just booleans, right? Why int8_t not bool?
> 

OK, thanks. I will change to bool in qemu. I often use char or int
instead of bool. For the latest C, bool is better.

>> +{
>> +    uint8_t rdst, rsrc, rsrcb;
>> +    target_ulong addr, tmp;
>> +    target_long val, sprval;
>> +    target_siginfo_t info;
>> +
>> +    start_exclusive();
>> +
>> +    rdst = (env->excparam >> 16) & 0xff;
>> +    rsrc = (env->excparam >> 8) & 0xff;
>> +    rsrcb = env->excparam & 0xff;
> 
> Consider extract32().
> 

OK, thanks. It sounds good.

>> +
>> +    addr = get_regval(env, rsrc);
>> +    if (quad ? get_user_s64(val, addr) : get_user_s32(val, addr)) {
>> +        goto do_sigsegv;
>> +    }
>> +    tmp = (target_ulong)val;  /* rdst may be the same to rsrcb, so buffer 
>> it */
> 
> Why do this, when we could just use a different variable
> rather than trashing val below?
> 

OK, thanks, the code need rewrite a little, just like you said below.

>> +
>> +    if (cmp) {
>> +        if (quad) {
>> +            sprval = (target_long)env->spregs[TILEGX_SPR_CMPEXCH];
> 
> Pointless cast.
> 

OK, thanks.

>> +        } else {
>> +            sprval = (int32_t)(env->spregs[TILEGX_SPR_CMPEXCH] & 
>> 0xffffffff);
> 
> Clearer as
>     sprval = sextract64(env->spregs[TILEGX_SPR_CMPEXCH], 0, 32);
> 

OK, thanks.

>> +        }
>> +    }
>> +
>> +    if (!cmp || val == sprval) {
>> +        val = get_regval(env, rsrcb);
> 
> If you say "target_long srcbval = ..." you don't trash val.
> 

OK, thanks.

>> +        if (quad ? put_user_u64(val, addr) : put_user_u32(val, addr)) {
>> +            goto do_sigsegv;
>> +        }
>> +    }
>> +
>> +    set_regval(env, rdst, tmp);
>> +
>> +    end_exclusive();
>> +    return;
>> +
>> +do_sigsegv:
>> +    end_exclusive();
>> +
>> +    info.si_signo = TARGET_SIGSEGV;
>> +    info.si_errno = 0;
>> +    info.si_code = TARGET_SEGV_MAPERR;
>> +    info._sifields._sigfault._addr = addr;
>> +    queue_signal(env, TARGET_SIGSEGV, &info);
>> +}
>> +
>> +static void do_fetch(CPUTLGState *env, int trapnr, int8_t quad)
>> +{
>> +    uint8_t rdst, rsrc, rsrcb;
>> +    int8_t write = 1;
>> +    target_ulong addr;
>> +    target_long val, tmp;
>> +    target_siginfo_t info;
>> +
>> +    start_exclusive();
>> +
>> +    rdst = (env->excparam >> 16) & 0xff;
>> +    rsrc = (env->excparam >> 8) & 0xff;
>> +    rsrcb = env->excparam & 0xff;
>> +
>> +    addr = get_regval(env, rsrc);
>> +    if (quad ? get_user_s64(val, addr) : get_user_s32(val, addr)) {
>> +        goto do_sigsegv;
>> +    }
>> +    tmp = val;  /* rdst may be the same to rsrcb, so buffer it */
> 
> Again, unnecessary copy when you could just use a different
> variable for the value in rsrcb.
> 

OK, thanks.

>> +    val = get_regval(env, rsrcb);
>> +    switch (trapnr) {
>> +    case TILEGX_EXCP_OPCODE_FETCHADD:
>> +    case TILEGX_EXCP_OPCODE_FETCHADD4:
>> +    case TILEGX_EXCP_OPCODE_FETCHADDGEZ:
>> +    case TILEGX_EXCP_OPCODE_FETCHADDGEZ4:
>> +        val += tmp;
>> +        if (trapnr == TILEGX_EXCP_OPCODE_FETCHADDGEZ) {
>> +            if (val < 0) {
>> +                write = 0;
>> +            }
>> +        } else if (trapnr == TILEGX_EXCP_OPCODE_FETCHADDGEZ4) {
>> +            if ((int32_t)val < 0) {
>> +                write = 0;
>> +            }
>> +        }
> 
> Just give these their own case blocks rather than
> writing an if() that's conditioned on the same variable
> we're switching on. The only duplication you're saving
> is a single line addition.
> 

OK, thanks.

>> +        break;
>> +    case TILEGX_EXCP_OPCODE_FETCHAND:
>> +    case TILEGX_EXCP_OPCODE_FETCHAND4:
>> +        val &= tmp;
>> +        break;
>> +    case TILEGX_EXCP_OPCODE_FETCHOR:
>> +    case TILEGX_EXCP_OPCODE_FETCHOR4:
>> +        val |= tmp;
>> +        break;
>> +    default:
>> +        g_assert_not_reached();
>> +    }
>> +
>> +    if (write) {
>> +        if (quad ? put_user_u64(val, addr) : put_user_u32(val, addr)) {
>> +            goto do_sigsegv;
>> +        }
>> +    }
>> +
>> +    set_regval(env, rdst, tmp);
>> +
>> +    end_exclusive();
>> +    return;
>> +
>> +do_sigsegv:
>> +    end_exclusive();
>> +
>> +    info.si_signo = TARGET_SIGSEGV;
>> +    info.si_errno = 0;
>> +    info.si_code = TARGET_SEGV_MAPERR;
>> +    info._sifields._sigfault._addr = addr;
>> +    queue_signal(env, TARGET_SIGSEGV, &info);
>> +}
>> +
>> +void cpu_loop(CPUTLGState *env)
>> +{
>> +    CPUState *cs = CPU(tilegx_env_get_cpu(env));
>> +    int trapnr;
>> +
>> +    while (1) {
>> +        cpu_exec_start(cs);
>> +        trapnr = cpu_tilegx_exec(env);
>> +        cpu_exec_end(cs);
>> +        switch (trapnr) {
>> +        case TILEGX_EXCP_SYSCALL:
>> +            env->regs[TILEGX_R_RE] = do_syscall(env, env->regs[TILEGX_R_NR],
>> +                                                env->regs[0], env->regs[1],
>> +                                                env->regs[2], env->regs[3],
>> +                                                env->regs[4], env->regs[5],
>> +                                                env->regs[6], env->regs[7]);
>> +            env->regs[TILEGX_R_ERR] = 
>> TILEGX_IS_ERRNO(env->regs[TILEGX_R_RE]) ?
>> +                                          env->regs[TILEGX_R_RE] : 0;
>> +            break;
>> +        case TILEGX_EXCP_OPCODE_EXCH:
>> +            do_exch(env, 1, 0);
> 
> these should be true, false assuming you change the arg type to bool.
> 

OK, thanks.

>> +            break;
>> +        case TILEGX_EXCP_OPCODE_EXCH4:
>> +            do_exch(env, 0, 0);
>> +            break;
>> +        case TILEGX_EXCP_OPCODE_CMPEXCH:
>> +            do_exch(env, 1, 1);
>> +            break;
>> +        case TILEGX_EXCP_OPCODE_CMPEXCH4:
>> +            do_exch(env, 0, 1);
>> +            break;
>> +        case TILEGX_EXCP_OPCODE_FETCHADD:
>> +            do_fetch(env, trapnr, 1);
>> +            break;
>> +        case TILEGX_EXCP_OPCODE_FETCHADDGEZ:
>> +        case TILEGX_EXCP_OPCODE_FETCHAND:
>> +        case TILEGX_EXCP_OPCODE_FETCHOR:
>> +            do_fetch(env, trapnr, 1);
>> +            exit(1);
> 
> ???
> 

OK, thanks.

>> +            break;
>> +        case TILEGX_EXCP_OPCODE_FETCHADD4:
>> +        case TILEGX_EXCP_OPCODE_FETCHADDGEZ4:
>> +        case TILEGX_EXCP_OPCODE_FETCHAND4:
>> +        case TILEGX_EXCP_OPCODE_FETCHOR4:
>> +            do_fetch(env, trapnr, 0);
>> +            exit(1);
> 
> Again.
> 

OK, thanks.

-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed