1. Patchew
  2. QEMU
  3. [PATCH v8 0/8] Support RISC-V IOPMP
  4. View patch

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

Ethan Chen via posted 8 patches 2 months, 1 week ago
Maintainers: Richard Henderson <richard.henderson@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>, Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Bin Meng <bmeng.cn@gmail.com>, Weiwei Li <liwei1518@gmail.com>, Daniel Henrique Barboza <dbarboza@ventanamicro.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>, Peter Xu <peterx@redhat.com>, David Hildenbrand <david@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>
[PATCH v8 5/8] hw/misc/riscv_iopmp: Add API to set up IOPMP protection for system memory
Posted by Ethan Chen via 2 months, 1 week ago 
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)
+{
+    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);
+    }
+}
+
+/*
+ * Create downstream of system memory for IOPMP, and overlap memory region
+ * specified in memmap with IOPMP translator. Make sure subregions are added to
+ * system memory before call this function. It also add entry to
+ * iopmp_protection_memmaps for recording the relationship between physical
+ * address regions and IOPMP.
+ */
+void iopmp_setup_system_memory(DeviceState *dev, const MemMapEntry *memmap,
+                               uint32_t map_entry_num)
+{
+    IopmpState *s = IOPMP(dev);
+    uint32_t i;
+    MemoryRegion *iommu_alias;
+    MemoryRegion *target_mr = get_system_memory();
+    MemoryRegion *downstream = g_malloc0(sizeof(MemoryRegion));
+    memory_region_init(downstream, NULL, "iopmp_downstream",
+                       memory_region_size(target_mr));
+    /* Copy subregions of target to downstream */
+    copy_memory_subregions(target_mr, downstream);
+
+    iopmp_protection_memmap *map;
+    for (i = 0; i < map_entry_num; i++) {
+        /* Memory access to protected regions of target are through IOPMP */
+        iommu_alias = g_new(MemoryRegion, 1);
+        memory_region_init_alias(iommu_alias, NULL, "iommu_alias",
+                                 MEMORY_REGION(&s->iommu), memmap[i].base,
+                                 memmap[i].size);
+        memory_region_add_subregion_overlap(target_mr, memmap[i].base,
+                                            iommu_alias, 1);
+        /* Record which IOPMP is responsible for the region */
+        map = g_new0(iopmp_protection_memmap, 1);
+        map->iopmp_s = s;
+        map->entry.base = memmap[i].base;
+        map->entry.size = memmap[i].size;
+        QLIST_INSERT_HEAD(&iopmp_protection_memmaps, map, list);
+    }
+    s->downstream = downstream;
+    address_space_init(&s->downstream_as, s->downstream,
+                       "iopmp-downstream-as");
+}
+
+
 type_init(iopmp_register_types);
diff --git a/include/hw/misc/riscv_iopmp.h b/include/hw/misc/riscv_iopmp.h
index b8fe479108..ebe9c4bc4a 100644
--- a/include/hw/misc/riscv_iopmp.h
+++ b/include/hw/misc/riscv_iopmp.h
@@ -165,4 +165,7 @@ typedef struct IopmpState {
     uint32_t fabricated_v;
 } IopmpState;
 
+void iopmp_setup_system_memory(DeviceState *dev, const MemMapEntry *memmap,
+                               uint32_t mapentry_num);
+
 #endif
