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

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
> 

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