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 :
