Avoid calling riscv_itrigger_enabled() when calculate the tbflags.
As the itrigger enable status can only be changed when write
tdata1, migration load or itrigger fire, update env->itrigger_enabled
at these places.
Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
---
target/riscv/cpu.h | 1 +
target/riscv/cpu_helper.c | 3 +--
target/riscv/debug.c | 3 +++
target/riscv/machine.c | 15 +++++++++++++++
4 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 13ca0f20ae..44499df9da 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -331,6 +331,7 @@ struct CPUArchState {
struct CPUWatchpoint *cpu_watchpoint[RV_MAX_TRIGGERS];
QEMUTimer *itrigger_timer[RV_MAX_TRIGGERS];
int64_t last_icount;
+ bool itrigger_enabled;
/* machine specific rdtime callback */
uint64_t (*rdtime_fn)(void *);
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 7d8089b218..95c766aec0 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -106,8 +106,7 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
get_field(env->mstatus_hs, MSTATUS_VS));
}
if (riscv_feature(env, RISCV_FEATURE_DEBUG) && !icount_enabled()) {
- flags = FIELD_DP32(flags, TB_FLAGS, ITRIGGER,
- riscv_itrigger_enabled(env));
+ flags = FIELD_DP32(flags, TB_FLAGS, ITRIGGER, env->itrigger_enabled);
}
#endif
diff --git a/target/riscv/debug.c b/target/riscv/debug.c
index db7745d4a3..2c0c8b18db 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -565,6 +565,7 @@ void helper_itrigger_match(CPURISCVState *env)
}
itrigger_set_count(env, i, count--);
if (!count) {
+ env->itrigger_enabled = riscv_itrigger_enabled(env);
do_trigger_action(env, i);
}
}
@@ -662,6 +663,8 @@ static void itrigger_reg_write(CPURISCVState *env, target_ulong index,
/* 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);
}
}
break;
diff --git a/target/riscv/machine.c b/target/riscv/machine.c
index c2a94a82b3..cd32a52e19 100644
--- a/target/riscv/machine.c
+++ b/target/riscv/machine.c
@@ -21,6 +21,8 @@
#include "qemu/error-report.h"
#include "sysemu/kvm.h"
#include "migration/cpu.h"
+#include "sysemu/cpu-timers.h"
+#include "debug.h"
static bool pmp_needed(void *opaque)
{
@@ -229,11 +231,24 @@ static bool debug_needed(void *opaque)
return riscv_feature(env, RISCV_FEATURE_DEBUG);
}
+static int debug_post_load(void *opaque, int version_id)
+{
+ RISCVCPU *cpu = opaque;
+ CPURISCVState *env = &cpu->env;
+
+ if (icount_enabled()) {
+ env->itrigger_enabled = riscv_itrigger_enabled(env);
+ }
+
+ return 0;
+}
+
static const VMStateDescription vmstate_debug = {
.name = "cpu/debug",
.version_id = 2,
.minimum_version_id = 2,
.needed = debug_needed,
+ .post_load = debug_post_load,
.fields = (VMStateField[]) {
VMSTATE_UINTTL(env.trigger_cur, RISCVCPU),
VMSTATE_UINTTL_ARRAY(env.tdata1, RISCVCPU, RV_MAX_TRIGGERS),
--
2.17.1On Thu, Oct 13, 2022 at 4:51 PM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote:
>
> Avoid calling riscv_itrigger_enabled() when calculate the tbflags.
> As the itrigger enable status can only be changed when write
> tdata1, migration load or itrigger fire, update env->itrigger_enabled
> at these places.
>
> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
> ---
> target/riscv/cpu.h | 1 +
> target/riscv/cpu_helper.c | 3 +--
> target/riscv/debug.c | 3 +++
> target/riscv/machine.c | 15 +++++++++++++++
> 4 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 13ca0f20ae..44499df9da 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -331,6 +331,7 @@ struct CPUArchState {
> struct CPUWatchpoint *cpu_watchpoint[RV_MAX_TRIGGERS];
> QEMUTimer *itrigger_timer[RV_MAX_TRIGGERS];
> int64_t last_icount;
> + bool itrigger_enabled;
>
> /* machine specific rdtime callback */
> uint64_t (*rdtime_fn)(void *);
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 7d8089b218..95c766aec0 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -106,8 +106,7 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
> get_field(env->mstatus_hs, MSTATUS_VS));
> }
> if (riscv_feature(env, RISCV_FEATURE_DEBUG) && !icount_enabled()) {
> - flags = FIELD_DP32(flags, TB_FLAGS, ITRIGGER,
> - riscv_itrigger_enabled(env));
> + flags = FIELD_DP32(flags, TB_FLAGS, ITRIGGER, env->itrigger_enabled);
> }
> #endif
>
> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> index db7745d4a3..2c0c8b18db 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -565,6 +565,7 @@ void helper_itrigger_match(CPURISCVState *env)
> }
> itrigger_set_count(env, i, count--);
> if (!count) {
> + env->itrigger_enabled = riscv_itrigger_enabled(env);
> do_trigger_action(env, i);
> }
> }
> @@ -662,6 +663,8 @@ static void itrigger_reg_write(CPURISCVState *env, target_ulong index,
> /* 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);
> }
> }
> break;
> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> index c2a94a82b3..cd32a52e19 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -21,6 +21,8 @@
> #include "qemu/error-report.h"
> #include "sysemu/kvm.h"
> #include "migration/cpu.h"
> +#include "sysemu/cpu-timers.h"
> +#include "debug.h"
>
> static bool pmp_needed(void *opaque)
> {
> @@ -229,11 +231,24 @@ static bool debug_needed(void *opaque)
> return riscv_feature(env, RISCV_FEATURE_DEBUG);
> }
>
> +static int debug_post_load(void *opaque, int version_id)
> +{
> + RISCVCPU *cpu = opaque;
> + CPURISCVState *env = &cpu->env;
> +
> + if (icount_enabled()) {
> + env->itrigger_enabled = riscv_itrigger_enabled(env);
> + }
> +
> + return 0;
> +}
> +
> static const VMStateDescription vmstate_debug = {
> .name = "cpu/debug",
> .version_id = 2,
> .minimum_version_id = 2,
The versions here should be bumped
Alistair
> .needed = debug_needed,
> + .post_load = debug_post_load,
> .fields = (VMStateField[]) {
> VMSTATE_UINTTL(env.trigger_cur, RISCVCPU),
> VMSTATE_UINTTL_ARRAY(env.tdata1, RISCVCPU, RV_MAX_TRIGGERS),
> --
> 2.17.1
>
>
On 2022/11/10 6:55, Alistair Francis wrote:
> On Thu, Oct 13, 2022 at 4:51 PM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote:
>> Avoid calling riscv_itrigger_enabled() when calculate the tbflags.
>> As the itrigger enable status can only be changed when write
>> tdata1, migration load or itrigger fire, update env->itrigger_enabled
>> at these places.
>>
>> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
>> ---
>> target/riscv/cpu.h | 1 +
>> target/riscv/cpu_helper.c | 3 +--
>> target/riscv/debug.c | 3 +++
>> target/riscv/machine.c | 15 +++++++++++++++
>> 4 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> index 13ca0f20ae..44499df9da 100644
>> --- a/target/riscv/cpu.h
>> +++ b/target/riscv/cpu.h
>> @@ -331,6 +331,7 @@ struct CPUArchState {
>> struct CPUWatchpoint *cpu_watchpoint[RV_MAX_TRIGGERS];
>> QEMUTimer *itrigger_timer[RV_MAX_TRIGGERS];
>> int64_t last_icount;
>> + bool itrigger_enabled;
>>
>> /* machine specific rdtime callback */
>> uint64_t (*rdtime_fn)(void *);
>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>> index 7d8089b218..95c766aec0 100644
>> --- a/target/riscv/cpu_helper.c
>> +++ b/target/riscv/cpu_helper.c
>> @@ -106,8 +106,7 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
>> get_field(env->mstatus_hs, MSTATUS_VS));
>> }
>> if (riscv_feature(env, RISCV_FEATURE_DEBUG) && !icount_enabled()) {
>> - flags = FIELD_DP32(flags, TB_FLAGS, ITRIGGER,
>> - riscv_itrigger_enabled(env));
>> + flags = FIELD_DP32(flags, TB_FLAGS, ITRIGGER, env->itrigger_enabled);
>> }
>> #endif
>>
>> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
>> index db7745d4a3..2c0c8b18db 100644
>> --- a/target/riscv/debug.c
>> +++ b/target/riscv/debug.c
>> @@ -565,6 +565,7 @@ void helper_itrigger_match(CPURISCVState *env)
>> }
>> itrigger_set_count(env, i, count--);
>> if (!count) {
>> + env->itrigger_enabled = riscv_itrigger_enabled(env);
>> do_trigger_action(env, i);
>> }
>> }
>> @@ -662,6 +663,8 @@ static void itrigger_reg_write(CPURISCVState *env, target_ulong index,
>> /* 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);
>> }
>> }
>> break;
>> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
>> index c2a94a82b3..cd32a52e19 100644
>> --- a/target/riscv/machine.c
>> +++ b/target/riscv/machine.c
>> @@ -21,6 +21,8 @@
>> #include "qemu/error-report.h"
>> #include "sysemu/kvm.h"
>> #include "migration/cpu.h"
>> +#include "sysemu/cpu-timers.h"
>> +#include "debug.h"
>>
>> static bool pmp_needed(void *opaque)
>> {
>> @@ -229,11 +231,24 @@ static bool debug_needed(void *opaque)
>> return riscv_feature(env, RISCV_FEATURE_DEBUG);
>> }
>>
>> +static int debug_post_load(void *opaque, int version_id)
>> +{
>> + RISCVCPU *cpu = opaque;
>> + CPURISCVState *env = &cpu->env;
>> +
>> + if (icount_enabled()) {
>> + env->itrigger_enabled = riscv_itrigger_enabled(env);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static const VMStateDescription vmstate_debug = {
>> .name = "cpu/debug",
>> .version_id = 2,
>> .minimum_version_id = 2,
> The versions here should be bumped
Hi Alistair and Richard,
I am not sure if we should bump versions when just add post_load
callback without adding new fields. I once upstreamed a patch
with a similar change but not bumping version.
Could you give some principles for bumping versions?
Thanks,
Zhiwei
> Alistair
>
>> .needed = debug_needed,
>> + .post_load = debug_post_load,
>> .fields = (VMStateField[]) {
>> VMSTATE_UINTTL(env.trigger_cur, RISCVCPU),
>> VMSTATE_UINTTL_ARRAY(env.tdata1, RISCVCPU, RV_MAX_TRIGGERS),
>> --
>> 2.17.1
>>
>>
On 11/10/22 13:15, LIU Zhiwei wrote:
>>> +static int debug_post_load(void *opaque, int version_id)
>>> +{
>>> + RISCVCPU *cpu = opaque;
>>> + CPURISCVState *env = &cpu->env;
>>> +
>>> + if (icount_enabled()) {
>>> + env->itrigger_enabled = riscv_itrigger_enabled(env);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> static const VMStateDescription vmstate_debug = {
>>> .name = "cpu/debug",
>>> .version_id = 2,
>>> .minimum_version_id = 2,
>> The versions here should be bumped
>
> Hi Alistair and Richard,
>
> I am not sure if we should bump versions when just add post_load callback without adding
> new fields. I once upstreamed a patch
> with a similar change but not bumping version.
Simply adding a post_load does not require a version bump.
r~
On Thu, Nov 10, 2022 at 3:35 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 11/10/22 13:15, LIU Zhiwei wrote:
> >>> +static int debug_post_load(void *opaque, int version_id)
> >>> +{
> >>> + RISCVCPU *cpu = opaque;
> >>> + CPURISCVState *env = &cpu->env;
> >>> +
> >>> + if (icount_enabled()) {
> >>> + env->itrigger_enabled = riscv_itrigger_enabled(env);
> >>> + }
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> static const VMStateDescription vmstate_debug = {
> >>> .name = "cpu/debug",
> >>> .version_id = 2,
> >>> .minimum_version_id = 2,
> >> The versions here should be bumped
> >
> > Hi Alistair and Richard,
> >
> > I am not sure if we should bump versions when just add post_load callback without adding
> > new fields. I once upstreamed a patch
> > with a similar change but not bumping version.
>
> Simply adding a post_load does not require a version bump.
Ah, my mistake then
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
>
>
> r~
© 2016 - 2026 Red Hat, Inc.