Re: [Qemu-devel] [PATCH] tilegx: Support raise instruction

[Chen Gang]

On 9/24/15 02:34, Richard Henderson wrote:
> You forgot to cc qemu-devel.
> 

Oh, we can not find it in qemu mail archive list. But I really cc
qemu-devel.

When I send the next patches, I'll notice about it, if still "can not cc
qemu-devel", I shall try to send patches from my hotmail client.


> This patch needs to be split.
> 

OK.

> On 09/22/2015 03:38 PM, address@hidden wrote:
>>  
>> +static int sigdata_code(uint64_t sigdata)
>> +{
>> +    return (sigdata & 0x3ff) >> 6;
>> +}
>> +
>> +static int sigdata_no(uint64_t sigdata)
>> +{
>> +    return sigdata & 0x3f;
>> +}
>> +
>> +static void do_raise(CPUTLGState *env)
>> +{
>> +    target_siginfo_t info;
>> +
>> +    info.si_signo = sigdata_no(env->sigdata);
>> +    info.si_errno = 0;
>> +    info.si_code = sigdata_code(env->sigdata);
>> +    info._sifields._sigfault._addr = env->pc;
>> +    queue_signal(env, info.si_signo, &info);
>> +}
> 
> 
> This should look much more like the linux kernel code, where instead of 
> passing
> "sigdata", we load the instruction that faulted and parse out the data.  You
> should only need TILEGX_EXCP_OPCODE_ILL.
>

OK, thank, I will try, next.

>> 
>> +    env->spregs[TILEGX_SPR_EX_CONTEXT_1] = 
>> TILEGX_PL_ICS_EX1(TILEGX_USER_PL, 1);
>> +    env->regs[TILEGX_R_SP] = (unsigned long) frame;
>> +    env->regs[TILEGX_R_LR] = restorer;
>> +    env->regs[0] = (unsigned long) sig;
>> +    env->regs[1] = (unsigned long) &frame->info;
>> +    env->regs[2] = (unsigned long) &frame->uc;
>> +    /* regs->flags |= PT_FLAGS_CALLER_SAVES; FIXME: we can skip it? */
>> +
>> +    unlock_user_struct(frame, frame_addr, 1);
>> +    return;
>> +
>> +give_sigsegv:
>> +    if (sig == TARGET_SIGSEGV) {
>> +        ka->_sa_handler = TARGET_SIG_DFL;
>> +    }
>> +    force_sig(TARGET_SIGSEGV /* , current */);
>> +}
>> +
>> +/* kernel: arch/tile/kernel/signal.c */
>> +long do_rt_sigreturn(CPUArchState *env)
>> +{
>> +    fprintf(stderr, "do_rt_sigreturn: not implemented\n");
>> +    return -TARGET_ENOSYS;
>> +}
>> +
> 
> The introduction of signal handling needs to be it's own patch.
> 

OK, thanks.

> I'm also disapointed that you didn't fill in rt_sigreturn; there's no point in
> not doing them together.
> 

This patch is only for raise insn, so I only implement the related code
for raise.

But, OK, I will let them together in one patch, next.

>>  
>>  #include "exec/cpu-defs.h"
>> +#include "spr_def_64.h"
>>  
>> +#define TILEGX_EX1_PL(ex1) \
>> +  (((ex1) >> SPR_EX_CONTEXT_1_1__PL_SHIFT) & SPR_EX_CONTEXT_1_1__PL_RMASK)
>> +#define TILEGX_EX1_ICS(ex1) \
>> +  (((ex1) >> SPR_EX_CONTEXT_1_1__ICS_SHIFT) & SPR_EX_CONTEXT_1_1__ICS_RMASK)
>> +#define TILEGX_PL_ICS_EX1(pl, ics) \
>> +  (((pl) << SPR_EX_CONTEXT_1_1__PL_SHIFT) | \
>> +   ((ics) << SPR_EX_CONTEXT_1_1__ICS_SHIFT))
>> +
>> +#define TILEGX_USER_PL 0
> 
> What's this for?  It appears to be something unrelated and not used.
>

The previous setup_rt_frame() will use some of them.

But it is OK for me to move them to the related file. 
 
>>  
>>      case OE(ADDLI_OPCODE_X0, 0, X0):
>>      case OE(ADDLI_OPCODE_X1, 0, X1):
>> -        tcg_gen_addi_tl(tdest, tsrca, imm);
>> +        if ((srca == TILEGX_R_ZERO) && (dest == TILEGX_R_ZERO)) {
>> +            t0 = tcg_const_tl(imm & 0xffff);
>> +            tcg_gen_st_tl(t0, cpu_env, offsetof(CPUTLGState, sigdata));
>> +            tcg_temp_free(t0);
>> +        } else {
>> +            tcg_gen_addi_tl(tdest, tsrca, imm);
>> +        }
>>          mnemonic = "addli";
>>          break;
>>      case OE(ADDXLI_OPCODE_X0, 0, X0):
>>
> 
> Certainly you should not be complicating addli like this.
> 

OK, I'll try.


Thanks.
-- 
Chen Gang (陈刚)

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