Glad to see I’m not alone seeing this bug :-) And THANKS for the fix Jim,
My idea had been to carry the address space in the fitly full entry, but I like your plan even more, it removes even more complexity. I’ve tested in it my setup, and it works, Cheers Mark. > On 28 Jan 2026, at 09:08, Philippe Mathieu-Daudé <[email protected]> wrote: > > WARNING: This email originated from outside of Qualcomm. Please be wary of > any links or attachments, and do not enable macros. > > Hi Jim, > > On 28/1/26 07:39, Jim Shu wrote: >> 'CPUTLBEntryFull.xlat_section' stores section_index in last 12 bits to >> find the correct section when CPU access the IO region over the IOTLB. >> However, section_index is only unique inside single AddressSpace. If >> address space translation is over IOMMUMemoryRegion, it could return >> section from other AddressSpace. 'iotlb_to_section()' API only finds the >> sections from CPU's AddressSpace so that it couldn't find section in >> other AddressSpace. Thus, using 'iotlb_to_section()' API will find the >> wrong section and QEMU will have wrong load/store access. >> >> To fix this bug of iotlb_to_section(), store complete MemoryRegionSection >> pointer in CPUTLBEntryFull to replace the section_index in xlat_section. >> Rename 'xlat_section' to 'xlat' as we remove last 12 bits section_index >> inside. Also, since we directly use section pointer in the >> CPUTLBEntryFull (full->section), we can remove the unused functions: >> iotlb_to_section(), memory_region_section_get_iotlb(). >> >> This bug occurs only when >> (1) IOMMUMemoryRegion is in the path of CPU access. >> (2) IOMMUMemoryRegion returns different target_as and the section is in >> the IO region. >> >> Common IOMMU devices don't have this issue since they are only in the >> path of DMA access. Currently, the bug only occurs when ARM MPC device >> (hw/misc/tz-mpc.c) returns 'blocked_io_as' to emulate blocked access >> handling. Upcoming RISC-V wgChecker [1] and IOPMP [2] devices are also >> affected by this bug. >> >> [1] RISC-V WG: >> https://patchew.org/QEMU/[email protected]/ >> [2] RISC-V IOPMP: >> https://patchew.org/QEMU/[email protected]/ >> >> Signed-off-by: Jim Shu <[email protected]> >> --- >> accel/tcg/cputlb.c | 32 +++++++++++++++----------------- >> include/accel/tcg/iommu.h | 15 --------------- >> include/exec/cputlb.h | 2 +- >> include/hw/core/cpu.h | 12 +++++++----- >> system/physmem.c | 25 ------------------------- >> 5 files changed, 23 insertions(+), 63 deletions(-) >> >> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c >> index 6900a126827..c61339d10a3 100644 >> --- a/accel/tcg/cputlb.c >> +++ b/accel/tcg/cputlb.c >> @@ -1090,7 +1090,7 @@ void tlb_set_page_full(CPUState *cpu, int mmu_idx, >> } >> } else { >> /* I/O or ROMD */ >> - iotlb = memory_region_section_get_iotlb(cpu, section) + xlat; >> + iotlb = xlat; >> /* >> * Writes to romd devices must go through MMIO to enable write. >> * Reads to romd devices go through the ram_ptr found above, >> @@ -1141,10 +1141,9 @@ void tlb_set_page_full(CPUState *cpu, int mmu_idx, >> /* >> * When memory region is ram, iotlb contains a TARGET_PAGE_BITS >> * aligned ram_addr_t of the page base of the target RAM. >> - * Otherwise, iotlb contains >> - * - a physical section number in the lower TARGET_PAGE_BITS >> - * - the offset within section->mr of the page base (I/O, ROMD) with >> the >> - * TARGET_PAGE_BITS masked off. >> + * Otherwise, iotlb contains a TARGET_PAGE_BITS aligned >> + * offset within section->mr of the page base (I/O, ROMD) >> + * >> * We subtract addr_page (which is page aligned and thus won't >> * disturb the low bits) to give an offset which can be added to the >> * (non-page-aligned) vaddr of the eventual memory access to get >> @@ -1154,7 +1153,8 @@ void tlb_set_page_full(CPUState *cpu, int mmu_idx, >> */ >> desc->fulltlb[index] = *full; >> full = &desc->fulltlb[index]; >> - full->xlat_section = iotlb - addr_page; >> + full->xlat = iotlb - addr_page; >> + full->section = section; >> full->phys_addr = paddr_page; >> >> /* Now calculate the new entry */ >> @@ -1270,14 +1270,14 @@ static inline void cpu_unaligned_access(CPUState >> *cpu, vaddr addr, >> } >> >> static MemoryRegionSection * >> -io_prepare(hwaddr *out_offset, CPUState *cpu, hwaddr xlat, >> +io_prepare(hwaddr *out_offset, CPUState *cpu, CPUTLBEntryFull *full, >> MemTxAttrs attrs, vaddr addr, uintptr_t retaddr) > > Can you move the io_prepare() argument change in a preliminary patch > to make this one simpler to review? > >> { >> MemoryRegionSection *section; >> hwaddr mr_offset; >> >> - section = iotlb_to_section(cpu, xlat, attrs); >> - mr_offset = (xlat & TARGET_PAGE_MASK) + addr; >> + section = full->section; >> + mr_offset = full->xlat + addr; >> cpu->mem_io_pc = retaddr; >> if (!cpu->neg.can_do_io) { >> cpu_io_recompile(cpu, retaddr); >> @@ -1336,7 +1336,7 @@ static bool victim_tlb_hit(CPUState *cpu, size_t >> mmu_idx, size_t index, >> static void notdirty_write(CPUState *cpu, vaddr mem_vaddr, unsigned size, >> CPUTLBEntryFull *full, uintptr_t retaddr) >> { >> - ram_addr_t ram_addr = mem_vaddr + full->xlat_section; >> + ram_addr_t ram_addr = mem_vaddr + full->xlat; >> >> trace_memory_notdirty_write_access(mem_vaddr, ram_addr, size); >> >> @@ -1593,9 +1593,7 @@ bool tlb_plugin_lookup(CPUState *cpu, vaddr addr, int >> mmu_idx, >> >> /* We must have an iotlb entry for MMIO */ >> if (tlb_addr & TLB_MMIO) { >> - MemoryRegionSection *section = >> - iotlb_to_section(cpu, full->xlat_section & ~TARGET_PAGE_MASK, >> - full->attrs); >> + MemoryRegionSection *section = full->section; >> data->is_io = true; >> data->mr = section->mr; >> } else { >> @@ -1981,7 +1979,7 @@ static uint64_t do_ld_mmio_beN(CPUState *cpu, >> CPUTLBEntryFull *full, >> tcg_debug_assert(size > 0 && size <= 8); >> >> attrs = full->attrs; >> - section = io_prepare(&mr_offset, cpu, full->xlat_section, attrs, addr, >> ra); >> + section = io_prepare(&mr_offset, cpu, full, attrs, addr, ra); >> mr = section->mr; >> >> BQL_LOCK_GUARD(); >> @@ -2002,7 +2000,7 @@ static Int128 do_ld16_mmio_beN(CPUState *cpu, >> CPUTLBEntryFull *full, >> tcg_debug_assert(size > 8 && size <= 16); >> >> attrs = full->attrs; >> - section = io_prepare(&mr_offset, cpu, full->xlat_section, attrs, addr, >> ra); >> + section = io_prepare(&mr_offset, cpu, full, attrs, addr, ra); >> mr = section->mr; >> >> BQL_LOCK_GUARD(); >> @@ -2499,7 +2497,7 @@ static uint64_t do_st_mmio_leN(CPUState *cpu, >> CPUTLBEntryFull *full, >> tcg_debug_assert(size > 0 && size <= 8); >> >> attrs = full->attrs; >> - section = io_prepare(&mr_offset, cpu, full->xlat_section, attrs, addr, >> ra); >> + section = io_prepare(&mr_offset, cpu, full, attrs, addr, ra); >> mr = section->mr; >> >> BQL_LOCK_GUARD(); >> @@ -2519,7 +2517,7 @@ static uint64_t do_st16_mmio_leN(CPUState *cpu, >> CPUTLBEntryFull *full, >> tcg_debug_assert(size > 8 && size <= 16); >> >> attrs = full->attrs; >> - section = io_prepare(&mr_offset, cpu, full->xlat_section, attrs, addr, >> ra); >> + section = io_prepare(&mr_offset, cpu, full, attrs, addr, ra); >> mr = section->mr; >> >> BQL_LOCK_GUARD(); >> diff --git a/include/accel/tcg/iommu.h b/include/accel/tcg/iommu.h >> index 90cfd6c0ed1..547f8ea0ef0 100644 >> --- a/include/accel/tcg/iommu.h >> +++ b/include/accel/tcg/iommu.h >> @@ -14,18 +14,6 @@ >> #include "exec/hwaddr.h" >> #include "exec/memattrs.h" >> >> -/** >> - * iotlb_to_section: >> - * @cpu: CPU performing the access >> - * @index: TCG CPU IOTLB entry >> - * >> - * Given a TCG CPU IOTLB entry, return the MemoryRegionSection that >> - * it refers to. @index will have been initially created and returned >> - * by memory_region_section_get_iotlb(). >> - */ >> -MemoryRegionSection *iotlb_to_section(CPUState *cpu, >> - hwaddr index, MemTxAttrs attrs); >> - >> MemoryRegionSection *address_space_translate_for_iotlb(CPUState *cpu, >> int asidx, >> hwaddr addr, >> @@ -34,8 +22,5 @@ MemoryRegionSection >> *address_space_translate_for_iotlb(CPUState *cpu, >> MemTxAttrs attrs, >> int *prot); >> >> -hwaddr memory_region_section_get_iotlb(CPUState *cpu, >> - MemoryRegionSection *section); >> - >> #endif >> >> diff --git a/include/exec/cputlb.h b/include/exec/cputlb.h >> index 0d1d46429c9..e599a0f7627 100644 >> --- a/include/exec/cputlb.h >> +++ b/include/exec/cputlb.h >> @@ -44,7 +44,7 @@ void tlb_reset_dirty_range_all(ram_addr_t start, >> ram_addr_t length); >> * @full: the details of the tlb entry >> * >> * Add an entry to @cpu tlb index @mmu_idx. All of the fields of >> - * @full must be filled, except for xlat_section, and constitute >> + * @full must be filled, except for xlat, and constitute >> * the complete description of the translated page. >> * >> * This is generally called by the target tlb_fill function after >> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h >> index 61da2ea4331..7de576ab602 100644 >> --- a/include/hw/core/cpu.h >> +++ b/include/hw/core/cpu.h >> @@ -219,15 +219,17 @@ typedef uint32_t MMUIdxMap; >> */ >> struct CPUTLBEntryFull { >> /* >> - * @xlat_section contains: >> - * - in the lower TARGET_PAGE_BITS, a physical section number >> - * - with the lower TARGET_PAGE_BITS masked off, an offset which >> - * must be added to the virtual address to obtain: >> + * @xlat contains: >> + * - a TARGET_PAGE_BITS aligned offset which must be added to > > Drop "contains": > > * @xlat_offset: TARGET_PAGE_BITS aligned offset which must be added to > >> + * the virtual address to obtain: >> * + the ram_addr_t of the target RAM (if the physical section >> * number is PHYS_SECTION_NOTDIRTY or PHYS_SECTION_ROM) >> * + the offset within the target MemoryRegion (otherwise) >> */ >> - hwaddr xlat_section; >> + hwaddr xlat; > > I'd rename as 'xlat_offset' instead for clarity. > >> + >> + /* @section contains physical section. */ >> + MemoryRegionSection *section; > > Good. Safe because 'the CPUIOTLBEntry layout is not as critical as that > of the CPUTLBEntry.' (commit e469b22ffda). > >> >> /* >> * @phys_addr contains the physical address in the address space >> diff --git a/system/physmem.c b/system/physmem.c >> index b0311f45312..d17596a77fb 100644 >> --- a/system/physmem.c >> +++ b/system/physmem.c >> @@ -747,31 +747,6 @@ translate_fail: >> return &d->map.sections[PHYS_SECTION_UNASSIGNED]; >> } >> >> -MemoryRegionSection *iotlb_to_section(CPUState *cpu, >> - hwaddr index, MemTxAttrs attrs) >> -{ >> - int asidx = cpu_asidx_from_attrs(cpu, attrs); >> - CPUAddressSpace *cpuas = &cpu->cpu_ases[asidx]; >> - AddressSpaceDispatch *d = address_space_to_dispatch(cpuas->as); >> - int section_index = index & ~TARGET_PAGE_MASK; >> - MemoryRegionSection *ret; >> - >> - assert(section_index < d->map.sections_nb); >> - ret = d->map.sections + section_index; >> - assert(ret->mr); >> - assert(ret->mr->ops); >> - >> - return ret; >> -} >> - >> -/* Called from RCU critical section */ >> -hwaddr memory_region_section_get_iotlb(CPUState *cpu, >> - MemoryRegionSection *section) >> -{ >> - AddressSpaceDispatch *d = flatview_to_dispatch(section->fv); >> - return section - d->map.sections; >> -} >> - >> #endif /* CONFIG_TCG */ > > Reviewed-by: Philippe Mathieu-Daudé <[email protected]> >
