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 02/14] memory: Replace io_mem_read/write with memory_region_dispatch_read/write
Date: Fri, 17 Apr 2015 17:01:37 +0100 [thread overview]
Message-ID: <87mw26g2vy.fsf@linaro.org> (raw)
In-Reply-To: <1428931324-4973-3-git-send-email-peter.maydell@linaro.org>
Peter Maydell <peter.maydell@linaro.org> writes:
> Rather than retaining io_mem_read/write as simple wrappers around
> the memory_region_dispatch_read/write functions, make the latter
> public and change all the callers to use them, since we need to
> touch all the callsites anyway to add MemTxAttrs and MemTxResult
> support. Delete io_mem_read and io_mem_write entirely.
>
> (All the callers currently pass MEMTXATTRS_UNSPECIFIED
> and convert the return value back to bool or ignore it.)
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Well that was easier to follow ;-)
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> exec.c | 47 +++++++++++++++++++++++++++++++----------------
> hw/s390x/s390-pci-inst.c | 10 +++++++---
> hw/vfio/pci.c | 18 ++++++++++++------
> include/exec/exec-all.h | 4 ----
> include/exec/memory.h | 31 +++++++++++++++++++++++++++++++
> memory.c | 33 ++++++++++-----------------------
> softmmu_template.h | 6 ++++--
> 7 files changed, 95 insertions(+), 54 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 874ecfc..34dafd2 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2312,7 +2312,8 @@ bool address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
> uint64_t val;
> hwaddr addr1;
> MemoryRegion *mr;
> - bool error = false;
> + MemTxResult result = MEMTX_OK;
> + MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
>
> while (len > 0) {
> l = len;
> @@ -2327,22 +2328,26 @@ bool address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
> case 8:
> /* 64 bit write access */
> val = ldq_p(buf);
> - error |= io_mem_write(mr, addr1, val, 8);
> + result |= memory_region_dispatch_write(mr, addr1, val, 8,
> + attrs);
> break;
> case 4:
> /* 32 bit write access */
> val = ldl_p(buf);
> - error |= io_mem_write(mr, addr1, val, 4);
> + result |= memory_region_dispatch_write(mr, addr1, val, 4,
> + attrs);
> break;
> case 2:
> /* 16 bit write access */
> val = lduw_p(buf);
> - error |= io_mem_write(mr, addr1, val, 2);
> + result |= memory_region_dispatch_write(mr, addr1, val, 2,
> + attrs);
> break;
> case 1:
> /* 8 bit write access */
> val = ldub_p(buf);
> - error |= io_mem_write(mr, addr1, val, 1);
> + result |= memory_region_dispatch_write(mr, addr1, val, 1,
> + attrs);
> break;
> default:
> abort();
> @@ -2361,22 +2366,26 @@ bool address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
> switch (l) {
> case 8:
> /* 64 bit read access */
> - error |= io_mem_read(mr, addr1, &val, 8);
> + result |= memory_region_dispatch_read(mr, addr1, &val, 8,
> + attrs);
> stq_p(buf, val);
> break;
> case 4:
> /* 32 bit read access */
> - error |= io_mem_read(mr, addr1, &val, 4);
> + result |= memory_region_dispatch_read(mr, addr1, &val, 4,
> + attrs);
> stl_p(buf, val);
> break;
> case 2:
> /* 16 bit read access */
> - error |= io_mem_read(mr, addr1, &val, 2);
> + result |= memory_region_dispatch_read(mr, addr1, &val, 2,
> + attrs);
> stw_p(buf, val);
> break;
> case 1:
> /* 8 bit read access */
> - error |= io_mem_read(mr, addr1, &val, 1);
> + result |= memory_region_dispatch_read(mr, addr1, &val, 1,
> + attrs);
> stb_p(buf, val);
> break;
> default:
> @@ -2393,7 +2402,7 @@ bool address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
> addr += l;
> }
>
> - return error;
> + return result;
> }
>
> bool address_space_write(AddressSpace *as, hwaddr addr,
> @@ -2669,7 +2678,8 @@ static inline uint32_t ldl_phys_internal(AddressSpace *as, hwaddr addr,
> mr = address_space_translate(as, addr, &addr1, &l, false);
> if (l < 4 || !memory_access_is_direct(mr, false)) {
> /* I/O case */
> - io_mem_read(mr, addr1, &val, 4);
> + memory_region_dispatch_read(mr, addr1, &val, 4,
> + MEMTXATTRS_UNSPECIFIED);
> #if defined(TARGET_WORDS_BIGENDIAN)
> if (endian == DEVICE_LITTLE_ENDIAN) {
> val = bswap32(val);
> @@ -2728,7 +2738,8 @@ static inline uint64_t ldq_phys_internal(AddressSpace *as, hwaddr addr,
> false);
> if (l < 8 || !memory_access_is_direct(mr, false)) {
> /* I/O case */
> - io_mem_read(mr, addr1, &val, 8);
> + memory_region_dispatch_read(mr, addr1, &val, 8,
> + MEMTXATTRS_UNSPECIFIED);
> #if defined(TARGET_WORDS_BIGENDIAN)
> if (endian == DEVICE_LITTLE_ENDIAN) {
> val = bswap64(val);
> @@ -2795,7 +2806,8 @@ static inline uint32_t lduw_phys_internal(AddressSpace *as, hwaddr addr,
> false);
> if (l < 2 || !memory_access_is_direct(mr, false)) {
> /* I/O case */
> - io_mem_read(mr, addr1, &val, 2);
> + memory_region_dispatch_read(mr, addr1, &val, 2,
> + MEMTXATTRS_UNSPECIFIED);
> #if defined(TARGET_WORDS_BIGENDIAN)
> if (endian == DEVICE_LITTLE_ENDIAN) {
> val = bswap16(val);
> @@ -2853,7 +2865,8 @@ void stl_phys_notdirty(AddressSpace *as, hwaddr addr, uint32_t val)
> mr = address_space_translate(as, addr, &addr1, &l,
> true);
> if (l < 4 || !memory_access_is_direct(mr, true)) {
> - io_mem_write(mr, addr1, val, 4);
> + memory_region_dispatch_write(mr, addr1, val, 4,
> + MEMTXATTRS_UNSPECIFIED);
> } else {
> addr1 += memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK;
> ptr = qemu_get_ram_ptr(addr1);
> @@ -2892,7 +2905,8 @@ static inline void stl_phys_internal(AddressSpace *as,
> val = bswap32(val);
> }
> #endif
> - io_mem_write(mr, addr1, val, 4);
> + memory_region_dispatch_write(mr, addr1, val, 4,
> + MEMTXATTRS_UNSPECIFIED);
> } else {
> /* RAM case */
> addr1 += memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK;
> @@ -2955,7 +2969,8 @@ static inline void stw_phys_internal(AddressSpace *as,
> val = bswap16(val);
> }
> #endif
> - io_mem_write(mr, addr1, val, 2);
> + memory_region_dispatch_write(mr, addr1, val, 2,
> + MEMTXATTRS_UNSPECIFIED);
> } else {
> /* RAM case */
> addr1 += memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK;
> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index 08d8aa6..8f7288f 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -331,7 +331,8 @@ int pcilg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
> return 0;
> }
> MemoryRegion *mr = pbdev->pdev->io_regions[pcias].memory;
> - io_mem_read(mr, offset, &data, len);
> + memory_region_dispatch_read(mr, offset, &data, len,
> + MEMTXATTRS_UNSPECIFIED);
> } else if (pcias == 15) {
> if ((4 - (offset & 0x3)) < len) {
> program_interrupt(env, PGM_OPERAND, 4);
> @@ -456,7 +457,8 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
> mr = pbdev->pdev->io_regions[pcias].memory;
> }
>
> - io_mem_write(mr, offset, data, len);
> + memory_region_dispatch_write(mr, offset, data, len,
> + MEMTXATTRS_UNSPECIFIED);
> } else if (pcias == 15) {
> if ((4 - (offset & 0x3)) < len) {
> program_interrupt(env, PGM_OPERAND, 4);
> @@ -606,7 +608,9 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr)
> }
>
> for (i = 0; i < len / 8; i++) {
> - io_mem_write(mr, env->regs[r3] + i * 8, ldq_p(buffer + i * 8), 8);
> + memory_region_dispatch_write(mr, env->regs[r3] + i * 8,
> + ldq_p(buffer + i * 8), 8,
> + MEMTXATTRS_UNSPECIFIED);
> }
>
> setcc(cpu, ZPCI_PCI_LS_OK);
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 6b80539..cd15b20 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1531,9 +1531,12 @@ static uint64_t vfio_rtl8168_window_quirk_read(void *opaque,
> return 0;
> }
>
> - io_mem_read(&vdev->pdev.msix_table_mmio,
> - (hwaddr)(quirk->data.address_match & 0xfff),
> - &val, size);
> + memory_region_dispatch_read(&vdev->pdev.msix_table_mmio,
> + (hwaddr)(quirk->data.address_match
> + & 0xfff),
> + &val,
> + size,
> + MEMTXATTRS_UNSPECIFIED);
> return val;
> }
> }
> @@ -1561,9 +1564,12 @@ static void vfio_rtl8168_window_quirk_write(void *opaque, hwaddr addr,
> memory_region_name(&quirk->mem),
> vdev->vbasedev.name);
>
> - io_mem_write(&vdev->pdev.msix_table_mmio,
> - (hwaddr)(quirk->data.address_match & 0xfff),
> - data, size);
> + memory_region_dispatch_write(&vdev->pdev.msix_table_mmio,
> + (hwaddr)(quirk->data.address_match
> + & 0xfff),
> + data,
> + size,
> + MEMTXATTRS_UNSPECIFIED);
> }
>
> quirk->data.flags = 1;
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 8eb0db3..ff1bc3e 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -341,10 +341,6 @@ void phys_mem_set_alloc(void *(*alloc)(size_t, uint64_t *align));
>
> struct MemoryRegion *iotlb_to_region(CPUState *cpu,
> hwaddr index);
> -bool io_mem_read(struct MemoryRegion *mr, hwaddr addr,
> - uint64_t *pvalue, unsigned size);
> -bool io_mem_write(struct MemoryRegion *mr, hwaddr addr,
> - uint64_t value, unsigned size);
>
> void tlb_fill(CPUState *cpu, target_ulong addr, int is_write, int mmu_idx,
> uintptr_t retaddr);
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 703d9e5..970a3a9 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1053,6 +1053,37 @@ void memory_global_dirty_log_stop(void);
> void mtree_info(fprintf_function mon_printf, void *f);
>
> /**
> + * memory_region_dispatch_read: perform a read 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
> + * @size: size of the access in bytes
> + * @attrs: memory transaction attributes to use for the access
> + */
> +MemTxResult memory_region_dispatch_read(MemoryRegion *mr,
> + hwaddr addr,
> + uint64_t *pval,
> + unsigned size,
> + MemTxAttrs attrs);
> +/**
> + * memory_region_dispatch_write: perform a write directly to the specified
> + * MemoryRegion.
> + *
> + * @mr: #MemoryRegion to access
> + * @addr: address within that region
> + * @data: data to write
> + * @size: size of the access in bytes
> + * @attrs: memory transaction attributes to use for the access
> + */
> +MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
> + hwaddr addr,
> + uint64_t data,
> + unsigned size,
> + MemTxAttrs attrs);
> +
> +/**
> * address_space_init: initializes an address space
> *
> * @as: an uninitialized #AddressSpace
> diff --git a/memory.c b/memory.c
> index 9bb5674..a403c86 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1131,11 +1131,11 @@ static MemTxResult memory_region_dispatch_read1(MemoryRegion *mr,
> }
> }
>
> -static MemTxResult memory_region_dispatch_read(MemoryRegion *mr,
> - hwaddr addr,
> - uint64_t *pval,
> - unsigned size,
> - MemTxAttrs attrs)
> +MemTxResult memory_region_dispatch_read(MemoryRegion *mr,
> + hwaddr addr,
> + uint64_t *pval,
> + unsigned size,
> + MemTxAttrs attrs)
> {
> MemTxResult r;
>
> @@ -1149,11 +1149,11 @@ static MemTxResult memory_region_dispatch_read(MemoryRegion *mr,
> return r;
> }
>
> -static MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
> - hwaddr addr,
> - uint64_t data,
> - unsigned size,
> - MemTxAttrs attrs)
> +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);
> @@ -2063,19 +2063,6 @@ void address_space_destroy(AddressSpace *as)
> call_rcu(as, do_address_space_destroy, rcu);
> }
>
> -bool io_mem_read(MemoryRegion *mr, hwaddr addr, uint64_t *pval, unsigned 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,
> - MEMTXATTRS_UNSPECIFIED);
> -}
> -
> typedef struct MemoryRegionList MemoryRegionList;
>
> struct MemoryRegionList {
> diff --git a/softmmu_template.h b/softmmu_template.h
> index 0e3dd35..9c1d53e 100644
> --- a/softmmu_template.h
> +++ b/softmmu_template.h
> @@ -158,7 +158,8 @@ static inline DATA_TYPE glue(io_read, SUFFIX)(CPUArchState *env,
> }
>
> cpu->mem_io_vaddr = addr;
> - io_mem_read(mr, physaddr, &val, 1 << SHIFT);
> + memory_region_dispatch_read(mr, physaddr, &val, 1 << SHIFT,
> + MEMTXATTRS_UNSPECIFIED);
> return val;
> }
> #endif
> @@ -378,7 +379,8 @@ static inline void glue(io_write, SUFFIX)(CPUArchState *env,
>
> cpu->mem_io_vaddr = addr;
> cpu->mem_io_pc = retaddr;
> - io_mem_write(mr, physaddr, val, 1 << SHIFT);
> + memory_region_dispatch_write(mr, physaddr, val, 1 << SHIFT,
> + MEMTXATTRS_UNSPECIFIED);
> }
>
> void helper_le_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val,
--
Alex Bennée
next prev parent reply other threads:[~2015-04-17 16:01 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
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 [this message]
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=87mw26g2vy.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).