* Re: [Qemu-devel] [PATCH 1/9] Avoid needless calls to address_space_rw()
[not found] ` <1467934423-5997-2-git-send-email-andrew.smirnov@gmail.com>
@ 2016-07-08 3:27 ` David Gibson
0 siblings, 0 replies; 20+ messages in thread
From: David Gibson @ 2016-07-08 3:27 UTC (permalink / raw)
To: Andrey Smirnov; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 11307 bytes --]
On Thu, Jul 07, 2016 at 04:33:35PM -0700, Andrey Smirnov wrote:
> Avoid calling address_space_rw() when direction of the transfer is
> constant and known at compile time and replace them with explicit calls
> to address_space_read()/address_space_write().
>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> dma-helpers.c | 4 ++--
> exec.c | 10 +++++-----
> hw/net/dp8393x.c | 56 ++++++++++++++++++++++++++++----------------------------
> 3 files changed, 35 insertions(+), 35 deletions(-)
>
> diff --git a/dma-helpers.c b/dma-helpers.c
> index b521d84..96a6d6e 100644
> --- a/dma-helpers.c
> +++ b/dma-helpers.c
> @@ -28,8 +28,8 @@ int dma_memory_set(AddressSpace *as, dma_addr_t addr, uint8_t c, dma_addr_t len)
> memset(fillbuf, c, FILLBUF_SIZE);
> while (len > 0) {
> l = len < FILLBUF_SIZE ? len : FILLBUF_SIZE;
> - error |= address_space_rw(as, addr, MEMTXATTRS_UNSPECIFIED,
> - fillbuf, l, true);
> + error |= address_space_write(as, addr, MEMTXATTRS_UNSPECIFIED,
> + fillbuf, l);
> len -= l;
> addr += l;
> }
> diff --git a/exec.c b/exec.c
> index 0122ef7..93611f1 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -3224,7 +3224,7 @@ uint32_t address_space_ldub(AddressSpace *as, hwaddr addr,
> uint8_t val;
> MemTxResult r;
>
> - r = address_space_rw(as, addr, attrs, &val, 1, 0);
> + r = address_space_read(as, addr, attrs, &val, 1);
> if (result) {
> *result = r;
> }
> @@ -3474,7 +3474,7 @@ void address_space_stb(AddressSpace *as, hwaddr addr, uint32_t val,
> uint8_t v = val;
> MemTxResult r;
>
> - r = address_space_rw(as, addr, attrs, &v, 1, 1);
> + r = address_space_write(as, addr, attrs, &v, 1);
> if (result) {
> *result = r;
> }
> @@ -3582,7 +3582,7 @@ void address_space_stq(AddressSpace *as, hwaddr addr, uint64_t val,
> {
> MemTxResult r;
> val = tswap64(val);
> - r = address_space_rw(as, addr, attrs, (void *) &val, 8, 1);
> + r = address_space_write(as, addr, attrs, (void *) &val, 8);
> if (result) {
> *result = r;
> }
> @@ -3593,7 +3593,7 @@ void address_space_stq_le(AddressSpace *as, hwaddr addr, uint64_t val,
> {
> MemTxResult r;
> val = cpu_to_le64(val);
> - r = address_space_rw(as, addr, attrs, (void *) &val, 8, 1);
> + r = address_space_write(as, addr, attrs, (void *) &val, 8);
> if (result) {
> *result = r;
> }
> @@ -3603,7 +3603,7 @@ void address_space_stq_be(AddressSpace *as, hwaddr addr, uint64_t val,
> {
> MemTxResult r;
> val = cpu_to_be64(val);
> - r = address_space_rw(as, addr, attrs, (void *) &val, 8, 1);
> + r = address_space_write(as, addr, attrs, (void *) &val, 8);
> if (result) {
> *result = r;
> }
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 0fa652c..2849542 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -203,9 +203,9 @@ static void dp8393x_do_load_cam(dp8393xState *s)
>
> while (s->regs[SONIC_CDC] & 0x1f) {
> /* Fill current entry */
> - address_space_rw(&s->as,
> + address_space_read(&s->as,
> (s->regs[SONIC_URRA] << 16) | s->regs[SONIC_CDP],
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size, 0);
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size);
> s->cam[index][0] = data[1 * width] & 0xff;
> s->cam[index][1] = data[1 * width] >> 8;
> s->cam[index][2] = data[2 * width] & 0xff;
> @@ -222,9 +222,9 @@ static void dp8393x_do_load_cam(dp8393xState *s)
> }
>
> /* Read CAM enable */
> - address_space_rw(&s->as,
> + address_space_read(&s->as,
> (s->regs[SONIC_URRA] << 16) | s->regs[SONIC_CDP],
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size, 0);
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size);
> s->regs[SONIC_CE] = data[0 * width];
> DPRINTF("load cam done. cam enable mask 0x%04x\n", s->regs[SONIC_CE]);
>
> @@ -242,9 +242,9 @@ static void dp8393x_do_read_rra(dp8393xState *s)
> /* Read memory */
> width = (s->regs[SONIC_DCR] & SONIC_DCR_DW) ? 2 : 1;
> size = sizeof(uint16_t) * 4 * width;
> - address_space_rw(&s->as,
> + address_space_read(&s->as,
> (s->regs[SONIC_URRA] << 16) | s->regs[SONIC_RRP],
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size, 0);
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size);
>
> /* Update SONIC registers */
> s->regs[SONIC_CRBA0] = data[0 * width];
> @@ -360,9 +360,9 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
> (s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_CTDA]);
> size = sizeof(uint16_t) * 6 * width;
> s->regs[SONIC_TTDA] = s->regs[SONIC_CTDA];
> - address_space_rw(&s->as,
> + address_space_read(&s->as,
> ((s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_TTDA]) + sizeof(uint16_t) * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size, 0);
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size);
> tx_len = 0;
>
> /* Update registers */
> @@ -386,18 +386,18 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
> if (tx_len + len > sizeof(s->tx_buffer)) {
> len = sizeof(s->tx_buffer) - tx_len;
> }
> - address_space_rw(&s->as,
> + address_space_read(&s->as,
> (s->regs[SONIC_TSA1] << 16) | s->regs[SONIC_TSA0],
> - MEMTXATTRS_UNSPECIFIED, &s->tx_buffer[tx_len], len, 0);
> + MEMTXATTRS_UNSPECIFIED, &s->tx_buffer[tx_len], len);
> tx_len += len;
>
> i++;
> if (i != s->regs[SONIC_TFC]) {
> /* Read next fragment details */
> size = sizeof(uint16_t) * 3 * width;
> - address_space_rw(&s->as,
> + address_space_read(&s->as,
> ((s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_TTDA]) + sizeof(uint16_t) * (4 + 3 * i) * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size, 0);
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size);
> s->regs[SONIC_TSA0] = data[0 * width];
> s->regs[SONIC_TSA1] = data[1 * width];
> s->regs[SONIC_TFS] = data[2 * width];
> @@ -429,16 +429,16 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
> /* Write status */
> data[0 * width] = s->regs[SONIC_TCR] & 0x0fff; /* status */
> size = sizeof(uint16_t) * width;
> - address_space_rw(&s->as,
> + address_space_write(&s->as,
> (s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_TTDA],
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size, 1);
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size);
>
> if (!(s->regs[SONIC_CR] & SONIC_CR_HTX)) {
> /* Read footer of packet */
> size = sizeof(uint16_t) * width;
> - address_space_rw(&s->as,
> + address_space_read(&s->as,
> ((s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_TTDA]) + sizeof(uint16_t) * (4 + 3 * s->regs[SONIC_TFC]) * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size, 0);
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size);
> s->regs[SONIC_CTDA] = data[0 * width] & ~0x1;
> if (data[0 * width] & 0x1) {
> /* EOL detected */
> @@ -701,8 +701,8 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> /* Are we still in resource exhaustion? */
> size = sizeof(uint16_t) * 1 * width;
> address = ((s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA]) + sizeof(uint16_t) * 5 * width;
> - address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *)data, size, 0);
> + address_space_read(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)data, size);
> if (data[0 * width] & 0x1) {
> /* Still EOL ; stop reception */
> return -1;
> @@ -721,11 +721,11 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> /* Put packet into RBA */
> DPRINTF("Receive packet at %08x\n", (s->regs[SONIC_CRBA1] << 16) | s->regs[SONIC_CRBA0]);
> address = (s->regs[SONIC_CRBA1] << 16) | s->regs[SONIC_CRBA0];
> - address_space_rw(&s->as, address,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)buf, rx_len, 1);
> + address_space_write(&s->as, address,
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)buf, rx_len);
> address += rx_len;
> - address_space_rw(&s->as, address,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)&checksum, 4, 1);
> + address_space_write(&s->as, address,
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)&checksum, 4);
> rx_len += 4;
> s->regs[SONIC_CRBA1] = address >> 16;
> s->regs[SONIC_CRBA0] = address & 0xffff;
> @@ -753,23 +753,23 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> data[3 * width] = s->regs[SONIC_TRBA1]; /* pkt_ptr1 */
> data[4 * width] = s->regs[SONIC_RSC]; /* seq_no */
> size = sizeof(uint16_t) * 5 * width;
> - address_space_rw(&s->as, (s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA],
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size, 1);
> + address_space_write(&s->as, (s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA],
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size);
>
> /* Move to next descriptor */
> size = sizeof(uint16_t) * width;
> - address_space_rw(&s->as,
> + address_space_read(&s->as,
> ((s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA]) + sizeof(uint16_t) * 5 * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size, 0);
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size);
> s->regs[SONIC_LLFA] = data[0 * width];
> if (s->regs[SONIC_LLFA] & 0x1) {
> /* EOL detected */
> s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
> } else {
> data[0 * width] = 0; /* in_use */
> - address_space_rw(&s->as,
> + address_space_write(&s->as,
> ((s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA]) + sizeof(uint16_t) * 6 * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, sizeof(uint16_t), 1);
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, sizeof(uint16_t));
> s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
> s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX;
> s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) | (((s->regs[SONIC_RSC] & 0x00ff) + 1) & 0x00ff);
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 2/9] Change signature of address_space_read() to avoid casting
[not found] ` <1467934423-5997-3-git-send-email-andrew.smirnov@gmail.com>
@ 2016-07-08 3:33 ` David Gibson
0 siblings, 0 replies; 20+ messages in thread
From: David Gibson @ 2016-07-08 3:33 UTC (permalink / raw)
To: Andrey Smirnov; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 6678 bytes --]
On Thu, Jul 07, 2016 at 04:33:36PM -0700, Andrey Smirnov wrote:
> Change signature of address_space_read() to expectet void * as a buffer
> instead of uint8_t * to avoid forcing the caller of the function to do a
> type cast.
>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Seems reasonable.
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> exec.c | 2 +-
> hw/net/dp8393x.c | 16 ++++++++--------
> hw/virtio/virtio.c | 2 +-
> include/exec/memory.h | 2 +-
> 4 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 93611f1..1bd2204 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2736,7 +2736,7 @@ MemTxResult address_space_rw(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
> if (is_write) {
> return address_space_write(as, addr, attrs, (uint8_t *)buf, len);
> } else {
> - return address_space_read(as, addr, attrs, (uint8_t *)buf, len);
> + return address_space_read(as, addr, attrs, buf, len);
> }
> }
>
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 2849542..06dc99b 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -205,7 +205,7 @@ static void dp8393x_do_load_cam(dp8393xState *s)
> /* Fill current entry */
> address_space_read(&s->as,
> (s->regs[SONIC_URRA] << 16) | s->regs[SONIC_CDP],
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size);
> + MEMTXATTRS_UNSPECIFIED, data, size);
> s->cam[index][0] = data[1 * width] & 0xff;
> s->cam[index][1] = data[1 * width] >> 8;
> s->cam[index][2] = data[2 * width] & 0xff;
> @@ -224,7 +224,7 @@ static void dp8393x_do_load_cam(dp8393xState *s)
> /* Read CAM enable */
> address_space_read(&s->as,
> (s->regs[SONIC_URRA] << 16) | s->regs[SONIC_CDP],
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size);
> + MEMTXATTRS_UNSPECIFIED, data, size);
> s->regs[SONIC_CE] = data[0 * width];
> DPRINTF("load cam done. cam enable mask 0x%04x\n", s->regs[SONIC_CE]);
>
> @@ -244,7 +244,7 @@ static void dp8393x_do_read_rra(dp8393xState *s)
> size = sizeof(uint16_t) * 4 * width;
> address_space_read(&s->as,
> (s->regs[SONIC_URRA] << 16) | s->regs[SONIC_RRP],
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size);
> + MEMTXATTRS_UNSPECIFIED, data, size);
>
> /* Update SONIC registers */
> s->regs[SONIC_CRBA0] = data[0 * width];
> @@ -362,7 +362,7 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
> s->regs[SONIC_TTDA] = s->regs[SONIC_CTDA];
> address_space_read(&s->as,
> ((s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_TTDA]) + sizeof(uint16_t) * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size);
> + MEMTXATTRS_UNSPECIFIED, data, size);
> tx_len = 0;
>
> /* Update registers */
> @@ -397,7 +397,7 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
> size = sizeof(uint16_t) * 3 * width;
> address_space_read(&s->as,
> ((s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_TTDA]) + sizeof(uint16_t) * (4 + 3 * i) * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size);
> + MEMTXATTRS_UNSPECIFIED, data, size);
> s->regs[SONIC_TSA0] = data[0 * width];
> s->regs[SONIC_TSA1] = data[1 * width];
> s->regs[SONIC_TFS] = data[2 * width];
> @@ -438,7 +438,7 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
> size = sizeof(uint16_t) * width;
> address_space_read(&s->as,
> ((s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_TTDA]) + sizeof(uint16_t) * (4 + 3 * s->regs[SONIC_TFC]) * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size);
> + MEMTXATTRS_UNSPECIFIED, data, size);
> s->regs[SONIC_CTDA] = data[0 * width] & ~0x1;
> if (data[0 * width] & 0x1) {
> /* EOL detected */
> @@ -702,7 +702,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> size = sizeof(uint16_t) * 1 * width;
> address = ((s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA]) + sizeof(uint16_t) * 5 * width;
> address_space_read(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *)data, size);
> + data, size);
> if (data[0 * width] & 0x1) {
> /* Still EOL ; stop reception */
> return -1;
> @@ -760,7 +760,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> size = sizeof(uint16_t) * width;
> address_space_read(&s->as,
> ((s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA]) + sizeof(uint16_t) * 5 * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size);
> + MEMTXATTRS_UNSPECIFIED, data, size);
> s->regs[SONIC_LLFA] = data[0 * width];
> if (s->regs[SONIC_LLFA] & 0x1) {
> /* EOL detected */
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 7ed06ea..449f125 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -122,7 +122,7 @@ static void vring_desc_read(VirtIODevice *vdev, VRingDesc *desc,
> hwaddr desc_pa, int i)
> {
> address_space_read(&address_space_memory, desc_pa + i * sizeof(VRingDesc),
> - MEMTXATTRS_UNSPECIFIED, (void *)desc, sizeof(VRingDesc));
> + MEMTXATTRS_UNSPECIFIED, desc, sizeof(VRingDesc));
> virtio_tswap64s(vdev, &desc->addr);
> virtio_tswap32s(vdev, &desc->len);
> virtio_tswap16s(vdev, &desc->flags);
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 4ab6800..576c5a5 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1418,7 +1418,7 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
> */
> static inline __attribute__((__always_inline__))
> MemTxResult address_space_read(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
> - uint8_t *buf, int len)
> + void *buf, int len)
> {
> MemTxResult result = MEMTX_OK;
> hwaddr l, addr1;
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 3/9] Change signature of address_space_write() to avoid casting
[not found] ` <1467934423-5997-4-git-send-email-andrew.smirnov@gmail.com>
@ 2016-07-08 3:34 ` David Gibson
0 siblings, 0 replies; 20+ messages in thread
From: David Gibson @ 2016-07-08 3:34 UTC (permalink / raw)
To: Andrey Smirnov; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 6355 bytes --]
On Thu, Jul 07, 2016 at 04:33:37PM -0700, Andrey Smirnov wrote:
> Change signature of address_space_write() to expectet void * as a buffer
> instead of uint8_t * to avoid forcing the caller of the function to do a
> type cast.
>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> exec.c | 10 +++++-----
> hw/net/dp8393x.c | 10 +++++-----
> hw/virtio/virtio.c | 2 +-
> include/exec/memory.h | 2 +-
> 4 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 1bd2204..c26bcea 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2621,7 +2621,7 @@ static MemTxResult address_space_write_continue(AddressSpace *as, hwaddr addr,
> }
>
> MemTxResult address_space_write(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
> - const uint8_t *buf, int len)
> + const void *buf, int len)
> {
> hwaddr l;
> hwaddr addr1;
> @@ -2734,7 +2734,7 @@ MemTxResult address_space_rw(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
> uint8_t *buf, int len, bool is_write)
> {
> if (is_write) {
> - return address_space_write(as, addr, attrs, (uint8_t *)buf, len);
> + return address_space_write(as, addr, attrs, buf, len);
> } else {
> return address_space_read(as, addr, attrs, buf, len);
> }
> @@ -3582,7 +3582,7 @@ void address_space_stq(AddressSpace *as, hwaddr addr, uint64_t val,
> {
> MemTxResult r;
> val = tswap64(val);
> - r = address_space_write(as, addr, attrs, (void *) &val, 8);
> + r = address_space_write(as, addr, attrs, &val, 8);
> if (result) {
> *result = r;
> }
> @@ -3593,7 +3593,7 @@ void address_space_stq_le(AddressSpace *as, hwaddr addr, uint64_t val,
> {
> MemTxResult r;
> val = cpu_to_le64(val);
> - r = address_space_write(as, addr, attrs, (void *) &val, 8);
> + r = address_space_write(as, addr, attrs, &val, 8);
> if (result) {
> *result = r;
> }
> @@ -3603,7 +3603,7 @@ void address_space_stq_be(AddressSpace *as, hwaddr addr, uint64_t val,
> {
> MemTxResult r;
> val = cpu_to_be64(val);
> - r = address_space_write(as, addr, attrs, (void *) &val, 8);
> + r = address_space_write(as, addr, attrs, &val, 8);
> if (result) {
> *result = r;
> }
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 06dc99b..4196194 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -431,7 +431,7 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
> size = sizeof(uint16_t) * width;
> address_space_write(&s->as,
> (s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_TTDA],
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size);
> + MEMTXATTRS_UNSPECIFIED, data, size);
>
> if (!(s->regs[SONIC_CR] & SONIC_CR_HTX)) {
> /* Read footer of packet */
> @@ -722,10 +722,10 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> DPRINTF("Receive packet at %08x\n", (s->regs[SONIC_CRBA1] << 16) | s->regs[SONIC_CRBA0]);
> address = (s->regs[SONIC_CRBA1] << 16) | s->regs[SONIC_CRBA0];
> address_space_write(&s->as, address,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)buf, rx_len);
> + MEMTXATTRS_UNSPECIFIED, buf, rx_len);
> address += rx_len;
> address_space_write(&s->as, address,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)&checksum, 4);
> + MEMTXATTRS_UNSPECIFIED, &checksum, 4);
> rx_len += 4;
> s->regs[SONIC_CRBA1] = address >> 16;
> s->regs[SONIC_CRBA0] = address & 0xffff;
> @@ -754,7 +754,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> data[4 * width] = s->regs[SONIC_RSC]; /* seq_no */
> size = sizeof(uint16_t) * 5 * width;
> address_space_write(&s->as, (s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA],
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size);
> + MEMTXATTRS_UNSPECIFIED, data, size);
>
> /* Move to next descriptor */
> size = sizeof(uint16_t) * width;
> @@ -769,7 +769,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> data[0 * width] = 0; /* in_use */
> address_space_write(&s->as,
> ((s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA]) + sizeof(uint16_t) * 6 * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, sizeof(uint16_t));
> + MEMTXATTRS_UNSPECIFIED, data, sizeof(uint16_t));
> s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
> s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX;
> s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) | (((s->regs[SONIC_RSC] & 0x00ff) + 1) & 0x00ff);
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 449f125..cda7075 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -164,7 +164,7 @@ static inline void vring_used_write(VirtQueue *vq, VRingUsedElem *uelem,
> virtio_tswap32s(vq->vdev, &uelem->len);
> pa = vq->vring.used + offsetof(VRingUsed, ring[i]);
> address_space_write(&address_space_memory, pa, MEMTXATTRS_UNSPECIFIED,
> - (void *)uelem, sizeof(VRingUsedElem));
> + uelem, sizeof(VRingUsedElem));
> }
>
> static uint16_t vring_used_idx(VirtQueue *vq)
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 576c5a5..51e4c2f 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1271,7 +1271,7 @@ MemTxResult address_space_rw(AddressSpace *as, hwaddr addr,
> */
> MemTxResult address_space_write(AddressSpace *as, hwaddr addr,
> MemTxAttrs attrs,
> - const uint8_t *buf, int len);
> + const void *buf, int len);
>
> /* address_space_ld*: load from an address space
> * address_space_st*: store to an address space
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 4/9] address_space_write_continue: Distill common code
[not found] ` <1467934423-5997-5-git-send-email-andrew.smirnov@gmail.com>
@ 2016-07-08 3:38 ` David Gibson
2016-07-12 15:28 ` Andrey Smirnov
0 siblings, 1 reply; 20+ messages in thread
From: David Gibson @ 2016-07-08 3:38 UTC (permalink / raw)
To: Andrey Smirnov; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2975 bytes --]
On Thu, Jul 07, 2016 at 04:33:38PM -0700, Andrey Smirnov wrote:
> Move call to memory_region_dispatch_write() outside of swtich statement
> since the only thing that is different about all of those call is
> "length" argument and that matches value in "l". Also collapse switch
> statement to occupy less vertical space.
>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
I'm not a big fan of putting multiple statements on a single line,
especially flow control like break. But apart from that,
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> exec.c | 31 +++++++------------------------
> 1 file changed, 7 insertions(+), 24 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index c26bcea..eea3018 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2566,33 +2566,16 @@ static MemTxResult address_space_write_continue(AddressSpace *as, hwaddr addr,
> /* XXX: could force current_cpu to NULL to avoid
> potential bugs */
> switch (l) {
> - case 8:
> - /* 64 bit write access */
> - val = ldq_p(buf);
> - result |= memory_region_dispatch_write(mr, addr1, val, 8,
> - attrs);
> - break;
> - case 4:
> - /* 32 bit write access */
> - val = ldl_p(buf);
> - result |= memory_region_dispatch_write(mr, addr1, val, 4,
> - attrs);
> - break;
> - case 2:
> - /* 16 bit write access */
> - val = lduw_p(buf);
> - result |= memory_region_dispatch_write(mr, addr1, val, 2,
> - attrs);
> - break;
> - case 1:
> - /* 8 bit write access */
> - val = ldub_p(buf);
> - result |= memory_region_dispatch_write(mr, addr1, val, 1,
> - attrs);
> - break;
> + case 8: val = ldq_p(buf); break; /* 64 bit write access */
> + case 4: val = ldl_p(buf); break; /* 32 bit write access */
> + case 2: val = lduw_p(buf); break; /* 16 bit write access */
> + case 1: val = ldub_p(buf); break; /* 8 bit write access */
> default:
> abort();
> }
> +
> + result |= memory_region_dispatch_write(mr, addr1, val, l,
> + attrs);
> } else {
> /* RAM case */
> ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 5/9] Change signature of cpu_memory_rw_debug() to avoid casting
[not found] ` <1467934423-5997-6-git-send-email-andrew.smirnov@gmail.com>
@ 2016-07-08 3:39 ` David Gibson
0 siblings, 0 replies; 20+ messages in thread
From: David Gibson @ 2016-07-08 3:39 UTC (permalink / raw)
To: Andrey Smirnov; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 14008 bytes --]
On Thu, Jul 07, 2016 at 04:33:39PM -0700, Andrey Smirnov wrote:
> Change signature of cpu_memory_rw_debug() to expectet void * as a buffer
> instead of uint8_t * to avoid forcing the caller of the function to do a
> type cast.
>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Seems reasonable.
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> exec.c | 6 ++++--
> hw/i386/kvmvapic.c | 10 +++++-----
> include/exec/cpu-all.h | 2 +-
> include/exec/softmmu-semi.h | 8 ++++----
> target-arm/arm-semi.c | 2 +-
> target-arm/kvm64.c | 8 ++++----
> target-i386/helper.c | 4 ++--
> target-i386/kvm.c | 8 ++++----
> target-ppc/kvm.c | 8 ++++----
> target-s390x/kvm.c | 6 +++---
> target-xtensa/xtensa-semi.c | 6 +++---
> 11 files changed, 35 insertions(+), 33 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index eea3018..5cef9fe 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2436,11 +2436,12 @@ MemoryRegion *get_system_io(void)
> /* physical memory access (slow version, mainly for debug) */
> #if defined(CONFIG_USER_ONLY)
> int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
> - uint8_t *buf, int len, int is_write)
> + void *b, int len, int is_write)
> {
> int l, flags;
> target_ulong page;
> void * p;
> + uint8_t *buf = b;
>
> while (len > 0) {
> page = addr & TARGET_PAGE_MASK;
> @@ -3609,11 +3610,12 @@ void stq_be_phys(AddressSpace *as, hwaddr addr, uint64_t val)
>
> /* virtual memory access for debug (includes writing to ROM) */
> int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
> - uint8_t *buf, int len, int is_write)
> + void *b, int len, int is_write)
> {
> int l;
> hwaddr phys_addr;
> target_ulong page;
> + uint8_t *buf = b;
>
> while (len > 0) {
> int asidx;
> diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
> index 3bf1ddd..c684675 100644
> --- a/hw/i386/kvmvapic.c
> +++ b/hw/i386/kvmvapic.c
> @@ -261,7 +261,7 @@ instruction_ok:
> * and update the cached values.
> */
> if (cpu_memory_rw_debug(cs, ip + instr->addr_offset,
> - (void *)&real_tpr_addr,
> + &real_tpr_addr,
> sizeof(real_tpr_addr), 0) < 0) {
> return -1;
> }
> @@ -349,7 +349,7 @@ static int get_kpcr_number(X86CPU *cpu)
> } QEMU_PACKED kpcr;
>
> if (cpu_memory_rw_debug(CPU(cpu), env->segs[R_FS].base,
> - (void *)&kpcr, sizeof(kpcr), 0) < 0 ||
> + &kpcr, sizeof(kpcr), 0) < 0 ||
> kpcr.self != env->segs[R_FS].base) {
> return -1;
> }
> @@ -388,7 +388,7 @@ static void patch_call(VAPICROMState *s, X86CPU *cpu, target_ulong ip,
>
> offset = cpu_to_le32(target - ip - 5);
> patch_byte(cpu, ip, 0xe8); /* call near */
> - cpu_memory_rw_debug(CPU(cpu), ip + 1, (void *)&offset, sizeof(offset), 1);
> + cpu_memory_rw_debug(CPU(cpu), ip + 1, &offset, sizeof(offset), 1);
> }
>
> static void patch_instruction(VAPICROMState *s, X86CPU *cpu, target_ulong ip)
> @@ -434,8 +434,8 @@ static void patch_instruction(VAPICROMState *s, X86CPU *cpu, target_ulong ip)
> break;
> case 0xc7: /* mov imm32, r/m32 (c7/0) */
> patch_byte(cpu, ip, 0x68); /* push imm32 */
> - cpu_memory_rw_debug(cs, ip + 6, (void *)&imm32, sizeof(imm32), 0);
> - cpu_memory_rw_debug(cs, ip + 1, (void *)&imm32, sizeof(imm32), 1);
> + cpu_memory_rw_debug(cs, ip + 6, &imm32, sizeof(imm32), 0);
> + cpu_memory_rw_debug(cs, ip + 1, &imm32, sizeof(imm32), 1);
> patch_call(s, cpu, ip + 5, handlers->set_tpr);
> break;
> case 0xff: /* push r/m32 */
> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> index 9f38edf..2b2f38a 100644
> --- a/include/exec/cpu-all.h
> +++ b/include/exec/cpu-all.h
> @@ -302,6 +302,6 @@ void dump_opcount_info(FILE *f, fprintf_function cpu_fprintf);
> #endif /* !CONFIG_USER_ONLY */
>
> int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
> - uint8_t *buf, int len, int is_write);
> + void *buf, int len, int is_write);
>
> #endif /* CPU_ALL_H */
> diff --git a/include/exec/softmmu-semi.h b/include/exec/softmmu-semi.h
> index 3a58c3f..98e5b5c 100644
> --- a/include/exec/softmmu-semi.h
> +++ b/include/exec/softmmu-semi.h
> @@ -13,7 +13,7 @@ static inline uint64_t softmmu_tget64(CPUArchState *env, target_ulong addr)
> {
> uint64_t val;
>
> - cpu_memory_rw_debug(ENV_GET_CPU(env), addr, (uint8_t *)&val, 8, 0);
> + cpu_memory_rw_debug(ENV_GET_CPU(env), addr, &val, 8, 0);
> return tswap64(val);
> }
>
> @@ -21,7 +21,7 @@ static inline uint32_t softmmu_tget32(CPUArchState *env, target_ulong addr)
> {
> uint32_t val;
>
> - cpu_memory_rw_debug(ENV_GET_CPU(env), addr, (uint8_t *)&val, 4, 0);
> + cpu_memory_rw_debug(ENV_GET_CPU(env), addr, &val, 4, 0);
> return tswap32(val);
> }
>
> @@ -42,14 +42,14 @@ static inline void softmmu_tput64(CPUArchState *env,
> target_ulong addr, uint64_t val)
> {
> val = tswap64(val);
> - cpu_memory_rw_debug(ENV_GET_CPU(env), addr, (uint8_t *)&val, 8, 1);
> + cpu_memory_rw_debug(ENV_GET_CPU(env), addr, &val, 8, 1);
> }
>
> static inline void softmmu_tput32(CPUArchState *env,
> target_ulong addr, uint32_t val)
> {
> val = tswap32(val);
> - cpu_memory_rw_debug(ENV_GET_CPU(env), addr, (uint8_t *)&val, 4, 1);
> + cpu_memory_rw_debug(ENV_GET_CPU(env), addr, &val, 4, 1);
> }
> #define put_user_u64(arg, p) ({ softmmu_tput64(env, p, arg) ; 0; })
> #define put_user_u32(arg, p) ({ softmmu_tput32(env, p, arg) ; 0; })
> diff --git a/target-arm/arm-semi.c b/target-arm/arm-semi.c
> index 8be0645..03c959d 100644
> --- a/target-arm/arm-semi.c
> +++ b/target-arm/arm-semi.c
> @@ -187,7 +187,7 @@ static void arm_semi_flen_cb(CPUState *cs, target_ulong ret, target_ulong err)
> /* The size is always stored in big-endian order, extract
> the value. We assume the size always fit in 32 bits. */
> uint32_t size;
> - cpu_memory_rw_debug(cs, arm_flen_buf(cpu) + 32, (uint8_t *)&size, 4, 0);
> + cpu_memory_rw_debug(cs, arm_flen_buf(cpu) + 32, &size, 4, 0);
> size = be32_to_cpu(size);
> if (is_a64(env)) {
> env->xregs[0] = size;
> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> index 5faa76c..7ba5acd 100644
> --- a/target-arm/kvm64.c
> +++ b/target-arm/kvm64.c
> @@ -874,8 +874,8 @@ static const uint32_t brk_insn = 0xd4200000;
> int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> {
> if (have_guest_debug) {
> - if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0) ||
> - cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&brk_insn, 4, 1)) {
> + if (cpu_memory_rw_debug(cs, bp->pc, &bp->saved_insn, 4, 0) ||
> + cpu_memory_rw_debug(cs, bp->pc, &brk_insn, 4, 1)) {
> return -EINVAL;
> }
> return 0;
> @@ -890,9 +890,9 @@ int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> static uint32_t brk;
>
> if (have_guest_debug) {
> - if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&brk, 4, 0) ||
> + if (cpu_memory_rw_debug(cs, bp->pc, &brk, 4, 0) ||
> brk != brk_insn ||
> - cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 1)) {
> + cpu_memory_rw_debug(cs, bp->pc, &bp->saved_insn, 4, 1)) {
> return -EINVAL;
> }
> return 0;
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index 1c250b8..6b10a8e 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -1286,8 +1286,8 @@ int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector,
> index = selector & ~7;
> ptr = dt->base + index;
> if ((index + 7) > dt->limit
> - || cpu_memory_rw_debug(cs, ptr, (uint8_t *)&e1, sizeof(e1), 0) != 0
> - || cpu_memory_rw_debug(cs, ptr+4, (uint8_t *)&e2, sizeof(e2), 0) != 0)
> + || cpu_memory_rw_debug(cs, ptr, &e1, sizeof(e1), 0) != 0
> + || cpu_memory_rw_debug(cs, ptr+4, &e2, sizeof(e2), 0) != 0)
> return 0;
>
> *base = ((e1 >> 16) | ((e2 & 0xff) << 16) | (e2 & 0xff000000));
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index ff92b1d..dc0cf6b 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -2864,10 +2864,10 @@ static int kvm_handle_tpr_access(X86CPU *cpu)
>
> int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> {
> - static const uint8_t int3 = 0xcc;
> + uint8_t int3 = 0xcc;
>
> - if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 1, 0) ||
> - cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&int3, 1, 1)) {
> + if (cpu_memory_rw_debug(cs, bp->pc, &bp->saved_insn, 1, 0) ||
> + cpu_memory_rw_debug(cs, bp->pc, &int3, 1, 1)) {
> return -EINVAL;
> }
> return 0;
> @@ -2878,7 +2878,7 @@ int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> uint8_t int3;
>
> if (cpu_memory_rw_debug(cs, bp->pc, &int3, 1, 0) || int3 != 0xcc ||
> - cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 1, 1)) {
> + cpu_memory_rw_debug(cs, bp->pc, &bp->saved_insn, 1, 1)) {
> return -EINVAL;
> }
> return 0;
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 1620864..efa257b 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -1432,9 +1432,9 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> /* Mixed endian case is not handled */
> uint32_t sc = debug_inst_opcode;
>
> - if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn,
> + if (cpu_memory_rw_debug(cs, bp->pc, &bp->saved_insn,
> sizeof(sc), 0) ||
> - cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&sc, sizeof(sc), 1)) {
> + cpu_memory_rw_debug(cs, bp->pc, &sc, sizeof(sc), 1)) {
> return -EINVAL;
> }
>
> @@ -1445,9 +1445,9 @@ int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> {
> uint32_t sc;
>
> - if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&sc, sizeof(sc), 0) ||
> + if (cpu_memory_rw_debug(cs, bp->pc, &sc, sizeof(sc), 0) ||
> sc != debug_inst_opcode ||
> - cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn,
> + cpu_memory_rw_debug(cs, bp->pc, &bp->saved_insn,
> sizeof(sc), 1)) {
> return -EINVAL;
> }
> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> index 45e94ca..fb7dd55 100644
> --- a/target-s390x/kvm.c
> +++ b/target-s390x/kvm.c
> @@ -671,9 +671,9 @@ static const uint8_t diag_501[] = {0x83, 0x24, 0x05, 0x01};
> int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> {
>
> - if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn,
> + if (cpu_memory_rw_debug(cs, bp->pc, &bp->saved_insn,
> sizeof(diag_501), 0) ||
> - cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)diag_501,
> + cpu_memory_rw_debug(cs, bp->pc, diag_501,
> sizeof(diag_501), 1)) {
> return -EINVAL;
> }
> @@ -688,7 +688,7 @@ int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> return -EINVAL;
> } else if (memcmp(t, diag_501, sizeof(diag_501))) {
> return -EINVAL;
> - } else if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn,
> + } else if (cpu_memory_rw_debug(cs, bp->pc, &bp->saved_insn,
> sizeof(diag_501), 1)) {
> return -EINVAL;
> }
> diff --git a/target-xtensa/xtensa-semi.c b/target-xtensa/xtensa-semi.c
> index 370e365..ec199ac 100644
> --- a/target-xtensa/xtensa-semi.c
> +++ b/target-xtensa/xtensa-semi.c
> @@ -202,7 +202,7 @@ void HELPER(simcall)(CPUXtensaState *env)
>
> for (i = 0; i < ARRAY_SIZE(name); ++i) {
> rc = cpu_memory_rw_debug(cs, regs[3] + i,
> - (uint8_t *)name + i, 1, 0);
> + &name[i], 1, 0);
> if (rc != 0 || name[i] == 0) {
> break;
> }
> @@ -247,7 +247,7 @@ void HELPER(simcall)(CPUXtensaState *env)
>
> if (target_tv) {
> cpu_memory_rw_debug(cs, target_tv,
> - (uint8_t *)target_tvv, sizeof(target_tvv), 0);
> + target_tvv, sizeof(target_tvv), 0);
> tv.tv_sec = (int32_t)tswap32(target_tvv[0]);
> tv.tv_usec = (int32_t)tswap32(target_tvv[1]);
> }
> @@ -282,7 +282,7 @@ void HELPER(simcall)(CPUXtensaState *env)
>
> argv.argptr[0] = tswap32(regs[3] + offsetof(struct Argv, text));
> cpu_memory_rw_debug(cs,
> - regs[3], (uint8_t *)&argv, sizeof(argv), 1);
> + regs[3], &argv, sizeof(argv), 1);
> }
> break;
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 6/9] Convert cpu_memory_rw_debug to use MMUAccessType
[not found] ` <1467934423-5997-7-git-send-email-andrew.smirnov@gmail.com>
@ 2016-07-08 3:42 ` David Gibson
2016-07-10 19:32 ` Peter Maydell
0 siblings, 1 reply; 20+ messages in thread
From: David Gibson @ 2016-07-08 3:42 UTC (permalink / raw)
To: Andrey Smirnov; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 23670 bytes --]
On Thu, Jul 07, 2016 at 04:33:40PM -0700, Andrey Smirnov wrote:
> Convert cpu_memory_rw_debug() to use MMUAccessType as a way of
> specifying memory reads/writes. This makes caller code be more obvious
> in what it does (previously one had to interpret 0 or 1 and remember the
> semantics of the last boolean argument of the function) and uses the
> same approach used by tlb_* functions.
>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
My only concern here is that the constants are named
*MMU*_DATA_... whereas these are physical memory accesses not
involving the MMU. I can't actually see any current users of
MMUAccessType which makes me a bit confused as to what it's intended
meaning was.
> ---
> cpus.c | 2 +-
> disas.c | 4 ++--
> exec.c | 14 ++++++++++----
> gdbstub.c | 3 ++-
> hw/i386/kvmvapic.c | 20 +++++++++++---------
> include/exec/cpu-all.h | 2 +-
> include/exec/softmmu-semi.h | 16 ++++++++--------
> monitor.c | 3 ++-
> target-arm/arm-semi.c | 2 +-
> target-arm/kvm64.c | 12 ++++++++----
> target-i386/helper.c | 7 ++++---
> target-i386/kvm.c | 9 +++++----
> target-ppc/kvm.c | 9 +++++----
> target-s390x/kvm.c | 9 +++++----
> target-sparc/mmu_helper.c | 8 ++++++--
> target-xtensa/xtensa-semi.c | 10 +++++-----
> 16 files changed, 76 insertions(+), 54 deletions(-)
>
> diff --git a/cpus.c b/cpus.c
> index 84c3520..7aa984b 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1691,7 +1691,7 @@ void qmp_memsave(int64_t addr, int64_t size, const char *filename,
> l = sizeof(buf);
> if (l > size)
> l = size;
> - if (cpu_memory_rw_debug(cpu, addr, buf, l, 0) != 0) {
> + if (cpu_memory_rw_debug(cpu, addr, buf, l, MMU_DATA_LOAD) != 0) {
> error_setg(errp, "Invalid addr 0x%016" PRIx64 "/size %" PRId64
> " specified", orig_addr, orig_size);
> goto exit;
> diff --git a/disas.c b/disas.c
> index 05a7a12..75cae10 100644
> --- a/disas.c
> +++ b/disas.c
> @@ -39,7 +39,7 @@ target_read_memory (bfd_vma memaddr,
> {
> CPUDebug *s = container_of(info, CPUDebug, info);
>
> - cpu_memory_rw_debug(s->cpu, memaddr, myaddr, length, 0);
> + cpu_memory_rw_debug(s->cpu, memaddr, myaddr, length, MMU_DATA_LOAD);
> return 0;
> }
>
> @@ -358,7 +358,7 @@ monitor_read_memory (bfd_vma memaddr, bfd_byte *myaddr, int length,
> if (monitor_disas_is_physical) {
> cpu_physical_memory_read(memaddr, myaddr, length);
> } else {
> - cpu_memory_rw_debug(s->cpu, memaddr, myaddr, length, 0);
> + cpu_memory_rw_debug(s->cpu, memaddr, myaddr, length, MMU_DATA_LOAD);
> }
> return 0;
> }
> diff --git a/exec.c b/exec.c
> index 5cef9fe..36a66e6 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2436,13 +2436,16 @@ MemoryRegion *get_system_io(void)
> /* physical memory access (slow version, mainly for debug) */
> #if defined(CONFIG_USER_ONLY)
> int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
> - void *b, int len, int is_write)
> + void *b, int len, MMUAccessType access_type)
> {
> int l, flags;
> target_ulong page;
> void * p;
> uint8_t *buf = b;
>
> + g_assert(access_type == MMU_DATA_STORE ||
> + access_type == MMU_DATA_LOAD);
> +
> while (len > 0) {
> page = addr & TARGET_PAGE_MASK;
> l = (page + TARGET_PAGE_SIZE) - addr;
> @@ -2451,7 +2454,7 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
> flags = page_get_flags(page);
> if (!(flags & PAGE_VALID))
> return -1;
> - if (is_write) {
> + if (access_type == MMU_DATA_STORE) {
> if (!(flags & PAGE_WRITE))
> return -1;
> /* XXX: this code should not depend on lock_user */
> @@ -3610,13 +3613,16 @@ void stq_be_phys(AddressSpace *as, hwaddr addr, uint64_t val)
>
> /* virtual memory access for debug (includes writing to ROM) */
> int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
> - void *b, int len, int is_write)
> + void *b, int len, MMUAccessType access_type)
> {
> int l;
> hwaddr phys_addr;
> target_ulong page;
> uint8_t *buf = b;
>
> + g_assert(access_type == MMU_DATA_STORE ||
> + access_type == MMU_DATA_LOAD);
> +
> while (len > 0) {
> int asidx;
> MemTxAttrs attrs;
> @@ -3631,7 +3637,7 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
> if (l > len)
> l = len;
> phys_addr += (addr & ~TARGET_PAGE_MASK);
> - if (is_write) {
> + if (access_type == MMU_DATA_STORE) {
> cpu_physical_memory_write_rom(cpu->cpu_ases[asidx].as,
> phys_addr, buf, l);
> } else {
> diff --git a/gdbstub.c b/gdbstub.c
> index 5da66f1..89cb538 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -51,7 +51,8 @@ static inline int target_memory_rw_debug(CPUState *cpu, target_ulong addr,
> if (cc->memory_rw_debug) {
> return cc->memory_rw_debug(cpu, addr, buf, len, is_write);
> }
> - return cpu_memory_rw_debug(cpu, addr, buf, len, is_write);
> + return cpu_memory_rw_debug(cpu, addr, buf, len,
> + is_write ? MMU_DATA_STORE : MMU_DATA_LOAD);
> }
>
> enum {
> diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
> index c684675..68f87c8 100644
> --- a/hw/i386/kvmvapic.c
> +++ b/hw/i386/kvmvapic.c
> @@ -233,7 +233,7 @@ static int evaluate_tpr_instruction(VAPICROMState *s, X86CPU *cpu,
> continue;
> }
> if (cpu_memory_rw_debug(cs, ip - instr->length, opcode,
> - sizeof(opcode), 0) < 0) {
> + sizeof(opcode), MMU_DATA_LOAD) < 0) {
> return -1;
> }
> if (opcode_matches(opcode, instr)) {
> @@ -243,7 +243,8 @@ static int evaluate_tpr_instruction(VAPICROMState *s, X86CPU *cpu,
> }
> return -1;
> } else {
> - if (cpu_memory_rw_debug(cs, ip, opcode, sizeof(opcode), 0) < 0) {
> + if (cpu_memory_rw_debug(cs, ip, opcode,
> + sizeof(opcode), MMU_DATA_LOAD) < 0) {
> return -1;
> }
> for (i = 0; i < ARRAY_SIZE(tpr_instr); i++) {
> @@ -262,7 +263,7 @@ instruction_ok:
> */
> if (cpu_memory_rw_debug(cs, ip + instr->addr_offset,
> &real_tpr_addr,
> - sizeof(real_tpr_addr), 0) < 0) {
> + sizeof(real_tpr_addr), MMU_DATA_LOAD) < 0) {
> return -1;
> }
> real_tpr_addr = le32_to_cpu(real_tpr_addr);
> @@ -349,7 +350,7 @@ static int get_kpcr_number(X86CPU *cpu)
> } QEMU_PACKED kpcr;
>
> if (cpu_memory_rw_debug(CPU(cpu), env->segs[R_FS].base,
> - &kpcr, sizeof(kpcr), 0) < 0 ||
> + &kpcr, sizeof(kpcr), MMU_DATA_LOAD) < 0 ||
> kpcr.self != env->segs[R_FS].base) {
> return -1;
> }
> @@ -378,7 +379,7 @@ static int vapic_enable(VAPICROMState *s, X86CPU *cpu)
>
> static void patch_byte(X86CPU *cpu, target_ulong addr, uint8_t byte)
> {
> - cpu_memory_rw_debug(CPU(cpu), addr, &byte, 1, 1);
> + cpu_memory_rw_debug(CPU(cpu), addr, &byte, 1, MMU_DATA_STORE);
> }
>
> static void patch_call(VAPICROMState *s, X86CPU *cpu, target_ulong ip,
> @@ -388,7 +389,8 @@ static void patch_call(VAPICROMState *s, X86CPU *cpu, target_ulong ip,
>
> offset = cpu_to_le32(target - ip - 5);
> patch_byte(cpu, ip, 0xe8); /* call near */
> - cpu_memory_rw_debug(CPU(cpu), ip + 1, &offset, sizeof(offset), 1);
> + cpu_memory_rw_debug(CPU(cpu), ip + 1, &offset,
> + sizeof(offset), MMU_DATA_STORE);
> }
>
> static void patch_instruction(VAPICROMState *s, X86CPU *cpu, target_ulong ip)
> @@ -415,7 +417,7 @@ static void patch_instruction(VAPICROMState *s, X86CPU *cpu, target_ulong ip)
>
> pause_all_vcpus();
>
> - cpu_memory_rw_debug(cs, ip, opcode, sizeof(opcode), 0);
> + cpu_memory_rw_debug(cs, ip, opcode, sizeof(opcode), MMU_DATA_LOAD);
>
> switch (opcode[0]) {
> case 0x89: /* mov r32 to r/m32 */
> @@ -434,8 +436,8 @@ static void patch_instruction(VAPICROMState *s, X86CPU *cpu, target_ulong ip)
> break;
> case 0xc7: /* mov imm32, r/m32 (c7/0) */
> patch_byte(cpu, ip, 0x68); /* push imm32 */
> - cpu_memory_rw_debug(cs, ip + 6, &imm32, sizeof(imm32), 0);
> - cpu_memory_rw_debug(cs, ip + 1, &imm32, sizeof(imm32), 1);
> + cpu_memory_rw_debug(cs, ip + 6, &imm32, sizeof(imm32), MMU_DATA_LOAD);
> + cpu_memory_rw_debug(cs, ip + 1, &imm32, sizeof(imm32), MMU_DATA_STORE);
> patch_call(s, cpu, ip + 5, handlers->set_tpr);
> break;
> case 0xff: /* push r/m32 */
> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> index 2b2f38a..062cc52 100644
> --- a/include/exec/cpu-all.h
> +++ b/include/exec/cpu-all.h
> @@ -302,6 +302,6 @@ void dump_opcount_info(FILE *f, fprintf_function cpu_fprintf);
> #endif /* !CONFIG_USER_ONLY */
>
> int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
> - void *buf, int len, int is_write);
> + void *buf, int len, MMUAccessType access_type);
>
> #endif /* CPU_ALL_H */
> diff --git a/include/exec/softmmu-semi.h b/include/exec/softmmu-semi.h
> index 98e5b5c..65d933e 100644
> --- a/include/exec/softmmu-semi.h
> +++ b/include/exec/softmmu-semi.h
> @@ -13,7 +13,7 @@ static inline uint64_t softmmu_tget64(CPUArchState *env, target_ulong addr)
> {
> uint64_t val;
>
> - cpu_memory_rw_debug(ENV_GET_CPU(env), addr, &val, 8, 0);
> + cpu_memory_rw_debug(ENV_GET_CPU(env), addr, &val, 8, MMU_DATA_LOAD);
> return tswap64(val);
> }
>
> @@ -21,7 +21,7 @@ static inline uint32_t softmmu_tget32(CPUArchState *env, target_ulong addr)
> {
> uint32_t val;
>
> - cpu_memory_rw_debug(ENV_GET_CPU(env), addr, &val, 4, 0);
> + cpu_memory_rw_debug(ENV_GET_CPU(env), addr, &val, 4, MMU_DATA_LOAD);
> return tswap32(val);
> }
>
> @@ -29,7 +29,7 @@ static inline uint32_t softmmu_tget8(CPUArchState *env, target_ulong addr)
> {
> uint8_t val;
>
> - cpu_memory_rw_debug(ENV_GET_CPU(env), addr, &val, 1, 0);
> + cpu_memory_rw_debug(ENV_GET_CPU(env), addr, &val, 1, MMU_DATA_LOAD);
> return val;
> }
>
> @@ -42,14 +42,14 @@ static inline void softmmu_tput64(CPUArchState *env,
> target_ulong addr, uint64_t val)
> {
> val = tswap64(val);
> - cpu_memory_rw_debug(ENV_GET_CPU(env), addr, &val, 8, 1);
> + cpu_memory_rw_debug(ENV_GET_CPU(env), addr, &val, 8, MMU_DATA_STORE);
> }
>
> static inline void softmmu_tput32(CPUArchState *env,
> target_ulong addr, uint32_t val)
> {
> val = tswap32(val);
> - cpu_memory_rw_debug(ENV_GET_CPU(env), addr, &val, 4, 1);
> + cpu_memory_rw_debug(ENV_GET_CPU(env), addr, &val, 4, MMU_DATA_STORE);
> }
> #define put_user_u64(arg, p) ({ softmmu_tput64(env, p, arg) ; 0; })
> #define put_user_u32(arg, p) ({ softmmu_tput32(env, p, arg) ; 0; })
> @@ -62,7 +62,7 @@ static void *softmmu_lock_user(CPUArchState *env,
> /* TODO: Make this something that isn't fixed size. */
> p = malloc(len);
> if (p && copy) {
> - cpu_memory_rw_debug(ENV_GET_CPU(env), addr, p, len, 0);
> + cpu_memory_rw_debug(ENV_GET_CPU(env), addr, p, len, MMU_DATA_LOAD);
> }
> return p;
> }
> @@ -78,7 +78,7 @@ static char *softmmu_lock_user_string(CPUArchState *env, target_ulong addr)
> return NULL;
> }
> do {
> - cpu_memory_rw_debug(ENV_GET_CPU(env), addr, &c, 1, 0);
> + cpu_memory_rw_debug(ENV_GET_CPU(env), addr, &c, 1, MMU_DATA_LOAD);
> addr++;
> *(p++) = c;
> } while (c);
> @@ -89,7 +89,7 @@ static void softmmu_unlock_user(CPUArchState *env, void *p, target_ulong addr,
> target_ulong len)
> {
> if (len) {
> - cpu_memory_rw_debug(ENV_GET_CPU(env), addr, p, len, 1);
> + cpu_memory_rw_debug(ENV_GET_CPU(env), addr, p, len, MMU_DATA_STORE);
> }
> free(p);
> }
> diff --git a/monitor.c b/monitor.c
> index a27e115..e622d9b 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1268,7 +1268,8 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize,
> if (is_physical) {
> cpu_physical_memory_read(addr, buf, l);
> } else {
> - if (cpu_memory_rw_debug(mon_get_cpu(), addr, buf, l, 0) < 0) {
> + if (cpu_memory_rw_debug(mon_get_cpu(), addr, buf,
> + l, MMU_DATA_LOAD) < 0) {
> monitor_printf(mon, " Cannot access memory\n");
> break;
> }
> diff --git a/target-arm/arm-semi.c b/target-arm/arm-semi.c
> index 03c959d..42a2856 100644
> --- a/target-arm/arm-semi.c
> +++ b/target-arm/arm-semi.c
> @@ -187,7 +187,7 @@ static void arm_semi_flen_cb(CPUState *cs, target_ulong ret, target_ulong err)
> /* The size is always stored in big-endian order, extract
> the value. We assume the size always fit in 32 bits. */
> uint32_t size;
> - cpu_memory_rw_debug(cs, arm_flen_buf(cpu) + 32, &size, 4, 0);
> + cpu_memory_rw_debug(cs, arm_flen_buf(cpu) + 32, &size, 4, MMU_DATA_LOAD);
> size = be32_to_cpu(size);
> if (is_a64(env)) {
> env->xregs[0] = size;
> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> index 7ba5acd..ed324fd 100644
> --- a/target-arm/kvm64.c
> +++ b/target-arm/kvm64.c
> @@ -874,8 +874,10 @@ static const uint32_t brk_insn = 0xd4200000;
> int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> {
> if (have_guest_debug) {
> - if (cpu_memory_rw_debug(cs, bp->pc, &bp->saved_insn, 4, 0) ||
> - cpu_memory_rw_debug(cs, bp->pc, &brk_insn, 4, 1)) {
> + if (cpu_memory_rw_debug(cs, bp->pc, &bp->saved_insn,
> + 4, MMU_DATA_LOAD) ||
> + cpu_memory_rw_debug(cs, bp->pc, &brk_insn,
> + 4, MMU_DATA_STORE)) {
> return -EINVAL;
> }
> return 0;
> @@ -890,9 +892,11 @@ int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> static uint32_t brk;
>
> if (have_guest_debug) {
> - if (cpu_memory_rw_debug(cs, bp->pc, &brk, 4, 0) ||
> + if (cpu_memory_rw_debug(cs, bp->pc, &brk,
> + 4, MMU_DATA_LOAD) ||
> brk != brk_insn ||
> - cpu_memory_rw_debug(cs, bp->pc, &bp->saved_insn, 4, 1)) {
> + cpu_memory_rw_debug(cs, bp->pc, &bp->saved_insn,
> + 4, MMU_DATA_STORE)) {
> return -EINVAL;
> }
> return 0;
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index 6b10a8e..7f176fa 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -555,7 +555,8 @@ void x86_cpu_dump_state(CPUState *cs, FILE *f, fprintf_function cpu_fprintf,
>
> cpu_fprintf(f, "Code=");
> for (i = 0; i < DUMP_CODE_BYTES_TOTAL; i++) {
> - if (cpu_memory_rw_debug(cs, base - offs + i, &code, 1, 0) == 0) {
> + if (cpu_memory_rw_debug(cs, base - offs + i, &code,
> + 1, MMU_DATA_LOAD) == 0) {
> snprintf(codestr, sizeof(codestr), "%02x", code);
> } else {
> snprintf(codestr, sizeof(codestr), "??");
> @@ -1286,8 +1287,8 @@ int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector,
> index = selector & ~7;
> ptr = dt->base + index;
> if ((index + 7) > dt->limit
> - || cpu_memory_rw_debug(cs, ptr, &e1, sizeof(e1), 0) != 0
> - || cpu_memory_rw_debug(cs, ptr+4, &e2, sizeof(e2), 0) != 0)
> + || cpu_memory_rw_debug(cs, ptr, &e1, sizeof(e1), MMU_DATA_LOAD) != 0
> + || cpu_memory_rw_debug(cs, ptr+4, &e2, sizeof(e2), MMU_DATA_LOAD) != 0)
> return 0;
>
> *base = ((e1 >> 16) | ((e2 & 0xff) << 16) | (e2 & 0xff000000));
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index dc0cf6b..beefcf4 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -2866,8 +2866,8 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> {
> uint8_t int3 = 0xcc;
>
> - if (cpu_memory_rw_debug(cs, bp->pc, &bp->saved_insn, 1, 0) ||
> - cpu_memory_rw_debug(cs, bp->pc, &int3, 1, 1)) {
> + if (cpu_memory_rw_debug(cs, bp->pc, &bp->saved_insn, 1, MMU_DATA_LOAD) ||
> + cpu_memory_rw_debug(cs, bp->pc, &int3, 1, MMU_DATA_STORE)) {
> return -EINVAL;
> }
> return 0;
> @@ -2877,8 +2877,9 @@ int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> {
> uint8_t int3;
>
> - if (cpu_memory_rw_debug(cs, bp->pc, &int3, 1, 0) || int3 != 0xcc ||
> - cpu_memory_rw_debug(cs, bp->pc, &bp->saved_insn, 1, 1)) {
> + if (cpu_memory_rw_debug(cs, bp->pc, &int3, 1, MMU_DATA_LOAD) ||
> + int3 != 0xcc ||
> + cpu_memory_rw_debug(cs, bp->pc, &bp->saved_insn, 1, MMU_DATA_STORE)) {
> return -EINVAL;
> }
> return 0;
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index efa257b..0a6032a 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -1433,8 +1433,8 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> uint32_t sc = debug_inst_opcode;
>
> if (cpu_memory_rw_debug(cs, bp->pc, &bp->saved_insn,
> - sizeof(sc), 0) ||
> - cpu_memory_rw_debug(cs, bp->pc, &sc, sizeof(sc), 1)) {
> + sizeof(sc), MMU_DATA_LOAD) ||
> + cpu_memory_rw_debug(cs, bp->pc, &sc, sizeof(sc), MMU_DATA_STORE)) {
> return -EINVAL;
> }
>
> @@ -1445,10 +1445,11 @@ int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> {
> uint32_t sc;
>
> - if (cpu_memory_rw_debug(cs, bp->pc, &sc, sizeof(sc), 0) ||
> + if (cpu_memory_rw_debug(cs, bp->pc, &sc,
> + sizeof(sc), MMU_DATA_LOAD) ||
> sc != debug_inst_opcode ||
> cpu_memory_rw_debug(cs, bp->pc, &bp->saved_insn,
> - sizeof(sc), 1)) {
> + sizeof(sc), MMU_DATA_STORE)) {
> return -EINVAL;
> }
>
> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> index fb7dd55..c55d853 100644
> --- a/target-s390x/kvm.c
> +++ b/target-s390x/kvm.c
> @@ -672,9 +672,9 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> {
>
> if (cpu_memory_rw_debug(cs, bp->pc, &bp->saved_insn,
> - sizeof(diag_501), 0) ||
> + sizeof(diag_501), MMU_DATA_LOAD) ||
> cpu_memory_rw_debug(cs, bp->pc, diag_501,
> - sizeof(diag_501), 1)) {
> + sizeof(diag_501), MMU_DATA_STORE)) {
> return -EINVAL;
> }
> return 0;
> @@ -684,12 +684,13 @@ int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> {
> uint8_t t[sizeof(diag_501)];
>
> - if (cpu_memory_rw_debug(cs, bp->pc, t, sizeof(diag_501), 0)) {
> + if (cpu_memory_rw_debug(cs, bp->pc, t,
> + sizeof(diag_501), MMU_DATA_LOAD)) {
> return -EINVAL;
> } else if (memcmp(t, diag_501, sizeof(diag_501))) {
> return -EINVAL;
> } else if (cpu_memory_rw_debug(cs, bp->pc, &bp->saved_insn,
> - sizeof(diag_501), 1)) {
> + sizeof(diag_501), MMU_DATA_STORE)) {
> return -EINVAL;
> }
>
> diff --git a/target-sparc/mmu_helper.c b/target-sparc/mmu_helper.c
> index 32b629f..1cf7c99 100644
> --- a/target-sparc/mmu_helper.c
> +++ b/target-sparc/mmu_helper.c
> @@ -398,7 +398,10 @@ int sparc_cpu_memory_rw_debug(CPUState *cs, vaddr address,
> /* Handle access before this window. */
> if (addr < fp) {
> len1 = fp - addr;
> - if (cpu_memory_rw_debug(cs, addr, buf, len1, is_write) != 0) {
> + if (cpu_memory_rw_debug(cs, addr, buf, len1,
> + is_write ?
> + MMU_DATA_STORE :
> + MMU_DATA_LOAD) != 0) {
> return -1;
> }
> addr += len1;
> @@ -434,7 +437,8 @@ int sparc_cpu_memory_rw_debug(CPUState *cs, vaddr address,
> }
> }
> }
> - return cpu_memory_rw_debug(cs, addr, buf, len, is_write);
> + return cpu_memory_rw_debug(cs, addr, buf, len,
> + is_write ? MMU_DATA_STORE : MMU_DATA_LOAD);
> }
>
> #else /* !TARGET_SPARC64 */
> diff --git a/target-xtensa/xtensa-semi.c b/target-xtensa/xtensa-semi.c
> index ec199ac..161a5e0 100644
> --- a/target-xtensa/xtensa-semi.c
> +++ b/target-xtensa/xtensa-semi.c
> @@ -202,7 +202,7 @@ void HELPER(simcall)(CPUXtensaState *env)
>
> for (i = 0; i < ARRAY_SIZE(name); ++i) {
> rc = cpu_memory_rw_debug(cs, regs[3] + i,
> - &name[i], 1, 0);
> + &name[i], 1, MMU_DATA_LOAD);
> if (rc != 0 || name[i] == 0) {
> break;
> }
> @@ -246,8 +246,8 @@ void HELPER(simcall)(CPUXtensaState *env)
> FD_SET(fd, &fdset);
>
> if (target_tv) {
> - cpu_memory_rw_debug(cs, target_tv,
> - target_tvv, sizeof(target_tvv), 0);
> + cpu_memory_rw_debug(cs, target_tv, target_tvv,
> + sizeof(target_tvv), MMU_DATA_LOAD);
> tv.tv_sec = (int32_t)tswap32(target_tvv[0]);
> tv.tv_usec = (int32_t)tswap32(target_tvv[1]);
> }
> @@ -281,8 +281,8 @@ void HELPER(simcall)(CPUXtensaState *env)
> };
>
> argv.argptr[0] = tswap32(regs[3] + offsetof(struct Argv, text));
> - cpu_memory_rw_debug(cs,
> - regs[3], &argv, sizeof(argv), 1);
> + cpu_memory_rw_debug(cs, regs[3], &argv,
> + sizeof(argv), MMU_DATA_STORE);
> }
> break;
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 7/9] Convert address_space_rw to use MMUAccessType
[not found] ` <1467934423-5997-8-git-send-email-andrew.smirnov@gmail.com>
@ 2016-07-08 3:45 ` David Gibson
2016-07-12 15:41 ` Andrey Smirnov
0 siblings, 1 reply; 20+ messages in thread
From: David Gibson @ 2016-07-08 3:45 UTC (permalink / raw)
To: Andrey Smirnov; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 5645 bytes --]
On Thu, Jul 07, 2016 at 04:33:41PM -0700, Andrey Smirnov wrote:
> Convert address_space_rw() to use MMUAccessType following the conversion
> of cpu_memory_rw_debug().
Same concerns as the previous patch. These paths don't actually have
anything to do with the MMU, which makes the constants oddly named.
>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
> exec.c | 13 ++++++++-----
> include/exec/memory.h | 7 +++++--
> kvm-all.c | 8 +++++---
> scripts/coverity-model.c | 7 +++++--
> 4 files changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 36a66e6..1fc6726 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2718,12 +2718,15 @@ MemTxResult address_space_read_full(AddressSpace *as, hwaddr addr,
> }
>
> MemTxResult address_space_rw(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
> - uint8_t *buf, int len, bool is_write)
> + uint8_t *buf, int len, MMUAccessType access_type)
> {
> - if (is_write) {
> + switch (access_type) {
> + case MMU_DATA_STORE:
> return address_space_write(as, addr, attrs, buf, len);
> - } else {
> + case MMU_DATA_LOAD:
> return address_space_read(as, addr, attrs, buf, len);
> + default:
> + abort();
> }
> }
>
> @@ -2731,7 +2734,7 @@ void cpu_physical_memory_rw(hwaddr addr, uint8_t *buf,
> int len, int is_write)
> {
> address_space_rw(&address_space_memory, addr, MEMTXATTRS_UNSPECIFIED,
> - buf, len, is_write);
> + buf, len, is_write ? MMU_DATA_STORE : MMU_DATA_LOAD);
> }
>
> enum write_rom_type {
> @@ -3643,7 +3646,7 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
> } else {
> address_space_rw(cpu->cpu_ases[asidx].as, phys_addr,
> MEMTXATTRS_UNSPECIFIED,
> - buf, l, 0);
> + buf, l, access_type);
Replacing a fixed 0 with access_type looks wrong here, but I don't
have the context readily to hand to be sure.
> }
> len -= l;
> buf += l;
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 51e4c2f..368263c 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -22,6 +22,7 @@
> #define DIRTY_MEMORY_NUM 3 /* num of dirty bits */
>
> #include "exec/cpu-common.h"
> +#include "qom/cpu.h"
> #ifndef CONFIG_USER_ONLY
> #include "exec/hwaddr.h"
> #endif
> @@ -1251,11 +1252,13 @@ void address_space_destroy(AddressSpace *as);
> * @addr: address within that address space
> * @attrs: memory transaction attributes
> * @buf: buffer with the data transferred
> - * @is_write: indicates the transfer direction
> + * @access_type: indicates the transfer direction (only valid values
> + * are MMU_DATA_LOAD for data reads and MMU_DATA_STORE for data
> + * writes)
> */
> MemTxResult address_space_rw(AddressSpace *as, hwaddr addr,
> MemTxAttrs attrs, uint8_t *buf,
> - int len, bool is_write);
> + int len, MMUAccessType access_type);
>
> /**
> * address_space_write: write to address space.
> diff --git a/kvm-all.c b/kvm-all.c
> index a88f917..7582275 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1767,11 +1767,12 @@ static void kvm_handle_io(uint16_t port, MemTxAttrs attrs, void *data, int direc
> {
> int i;
> uint8_t *ptr = data;
> + MMUAccessType access_type =
> + (direction == KVM_EXIT_IO_OUT) ? MMU_DATA_STORE : MMU_DATA_LOAD;
>
> for (i = 0; i < count; i++) {
> address_space_rw(&address_space_io, port, attrs,
> - ptr, size,
> - direction == KVM_EXIT_IO_OUT);
> + ptr, size, access_type);
> ptr += size;
> }
> }
> @@ -1947,7 +1948,8 @@ int kvm_cpu_exec(CPUState *cpu)
> run->mmio.phys_addr, attrs,
> run->mmio.data,
> run->mmio.len,
> - run->mmio.is_write);
> + run->mmio.is_write ?
> + MMU_DATA_STORE : MMU_DATA_LOAD);
> ret = 0;
> break;
> case KVM_EXIT_IRQ_WINDOW_OPEN:
> diff --git a/scripts/coverity-model.c b/scripts/coverity-model.c
> index ee5bf9d..be3418d 100644
> --- a/scripts/coverity-model.c
> +++ b/scripts/coverity-model.c
> @@ -68,13 +68,16 @@ static void __bufread(uint8_t *buf, ssize_t len)
> }
>
> MemTxResult address_space_rw(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
> - uint8_t *buf, int len, bool is_write)
> + uint8_t *buf, int len, MMUAccessType access_type)
> {
> MemTxResult result;
>
> // TODO: investigate impact of treating reads as producing
> // tainted data, with __coverity_tainted_data_argument__(buf).
> - if (is_write) __bufread(buf, len); else __bufwrite(buf, len);
> + if (access_type == MMU_DATA_STORE)
> + __bufread(buf, len);
> + else
> + __bufwrite(buf, len);
>
> return result;
> }
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 8/9] gdbstub: Convert target_memory_rw_debug to use MMUAccessType
[not found] ` <1467934423-5997-9-git-send-email-andrew.smirnov@gmail.com>
@ 2016-07-08 3:46 ` David Gibson
0 siblings, 0 replies; 20+ messages in thread
From: David Gibson @ 2016-07-08 3:46 UTC (permalink / raw)
To: Andrey Smirnov; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2465 bytes --]
On Thu, Jul 07, 2016 at 04:33:42PM -0700, Andrey Smirnov wrote:
> Convert target_memory_rw_debug to use MMUAccessType as to follow similar
> conversion of cpu_memory_rw_debug.
Apart from previously mentioned concerns with the confusingly named
MMU_* tokens,
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
> gdbstub.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 89cb538..a8cff45 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -44,15 +44,16 @@
> #endif
>
> static inline int target_memory_rw_debug(CPUState *cpu, target_ulong addr,
> - uint8_t *buf, int len, bool is_write)
> + uint8_t *buf, int len,
> + MMUAccessType access_type)
> {
> CPUClass *cc = CPU_GET_CLASS(cpu);
>
> if (cc->memory_rw_debug) {
> + const bool is_write = (access_type == MMU_DATA_STORE);
> return cc->memory_rw_debug(cpu, addr, buf, len, is_write);
> }
> - return cpu_memory_rw_debug(cpu, addr, buf, len,
> - is_write ? MMU_DATA_STORE : MMU_DATA_LOAD);
> + return cpu_memory_rw_debug(cpu, addr, buf, len, access_type);
> }
>
> enum {
> @@ -966,7 +967,8 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
> break;
> }
>
> - if (target_memory_rw_debug(s->g_cpu, addr, mem_buf, len, false) != 0) {
> + if (target_memory_rw_debug(s->g_cpu, addr, mem_buf,
> + len, MMU_DATA_LOAD) != 0) {
> put_packet (s, "E14");
> } else {
> memtohex(buf, mem_buf, len);
> @@ -988,7 +990,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
> }
> hextomem(mem_buf, p, len);
> if (target_memory_rw_debug(s->g_cpu, addr, mem_buf, len,
> - true) != 0) {
> + MMU_DATA_STORE) != 0) {
> put_packet(s, "E14");
> } else {
> put_packet(s, "OK");
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 6/9] Convert cpu_memory_rw_debug to use MMUAccessType
2016-07-08 3:42 ` [Qemu-devel] [PATCH 6/9] Convert cpu_memory_rw_debug to use MMUAccessType David Gibson
@ 2016-07-10 19:32 ` Peter Maydell
2016-07-11 2:24 ` David Gibson
0 siblings, 1 reply; 20+ messages in thread
From: Peter Maydell @ 2016-07-10 19:32 UTC (permalink / raw)
To: David Gibson; +Cc: Andrey Smirnov, QEMU Developers
On 8 July 2016 at 04:42, David Gibson <david@gibson.dropbear.id.au> wrote:
> My only concern here is that the constants are named
> *MMU*_DATA_... whereas these are physical memory accesses not
> involving the MMU. I can't actually see any current users of
> MMUAccessType which makes me a bit confused as to what it's intended
> meaning was
If you grep for MMU_DATA_LOAD/MMU_DATA_STORE/MMU_INST_FETCH
you'll see the uses. A lot of the softmmu code uses the
convention of 0=read,1=write,2=insn (which developed I
think historically from a bool "is_write", which you'll
still see in some function argument names, that was
augmented to handle insn-fetch separately). The enum
gives us some symbolic names for the constant values.
(There's a proposed patch somewhere to change the
'int is_write' arguments to actually use the enum type.)
thanks
-- PMM
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 6/9] Convert cpu_memory_rw_debug to use MMUAccessType
2016-07-10 19:32 ` Peter Maydell
@ 2016-07-11 2:24 ` David Gibson
2016-07-11 16:27 ` Peter Maydell
0 siblings, 1 reply; 20+ messages in thread
From: David Gibson @ 2016-07-11 2:24 UTC (permalink / raw)
To: Peter Maydell; +Cc: Andrey Smirnov, QEMU Developers
[-- Attachment #1: Type: text/plain, Size: 1266 bytes --]
On Sun, Jul 10, 2016 at 08:32:32PM +0100, Peter Maydell wrote:
> On 8 July 2016 at 04:42, David Gibson <david@gibson.dropbear.id.au> wrote:
> > My only concern here is that the constants are named
> > *MMU*_DATA_... whereas these are physical memory accesses not
> > involving the MMU. I can't actually see any current users of
> > MMUAccessType which makes me a bit confused as to what it's intended
> > meaning was
>
> If you grep for MMU_DATA_LOAD/MMU_DATA_STORE/MMU_INST_FETCH
> you'll see the uses. A lot of the softmmu code uses the
> convention of 0=read,1=write,2=insn (which developed I
> think historically from a bool "is_write", which you'll
> still see in some function argument names, that was
> augmented to handle insn-fetch separately). The enum
> gives us some symbolic names for the constant values.
> (There's a proposed patch somewhere to change the
> 'int is_write' arguments to actually use the enum type.)
Ah, yes, I see. Still surprisingly few, actually.
My concern about the potentially misleading name still stands..
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 6/9] Convert cpu_memory_rw_debug to use MMUAccessType
2016-07-11 2:24 ` David Gibson
@ 2016-07-11 16:27 ` Peter Maydell
2016-07-12 5:27 ` David Gibson
0 siblings, 1 reply; 20+ messages in thread
From: Peter Maydell @ 2016-07-11 16:27 UTC (permalink / raw)
To: David Gibson; +Cc: Andrey Smirnov, QEMU Developers
On 11 July 2016 at 03:24, David Gibson <david@gibson.dropbear.id.au> wrote:
> On Sun, Jul 10, 2016 at 08:32:32PM +0100, Peter Maydell wrote:
>> On 8 July 2016 at 04:42, David Gibson <david@gibson.dropbear.id.au> wrote:
>> > My only concern here is that the constants are named
>> > *MMU*_DATA_... whereas these are physical memory accesses not
>> > involving the MMU. I can't actually see any current users of
>> > MMUAccessType which makes me a bit confused as to what it's intended
>> > meaning was
>>
>> If you grep for MMU_DATA_LOAD/MMU_DATA_STORE/MMU_INST_FETCH
>> you'll see the uses. A lot of the softmmu code uses the
>> convention of 0=read,1=write,2=insn (which developed I
>> think historically from a bool "is_write", which you'll
>> still see in some function argument names, that was
>> augmented to handle insn-fetch separately). The enum
>> gives us some symbolic names for the constant values.
>> (There's a proposed patch somewhere to change the
>> 'int is_write' arguments to actually use the enum type.)
>
> Ah, yes, I see. Still surprisingly few, actually.
Yeah, we didn't go through (yet) and update the legacy code,
just provided the common type so new code could use it.
> My concern about the potentially misleading name still stands.
I don't mind if we want to rename it, but I don't think we want
to have two types. This is all in the softmmu code, whether it's
in the physical-address parts or the virtual-address parts.
thanks
-- PMM
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 6/9] Convert cpu_memory_rw_debug to use MMUAccessType
2016-07-11 16:27 ` Peter Maydell
@ 2016-07-12 5:27 ` David Gibson
2016-07-12 16:05 ` Andrey Smirnov
0 siblings, 1 reply; 20+ messages in thread
From: David Gibson @ 2016-07-12 5:27 UTC (permalink / raw)
To: Peter Maydell; +Cc: Andrey Smirnov, QEMU Developers
[-- Attachment #1: Type: text/plain, Size: 1984 bytes --]
On Mon, Jul 11, 2016 at 05:27:50PM +0100, Peter Maydell wrote:
> On 11 July 2016 at 03:24, David Gibson <david@gibson.dropbear.id.au> wrote:
> > On Sun, Jul 10, 2016 at 08:32:32PM +0100, Peter Maydell wrote:
> >> On 8 July 2016 at 04:42, David Gibson <david@gibson.dropbear.id.au> wrote:
> >> > My only concern here is that the constants are named
> >> > *MMU*_DATA_... whereas these are physical memory accesses not
> >> > involving the MMU. I can't actually see any current users of
> >> > MMUAccessType which makes me a bit confused as to what it's intended
> >> > meaning was
> >>
> >> If you grep for MMU_DATA_LOAD/MMU_DATA_STORE/MMU_INST_FETCH
> >> you'll see the uses. A lot of the softmmu code uses the
> >> convention of 0=read,1=write,2=insn (which developed I
> >> think historically from a bool "is_write", which you'll
> >> still see in some function argument names, that was
> >> augmented to handle insn-fetch separately). The enum
> >> gives us some symbolic names for the constant values.
> >> (There's a proposed patch somewhere to change the
> >> 'int is_write' arguments to actually use the enum type.)
> >
> > Ah, yes, I see. Still surprisingly few, actually.
>
> Yeah, we didn't go through (yet) and update the legacy code,
> just provided the common type so new code could use it.
Ah, yes, I see.
> > My concern about the potentially misleading name still stands.
>
> I don't mind if we want to rename it, but I don't think we want
> to have two types. This is all in the softmmu code, whether it's
> in the physical-address parts or the virtual-address parts.
Right, I agree we shouldn't have two types.
I think we should rename the existing constants, though, since it
doesn't really have anything to do with the MMU.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 4/9] address_space_write_continue: Distill common code
2016-07-08 3:38 ` [Qemu-devel] [PATCH 4/9] address_space_write_continue: Distill common code David Gibson
@ 2016-07-12 15:28 ` Andrey Smirnov
2016-07-12 15:41 ` Peter Maydell
0 siblings, 1 reply; 20+ messages in thread
From: Andrey Smirnov @ 2016-07-12 15:28 UTC (permalink / raw)
To: David Gibson; +Cc: QEMU Developers
On Thu, Jul 7, 2016 at 8:38 PM, David Gibson
<david@gibson.dropbear.id.au> wrote:
> On Thu, Jul 07, 2016 at 04:33:38PM -0700, Andrey Smirnov wrote:
>> Move call to memory_region_dispatch_write() outside of swtich statement
>> since the only thing that is different about all of those call is
>> "length" argument and that matches value in "l". Also collapse switch
>> statement to occupy less vertical space.
>>
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>
> I'm not a big fan of putting multiple statements on a single line,
> especially flow control like break. But apart from that,
I don't usually do it either, except in simple cases like this where a
lot of vertical real estate can be gained without making the code more
difficult to read. All of that is IMHO, of course. I am not
particularly attached to this formatting so if people want me to
change this I'll happily oblige.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 4/9] address_space_write_continue: Distill common code
2016-07-12 15:28 ` Andrey Smirnov
@ 2016-07-12 15:41 ` Peter Maydell
2016-07-12 16:09 ` Andrey Smirnov
0 siblings, 1 reply; 20+ messages in thread
From: Peter Maydell @ 2016-07-12 15:41 UTC (permalink / raw)
To: Andrey Smirnov; +Cc: David Gibson, QEMU Developers
On 12 July 2016 at 16:28, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> On Thu, Jul 7, 2016 at 8:38 PM, David Gibson
> <david@gibson.dropbear.id.au> wrote:
>> On Thu, Jul 07, 2016 at 04:33:38PM -0700, Andrey Smirnov wrote:
>>> Move call to memory_region_dispatch_write() outside of swtich statement
>>> since the only thing that is different about all of those call is
>>> "length" argument and that matches value in "l". Also collapse switch
>>> statement to occupy less vertical space.
>>>
>>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>>
>> I'm not a big fan of putting multiple statements on a single line,
>> especially flow control like break. But apart from that,
>
> I don't usually do it either, except in simple cases like this where a
> lot of vertical real estate can be gained without making the code more
> difficult to read. All of that is IMHO, of course. I am not
> particularly attached to this formatting so if people want me to
> change this I'll happily oblige.
We don't generally do multi-statement lines (does checkpatch
complain about this), so I'd favour retaining the newlines.
Not a big deal either way though.
thanks
-- PMM
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 7/9] Convert address_space_rw to use MMUAccessType
2016-07-08 3:45 ` [Qemu-devel] [PATCH 7/9] Convert address_space_rw " David Gibson
@ 2016-07-12 15:41 ` Andrey Smirnov
0 siblings, 0 replies; 20+ messages in thread
From: Andrey Smirnov @ 2016-07-12 15:41 UTC (permalink / raw)
To: David Gibson; +Cc: QEMU Developers
>> }
>>
>> enum write_rom_type {
>> @@ -3643,7 +3646,7 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
>> } else {
>> address_space_rw(cpu->cpu_ases[asidx].as, phys_addr,
>> MEMTXATTRS_UNSPECIFIED,
>> - buf, l, 0);
>> + buf, l, access_type);
>
> Replacing a fixed 0 with access_type looks wrong here, but I don't
> have the context readily to hand to be sure.
In previous patch in the series the "if" part of that statement was
converted to if (access_type == MMU_DATA_STORE), so in the "else"
branch access_type is guaranteed to be MMU_DATA_LOAD (there's also a
g_assert making sure those two are only possible options). Using
"access_type" instead of actual constant here makes the following
patch in which the whole if statement is replaced with just a call to
"address_space_rw" a bit simpler (or at least so I hope).
Andrey
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 6/9] Convert cpu_memory_rw_debug to use MMUAccessType
2016-07-12 5:27 ` David Gibson
@ 2016-07-12 16:05 ` Andrey Smirnov
2016-07-13 0:54 ` David Gibson
0 siblings, 1 reply; 20+ messages in thread
From: Andrey Smirnov @ 2016-07-12 16:05 UTC (permalink / raw)
To: David Gibson; +Cc: Peter Maydell, QEMU Developers
On Mon, Jul 11, 2016 at 10:27 PM, David Gibson
<david@gibson.dropbear.id.au> wrote:
> On Mon, Jul 11, 2016 at 05:27:50PM +0100, Peter Maydell wrote:
>> On 11 July 2016 at 03:24, David Gibson <david@gibson.dropbear.id.au> wrote:
>> > On Sun, Jul 10, 2016 at 08:32:32PM +0100, Peter Maydell wrote:
>> >> On 8 July 2016 at 04:42, David Gibson <david@gibson.dropbear.id.au> wrote:
>> >> > My only concern here is that the constants are named
>> >> > *MMU*_DATA_... whereas these are physical memory accesses not
>> >> > involving the MMU. I can't actually see any current users of
>> >> > MMUAccessType which makes me a bit confused as to what it's intended
>> >> > meaning was
>> >>
>> >> If you grep for MMU_DATA_LOAD/MMU_DATA_STORE/MMU_INST_FETCH
>> >> you'll see the uses. A lot of the softmmu code uses the
>> >> convention of 0=read,1=write,2=insn (which developed I
>> >> think historically from a bool "is_write", which you'll
>> >> still see in some function argument names, that was
>> >> augmented to handle insn-fetch separately). The enum
>> >> gives us some symbolic names for the constant values.
>> >> (There's a proposed patch somewhere to change the
>> >> 'int is_write' arguments to actually use the enum type.)
>> >
>> > Ah, yes, I see. Still surprisingly few, actually.
>>
>> Yeah, we didn't go through (yet) and update the legacy code,
>> just provided the common type so new code could use it.
>
> Ah, yes, I see.
>
>> > My concern about the potentially misleading name still stands.
>>
>> I don't mind if we want to rename it, but I don't think we want
>> to have two types. This is all in the softmmu code, whether it's
>> in the physical-address parts or the virtual-address parts.
>
> Right, I agree we shouldn't have two types.
>
> I think we should rename the existing constants, though, since it
> doesn't really have anything to do with the MMU.
>
I agree with your concern about confusing naming, David, and think
those should be renamed as well. In the original version of this patch
I introduced my own enumeration which was duplication MMUAccessType,
so in his feedback Peter asked me to use MMUAccessType instead.
Please let me know how I should proceed with this series.
Thanks,
Andrey
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 4/9] address_space_write_continue: Distill common code
2016-07-12 15:41 ` Peter Maydell
@ 2016-07-12 16:09 ` Andrey Smirnov
0 siblings, 0 replies; 20+ messages in thread
From: Andrey Smirnov @ 2016-07-12 16:09 UTC (permalink / raw)
To: Peter Maydell; +Cc: David Gibson, QEMU Developers
On Tue, Jul 12, 2016 at 8:41 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 12 July 2016 at 16:28, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>> On Thu, Jul 7, 2016 at 8:38 PM, David Gibson
>> <david@gibson.dropbear.id.au> wrote:
>>> On Thu, Jul 07, 2016 at 04:33:38PM -0700, Andrey Smirnov wrote:
>>>> Move call to memory_region_dispatch_write() outside of swtich statement
>>>> since the only thing that is different about all of those call is
>>>> "length" argument and that matches value in "l". Also collapse switch
>>>> statement to occupy less vertical space.
>>>>
>>>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>>>
>>> I'm not a big fan of putting multiple statements on a single line,
>>> especially flow control like break. But apart from that,
>>
>> I don't usually do it either, except in simple cases like this where a
>> lot of vertical real estate can be gained without making the code more
>> difficult to read. All of that is IMHO, of course. I am not
>> particularly attached to this formatting so if people want me to
>> change this I'll happily oblige.
>
> We don't generally do multi-statement lines (does checkpatch
> complain about this), so I'd favour retaining the newlines.
> Not a big deal either way though.
I need to send the patches again anyway, since they were bounced by
nongnu.org due to some problems in e-mail header, so I'll make this
change and mark them v3 (plus any other changes that we decide are
needed).
Andrey
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 6/9] Convert cpu_memory_rw_debug to use MMUAccessType
2016-07-12 16:05 ` Andrey Smirnov
@ 2016-07-13 0:54 ` David Gibson
2016-07-13 9:52 ` Peter Maydell
0 siblings, 1 reply; 20+ messages in thread
From: David Gibson @ 2016-07-13 0:54 UTC (permalink / raw)
To: Andrey Smirnov; +Cc: Peter Maydell, QEMU Developers
[-- Attachment #1: Type: text/plain, Size: 2842 bytes --]
On Tue, Jul 12, 2016 at 09:05:24AM -0700, Andrey Smirnov wrote:
> On Mon, Jul 11, 2016 at 10:27 PM, David Gibson
> <david@gibson.dropbear.id.au> wrote:
> > On Mon, Jul 11, 2016 at 05:27:50PM +0100, Peter Maydell wrote:
> >> On 11 July 2016 at 03:24, David Gibson <david@gibson.dropbear.id.au> wrote:
> >> > On Sun, Jul 10, 2016 at 08:32:32PM +0100, Peter Maydell wrote:
> >> >> On 8 July 2016 at 04:42, David Gibson <david@gibson.dropbear.id.au> wrote:
> >> >> > My only concern here is that the constants are named
> >> >> > *MMU*_DATA_... whereas these are physical memory accesses not
> >> >> > involving the MMU. I can't actually see any current users of
> >> >> > MMUAccessType which makes me a bit confused as to what it's intended
> >> >> > meaning was
> >> >>
> >> >> If you grep for MMU_DATA_LOAD/MMU_DATA_STORE/MMU_INST_FETCH
> >> >> you'll see the uses. A lot of the softmmu code uses the
> >> >> convention of 0=read,1=write,2=insn (which developed I
> >> >> think historically from a bool "is_write", which you'll
> >> >> still see in some function argument names, that was
> >> >> augmented to handle insn-fetch separately). The enum
> >> >> gives us some symbolic names for the constant values.
> >> >> (There's a proposed patch somewhere to change the
> >> >> 'int is_write' arguments to actually use the enum type.)
> >> >
> >> > Ah, yes, I see. Still surprisingly few, actually.
> >>
> >> Yeah, we didn't go through (yet) and update the legacy code,
> >> just provided the common type so new code could use it.
> >
> > Ah, yes, I see.
> >
> >> > My concern about the potentially misleading name still stands.
> >>
> >> I don't mind if we want to rename it, but I don't think we want
> >> to have two types. This is all in the softmmu code, whether it's
> >> in the physical-address parts or the virtual-address parts.
> >
> > Right, I agree we shouldn't have two types.
> >
> > I think we should rename the existing constants, though, since it
> > doesn't really have anything to do with the MMU.
> >
>
> I agree with your concern about confusing naming, David, and think
> those should be renamed as well. In the original version of this patch
> I introduced my own enumeration which was duplication MMUAccessType,
> so in his feedback Peter asked me to use MMUAccessType instead.
>
> Please let me know how I should proceed with this series.
My preference would be to start the series with something which
renames MMUAccessType and its individual values to something more
generic, including changing the existing users.
But I can't speak to what Peter would prefer.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 6/9] Convert cpu_memory_rw_debug to use MMUAccessType
2016-07-13 0:54 ` David Gibson
@ 2016-07-13 9:52 ` Peter Maydell
2016-07-13 17:09 ` Andrey Smirnov
0 siblings, 1 reply; 20+ messages in thread
From: Peter Maydell @ 2016-07-13 9:52 UTC (permalink / raw)
To: David Gibson; +Cc: Andrey Smirnov, QEMU Developers
On 13 July 2016 at 01:54, David Gibson <david@gibson.dropbear.id.au> wrote:
> On Tue, Jul 12, 2016 at 09:05:24AM -0700, Andrey Smirnov wrote:
>> Please let me know how I should proceed with this series.
>
> My preference would be to start the series with something which
> renames MMUAccessType and its individual values to something more
> generic, including changing the existing users.
>
> But I can't speak to what Peter would prefer.
Yes, if we're going to rename the type then that's the way to do it.
thanks
-- PMM
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 6/9] Convert cpu_memory_rw_debug to use MMUAccessType
2016-07-13 9:52 ` Peter Maydell
@ 2016-07-13 17:09 ` Andrey Smirnov
0 siblings, 0 replies; 20+ messages in thread
From: Andrey Smirnov @ 2016-07-13 17:09 UTC (permalink / raw)
To: Peter Maydell; +Cc: David Gibson, QEMU Developers
On Wed, Jul 13, 2016 at 2:52 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 13 July 2016 at 01:54, David Gibson <david@gibson.dropbear.id.au> wrote:
>> On Tue, Jul 12, 2016 at 09:05:24AM -0700, Andrey Smirnov wrote:
>>> Please let me know how I should proceed with this series.
>>
>> My preference would be to start the series with something which
>> renames MMUAccessType and its individual values to something more
>> generic, including changing the existing users.
>>
>> But I can't speak to what Peter would prefer.
>
> Yes, if we're going to rename the type then that's the way to do it.
Any suggestions on naming?
Thanks,
Andrey
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2016-07-13 17:09 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1467934423-5997-1-git-send-email-andrew.smirnov@gmail.com>
[not found] ` <1467934423-5997-2-git-send-email-andrew.smirnov@gmail.com>
2016-07-08 3:27 ` [Qemu-devel] [PATCH 1/9] Avoid needless calls to address_space_rw() David Gibson
[not found] ` <1467934423-5997-3-git-send-email-andrew.smirnov@gmail.com>
2016-07-08 3:33 ` [Qemu-devel] [PATCH 2/9] Change signature of address_space_read() to avoid casting David Gibson
[not found] ` <1467934423-5997-4-git-send-email-andrew.smirnov@gmail.com>
2016-07-08 3:34 ` [Qemu-devel] [PATCH 3/9] Change signature of address_space_write() " David Gibson
[not found] ` <1467934423-5997-5-git-send-email-andrew.smirnov@gmail.com>
2016-07-08 3:38 ` [Qemu-devel] [PATCH 4/9] address_space_write_continue: Distill common code David Gibson
2016-07-12 15:28 ` Andrey Smirnov
2016-07-12 15:41 ` Peter Maydell
2016-07-12 16:09 ` Andrey Smirnov
[not found] ` <1467934423-5997-6-git-send-email-andrew.smirnov@gmail.com>
2016-07-08 3:39 ` [Qemu-devel] [PATCH 5/9] Change signature of cpu_memory_rw_debug() to avoid casting David Gibson
[not found] ` <1467934423-5997-7-git-send-email-andrew.smirnov@gmail.com>
2016-07-08 3:42 ` [Qemu-devel] [PATCH 6/9] Convert cpu_memory_rw_debug to use MMUAccessType David Gibson
2016-07-10 19:32 ` Peter Maydell
2016-07-11 2:24 ` David Gibson
2016-07-11 16:27 ` Peter Maydell
2016-07-12 5:27 ` David Gibson
2016-07-12 16:05 ` Andrey Smirnov
2016-07-13 0:54 ` David Gibson
2016-07-13 9:52 ` Peter Maydell
2016-07-13 17:09 ` Andrey Smirnov
[not found] ` <1467934423-5997-8-git-send-email-andrew.smirnov@gmail.com>
2016-07-08 3:45 ` [Qemu-devel] [PATCH 7/9] Convert address_space_rw " David Gibson
2016-07-12 15:41 ` Andrey Smirnov
[not found] ` <1467934423-5997-9-git-send-email-andrew.smirnov@gmail.com>
2016-07-08 3:46 ` [Qemu-devel] [PATCH 8/9] gdbstub: Convert target_memory_rw_debug " David Gibson
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).