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

[Richard Henderson]

You forgot to cc qemu-devel.

This patch needs to be split.

On 09/22/2015 03:38 PM, address@hidden wrote:
> From: Chen Gang <address@hidden>
> 
> It passes the related test.
> 
> Signed-off-by: Chen Gang <address@hidden>
> ---
>  linux-user/main.c         |  25 +++++++++
>  linux-user/signal.c       | 129 
> +++++++++++++++++++++++++++++++++++++++++++++-
>  target-tilegx/cpu.h       |  14 +++++
>  target-tilegx/translate.c |  15 +++++-
>  4 files changed, 180 insertions(+), 3 deletions(-)
> 
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 782037d..55a2670 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -3414,6 +3414,27 @@ void cpu_loop(CPUS390XState *env)
>  
>  #ifdef TARGET_TILEGX
>  
> +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.

> +
>  static void gen_sigsegv_maperr(CPUTLGState *env, target_ulong addr)
>  {
>      target_siginfo_t info;
> @@ -3622,10 +3643,14 @@ void cpu_loop(CPUTLGState *env)
>          case TILEGX_EXCP_OPCODE_FETCHOR4:
>              do_fetch(env, trapnr, false);
>              break;
> +        case TILEGX_EXCP_OPCODE_ILL:
>          case TILEGX_EXCP_REG_IDN_ACCESS:
>          case TILEGX_EXCP_REG_UDN_ACCESS:
>              gen_sigill_reg(env);
>              break;
> +        case TILEGX_EXCP_OPCODE_RAISE:
> +            do_raise(env);
> +            break;
>          case TILEGX_EXCP_SEGV:
>              gen_sigsegv_maperr(env, env->excaddr);
>              break;
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 502efd9..29144e5 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -5543,6 +5543,132 @@ long do_rt_sigreturn(CPUAlphaState *env)
>      force_sig(TARGET_SIGSEGV);
>  }
>  
> +#elif defined(TARGET_TILEGX)
> +
> +struct target_sigcontext {
> +    union {
> +        /* General-purpose registers.  */
> +        abi_ulong gregs[56];
> +        struct {
> +            abi_ulong __gregs[53];
> +            abi_ulong tp;        /* Aliases gregs[TREG_TP].  */
> +            abi_ulong sp;        /* Aliases gregs[TREG_SP].  */
> +            abi_ulong lr;        /* Aliases gregs[TREG_LR].  */
> +        };
> +    };
> +    abi_ulong pc;        /* Program counter.  */
> +    abi_ulong ics;       /* In Interrupt Critical Section?  */
> +    abi_ulong faultnum;  /* Fault number.  */
> +    abi_ulong pad[5];
> +};
> +
> +struct target_ucontext {
> +    abi_ulong tuc_flags;
> +    abi_ulong tuc_link;
> +    target_stack_t tuc_stack;
> +    struct target_sigcontext tuc_mcontext;
> +    target_sigset_t tuc_sigmask;   /* mask last for extensibility */
> +};
> +
> +struct target_rt_sigframe {
> +    unsigned char save_area[16]; /* caller save area */
> +    struct target_siginfo info;
> +    struct target_ucontext uc;
> +};
> +
> +static void setup_sigcontext(struct target_sigcontext *sc,
> +                             CPUArchState *env, int signo)
> +{
> +    int i;
> +
> +    for (i = 0; i < TILEGX_R_COUNT; ++i) {
> +        __put_user(env->regs[i], &sc->gregs[i]);
> +    }
> +
> +    __put_user(env->pc, &sc->pc);
> +    __put_user(0, &sc->ics);
> +    __put_user(signo, &sc->faultnum);
> +}
> +
> +static abi_ulong get_sigframe(struct target_sigaction *ka, CPUArchState *env,
> +                              size_t frame_size)
> +{
> +    unsigned long sp = env->regs[TILEGX_R_SP];
> +
> +    if (on_sig_stack(sp) && !likely(on_sig_stack(sp - frame_size))) {
> +        return -1UL;
> +    }
> +
> +    if ((ka->sa_flags & SA_ONSTACK) && !sas_ss_flags(sp)) {
> +        sp = target_sigaltstack_used.ss_sp + target_sigaltstack_used.ss_size;
> +    }
> +
> +    sp -= frame_size;
> +    sp &= -16UL;
> +    return sp;
> +}
> +
> +static void setup_rt_frame(int sig, struct target_sigaction *ka,
> +                           target_siginfo_t *info,
> +                           target_sigset_t *set, CPUArchState *env)
> +{
> +    abi_ulong frame_addr;
> +    struct target_rt_sigframe *frame;
> +    unsigned long restorer;
> +
> +    frame_addr = get_sigframe(ka, env, sizeof(*frame));
> +    if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0)) {
> +        goto give_sigsegv;
> +    }
> +
> +    /* Always write at least the signal number for the stack backtracer. */
> +    if (ka->sa_flags & TARGET_SA_SIGINFO) {
> +        /* At sigreturn time, restore the callee-save registers too. */
> +        tswap_siginfo(&frame->info, info);
> +        /* regs->flags |= PT_FLAGS_RESTORE_REGS; FIXME: we can skip it? */
> +    } else {
> +        __put_user(info->si_signo, &frame->info.si_signo);
> +    }
> +
> +    /* Create the ucontext.  */
> +    __put_user(0, &frame->uc.tuc_flags);
> +    __put_user(0, &frame->uc.tuc_link);
> +    __put_user(target_sigaltstack_used.ss_sp, &frame->uc.tuc_stack.ss_sp);
> +    __put_user(sas_ss_flags(env->regs[TILEGX_R_SP]),
> +               &frame->uc.tuc_stack.ss_flags);
> +    __put_user(target_sigaltstack_used.ss_size, 
> &frame->uc.tuc_stack.ss_size);
> +    setup_sigcontext(&frame->uc.tuc_mcontext, env, info->si_signo);
> +
> +    restorer = (unsigned long) do_rt_sigreturn;
> +    if (ka->sa_flags & TARGET_SA_RESTORER) {
> +            restorer = (unsigned long) ka->sa_restorer;
> +    }
> +    env->pc = (unsigned long) ka->_sa_handler;
> +    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.

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

