Re: [PATCH v3 16/27] linux-user/nios2: Properly emulate EXCP_TRAP
On 9/27/21 9:23 AM, Peter Maydell wrote:
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.
And we actively get it wrong, e.g. mips.
So I have patches to move that code *out* of linux-user.
We have macros in target/nios2/ to do the field decode, and not in linux-user/.
I think I prefer that over putting
linux-user specific fields and handling into the target/nios2
code.
Would you prefer a generic-y named field like error_code, which we include in
other targets?
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"
Dang it, how did I miss those?
r~
Re: [PATCH v3 16/27] linux-user/nios2: Properly emulate EXCP_TRAP
On Fri, 24 Sept 2021 at 17:59, Richard Henderson
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
> ---
> 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 > 0xf000ULL);
> -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 > 0xf000u;
> 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
[PATCH v3 16/27] linux-user/nios2: Properly emulate EXCP_TRAP
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
---
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
};
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 > 0xf000ULL);
-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 > 0xf000u;
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;
}
+break;
+
case EXCP_DEBUG:
info.si_signo = TARGET_SIGTRAP;
info.si_errno = 0;
diff --git a/target/nios2/translate.c b/target/nios2/translate.c
index 08d7ac5398..485b487665 100644
--- a/target/nios2/translate.c
+++ b/target/nios2/translate.c
@@ -636,6 +636,21 @@ static void divu(DisasContext *dc, uint32_t code, uint32_t
flags)
tcg_temp_free(t0);
}
+static void trap(DisasContext *dc, uint32_t code, uint32_t flags)
+{
+#ifdef CONFIG_USER_ONLY
+/*
+ * The imm5 field is not stored anywhere on real hw; the kernel
+ * has to load the insn and extract the field. But we can make
+ * things easier for cpu_loop if we pop this into env->trap_code.
+ */
+R_TYPE(instr, code);
+tcg_gen_st_i32(tcg_constant_i32(instr.imm5), cpu_env,
+ offsetof(CPUNios2State, trap_code));
+#endif
+t_gen_helper_raise_exception(dc, EXCP_TRAP);
+}
+
static const Nios2Instruction r_type_instructions[] = {
INSTRUCTION_ILLEGAL(),
INSTRUCTION(eret),/* eret */
@@ -682,7 +697,7 @@ static const Nios2Instruction r_type_instructions[] = {
INSTRUCTION_ILLEGAL(),
INSTRUCTION_ILLEGAL(),
INSTRUCTION_ILLEGAL(),
-INSTRUCTION_FLG(gen_excp, EXCP_TRAP), /* trap */
+INSTRUCTION(trap),/* trap */
INSTRUCTION(wrctl), /* wrctl */
INSTRUCTION_ILLEGAL(),
INSTRUCTION_FLG(gen_cmpxx, TCG_COND_LTU), /* cmpltu */
--
2.25.1
