hw/riscv/riscv-iommu.c | 50 ++++++++++++++++++++++++------------------ 1 file changed, 29 insertions(+), 21 deletions(-)
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 <andrew.jones@oss.qualcomm.com>
---
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 :
--
2.43.0On Fri, Jul 17, 2026 at 04:45:25PM +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 <andrew.jones@oss.qualcomm.com>
> ---
> hw/riscv/riscv-iommu.c | 50 ++++++++++++++++++++++++------------------
> 1 file changed, 29 insertions(+), 21 deletions(-)
>
Based-on: 20260717112340.1071148-1-andrew.jones@oss.qualcomm.com
Thanks,
drew
On 7/17/2026 11:45 AM, 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 <andrew.jones@oss.qualcomm.com>
> ---
Patch is good but it won't apply cleanly on master without applying the
other fix first:
[PATCH] hw/riscv/riscv-iommu: fix U-bit check to apply only to leaf S/VS-stage PTEs
You can either re-send this rebased on top of master or send these 2 fixes in a
patch series together, implying that one comes after the other.
Reviewed-by: Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com>
> 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 :
© 2016 - 2026 Red Hat, Inc.