On Mon, Aug 12, 2024 at 10:47:33AM +1000, Alistair Francis wrote:
> [EXTERNAL MAIL]
>
> 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()`.
Thanks for the suggestion. I will clarify it.
>
> > > > +{
> > > > + 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?
>
Did you mean the target_mr in iopmp_setup_system_memory? It specifies
the container that IOPMP wants to protect. We don't create
separate iommus for each subregion. We create a single iommu for the
entire container (system memory).
The access to target_mr (system memory) which has iommu region of
IOPMP, will be translated to downstream which has protected device
regions.
Thanks,
Ethan Chen