-- 
2.34.1
Re: [PATCH v8 5/8] hw/misc/riscv_iopmp: Add API to set up IOPMP protection for system memory
Posted by Alistair Francis 1 month, 2 weeks ago 
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)
> +{
> +    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

Alistair

> +
> +/*
> + * Create downstream of system memory for IOPMP, and overlap memory region
> + * specified in memmap with IOPMP translator. Make sure subregions are added to
> + * system memory before call this function. It also add entry to
> + * iopmp_protection_memmaps for recording the relationship between physical
> + * address regions and IOPMP.
> + */
> +void iopmp_setup_system_memory(DeviceState *dev, const MemMapEntry *memmap,
> +                               uint32_t map_entry_num)
> +{
> +    IopmpState *s = IOPMP(dev);
> +    uint32_t i;
> +    MemoryRegion *iommu_alias;
> +    MemoryRegion *target_mr = get_system_memory();
> +    MemoryRegion *downstream = g_malloc0(sizeof(MemoryRegion));
> +    memory_region_init(downstream, NULL, "iopmp_downstream",
> +                       memory_region_size(target_mr));
> +    /* Copy subregions of target to downstream */
> +    copy_memory_subregions(target_mr, downstream);
> +
> +    iopmp_protection_memmap *map;
> +    for (i = 0; i < map_entry_num; i++) {
> +        /* Memory access to protected regions of target are through IOPMP */
> +        iommu_alias = g_new(MemoryRegion, 1);
> +        memory_region_init_alias(iommu_alias, NULL, "iommu_alias",
> +                                 MEMORY_REGION(&s->iommu), memmap[i].base,
> +                                 memmap[i].size);
> +        memory_region_add_subregion_overlap(target_mr, memmap[i].base,
> +                                            iommu_alias, 1);
> +        /* Record which IOPMP is responsible for the region */
> +        map = g_new0(iopmp_protection_memmap, 1);
> +        map->iopmp_s = s;
> +        map->entry.base = memmap[i].base;
> +        map->entry.size = memmap[i].size;
> +        QLIST_INSERT_HEAD(&iopmp_protection_memmaps, map, list);
> +    }
> +    s->downstream = downstream;
> +    address_space_init(&s->downstream_as, s->downstream,
> +                       "iopmp-downstream-as");
> +}
> +
> +
>  type_init(iopmp_register_types);
> diff --git a/include/hw/misc/riscv_iopmp.h b/include/hw/misc/riscv_iopmp.h
> index b8fe479108..ebe9c4bc4a 100644
> --- a/include/hw/misc/riscv_iopmp.h
> +++ b/include/hw/misc/riscv_iopmp.h
> @@ -165,4 +165,7 @@ typedef struct IopmpState {
>      uint32_t fabricated_v;
>  } IopmpState;
>
> +void iopmp_setup_system_memory(DeviceState *dev, const MemMapEntry *memmap,
> +                               uint32_t mapentry_num);
> +
>  #endif
> --
> 2.34.1
>
>
Re: [PATCH v8 5/8] hw/misc/riscv_iopmp: Add API to set up IOPMP protection for system memory
Posted by Ethan Chen via 1 month, 1 week ago 
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)
> > +{
> > +    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.

Thanks,
Ethan Chen

> Alistair
> 
> > +
> > +/*
> > + * Create downstream of system memory for IOPMP, and overlap memory region
> > + * specified in memmap with IOPMP translator. Make sure subregions are added to
> > + * system memory before call this function. It also add entry to
> > + * iopmp_protection_memmaps for recording the relationship between physical
> > + * address regions and IOPMP.
> > + */
> > +void iopmp_setup_system_memory(DeviceState *dev, const MemMapEntry *memmap,
> > +                               uint32_t map_entry_num)
> > +{
> > +    IopmpState *s = IOPMP(dev);
> > +    uint32_t i;
> > +    MemoryRegion *iommu_alias;
> > +    MemoryRegion *target_mr = get_system_memory();
> > +    MemoryRegion *downstream = g_malloc0(sizeof(MemoryRegion));
> > +    memory_region_init(downstream, NULL, "iopmp_downstream",
> > +                       memory_region_size(target_mr));
> > +    /* Copy subregions of target to downstream */
> > +    copy_memory_subregions(target_mr, downstream);
> > +
> > +    iopmp_protection_memmap *map;
> > +    for (i = 0; i < map_entry_num; i++) {
> > +        /* Memory access to protected regions of target are through IOPMP */
> > +        iommu_alias = g_new(MemoryRegion, 1);
> > +        memory_region_init_alias(iommu_alias, NULL, "iommu_alias",
> > +                                 MEMORY_REGION(&s->iommu), memmap[i].base,
> > +                                 memmap[i].size);
> > +        memory_region_add_subregion_overlap(target_mr, memmap[i].base,
> > +                                            iommu_alias, 1);
> > +        /* Record which IOPMP is responsible for the region */
> > +        map = g_new0(iopmp_protection_memmap, 1);
> > +        map->iopmp_s = s;
> > +        map->entry.base = memmap[i].base;
> > +        map->entry.size = memmap[i].size;
> > +        QLIST_INSERT_HEAD(&iopmp_protection_memmaps, map, list);
> > +    }
> > +    s->downstream = downstream;
> > +    address_space_init(&s->downstream_as, s->downstream,
> > +                       "iopmp-downstream-as");
> > +}
> > +
> > +
> >  type_init(iopmp_register_types);
> > diff --git a/include/hw/misc/riscv_iopmp.h b/include/hw/misc/riscv_iopmp.h
> > index b8fe479108..ebe9c4bc4a 100644
> > --- a/include/hw/misc/riscv_iopmp.h
> > +++ b/include/hw/misc/riscv_iopmp.h
> > @@ -165,4 +165,7 @@ typedef struct IopmpState {
> >      uint32_t fabricated_v;
> >  } IopmpState;
> >
> > +void iopmp_setup_system_memory(DeviceState *dev, const MemMapEntry *memmap,
> > +                               uint32_t mapentry_num);
> > +
> >  #endif
> > --
> > 2.34.1
> >
> >
Re: [PATCH v8 5/8] hw/misc/riscv_iopmp: Add API to set up IOPMP protection for system memory
Posted by Alistair Francis 1 month, 1 week ago 
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()`.

> > > +{
> > > +    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
Re: [PATCH v8 5/8] hw/misc/riscv_iopmp: Add API to set up IOPMP protection for system memory
Posted by Ethan Chen via 1 month, 1 week ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.