[RFC PATCH] memory: Introduce memory region fetch operation

Ethan Chen via posted 1 patch 11 months ago
accel/tcg/cputlb.c    |   9 +++-
include/exec/memory.h |  30 ++++++++++++
system/memory.c       | 104 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 141 insertions(+), 2 deletions(-)
[RFC PATCH] memory: Introduce memory region fetch operation
Posted by Ethan Chen via 11 months ago
Allow the memory region to have different behaviors for read and fetch
operations.

For example RISCV IOPMP will raise interrupt when cpu try to fetch a
non-excutable region.

If fetch operation of a memory region is not implemented, it still uses the
read operation for fetch.

Signed-off-by: Ethan Chen <ethan84@andestech.com>
---
 accel/tcg/cputlb.c    |   9 +++-
 include/exec/memory.h |  30 ++++++++++++
 system/memory.c       | 104 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 141 insertions(+), 2 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 117b516739..edb3715017 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1942,8 +1942,13 @@ static uint64_t int_ld_mmio_beN(CPUState *cpu, CPUTLBEntryFull *full,
         this_size = 1 << this_mop;
         this_mop |= MO_BE;
 
-        r = memory_region_dispatch_read(mr, mr_offset, &val,
-                                        this_mop, full->attrs);
+        if (type == MMU_INST_FETCH) {
+            r = memory_region_dispatch_fetch(mr, mr_offset, &val,
+                                             this_mop, full->attrs);
+        } else {
+            r = memory_region_dispatch_read(mr, mr_offset, &val,
+                                            this_mop, full->attrs);
+        }
         if (unlikely(r != MEMTX_OK)) {
             io_failed(cpu, full, addr, this_size, type, mmu_idx, r, ra);
         }
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 1be58f694c..6d61ec443e 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -274,6 +274,13 @@ struct MemoryRegionOps {
                   uint64_t data,
                   unsigned size);
 
+    /* Fetch to the memory region. @addr is relative to @mr; @size is
+     * in bytes. */
+    uint64_t (*fetch)(void *opaque,
+                      hwaddr addr,
+                      unsigned size);
+
+
     MemTxResult (*read_with_attrs)(void *opaque,
                                    hwaddr addr,
                                    uint64_t *data,
@@ -284,6 +291,12 @@ struct MemoryRegionOps {
                                     uint64_t data,
                                     unsigned size,
                                     MemTxAttrs attrs);
+    MemTxResult (*fetch_with_attrs)(void *opaque,
+                                    hwaddr addr,
+                                    uint64_t *data,
+                                    unsigned size,
+                                    MemTxAttrs attrs);
+
 
     enum device_endian endianness;
     /* Guest-visible constraints: */
@@ -2672,6 +2685,23 @@ MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
                                          MemOp op,
                                          MemTxAttrs attrs);
 
+
+/**
+ * memory_region_dispatch_fetch: perform a fetch directly to the specified
+ * MemoryRegion.
+ *
+ * @mr: #MemoryRegion to access
+ * @addr: address within that region
+ * @pval: pointer to uint64_t which the data is written to
+ * @op: size, sign, and endianness of the memory operation
+ * @attrs: memory transaction attributes to use for the access
+ */
+MemTxResult memory_region_dispatch_fetch(MemoryRegion *mr,
+                                         hwaddr addr,
+                                         uint64_t *pval,
+                                         MemOp op,
+                                         MemTxAttrs attrs);
+
 /**
  * address_space_init: initializes an address space
  *
diff --git a/system/memory.c b/system/memory.c
index 74cd73ebc7..ecd3020b1c 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -477,6 +477,51 @@ static MemTxResult memory_region_read_with_attrs_accessor(MemoryRegion *mr,
     return r;
 }
 
+static MemTxResult memory_region_fetch_accessor(MemoryRegion *mr,
+                                                hwaddr addr,
+                                                uint64_t *value,
+                                                unsigned size,
+                                                signed shift,
+                                                uint64_t mask,
+                                                MemTxAttrs attrs)
+{
+    uint64_t tmp;
+
+    tmp = mr->ops->fetch(mr->opaque, addr, size);
+    if (mr->subpage) {
+        trace_memory_region_subpage_fetch(get_cpu_index(), mr, addr, tmp, size);
+    } else if (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_FETCH)) {
+        hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr);
+        trace_memory_region_ops_fetch(get_cpu_index(), mr, abs_addr, tmp, size,
+                                     memory_region_name(mr));
+    }
+    memory_region_shift_read_access(value, shift, mask, tmp);
+    return MEMTX_OK;
+}
+
+static MemTxResult memory_region_fetch_with_attrs_accessor(MemoryRegion *mr,
+                                                          hwaddr addr,
+                                                          uint64_t *value,
+                                                          unsigned size,
+                                                          signed shift,
+                                                          uint64_t mask,
+                                                          MemTxAttrs attrs)
+{
+    uint64_t tmp = 0;
+    MemTxResult r;
+
+    r = mr->ops->fetch_with_attrs(mr->opaque, addr, &tmp, size, attrs);
+    if (mr->subpage) {
+        trace_memory_region_subpage_fetch(get_cpu_index(), mr, addr, tmp, size);
+    } else if (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_FETCH)) {
+        hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr);
+        trace_memory_region_ops_fetch(get_cpu_index(), mr, abs_addr, tmp, size,
+                                      memory_region_name(mr));
+    }
+    memory_region_shift_read_access(value, shift, mask, tmp);
+    return r;
+}
+
 static MemTxResult memory_region_write_accessor(MemoryRegion *mr,
                                                 hwaddr addr,
                                                 uint64_t *value,
@@ -1461,6 +1506,65 @@ MemTxResult memory_region_dispatch_read(MemoryRegion *mr,
     return r;
 }
 
+static MemTxResult memory_region_dispatch_fetch1(MemoryRegion *mr,
+                                                hwaddr addr,
+                                                uint64_t *pval,
+                                                unsigned size,
+                                                MemTxAttrs attrs)
+{
+    *pval = 0;
+
+    if (mr->ops->fetch) {
+        return access_with_adjusted_size(addr, pval, size,
+                                         mr->ops->impl.min_access_size,
+                                         mr->ops->impl.max_access_size,
+                                         memory_region_fetch_accessor,
+                                         mr, attrs);
+    } else if (mr->ops->fetch_with_attrs) {
+        return access_with_adjusted_size(addr, pval, size,
+            mr->ops->impl.min_access_size,
+            mr->ops->impl.max_access_size,
+            memory_region_fetch_with_attrs_accessor,
+            mr, attrs);
+    } else if (mr->ops->read) {
+        return access_with_adjusted_size(addr, pval, size,
+                                         mr->ops->impl.min_access_size,
+                                         mr->ops->impl.max_access_size,
+                                         memory_region_read_accessor,
+                                         mr, attrs);
+    } else {
+        return access_with_adjusted_size(addr, pval, size,
+                                         mr->ops->impl.min_access_size,
+                                         mr->ops->impl.max_access_size,
+                                         memory_region_read_with_attrs_accessor,
+                                         mr, attrs);
+    }
+}
+
+MemTxResult memory_region_dispatch_fetch(MemoryRegion *mr,
+                                        hwaddr addr,
+                                        uint64_t *pval,
+                                        MemOp op,
+                                        MemTxAttrs attrs)
+{
+    unsigned size = memop_size(op);
+    MemTxResult r;
+
+    if (mr->alias) {
+        return memory_region_dispatch_fetch(mr->alias,
+                                           mr->alias_offset + addr,
+                                           pval, op, attrs);
+    }
+    if (!memory_region_access_valid(mr, addr, size, false, attrs)) {
+        *pval = unassigned_mem_read(mr, addr, size);
+        return MEMTX_DECODE_ERROR;
+    }
+
+    r = memory_region_dispatch_fetch1(mr, addr, pval, size, attrs);
+    adjust_endianness(mr, pval, op);
+    return r;
+}
+
 /* Return true if an eventfd was signalled */
 static bool memory_region_dispatch_write_eventfds(MemoryRegion *mr,
                                                     hwaddr addr,
-- 
2.34.1
Re: [RFC PATCH] memory: Introduce memory region fetch operation
Posted by Peter Maydell 11 months ago
On Wed, 12 Jun 2024 at 10:02, Ethan Chen via <qemu-devel@nongnu.org> wrote:
>
> Allow the memory region to have different behaviors for read and fetch
> operations.
>
> For example RISCV IOPMP will raise interrupt when cpu try to fetch a
> non-excutable region.

It actually raises an interrupt rather than it being a permissions fault?

> If fetch operation of a memory region is not implemented, it still uses the
> read operation for fetch.

This patch should probably be part of the series with the device that
needs it.

thanks
-- PMM
Re: [RFC PATCH] memory: Introduce memory region fetch operation
Posted by Ethan Chen via 11 months ago
On Wed, Jun 12, 2024 at 01:43:41PM +0100, Peter Maydell wrote:
> 
> On Wed, 12 Jun 2024 at 10:02, Ethan Chen via <qemu-devel@nongnu.org> wrote:
> >
> > Allow the memory region to have different behaviors for read and fetch
> > operations.
> >
> > For example RISCV IOPMP will raise interrupt when cpu try to fetch a
> > non-excutable region.
> 
> It actually raises an interrupt rather than it being a permissions fault?

Device can return bus error, interrupt or success with fabricated data.

> 
> > If fetch operation of a memory region is not implemented, it still uses the
> > read operation for fetch.
> 
> This patch should probably be part of the series with the device that
> needs it.

I will add this patch to IOPMP patch series.

Thanks,
Ethan