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

[LIU Zhiwei]

On 2024/6/13 18:37, Jim Shu wrote:
> 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.

Thanks. It is clear and very helpful.

Zhiwei

> 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).