On Fri, 24 Sept 2021 at 17:59, Richard Henderson
<richard.hender...@linaro.org> wrote:
>
> The real kernel has to load the instruction and extract
> the imm5 field; for qemu, modify the translator to do this.
>
> The use of R_AT for this in cpu_loop was a bug. Handle
> the other trap numbers as per the kernel's trap_table.
>
> Signed-off-by: Richard Henderson <richard.hender...@linaro.org>
> ---
> target/nios2/cpu.h | 5 +++--
> linux-user/nios2/cpu_loop.c | 35 ++++++++++++++++++-----------------
> target/nios2/translate.c | 17 ++++++++++++++++-
> 3 files changed, 37 insertions(+), 20 deletions(-)
>
> diff --git a/target/nios2/cpu.h b/target/nios2/cpu.h
> index 2ab82fdc71..395e4d3281 100644
> --- a/target/nios2/cpu.h
> +++ b/target/nios2/cpu.h
> @@ -158,9 +158,10 @@ struct Nios2CPUClass {
> struct CPUNios2State {
> uint32_t regs[NUM_CORE_REGS];
>
> -#if !defined(CONFIG_USER_ONLY)
> +#ifdef CONFIG_USER_ONLY
> + int trap_code;
> +#else
> Nios2MMU mmu;
> -
> uint32_t irq_pending;
> #endif
> };
Loading the insn and fishing out the imm5 field is about 2
lines of code, isn't it ? It's how we handle similar cases
for other targets. I think I prefer that over putting
linux-user specific fields and handling into the target/nios2
code.
> diff --git a/linux-user/nios2/cpu_loop.c b/linux-user/nios2/cpu_loop.c
> index 34290fb3b5..246293a501 100644
> --- a/linux-user/nios2/cpu_loop.c
> +++ b/linux-user/nios2/cpu_loop.c
> @@ -39,9 +39,10 @@ void cpu_loop(CPUNios2State *env)
> case EXCP_INTERRUPT:
> /* just indicate that signals should be handled asap */
> break;
> +
> case EXCP_TRAP:
> - if (env->regs[R_AT] == 0) {
> - abi_long ret;
> + switch (env->trap_code) {
> + case 0:
> qemu_log_mask(CPU_LOG_INT, "\nSyscall\n");
>
> ret = do_syscall(env, env->regs[2],
> @@ -55,26 +56,26 @@ void cpu_loop(CPUNios2State *env)
>
> env->regs[2] = abs(ret);
> /* Return value is 0..4096 */
> - env->regs[7] = (ret > 0xfffffffffffff000ULL);
> - env->regs[CR_ESTATUS] = env->regs[CR_STATUS];
> - env->regs[CR_STATUS] &= ~0x3;
> - env->regs[R_EA] = env->regs[R_PC] + 4;
> + env->regs[7] = ret > 0xfffff000u;
> env->regs[R_PC] += 4;
> break;
> - } else {
> - qemu_log_mask(CPU_LOG_INT, "\nTrap\n");
>
> - env->regs[CR_ESTATUS] = env->regs[CR_STATUS];
> - env->regs[CR_STATUS] &= ~0x3;
> - env->regs[R_EA] = env->regs[R_PC] + 4;
> - env->regs[R_PC] = cpu->exception_addr;
> -
> - info.si_signo = TARGET_SIGTRAP;
> - info.si_errno = 0;
> - info.si_code = TARGET_TRAP_BRKPT;
> - queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
> + case 1:
> + qemu_log_mask(CPU_LOG_INT, "\nTrap 1\n");
> + force_sig_fault(TARGET_SIGUSR1, 0, env->regs[R_PC]);
> + break;
> + case 2:
> + qemu_log_mask(CPU_LOG_INT, "\nTrap 2\n");
> + force_sig_fault(TARGET_SIGUSR2, 0, env->regs[R_PC]);
> + break;
> + default:
> + qemu_log_mask(CPU_LOG_INT, "\nTrap %d\n", env->trap_code);
> + force_sig_fault(TARGET_SIGILL, TARGET_ILL_ILLTRP,
> + env->regs[R_PC]);
> break;
> }
The kernel also defines:
* trap 31 ("breakpoint"), which should wind PC back by 4 and
send a SIGTRAP/TRAP_BRKPT
* trap 30 ("KGDB breakpoint"), which we should treat the same
as the "default" case since we should be acting like "kernel
with CONFIG_KGDB not defined"
Side note: the kernel code for the "CONFIG_KGDB not defined" case
of trap 30 seems buggy to me. It points the trap at 'instruction_trap',
but that is the "emulate multiply and divide insns" entry point, and
that emulation code assumes that it really is getting a mul or div,
not a trap, so I think it will do something bogus. This seems to
be an error introduced in kernel commit baa54ab93c2e1, which refactored
trap handling and changed the reserved-trap-number handling from
"instruction_trap" to "handle_trap_reserved" but forgot this one entry.
> + break;
> +
> case EXCP_DEBUG:
> info.si_signo = TARGET_SIGTRAP;
> info.si_errno = 0;
thanks
-- PMM
