[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 01/14] memory: Define API for MemoryRegionOps to
From: |
Edgar E. Iglesias |
Subject: |
Re: [Qemu-devel] [PATCH 01/14] memory: Define API for MemoryRegionOps to take attrs and return status |
Date: |
Thu, 9 Apr 2015 18:55:37 +1000 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Tue, Apr 07, 2015 at 09:09:47PM +0100, Peter Maydell wrote:
> Define an API so that devices can register MemoryRegionOps whose read
> and write callback functions are passed an arbitrary pointer to some
> transaction attributes and can return a success-or-failure status code.
> This will allow us to model devices which:
> * behave differently for ARM Secure/NonSecure memory accesses
> * behave differently for privileged/unprivileged accesses
> * may return a transaction failure (causing a guest exception)
> for erroneous accesses
>
> This patch defines the new API and plumbs the attributes parameter through
> to the memory.c public level functions io_mem_read() and io_mem_write(),
> where it is currently dummied out.
>
> The success/failure response indication is also propagated out to
> io_mem_read() and io_mem_write(), which retain the old-style
> boolean true-for-error return.
>
> Signed-off-by: Peter Maydell <address@hidden>
> ---
> include/exec/memattrs.h | 34 ++++++++
> include/exec/memory.h | 22 +++++
> memory.c | 207
> ++++++++++++++++++++++++++++++++----------------
> 3 files changed, 196 insertions(+), 67 deletions(-)
> create mode 100644 include/exec/memattrs.h
>
> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
> new file mode 100644
> index 0000000..b8d7808
> --- /dev/null
> +++ b/include/exec/memattrs.h
> @@ -0,0 +1,34 @@
> +/*
> + * Memory transaction attributes
> + *
> + * Copyright (c) 2015 Linaro Limited.
> + *
> + * Authors:
> + * Peter Maydell <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#ifndef MEMATTRS_H
> +#define MEMATTRS_H
> +
> +/* Every memory transaction has associated with it a set of
> + * attributes. Some of these are generic (such as the ID of
> + * the bus master); some are specific to a particular kind of
> + * bus (such as the ARM Secure/NonSecure bit). We define them
> + * all as non-overlapping bits in a single integer to avoid
> + * confusion if different parts of QEMU used the same bit for
> + * different semantics.
> + */
> +typedef uint64_t MemTxAttrs;
Hi,
Did you consider using a struct here?
e.g:
typedef struct MemTxAttrs {
unsigned int secure : 1;
unsigned int master_id : 10;
unsigned int etc : 1;
} MemTxAttrs;
I think you could still pass it by value and my understanding is
that the compiler will generate similar code.
I find it more readable, you ca go:
attrs.secure = 1;
attrs.master_id = 0x77;
if (!attrs.secure)
instead of:
attrs |= MEMTXATTRS_SECURE
if (!(attrs & MEMTXATTRS_SECURE))
etc...
Or do you see any disadvantages with this?
> +
> +/* Bus masters which don't specify any attributes will get this,
> + * which has all attribute bits clear except the topmost one
> + * (so that we can distinguish "all attributes deliberately clear"
> + * from "didn't specify" if necessary).
> + */
> +#define MEMTXATTRS_UNSPECIFIED (1ULL << 63)
> +
> +#endif
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 06ffa1d..703d9e5 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -28,6 +28,7 @@
> #ifndef CONFIG_USER_ONLY
> #include "exec/hwaddr.h"
> #endif
> +#include "exec/memattrs.h"
> #include "qemu/queue.h"
> #include "qemu/int128.h"
> #include "qemu/notify.h"
> @@ -68,6 +69,16 @@ struct IOMMUTLBEntry {
> IOMMUAccessFlags perm;
> };
>
> +/* New-style MMIO accessors can indicate that the transaction failed.
> + * A zero (MEMTX_OK) response means success; anything else is a failure
> + * of some kind. The memory subsystem will bitwise-OR together results
> + * if it is synthesizing an operation from multiple smaller accesses.
> + */
> +#define MEMTX_OK 0
> +#define MEMTX_ERROR (1U << 0) /* device returned an error */
> +#define MEMTX_DECODE_ERROR (1U << 1) /* nothing at that address */
> +typedef uint32_t MemTxResult;
> +
> /*
> * Memory region callbacks
> */
> @@ -84,6 +95,17 @@ struct MemoryRegionOps {
> uint64_t data,
> unsigned size);
>
> + MemTxResult (*read_with_attrs)(void *opaque,
> + hwaddr addr,
> + uint64_t *data,
> + unsigned size,
> + MemTxAttrs attrs);
> + MemTxResult (*write_with_attrs)(void *opaque,
> + hwaddr addr,
> + uint64_t data,
> + unsigned size,
> + MemTxAttrs attrs);
> +
> enum device_endian endianness;
> /* Guest-visible constraints: */
> struct {
> diff --git a/memory.c b/memory.c
> index ee3f2a8..9bb5674 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -368,57 +368,84 @@ static void adjust_endianness(MemoryRegion *mr,
> uint64_t *data, unsigned size)
> }
> }
>
> -static void memory_region_oldmmio_read_accessor(MemoryRegion *mr,
> +static MemTxResult memory_region_oldmmio_read_accessor(MemoryRegion *mr,
> + hwaddr addr,
> + uint64_t *value,
> + unsigned size,
> + unsigned shift,
> + uint64_t mask,
> + MemTxAttrs attrs)
> +{
> + uint64_t tmp;
> +
> + tmp = mr->ops->old_mmio.read[ctz32(size)](mr->opaque, addr);
> + trace_memory_region_ops_read(mr, addr, tmp, size);
> + *value |= (tmp & mask) << shift;
> + return MEMTX_OK;
> +}
> +
> +static MemTxResult memory_region_read_accessor(MemoryRegion *mr,
> hwaddr addr,
> uint64_t *value,
> unsigned size,
> unsigned shift,
> - uint64_t mask)
> + uint64_t mask,
> + MemTxAttrs attrs)
> {
> uint64_t tmp;
>
> - tmp = mr->ops->old_mmio.read[ctz32(size)](mr->opaque, addr);
> + if (mr->flush_coalesced_mmio) {
> + qemu_flush_coalesced_mmio_buffer();
> + }
> + tmp = mr->ops->read(mr->opaque, addr, size);
> trace_memory_region_ops_read(mr, addr, tmp, size);
> *value |= (tmp & mask) << shift;
> + return MEMTX_OK;
> }
>
> -static void memory_region_read_accessor(MemoryRegion *mr,
> - hwaddr addr,
> - uint64_t *value,
> - unsigned size,
> - unsigned shift,
> - uint64_t mask)
> +static MemTxResult memory_region_read_with_attrs_accessor(MemoryRegion *mr,
> + hwaddr addr,
> + uint64_t *value,
> + unsigned size,
> + unsigned shift,
> + uint64_t mask,
> + MemTxAttrs attrs)
> {
> - uint64_t tmp;
> + uint64_t tmp = 0;
> + MemTxResult r;
>
> if (mr->flush_coalesced_mmio) {
> qemu_flush_coalesced_mmio_buffer();
> }
> - tmp = mr->ops->read(mr->opaque, addr, size);
> + r = mr->ops->read_with_attrs(mr->opaque, addr, &tmp, size, attrs);
> trace_memory_region_ops_read(mr, addr, tmp, size);
> *value |= (tmp & mask) << shift;
> + return r;
> }
>
> -static void memory_region_oldmmio_write_accessor(MemoryRegion *mr,
> - hwaddr addr,
> - uint64_t *value,
> - unsigned size,
> - unsigned shift,
> - uint64_t mask)
> +static MemTxResult memory_region_oldmmio_write_accessor(MemoryRegion *mr,
> + hwaddr addr,
> + uint64_t *value,
> + unsigned size,
> + unsigned shift,
> + uint64_t mask,
> + MemTxAttrs attrs)
> {
> uint64_t tmp;
>
> tmp = (*value >> shift) & mask;
> trace_memory_region_ops_write(mr, addr, tmp, size);
> mr->ops->old_mmio.write[ctz32(size)](mr->opaque, addr, tmp);
> + return MEMTX_OK;
> }
>
> -static void memory_region_write_accessor(MemoryRegion *mr,
> - hwaddr addr,
> - uint64_t *value,
> - unsigned size,
> - unsigned shift,
> - uint64_t mask)
> +static MemTxResult memory_region_write_accessor(MemoryRegion *mr,
> + hwaddr addr,
> + uint64_t *value,
> + unsigned size,
> + unsigned shift,
> + uint64_t mask,
> + MemTxAttrs attrs)
> {
> uint64_t tmp;
>
> @@ -428,24 +455,46 @@ static void memory_region_write_accessor(MemoryRegion
> *mr,
> tmp = (*value >> shift) & mask;
> trace_memory_region_ops_write(mr, addr, tmp, size);
> mr->ops->write(mr->opaque, addr, tmp, size);
> + return MEMTX_OK;
> }
>
> -static void access_with_adjusted_size(hwaddr addr,
> +static MemTxResult memory_region_write_with_attrs_accessor(MemoryRegion *mr,
> + hwaddr addr,
> + uint64_t *value,
> + unsigned size,
> + unsigned shift,
> + uint64_t mask,
> + MemTxAttrs attrs)
> +{
> + uint64_t tmp;
> +
> + if (mr->flush_coalesced_mmio) {
> + qemu_flush_coalesced_mmio_buffer();
> + }
> + tmp = (*value >> shift) & mask;
> + trace_memory_region_ops_write(mr, addr, tmp, size);
> + return mr->ops->write_with_attrs(mr->opaque, addr, tmp, size, attrs);
> +}
> +
> +static MemTxResult access_with_adjusted_size(hwaddr addr,
> uint64_t *value,
> unsigned size,
> unsigned access_size_min,
> unsigned access_size_max,
> - void (*access)(MemoryRegion *mr,
> - hwaddr addr,
> - uint64_t *value,
> - unsigned size,
> - unsigned shift,
> - uint64_t mask),
> - MemoryRegion *mr)
> + MemTxResult (*access)(MemoryRegion *mr,
> + hwaddr addr,
> + uint64_t *value,
> + unsigned size,
> + unsigned shift,
> + uint64_t mask,
> + MemTxAttrs
> attrs),
> + MemoryRegion *mr,
> + MemTxAttrs attrs)
> {
> uint64_t access_mask;
> unsigned access_size;
> unsigned i;
> + MemTxResult r = MEMTX_OK;
>
> if (!access_size_min) {
> access_size_min = 1;
> @@ -459,14 +508,16 @@ static void access_with_adjusted_size(hwaddr addr,
> access_mask = -1ULL >> (64 - access_size * 8);
> if (memory_region_big_endian(mr)) {
> for (i = 0; i < size; i += access_size) {
> - access(mr, addr + i, value, access_size,
> - (size - access_size - i) * 8, access_mask);
> + r |= access(mr, addr + i, value, access_size,
> + (size - access_size - i) * 8, access_mask, attrs);
> }
> } else {
> for (i = 0; i < size; i += access_size) {
> - access(mr, addr + i, value, access_size, i * 8, access_mask);
> + r |= access(mr, addr + i, value, access_size, i * 8,
> + access_mask, attrs);
> }
> }
> + return r;
> }
>
> static AddressSpace *memory_region_to_address_space(MemoryRegion *mr)
> @@ -1053,62 +1104,82 @@ bool memory_region_access_valid(MemoryRegion *mr,
> return true;
> }
>
> -static uint64_t memory_region_dispatch_read1(MemoryRegion *mr,
> - hwaddr addr,
> - unsigned size)
> +static MemTxResult memory_region_dispatch_read1(MemoryRegion *mr,
> + hwaddr addr,
> + uint64_t *pval,
> + unsigned size,
> + MemTxAttrs attrs)
> {
> - uint64_t data = 0;
> + *pval = 0;
>
> if (mr->ops->read) {
> - access_with_adjusted_size(addr, &data, size,
> - mr->ops->impl.min_access_size,
> - mr->ops->impl.max_access_size,
> - memory_region_read_accessor, mr);
> + 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 if (mr->ops->read_with_attrs) {
> + 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);
> } else {
> - access_with_adjusted_size(addr, &data, size, 1, 4,
> - memory_region_oldmmio_read_accessor, mr);
> + return access_with_adjusted_size(addr, pval, size, 1, 4,
> + memory_region_oldmmio_read_accessor,
> + mr, attrs);
> }
> -
> - return data;
> }
>
> -static bool memory_region_dispatch_read(MemoryRegion *mr,
> - hwaddr addr,
> - uint64_t *pval,
> - unsigned size)
> +static MemTxResult memory_region_dispatch_read(MemoryRegion *mr,
> + hwaddr addr,
> + uint64_t *pval,
> + unsigned size,
> + MemTxAttrs attrs)
> {
> + MemTxResult r;
> +
> if (!memory_region_access_valid(mr, addr, size, false)) {
> *pval = unassigned_mem_read(mr, addr, size);
> - return true;
> + return MEMTX_DECODE_ERROR;
> }
>
> - *pval = memory_region_dispatch_read1(mr, addr, size);
> + r = memory_region_dispatch_read1(mr, addr, pval, size, attrs);
> adjust_endianness(mr, pval, size);
> - return false;
> + return r;
> }
>
> -static bool memory_region_dispatch_write(MemoryRegion *mr,
> - hwaddr addr,
> - uint64_t data,
> - unsigned size)
> +static MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
> + hwaddr addr,
> + uint64_t data,
> + unsigned size,
> + MemTxAttrs attrs)
> {
> if (!memory_region_access_valid(mr, addr, size, true)) {
> unassigned_mem_write(mr, addr, data, size);
> - return true;
> + return MEMTX_DECODE_ERROR;
> }
>
> adjust_endianness(mr, &data, size);
>
> if (mr->ops->write) {
> - access_with_adjusted_size(addr, &data, size,
> - mr->ops->impl.min_access_size,
> - mr->ops->impl.max_access_size,
> - memory_region_write_accessor, mr);
> + return access_with_adjusted_size(addr, &data, size,
> + mr->ops->impl.min_access_size,
> + mr->ops->impl.max_access_size,
> + memory_region_write_accessor, mr,
> + attrs);
> + } else if (mr->ops->write_with_attrs) {
> + return
> + access_with_adjusted_size(addr, &data, size,
> + mr->ops->impl.min_access_size,
> + mr->ops->impl.max_access_size,
> +
> memory_region_write_with_attrs_accessor,
> + mr, attrs);
> } else {
> - access_with_adjusted_size(addr, &data, size, 1, 4,
> - memory_region_oldmmio_write_accessor, mr);
> + return access_with_adjusted_size(addr, &data, size, 1, 4,
> +
> memory_region_oldmmio_write_accessor,
> + mr, attrs);
> }
> - return false;
> }
>
> void memory_region_init_io(MemoryRegion *mr,
> @@ -1994,13 +2065,15 @@ void address_space_destroy(AddressSpace *as)
>
> bool io_mem_read(MemoryRegion *mr, hwaddr addr, uint64_t *pval, unsigned
> size)
> {
> - return memory_region_dispatch_read(mr, addr, pval, size);
> + return memory_region_dispatch_read(mr, addr, pval, size,
> + MEMTXATTRS_UNSPECIFIED);
> }
>
> bool io_mem_write(MemoryRegion *mr, hwaddr addr,
> uint64_t val, unsigned size)
> {
> - return memory_region_dispatch_write(mr, addr, val, size);
> + return memory_region_dispatch_write(mr, addr, val, size,
> + MEMTXATTRS_UNSPECIFIED);
> }
>
> typedef struct MemoryRegionList MemoryRegionList;
> --
> 1.9.1
>
- Re: [Qemu-devel] [PATCH 07/14] exec.c: Add new address_space_ld*/st* functions, (continued)
Re: [Qemu-devel] [PATCH 07/14] exec.c: Add new address_space_ld*/st* functions, Edgar E. Iglesias, 2015/04/09
[Qemu-devel] [PATCH 14/14] target-arm: Check watchpoints against CPU security state, Peter Maydell, 2015/04/07
[Qemu-devel] [PATCH 01/14] memory: Define API for MemoryRegionOps to take attrs and return status, Peter Maydell, 2015/04/07
- Re: [Qemu-devel] [PATCH 01/14] memory: Define API for MemoryRegionOps to take attrs and return status, Paolo Bonzini, 2015/04/08
- Re: [Qemu-devel] [PATCH 01/14] memory: Define API for MemoryRegionOps to take attrs and return status,
Edgar E. Iglesias <=
- Re: [Qemu-devel] [PATCH 01/14] memory: Define API for MemoryRegionOps to take attrs and return status, Peter Maydell, 2015/04/09
- Re: [Qemu-devel] [PATCH 01/14] memory: Define API for MemoryRegionOps to take attrs and return status, Paolo Bonzini, 2015/04/09
- Re: [Qemu-devel] [PATCH 01/14] memory: Define API for MemoryRegionOps to take attrs and return status, Edgar E. Iglesias, 2015/04/09
- Re: [Qemu-devel] [PATCH 01/14] memory: Define API for MemoryRegionOps to take attrs and return status, Peter Maydell, 2015/04/10
- Re: [Qemu-devel] [PATCH 01/14] memory: Define API for MemoryRegionOps to take attrs and return status, Edgar E. Iglesias, 2015/04/11
Re: [Qemu-devel] [PATCH 01/14] memory: Define API for MemoryRegionOps to take attrs and return status, Edgar E. Iglesias, 2015/04/09
[Qemu-devel] [PATCH 05/14] exec.c: Convert subpage memory ops to _with_attrs, Peter Maydell, 2015/04/07
[Qemu-devel] [PATCH 08/14] Switch non-CPU callers from ld/st*_phys to address_space_ld/st*, Peter Maydell, 2015/04/07