From: "Alex Bennée" <alex.bennee@linaro.org>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: Peter Crosthwaite <peter.crosthwaite@xilinx.com>,
patches@linaro.org,
"Edgar E. Iglesias" <edgar.iglesias@gmail.com>,
qemu-devel@nongnu.org, Greg Bellows <greg.bellows@linaro.org>,
Paolo Bonzini <pbonzini@redhat.com>,
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: Fri, 17 Apr 2015 17:00:07 +0100 [thread overview]
Message-ID: <87oammg2yg.fsf@linaro.org> (raw)
In-Reply-To: <1428931324-4973-2-git-send-email-peter.maydell@linaro.org>
Peter Maydell <peter.maydell@linaro.org> writes:
> 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>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
git format-patch was doing its best to confuse me by mixing up what
were new functions and changes to existing ones. Anyway after reviewing
it in the final file:
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> 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;
--
Alex Bennée
next prev parent reply other threads:[~2015-04-17 15:59 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
2015-04-17 16:00 ` Alex Bennée [this message]
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=87oammg2yg.fsf@linaro.org \
--to=alex.bennee@linaro.org \
--cc=edgar.iglesias@gmail.com \
--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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).