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]>
Thanks!
Applied to riscv-to-apply.next
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
>
>
