[PATCH] target/riscv: fix uninitialised upper half in pmu_timer_trigger_irq()

[PATCH] target/riscv: fix uninitialised upper half in pmu_timer_trigger_irq()

[Harshit Mukhedkar]

pmu_timer_trigger_irq() reads the counter by passing
(target_ulong *)&curr_ctr_val to riscv_pmu_read_ctr(). On RV32 that
pointer is to a 32 bit type, so only half of the uint64_t gets written
and the rest is whatever was on the stack. The high word read from
riscv_pmu_read_ctr() is then ORed in on top of that, and the result is
compared against the software written counter to decide whether to raise
LCOFI. A stray bit in the garbage half can make that comparison wrong.

Read the low and high halves into their own target_ulong variables and
assemble the 64-bit value explicitly.

This was found while looking at #4724, whose reported mhpmcounterh
mismatch is fixed separately by 57b484e2d232.

Fixes: 74112400df65 ("target/riscv: More accurately model priv mode filtering.")
Cc: qemu-stable@nongnu.org
Signed-off-by: Harshit Mukhedkar <harshit007k@gmail.com>
---
 target/riscv/tcg/pmu.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/target/riscv/tcg/pmu.c b/target/riscv/tcg/pmu.c
index 1a4658319b..0165faf06b 100644
--- a/target/riscv/tcg/pmu.c
+++ b/target/riscv/tcg/pmu.c
@@ -355,7 +355,8 @@ static void pmu_timer_trigger_irq(RISCVCPU *cpu,
     CPURISCVState *env = &cpu->env;
     PMUCTRState *counter;
     int64_t irq_trigger_at;
-    uint64_t curr_ctr_val, curr_ctrh_val;
+    target_ulong ctr_lo = 0, ctr_hi = 0;
+    uint64_t curr_ctr_val;
     uint64_t ctr_val;
 
     if (evt_idx != RISCV_PMU_EVENT_HW_CPU_CYCLES &&
@@ -383,12 +384,19 @@ static void pmu_timer_trigger_irq(RISCVCPU *cpu,
         return;
     }
 
-    riscv_pmu_read_ctr(env, (target_ulong *)&curr_ctr_val, false, ctr_idx);
-    ctr_val = counter->mhpmcounter_val;
+    /*
+     * riscv_pmu_read_ctr() stores a target_ulong, so on RV32 each call fills
+     * only 32 bits. Read each half into its own target_ulong and build the
+     * 64 bit value explicitly instead of passing a cast uint64_t pointer,
+     * which would leave the upper half uninitialised.
+     */
+    riscv_pmu_read_ctr(env, &ctr_lo, false, ctr_idx);
+    curr_ctr_val = ctr_lo;
     if (riscv_cpu_mxl(env) == MXL_RV32) {
-        riscv_pmu_read_ctr(env, (target_ulong *)&curr_ctrh_val, true, ctr_idx);
-        curr_ctr_val = curr_ctr_val | (curr_ctrh_val << 32);
+        riscv_pmu_read_ctr(env, &ctr_hi, true, ctr_idx);
+        curr_ctr_val |= (uint64_t)ctr_hi << 32;
     }
+    ctr_val = counter->mhpmcounter_val;
 
     /*
      * We can not accommodate for inhibited modes when setting up timer. Check
-- 
2.56.0

Re: [PATCH] target/riscv: fix uninitialised upper half in pmu_timer_trigger_irq()

[Alistair Francis]

On Fri, 2026-10-23 at 15:05 +0530, Harshit Mukhedkar wrote:
> pmu_timer_trigger_irq() reads the counter by passing
> (target_ulong *)&curr_ctr_val to riscv_pmu_read_ctr(). On RV32 that
> pointer is to a 32 bit type, so only half of the uint64_t gets
> written
> and the rest is whatever was on the stack. The high word read from
> riscv_pmu_read_ctr() is then ORed in on top of that, and the result
> is
> compared against the software written counter to decide whether to
> raise
> LCOFI. A stray bit in the garbage half can make that comparison
> wrong.
> 
> Read the low and high halves into their own target_ulong variables
> and
> assemble the 64-bit value explicitly.
> 
> This was found while looking at #4724, whose reported mhpmcounterh
> mismatch is fixed separately by 57b484e2d232.
> 
> Fixes: 74112400df65 ("target/riscv: More accurately model priv mode
> filtering.")
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Harshit Mukhedkar <harshit007k@gmail.com>

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

Alistair

> ---
>  target/riscv/tcg/pmu.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/target/riscv/tcg/pmu.c b/target/riscv/tcg/pmu.c
> index 1a4658319b..0165faf06b 100644
> --- a/target/riscv/tcg/pmu.c
> +++ b/target/riscv/tcg/pmu.c
> @@ -355,7 +355,8 @@ static void pmu_timer_trigger_irq(RISCVCPU *cpu,
>      CPURISCVState *env = &cpu->env;
>      PMUCTRState *counter;
>      int64_t irq_trigger_at;
> -    uint64_t curr_ctr_val, curr_ctrh_val;
> +    target_ulong ctr_lo = 0, ctr_hi = 0;
> +    uint64_t curr_ctr_val;
>      uint64_t ctr_val;
>  
>      if (evt_idx != RISCV_PMU_EVENT_HW_CPU_CYCLES &&
> @@ -383,12 +384,19 @@ static void pmu_timer_trigger_irq(RISCVCPU
> *cpu,
>          return;
>      }
>  
> -    riscv_pmu_read_ctr(env, (target_ulong *)&curr_ctr_val, false,
> ctr_idx);
> -    ctr_val = counter->mhpmcounter_val;
> +    /*
> +     * riscv_pmu_read_ctr() stores a target_ulong, so on RV32 each
> call fills
> +     * only 32 bits. Read each half into its own target_ulong and
> build the
> +     * 64 bit value explicitly instead of passing a cast uint64_t
> pointer,
> +     * which would leave the upper half uninitialised.
> +     */
> +    riscv_pmu_read_ctr(env, &ctr_lo, false, ctr_idx);
> +    curr_ctr_val = ctr_lo;
>      if (riscv_cpu_mxl(env) == MXL_RV32) {
> -        riscv_pmu_read_ctr(env, (target_ulong *)&curr_ctrh_val,
> true, ctr_idx);
> -        curr_ctr_val = curr_ctr_val | (curr_ctrh_val << 32);
> +        riscv_pmu_read_ctr(env, &ctr_hi, true, ctr_idx);
> +        curr_ctr_val |= (uint64_t)ctr_hi << 32;
>      }
> +    ctr_val = counter->mhpmcounter_val;
>  
>      /*
>       * We can not accommodate for inhibited modes when setting up
> timer. Check

Re: [PATCH] target/riscv: fix uninitialised upper half in pmu_timer_trigger_irq()

[Alistair Francis]

On Fri, 2026-10-23 at 15:05 +0530, Harshit Mukhedkar wrote:
> pmu_timer_trigger_irq() reads the counter by passing
> (target_ulong *)&curr_ctr_val to riscv_pmu_read_ctr(). On RV32 that
> pointer is to a 32 bit type, so only half of the uint64_t gets
> written
> and the rest is whatever was on the stack. The high word read from
> riscv_pmu_read_ctr() is then ORed in on top of that, and the result
> is
> compared against the software written counter to decide whether to
> raise
> LCOFI. A stray bit in the garbage half can make that comparison
> wrong.
> 
> Read the low and high halves into their own target_ulong variables
> and
> assemble the 64-bit value explicitly.
> 
> This was found while looking at #4724, whose reported mhpmcounterh
> mismatch is fixed separately by 57b484e2d232.
> 
> Fixes: 74112400df65 ("target/riscv: More accurately model priv mode
> filtering.")
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Harshit Mukhedkar <harshit007k@gmail.com>

Thanks!

Applied to riscv-to-apply.next

Alistair

> ---
>  target/riscv/tcg/pmu.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/target/riscv/tcg/pmu.c b/target/riscv/tcg/pmu.c
> index 1a4658319b..0165faf06b 100644
> --- a/target/riscv/tcg/pmu.c
> +++ b/target/riscv/tcg/pmu.c
> @@ -355,7 +355,8 @@ static void pmu_timer_trigger_irq(RISCVCPU *cpu,
>      CPURISCVState *env = &cpu->env;
>      PMUCTRState *counter;
>      int64_t irq_trigger_at;
> -    uint64_t curr_ctr_val, curr_ctrh_val;
> +    target_ulong ctr_lo = 0, ctr_hi = 0;
> +    uint64_t curr_ctr_val;
>      uint64_t ctr_val;
>  
>      if (evt_idx != RISCV_PMU_EVENT_HW_CPU_CYCLES &&
> @@ -383,12 +384,19 @@ static void pmu_timer_trigger_irq(RISCVCPU
> *cpu,
>          return;
>      }
>  
> -    riscv_pmu_read_ctr(env, (target_ulong *)&curr_ctr_val, false,
> ctr_idx);
> -    ctr_val = counter->mhpmcounter_val;
> +    /*
> +     * riscv_pmu_read_ctr() stores a target_ulong, so on RV32 each
> call fills
> +     * only 32 bits. Read each half into its own target_ulong and
> build the
> +     * 64 bit value explicitly instead of passing a cast uint64_t
> pointer,
> +     * which would leave the upper half uninitialised.
> +     */
> +    riscv_pmu_read_ctr(env, &ctr_lo, false, ctr_idx);
> +    curr_ctr_val = ctr_lo;
>      if (riscv_cpu_mxl(env) == MXL_RV32) {
> -        riscv_pmu_read_ctr(env, (target_ulong *)&curr_ctrh_val,
> true, ctr_idx);
> -        curr_ctr_val = curr_ctr_val | (curr_ctrh_val << 32);
> +        riscv_pmu_read_ctr(env, &ctr_hi, true, ctr_idx);
> +        curr_ctr_val |= (uint64_t)ctr_hi << 32;
>      }
> +    ctr_val = counter->mhpmcounter_val;
>  
>      /*
>       * We can not accommodate for inhibited modes when setting up
> timer. Check

Re: [PATCH] target/riscv: fix uninitialised upper half in pmu_timer_trigger_irq()

[Michael Tokarev]

On 10/23/26 12:35, Harshit Mukhedkar wrote:
> pmu_timer_trigger_irq() reads the counter by passing
> (target_ulong *)&curr_ctr_val to riscv_pmu_read_ctr(). On RV32 that
> pointer is to a 32 bit type, so only half of the uint64_t gets written
> and the rest is whatever was on the stack. The high word read from
> riscv_pmu_read_ctr() is then ORed in on top of that, and the result is
> compared against the software written counter to decide whether to raise
> LCOFI. A stray bit in the garbage half can make that comparison wrong.
> 
> Read the low and high halves into their own target_ulong variables and
> assemble the 64-bit value explicitly.
> 
> This was found while looking at #4724, whose reported mhpmcounterh
> mismatch is fixed separately by 57b484e2d232.
> 
> Fixes: 74112400df65 ("target/riscv: More accurately model priv mode filtering.")
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Harshit Mukhedkar <harshit007k@gmail.com>

This change has been marked for-stable.  However, just like a previous
change for RV32, I'm not applying it to 10.0.x branch, because it lacks
a few other commits in this area, and because RV32 is a very niche
platform to support in older stable releases.  Only 11.1.x gets it.

FWIW.

Thanks,

/mjt