[RFC PATCH] memory: Introduce memory region fetch operation

Ethan Chen via posted 1 patch 5 months, 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20240612090132.3522566-1-ethan84@andestech.com
Maintainers: Richard Henderson <richard.henderson@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>, Peter Xu <peterx@redhat.com>, David Hildenbrand <david@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>
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 5 months, 2 weeks 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 5 months, 2 weeks 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 5 months, 2 weeks 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