[PATCH 0/2] hw/riscv/riscv-iommu: Bug fixes and IPSR.PMIP support

[PATCH 0/2] hw/riscv/riscv-iommu: Bug fixes and IPSR.PMIP support

[Jay Chang]

This series contains two fixes for the RISC-V IOMMU implementation:

1. Fix a bug in the HPM (Hardware Performance Monitor) timer setup where
   irq_overflow_left was not properly reset, causing stale values from
   previous timer setups to affect new timer behavior.

2. Add proper RW1C (Read/Write 1 to Clear) support for the IPSR.PMIP
   (Performance Monitor Interrupt Pending) bit, which was missing from
   the IPSR register implementation.

Jay Chang (2):
  hw/riscv/riscv-iommu-hpm: Fix irq_overflow_left residual value bug
  hw/riscv/riscv-iommu: Add IPSR.PMIP RW1C support

 hw/riscv/riscv-iommu-bits.h | 1 +
 hw/riscv/riscv-iommu-hpm.c  | 1 +
 hw/riscv/riscv-iommu.c      | 4 ++++
 3 files changed, 6 insertions(+)

-- 
2.48.1

[PATCH 1/2] hw/riscv/riscv-iommu-hpm: Fix irq_overflow_left residual value bug

[Jay Chang]

Reset irq_overflow_left to 0 before setting up a new timer. Without
this fix, a stale irq_overflow_left value from a previous timer setup
could cause incorrect timer behavior.

Signed-off-by: Jay Chang <jay.chang@sifive.com>
Reviewed-by: Frank Chang <frank.chang@sifive.com>
---
 hw/riscv/riscv-iommu-hpm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/riscv/riscv-iommu-hpm.c b/hw/riscv/riscv-iommu-hpm.c
index c5034bff79..e8d284ac8b 100644
--- a/hw/riscv/riscv-iommu-hpm.c
+++ b/hw/riscv/riscv-iommu-hpm.c
@@ -228,6 +228,7 @@ static void hpm_setup_timer(RISCVIOMMUState *s, uint64_t value)
     }
 
     overflow_at = (uint64_t)qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + overflow_ns;
+    s->irq_overflow_left = 0;
 
     if (overflow_at > INT64_MAX) {
         s->irq_overflow_left = overflow_at - INT64_MAX;
-- 
2.48.1

Re: [PATCH 1/2] hw/riscv/riscv-iommu-hpm: Fix irq_overflow_left residual value bug

[Chao Liu]

On Wed, Mar 04, 2026 at 12:09:58PM +0800, Jay Chang wrote:
> Reset irq_overflow_left to 0 before setting up a new timer. Without
> this fix, a stale irq_overflow_left value from a previous timer setup
> could cause incorrect timer behavior.
> 
> Signed-off-by: Jay Chang <jay.chang@sifive.com>
> Reviewed-by: Frank Chang <frank.chang@sifive.com>
> ---
>  hw/riscv/riscv-iommu-hpm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/riscv/riscv-iommu-hpm.c b/hw/riscv/riscv-iommu-hpm.c
> index c5034bff79..e8d284ac8b 100644
> --- a/hw/riscv/riscv-iommu-hpm.c
> +++ b/hw/riscv/riscv-iommu-hpm.c
> @@ -228,6 +228,7 @@ static void hpm_setup_timer(RISCVIOMMUState *s, uint64_t value)
>      }
>  
>      overflow_at = (uint64_t)qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + overflow_ns;
> +    s->irq_overflow_left = 0;
>  
Looks good. Without this reset, if a previous call set
irq_overflow_left (overflow_at > INT64_MAX) and the current
call does NOT overflow, the stale value persists and the
two-phase timer fires incorrectly.

Reviewed-by: Chao Liu <chao.liu.zevorn@gmail.com>

