On Wed, Mar 04, 2026 at 12:09:59PM +0800, Jay Chang wrote:
> Signed-off-by: Jay Chang <[email protected]>
> Reviewed-by: Frank Chang <[email protected]>
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 <[email protected]>
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
>
