On Thu, Mar 05, 2026 at 01:35:51PM +0800, Jim Shu wrote: > Hi Chao, > > > On Mon, Mar 2, 2026 at 1:51 PM Chao Liu <[email protected]> wrote: > > > > Hi Jim, > > > > On Mon, Feb 02, 2026 at 03:53:58PM +0800, Jim Shu wrote: > > > Instead of IOMMU_NONE, address_space_translate_for_iotlb() now can pass > > > the correct iommu_flags to the IOMMU translate function from the > > > access_type. > > > > > > Since RISC-V wgChecker [1] could permit access in RO or WO permission > > > only, the IOMMUMemoryRegion could return different section for > > > read and write access. To support this kind of IOMMUMemoryRegion > > > in the path of CPU access, we should pass correct iommu_flags here. > > > > > > [1] RISC-V WG: > > > https://patchew.org/QEMU/[email protected]/ > > > > > > Signed-off-by: Jim Shu <[email protected]> > > > --- > > > accel/tcg/cputlb.c | 3 ++- > > > include/accel/tcg/iommu.h | 3 ++- > > > system/physmem.c | 16 +++++++++++----- > > > 3 files changed, 15 insertions(+), 7 deletions(-) > > > > > > diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c > > > index 2a0f4cfff62..404a8607b9b 100644 > > > --- a/accel/tcg/cputlb.c > > > +++ b/accel/tcg/cputlb.c > > > @@ -1050,7 +1050,8 @@ void tlb_set_page_full(CPUState *cpu, int mmu_idx, > > > prot = full->prot; > > > asidx = cpu_asidx_from_attrs(cpu, full->attrs); > > > section = address_space_translate_for_iotlb(cpu, asidx, paddr_page, > > > - &xlat, &sz, full->attrs, > > > &prot); > > > + &xlat, &sz, full->attrs, > > > &prot, > > > + access_type); > > > assert(sz >= TARGET_PAGE_SIZE); > > > > > > tlb_debug("vaddr=%016" VADDR_PRIx " paddr=0x" HWADDR_FMT_plx > > > diff --git a/include/accel/tcg/iommu.h b/include/accel/tcg/iommu.h > > > index 547f8ea0ef0..2a79f859834 100644 > > > --- a/include/accel/tcg/iommu.h > > > +++ b/include/accel/tcg/iommu.h > > > @@ -20,7 +20,8 @@ MemoryRegionSection > > > *address_space_translate_for_iotlb(CPUState *cpu, > > > hwaddr *xlat, > > > hwaddr *plen, > > > MemTxAttrs attrs, > > > - int *prot); > > > + int *prot, > > > + MMUAccessType > > > access_type); > > > > > > #endif > > > > > > diff --git a/system/physmem.c b/system/physmem.c > > > index 2fb0c25c93b..337137489de 100644 > > > --- a/system/physmem.c > > > +++ b/system/physmem.c > > > @@ -683,12 +683,14 @@ void tcg_iommu_init_notifier_list(CPUState *cpu) > > > MemoryRegionSection * > > > address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr > > > orig_addr, > > > hwaddr *xlat, hwaddr *plen, > > > - MemTxAttrs attrs, int *prot) > > > + MemTxAttrs attrs, int *prot, > > > + MMUAccessType access_type) > > > { > > > MemoryRegionSection *section; > > > IOMMUMemoryRegion *iommu_mr; > > > IOMMUMemoryRegionClass *imrc; > > > IOMMUTLBEntry iotlb; > > > + IOMMUAccessFlags iommu_flags; > > > int iommu_idx; > > > hwaddr addr = orig_addr; > > > AddressSpaceDispatch *d = > > > address_space_to_dispatch(cpu->cpu_ases[asidx].as); > > > @@ -705,10 +707,14 @@ address_space_translate_for_iotlb(CPUState *cpu, > > > int asidx, hwaddr orig_addr, > > > > > > iommu_idx = imrc->attrs_to_index(iommu_mr, attrs); > > > tcg_register_iommu_notifier(cpu, iommu_mr, iommu_idx); > > > - /* We need all the permissions, so pass IOMMU_NONE so the IOMMU > > > - * doesn't short-cut its translation table walk. > > > - */ > > > - iotlb = imrc->translate(iommu_mr, addr, IOMMU_NONE, iommu_idx); > > > + > > > + if (access_type == MMU_DATA_STORE) { > > > + iommu_flags = IOMMU_WO; > > > + } else { > > > + iommu_flags = IOMMU_RO; > > > + } > > This maps both MMU_INST_FETCH and MMU_DATA_LOAD to IOMMU_RO. Is this > > intentional? > > Some IOMMU implementations might want to distinguish between instruction > > fetch and data read. > > Thanks for pointing out it! > I don't know IOMMUAccessFlags enum now has IOMMU_EXEC. > I think mapping three access_types to IOMMU_RO/WO/EXEC is more > accurate. I will fix it in the v2 series. > > > > > Or is the current mapping sufficient for your use case (RISC-V wgChecker)? > Yes, RW is enough for RISC-V wgChecker now. We don't distinguish EXEC > permission from READ permission. > But I think supporting RWX permissions in the generic code is better. > Perhaps future devices will distinguish RWX permissions. > Thanks for the explanation. It makes sense that wgChecker treats instruction fetch and data read uniformly at the IOMMU level given the current RISC-V spec.
I agree that adding full RWX support in the generic path is the right approach. Having the mapping explicitly cover all three cases: MMU_DATA_LOAD -> IOMMU_RO MMU_DATA_STORE -> IOMMU_WO MMU_INST_FETCH -> IOMMU_EXEC will make the code cleaner and ready for IOMMU implementations that do enforce execute permissions separately. Looking forward to the v2 series. Thanks, Chao > > > > Thanks, > > Chao > > > + > > > + iotlb = imrc->translate(iommu_mr, addr, iommu_flags, iommu_idx); > > > addr = ((iotlb.translated_addr & ~iotlb.addr_mask) > > > | (addr & iotlb.addr_mask)); > > > /* Update the caller's prot bits to remove permissions the IOMMU > > > -- > > > 2.43.0 > > > > > >
