From: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: "Peter Crosthwaite" <peter.crosthwaite@xilinx.com>,
patches@linaro.org, qemu-devel@nongnu.org,
"Greg Bellows" <greg.bellows@linaro.org>,
"Paolo Bonzini" <pbonzini@redhat.com>,
"Alex Bennée" <alex.bennee@linaro.org>,
"Richard Henderson" <rth@twiddle.net>
Subject: Re: [Qemu-devel] [PATCH v2 01/14] memory: Define API for MemoryRegionOps to take attrs and return status
Date: Wed, 15 Apr 2015 11:35:54 +1000 [thread overview]
Message-ID: <20150415013553.GC28944@toto> (raw)
In-Reply-To: <1428931324-4973-2-git-send-email-peter.maydell@linaro.org>
On Mon, Apr 13, 2015 at 02:21:51PM +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 <peter.maydell@linaro.org>
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> include/exec/memattrs.h | 41 ++++++++++
> include/exec/memory.h | 22 +++++
> memory.c | 207 ++++++++++++++++++++++++++++++++----------------
> 3 files changed, 203 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..1cb3fc0
> --- /dev/null
> +++ b/include/exec/memattrs.h
> @@ -0,0 +1,41 @@
> +/*
> + * Memory transaction attributes
> + *
> + * Copyright (c) 2015 Linaro Limited.
> + *
> + * Authors:
> + * Peter Maydell <peter.maydell@linaro.org>
> + *
> + * 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 bitfields in a single struct to avoid
> + * confusion if different parts of QEMU used the same bit for
> + * different semantics.
> + */
> +typedef struct MemTxAttrs {
> + /* Bus masters which don't specify any attributes will get this
> + * (via the MEMTXATTRS_UNSPECIFIED constant), so that we can
> + * distinguish "all attributes deliberately clear" from
> + * "didn't specify" if necessary.
> + */
> + unsigned int unspecified:1;
> +} MemTxAttrs;
> +
> +/* 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 ((MemTxAttrs) { .unspecified = 1 })
> +
> +#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
>
next prev parent reply other threads:[~2015-04-15 1:38 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-04-13 13:21 [Qemu-devel] [PATCH v2 00/14] Add memory attributes and use them in ARM Peter Maydell
2015-04-13 13:21 ` [Qemu-devel] [PATCH v2 01/14] memory: Define API for MemoryRegionOps to take attrs and return status Peter Maydell
2015-04-15 1:35 ` Edgar E. Iglesias [this message]
2015-04-17 16:00 ` Alex Bennée
2015-04-13 13:21 ` [Qemu-devel] [PATCH v2 02/14] memory: Replace io_mem_read/write with memory_region_dispatch_read/write Peter Maydell
2015-04-17 16:01 ` Alex Bennée
2015-04-13 13:21 ` [Qemu-devel] [PATCH v2 03/14] Make CPU iotlb a structure rather than a plain hwaddr Peter Maydell
2015-04-17 16:08 ` Alex Bennée
2015-04-13 13:21 ` [Qemu-devel] [PATCH v2 04/14] Add MemTxAttrs to the IOTLB Peter Maydell
2015-04-17 16:09 ` Alex Bennée
2015-04-13 13:21 ` [Qemu-devel] [PATCH v2 05/14] exec.c: Convert subpage memory ops to _with_attrs Peter Maydell
2015-04-17 16:15 ` Alex Bennée
2015-04-17 16:18 ` Peter Maydell
2015-04-17 16:25 ` Alex Bennée
2015-04-13 13:21 ` [Qemu-devel] [PATCH v2 06/14] exec.c: Make address_space_rw take transaction attributes Peter Maydell
2015-04-21 7:39 ` Alex Bennée
2015-04-21 13:27 ` Peter Maydell
2015-04-13 13:21 ` [Qemu-devel] [PATCH v2 07/14] exec.c: Add new address_space_ld*/st* functions Peter Maydell
2015-04-21 8:36 ` Alex Bennée
2015-04-13 13:21 ` [Qemu-devel] [PATCH v2 08/14] exec.c: Capture the memory attributes for a watchpoint hit Peter Maydell
2015-04-21 8:42 ` Alex Bennée
2015-04-13 13:21 ` [Qemu-devel] [PATCH v2 09/14] Switch non-CPU callers from ld/st*_phys to address_space_ld/st* Peter Maydell
2015-04-21 8:44 ` Alex Bennée
2015-04-13 13:22 ` [Qemu-devel] [PATCH v2 10/14] target-arm: Honour NS bits in page tables Peter Maydell
2015-04-21 9:24 ` Alex Bennée
2015-04-21 13:28 ` Peter Maydell
2015-04-13 13:22 ` [Qemu-devel] [PATCH v2 11/14] target-arm: Use correct memory attributes for page table walks Peter Maydell
2015-04-21 9:36 ` Alex Bennée
2015-04-13 13:22 ` [Qemu-devel] [PATCH v2 12/14] target-arm: Add user-mode transaction attribute Peter Maydell
2015-04-21 9:36 ` Alex Bennée
2015-04-13 13:22 ` [Qemu-devel] [PATCH v2 13/14] target-arm: Use attribute info to handle user-only watchpoints Peter Maydell
2015-04-21 9:37 ` Alex Bennée
2015-04-13 13:22 ` [Qemu-devel] [PATCH v2 14/14] target-arm: Check watchpoints against CPU security state Peter Maydell
2015-04-21 9:37 ` Alex Bennée
2015-04-21 13:35 ` [Qemu-devel] [PATCH v2 00/14] Add memory attributes and use them in ARM Peter Maydell
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150415013553.GC28944@toto \
--to=edgar.iglesias@gmail.com \
--cc=alex.bennee@linaro.org \
--cc=greg.bellows@linaro.org \
--cc=patches@linaro.org \
--cc=pbonzini@redhat.com \
--cc=peter.crosthwaite@xilinx.com \
--cc=peter.maydell@linaro.org \
--cc=qemu-devel@nongnu.org \
--cc=rth@twiddle.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.