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 
> Reviewed-by: Frank Chang 
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_IPSR0x0054
>  #define RISCV_IOMMU_IPSR_CIPBIT(0)
>  #define RISCV_IOMMU_IPSR_FIPBIT(1)
> +#define RISCV_IOMMU_IPSR_PMIP   BIT(2)
>  #define RISCV_IOMMU_IPSR_PIPBIT(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 

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
> 

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 
Reviewed-by: Frank Chang 
---
  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_IPSR0x0054
  #define RISCV_IOMMU_IPSR_CIPBIT(0)
  #define RISCV_IOMMU_IPSR_FIPBIT(1)
+#define RISCV_IOMMU_IPSR_PMIP   BIT(2)
  #define RISCV_IOMMU_IPSR_PIPBIT(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 

Thanks,
Nutty

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