On 06/03/2015 01:40 AM, Peter Maydell wrote:
> On 30 May 2015 at 22:10, Chen Gang <xili_gchen_5...@hotmail.com> 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
