diff mbox series

[v1,4/4] target/riscv: Add itrigger_enabled field to CPURISCVState

Message ID 20221013062946.7530-5-zhiwei_liu@linux.alibaba.com (mailing list archive)
State New, archived
Headers show
Series Support native debug icount trigger | expand

Commit Message

LIU Zhiwei Oct. 13, 2022, 6:29 a.m. UTC 
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(-)

Comments

Alistair Francis Nov. 9, 2022, 10:55 p.m. UTC | #1 
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

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
>
>
LIU Zhiwei Nov. 10, 2022, 2:15 a.m. UTC | #2 
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
>>
>>
Richard Henderson Nov. 10, 2022, 5:35 a.m. UTC | #3 
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~
Alistair Francis Nov. 11, 2022, 12:54 a.m. UTC | #4 
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~
diff mbox series

Patch

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),