target/riscv: Fix mcycle/minstret increment behavior

[PATCH] target/riscv: Fix mcycle/minstret increment behavior

Posted by Xu Lu 2 years, 1 month ago

The mcycle/minstret counter's stop flag is mistakenly updated on a copy
on stack. Thus the counter increments even when the CY/IR bit in the
mcountinhibit register is set. This commit corrects its behavior.

Fixes: 3780e33732f88 (target/riscv: Support mcycle/minstret write operation)
Signed-off-by: Xu Lu <luxu.kernel@bytedance.com>
---
 target/riscv/csr.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index fde7ce1a5336..c50a33397c51 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -907,11 +907,11 @@ static int write_mhpmcounterh(CPURISCVState *env, int csrno, target_ulong val)
 static RISCVException riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *val,
                                          bool upper_half, uint32_t ctr_idx)
 {
-    PMUCTRState counter = env->pmu_ctrs[ctr_idx];
-    target_ulong ctr_prev = upper_half ? counter.mhpmcounterh_prev :
-                                         counter.mhpmcounter_prev;
-    target_ulong ctr_val = upper_half ? counter.mhpmcounterh_val :
-                                        counter.mhpmcounter_val;
+    PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
+    target_ulong ctr_prev = upper_half ? counter->mhpmcounterh_prev :
+                                         counter->mhpmcounter_prev;
+    target_ulong ctr_val = upper_half ? counter->mhpmcounterh_val :
+                                        counter->mhpmcounter_val;
 
     if (get_field(env->mcountinhibit, BIT(ctr_idx))) {
         /*
@@ -919,12 +919,12 @@ static RISCVException riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *val,
          * stop the icount counting. Just return the counter value written by
          * the supervisor to indicate that counter was not incremented.
          */
-        if (!counter.started) {
+        if (!counter->started) {
             *val = ctr_val;
             return RISCV_EXCP_NONE;
         } else {
             /* Mark that the counter has been stopped */
-            counter.started = false;
+            counter->started = false;
         }
     }
 
-- 
2.20.1

Re: [PATCH] target/riscv: Fix mcycle/minstret increment behavior

Posted by Alistair Francis 2 years, 1 month ago

On Tue, Dec 26, 2023 at 3:29 PM Xu Lu <luxu.kernel@bytedance.com> wrote:
>
> The mcycle/minstret counter's stop flag is mistakenly updated on a copy
> on stack. Thus the counter increments even when the CY/IR bit in the
> mcountinhibit register is set. This commit corrects its behavior.
>
> Fixes: 3780e33732f88 (target/riscv: Support mcycle/minstret write operation)
> Signed-off-by: Xu Lu <luxu.kernel@bytedance.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/csr.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index fde7ce1a5336..c50a33397c51 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -907,11 +907,11 @@ static int write_mhpmcounterh(CPURISCVState *env, int csrno, target_ulong val)
>  static RISCVException riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *val,
>                                           bool upper_half, uint32_t ctr_idx)
>  {
> -    PMUCTRState counter = env->pmu_ctrs[ctr_idx];
> -    target_ulong ctr_prev = upper_half ? counter.mhpmcounterh_prev :
> -                                         counter.mhpmcounter_prev;
> -    target_ulong ctr_val = upper_half ? counter.mhpmcounterh_val :
> -                                        counter.mhpmcounter_val;
> +    PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
> +    target_ulong ctr_prev = upper_half ? counter->mhpmcounterh_prev :
> +                                         counter->mhpmcounter_prev;
> +    target_ulong ctr_val = upper_half ? counter->mhpmcounterh_val :
> +                                        counter->mhpmcounter_val;
>
>      if (get_field(env->mcountinhibit, BIT(ctr_idx))) {
>          /*
> @@ -919,12 +919,12 @@ static RISCVException riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *val,
>           * stop the icount counting. Just return the counter value written by
>           * the supervisor to indicate that counter was not incremented.
>           */
> -        if (!counter.started) {
> +        if (!counter->started) {
>              *val = ctr_val;
>              return RISCV_EXCP_NONE;
>          } else {
>              /* Mark that the counter has been stopped */
> -            counter.started = false;
> +            counter->started = false;
>          }
>      }
>
> --
> 2.20.1
>
>

Re: [PATCH] target/riscv: Fix mcycle/minstret increment behavior

Posted by Alistair Francis 2 years, 1 month ago

On Tue, Dec 26, 2023 at 3:29 PM Xu Lu <luxu.kernel@bytedance.com> wrote:
>
> The mcycle/minstret counter's stop flag is mistakenly updated on a copy
> on stack. Thus the counter increments even when the CY/IR bit in the
> mcountinhibit register is set. This commit corrects its behavior.
>
> Fixes: 3780e33732f88 (target/riscv: Support mcycle/minstret write operation)
> Signed-off-by: Xu Lu <luxu.kernel@bytedance.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/csr.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index fde7ce1a5336..c50a33397c51 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -907,11 +907,11 @@ static int write_mhpmcounterh(CPURISCVState *env, int csrno, target_ulong val)
>  static RISCVException riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *val,
>                                           bool upper_half, uint32_t ctr_idx)
>  {
> -    PMUCTRState counter = env->pmu_ctrs[ctr_idx];
> -    target_ulong ctr_prev = upper_half ? counter.mhpmcounterh_prev :
> -                                         counter.mhpmcounter_prev;
> -    target_ulong ctr_val = upper_half ? counter.mhpmcounterh_val :
> -                                        counter.mhpmcounter_val;
> +    PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
> +    target_ulong ctr_prev = upper_half ? counter->mhpmcounterh_prev :
> +                                         counter->mhpmcounter_prev;
> +    target_ulong ctr_val = upper_half ? counter->mhpmcounterh_val :
> +                                        counter->mhpmcounter_val;
>
>      if (get_field(env->mcountinhibit, BIT(ctr_idx))) {
>          /*
> @@ -919,12 +919,12 @@ static RISCVException riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *val,
>           * stop the icount counting. Just return the counter value written by
>           * the supervisor to indicate that counter was not incremented.
>           */
> -        if (!counter.started) {
> +        if (!counter->started) {
>              *val = ctr_val;
>              return RISCV_EXCP_NONE;
>          } else {
>              /* Mark that the counter has been stopped */
> -            counter.started = false;
> +            counter->started = false;
>          }
>      }
>
> --
> 2.20.1
>
>

Re: [PATCH] target/riscv: Fix mcycle/minstret increment behavior

Posted by Daniel Henrique Barboza 2 years, 1 month ago

Michael,

This is a good candidate for qemu-trivial. Already acked.


Thanks,

Daniel

On 12/26/23 01:05, Xu Lu wrote:
> The mcycle/minstret counter's stop flag is mistakenly updated on a copy
> on stack. Thus the counter increments even when the CY/IR bit in the
> mcountinhibit register is set. This commit corrects its behavior.
> 
> Fixes: 3780e33732f88 (target/riscv: Support mcycle/minstret write operation)
> Signed-off-by: Xu Lu <luxu.kernel@bytedance.com>
> ---
>   target/riscv/csr.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index fde7ce1a5336..c50a33397c51 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -907,11 +907,11 @@ static int write_mhpmcounterh(CPURISCVState *env, int csrno, target_ulong val)
>   static RISCVException riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *val,
>                                            bool upper_half, uint32_t ctr_idx)
>   {
> -    PMUCTRState counter = env->pmu_ctrs[ctr_idx];
> -    target_ulong ctr_prev = upper_half ? counter.mhpmcounterh_prev :
> -                                         counter.mhpmcounter_prev;
> -    target_ulong ctr_val = upper_half ? counter.mhpmcounterh_val :
> -                                        counter.mhpmcounter_val;
> +    PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
> +    target_ulong ctr_prev = upper_half ? counter->mhpmcounterh_prev :
> +                                         counter->mhpmcounter_prev;
> +    target_ulong ctr_val = upper_half ? counter->mhpmcounterh_val :
> +                                        counter->mhpmcounter_val;
>   
>       if (get_field(env->mcountinhibit, BIT(ctr_idx))) {
>           /*
> @@ -919,12 +919,12 @@ static RISCVException riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *val,
>            * stop the icount counting. Just return the counter value written by
>            * the supervisor to indicate that counter was not incremented.
>            */
> -        if (!counter.started) {
> +        if (!counter->started) {
>               *val = ctr_val;
>               return RISCV_EXCP_NONE;
>           } else {
>               /* Mark that the counter has been stopped */
> -            counter.started = false;
> +            counter->started = false;
>           }
>       }
>

Re: [PATCH] target/riscv: Fix mcycle/minstret increment behavior

Posted by Michael Tokarev 2 years, 1 month ago

26.12.2023 12:21, Daniel Henrique Barboza:
> Michael,
> 
> This is a good candidate for qemu-trivial. Already acked.

Yeah, I've noticed it it earlier today and already copied to the
qemu-stable-toapply folder :)

Thanks!

/mjt

Re: [PATCH] target/riscv: Fix mcycle/minstret increment behavior

Posted by Daniel Henrique Barboza 2 years, 1 month ago


On 12/26/23 01:05, Xu Lu wrote:
> The mcycle/minstret counter's stop flag is mistakenly updated on a copy
> on stack. Thus the counter increments even when the CY/IR bit in the
> mcountinhibit register is set. This commit corrects its behavior.
> 

Good catch.

> Fixes: 3780e33732f88 (target/riscv: Support mcycle/minstret write operation)
> Signed-off-by: Xu Lu <luxu.kernel@bytedance.com>
> ---

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

>   target/riscv/csr.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index fde7ce1a5336..c50a33397c51 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -907,11 +907,11 @@ static int write_mhpmcounterh(CPURISCVState *env, int csrno, target_ulong val)
>   static RISCVException riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *val,
>                                            bool upper_half, uint32_t ctr_idx)
>   {
> -    PMUCTRState counter = env->pmu_ctrs[ctr_idx];
> -    target_ulong ctr_prev = upper_half ? counter.mhpmcounterh_prev :
> -                                         counter.mhpmcounter_prev;
> -    target_ulong ctr_val = upper_half ? counter.mhpmcounterh_val :
> -                                        counter.mhpmcounter_val;
> +    PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
> +    target_ulong ctr_prev = upper_half ? counter->mhpmcounterh_prev :
> +                                         counter->mhpmcounter_prev;
> +    target_ulong ctr_val = upper_half ? counter->mhpmcounterh_val :
> +                                        counter->mhpmcounter_val;
>   
>       if (get_field(env->mcountinhibit, BIT(ctr_idx))) {
>           /*
> @@ -919,12 +919,12 @@ static RISCVException riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *val,
>            * stop the icount counting. Just return the counter value written by
>            * the supervisor to indicate that counter was not incremented.
>            */
> -        if (!counter.started) {
> +        if (!counter->started) {
>               *val = ctr_val;
>               return RISCV_EXCP_NONE;
>           } else {
>               /* Mark that the counter has been stopped */
> -            counter.started = false;
> +            counter->started = false;
>           }
>       }
>