Hi,
Please ignore this patch.
I just found out that this same code fixes another gitlab bug:
https://gitlab.com/qemu-project/qemu/-/work_items/3557
So I'll re-send it with a slight different commit msg and with a second
"Resolves" tag. The code will be the same.
Thanks,
Daniel
On 6/29/2026 10:09 PM, Daniel Henrique Barboza wrote:
Under certain circunstances, like the one described in [1], a read
operation that faults will be logged as a write fault instead if it
happens after the translation phase in riscv_iommu_spa_fetch().
The first problem is that we're overwriting iotlb->perm with PTE flags,
so an IOMMU_RO access flag can be overwritten by whatever flags
the PTE has. This will cause the wrong fault type to be thrown at
the end of the function in case a fault happens.
To solve the iotlb->perm overwrite we'll bit_and the original
iotlb->perm access flags with the PTE access flags, preserving the
original access type. So a IOMMU_RO access in a R+W PTE will result in
a IOMMU_RO perm.
Second, the resulting fault is received by riscv_iommu_translate(), which
will then report the fault. To do that we require a transaction type
(ttype). We're prioritizing checking "perm & IOMMU_RW" to set a
UADDR_WR ttype, and then checking "perm & IOMMU_RO" to set UADDR_RD
ttype. The issue with that is IOMMU_RO=1 and IOMMU_RW=3, thus checking
"perm & IOMMU_RW" for a write then "perm & IOMMU_RO" for a read will
cause the read fault to always be diagnosed as write.
Make the iotlb->perm matches more strict: "perm & IOMMU_RW" must be
exactly IOMMU_RW, ensuring that 'perm' has both flags. Then we can
check perm & IOMMU_WO and perm & IOMMU_RO without worrying about
overlapping with the RW flag.
[1] https://gitlab.com/qemu-project/qemu/-/work_items/3577
Fixes: 69a9ae4836 ("hw/riscv/riscv-iommu: add ATS support")
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3577
Signed-off-by: Daniel Henrique Barboza <[email protected]>
---
hw/riscv/riscv-iommu.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/hw/riscv/riscv-iommu.c b/hw/riscv/riscv-iommu.c
index c9687e01a8..e1ff5eb864 100644
--- a/hw/riscv/riscv-iommu.c
+++ b/hw/riscv/riscv-iommu.c
@@ -282,6 +282,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;
dma_addr_t addr, base;
uint64_t satp, gatp, pte;
bool en_s, en_g;
@@ -509,8 +510,16 @@ static int riscv_iommu_spa_fetch(RISCVIOMMUState *s,
RISCVIOMMUContext *ctx,
}
/* Translation phase completed (GPA or SPA) */
iotlb->translated_addr = base;
- iotlb->perm = (pte & PTE_W) ? ((pte & PTE_R) ? IOMMU_RW : IOMMU_WO)
- : IOMMU_RO;
+
+ /*
+ * 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) &&
@@ -1704,7 +1713,8 @@ done:
if (fault) {
unsigned ttype = RISCV_IOMMU_FQ_TTYPE_PCIE_ATS_REQ;
- if (iotlb->perm & IOMMU_RW) {
+ if ((iotlb->perm & IOMMU_RW) == IOMMU_RW
+ || iotlb->perm & IOMMU_WO) {
ttype = RISCV_IOMMU_FQ_TTYPE_UADDR_WR;
} else if (iotlb->perm & IOMMU_RO) {
ttype = RISCV_IOMMU_FQ_TTYPE_UADDR_RD;