Best regards,
Chao Liu
>      if (overflow_at > INT64_MAX) {
>          s->irq_overflow_left = overflow_at - INT64_MAX;
> -- 
> 2.48.1
> 

[PATCH 2/2] hw/riscv/riscv-iommu: Add IPSR.PMIP RW1C support

[Jay Chang]

Signed-off-by: Jay Chang <jay.chang@sifive.com>
Reviewed-by: Frank Chang <frank.chang@sifive.com>
---
 hw/riscv/riscv-iommu-bits.h | 1 +
 hw/riscv/riscv-iommu.c      | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/hw/riscv/riscv-iommu-bits.h b/hw/riscv/riscv-iommu-bits.h
index 47fe01bee5..a938fd3eb4 100644
--- a/hw/riscv/riscv-iommu-bits.h
+++ b/hw/riscv/riscv-iommu-bits.h
@@ -189,6 +189,7 @@ enum riscv_iommu_ddtp_modes {
 #define RISCV_IOMMU_REG_IPSR            0x0054
 #define RISCV_IOMMU_IPSR_CIP            BIT(0)
 #define RISCV_IOMMU_IPSR_FIP            BIT(1)
+#define RISCV_IOMMU_IPSR_PMIP           BIT(2)
 #define RISCV_IOMMU_IPSR_PIP            BIT(3)
 
 enum {
diff --git a/hw/riscv/riscv-iommu.c b/hw/riscv/riscv-iommu.c
index 98345b1280..610cdebac2 100644
--- a/hw/riscv/riscv-iommu.c
+++ b/hw/riscv/riscv-iommu.c
@@ -2153,6 +2153,10 @@ static void riscv_iommu_update_ipsr(RISCVIOMMUState *s, uint64_t data)
         ipsr_clr |= RISCV_IOMMU_IPSR_FIP;
     }
 
+    if (!(data & RISCV_IOMMU_IPSR_PMIP)) {
+        ipsr_clr |= RISCV_IOMMU_IPSR_PMIP;
+    }
+
     if (data & RISCV_IOMMU_IPSR_PIP) {
         pqcsr = riscv_iommu_reg_get32(s, RISCV_IOMMU_REG_PQCSR);
 
-- 
2.48.1

Re: [PATCH 2/2] hw/riscv/riscv-iommu: Add IPSR.PMIP RW1C support

[Nutty.Liu]

On 3/4/2026 12:09 PM, Jay Chang wrote:
> Signed-off-by: Jay Chang <jay.chang@sifive.com>
> Reviewed-by: Frank Chang <frank.chang@sifive.com>
> ---
>   hw/riscv/riscv-iommu-bits.h | 1 +
>   hw/riscv/riscv-iommu.c      | 4 ++++
>   2 files changed, 5 insertions(+)
>
> diff --git a/hw/riscv/riscv-iommu-bits.h b/hw/riscv/riscv-iommu-bits.h
> index 47fe01bee5..a938fd3eb4 100644
> --- a/hw/riscv/riscv-iommu-bits.h
> +++ b/hw/riscv/riscv-iommu-bits.h
> @@ -189,6 +189,7 @@ enum riscv_iommu_ddtp_modes {
>   #define RISCV_IOMMU_REG_IPSR            0x0054
>   #define RISCV_IOMMU_IPSR_CIP            BIT(0)
>   #define RISCV_IOMMU_IPSR_FIP            BIT(1)
> +#define RISCV_IOMMU_IPSR_PMIP           BIT(2)
>   #define RISCV_IOMMU_IPSR_PIP            BIT(3)
>   
>   enum {
> diff --git a/hw/riscv/riscv-iommu.c b/hw/riscv/riscv-iommu.c
> index 98345b1280..610cdebac2 100644
> --- a/hw/riscv/riscv-iommu.c
> +++ b/hw/riscv/riscv-iommu.c
> @@ -2153,6 +2153,10 @@ static void riscv_iommu_update_ipsr(RISCVIOMMUState *s, uint64_t data)
>           ipsr_clr |= RISCV_IOMMU_IPSR_FIP;
>       }
>   
> +    if (!(data & RISCV_IOMMU_IPSR_PMIP)) {
> +        ipsr_clr |= RISCV_IOMMU_IPSR_PMIP;
> +    }
> +
According to the following description of 'pmip' in the specification:
"The performance-monitoring-interrupt-pending is set to 1 when OF bit 
iniohpmcycles or in any of the iohpmctr1-31 registers transitions
from 0 to 1."

Would it be better to handle like the following ?

     if (data & RISCV_IOMMU_IPSR_PMIP) {
         ipsr_set ...
     } else {
          ipsr_clr ...
     }

Otherwise,
Reviewed-by: Nutty Liu <nutty.liu@hotmail.com>

Thanks,
Nutty
>       if (data & RISCV_IOMMU_IPSR_PIP) {
>           pqcsr = riscv_iommu_reg_get32(s, RISCV_IOMMU_REG_PQCSR);
>   

Re: [PATCH 2/2] hw/riscv/riscv-iommu: Add IPSR.PMIP RW1C support

[Chao Liu]

On Wed, Mar 04, 2026 at 12:09:59PM +0800, Jay Chang wrote:
> Signed-off-by: Jay Chang <jay.chang@sifive.com>
> Reviewed-by: Frank Chang <frank.chang@sifive.com>
The commit message has no body text. Please add a short
description of what PMIP is and why RW1C support is needed,
similar to patch 1's commit message style.

You can copy the second point of the cover letter too:

2. Add proper RW1C (Read/Write 1 to Clear) support for the IPSR.PMIP
   (Performance Monitor Interrupt Pending) bit, which was missing from
   the IPSR register implementation.

> ---
>  hw/riscv/riscv-iommu-bits.h | 1 +
>  hw/riscv/riscv-iommu.c      | 4 ++++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/hw/riscv/riscv-iommu-bits.h b/hw/riscv/riscv-iommu-bits.h
> index 47fe01bee5..a938fd3eb4 100644
> --- a/hw/riscv/riscv-iommu-bits.h
> +++ b/hw/riscv/riscv-iommu-bits.h
> @@ -189,6 +189,7 @@ enum riscv_iommu_ddtp_modes {
>  #define RISCV_IOMMU_REG_IPSR            0x0054
>  #define RISCV_IOMMU_IPSR_CIP            BIT(0)
>  #define RISCV_IOMMU_IPSR_FIP            BIT(1)
> +#define RISCV_IOMMU_IPSR_PMIP           BIT(2)
>  #define RISCV_IOMMU_IPSR_PIP            BIT(3)
>  
Good, BIT(2) matches the IOMMU spec for PMIP.

>  enum {
> diff --git a/hw/riscv/riscv-iommu.c b/hw/riscv/riscv-iommu.c
> index 98345b1280..610cdebac2 100644
> --- a/hw/riscv/riscv-iommu.c
> +++ b/hw/riscv/riscv-iommu.c
> @@ -2153,6 +2153,10 @@ static void riscv_iommu_update_ipsr(RISCVIOMMUState *s, uint64_t data)
>          ipsr_clr |= RISCV_IOMMU_IPSR_FIP;
>      }
>  
> +    if (!(data & RISCV_IOMMU_IPSR_PMIP)) {
This works correctly in practice because `data` (the val
computed by riscv_iommu_write_reg_val) is always 0 for
IPSR writes — regs_wc is ~0 and regs_ro is 0, so the
formula yields data & ~data = 0. The condition
`!(0 & PMIP)` is always true, so PMIP is always cleared.

That said, since `data` is always 0 here, you could
simplify this to just:

    ipsr_clr |= RISCV_IOMMU_IPSR_PMIP;

without the conditional. This would also be more
consistent with the effective behavior of the CIP/FIP/PIP
branches (which always take the "else" / clear path for
the same reason).

Not a blocker — the current code is correct. Just a
suggestion for clarity.


Reviewed-by: Chao Liu <chao.liu.zevorn@gmail.com>

Best regards,
Chao Liu
> +        ipsr_clr |= RISCV_IOMMU_IPSR_PMIP;
> +    }
> +
>      if (data & RISCV_IOMMU_IPSR_PIP) {
>          pqcsr = riscv_iommu_reg_get32(s, RISCV_IOMMU_REG_PQCSR);
>  
> -- 
> 2.48.1
>