target/riscv: Fix icount trigger exception PC
PATCH 1/1 target/riscv: Fix icount trigger exception PC
Per the Sdtrig specification, on a breakpoint exception with icount, the xepc must contain the virtual address of the next instruction that must be executed to preserve the program flow.
Since SBI DBTR exposes these trigger encodings to supervisor software, an icount trigger used as a one-instruction step must fire only after the counted instruction retires and the architectural successor PC is known, which for a taken branch is the branch target, not the trigger install address or fallthrough.
QEMU previously disabled the translated-block trigger flag when icount was enabled and relied on timer and privilege-change callbacks to update the count, so the trigger could fire outside instruction retirement before the guest PC advanced, causing Linux to observe the old PC and re-enter the same breakpoint while single-stepping with icount.
Fix this by
- publishing the icount trigger flag in translated blocks even with icount enabled
- making the helper decrement the trigger count after the instruction has committed its successor PC and raise the trigger action only then
- privilege-change callbacks refresh cached enabled state because itriggers tied to privilege mode.
Also make trigger data CSR availability depend on an implemented trigger slot rather than the currently selected trigger type, so software can disable a trigger with type zero and still write the rest of the slot state before selecting another supported type.
itrigger timers are completely removed as they are not required now.
Signed-off-by Himanshu Chauhan <[email protected]>
--- target/riscv/cpu.h | 5 +- target/riscv/tcg/cpu_helper.c | 10 +- target/riscv/tcg/debug.c | 169 ++++++++++------------------------ target/riscv/tcg/tcg-cpu.c | 7 +- 4 files changed, 59 insertions(+), 132 deletions(-)
target/riscv/cpu.h +1 -4
@@ -484,8 +484,7 @@ struct CPUArchState {uint8_t trigger_cur;/* * num_triggers is the length of tdata1, tdata2, tdata3, -* cpu_breakpoint, cpu_watchpoint and itrigger_timer -* arrays. +* cpu_breakpoint and cpu_watchpoint arrays. */ uint32_t num_triggers;uint64_t *tdata1;
@@ -493,8 +492,6 @@ struct CPUArchState {uint64_t *tdata3;struct CPUBreakpoint **cpu_breakpoint;struct CPUWatchpoint **cpu_watchpoint; -QEMUTimer **itrigger_timer; -int64_t last_icount;bool itrigger_enabled;/* machine specific rdtime callback */
target/riscv/tcg/cpu_helper.c +6 -4
@@ -940,10 +940,6 @@ void riscv_cpu_set_mode(CPURISCVState *env, privilege_mode_t newpriv,g_assert (newpriv <=PRV_M && newpriv !=PRV_RESERVED );if (newpriv != env->priv || env->virt_enabled != virt_en) { -if (icount_enabled ()) { -riscv_itrigger_update_priv (env); - } -riscv_pmu_update_fixed_ctrs (env, newpriv, virt_en); }
@@ -982,6 +978,12 @@ void riscv_cpu_set_mode(CPURISCVState *env, privilege_mode_t newpriv,riscv_cpu_interrupt (env); } } + +/* +* Icount triggers are scoped by U/S/M and VU/VS mode bits. Refresh the +* cached translated-block flag after privilege or virtualization changes. +*/ +riscv_itrigger_update_priv (env); }/*
target/riscv/tcg/debug.c +47 -122
279 lines not on this page · raw message
target/riscv/tcg/tcg-cpu.c +5 -2
@@ -181,8 +181,11 @@ static TCGTBCPUState riscv_get_tb_cpu_state(CPUState *cs) ? EXT_STATUS_DIRTY : EXT_STATUS_DISABLED; } -if ((cpu->cfg.debug || cpu->cfg.ext_sdtrig) - && !icount_enabled ()) { +/* +* Publish itrigger state even when QEMU icount is enabled. The helper +* runs from translated code after committing the guest successor PC. +*/ +if (cpu->cfg.debug || cpu->cfg.ext_sdtrig) { flags =FIELD_DP32 (flags,TB_FLAGS ,ITRIGGER , env->itrigger_enabled); }#endif
Signature · 1 line
2.43.0
Quoted 30 lines · Himanshu Chauhan
Per the Sdtrig specification, on a breakpoint exception with icount, the xepc must contain the virtual address of the next instruction that must be executed to preserve the program flow.
Since SBI DBTR exposes these trigger encodings to supervisor software, an icount trigger used as a one-instruction step must fire only after the counted instruction retires and the architectural successor PC is known, which for a taken branch is the branch target, not the trigger install address or fallthrough.
QEMU previously disabled the translated-block trigger flag when icount was enabled and relied on timer and privilege-change callbacks to update the count, so the trigger could fire outside instruction retirement before the guest PC advanced, causing Linux to observe the old PC and re-enter the same breakpoint while single-stepping with icount.
Fix this by
- publishing the icount trigger flag in translated blocks even with icount enabled
- making the helper decrement the trigger count after the instruction has committed its successor PC and raise the trigger action only then
- privilege-change callbacks refresh cached enabled state because itriggers tied to privilege mode.Also make trigger data CSR availability depend on an implemented trigger slot rather than the currently selected trigger type, so software can disable a trigger with type zero and still write the rest of the slot state before selecting another supported type.
Quoted 4 lines
itrigger timers are completely removed as they are not required now.
Signed-off-by Himanshu Chauhan <[email protected]>---
LGTM. I took a look at other instances of itrigger_enabled and seems like we're missing one instance in this patch:
target/riscv/machine.c, debug_post_load():
RISCVCPU *cpu = opaque;
CPURISCVState *env = &cpu->env;
if (icount_enabled()) {
env->itrigger_enabled = riscv_itrigger_enabled(env);
}Since itrigger_enabled is no longer reliant on icount_enabled() we need to remove the icount_enabled() from machine.c too.
Thanks,
Daniel
Quoted 373 lines
target/riscv/cpu.h | 5 +- target/riscv/tcg/cpu_helper.c | 10 +- target/riscv/tcg/debug.c | 169 ++++++++++------------------------ target/riscv/tcg/tcg-cpu.c | 7 +- 4 files changed, 59 insertions(+), 132 deletions(-)target/riscv/cpu.h +1
-4@@ -484,8 +484,7 @@ struct CPUArchState {uint8_t trigger_cur;/* * num_triggers is the length of tdata1, tdata2, tdata3, -* cpu_breakpoint, cpu_watchpoint and itrigger_timer -* arrays. +* cpu_breakpoint and cpu_watchpoint arrays. */ uint32_t num_triggers;uint64_t *tdata1;@@ -493,8 +492,6 @@ struct CPUArchState {uint64_t *tdata3;struct CPUBreakpoint **cpu_breakpoint;struct CPUWatchpoint **cpu_watchpoint; -QEMUTimer **itrigger_timer; -int64_t last_icount;bool itrigger_enabled;/* machine specific rdtime callback */ target/riscv/tcg/cpu_helper.c +6
-4@@ -940,10 +940,6 @@ void riscv_cpu_set_mode(CPURISCVState *env, privilege_mode_t newpriv,g_assert (newpriv <=PRV_M && newpriv !=PRV_RESERVED );if (newpriv != env->priv || env->virt_enabled != virt_en) { -if (icount_enabled ()) { -riscv_itrigger_update_priv (env); - } -riscv_pmu_update_fixed_ctrs (env, newpriv, virt_en); }@@ -982,6 +978,12 @@ void riscv_cpu_set_mode(CPURISCVState *env, privilege_mode_t newpriv,riscv_cpu_interrupt (env); } } + +/* +* Icount triggers are scoped by U/S/M and VU/VS mode bits. Refresh the +* cached translated-block flag after privilege or virtualization changes. +*/ +riscv_itrigger_update_priv (env); }/* target/riscv/tcg/debug.c +47
-122@@ -32,8 +32,6 @@#include "trace.h" #include "exec/helper-proto.h" #include "exec/watchpoint.h" -#include "system/cpu-timers.h" -#include "exec/icount.h" #include "accel/tcg/cpu-loop.h" /* @@ -53,20 +51,6 @@* 1 | 2 | tdata1, tdata2 | Address / Data Match */ -/* tdata availability of a trigger */ -typedef bool tdata_avail [TDATA_NUM ]; - -static tdata_avail tdata_mapping[TRIGGER_TYPE_NUM ] = { - [TRIGGER_TYPE_NO_EXIST ] = { false, false, false }, - [TRIGGER_TYPE_AD_MATCH ] = { true, true, true }, - [TRIGGER_TYPE_INST_CNT ] = { true, false, true }, - [TRIGGER_TYPE_INT ] = { true, true, true }, - [TRIGGER_TYPE_EXCP ] = { true, true, true }, - [TRIGGER_TYPE_AD_MATCH6 ] = { true, true, true }, - [TRIGGER_TYPE_EXT_SRC ] = { true, false, false }, - [TRIGGER_TYPE_UNAVAIL ] = { true, true, true } -}; -/* only breakpoint size 1/2/4/8 supported */ static int access_size[SIZE_NUM ] = { [SIZE_ANY ] =0 ,@@ -165,13 +149,17 @@ static inline target_ulong build_tdata1(CPURISCVState *env,bool tdata_available (CPURISCVState *env,int tdata_index) { -int trigger_type =get_trigger_type (env, env->trigger_cur); -if (unlikely (tdata_index >=TDATA_NUM )) {return false; } -return tdata_mapping[trigger_type][tdata_index]; +/* +* tdata CSRs belong to the trigger slot, not to the currently selected +* trigger type. A type-zero trigger is disabled, but the slot still +* exists, so software must still be able to write tdata2/tdata3 before +* selecting a supported trigger type. +*/ +return env->trigger_cur < env->num_triggers; }target_ulong tselect_csr_read (CPURISCVState *env)@@ -729,6 +717,30 @@ static void type6_reg_write(CPURISCVState *env, target_ulong index, } } +static void no_trigger_reg_write (CPURISCVState *env,target_ulong index, +int tdata_index,target_ulong val) +{ +/* +* OpenSBI programs DBTR slots by first writing type zero to tdata1, then +* writing tdata2/tdata3, and finally writing the desired trigger type. +*/ +switch (tdata_index) { +case TDATA1 : +type2_breakpoint_remove (env, index); + env->tdata1[index] =0 ; + env->itrigger_enabled =riscv_itrigger_enabled (env); +break ; +case TDATA2 : + env->tdata2[index] = val; +break ; +case TDATA3 : + env->tdata3[index] =textra_validate (env, val); +break ; +default : +g_assert_not_reached (); + } +} +/* icount trigger type */ static inline int itrigger_get_count (CPURISCVState *env,int index)@@ -754,21 +766,6 @@ itrigger_set_pending(CPURISCVState *env, int index, int val) ITRIGGER_PENDING, val); } -static bool check_itrigger_priv (CPURISCVState *env,int index) -{ -target_ulong tdata1 = env->tdata1[index]; -if (env->virt_enabled) { -/* check VU/VS bit against current privilege level */ -return (get_field (tdata1,ITRIGGER_VS ) == env->priv) || - (get_field (tdata1,ITRIGGER_VU ) == env->priv); - }else { -/* check U/S/M bit against current privilege level */ -return (get_field (tdata1,ITRIGGER_M ) == env->priv) || - (get_field (tdata1,ITRIGGER_S ) == env->priv) || - (get_field (tdata1,ITRIGGER_U ) == env->priv); - } -} -bool riscv_itrigger_enabled (CPURISCVState *env) {int count;@@ -776,7 +773,7 @@ bool riscv_itrigger_enabled(CPURISCVState *env)if (get_trigger_type (env, i) !=TRIGGER_TYPE_INST_CNT ) {continue ; } -if (check_itrigger_priv (env, i)) { +if (!trigger_common_match (env,TRIGGER_TYPE_INST_CNT , i)) {continue ; } count =itrigger_get_count (env, i);@@ -803,7 +800,12 @@ void HELPER(itrigger_match)(CPURISCVState *env)if (!count) {continue ; } -itrigger_set_count (env, i, count--); +/* +* This helper is emitted at TB exit after one guest instruction and +* after the translator has committed env->pc to the successor PC. +*/ + count--; +itrigger_set_count (env, i, count);if (!count) {/* * From the 1.0 spec: "When pending is set, the trigger @@ -822,61 +824,21 @@ void HELPER(itrigger_match)(CPURISCVState *env)itrigger_set_pending (env, i,1 ); env->itrigger_enabled =riscv_itrigger_enabled (env); -do_trigger_action (env, i);itrigger_set_pending (env, i,0 ); +do_trigger_action (env, i); } } }static void riscv_itrigger_update_count (CPURISCVState *env) { -int count, executed;/* -* Record last icount, so that we can evaluate the executed instructions -* since last privilege mode change or timer expire. +* Icount triggers are matched from translated code after an instruction +* has committed its successor PC. Timer and privilege-change callbacks +* therefore must not decrement count or fire the action here; they only +* refresh the cached TB flag state. */ -int64_t last_icount = env->last_icount, current_icount; - current_icount = env->last_icount =icount_get_raw (); - -for (int i =0 ; i < env->num_triggers; i++) { -if (get_trigger_type (env, i) !=TRIGGER_TYPE_INST_CNT ) { -continue ; - } - count =itrigger_get_count (env, i); -if (!count) { -continue ; - } -/* -* Only when privilege is changed or itrigger timer expires, -* the count field in itrigger tdata1 register is updated. -* And the count field in itrigger only contains remaining value. -*/ -if (check_itrigger_priv (env, i)) { -/* -* If itrigger enabled in this privilege mode, the number of -* executed instructions since last privilege change -* should be reduced from current itrigger count. -*/ - executed = current_icount - last_icount; -itrigger_set_count (env, i, count - executed); -if (count == executed) { -do_trigger_action (env, i); - } - }else { -/* -* If itrigger is not enabled in this privilege mode, -* the number of executed instructions will be discard and -* the count field in itrigger will not change. -*/ -timer_mod (env->itrigger_timer[i], - current_icount + count); - } - } -} - -static void riscv_itrigger_timer_cb (void *opaque) -{ -riscv_itrigger_update_count ((CPURISCVState *)opaque); + env->itrigger_enabled =riscv_itrigger_enabled (env); }void riscv_itrigger_update_priv (CPURISCVState *env)@@ -911,18 +873,10 @@ static void itrigger_reg_write(CPURISCVState *env, target_ulong index,switch (tdata_index) {case TDATA1 : -/* set timer for icount */ new_val =itrigger_validate (env, val);if (new_val != env->tdata1[index]) { env->tdata1[index] = new_val; -if (icount_enabled ()) { - env->last_icount =icount_get_raw (); -/* set the count to timer */ -timer_mod (env->itrigger_timer[index], - env->last_icount +itrigger_get_count (env, index)); - }else { - env->itrigger_enabled =riscv_itrigger_enabled (env); - } + env->itrigger_enabled =riscv_itrigger_enabled (env); }break ;case TDATA2 :@@ -937,27 +891,10 @@ static void itrigger_reg_write(CPURISCVState *env, target_ulong index, } } -static int itrigger_get_adjust_count (CPURISCVState *env) -{ -int count =itrigger_get_count (env, env->trigger_cur), executed; -if ((count !=0 ) &&check_itrigger_priv (env, env->trigger_cur)) { - executed =icount_get_raw () - env->last_icount; - count += executed; - } -return count; -} -target_ulong tdata_csr_read (CPURISCVState *env,int tdata_index) { -int trigger_type;switch (tdata_index) {case TDATA1 : - trigger_type =extract_trigger_type (env, - env->tdata1[env->trigger_cur]); -if ((trigger_type ==TRIGGER_TYPE_INST_CNT ) &&icount_enabled ()) { -return deposit64 (env->tdata1[env->trigger_cur],10 ,14 , -itrigger_get_adjust_count (env)); - }return env->tdata1[env->trigger_cur];case TDATA2 :return env->tdata2[env->trigger_cur];@@ -995,6 +932,8 @@ void tdata_csr_write(CPURISCVState *env, int tdata_index, target_ulong val) trigger_type);break ;case TRIGGER_TYPE_NO_EXIST : +no_trigger_reg_write (env, env->trigger_cur, tdata_index, val); +break ;case TRIGGER_TYPE_UNAVAIL :qemu_log_mask (LOG_GUEST_ERROR ,"trigger type: %d does not exit\n" , trigger_type);@@ -1147,8 +1086,6 @@ bool riscv_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)void riscv_trigger_realize (CPURISCVState *env) { -int i; -if (env->num_triggers >RV_MAX_TRIGGERS ) { error_report( "Invalid configuration: 'num-triggers' must be less than %u",@@ -1161,12 +1098,6 @@ void riscv_trigger_realize(CPURISCVState *env) env->tdata3 =g_new0 (uint64_t, env->num_triggers); env->cpu_breakpoint = g_new0(struct CPUBreakpoint *, env->num_triggers); env->cpu_watchpoint = g_new0(struct CPUWatchpoint *, env->num_triggers); - env->itrigger_timer = g_new0(QEMUTimer *, env->num_triggers); - -for (i =0 ; i < env->num_triggers; i++) { - env->itrigger_timer[i] =timer_new_ns (QEMU_CLOCK_VIRTUAL , - riscv_itrigger_timer_cb, env); - } }void riscv_trigger_unrealize (CPURISCVState *env)@@ -1177,11 +1108,6 @@ void riscv_trigger_unrealize(CPURISCVState *env)g_free (env->cpu_breakpoint);g_free (env->cpu_watchpoint); - -for (int i =0 ; i < env->num_triggers; i++) { -timer_free (env->itrigger_timer[i]); - } -g_free (env->itrigger_timer); }void riscv_trigger_reset_hold (CPURISCVState *env)@@ -1209,7 +1135,6 @@ void riscv_trigger_reset_hold(CPURISCVState *env) env->tdata3[i] =0 ; env->cpu_breakpoint[i] =NULL ; env->cpu_watchpoint[i] =NULL ; -timer_del (env->itrigger_timer[i]); } env->mcontext =0 ;target/riscv/tcg/tcg-cpu.c +5
-2@@ -181,8 +181,11 @@ static TCGTBCPUState riscv_get_tb_cpu_state(CPUState *cs) ? EXT_STATUS_DIRTY : EXT_STATUS_DISABLED; } -if ((cpu->cfg.debug || cpu->cfg.ext_sdtrig) - && !icount_enabled ()) { +/* +* Publish itrigger state even when QEMU icount is enabled. The helper +* runs from translated code after committing the guest successor PC. +*/ +if (cpu->cfg.debug || cpu->cfg.ext_sdtrig) { flags =FIELD_DP32 (flags,TB_FLAGS ,ITRIGGER , env->itrigger_enabled); }#endif
Quoted 42 lines · Daniel Henrique Barboza
Quoted 30 lines · Himanshu Chauhan
Per the Sdtrig specification, on a breakpoint exception with icount, the xepc must contain the virtual address of the next instruction that must be executed to preserve the program flow.
Since SBI DBTR exposes these trigger encodings to supervisor software, an icount trigger used as a one-instruction step must fire only after the counted instruction retires and the architectural successor PC is known, which for a taken branch is the branch target, not the trigger install address or fallthrough.
QEMU previously disabled the translated-block trigger flag when icount was enabled and relied on timer and privilege-change callbacks to update the count, so the trigger could fire outside instruction retirement before the guest PC advanced, causing Linux to observe the old PC and re-enter the same breakpoint while single-stepping with icount.
Fix this by
- publishing the icount trigger flag in translated blocks even with icount enabled
- making the helper decrement the trigger count after the instruction has committed its successor PC and raise the trigger action only then
- privilege-change callbacks refresh cached enabled state because itriggers tied to privilege mode.Also make trigger data CSR availability depend on an implemented trigger slot rather than the currently selected trigger type, so software can disable a trigger with type zero and still write the rest of the slot state before selecting another supported type.
Quoted 4 lines
itrigger timers are completely removed as they are not required now.
Signed-off-by Himanshu Chauhan <[email protected]>---
LGTM. I took a look at other instances of
itrigger_enabledand seems like we're missing one instance in this patch:
Quoted 9 lines
target/riscv/machine.c,debug_post_load():RISCVCPU *cpu = opaque; CPURISCVState *env = &cpu->env; if (icount_enabled()) { env->itrigger_enabled = riscv_itrigger_enabled(env); }
Hi Daniel,
Thanks for looking at it and pointing this out. I will send a revision which fixes this and the sashiko reports.
Regards
Himanshu
Quoted 360 lines
Since
itrigger_enabledis no longer reliant onicount_enabled()we need to remove theicount_enabled()frommachine.ctoo.Thanks,
DanielQuoted 351 lines
target/riscv/cpu.h | 5 +- target/riscv/tcg/cpu_helper.c | 10 +- target/riscv/tcg/debug.c | 169 ++++++++++------------------------ target/riscv/tcg/tcg-cpu.c | 7 +- 4 files changed, 59 insertions(+), 132 deletions(-)target/riscv/cpu.h +1
-4@@ -484,8 +484,7 @@ struct CPUArchState {uint8_t trigger_cur;/* * num_triggers is the length of tdata1, tdata2, tdata3, -* cpu_breakpoint, cpu_watchpoint and itrigger_timer -* arrays. +* cpu_breakpoint and cpu_watchpoint arrays. */ uint32_t num_triggers;uint64_t *tdata1;@@ -493,8 +492,6 @@ struct CPUArchState {uint64_t *tdata3;struct CPUBreakpoint **cpu_breakpoint;struct CPUWatchpoint **cpu_watchpoint; -QEMUTimer **itrigger_timer; -int64_t last_icount;bool itrigger_enabled;/* machine specific rdtime callback */ target/riscv/tcg/cpu_helper.c +6
-4@@ -940,10 +940,6 @@ void riscv_cpu_set_mode(CPURISCVState *env, privilege_mode_t newpriv,g_assert (newpriv <=PRV_M && newpriv !=PRV_RESERVED );if (newpriv != env->priv || env->virt_enabled != virt_en) { -if (icount_enabled ()) { -riscv_itrigger_update_priv (env); - } -riscv_pmu_update_fixed_ctrs (env, newpriv, virt_en); }@@ -982,6 +978,12 @@ void riscv_cpu_set_mode(CPURISCVState *env, privilege_mode_t newpriv,riscv_cpu_interrupt (env); } } + +/* +* Icount triggers are scoped by U/S/M and VU/VS mode bits. Refresh the +* cached translated-block flag after privilege or virtualization changes. +*/ +riscv_itrigger_update_priv (env); }/* target/riscv/tcg/debug.c +47
-122@@ -32,8 +32,6 @@#include "trace.h" #include "exec/helper-proto.h" #include "exec/watchpoint.h" -#include "system/cpu-timers.h" -#include "exec/icount.h" #include "accel/tcg/cpu-loop.h" /* @@ -53,20 +51,6 @@* 1 | 2 | tdata1, tdata2 | Address / Data Match */ -/* tdata availability of a trigger */ -typedef bool tdata_avail [TDATA_NUM ]; - -static tdata_avail tdata_mapping[TRIGGER_TYPE_NUM ] = { - [TRIGGER_TYPE_NO_EXIST ] = { false, false, false }, - [TRIGGER_TYPE_AD_MATCH ] = { true, true, true }, - [TRIGGER_TYPE_INST_CNT ] = { true, false, true }, - [TRIGGER_TYPE_INT ] = { true, true, true }, - [TRIGGER_TYPE_EXCP ] = { true, true, true }, - [TRIGGER_TYPE_AD_MATCH6 ] = { true, true, true }, - [TRIGGER_TYPE_EXT_SRC ] = { true, false, false }, - [TRIGGER_TYPE_UNAVAIL ] = { true, true, true } -}; -/* only breakpoint size 1/2/4/8 supported */ static int access_size[SIZE_NUM ] = { [SIZE_ANY ] =0 ,@@ -165,13 +149,17 @@ static inline target_ulong build_tdata1(CPURISCVState *env,bool tdata_available (CPURISCVState *env,int tdata_index) { -int trigger_type =get_trigger_type (env, env->trigger_cur); -if (unlikely (tdata_index >=TDATA_NUM )) {return false; } -return tdata_mapping[trigger_type][tdata_index]; +/* +* tdata CSRs belong to the trigger slot, not to the currently selected +* trigger type. A type-zero trigger is disabled, but the slot still +* exists, so software must still be able to write tdata2/tdata3 before +* selecting a supported trigger type. +*/ +return env->trigger_cur < env->num_triggers; }target_ulong tselect_csr_read (CPURISCVState *env)@@ -729,6 +717,30 @@ static void type6_reg_write(CPURISCVState *env, target_ulong index, } } +static void no_trigger_reg_write (CPURISCVState *env,target_ulong index, +int tdata_index,target_ulong val) +{ +/* +* OpenSBI programs DBTR slots by first writing type zero to tdata1, then +* writing tdata2/tdata3, and finally writing the desired trigger type. +*/ +switch (tdata_index) { +case TDATA1 : +type2_breakpoint_remove (env, index); + env->tdata1[index] =0 ; + env->itrigger_enabled =riscv_itrigger_enabled (env); +break ; +case TDATA2 : + env->tdata2[index] = val; +break ; +case TDATA3 : + env->tdata3[index] =textra_validate (env, val); +break ; +default : +g_assert_not_reached (); + } +} +/* icount trigger type */ static inline int itrigger_get_count (CPURISCVState *env,int index)@@ -754,21 +766,6 @@ itrigger_set_pending(CPURISCVState *env, int index, int val) ITRIGGER_PENDING, val); } -static bool check_itrigger_priv (CPURISCVState *env,int index) -{ -target_ulong tdata1 = env->tdata1[index]; -if (env->virt_enabled) { -/* check VU/VS bit against current privilege level */ -return (get_field (tdata1,ITRIGGER_VS ) == env->priv) || - (get_field (tdata1,ITRIGGER_VU ) == env->priv); - }else { -/* check U/S/M bit against current privilege level */ -return (get_field (tdata1,ITRIGGER_M ) == env->priv) || - (get_field (tdata1,ITRIGGER_S ) == env->priv) || - (get_field (tdata1,ITRIGGER_U ) == env->priv); - } -} -bool riscv_itrigger_enabled (CPURISCVState *env) {int count;@@ -776,7 +773,7 @@ bool riscv_itrigger_enabled(CPURISCVState *env)if (get_trigger_type (env, i) !=TRIGGER_TYPE_INST_CNT ) {continue ; } -if (check_itrigger_priv (env, i)) { +if (!trigger_common_match (env,TRIGGER_TYPE_INST_CNT , i)) {continue ; } count =itrigger_get_count (env, i);@@ -803,7 +800,12 @@ void HELPER(itrigger_match)(CPURISCVState *env)if (!count) {continue ; } -itrigger_set_count (env, i, count--); +/* +* This helper is emitted at TB exit after one guest instruction and +* after the translator has committed env->pc to the successor PC. +*/ + count--; +itrigger_set_count (env, i, count);if (!count) {/* * From the 1.0 spec: "When pending is set, the trigger @@ -822,61 +824,21 @@ void HELPER(itrigger_match)(CPURISCVState *env)itrigger_set_pending (env, i,1 ); env->itrigger_enabled =riscv_itrigger_enabled (env); -do_trigger_action (env, i);itrigger_set_pending (env, i,0 ); +do_trigger_action (env, i); } } }static void riscv_itrigger_update_count (CPURISCVState *env) { -int count, executed;/* -* Record last icount, so that we can evaluate the executed instructions -* since last privilege mode change or timer expire. +* Icount triggers are matched from translated code after an instruction +* has committed its successor PC. Timer and privilege-change callbacks +* therefore must not decrement count or fire the action here; they only +* refresh the cached TB flag state. */ -int64_t last_icount = env->last_icount, current_icount; - current_icount = env->last_icount =icount_get_raw (); - -for (int i =0 ; i < env->num_triggers; i++) { -if (get_trigger_type (env, i) !=TRIGGER_TYPE_INST_CNT ) { -continue ; - } - count =itrigger_get_count (env, i); -if (!count) { -continue ; - } -/* -* Only when privilege is changed or itrigger timer expires, -* the count field in itrigger tdata1 register is updated. -* And the count field in itrigger only contains remaining value. -*/ -if (check_itrigger_priv (env, i)) { -/* -* If itrigger enabled in this privilege mode, the number of -* executed instructions since last privilege change -* should be reduced from current itrigger count. -*/ - executed = current_icount - last_icount; -itrigger_set_count (env, i, count - executed); -if (count == executed) { -do_trigger_action (env, i); - } - }else { -/* -* If itrigger is not enabled in this privilege mode, -* the number of executed instructions will be discard and -* the count field in itrigger will not change. -*/ -timer_mod (env->itrigger_timer[i], - current_icount + count); - } - } -} - -static void riscv_itrigger_timer_cb (void *opaque) -{ -riscv_itrigger_update_count ((CPURISCVState *)opaque); + env->itrigger_enabled =riscv_itrigger_enabled (env); }void riscv_itrigger_update_priv (CPURISCVState *env)@@ -911,18 +873,10 @@ static void itrigger_reg_write(CPURISCVState *env, target_ulong index,switch (tdata_index) {case TDATA1 : -/* set timer for icount */ new_val =itrigger_validate (env, val);if (new_val != env->tdata1[index]) { env->tdata1[index] = new_val; -if (icount_enabled ()) { - env->last_icount =icount_get_raw (); -/* set the count to timer */ -timer_mod (env->itrigger_timer[index], - env->last_icount +itrigger_get_count (env, index)); - }else { - env->itrigger_enabled =riscv_itrigger_enabled (env); - } + env->itrigger_enabled =riscv_itrigger_enabled (env); }break ;case TDATA2 :@@ -937,27 +891,10 @@ static void itrigger_reg_write(CPURISCVState *env, target_ulong index, } } -static int itrigger_get_adjust_count (CPURISCVState *env) -{ -int count =itrigger_get_count (env, env->trigger_cur), executed; -if ((count !=0 ) &&check_itrigger_priv (env, env->trigger_cur)) { - executed =icount_get_raw () - env->last_icount; - count += executed; - } -return count; -} -target_ulong tdata_csr_read (CPURISCVState *env,int tdata_index) { -int trigger_type;switch (tdata_index) {case TDATA1 : - trigger_type =extract_trigger_type (env, - env->tdata1[env->trigger_cur]); -if ((trigger_type ==TRIGGER_TYPE_INST_CNT ) &&icount_enabled ()) { -return deposit64 (env->tdata1[env->trigger_cur],10 ,14 , -itrigger_get_adjust_count (env)); - }return env->tdata1[env->trigger_cur];case TDATA2 :return env->tdata2[env->trigger_cur];@@ -995,6 +932,8 @@ void tdata_csr_write(CPURISCVState *env, int tdata_index, target_ulong val) trigger_type);break ;case TRIGGER_TYPE_NO_EXIST : +no_trigger_reg_write (env, env->trigger_cur, tdata_index, val); +break ;case TRIGGER_TYPE_UNAVAIL :qemu_log_mask (LOG_GUEST_ERROR ,"trigger type: %d does not exit\n" , trigger_type);@@ -1147,8 +1086,6 @@ bool riscv_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)void riscv_trigger_realize (CPURISCVState *env) { -int i; -if (env->num_triggers >RV_MAX_TRIGGERS ) { error_report( "Invalid configuration: 'num-triggers' must be less than %u",@@ -1161,12 +1098,6 @@ void riscv_trigger_realize(CPURISCVState *env) env->tdata3 =g_new0 (uint64_t, env->num_triggers); env->cpu_breakpoint = g_new0(struct CPUBreakpoint *, env->num_triggers); env->cpu_watchpoint = g_new0(struct CPUWatchpoint *, env->num_triggers); - env->itrigger_timer = g_new0(QEMUTimer *, env->num_triggers); - -for (i =0 ; i < env->num_triggers; i++) { - env->itrigger_timer[i] =timer_new_ns (QEMU_CLOCK_VIRTUAL , - riscv_itrigger_timer_cb, env); - } }void riscv_trigger_unrealize (CPURISCVState *env)@@ -1177,11 +1108,6 @@ void riscv_trigger_unrealize(CPURISCVState *env)g_free (env->cpu_breakpoint);g_free (env->cpu_watchpoint); - -for (int i =0 ; i < env->num_triggers; i++) { -timer_free (env->itrigger_timer[i]); - } -g_free (env->itrigger_timer); }void riscv_trigger_reset_hold (CPURISCVState *env)@@ -1209,7 +1135,6 @@ void riscv_trigger_reset_hold(CPURISCVState *env) env->tdata3[i] =0 ; env->cpu_breakpoint[i] =NULL ; env->cpu_watchpoint[i] =NULL ; -timer_del (env->itrigger_timer[i]); } env->mcontext =0 ;target/riscv/tcg/tcg-cpu.c +5
-2@@ -181,8 +181,11 @@ static TCGTBCPUState riscv_get_tb_cpu_state(CPUState *cs) ? EXT_STATUS_DIRTY : EXT_STATUS_DISABLED; } -if ((cpu->cfg.debug || cpu->cfg.ext_sdtrig) - && !icount_enabled ()) { +/* +* Publish itrigger state even when QEMU icount is enabled. The helper +* runs from translated code after committing the guest successor PC. +*/ +if (cpu->cfg.debug || cpu->cfg.ext_sdtrig) { flags =FIELD_DP32 (flags,TB_FLAGS ,ITRIGGER , env->itrigger_enabled); }#endif