Re: [Qemu-devel] [PATCH v2 4/5] target/tricore: Implement a qemu excptions helper

[Richard Henderson]

On 8/21/19 4:05 PM, Richard Henderson wrote:
> On 8/21/19 5:23 AM, Bastian Koppelmann wrote:
>> @@ -3928,7 +3937,7 @@ static void decode_sr_system(DisasContext *ctx)
>>  ctx->base.is_jmp = DISAS_NORETURN;
>>  break;
>>  case OPC2_16_SR_DEBUG:
>> -/* raise EXCP_DEBUG */
>> +generate_qemu_excp(ctx, EXCP_DEBUG);
>>  break;
>>  case OPC2_16_SR_FRET:
>>  gen_fret(ctx);
>> @@ -8354,7 +8363,7 @@ static void decode_sys_interrupts(DisasContext *ctx)
>>  
>>  switch (op2) {
>>  case OPC2_32_SYS_DEBUG:
>> -/* raise EXCP_DEBUG */
>> +generate_qemu_excp(ctx, EXCP_DEBUG);
>>  break;
>>  case OPC2_32_SYS_DISABLE:
>>  tcg_gen_andi_tl(cpu_ICR, cpu_ICR, ~MASK_ICR_IE_1_3);
> 
> This is not correct -- EXCP_DEBUG is an internal qemu exception.
> 
> The manual I have only describes the ISA and does not describe what a "Debug
> Event" would be.  I note that you're missing the DBGSR.DE check.  I also note
> that whatever a "Debug Event" is, RFM appears to be the return from it.  So 
> one
> can deduce some things about what it should be.

Anyway, remove these hunks and the rest of the patch is ok.
Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH v2 4/5] target/tricore: Implement a qemu excptions helper

[Richard Henderson]

On 8/21/19 5:23 AM, Bastian Koppelmann wrote:
> @@ -3928,7 +3937,7 @@ static void decode_sr_system(DisasContext *ctx)
>  ctx->base.is_jmp = DISAS_NORETURN;
>  break;
>  case OPC2_16_SR_DEBUG:
> -/* raise EXCP_DEBUG */
> +generate_qemu_excp(ctx, EXCP_DEBUG);
>  break;
>  case OPC2_16_SR_FRET:
>  gen_fret(ctx);
> @@ -8354,7 +8363,7 @@ static void decode_sys_interrupts(DisasContext *ctx)
>  
>  switch (op2) {
>  case OPC2_32_SYS_DEBUG:
> -/* raise EXCP_DEBUG */
> +generate_qemu_excp(ctx, EXCP_DEBUG);
>  break;
>  case OPC2_32_SYS_DISABLE:
>  tcg_gen_andi_tl(cpu_ICR, cpu_ICR, ~MASK_ICR_IE_1_3);

This is not correct -- EXCP_DEBUG is an internal qemu exception.

The manual I have only describes the ISA and does not describe what a "Debug
Event" would be.  I note that you're missing the DBGSR.DE check.  I also note
that whatever a "Debug Event" is, RFM appears to be the return from it.  So one
can deduce some things about what it should be.


r~

[Qemu-devel] [PATCH v2 4/5] target/tricore: Implement a qemu excptions helper

[Bastian Koppelmann]

this helper is only used to raise qemu specific exceptions. We use this
helper to raise it on breakpoints as well as the TriCore debug insn.

Signed-off-by: Bastian Koppelmann 
---
 target/tricore/helper.h|  1 +
 target/tricore/op_helper.c |  7 +++
 target/tricore/translate.c | 24 +---
 3 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/target/tricore/helper.h b/target/tricore/helper.h
index b64780c37d..78176aa17a 100644
--- a/target/tricore/helper.h
+++ b/target/tricore/helper.h
@@ -153,3 +153,4 @@ DEF_HELPER_2(psw_write, void, env, i32)
 DEF_HELPER_1(psw_read, i32, env)
 /* Exceptions */
 DEF_HELPER_3(raise_exception_sync, noreturn, env, i32, i32)
+DEF_HELPER_2(qemu_excp, noreturn, env, i32)
diff --git a/target/tricore/op_helper.c b/target/tricore/op_helper.c
index 9476d10d00..32c2bc1699 100644
--- a/target/tricore/op_helper.c
+++ b/target/tricore/op_helper.c
@@ -107,6 +107,13 @@ static void raise_exception_sync_helper(CPUTriCoreState 
*env, uint32_t class,
 raise_exception_sync_internal(env, class, tin, pc, 0);
 }
 
+void helper_qemu_excp(CPUTriCoreState *env, uint32_t excp)
+{
+CPUState *cs = env_cpu(env);
+cs->exception_index = excp;
+cpu_loop_exit(cs);
+}
+
 /* Addressing mode helper */
 
 static uint16_t reverse16(uint16_t val)
diff --git a/target/tricore/translate.c b/target/tricore/translate.c
index 5fb42abe35..19a0e4554c 100644
--- a/target/tricore/translate.c
+++ b/target/tricore/translate.c
@@ -3261,6 +3261,15 @@ static void generate_trap(DisasContext *ctx, int class, 
int tin)
 tcg_temp_free(tintemp);
 }
 
+static void generate_qemu_excp(DisasContext *ctx, int excp)
+{
+TCGv_i32 tmp = tcg_const_i32(excp);
+gen_save_pc(ctx->base.pc_next);
+gen_helper_qemu_excp(cpu_env, tmp);
+ctx->base.is_jmp = DISAS_NORETURN;
+tcg_temp_free(tmp);
+}
+
 static inline void gen_branch_cond(DisasContext *ctx, TCGCond cond, TCGv r1,
TCGv r2, int16_t address)
 {
@@ -3928,7 +3937,7 @@ static void decode_sr_system(DisasContext *ctx)
 ctx->base.is_jmp = DISAS_NORETURN;
 break;
 case OPC2_16_SR_DEBUG:
-/* raise EXCP_DEBUG */
+generate_qemu_excp(ctx, EXCP_DEBUG);
 break;
 case OPC2_16_SR_FRET:
 gen_fret(ctx);
@@ -8354,7 +8363,7 @@ static void decode_sys_interrupts(DisasContext *ctx)
 
 switch (op2) {
 case OPC2_32_SYS_DEBUG:
-/* raise EXCP_DEBUG */
+generate_qemu_excp(ctx, EXCP_DEBUG);
 break;
 case OPC2_32_SYS_DISABLE:
 tcg_gen_andi_tl(cpu_ICR, cpu_ICR, ~MASK_ICR_IE_1_3);
@@ -8808,7 +8817,16 @@ static void tricore_tr_insn_start(DisasContextBase 
*dcbase, CPUState *cpu)
 static bool tricore_tr_breakpoint_check(DisasContextBase *dcbase, CPUState 
*cpu,
   const CPUBreakpoint *bp)
 {
-return false;
+DisasContext *ctx = container_of(dcbase, DisasContext, base);
+generate_qemu_excp(ctx, EXCP_DEBUG);
+/*
+ * The address covered by the breakpoint must be included in
+ * [tb->pc, tb->pc + tb->size) in order to for it to be
+ * properly cleared -- thus we increment the PC here so that
+ * the logic setting tb->size below does the right thing.
+ */
+ctx->base.pc_next += 4;
+return true;
 }
 
 static void tricore_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
-- 
2.23.0