From: Daniel Henrique Barboza <[email protected]> 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]> Message-ID: <[email protected]> Signed-off-by: Alistair Francis <[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 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.54.0
