On Wed, Jul 1, 2026 at 10:41 PM Daniel Henrique Barboza <[email protected]> wrote: > > Under certain circunstances, like the one described in [1] and [2], > a read operation that faults will be logged as a write fault instead, > and vice-versa, if they happen 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/3557 > [2] 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/3557 > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3577 > Signed-off-by: Daniel Henrique Barboza <[email protected]>
Acked-by: Alistair Francis <[email protected]> Alistair > --- > 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 974042d017..7f94ce152e 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) && > @@ -1727,7 +1736,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; > -- > 2.43.0 > >
