On Fri, Aug 9, 2024 at 8:11 PM Ethan Chen <etha...@andestech.com> wrote:
>
> On Thu, Aug 08, 2024 at 02:23:56PM +1000, Alistair Francis wrote:
> >
> > On Mon, Jul 15, 2024 at 8:13 PM Ethan Chen via <qemu-devel@nongnu.org>
> > wrote:
> > >
> > > To enable system memory transactions through the IOPMP, memory regions
> > > must
> > > be moved to the IOPMP downstream and then replaced with IOMMUs for IOPMP
> > > translation.
> > >
> > > The iopmp_setup_system_memory() function copies subregions of system
> > > memory
> > > to create the IOPMP downstream and then replaces the specified memory
> > > regions in system memory with the IOMMU regions of the IOPMP. It also
> > > adds entries to a protection map that records the relationship between
> > > physical address regions and the IOPMP, which is used by the IOPMP DMA
> > > API to send transaction information.
> > >
> > > Signed-off-by: Ethan Chen <etha...@andestech.com>
> > > ---
> > > hw/misc/riscv_iopmp.c | 61 +++++++++++++++++++++++++++++++++++
> > > include/hw/misc/riscv_iopmp.h | 3 ++
> > > 2 files changed, 64 insertions(+)
> > >
> > > diff --git a/hw/misc/riscv_iopmp.c b/hw/misc/riscv_iopmp.c
> > > index db43e3c73f..e62ac57437 100644
> > > --- a/hw/misc/riscv_iopmp.c
> > > +++ b/hw/misc/riscv_iopmp.c
> > > @@ -1151,4 +1151,65 @@ iopmp_register_types(void)
> > > type_register_static(&iopmp_iommu_memory_region_info);
> > > }
> > >
> > > +/*
> > > + * Copies subregions from the source memory region to the destination
> > > memory
> > > + * region
> > > + */
> > > +static void copy_memory_subregions(MemoryRegion *src_mr, MemoryRegion
> > > *dst_mr)
Maybe `alias_memory_subregions()` or `link_memory_subregions()`
instead of `copy_memory_subregions()`.
> > > +{
> > > + int32_t priority;
> > > + hwaddr addr;
> > > + MemoryRegion *alias, *subregion;
> > > + QTAILQ_FOREACH(subregion, &src_mr->subregions, subregions_link) {
> > > + priority = subregion->priority;
> > > + addr = subregion->addr;
> > > + alias = g_malloc0(sizeof(MemoryRegion));
> > > + memory_region_init_alias(alias, NULL, subregion->name,
> > > subregion, 0,
> > > + memory_region_size(subregion));
> > > + memory_region_add_subregion_overlap(dst_mr, addr, alias,
> > > priority);
> > > + }
> > > +}
> >
> > This seems strange. Do we really need to do this?
> >
> > I haven't looked at the memory_region stuff for awhile, but this seems
> > clunky and prone to breakage.
> >
> > We already link s->iommu with the system memory
> >
>
> s->iommu occupies the address of the protected devices in system
> memory. Since IOPMP does not alter address, the target address space
> must differ from system memory to avoid infinite recursive iommu access.
>
> The transaction will be redirected to a downstream memory region, which
> is almost identical to system memory but without the iommu memory
> region of IOPMP.
>
> This function serves as a helper to create that downstream memory region.
What I don't understand is that we already have target_mr as a
subregion of downstream, is that not enough?
Alistair