>  #else
>  
>  static void setup_frame(int sig, struct target_sigaction *ka,
> @@ -5662,7 +5788,8 @@ void process_pending_signals(CPUArchState *cpu_env)
>          }
>  #endif
>          /* prepare the stack frame of the virtual CPU */
> -#if defined(TARGET_ABI_MIPSN32) || defined(TARGET_ABI_MIPSN64)
> +#if defined(TARGET_ABI_MIPSN32) || defined(TARGET_ABI_MIPSN64) \
> +    || defined(TARGET_TILEGX)
>          /* These targets do not have traditional signals.  */
>          setup_rt_frame(sig, sa, &q->info, &target_old_set, cpu_env);
>  #else
> diff --git a/target-tilegx/cpu.h b/target-tilegx/cpu.h
> index b9f5082..6b28d6e 100644
> --- a/target-tilegx/cpu.h
> +++ b/target-tilegx/cpu.h
> @@ -27,7 +27,17 @@
>  #define CPUArchState struct CPUTLGState
>  
>  #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.

>  
>  /* TILE-Gx common register alias */
>  #define TILEGX_R_RE    0   /*  0 register, for function/syscall return value 
> */
> @@ -53,6 +63,7 @@ enum {
>      TILEGX_SPR_CMPEXCH = 0,
>      TILEGX_SPR_CRITICAL_SEC = 1,
>      TILEGX_SPR_SIM_CONTROL = 2,
> +    TILEGX_SPR_EX_CONTEXT_1 = 3,
>      TILEGX_SPR_COUNT
>  };
>  
> @@ -75,6 +86,8 @@ typedef enum {
>      TILEGX_EXCP_OPCODE_FETCHAND4 = 0x10c,
>      TILEGX_EXCP_OPCODE_FETCHOR = 0x10d,
>      TILEGX_EXCP_OPCODE_FETCHOR4 = 0x10e,
> +    TILEGX_EXCP_OPCODE_ILL = 0x10f,
> +    TILEGX_EXCP_OPCODE_RAISE = 0x110,
>      TILEGX_EXCP_REG_IDN_ACCESS = 0x181,
>      TILEGX_EXCP_REG_UDN_ACCESS = 0x182,
>      TILEGX_EXCP_UNALIGNMENT = 0x201,
> @@ -86,6 +99,7 @@ typedef struct CPUTLGState {
>      uint64_t spregs[TILEGX_SPR_COUNT]; /* Special used registers by outside 
> */
>      uint64_t pc;                       /* Current pc */
>  
> +    uint64_t sigdata;                  /* signal data */
>  #if defined(CONFIG_USER_ONLY)
>      uint64_t atomic_srca;              /* Arguments to atomic "exceptions" */
>      uint64_t atomic_srcb;
> diff --git a/target-tilegx/translate.c b/target-tilegx/translate.c
> index a9fc4ce..1493b78 100644
> --- a/target-tilegx/translate.c
> +++ b/target-tilegx/translate.c
> @@ -407,10 +407,15 @@ static TileExcp gen_rr_opcode(DisasContext *dc, 
> unsigned opext,
>          mnemonic = "flushwb";
>          goto done0;
>      case OE_RR_X1(ILL):
> +        if (dest == 0x1d && srca == 0x25) {
> +            qemu_log_mask(CPU_LOG_TB_IN_ASM, "%s", "raise");
> +            return TILEGX_EXCP_OPCODE_RAISE;
> +        }
> +        /* Fall through */
>      case OE_RR_Y1(ILL):
>          mnemonic = (dest == 0x1c && srca == 0x25 ? "bpt" : "ill");
>          qemu_log_mask(CPU_LOG_TB_IN_ASM, "%s", mnemonic);
> -        return TILEGX_EXCP_OPCODE_UNKNOWN;
> +        return TILEGX_EXCP_OPCODE_ILL;
>      case OE_RR_X1(MF):
>          mnemonic = "mf";
>          goto done0;
> @@ -1520,7 +1525,13 @@ static TileExcp gen_rri_opcode(DisasContext *dc, 
> unsigned opext,
>  
>      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.


r~