On Wed, Jan 22, 2025 at 6:39 PM Ethan Chen via <qemu-devel@nongnu.org> wrote:
>
> Allow memory regions to have different behaviors for read and fetch
> operations.
>
> For example, the RISC-V IOPMP could raise an interrupt when the CPU
> tries to fetch from a non-executable region.
>
> If the fetch operation for a memory region is not implemented, the read
> operation will still be used for fetch operations.
>
> Signed-off-by: Ethan Chen <etha...@andestech.com>
This looks ok to me, but I would like someone who knows this better to
review it as well
Acked-by: Alistair Francis <alistair.fran...@wdc.com>
Alistair
> ---
> accel/tcg/cputlb.c | 9 +++-
> include/exec/memory.h | 27 +++++++++++
> system/memory.c | 104 ++++++++++++++++++++++++++++++++++++++++++
> system/trace-events | 2 +
> 4 files changed, 140 insertions(+), 2 deletions(-)
>
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index b4ccf0cdcb..71c16a1ac1 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -1947,8 +1947,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 3ee1901b52..6166d697d9 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -273,6 +273,11 @@ struct MemoryRegionOps {
> hwaddr addr,
> uint64_t data,
> unsigned size);
> + /* Fetch from 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,
> @@ -284,6 +289,11 @@ 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: */
> @@ -2604,6 +2614,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 b17b5538ff..7f26f681f9 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,
> @@ -1493,6 +1538,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,
> diff --git a/system/trace-events b/system/trace-events
> index 5bbc3fbffa..4e78bb515b 100644
> --- a/system/trace-events
> +++ b/system/trace-events
> @@ -18,8 +18,10 @@ cpu_out(unsigned int addr, char size, unsigned int val)
> "addr 0x%x(%c) value %u"
> # memory.c
> memory_region_ops_read(int cpu_index, void *mr, uint64_t addr, uint64_t
> value, unsigned size, const char *name) "cpu %d mr %p addr 0x%"PRIx64" value
> 0x%"PRIx64" size %u name '%s'"
> memory_region_ops_write(int cpu_index, void *mr, uint64_t addr, uint64_t
> value, unsigned size, const char *name) "cpu %d mr %p addr 0x%"PRIx64" value
> 0x%"PRIx64" size %u name '%s'"
> +memory_region_ops_fetch(int cpu_index, void *mr, uint64_t addr, uint64_t
> value, unsigned size, const char *name) "cpu %d mr %p addr 0x%"PRIx64" value
> 0x%"PRIx64" size %u name '%s'"
> memory_region_subpage_read(int cpu_index, void *mr, uint64_t offset,
> uint64_t value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value
> 0x%"PRIx64" size %u"
> memory_region_subpage_write(int cpu_index, void *mr, uint64_t offset,
> uint64_t value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value
> 0x%"PRIx64" size %u"
> +memory_region_subpage_fetch(int cpu_index, void *mr, uint64_t offset,
> uint64_t value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value
> 0x%"PRIx64" size %u"
> memory_region_ram_device_read(int cpu_index, void *mr, uint64_t addr,
> uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value
> 0x%"PRIx64" size %u"
> memory_region_ram_device_write(int cpu_index, void *mr, uint64_t addr,
> uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value
> 0x%"PRIx64" size %u"
> memory_region_sync_dirty(const char *mr, const char *listener, int global)
> "mr '%s' listener '%s' synced (global=%d)"
> --
> 2.34.1
>
>
