Re: [RFC PATCH 01/16] accel/tcg: Store section pointer in CPUTLBEntryFull

[Jim Shu]

Hi Zhiwei,

Common IOMMU devices will not have IOMMUMemoryRegion in the path of
CPU access since It only affects DMA access.
In QEMU, it usually places this IOMMU MR as the parent of
"system_memory", and changes the target_mr of DMA from "system_memory"
to IOMMU MR.

For the wgChecker, it is in front of memory or device MMIO and
protects both CPU/DMA access to memory or device MMIO.
In QEMU, wgChecker re-use IOMMUMemoryRegion to implement the memory
protection inside the translate() function of IOMMU MR.
In the machine code, wgChecker replaces the MemoryRegion of protected
resources with the checker's IOMMU MR in the MemoryRegion tree of
"system_memory".
Both CPU/DMA access will go through the "system_memory". They will go
through the checker's IOMMU MR when accessing the protected resources.

This mechanism is used by Cortex-M MPC devices (hw/misc/tz-mpc.c)
originally. I have leveraged it and extended it little (in patch 2) as
MPC doesn't support RO/WO permission.
If we'd like to have a device to do the memory protection of both CPU
& DMA access, we could implement it in this mechanism.
(p.s. Cortex-A TZASC is not supported in QEMU, which is similar to MPC
or wgChecker device.)

Thanks,
Jim Shu





On Thu, Jun 13, 2024 at 2:23 PM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote:
>
> On 2024/6/12 16:14, 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
> > (iotlb_to_section()).
> >
> > 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, store complete MemoryRegionSection pointer in
> > CPUTLBEntryFull instead of section_index.
> >
> > This bug occurs only when
> > (1) IOMMUMemoryRegion is in the path of CPU access.
>
> Hi Jim,
>
> Can you explain a little more on when IOMMUMemoryRegion is in the path
> of CPU access?
>
> Thanks,
> Zhiwei
>
> > (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 device is also affected by this bug.
> >
> > Signed-off-by: Jim Shu <jim.shu@sifive.com>
> > ---
> >   accel/tcg/cputlb.c    | 19 +++++++++----------
> >   include/hw/core/cpu.h |  3 +++
> >   2 files changed, 12 insertions(+), 10 deletions(-)
> >
> > diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> > index 117b516739..8cf124b760 100644
> > --- a/accel/tcg/cputlb.c
> > +++ b/accel/tcg/cputlb.c
> > @@ -1169,6 +1169,7 @@ 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->section = section;
> >       full->phys_addr = paddr_page;
> >
> >       /* Now calculate the new entry */
> > @@ -1248,14 +1249,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)
> >   {
> >       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_section & TARGET_PAGE_MASK) + addr;
> >       cpu->mem_io_pc = retaddr;
> >       if (!cpu->neg.can_do_io) {
> >           cpu_io_recompile(cpu, retaddr);
> > @@ -1571,9 +1572,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 {
> > @@ -1972,7 +1971,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();
> > @@ -1993,7 +1992,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();
> > @@ -2513,7 +2512,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();
> > @@ -2533,7 +2532,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/hw/core/cpu.h b/include/hw/core/cpu.h
> > index a2c8536943..3f6c10897b 100644
> > --- a/include/hw/core/cpu.h
> > +++ b/include/hw/core/cpu.h
> > @@ -217,6 +217,9 @@ typedef struct CPUTLBEntryFull {
> >        */
> >       hwaddr xlat_section;
> >
> > +    /* @section contains physical section. */
> > +    MemoryRegionSection *section;
> > +
> >       /*
> >        * @phys_addr contains the physical address in the address space
> >        * given by cpu_asidx_from_attrs(cpu, @attrs).