Re: [PATCH] hw/riscv/riscv-iommu: preserve requested perm in spa_fetch()

On Fri, 2026-07-17 at 16:45 +0200, Andrew Jones wrote:
> b18e3f0e2d0f fixed spa_fetch() faults whose TTYP used the leaf PTE
> permission instead of the original request permission.  However, it
> kept
> that request-narrowed value in iotlb->perm after a successful walk,
> and
> riscv_iommu_translate() caches iotlb->perm for later accesses to the
> same
> IOVA.
> 
> That means a write to an RW mapping can cache the entry as write-
> only.
> A later read then hits the cache and faults even though the mapping
> allows
> it, which showed up in NVMe testing as bogus completions and
> controller
> timeouts.
> 
> Keep the requested permission in a separate req_perm and use it for
> all
> permission checks and fault-type decisions.  Accumulate the leaf
> permissions separately and copy them to iotlb->perm only after the
> full
> walk succeeds, so cached entries describe the mapping rather than the
> current request.  Since faults leave iotlb->perm as the original
> request,
> the S-stage and G-stage TTYP fixes remain intact.
> 
> Fixes: b18e3f0e2d0f ("hw/riscv/riscv-iommu.c: fix fault type for
> spa_fetch() faults")
> Signed-off-by: Andrew Jones <[email protected]>

Thanks!

Applied to riscv-to-apply.next

Alistair

> ---
>  hw/riscv/riscv-iommu.c | 50 ++++++++++++++++++++++++----------------
> --
>  1 file changed, 29 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/riscv/riscv-iommu.c b/hw/riscv/riscv-iommu.c
> index ed9fb09f8bc7..323a041b4a55 100644
> --- a/hw/riscv/riscv-iommu.c
> +++ b/hw/riscv/riscv-iommu.c
> @@ -281,7 +281,7 @@ static hwaddr riscv_iommu_napot_page_mask(hwaddr
> ppn, hwaddr addr, hwaddr *out)
>  static int riscv_iommu_spa_fetch(RISCVIOMMUState *s,
> RISCVIOMMUContext *ctx,
>      IOMMUTLBEntry *iotlb)
>  {
> -    IOMMUAccessFlags pte_perm;
> +    IOMMUAccessFlags trans_perm = IOMMU_NONE;
>      dma_addr_t addr, base;
>      uint64_t satp, gatp, pte;
>      bool en_s, en_g;
> @@ -298,6 +298,14 @@ static int riscv_iommu_spa_fetch(RISCVIOMMUState
> *s, RISCVIOMMUContext *ctx,
>      } pass;
>      MemTxResult ret;
>      bool pv = !!ctx->process_id;
> +    /*
> +     * Keep the request permission separate from iotlb->perm.  G-
> stage
> +     * walks translate S-stage PTE addresses before the real leaf is
> +     * reached, but permission checks and fault types must still use
> the
> +     * original request.  A successful walk leaves iotlb->perm with
> the
> +     * effective leaf permission for the translation cache.
> +     */
> +    const IOMMUAccessFlags req_perm = iotlb->perm;
>  
>      satp = get_field(ctx->satp, RISCV_IOMMU_ATP_MODE_FIELD);
>      gatp = get_field(ctx->gatp, RISCV_IOMMU_ATP_MODE_FIELD);
> @@ -316,7 +324,7 @@ static int riscv_iommu_spa_fetch(RISCVIOMMUState
> *s, RISCVIOMMUContext *ctx,
>       * means we can't do an early MSI check unless we have
>       * strictly !en_s.
>       */
> -    if (!en_s && (iotlb->perm & IOMMU_WO) &&
> +    if (!en_s && (req_perm & IOMMU_WO) &&
>          riscv_iommu_msi_check(s, ctx, iotlb->iova)) {
>          iotlb->target_as = &s->trap_as;
>          iotlb->translated_addr = iotlb->iova;
> @@ -434,13 +442,13 @@ static int
> riscv_iommu_spa_fetch(RISCVIOMMUState *s, RISCVIOMMUContext *ctx,
>                  masked_msbs = (addr >> (va_len - 1)) & mask;
>  
>                  if (masked_msbs != 0 && masked_msbs != mask) {
> -                    return (iotlb->perm & IOMMU_WO) ?
> +                    return (req_perm & IOMMU_WO) ?
>                                  RISCV_IOMMU_FQ_CAUSE_WR_FAULT_S :
>                                  RISCV_IOMMU_FQ_CAUSE_RD_FAULT_S;
>                  }
>              } else {
>                  if ((addr & va_mask) != addr) {
> -                    return (iotlb->perm & IOMMU_WO) ?
> +                    return (req_perm & IOMMU_WO) ?
>                                  RISCV_IOMMU_FQ_CAUSE_WR_FAULT_VS :
>                                  RISCV_IOMMU_FQ_CAUSE_RD_FAULT_VS;
>                  }
> @@ -465,8 +473,8 @@ static int riscv_iommu_spa_fetch(RISCVIOMMUState
> *s, RISCVIOMMUContext *ctx,
>                               MEMTXATTRS_UNSPECIFIED);
>          }
>          if (ret != MEMTX_OK) {
> -            return (iotlb->perm & IOMMU_WO) ?
> RISCV_IOMMU_FQ_CAUSE_WR_FAULT
> -                                            :
> RISCV_IOMMU_FQ_CAUSE_RD_FAULT;
> +            return (req_perm & IOMMU_WO) ?
> RISCV_IOMMU_FQ_CAUSE_WR_FAULT
> +                                         :
> RISCV_IOMMU_FQ_CAUSE_RD_FAULT;
>          }
>  
>          sc[pass].step++;
> @@ -491,13 +499,13 @@ static int
> riscv_iommu_spa_fetch(RISCVIOMMUState *s, RISCVIOMMUContext *ctx,
>               * non-user mode leaf PTE and !pv we need to fault.
>               */
>              break;
> -        } else if ((iotlb->perm & IOMMU_RO) && !(pte & PTE_R)) {
> +        } else if ((req_perm & IOMMU_RO) && !(pte & PTE_R)) {
>              break;                /* Read access check failed */
> -        } else if ((iotlb->perm & IOMMU_WO) && !(pte & PTE_W)) {
> +        } else if ((req_perm & IOMMU_WO) && !(pte & PTE_W)) {
>              break;                /* Write access check failed */
>          } else if (!ade && !(pte & PTE_A)) {
>              break;                /* Access bit not set */
> -        } else if ((iotlb->perm & IOMMU_WO) && !ade && !(pte &
> PTE_D)) {
> +        } else if ((req_perm & IOMMU_WO) && !ade && !(pte & PTE_D))
> {
>              break;                /* Dirty bit not set */
>          } else if (pass == G_STAGE && !(pte & PTE_U)) {
>              /*
> @@ -532,21 +540,20 @@ static int
> riscv_iommu_spa_fetch(RISCVIOMMUState *s, RISCVIOMMUContext *ctx,
>                  addr = iotlb->iova;
>                  continue;
>              }
> +
> +            /* Cache the effective permission, not this request's
> subset. */
> +            IOMMUAccessFlags leaf_perm = (pte & PTE_W) ?
> +                                         ((pte & PTE_R) ? IOMMU_RW :
> IOMMU_WO) :
> +                                         IOMMU_RO;
> +
> +            trans_perm = trans_perm == IOMMU_NONE ?
> +                         leaf_perm : trans_perm & leaf_perm;
> +
>              /* Translation phase completed (GPA or SPA) */
>              iotlb->translated_addr = base;
>  
> -            /*
> -             * Do a bit_and between the PTE bits and the original
> -             * request flags to determine the exact permission we
> -             * need, i.e. if the original request is RO and the
> -             * PTE has RW flags the actual perm is RO.
> -             */
> -            pte_perm = (pte & PTE_W) ? ((pte & PTE_R) ? IOMMU_RW :
> IOMMU_WO)
> -                                     : IOMMU_RO;
> -            iotlb->perm &= pte_perm;
> -
>              /* Check MSI GPA address match */
> -            if (pass == S_STAGE && (iotlb->perm & IOMMU_WO) &&
> +            if (pass == S_STAGE && (req_perm & IOMMU_WO) &&
>                  riscv_iommu_msi_check(s, ctx, base)) {
>                  /* Trap MSI writes and return GPA address. */
>                  iotlb->target_as = &s->trap_as;
> @@ -563,6 +570,7 @@ static int riscv_iommu_spa_fetch(RISCVIOMMUState
> *s, RISCVIOMMUContext *ctx,
>                  continue;
>              }
>  
> +            iotlb->perm = trans_perm;
>              return 0;
>          }
>  
> @@ -587,7 +595,7 @@ static int riscv_iommu_spa_fetch(RISCVIOMMUState
> *s, RISCVIOMMUContext *ctx,
>       */
>      iotlb->translated_addr = addr;
>  
> -    return (iotlb->perm & IOMMU_WO) ?
> +    return (req_perm & IOMMU_WO) ?
>                  (pass ? RISCV_IOMMU_FQ_CAUSE_WR_FAULT_VS :
>                          RISCV_IOMMU_FQ_CAUSE_WR_FAULT_S) :
>                  (pass ? RISCV_IOMMU_FQ_CAUSE_RD_FAULT_VS :