Hi Philippe, Thanks for reviewing it!
On Wed, Jan 28, 2026 at 4:08 PM Philippe Mathieu-Daudé <[email protected]> wrote: > > 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? I will fix it in the v2 patch. > > > { > > 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": I will fix it in the v2 patch. > > * @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. I will rename it in the v2 patch. > > > + > > + /* @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]> >
