Re: [PATCH v8 5/8] hw/misc/riscv_iopmp: Add API to set up IOPMP protection for system memory

[Ethan Chen via <qemu-riscv@nongnu.org>]

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 <ethan84@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 <ethan84@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