* Re: [PATCH v2] Avoid address_space_rw() with a constant is_write argument
2020-02-18 11:24 [PATCH v2] Avoid address_space_rw() with a constant is_write argument Peter Maydell
@ 2020-02-16 17:24 ` Edgar E. Iglesias
2020-02-18 12:56 ` Philippe Mathieu-Daudé
` (6 subsequent siblings)
7 siblings, 0 replies; 14+ messages in thread
From: Edgar E. Iglesias @ 2020-02-16 17:24 UTC (permalink / raw)
To: Peter Maydell
Cc: Laurent Vivier, Thomas Huth, Alistair Francis, Eduardo Habkost,
Cornelia Huck, qemu-devel, Halil Pasic, Christian Borntraeger,
Cédric Le Goater, Paolo Bonzini, David Gibson
On Tue, Feb 18, 2020 at 11:24:57AM +0000, Peter Maydell wrote:
> The address_space_rw() function allows either reads or writes
> depending on the is_write argument passed to it; this is useful
> when the direction of the access is determined programmatically
> (as for instance when handling the KVM_EXIT_MMIO exit reason).
> Under the hood it just calls either address_space_write() or
> address_space_read_full().
>
> We also use it a lot with a constant is_write argument, though,
> which has two issues:
> * when reading "address_space_rw(..., 1)" this is less
> immediately clear to the reader as being a write than
> "address_space_write(...)"
> * calling address_space_rw() bypasses the optimization
> in address_space_read() that fast-paths reads of a
> fixed length
>
> This commit was produced with the included Coccinelle script
> scripts/coccinelle/as-rw-const.patch.
>
> Two lines in hw/net/dp8393x.c that Coccinelle produced that
> were over 80 characters were re-wrapped by hand.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I could break this down into separate patches by submaintainer,
> but the patch is not that large and I would argue that it's
> better for the project if we can try to avoid introducing too
> much friction into the process of doing 'safe' tree-wide
> minor refactorings.
For xlnx-zdma:
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>
> v1->v2: put the coccinelle script in scripts/coccinelle rather
> than just in the commit message.
> ---
> accel/kvm/kvm-all.c | 6 +--
> dma-helpers.c | 4 +-
> exec.c | 4 +-
> hw/dma/xlnx-zdma.c | 11 ++---
> hw/net/dp8393x.c | 68 ++++++++++++++--------------
> hw/net/i82596.c | 25 +++++-----
> hw/net/lasi_i82596.c | 5 +-
> hw/ppc/pnv_lpc.c | 8 ++--
> hw/s390x/css.c | 12 ++---
> qtest.c | 52 ++++++++++-----------
> target/i386/hvf/x86_mmu.c | 12 ++---
> scripts/coccinelle/as_rw_const.cocci | 30 ++++++++++++
> 12 files changed, 133 insertions(+), 104 deletions(-)
> create mode 100644 scripts/coccinelle/as_rw_const.cocci
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index c111312dfdd..0cfe6fd8ded 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2178,9 +2178,9 @@ void kvm_flush_coalesced_mmio_buffer(void)
> ent = &ring->coalesced_mmio[ring->first];
>
> if (ent->pio == 1) {
> - address_space_rw(&address_space_io, ent->phys_addr,
> - MEMTXATTRS_UNSPECIFIED, ent->data,
> - ent->len, true);
> + address_space_write(&address_space_io, ent->phys_addr,
> + MEMTXATTRS_UNSPECIFIED, ent->data,
> + ent->len);
> } else {
> cpu_physical_memory_write(ent->phys_addr, ent->data, ent->len);
> }
> diff --git a/dma-helpers.c b/dma-helpers.c
> index d3871dc61ea..e8a26e81e16 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 8e9cc3b47cf..baefe582393 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -3810,8 +3810,8 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
> address_space_write_rom(cpu->cpu_ases[asidx].as, phys_addr,
> attrs, buf, l);
> } else {
> - address_space_rw(cpu->cpu_ases[asidx].as, phys_addr,
> - attrs, buf, l, 0);
> + address_space_read(cpu->cpu_ases[asidx].as, phys_addr, attrs, buf,
> + l);
> }
> len -= l;
> buf += l;
> diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c
> index 8fb83f5b078..31936061e21 100644
> --- a/hw/dma/xlnx-zdma.c
> +++ b/hw/dma/xlnx-zdma.c
> @@ -311,8 +311,7 @@ static bool zdma_load_descriptor(XlnxZDMA *s, uint64_t addr, void *buf)
> return false;
> }
>
> - address_space_rw(s->dma_as, addr, s->attr,
> - buf, sizeof(XlnxZDMADescr), false);
> + address_space_read(s->dma_as, addr, s->attr, buf, sizeof(XlnxZDMADescr));
> return true;
> }
>
> @@ -364,7 +363,7 @@ static uint64_t zdma_update_descr_addr(XlnxZDMA *s, bool type,
> } else {
> addr = zdma_get_regaddr64(s, basereg);
> addr += sizeof(s->dsc_dst);
> - address_space_rw(s->dma_as, addr, s->attr, (void *) &next, 8, false);
> + address_space_read(s->dma_as, addr, s->attr, (void *)&next, 8);
> zdma_put_regaddr64(s, basereg, next);
> }
> return next;
> @@ -416,8 +415,7 @@ static void zdma_write_dst(XlnxZDMA *s, uint8_t *buf, uint32_t len)
> }
> }
>
> - address_space_rw(s->dma_as, s->dsc_dst.addr, s->attr, buf, dlen,
> - true);
> + address_space_write(s->dma_as, s->dsc_dst.addr, s->attr, buf, dlen);
> if (burst_type == AXI_BURST_INCR) {
> s->dsc_dst.addr += dlen;
> }
> @@ -493,8 +491,7 @@ static void zdma_process_descr(XlnxZDMA *s)
> len = s->cfg.bus_width / 8;
> }
> } else {
> - address_space_rw(s->dma_as, src_addr, s->attr, s->buf, len,
> - false);
> + address_space_read(s->dma_as, src_addr, s->attr, s->buf, len);
> if (burst_type == AXI_BURST_INCR) {
> src_addr += len;
> }
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index a134d431ae3..f5f1c669e80 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -275,8 +275,8 @@ static void dp8393x_do_load_cam(dp8393xState *s)
>
> while (s->regs[SONIC_CDC] & 0x1f) {
> /* Fill current entry */
> - address_space_rw(&s->as, dp8393x_cdp(s),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, dp8393x_cdp(s), MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
> s->cam[index][0] = dp8393x_get(s, width, 1) & 0xff;
> s->cam[index][1] = dp8393x_get(s, width, 1) >> 8;
> s->cam[index][2] = dp8393x_get(s, width, 2) & 0xff;
> @@ -293,8 +293,8 @@ static void dp8393x_do_load_cam(dp8393xState *s)
> }
>
> /* Read CAM enable */
> - address_space_rw(&s->as, dp8393x_cdp(s),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, dp8393x_cdp(s), MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
> s->regs[SONIC_CE] = dp8393x_get(s, width, 0);
> DPRINTF("load cam done. cam enable mask 0x%04x\n", s->regs[SONIC_CE]);
>
> @@ -311,8 +311,8 @@ 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, dp8393x_rrp(s),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, dp8393x_rrp(s), MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
>
> /* Update SONIC registers */
> s->regs[SONIC_CRBA0] = dp8393x_get(s, width, 0);
> @@ -426,8 +426,8 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
> size = sizeof(uint16_t) * 6 * width;
> s->regs[SONIC_TTDA] = s->regs[SONIC_CTDA];
> DPRINTF("Transmit packet at %08x\n", dp8393x_ttda(s));
> - address_space_rw(&s->as, dp8393x_ttda(s) + sizeof(uint16_t) * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, dp8393x_ttda(s) + sizeof(uint16_t) * width,
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size);
> tx_len = 0;
>
> /* Update registers */
> @@ -451,17 +451,19 @@ 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, dp8393x_tsa(s),
> - MEMTXATTRS_UNSPECIFIED, &s->tx_buffer[tx_len], len, 0);
> + address_space_read(&s->as, dp8393x_tsa(s), 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,
> - dp8393x_ttda(s) + sizeof(uint16_t) * (4 + 3 * i) * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as,
> + dp8393x_ttda(s)
> + + sizeof(uint16_t) * (4 + 3 * i) * width,
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data,
> + size);
> s->regs[SONIC_TSA0] = dp8393x_get(s, width, 0);
> s->regs[SONIC_TSA1] = dp8393x_get(s, width, 1);
> s->regs[SONIC_TFS] = dp8393x_get(s, width, 2);
> @@ -494,18 +496,18 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
> dp8393x_put(s, width, 0,
> s->regs[SONIC_TCR] & 0x0fff); /* status */
> size = sizeof(uint16_t) * width;
> - address_space_rw(&s->as,
> - dp8393x_ttda(s),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 1);
> + address_space_write(&s->as, dp8393x_ttda(s), MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
>
> if (!(s->regs[SONIC_CR] & SONIC_CR_HTX)) {
> /* Read footer of packet */
> size = sizeof(uint16_t) * width;
> - address_space_rw(&s->as,
> - dp8393x_ttda(s) +
> - sizeof(uint16_t) *
> - (4 + 3 * s->regs[SONIC_TFC]) * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as,
> + dp8393x_ttda(s)
> + + sizeof(uint16_t) * (4 + 3 * s->regs[SONIC_TFC])
> + * width,
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data,
> + size);
> s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0) & ~0x1;
> if (dp8393x_get(s, width, 0) & 0x1) {
> /* EOL detected */
> @@ -767,8 +769,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 = dp8393x_crda(s) + sizeof(uint16_t) * 5 * width;
> - address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
> if (dp8393x_get(s, width, 0) & 0x1) {
> /* Still EOL ; stop reception */
> return -1;
> @@ -787,11 +789,11 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> /* Put packet into RBA */
> DPRINTF("Receive packet at %08x\n", dp8393x_crba(s));
> address = dp8393x_crba(s);
> - 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;
> @@ -819,13 +821,13 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> dp8393x_put(s, width, 3, s->regs[SONIC_TRBA1]); /* pkt_ptr1 */
> dp8393x_put(s, width, 4, s->regs[SONIC_RSC]); /* seq_no */
> size = sizeof(uint16_t) * 5 * width;
> - address_space_rw(&s->as, dp8393x_crda(s),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 1);
> + address_space_write(&s->as, dp8393x_crda(s), MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
>
> /* Move to next descriptor */
> size = sizeof(uint16_t) * width;
> - address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 5 * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 5 * width,
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size);
> s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0);
> if (s->regs[SONIC_LLFA] & 0x1) {
> /* EOL detected */
> @@ -838,8 +840,8 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> offset += sizeof(uint16_t);
> }
> s->data[0] = 0;
> - address_space_rw(&s->as, offset, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *)s->data, sizeof(uint16_t), 1);
> + address_space_write(&s->as, offset, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->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/net/i82596.c b/hw/net/i82596.c
> index 3a0e1ec4c05..6a80c24af23 100644
> --- a/hw/net/i82596.c
> +++ b/hw/net/i82596.c
> @@ -148,8 +148,8 @@ static void i82596_transmit(I82596State *s, uint32_t addr)
>
> if (s->nic && len) {
> assert(len <= sizeof(s->tx_buffer));
> - address_space_rw(&address_space_memory, tba,
> - MEMTXATTRS_UNSPECIFIED, s->tx_buffer, len, 0);
> + address_space_read(&address_space_memory, tba,
> + MEMTXATTRS_UNSPECIFIED, s->tx_buffer, len);
> DBG(PRINT_PKTHDR("Send", &s->tx_buffer));
> DBG(printf("Sending %d bytes\n", len));
> qemu_send_packet(qemu_get_queue(s->nic), s->tx_buffer, len);
> @@ -172,8 +172,8 @@ static void set_individual_address(I82596State *s, uint32_t addr)
>
> nc = qemu_get_queue(s->nic);
> m = s->conf.macaddr.a;
> - address_space_rw(&address_space_memory, addr + 8,
> - MEMTXATTRS_UNSPECIFIED, m, ETH_ALEN, 0);
> + address_space_read(&address_space_memory, addr + 8,
> + MEMTXATTRS_UNSPECIFIED, m, ETH_ALEN);
> qemu_format_nic_info_str(nc, m);
> trace_i82596_new_mac(nc->info_str);
> }
> @@ -190,9 +190,8 @@ static void set_multicast_list(I82596State *s, uint32_t addr)
> }
> for (i = 0; i < mc_count; i++) {
> uint8_t multicast_addr[ETH_ALEN];
> - address_space_rw(&address_space_memory,
> - addr + i * ETH_ALEN, MEMTXATTRS_UNSPECIFIED,
> - multicast_addr, ETH_ALEN, 0);
> + address_space_read(&address_space_memory, addr + i * ETH_ALEN,
> + MEMTXATTRS_UNSPECIFIED, multicast_addr, ETH_ALEN);
> DBG(printf("Add multicast entry " MAC_FMT "\n",
> MAC_ARG(multicast_addr)));
> unsigned mcast_idx = (net_crc32(multicast_addr, ETH_ALEN) &
> @@ -260,8 +259,8 @@ static void command_loop(I82596State *s)
> byte_cnt = MAX(byte_cnt, 4);
> byte_cnt = MIN(byte_cnt, sizeof(s->config));
> /* copy byte_cnt max. */
> - address_space_rw(&address_space_memory, s->cmd_p + 8,
> - MEMTXATTRS_UNSPECIFIED, s->config, byte_cnt, 0);
> + address_space_read(&address_space_memory, s->cmd_p + 8,
> + MEMTXATTRS_UNSPECIFIED, s->config, byte_cnt);
> /* config byte according to page 35ff */
> s->config[2] &= 0x82; /* mask valid bits */
> s->config[2] |= 0x40;
> @@ -640,14 +639,14 @@ ssize_t i82596_receive(NetClientState *nc, const uint8_t *buf, size_t sz)
> }
> rba = get_uint32(rbd + 8);
> /* printf("rba is 0x%x\n", rba); */
> - address_space_rw(&address_space_memory, rba,
> - MEMTXATTRS_UNSPECIFIED, (void *)buf, num, 1);
> + address_space_write(&address_space_memory, rba,
> + MEMTXATTRS_UNSPECIFIED, (void *)buf, num);
> rba += num;
> buf += num;
> len -= num;
> if (len == 0) { /* copy crc */
> - address_space_rw(&address_space_memory, rba - 4,
> - MEMTXATTRS_UNSPECIFIED, crc_ptr, 4, 1);
> + address_space_write(&address_space_memory, rba - 4,
> + MEMTXATTRS_UNSPECIFIED, crc_ptr, 4);
> }
>
> num |= 0x4000; /* set F BIT */
> diff --git a/hw/net/lasi_i82596.c b/hw/net/lasi_i82596.c
> index 427b3fbf701..52637a562d8 100644
> --- a/hw/net/lasi_i82596.c
> +++ b/hw/net/lasi_i82596.c
> @@ -55,8 +55,9 @@ static void lasi_82596_mem_write(void *opaque, hwaddr addr,
> * Provided for SeaBIOS only. Write MAC of Network card to addr @val.
> * Needed for the PDC_LAN_STATION_ID_READ PDC call.
> */
> - address_space_rw(&address_space_memory, val,
> - MEMTXATTRS_UNSPECIFIED, d->state.conf.macaddr.a, ETH_ALEN, 1);
> + address_space_write(&address_space_memory, val,
> + MEMTXATTRS_UNSPECIFIED, d->state.conf.macaddr.a,
> + ETH_ALEN);
> break;
> }
> }
> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
> index 5989d723c50..f150deca340 100644
> --- a/hw/ppc/pnv_lpc.c
> +++ b/hw/ppc/pnv_lpc.c
> @@ -238,16 +238,16 @@ static bool opb_read(PnvLpcController *lpc, uint32_t addr, uint8_t *data,
> int sz)
> {
> /* XXX Handle access size limits and FW read caching here */
> - return !address_space_rw(&lpc->opb_as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, sz, false);
> + return !address_space_read(&lpc->opb_as, addr, MEMTXATTRS_UNSPECIFIED,
> + data, sz);
> }
>
> static bool opb_write(PnvLpcController *lpc, uint32_t addr, uint8_t *data,
> int sz)
> {
> /* XXX Handle access size limits here */
> - return !address_space_rw(&lpc->opb_as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, sz, true);
> + return !address_space_write(&lpc->opb_as, addr, MEMTXATTRS_UNSPECIFIED,
> + data, sz);
> }
>
> #define ECCB_CTL_READ PPC_BIT(15)
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 844caab4082..0e0fccd050e 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -874,18 +874,18 @@ static inline int ida_read_next_idaw(CcwDataStream *cds)
> if (idaw_addr & 0x07 || !cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
> return -EINVAL; /* channel program check */
> }
> - ret = address_space_rw(&address_space_memory, idaw_addr,
> - MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
> - sizeof(idaw.fmt2), false);
> + ret = address_space_read(&address_space_memory, idaw_addr,
> + MEMTXATTRS_UNSPECIFIED, (void *)&idaw.fmt2,
> + sizeof(idaw.fmt2));
> cds->cda = be64_to_cpu(idaw.fmt2);
> } else {
> idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
> if (idaw_addr & 0x03 || !cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
> return -EINVAL; /* channel program check */
> }
> - ret = address_space_rw(&address_space_memory, idaw_addr,
> - MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,
> - sizeof(idaw.fmt1), false);
> + ret = address_space_read(&address_space_memory, idaw_addr,
> + MEMTXATTRS_UNSPECIFIED, (void *)&idaw.fmt1,
> + sizeof(idaw.fmt1));
> cds->cda = be64_to_cpu(idaw.fmt1);
> if (cds->cda & 0x80000000) {
> return -EINVAL; /* channel program check */
> diff --git a/qtest.c b/qtest.c
> index 12432f99cf4..328d674bcc8 100644
> --- a/qtest.c
> +++ b/qtest.c
> @@ -429,23 +429,23 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
>
> if (words[0][5] == 'b') {
> uint8_t data = value;
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - &data, 1, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + &data, 1);
> } else if (words[0][5] == 'w') {
> uint16_t data = value;
> tswap16s(&data);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &data, 2, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&data, 2);
> } else if (words[0][5] == 'l') {
> uint32_t data = value;
> tswap32s(&data);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &data, 4, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&data, 4);
> } else if (words[0][5] == 'q') {
> uint64_t data = value;
> tswap64s(&data);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &data, 8, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&data, 8);
> }
> qtest_send_prefix(chr);
> qtest_send(chr, "OK\n");
> @@ -463,22 +463,22 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
>
> if (words[0][4] == 'b') {
> uint8_t data;
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - &data, 1, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + &data, 1);
> value = data;
> } else if (words[0][4] == 'w') {
> uint16_t data;
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &data, 2, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&data, 2);
> value = tswap16(data);
> } else if (words[0][4] == 'l') {
> uint32_t data;
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &data, 4, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&data, 4);
> value = tswap32(data);
> } else if (words[0][4] == 'q') {
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &value, 8, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&value, 8);
> tswap64s(&value);
> }
> qtest_send_prefix(chr);
> @@ -498,8 +498,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
> g_assert(len);
>
> data = g_malloc(len);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, len, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
> + len);
>
> enc = g_malloc(2 * len + 1);
> for (i = 0; i < len; i++) {
> @@ -524,8 +524,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
> g_assert(ret == 0);
>
> data = g_malloc(len);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, len, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
> + len);
> b64_data = g_base64_encode(data, len);
> qtest_send_prefix(chr);
> qtest_sendf(chr, "OK %s\n", b64_data);
> @@ -559,8 +559,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
> data[i] = 0;
> }
> }
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, len, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
> + len);
> g_free(data);
>
> qtest_send_prefix(chr);
> @@ -582,8 +582,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
> if (len) {
> data = g_malloc(len);
> memset(data, pattern, len);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, len, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + data, len);
> g_free(data);
> }
>
> @@ -616,8 +616,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
> out_len = MIN(out_len, len);
> }
>
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, len, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
> + len);
>
> qtest_send_prefix(chr);
> qtest_send(chr, "OK\n");
> diff --git a/target/i386/hvf/x86_mmu.c b/target/i386/hvf/x86_mmu.c
> index d5a0efe7188..ff016fc0145 100644
> --- a/target/i386/hvf/x86_mmu.c
> +++ b/target/i386/hvf/x86_mmu.c
> @@ -88,8 +88,8 @@ static bool get_pt_entry(struct CPUState *cpu, struct gpt_translation *pt,
> }
>
> index = gpt_entry(pt->gva, level, pae);
> - address_space_rw(&address_space_memory, gpa + index * pte_size(pae),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)&pte, pte_size(pae), 0);
> + address_space_read(&address_space_memory, gpa + index * pte_size(pae),
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)&pte, pte_size(pae));
>
> pt->pte[level - 1] = pte;
>
> @@ -238,8 +238,8 @@ void vmx_write_mem(struct CPUState *cpu, target_ulong gva, void *data, int bytes
> if (!mmu_gva_to_gpa(cpu, gva, &gpa)) {
> VM_PANIC_EX("%s: mmu_gva_to_gpa %llx failed\n", __func__, gva);
> } else {
> - address_space_rw(&address_space_memory, gpa, MEMTXATTRS_UNSPECIFIED,
> - data, copy, 1);
> + address_space_write(&address_space_memory, gpa,
> + MEMTXATTRS_UNSPECIFIED, data, copy);
> }
>
> bytes -= copy;
> @@ -259,8 +259,8 @@ void vmx_read_mem(struct CPUState *cpu, void *data, target_ulong gva, int bytes)
> if (!mmu_gva_to_gpa(cpu, gva, &gpa)) {
> VM_PANIC_EX("%s: mmu_gva_to_gpa %llx failed\n", __func__, gva);
> }
> - address_space_rw(&address_space_memory, gpa, MEMTXATTRS_UNSPECIFIED,
> - data, copy, 0);
> + address_space_read(&address_space_memory, gpa, MEMTXATTRS_UNSPECIFIED,
> + data, copy);
>
> bytes -= copy;
> gva += copy;
> diff --git a/scripts/coccinelle/as_rw_const.cocci b/scripts/coccinelle/as_rw_const.cocci
> new file mode 100644
> index 00000000000..30da707701b
> --- /dev/null
> +++ b/scripts/coccinelle/as_rw_const.cocci
> @@ -0,0 +1,30 @@
> +// Avoid uses of address_space_rw() with a constant is_write argument.
> +// Usage:
> +// spatch --sp-file as-rw-const.spatch --dir . --in-place
> +
> +@@
> +expression E1, E2, E3, E4, E5;
> +symbol false;
> +@@
> +
> +- address_space_rw(E1, E2, E3, E4, E5, false)
> ++ address_space_read(E1, E2, E3, E4, E5)
> +@@
> +expression E1, E2, E3, E4, E5;
> +@@
> +
> +- address_space_rw(E1, E2, E3, E4, E5, 0)
> ++ address_space_read(E1, E2, E3, E4, E5)
> +@@
> +expression E1, E2, E3, E4, E5;
> +symbol true;
> +@@
> +
> +- address_space_rw(E1, E2, E3, E4, E5, true)
> ++ address_space_write(E1, E2, E3, E4, E5)
> +@@
> +expression E1, E2, E3, E4, E5;
> +@@
> +
> +- address_space_rw(E1, E2, E3, E4, E5, 1)
> ++ address_space_write(E1, E2, E3, E4, E5)
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2] Avoid address_space_rw() with a constant is_write argument
@ 2020-02-18 11:24 Peter Maydell
2020-02-16 17:24 ` Edgar E. Iglesias
` (7 more replies)
0 siblings, 8 replies; 14+ messages in thread
From: Peter Maydell @ 2020-02-18 11:24 UTC (permalink / raw)
To: qemu-devel
Cc: Laurent Vivier, Thomas Huth, Alistair Francis, Eduardo Habkost,
Cornelia Huck, Halil Pasic, Christian Borntraeger,
Cédric Le Goater, Edgar E. Iglesias, Paolo Bonzini,
David Gibson
The address_space_rw() function allows either reads or writes
depending on the is_write argument passed to it; this is useful
when the direction of the access is determined programmatically
(as for instance when handling the KVM_EXIT_MMIO exit reason).
Under the hood it just calls either address_space_write() or
address_space_read_full().
We also use it a lot with a constant is_write argument, though,
which has two issues:
* when reading "address_space_rw(..., 1)" this is less
immediately clear to the reader as being a write than
"address_space_write(...)"
* calling address_space_rw() bypasses the optimization
in address_space_read() that fast-paths reads of a
fixed length
This commit was produced with the included Coccinelle script
scripts/coccinelle/as-rw-const.patch.
Two lines in hw/net/dp8393x.c that Coccinelle produced that
were over 80 characters were re-wrapped by hand.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I could break this down into separate patches by submaintainer,
but the patch is not that large and I would argue that it's
better for the project if we can try to avoid introducing too
much friction into the process of doing 'safe' tree-wide
minor refactorings.
v1->v2: put the coccinelle script in scripts/coccinelle rather
than just in the commit message.
---
accel/kvm/kvm-all.c | 6 +--
dma-helpers.c | 4 +-
exec.c | 4 +-
hw/dma/xlnx-zdma.c | 11 ++---
hw/net/dp8393x.c | 68 ++++++++++++++--------------
hw/net/i82596.c | 25 +++++-----
hw/net/lasi_i82596.c | 5 +-
hw/ppc/pnv_lpc.c | 8 ++--
hw/s390x/css.c | 12 ++---
qtest.c | 52 ++++++++++-----------
target/i386/hvf/x86_mmu.c | 12 ++---
scripts/coccinelle/as_rw_const.cocci | 30 ++++++++++++
12 files changed, 133 insertions(+), 104 deletions(-)
create mode 100644 scripts/coccinelle/as_rw_const.cocci
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index c111312dfdd..0cfe6fd8ded 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2178,9 +2178,9 @@ void kvm_flush_coalesced_mmio_buffer(void)
ent = &ring->coalesced_mmio[ring->first];
if (ent->pio == 1) {
- address_space_rw(&address_space_io, ent->phys_addr,
- MEMTXATTRS_UNSPECIFIED, ent->data,
- ent->len, true);
+ address_space_write(&address_space_io, ent->phys_addr,
+ MEMTXATTRS_UNSPECIFIED, ent->data,
+ ent->len);
} else {
cpu_physical_memory_write(ent->phys_addr, ent->data, ent->len);
}
diff --git a/dma-helpers.c b/dma-helpers.c
index d3871dc61ea..e8a26e81e16 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 8e9cc3b47cf..baefe582393 100644
--- a/exec.c
+++ b/exec.c
@@ -3810,8 +3810,8 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
address_space_write_rom(cpu->cpu_ases[asidx].as, phys_addr,
attrs, buf, l);
} else {
- address_space_rw(cpu->cpu_ases[asidx].as, phys_addr,
- attrs, buf, l, 0);
+ address_space_read(cpu->cpu_ases[asidx].as, phys_addr, attrs, buf,
+ l);
}
len -= l;
buf += l;
diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c
index 8fb83f5b078..31936061e21 100644
--- a/hw/dma/xlnx-zdma.c
+++ b/hw/dma/xlnx-zdma.c
@@ -311,8 +311,7 @@ static bool zdma_load_descriptor(XlnxZDMA *s, uint64_t addr, void *buf)
return false;
}
- address_space_rw(s->dma_as, addr, s->attr,
- buf, sizeof(XlnxZDMADescr), false);
+ address_space_read(s->dma_as, addr, s->attr, buf, sizeof(XlnxZDMADescr));
return true;
}
@@ -364,7 +363,7 @@ static uint64_t zdma_update_descr_addr(XlnxZDMA *s, bool type,
} else {
addr = zdma_get_regaddr64(s, basereg);
addr += sizeof(s->dsc_dst);
- address_space_rw(s->dma_as, addr, s->attr, (void *) &next, 8, false);
+ address_space_read(s->dma_as, addr, s->attr, (void *)&next, 8);
zdma_put_regaddr64(s, basereg, next);
}
return next;
@@ -416,8 +415,7 @@ static void zdma_write_dst(XlnxZDMA *s, uint8_t *buf, uint32_t len)
}
}
- address_space_rw(s->dma_as, s->dsc_dst.addr, s->attr, buf, dlen,
- true);
+ address_space_write(s->dma_as, s->dsc_dst.addr, s->attr, buf, dlen);
if (burst_type == AXI_BURST_INCR) {
s->dsc_dst.addr += dlen;
}
@@ -493,8 +491,7 @@ static void zdma_process_descr(XlnxZDMA *s)
len = s->cfg.bus_width / 8;
}
} else {
- address_space_rw(s->dma_as, src_addr, s->attr, s->buf, len,
- false);
+ address_space_read(s->dma_as, src_addr, s->attr, s->buf, len);
if (burst_type == AXI_BURST_INCR) {
src_addr += len;
}
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index a134d431ae3..f5f1c669e80 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -275,8 +275,8 @@ static void dp8393x_do_load_cam(dp8393xState *s)
while (s->regs[SONIC_CDC] & 0x1f) {
/* Fill current entry */
- address_space_rw(&s->as, dp8393x_cdp(s),
- MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
+ address_space_read(&s->as, dp8393x_cdp(s), MEMTXATTRS_UNSPECIFIED,
+ (uint8_t *)s->data, size);
s->cam[index][0] = dp8393x_get(s, width, 1) & 0xff;
s->cam[index][1] = dp8393x_get(s, width, 1) >> 8;
s->cam[index][2] = dp8393x_get(s, width, 2) & 0xff;
@@ -293,8 +293,8 @@ static void dp8393x_do_load_cam(dp8393xState *s)
}
/* Read CAM enable */
- address_space_rw(&s->as, dp8393x_cdp(s),
- MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
+ address_space_read(&s->as, dp8393x_cdp(s), MEMTXATTRS_UNSPECIFIED,
+ (uint8_t *)s->data, size);
s->regs[SONIC_CE] = dp8393x_get(s, width, 0);
DPRINTF("load cam done. cam enable mask 0x%04x\n", s->regs[SONIC_CE]);
@@ -311,8 +311,8 @@ 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, dp8393x_rrp(s),
- MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
+ address_space_read(&s->as, dp8393x_rrp(s), MEMTXATTRS_UNSPECIFIED,
+ (uint8_t *)s->data, size);
/* Update SONIC registers */
s->regs[SONIC_CRBA0] = dp8393x_get(s, width, 0);
@@ -426,8 +426,8 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
size = sizeof(uint16_t) * 6 * width;
s->regs[SONIC_TTDA] = s->regs[SONIC_CTDA];
DPRINTF("Transmit packet at %08x\n", dp8393x_ttda(s));
- address_space_rw(&s->as, dp8393x_ttda(s) + sizeof(uint16_t) * width,
- MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
+ address_space_read(&s->as, dp8393x_ttda(s) + sizeof(uint16_t) * width,
+ MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size);
tx_len = 0;
/* Update registers */
@@ -451,17 +451,19 @@ 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, dp8393x_tsa(s),
- MEMTXATTRS_UNSPECIFIED, &s->tx_buffer[tx_len], len, 0);
+ address_space_read(&s->as, dp8393x_tsa(s), 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,
- dp8393x_ttda(s) + sizeof(uint16_t) * (4 + 3 * i) * width,
- MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
+ address_space_read(&s->as,
+ dp8393x_ttda(s)
+ + sizeof(uint16_t) * (4 + 3 * i) * width,
+ MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data,
+ size);
s->regs[SONIC_TSA0] = dp8393x_get(s, width, 0);
s->regs[SONIC_TSA1] = dp8393x_get(s, width, 1);
s->regs[SONIC_TFS] = dp8393x_get(s, width, 2);
@@ -494,18 +496,18 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
dp8393x_put(s, width, 0,
s->regs[SONIC_TCR] & 0x0fff); /* status */
size = sizeof(uint16_t) * width;
- address_space_rw(&s->as,
- dp8393x_ttda(s),
- MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 1);
+ address_space_write(&s->as, dp8393x_ttda(s), MEMTXATTRS_UNSPECIFIED,
+ (uint8_t *)s->data, size);
if (!(s->regs[SONIC_CR] & SONIC_CR_HTX)) {
/* Read footer of packet */
size = sizeof(uint16_t) * width;
- address_space_rw(&s->as,
- dp8393x_ttda(s) +
- sizeof(uint16_t) *
- (4 + 3 * s->regs[SONIC_TFC]) * width,
- MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
+ address_space_read(&s->as,
+ dp8393x_ttda(s)
+ + sizeof(uint16_t) * (4 + 3 * s->regs[SONIC_TFC])
+ * width,
+ MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data,
+ size);
s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0) & ~0x1;
if (dp8393x_get(s, width, 0) & 0x1) {
/* EOL detected */
@@ -767,8 +769,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 = dp8393x_crda(s) + sizeof(uint16_t) * 5 * width;
- address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
- (uint8_t *)s->data, size, 0);
+ address_space_read(&s->as, address, MEMTXATTRS_UNSPECIFIED,
+ (uint8_t *)s->data, size);
if (dp8393x_get(s, width, 0) & 0x1) {
/* Still EOL ; stop reception */
return -1;
@@ -787,11 +789,11 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
/* Put packet into RBA */
DPRINTF("Receive packet at %08x\n", dp8393x_crba(s));
address = dp8393x_crba(s);
- 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;
@@ -819,13 +821,13 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
dp8393x_put(s, width, 3, s->regs[SONIC_TRBA1]); /* pkt_ptr1 */
dp8393x_put(s, width, 4, s->regs[SONIC_RSC]); /* seq_no */
size = sizeof(uint16_t) * 5 * width;
- address_space_rw(&s->as, dp8393x_crda(s),
- MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 1);
+ address_space_write(&s->as, dp8393x_crda(s), MEMTXATTRS_UNSPECIFIED,
+ (uint8_t *)s->data, size);
/* Move to next descriptor */
size = sizeof(uint16_t) * width;
- address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 5 * width,
- MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
+ address_space_read(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 5 * width,
+ MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size);
s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0);
if (s->regs[SONIC_LLFA] & 0x1) {
/* EOL detected */
@@ -838,8 +840,8 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
offset += sizeof(uint16_t);
}
s->data[0] = 0;
- address_space_rw(&s->as, offset, MEMTXATTRS_UNSPECIFIED,
- (uint8_t *)s->data, sizeof(uint16_t), 1);
+ address_space_write(&s->as, offset, MEMTXATTRS_UNSPECIFIED,
+ (uint8_t *)s->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/net/i82596.c b/hw/net/i82596.c
index 3a0e1ec4c05..6a80c24af23 100644
--- a/hw/net/i82596.c
+++ b/hw/net/i82596.c
@@ -148,8 +148,8 @@ static void i82596_transmit(I82596State *s, uint32_t addr)
if (s->nic && len) {
assert(len <= sizeof(s->tx_buffer));
- address_space_rw(&address_space_memory, tba,
- MEMTXATTRS_UNSPECIFIED, s->tx_buffer, len, 0);
+ address_space_read(&address_space_memory, tba,
+ MEMTXATTRS_UNSPECIFIED, s->tx_buffer, len);
DBG(PRINT_PKTHDR("Send", &s->tx_buffer));
DBG(printf("Sending %d bytes\n", len));
qemu_send_packet(qemu_get_queue(s->nic), s->tx_buffer, len);
@@ -172,8 +172,8 @@ static void set_individual_address(I82596State *s, uint32_t addr)
nc = qemu_get_queue(s->nic);
m = s->conf.macaddr.a;
- address_space_rw(&address_space_memory, addr + 8,
- MEMTXATTRS_UNSPECIFIED, m, ETH_ALEN, 0);
+ address_space_read(&address_space_memory, addr + 8,
+ MEMTXATTRS_UNSPECIFIED, m, ETH_ALEN);
qemu_format_nic_info_str(nc, m);
trace_i82596_new_mac(nc->info_str);
}
@@ -190,9 +190,8 @@ static void set_multicast_list(I82596State *s, uint32_t addr)
}
for (i = 0; i < mc_count; i++) {
uint8_t multicast_addr[ETH_ALEN];
- address_space_rw(&address_space_memory,
- addr + i * ETH_ALEN, MEMTXATTRS_UNSPECIFIED,
- multicast_addr, ETH_ALEN, 0);
+ address_space_read(&address_space_memory, addr + i * ETH_ALEN,
+ MEMTXATTRS_UNSPECIFIED, multicast_addr, ETH_ALEN);
DBG(printf("Add multicast entry " MAC_FMT "\n",
MAC_ARG(multicast_addr)));
unsigned mcast_idx = (net_crc32(multicast_addr, ETH_ALEN) &
@@ -260,8 +259,8 @@ static void command_loop(I82596State *s)
byte_cnt = MAX(byte_cnt, 4);
byte_cnt = MIN(byte_cnt, sizeof(s->config));
/* copy byte_cnt max. */
- address_space_rw(&address_space_memory, s->cmd_p + 8,
- MEMTXATTRS_UNSPECIFIED, s->config, byte_cnt, 0);
+ address_space_read(&address_space_memory, s->cmd_p + 8,
+ MEMTXATTRS_UNSPECIFIED, s->config, byte_cnt);
/* config byte according to page 35ff */
s->config[2] &= 0x82; /* mask valid bits */
s->config[2] |= 0x40;
@@ -640,14 +639,14 @@ ssize_t i82596_receive(NetClientState *nc, const uint8_t *buf, size_t sz)
}
rba = get_uint32(rbd + 8);
/* printf("rba is 0x%x\n", rba); */
- address_space_rw(&address_space_memory, rba,
- MEMTXATTRS_UNSPECIFIED, (void *)buf, num, 1);
+ address_space_write(&address_space_memory, rba,
+ MEMTXATTRS_UNSPECIFIED, (void *)buf, num);
rba += num;
buf += num;
len -= num;
if (len == 0) { /* copy crc */
- address_space_rw(&address_space_memory, rba - 4,
- MEMTXATTRS_UNSPECIFIED, crc_ptr, 4, 1);
+ address_space_write(&address_space_memory, rba - 4,
+ MEMTXATTRS_UNSPECIFIED, crc_ptr, 4);
}
num |= 0x4000; /* set F BIT */
diff --git a/hw/net/lasi_i82596.c b/hw/net/lasi_i82596.c
index 427b3fbf701..52637a562d8 100644
--- a/hw/net/lasi_i82596.c
+++ b/hw/net/lasi_i82596.c
@@ -55,8 +55,9 @@ static void lasi_82596_mem_write(void *opaque, hwaddr addr,
* Provided for SeaBIOS only. Write MAC of Network card to addr @val.
* Needed for the PDC_LAN_STATION_ID_READ PDC call.
*/
- address_space_rw(&address_space_memory, val,
- MEMTXATTRS_UNSPECIFIED, d->state.conf.macaddr.a, ETH_ALEN, 1);
+ address_space_write(&address_space_memory, val,
+ MEMTXATTRS_UNSPECIFIED, d->state.conf.macaddr.a,
+ ETH_ALEN);
break;
}
}
diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
index 5989d723c50..f150deca340 100644
--- a/hw/ppc/pnv_lpc.c
+++ b/hw/ppc/pnv_lpc.c
@@ -238,16 +238,16 @@ static bool opb_read(PnvLpcController *lpc, uint32_t addr, uint8_t *data,
int sz)
{
/* XXX Handle access size limits and FW read caching here */
- return !address_space_rw(&lpc->opb_as, addr, MEMTXATTRS_UNSPECIFIED,
- data, sz, false);
+ return !address_space_read(&lpc->opb_as, addr, MEMTXATTRS_UNSPECIFIED,
+ data, sz);
}
static bool opb_write(PnvLpcController *lpc, uint32_t addr, uint8_t *data,
int sz)
{
/* XXX Handle access size limits here */
- return !address_space_rw(&lpc->opb_as, addr, MEMTXATTRS_UNSPECIFIED,
- data, sz, true);
+ return !address_space_write(&lpc->opb_as, addr, MEMTXATTRS_UNSPECIFIED,
+ data, sz);
}
#define ECCB_CTL_READ PPC_BIT(15)
diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 844caab4082..0e0fccd050e 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -874,18 +874,18 @@ static inline int ida_read_next_idaw(CcwDataStream *cds)
if (idaw_addr & 0x07 || !cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
return -EINVAL; /* channel program check */
}
- ret = address_space_rw(&address_space_memory, idaw_addr,
- MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
- sizeof(idaw.fmt2), false);
+ ret = address_space_read(&address_space_memory, idaw_addr,
+ MEMTXATTRS_UNSPECIFIED, (void *)&idaw.fmt2,
+ sizeof(idaw.fmt2));
cds->cda = be64_to_cpu(idaw.fmt2);
} else {
idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
if (idaw_addr & 0x03 || !cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
return -EINVAL; /* channel program check */
}
- ret = address_space_rw(&address_space_memory, idaw_addr,
- MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,
- sizeof(idaw.fmt1), false);
+ ret = address_space_read(&address_space_memory, idaw_addr,
+ MEMTXATTRS_UNSPECIFIED, (void *)&idaw.fmt1,
+ sizeof(idaw.fmt1));
cds->cda = be64_to_cpu(idaw.fmt1);
if (cds->cda & 0x80000000) {
return -EINVAL; /* channel program check */
diff --git a/qtest.c b/qtest.c
index 12432f99cf4..328d674bcc8 100644
--- a/qtest.c
+++ b/qtest.c
@@ -429,23 +429,23 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
if (words[0][5] == 'b') {
uint8_t data = value;
- address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
- &data, 1, true);
+ address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
+ &data, 1);
} else if (words[0][5] == 'w') {
uint16_t data = value;
tswap16s(&data);
- address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
- (uint8_t *) &data, 2, true);
+ address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
+ (uint8_t *)&data, 2);
} else if (words[0][5] == 'l') {
uint32_t data = value;
tswap32s(&data);
- address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
- (uint8_t *) &data, 4, true);
+ address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
+ (uint8_t *)&data, 4);
} else if (words[0][5] == 'q') {
uint64_t data = value;
tswap64s(&data);
- address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
- (uint8_t *) &data, 8, true);
+ address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
+ (uint8_t *)&data, 8);
}
qtest_send_prefix(chr);
qtest_send(chr, "OK\n");
@@ -463,22 +463,22 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
if (words[0][4] == 'b') {
uint8_t data;
- address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
- &data, 1, false);
+ address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
+ &data, 1);
value = data;
} else if (words[0][4] == 'w') {
uint16_t data;
- address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
- (uint8_t *) &data, 2, false);
+ address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
+ (uint8_t *)&data, 2);
value = tswap16(data);
} else if (words[0][4] == 'l') {
uint32_t data;
- address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
- (uint8_t *) &data, 4, false);
+ address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
+ (uint8_t *)&data, 4);
value = tswap32(data);
} else if (words[0][4] == 'q') {
- address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
- (uint8_t *) &value, 8, false);
+ address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
+ (uint8_t *)&value, 8);
tswap64s(&value);
}
qtest_send_prefix(chr);
@@ -498,8 +498,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
g_assert(len);
data = g_malloc(len);
- address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
- data, len, false);
+ address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
+ len);
enc = g_malloc(2 * len + 1);
for (i = 0; i < len; i++) {
@@ -524,8 +524,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
g_assert(ret == 0);
data = g_malloc(len);
- address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
- data, len, false);
+ address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
+ len);
b64_data = g_base64_encode(data, len);
qtest_send_prefix(chr);
qtest_sendf(chr, "OK %s\n", b64_data);
@@ -559,8 +559,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
data[i] = 0;
}
}
- address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
- data, len, true);
+ address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
+ len);
g_free(data);
qtest_send_prefix(chr);
@@ -582,8 +582,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
if (len) {
data = g_malloc(len);
memset(data, pattern, len);
- address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
- data, len, true);
+ address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
+ data, len);
g_free(data);
}
@@ -616,8 +616,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
out_len = MIN(out_len, len);
}
- address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
- data, len, true);
+ address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
+ len);
qtest_send_prefix(chr);
qtest_send(chr, "OK\n");
diff --git a/target/i386/hvf/x86_mmu.c b/target/i386/hvf/x86_mmu.c
index d5a0efe7188..ff016fc0145 100644
--- a/target/i386/hvf/x86_mmu.c
+++ b/target/i386/hvf/x86_mmu.c
@@ -88,8 +88,8 @@ static bool get_pt_entry(struct CPUState *cpu, struct gpt_translation *pt,
}
index = gpt_entry(pt->gva, level, pae);
- address_space_rw(&address_space_memory, gpa + index * pte_size(pae),
- MEMTXATTRS_UNSPECIFIED, (uint8_t *)&pte, pte_size(pae), 0);
+ address_space_read(&address_space_memory, gpa + index * pte_size(pae),
+ MEMTXATTRS_UNSPECIFIED, (uint8_t *)&pte, pte_size(pae));
pt->pte[level - 1] = pte;
@@ -238,8 +238,8 @@ void vmx_write_mem(struct CPUState *cpu, target_ulong gva, void *data, int bytes
if (!mmu_gva_to_gpa(cpu, gva, &gpa)) {
VM_PANIC_EX("%s: mmu_gva_to_gpa %llx failed\n", __func__, gva);
} else {
- address_space_rw(&address_space_memory, gpa, MEMTXATTRS_UNSPECIFIED,
- data, copy, 1);
+ address_space_write(&address_space_memory, gpa,
+ MEMTXATTRS_UNSPECIFIED, data, copy);
}
bytes -= copy;
@@ -259,8 +259,8 @@ void vmx_read_mem(struct CPUState *cpu, void *data, target_ulong gva, int bytes)
if (!mmu_gva_to_gpa(cpu, gva, &gpa)) {
VM_PANIC_EX("%s: mmu_gva_to_gpa %llx failed\n", __func__, gva);
}
- address_space_rw(&address_space_memory, gpa, MEMTXATTRS_UNSPECIFIED,
- data, copy, 0);
+ address_space_read(&address_space_memory, gpa, MEMTXATTRS_UNSPECIFIED,
+ data, copy);
bytes -= copy;
gva += copy;
diff --git a/scripts/coccinelle/as_rw_const.cocci b/scripts/coccinelle/as_rw_const.cocci
new file mode 100644
index 00000000000..30da707701b
--- /dev/null
+++ b/scripts/coccinelle/as_rw_const.cocci
@@ -0,0 +1,30 @@
+// Avoid uses of address_space_rw() with a constant is_write argument.
+// Usage:
+// spatch --sp-file as-rw-const.spatch --dir . --in-place
+
+@@
+expression E1, E2, E3, E4, E5;
+symbol false;
+@@
+
+- address_space_rw(E1, E2, E3, E4, E5, false)
++ address_space_read(E1, E2, E3, E4, E5)
+@@
+expression E1, E2, E3, E4, E5;
+@@
+
+- address_space_rw(E1, E2, E3, E4, E5, 0)
++ address_space_read(E1, E2, E3, E4, E5)
+@@
+expression E1, E2, E3, E4, E5;
+symbol true;
+@@
+
+- address_space_rw(E1, E2, E3, E4, E5, true)
++ address_space_write(E1, E2, E3, E4, E5)
+@@
+expression E1, E2, E3, E4, E5;
+@@
+
+- address_space_rw(E1, E2, E3, E4, E5, 1)
++ address_space_write(E1, E2, E3, E4, E5)
--
2.20.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2] Avoid address_space_rw() with a constant is_write argument
2020-02-18 11:24 [PATCH v2] Avoid address_space_rw() with a constant is_write argument Peter Maydell
2020-02-16 17:24 ` Edgar E. Iglesias
@ 2020-02-18 12:56 ` Philippe Mathieu-Daudé
2020-02-18 13:33 ` Eric Blake
2020-02-20 9:27 ` Philippe Mathieu-Daudé
2020-02-18 13:13 ` Laurent Vivier
` (5 subsequent siblings)
7 siblings, 2 replies; 14+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-02-18 12:56 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: Laurent Vivier, Thomas Huth, Cornelia Huck, Eduardo Habkost,
Alistair Francis, Halil Pasic, Christian Borntraeger,
Cédric Le Goater, Paolo Bonzini, Edgar E. Iglesias,
David Gibson
On 2/18/20 12:24 PM, Peter Maydell wrote:
> The address_space_rw() function allows either reads or writes
> depending on the is_write argument passed to it; this is useful
> when the direction of the access is determined programmatically
> (as for instance when handling the KVM_EXIT_MMIO exit reason).
> Under the hood it just calls either address_space_write() or
> address_space_read_full().
>
> We also use it a lot with a constant is_write argument, though,
> which has two issues:
> * when reading "address_space_rw(..., 1)" this is less
> immediately clear to the reader as being a write than
> "address_space_write(...)"
> * calling address_space_rw() bypasses the optimization
> in address_space_read() that fast-paths reads of a
> fixed length
>
> This commit was produced with the included Coccinelle script
> scripts/coccinelle/as-rw-const.patch.
>
> Two lines in hw/net/dp8393x.c that Coccinelle produced that
> were over 80 characters were re-wrapped by hand.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I could break this down into separate patches by submaintainer,
> but the patch is not that large and I would argue that it's
> better for the project if we can try to avoid introducing too
> much friction into the process of doing 'safe' tree-wide
> minor refactorings.
>
> v1->v2: put the coccinelle script in scripts/coccinelle rather
> than just in the commit message.
> ---
> accel/kvm/kvm-all.c | 6 +--
> dma-helpers.c | 4 +-
> exec.c | 4 +-
> hw/dma/xlnx-zdma.c | 11 ++---
> hw/net/dp8393x.c | 68 ++++++++++++++--------------
> hw/net/i82596.c | 25 +++++-----
> hw/net/lasi_i82596.c | 5 +-
> hw/ppc/pnv_lpc.c | 8 ++--
> hw/s390x/css.c | 12 ++---
> qtest.c | 52 ++++++++++-----------
> target/i386/hvf/x86_mmu.c | 12 ++---
> scripts/coccinelle/as_rw_const.cocci | 30 ++++++++++++
> 12 files changed, 133 insertions(+), 104 deletions(-)
> create mode 100644 scripts/coccinelle/as_rw_const.cocci
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index c111312dfdd..0cfe6fd8ded 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2178,9 +2178,9 @@ void kvm_flush_coalesced_mmio_buffer(void)
> ent = &ring->coalesced_mmio[ring->first];
>
> if (ent->pio == 1) {
> - address_space_rw(&address_space_io, ent->phys_addr,
> - MEMTXATTRS_UNSPECIFIED, ent->data,
> - ent->len, true);
> + address_space_write(&address_space_io, ent->phys_addr,
> + MEMTXATTRS_UNSPECIFIED, ent->data,
> + ent->len);
> } else {
> cpu_physical_memory_write(ent->phys_addr, ent->data, ent->len);
> }
> diff --git a/dma-helpers.c b/dma-helpers.c
> index d3871dc61ea..e8a26e81e16 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 8e9cc3b47cf..baefe582393 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -3810,8 +3810,8 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
> address_space_write_rom(cpu->cpu_ases[asidx].as, phys_addr,
> attrs, buf, l);
> } else {
> - address_space_rw(cpu->cpu_ases[asidx].as, phys_addr,
> - attrs, buf, l, 0);
> + address_space_read(cpu->cpu_ases[asidx].as, phys_addr, attrs, buf,
> + l);
> }
> len -= l;
> buf += l;
> diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c
> index 8fb83f5b078..31936061e21 100644
> --- a/hw/dma/xlnx-zdma.c
> +++ b/hw/dma/xlnx-zdma.c
> @@ -311,8 +311,7 @@ static bool zdma_load_descriptor(XlnxZDMA *s, uint64_t addr, void *buf)
> return false;
> }
>
> - address_space_rw(s->dma_as, addr, s->attr,
> - buf, sizeof(XlnxZDMADescr), false);
> + address_space_read(s->dma_as, addr, s->attr, buf, sizeof(XlnxZDMADescr));
> return true;
> }
>
> @@ -364,7 +363,7 @@ static uint64_t zdma_update_descr_addr(XlnxZDMA *s, bool type,
> } else {
> addr = zdma_get_regaddr64(s, basereg);
> addr += sizeof(s->dsc_dst);
> - address_space_rw(s->dma_as, addr, s->attr, (void *) &next, 8, false);
> + address_space_read(s->dma_as, addr, s->attr, (void *)&next, 8);
> zdma_put_regaddr64(s, basereg, next);
> }
> return next;
> @@ -416,8 +415,7 @@ static void zdma_write_dst(XlnxZDMA *s, uint8_t *buf, uint32_t len)
> }
> }
>
> - address_space_rw(s->dma_as, s->dsc_dst.addr, s->attr, buf, dlen,
> - true);
> + address_space_write(s->dma_as, s->dsc_dst.addr, s->attr, buf, dlen);
> if (burst_type == AXI_BURST_INCR) {
> s->dsc_dst.addr += dlen;
> }
> @@ -493,8 +491,7 @@ static void zdma_process_descr(XlnxZDMA *s)
> len = s->cfg.bus_width / 8;
> }
> } else {
> - address_space_rw(s->dma_as, src_addr, s->attr, s->buf, len,
> - false);
> + address_space_read(s->dma_as, src_addr, s->attr, s->buf, len);
> if (burst_type == AXI_BURST_INCR) {
> src_addr += len;
> }
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index a134d431ae3..f5f1c669e80 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -275,8 +275,8 @@ static void dp8393x_do_load_cam(dp8393xState *s)
>
> while (s->regs[SONIC_CDC] & 0x1f) {
> /* Fill current entry */
> - address_space_rw(&s->as, dp8393x_cdp(s),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, dp8393x_cdp(s), MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
> s->cam[index][0] = dp8393x_get(s, width, 1) & 0xff;
> s->cam[index][1] = dp8393x_get(s, width, 1) >> 8;
> s->cam[index][2] = dp8393x_get(s, width, 2) & 0xff;
> @@ -293,8 +293,8 @@ static void dp8393x_do_load_cam(dp8393xState *s)
> }
>
> /* Read CAM enable */
> - address_space_rw(&s->as, dp8393x_cdp(s),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, dp8393x_cdp(s), MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
> s->regs[SONIC_CE] = dp8393x_get(s, width, 0);
> DPRINTF("load cam done. cam enable mask 0x%04x\n", s->regs[SONIC_CE]);
>
> @@ -311,8 +311,8 @@ 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, dp8393x_rrp(s),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, dp8393x_rrp(s), MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
>
> /* Update SONIC registers */
> s->regs[SONIC_CRBA0] = dp8393x_get(s, width, 0);
> @@ -426,8 +426,8 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
> size = sizeof(uint16_t) * 6 * width;
> s->regs[SONIC_TTDA] = s->regs[SONIC_CTDA];
> DPRINTF("Transmit packet at %08x\n", dp8393x_ttda(s));
> - address_space_rw(&s->as, dp8393x_ttda(s) + sizeof(uint16_t) * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, dp8393x_ttda(s) + sizeof(uint16_t) * width,
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size);
> tx_len = 0;
>
> /* Update registers */
> @@ -451,17 +451,19 @@ 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, dp8393x_tsa(s),
> - MEMTXATTRS_UNSPECIFIED, &s->tx_buffer[tx_len], len, 0);
> + address_space_read(&s->as, dp8393x_tsa(s), 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,
> - dp8393x_ttda(s) + sizeof(uint16_t) * (4 + 3 * i) * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as,
> + dp8393x_ttda(s)
> + + sizeof(uint16_t) * (4 + 3 * i) * width,
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data,
> + size);
> s->regs[SONIC_TSA0] = dp8393x_get(s, width, 0);
> s->regs[SONIC_TSA1] = dp8393x_get(s, width, 1);
> s->regs[SONIC_TFS] = dp8393x_get(s, width, 2);
> @@ -494,18 +496,18 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
> dp8393x_put(s, width, 0,
> s->regs[SONIC_TCR] & 0x0fff); /* status */
> size = sizeof(uint16_t) * width;
> - address_space_rw(&s->as,
> - dp8393x_ttda(s),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 1);
> + address_space_write(&s->as, dp8393x_ttda(s), MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
>
> if (!(s->regs[SONIC_CR] & SONIC_CR_HTX)) {
> /* Read footer of packet */
> size = sizeof(uint16_t) * width;
> - address_space_rw(&s->as,
> - dp8393x_ttda(s) +
> - sizeof(uint16_t) *
> - (4 + 3 * s->regs[SONIC_TFC]) * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as,
> + dp8393x_ttda(s)
> + + sizeof(uint16_t) * (4 + 3 * s->regs[SONIC_TFC])
> + * width,
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data,
> + size);
> s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0) & ~0x1;
> if (dp8393x_get(s, width, 0) & 0x1) {
> /* EOL detected */
> @@ -767,8 +769,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 = dp8393x_crda(s) + sizeof(uint16_t) * 5 * width;
> - address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
> if (dp8393x_get(s, width, 0) & 0x1) {
> /* Still EOL ; stop reception */
> return -1;
> @@ -787,11 +789,11 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> /* Put packet into RBA */
> DPRINTF("Receive packet at %08x\n", dp8393x_crba(s));
> address = dp8393x_crba(s);
> - 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;
> @@ -819,13 +821,13 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> dp8393x_put(s, width, 3, s->regs[SONIC_TRBA1]); /* pkt_ptr1 */
> dp8393x_put(s, width, 4, s->regs[SONIC_RSC]); /* seq_no */
> size = sizeof(uint16_t) * 5 * width;
> - address_space_rw(&s->as, dp8393x_crda(s),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 1);
> + address_space_write(&s->as, dp8393x_crda(s), MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
>
> /* Move to next descriptor */
> size = sizeof(uint16_t) * width;
> - address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 5 * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 5 * width,
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size);
> s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0);
> if (s->regs[SONIC_LLFA] & 0x1) {
> /* EOL detected */
> @@ -838,8 +840,8 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> offset += sizeof(uint16_t);
> }
> s->data[0] = 0;
> - address_space_rw(&s->as, offset, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *)s->data, sizeof(uint16_t), 1);
> + address_space_write(&s->as, offset, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->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/net/i82596.c b/hw/net/i82596.c
> index 3a0e1ec4c05..6a80c24af23 100644
> --- a/hw/net/i82596.c
> +++ b/hw/net/i82596.c
> @@ -148,8 +148,8 @@ static void i82596_transmit(I82596State *s, uint32_t addr)
>
> if (s->nic && len) {
> assert(len <= sizeof(s->tx_buffer));
> - address_space_rw(&address_space_memory, tba,
> - MEMTXATTRS_UNSPECIFIED, s->tx_buffer, len, 0);
> + address_space_read(&address_space_memory, tba,
> + MEMTXATTRS_UNSPECIFIED, s->tx_buffer, len);
> DBG(PRINT_PKTHDR("Send", &s->tx_buffer));
> DBG(printf("Sending %d bytes\n", len));
> qemu_send_packet(qemu_get_queue(s->nic), s->tx_buffer, len);
> @@ -172,8 +172,8 @@ static void set_individual_address(I82596State *s, uint32_t addr)
>
> nc = qemu_get_queue(s->nic);
> m = s->conf.macaddr.a;
> - address_space_rw(&address_space_memory, addr + 8,
> - MEMTXATTRS_UNSPECIFIED, m, ETH_ALEN, 0);
> + address_space_read(&address_space_memory, addr + 8,
> + MEMTXATTRS_UNSPECIFIED, m, ETH_ALEN);
> qemu_format_nic_info_str(nc, m);
> trace_i82596_new_mac(nc->info_str);
> }
> @@ -190,9 +190,8 @@ static void set_multicast_list(I82596State *s, uint32_t addr)
> }
> for (i = 0; i < mc_count; i++) {
> uint8_t multicast_addr[ETH_ALEN];
> - address_space_rw(&address_space_memory,
> - addr + i * ETH_ALEN, MEMTXATTRS_UNSPECIFIED,
> - multicast_addr, ETH_ALEN, 0);
> + address_space_read(&address_space_memory, addr + i * ETH_ALEN,
> + MEMTXATTRS_UNSPECIFIED, multicast_addr, ETH_ALEN);
> DBG(printf("Add multicast entry " MAC_FMT "\n",
> MAC_ARG(multicast_addr)));
> unsigned mcast_idx = (net_crc32(multicast_addr, ETH_ALEN) &
> @@ -260,8 +259,8 @@ static void command_loop(I82596State *s)
> byte_cnt = MAX(byte_cnt, 4);
> byte_cnt = MIN(byte_cnt, sizeof(s->config));
> /* copy byte_cnt max. */
> - address_space_rw(&address_space_memory, s->cmd_p + 8,
> - MEMTXATTRS_UNSPECIFIED, s->config, byte_cnt, 0);
> + address_space_read(&address_space_memory, s->cmd_p + 8,
> + MEMTXATTRS_UNSPECIFIED, s->config, byte_cnt);
> /* config byte according to page 35ff */
> s->config[2] &= 0x82; /* mask valid bits */
> s->config[2] |= 0x40;
> @@ -640,14 +639,14 @@ ssize_t i82596_receive(NetClientState *nc, const uint8_t *buf, size_t sz)
> }
> rba = get_uint32(rbd + 8);
> /* printf("rba is 0x%x\n", rba); */
> - address_space_rw(&address_space_memory, rba,
> - MEMTXATTRS_UNSPECIFIED, (void *)buf, num, 1);
> + address_space_write(&address_space_memory, rba,
> + MEMTXATTRS_UNSPECIFIED, (void *)buf, num);
> rba += num;
> buf += num;
> len -= num;
> if (len == 0) { /* copy crc */
> - address_space_rw(&address_space_memory, rba - 4,
> - MEMTXATTRS_UNSPECIFIED, crc_ptr, 4, 1);
> + address_space_write(&address_space_memory, rba - 4,
> + MEMTXATTRS_UNSPECIFIED, crc_ptr, 4);
> }
>
> num |= 0x4000; /* set F BIT */
> diff --git a/hw/net/lasi_i82596.c b/hw/net/lasi_i82596.c
> index 427b3fbf701..52637a562d8 100644
> --- a/hw/net/lasi_i82596.c
> +++ b/hw/net/lasi_i82596.c
> @@ -55,8 +55,9 @@ static void lasi_82596_mem_write(void *opaque, hwaddr addr,
> * Provided for SeaBIOS only. Write MAC of Network card to addr @val.
> * Needed for the PDC_LAN_STATION_ID_READ PDC call.
> */
> - address_space_rw(&address_space_memory, val,
> - MEMTXATTRS_UNSPECIFIED, d->state.conf.macaddr.a, ETH_ALEN, 1);
> + address_space_write(&address_space_memory, val,
> + MEMTXATTRS_UNSPECIFIED, d->state.conf.macaddr.a,
> + ETH_ALEN);
> break;
> }
> }
> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
> index 5989d723c50..f150deca340 100644
> --- a/hw/ppc/pnv_lpc.c
> +++ b/hw/ppc/pnv_lpc.c
> @@ -238,16 +238,16 @@ static bool opb_read(PnvLpcController *lpc, uint32_t addr, uint8_t *data,
> int sz)
> {
> /* XXX Handle access size limits and FW read caching here */
> - return !address_space_rw(&lpc->opb_as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, sz, false);
> + return !address_space_read(&lpc->opb_as, addr, MEMTXATTRS_UNSPECIFIED,
> + data, sz);
> }
>
> static bool opb_write(PnvLpcController *lpc, uint32_t addr, uint8_t *data,
> int sz)
> {
> /* XXX Handle access size limits here */
> - return !address_space_rw(&lpc->opb_as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, sz, true);
> + return !address_space_write(&lpc->opb_as, addr, MEMTXATTRS_UNSPECIFIED,
> + data, sz);
> }
>
> #define ECCB_CTL_READ PPC_BIT(15)
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 844caab4082..0e0fccd050e 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -874,18 +874,18 @@ static inline int ida_read_next_idaw(CcwDataStream *cds)
> if (idaw_addr & 0x07 || !cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
> return -EINVAL; /* channel program check */
> }
> - ret = address_space_rw(&address_space_memory, idaw_addr,
> - MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
> - sizeof(idaw.fmt2), false);
> + ret = address_space_read(&address_space_memory, idaw_addr,
> + MEMTXATTRS_UNSPECIFIED, (void *)&idaw.fmt2,
> + sizeof(idaw.fmt2));
> cds->cda = be64_to_cpu(idaw.fmt2);
> } else {
> idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
> if (idaw_addr & 0x03 || !cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
> return -EINVAL; /* channel program check */
> }
> - ret = address_space_rw(&address_space_memory, idaw_addr,
> - MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,
> - sizeof(idaw.fmt1), false);
> + ret = address_space_read(&address_space_memory, idaw_addr,
> + MEMTXATTRS_UNSPECIFIED, (void *)&idaw.fmt1,
> + sizeof(idaw.fmt1));
> cds->cda = be64_to_cpu(idaw.fmt1);
> if (cds->cda & 0x80000000) {
> return -EINVAL; /* channel program check */
> diff --git a/qtest.c b/qtest.c
> index 12432f99cf4..328d674bcc8 100644
> --- a/qtest.c
> +++ b/qtest.c
> @@ -429,23 +429,23 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
>
> if (words[0][5] == 'b') {
> uint8_t data = value;
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - &data, 1, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + &data, 1);
> } else if (words[0][5] == 'w') {
> uint16_t data = value;
> tswap16s(&data);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &data, 2, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&data, 2);
> } else if (words[0][5] == 'l') {
> uint32_t data = value;
> tswap32s(&data);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &data, 4, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&data, 4);
> } else if (words[0][5] == 'q') {
> uint64_t data = value;
> tswap64s(&data);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &data, 8, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&data, 8);
> }
> qtest_send_prefix(chr);
> qtest_send(chr, "OK\n");
> @@ -463,22 +463,22 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
>
> if (words[0][4] == 'b') {
> uint8_t data;
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - &data, 1, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + &data, 1);
> value = data;
> } else if (words[0][4] == 'w') {
> uint16_t data;
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &data, 2, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&data, 2);
> value = tswap16(data);
> } else if (words[0][4] == 'l') {
> uint32_t data;
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &data, 4, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&data, 4);
> value = tswap32(data);
> } else if (words[0][4] == 'q') {
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &value, 8, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&value, 8);
> tswap64s(&value);
> }
> qtest_send_prefix(chr);
> @@ -498,8 +498,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
> g_assert(len);
>
> data = g_malloc(len);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, len, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
> + len);
>
> enc = g_malloc(2 * len + 1);
> for (i = 0; i < len; i++) {
> @@ -524,8 +524,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
> g_assert(ret == 0);
>
> data = g_malloc(len);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, len, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
> + len);
> b64_data = g_base64_encode(data, len);
> qtest_send_prefix(chr);
> qtest_sendf(chr, "OK %s\n", b64_data);
> @@ -559,8 +559,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
> data[i] = 0;
> }
> }
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, len, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
> + len);
> g_free(data);
>
> qtest_send_prefix(chr);
> @@ -582,8 +582,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
> if (len) {
> data = g_malloc(len);
> memset(data, pattern, len);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, len, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + data, len);
> g_free(data);
> }
>
> @@ -616,8 +616,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
> out_len = MIN(out_len, len);
> }
>
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, len, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
> + len);
>
> qtest_send_prefix(chr);
> qtest_send(chr, "OK\n");
> diff --git a/target/i386/hvf/x86_mmu.c b/target/i386/hvf/x86_mmu.c
> index d5a0efe7188..ff016fc0145 100644
> --- a/target/i386/hvf/x86_mmu.c
> +++ b/target/i386/hvf/x86_mmu.c
> @@ -88,8 +88,8 @@ static bool get_pt_entry(struct CPUState *cpu, struct gpt_translation *pt,
> }
>
> index = gpt_entry(pt->gva, level, pae);
> - address_space_rw(&address_space_memory, gpa + index * pte_size(pae),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)&pte, pte_size(pae), 0);
> + address_space_read(&address_space_memory, gpa + index * pte_size(pae),
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)&pte, pte_size(pae));
>
> pt->pte[level - 1] = pte;
>
> @@ -238,8 +238,8 @@ void vmx_write_mem(struct CPUState *cpu, target_ulong gva, void *data, int bytes
> if (!mmu_gva_to_gpa(cpu, gva, &gpa)) {
> VM_PANIC_EX("%s: mmu_gva_to_gpa %llx failed\n", __func__, gva);
> } else {
> - address_space_rw(&address_space_memory, gpa, MEMTXATTRS_UNSPECIFIED,
> - data, copy, 1);
> + address_space_write(&address_space_memory, gpa,
> + MEMTXATTRS_UNSPECIFIED, data, copy);
> }
>
> bytes -= copy;
> @@ -259,8 +259,8 @@ void vmx_read_mem(struct CPUState *cpu, void *data, target_ulong gva, int bytes)
> if (!mmu_gva_to_gpa(cpu, gva, &gpa)) {
> VM_PANIC_EX("%s: mmu_gva_to_gpa %llx failed\n", __func__, gva);
> }
> - address_space_rw(&address_space_memory, gpa, MEMTXATTRS_UNSPECIFIED,
> - data, copy, 0);
> + address_space_read(&address_space_memory, gpa, MEMTXATTRS_UNSPECIFIED,
> + data, copy);
>
> bytes -= copy;
> gva += copy;
> diff --git a/scripts/coccinelle/as_rw_const.cocci b/scripts/coccinelle/as_rw_const.cocci
> new file mode 100644
> index 00000000000..30da707701b
> --- /dev/null
> +++ b/scripts/coccinelle/as_rw_const.cocci
> @@ -0,0 +1,30 @@
> +// Avoid uses of address_space_rw() with a constant is_write argument.
> +// Usage:
> +// spatch --sp-file as-rw-const.spatch --dir . --in-place
Nitpick, script is now scripts/coccinelle/as_rw_const.cocci.
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> +
> +@@
> +expression E1, E2, E3, E4, E5;
> +symbol false;
> +@@
> +
> +- address_space_rw(E1, E2, E3, E4, E5, false)
> ++ address_space_read(E1, E2, E3, E4, E5)
> +@@
> +expression E1, E2, E3, E4, E5;
> +@@
> +
> +- address_space_rw(E1, E2, E3, E4, E5, 0)
> ++ address_space_read(E1, E2, E3, E4, E5)
> +@@
> +expression E1, E2, E3, E4, E5;
> +symbol true;
> +@@
> +
> +- address_space_rw(E1, E2, E3, E4, E5, true)
> ++ address_space_write(E1, E2, E3, E4, E5)
> +@@
> +expression E1, E2, E3, E4, E5;
> +@@
> +
> +- address_space_rw(E1, E2, E3, E4, E5, 1)
> ++ address_space_write(E1, E2, E3, E4, E5)
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] Avoid address_space_rw() with a constant is_write argument
2020-02-18 11:24 [PATCH v2] Avoid address_space_rw() with a constant is_write argument Peter Maydell
2020-02-16 17:24 ` Edgar E. Iglesias
2020-02-18 12:56 ` Philippe Mathieu-Daudé
@ 2020-02-18 13:13 ` Laurent Vivier
2020-02-18 13:20 ` Peter Maydell
2020-02-20 11:28 ` Paolo Bonzini
2020-02-18 13:30 ` Cédric Le Goater
` (4 subsequent siblings)
7 siblings, 2 replies; 14+ messages in thread
From: Laurent Vivier @ 2020-02-18 13:13 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: Thomas Huth, Alistair Francis, Eduardo Habkost, Cornelia Huck,
Halil Pasic, Christian Borntraeger, Cédric Le Goater,
Edgar E. Iglesias, Paolo Bonzini, David Gibson
On 18/02/2020 12:24, Peter Maydell wrote:
> The address_space_rw() function allows either reads or writes
> depending on the is_write argument passed to it; this is useful
> when the direction of the access is determined programmatically
> (as for instance when handling the KVM_EXIT_MMIO exit reason).
> Under the hood it just calls either address_space_write() or
> address_space_read_full().
>
> We also use it a lot with a constant is_write argument, though,
> which has two issues:
> * when reading "address_space_rw(..., 1)" this is less
> immediately clear to the reader as being a write than
> "address_space_write(...)"
> * calling address_space_rw() bypasses the optimization
> in address_space_read() that fast-paths reads of a
> fixed length
>
> This commit was produced with the included Coccinelle script
> scripts/coccinelle/as-rw-const.patch.
>
> Two lines in hw/net/dp8393x.c that Coccinelle produced that
> were over 80 characters were re-wrapped by hand.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I could break this down into separate patches by submaintainer,
> but the patch is not that large and I would argue that it's
> better for the project if we can try to avoid introducing too
> much friction into the process of doing 'safe' tree-wide
> minor refactorings.
>
> v1->v2: put the coccinelle script in scripts/coccinelle rather
> than just in the commit message.
> ---
> accel/kvm/kvm-all.c | 6 +--
> dma-helpers.c | 4 +-
> exec.c | 4 +-
> hw/dma/xlnx-zdma.c | 11 ++---
> hw/net/dp8393x.c | 68 ++++++++++++++--------------
> hw/net/i82596.c | 25 +++++-----
> hw/net/lasi_i82596.c | 5 +-
> hw/ppc/pnv_lpc.c | 8 ++--
> hw/s390x/css.c | 12 ++---
> qtest.c | 52 ++++++++++-----------
> target/i386/hvf/x86_mmu.c | 12 ++---
> scripts/coccinelle/as_rw_const.cocci | 30 ++++++++++++
> 12 files changed, 133 insertions(+), 104 deletions(-)
> create mode 100644 scripts/coccinelle/as_rw_const.cocci
There is one in target/i386/hvf/vmx.h: macvm_set_cr0() you didn't change.
You must update the script name in the script comment (as suggested by
Philippe) and in the commit message.
Anyway:
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index c111312dfdd..0cfe6fd8ded 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2178,9 +2178,9 @@ void kvm_flush_coalesced_mmio_buffer(void)
> ent = &ring->coalesced_mmio[ring->first];
>
> if (ent->pio == 1) {
> - address_space_rw(&address_space_io, ent->phys_addr,
> - MEMTXATTRS_UNSPECIFIED, ent->data,
> - ent->len, true);
> + address_space_write(&address_space_io, ent->phys_addr,
> + MEMTXATTRS_UNSPECIFIED, ent->data,
> + ent->len);
> } else {
> cpu_physical_memory_write(ent->phys_addr, ent->data, ent->len);
> }
> diff --git a/dma-helpers.c b/dma-helpers.c
> index d3871dc61ea..e8a26e81e16 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 8e9cc3b47cf..baefe582393 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -3810,8 +3810,8 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
> address_space_write_rom(cpu->cpu_ases[asidx].as, phys_addr,
> attrs, buf, l);
> } else {
> - address_space_rw(cpu->cpu_ases[asidx].as, phys_addr,
> - attrs, buf, l, 0);
> + address_space_read(cpu->cpu_ases[asidx].as, phys_addr, attrs, buf,
> + l);
> }
> len -= l;
> buf += l;
> diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c
> index 8fb83f5b078..31936061e21 100644
> --- a/hw/dma/xlnx-zdma.c
> +++ b/hw/dma/xlnx-zdma.c
> @@ -311,8 +311,7 @@ static bool zdma_load_descriptor(XlnxZDMA *s, uint64_t addr, void *buf)
> return false;
> }
>
> - address_space_rw(s->dma_as, addr, s->attr,
> - buf, sizeof(XlnxZDMADescr), false);
> + address_space_read(s->dma_as, addr, s->attr, buf, sizeof(XlnxZDMADescr));
> return true;
> }
>
> @@ -364,7 +363,7 @@ static uint64_t zdma_update_descr_addr(XlnxZDMA *s, bool type,
> } else {
> addr = zdma_get_regaddr64(s, basereg);
> addr += sizeof(s->dsc_dst);
> - address_space_rw(s->dma_as, addr, s->attr, (void *) &next, 8, false);
> + address_space_read(s->dma_as, addr, s->attr, (void *)&next, 8);
> zdma_put_regaddr64(s, basereg, next);
> }
> return next;
> @@ -416,8 +415,7 @@ static void zdma_write_dst(XlnxZDMA *s, uint8_t *buf, uint32_t len)
> }
> }
>
> - address_space_rw(s->dma_as, s->dsc_dst.addr, s->attr, buf, dlen,
> - true);
> + address_space_write(s->dma_as, s->dsc_dst.addr, s->attr, buf, dlen);
> if (burst_type == AXI_BURST_INCR) {
> s->dsc_dst.addr += dlen;
> }
> @@ -493,8 +491,7 @@ static void zdma_process_descr(XlnxZDMA *s)
> len = s->cfg.bus_width / 8;
> }
> } else {
> - address_space_rw(s->dma_as, src_addr, s->attr, s->buf, len,
> - false);
> + address_space_read(s->dma_as, src_addr, s->attr, s->buf, len);
> if (burst_type == AXI_BURST_INCR) {
> src_addr += len;
> }
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index a134d431ae3..f5f1c669e80 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -275,8 +275,8 @@ static void dp8393x_do_load_cam(dp8393xState *s)
>
> while (s->regs[SONIC_CDC] & 0x1f) {
> /* Fill current entry */
> - address_space_rw(&s->as, dp8393x_cdp(s),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, dp8393x_cdp(s), MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
> s->cam[index][0] = dp8393x_get(s, width, 1) & 0xff;
> s->cam[index][1] = dp8393x_get(s, width, 1) >> 8;
> s->cam[index][2] = dp8393x_get(s, width, 2) & 0xff;
> @@ -293,8 +293,8 @@ static void dp8393x_do_load_cam(dp8393xState *s)
> }
>
> /* Read CAM enable */
> - address_space_rw(&s->as, dp8393x_cdp(s),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, dp8393x_cdp(s), MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
> s->regs[SONIC_CE] = dp8393x_get(s, width, 0);
> DPRINTF("load cam done. cam enable mask 0x%04x\n", s->regs[SONIC_CE]);
>
> @@ -311,8 +311,8 @@ 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, dp8393x_rrp(s),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, dp8393x_rrp(s), MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
>
> /* Update SONIC registers */
> s->regs[SONIC_CRBA0] = dp8393x_get(s, width, 0);
> @@ -426,8 +426,8 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
> size = sizeof(uint16_t) * 6 * width;
> s->regs[SONIC_TTDA] = s->regs[SONIC_CTDA];
> DPRINTF("Transmit packet at %08x\n", dp8393x_ttda(s));
> - address_space_rw(&s->as, dp8393x_ttda(s) + sizeof(uint16_t) * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, dp8393x_ttda(s) + sizeof(uint16_t) * width,
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size);
> tx_len = 0;
>
> /* Update registers */
> @@ -451,17 +451,19 @@ 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, dp8393x_tsa(s),
> - MEMTXATTRS_UNSPECIFIED, &s->tx_buffer[tx_len], len, 0);
> + address_space_read(&s->as, dp8393x_tsa(s), 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,
> - dp8393x_ttda(s) + sizeof(uint16_t) * (4 + 3 * i) * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as,
> + dp8393x_ttda(s)
> + + sizeof(uint16_t) * (4 + 3 * i) * width,
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data,
> + size);
> s->regs[SONIC_TSA0] = dp8393x_get(s, width, 0);
> s->regs[SONIC_TSA1] = dp8393x_get(s, width, 1);
> s->regs[SONIC_TFS] = dp8393x_get(s, width, 2);
> @@ -494,18 +496,18 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
> dp8393x_put(s, width, 0,
> s->regs[SONIC_TCR] & 0x0fff); /* status */
> size = sizeof(uint16_t) * width;
> - address_space_rw(&s->as,
> - dp8393x_ttda(s),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 1);
> + address_space_write(&s->as, dp8393x_ttda(s), MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
>
> if (!(s->regs[SONIC_CR] & SONIC_CR_HTX)) {
> /* Read footer of packet */
> size = sizeof(uint16_t) * width;
> - address_space_rw(&s->as,
> - dp8393x_ttda(s) +
> - sizeof(uint16_t) *
> - (4 + 3 * s->regs[SONIC_TFC]) * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as,
> + dp8393x_ttda(s)
> + + sizeof(uint16_t) * (4 + 3 * s->regs[SONIC_TFC])
> + * width,
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data,
> + size);
> s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0) & ~0x1;
> if (dp8393x_get(s, width, 0) & 0x1) {
> /* EOL detected */
> @@ -767,8 +769,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 = dp8393x_crda(s) + sizeof(uint16_t) * 5 * width;
> - address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
> if (dp8393x_get(s, width, 0) & 0x1) {
> /* Still EOL ; stop reception */
> return -1;
> @@ -787,11 +789,11 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> /* Put packet into RBA */
> DPRINTF("Receive packet at %08x\n", dp8393x_crba(s));
> address = dp8393x_crba(s);
> - 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;
> @@ -819,13 +821,13 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> dp8393x_put(s, width, 3, s->regs[SONIC_TRBA1]); /* pkt_ptr1 */
> dp8393x_put(s, width, 4, s->regs[SONIC_RSC]); /* seq_no */
> size = sizeof(uint16_t) * 5 * width;
> - address_space_rw(&s->as, dp8393x_crda(s),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 1);
> + address_space_write(&s->as, dp8393x_crda(s), MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
>
> /* Move to next descriptor */
> size = sizeof(uint16_t) * width;
> - address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 5 * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 5 * width,
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size);
> s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0);
> if (s->regs[SONIC_LLFA] & 0x1) {
> /* EOL detected */
> @@ -838,8 +840,8 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> offset += sizeof(uint16_t);
> }
> s->data[0] = 0;
> - address_space_rw(&s->as, offset, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *)s->data, sizeof(uint16_t), 1);
> + address_space_write(&s->as, offset, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->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/net/i82596.c b/hw/net/i82596.c
> index 3a0e1ec4c05..6a80c24af23 100644
> --- a/hw/net/i82596.c
> +++ b/hw/net/i82596.c
> @@ -148,8 +148,8 @@ static void i82596_transmit(I82596State *s, uint32_t addr)
>
> if (s->nic && len) {
> assert(len <= sizeof(s->tx_buffer));
> - address_space_rw(&address_space_memory, tba,
> - MEMTXATTRS_UNSPECIFIED, s->tx_buffer, len, 0);
> + address_space_read(&address_space_memory, tba,
> + MEMTXATTRS_UNSPECIFIED, s->tx_buffer, len);
> DBG(PRINT_PKTHDR("Send", &s->tx_buffer));
> DBG(printf("Sending %d bytes\n", len));
> qemu_send_packet(qemu_get_queue(s->nic), s->tx_buffer, len);
> @@ -172,8 +172,8 @@ static void set_individual_address(I82596State *s, uint32_t addr)
>
> nc = qemu_get_queue(s->nic);
> m = s->conf.macaddr.a;
> - address_space_rw(&address_space_memory, addr + 8,
> - MEMTXATTRS_UNSPECIFIED, m, ETH_ALEN, 0);
> + address_space_read(&address_space_memory, addr + 8,
> + MEMTXATTRS_UNSPECIFIED, m, ETH_ALEN);
> qemu_format_nic_info_str(nc, m);
> trace_i82596_new_mac(nc->info_str);
> }
> @@ -190,9 +190,8 @@ static void set_multicast_list(I82596State *s, uint32_t addr)
> }
> for (i = 0; i < mc_count; i++) {
> uint8_t multicast_addr[ETH_ALEN];
> - address_space_rw(&address_space_memory,
> - addr + i * ETH_ALEN, MEMTXATTRS_UNSPECIFIED,
> - multicast_addr, ETH_ALEN, 0);
> + address_space_read(&address_space_memory, addr + i * ETH_ALEN,
> + MEMTXATTRS_UNSPECIFIED, multicast_addr, ETH_ALEN);
> DBG(printf("Add multicast entry " MAC_FMT "\n",
> MAC_ARG(multicast_addr)));
> unsigned mcast_idx = (net_crc32(multicast_addr, ETH_ALEN) &
> @@ -260,8 +259,8 @@ static void command_loop(I82596State *s)
> byte_cnt = MAX(byte_cnt, 4);
> byte_cnt = MIN(byte_cnt, sizeof(s->config));
> /* copy byte_cnt max. */
> - address_space_rw(&address_space_memory, s->cmd_p + 8,
> - MEMTXATTRS_UNSPECIFIED, s->config, byte_cnt, 0);
> + address_space_read(&address_space_memory, s->cmd_p + 8,
> + MEMTXATTRS_UNSPECIFIED, s->config, byte_cnt);
> /* config byte according to page 35ff */
> s->config[2] &= 0x82; /* mask valid bits */
> s->config[2] |= 0x40;
> @@ -640,14 +639,14 @@ ssize_t i82596_receive(NetClientState *nc, const uint8_t *buf, size_t sz)
> }
> rba = get_uint32(rbd + 8);
> /* printf("rba is 0x%x\n", rba); */
> - address_space_rw(&address_space_memory, rba,
> - MEMTXATTRS_UNSPECIFIED, (void *)buf, num, 1);
> + address_space_write(&address_space_memory, rba,
> + MEMTXATTRS_UNSPECIFIED, (void *)buf, num);
> rba += num;
> buf += num;
> len -= num;
> if (len == 0) { /* copy crc */
> - address_space_rw(&address_space_memory, rba - 4,
> - MEMTXATTRS_UNSPECIFIED, crc_ptr, 4, 1);
> + address_space_write(&address_space_memory, rba - 4,
> + MEMTXATTRS_UNSPECIFIED, crc_ptr, 4);
> }
>
> num |= 0x4000; /* set F BIT */
> diff --git a/hw/net/lasi_i82596.c b/hw/net/lasi_i82596.c
> index 427b3fbf701..52637a562d8 100644
> --- a/hw/net/lasi_i82596.c
> +++ b/hw/net/lasi_i82596.c
> @@ -55,8 +55,9 @@ static void lasi_82596_mem_write(void *opaque, hwaddr addr,
> * Provided for SeaBIOS only. Write MAC of Network card to addr @val.
> * Needed for the PDC_LAN_STATION_ID_READ PDC call.
> */
> - address_space_rw(&address_space_memory, val,
> - MEMTXATTRS_UNSPECIFIED, d->state.conf.macaddr.a, ETH_ALEN, 1);
> + address_space_write(&address_space_memory, val,
> + MEMTXATTRS_UNSPECIFIED, d->state.conf.macaddr.a,
> + ETH_ALEN);
> break;
> }
> }
> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
> index 5989d723c50..f150deca340 100644
> --- a/hw/ppc/pnv_lpc.c
> +++ b/hw/ppc/pnv_lpc.c
> @@ -238,16 +238,16 @@ static bool opb_read(PnvLpcController *lpc, uint32_t addr, uint8_t *data,
> int sz)
> {
> /* XXX Handle access size limits and FW read caching here */
> - return !address_space_rw(&lpc->opb_as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, sz, false);
> + return !address_space_read(&lpc->opb_as, addr, MEMTXATTRS_UNSPECIFIED,
> + data, sz);
> }
>
> static bool opb_write(PnvLpcController *lpc, uint32_t addr, uint8_t *data,
> int sz)
> {
> /* XXX Handle access size limits here */
> - return !address_space_rw(&lpc->opb_as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, sz, true);
> + return !address_space_write(&lpc->opb_as, addr, MEMTXATTRS_UNSPECIFIED,
> + data, sz);
> }
>
> #define ECCB_CTL_READ PPC_BIT(15)
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 844caab4082..0e0fccd050e 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -874,18 +874,18 @@ static inline int ida_read_next_idaw(CcwDataStream *cds)
> if (idaw_addr & 0x07 || !cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
> return -EINVAL; /* channel program check */
> }
> - ret = address_space_rw(&address_space_memory, idaw_addr,
> - MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
> - sizeof(idaw.fmt2), false);
> + ret = address_space_read(&address_space_memory, idaw_addr,
> + MEMTXATTRS_UNSPECIFIED, (void *)&idaw.fmt2,
> + sizeof(idaw.fmt2));
> cds->cda = be64_to_cpu(idaw.fmt2);
> } else {
> idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
> if (idaw_addr & 0x03 || !cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
> return -EINVAL; /* channel program check */
> }
> - ret = address_space_rw(&address_space_memory, idaw_addr,
> - MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,
> - sizeof(idaw.fmt1), false);
> + ret = address_space_read(&address_space_memory, idaw_addr,
> + MEMTXATTRS_UNSPECIFIED, (void *)&idaw.fmt1,
> + sizeof(idaw.fmt1));
> cds->cda = be64_to_cpu(idaw.fmt1);
> if (cds->cda & 0x80000000) {
> return -EINVAL; /* channel program check */
> diff --git a/qtest.c b/qtest.c
> index 12432f99cf4..328d674bcc8 100644
> --- a/qtest.c
> +++ b/qtest.c
> @@ -429,23 +429,23 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
>
> if (words[0][5] == 'b') {
> uint8_t data = value;
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - &data, 1, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + &data, 1);
> } else if (words[0][5] == 'w') {
> uint16_t data = value;
> tswap16s(&data);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &data, 2, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&data, 2);
> } else if (words[0][5] == 'l') {
> uint32_t data = value;
> tswap32s(&data);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &data, 4, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&data, 4);
> } else if (words[0][5] == 'q') {
> uint64_t data = value;
> tswap64s(&data);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &data, 8, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&data, 8);
> }
> qtest_send_prefix(chr);
> qtest_send(chr, "OK\n");
> @@ -463,22 +463,22 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
>
> if (words[0][4] == 'b') {
> uint8_t data;
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - &data, 1, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + &data, 1);
> value = data;
> } else if (words[0][4] == 'w') {
> uint16_t data;
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &data, 2, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&data, 2);
> value = tswap16(data);
> } else if (words[0][4] == 'l') {
> uint32_t data;
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &data, 4, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&data, 4);
> value = tswap32(data);
> } else if (words[0][4] == 'q') {
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &value, 8, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&value, 8);
> tswap64s(&value);
> }
> qtest_send_prefix(chr);
> @@ -498,8 +498,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
> g_assert(len);
>
> data = g_malloc(len);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, len, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
> + len);
>
> enc = g_malloc(2 * len + 1);
> for (i = 0; i < len; i++) {
> @@ -524,8 +524,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
> g_assert(ret == 0);
>
> data = g_malloc(len);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, len, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
> + len);
> b64_data = g_base64_encode(data, len);
> qtest_send_prefix(chr);
> qtest_sendf(chr, "OK %s\n", b64_data);
> @@ -559,8 +559,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
> data[i] = 0;
> }
> }
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, len, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
> + len);
> g_free(data);
>
> qtest_send_prefix(chr);
> @@ -582,8 +582,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
> if (len) {
> data = g_malloc(len);
> memset(data, pattern, len);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, len, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + data, len);
> g_free(data);
> }
>
> @@ -616,8 +616,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
> out_len = MIN(out_len, len);
> }
>
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, len, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
> + len);
>
> qtest_send_prefix(chr);
> qtest_send(chr, "OK\n");
> diff --git a/target/i386/hvf/x86_mmu.c b/target/i386/hvf/x86_mmu.c
> index d5a0efe7188..ff016fc0145 100644
> --- a/target/i386/hvf/x86_mmu.c
> +++ b/target/i386/hvf/x86_mmu.c
> @@ -88,8 +88,8 @@ static bool get_pt_entry(struct CPUState *cpu, struct gpt_translation *pt,
> }
>
> index = gpt_entry(pt->gva, level, pae);
> - address_space_rw(&address_space_memory, gpa + index * pte_size(pae),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)&pte, pte_size(pae), 0);
> + address_space_read(&address_space_memory, gpa + index * pte_size(pae),
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)&pte, pte_size(pae));
>
> pt->pte[level - 1] = pte;
>
> @@ -238,8 +238,8 @@ void vmx_write_mem(struct CPUState *cpu, target_ulong gva, void *data, int bytes
> if (!mmu_gva_to_gpa(cpu, gva, &gpa)) {
> VM_PANIC_EX("%s: mmu_gva_to_gpa %llx failed\n", __func__, gva);
> } else {
> - address_space_rw(&address_space_memory, gpa, MEMTXATTRS_UNSPECIFIED,
> - data, copy, 1);
> + address_space_write(&address_space_memory, gpa,
> + MEMTXATTRS_UNSPECIFIED, data, copy);
> }
>
> bytes -= copy;
> @@ -259,8 +259,8 @@ void vmx_read_mem(struct CPUState *cpu, void *data, target_ulong gva, int bytes)
> if (!mmu_gva_to_gpa(cpu, gva, &gpa)) {
> VM_PANIC_EX("%s: mmu_gva_to_gpa %llx failed\n", __func__, gva);
> }
> - address_space_rw(&address_space_memory, gpa, MEMTXATTRS_UNSPECIFIED,
> - data, copy, 0);
> + address_space_read(&address_space_memory, gpa, MEMTXATTRS_UNSPECIFIED,
> + data, copy);
>
> bytes -= copy;
> gva += copy;
> diff --git a/scripts/coccinelle/as_rw_const.cocci b/scripts/coccinelle/as_rw_const.cocci
> new file mode 100644
> index 00000000000..30da707701b
> --- /dev/null
> +++ b/scripts/coccinelle/as_rw_const.cocci
> @@ -0,0 +1,30 @@
> +// Avoid uses of address_space_rw() with a constant is_write argument.
> +// Usage:
> +// spatch --sp-file as-rw-const.spatch --dir . --in-place
> +
> +@@
> +expression E1, E2, E3, E4, E5;
> +symbol false;
> +@@
> +
> +- address_space_rw(E1, E2, E3, E4, E5, false)
> ++ address_space_read(E1, E2, E3, E4, E5)
> +@@
> +expression E1, E2, E3, E4, E5;
> +@@
> +
> +- address_space_rw(E1, E2, E3, E4, E5, 0)
> ++ address_space_read(E1, E2, E3, E4, E5)
> +@@
> +expression E1, E2, E3, E4, E5;
> +symbol true;
> +@@
> +
> +- address_space_rw(E1, E2, E3, E4, E5, true)
> ++ address_space_write(E1, E2, E3, E4, E5)
> +@@
> +expression E1, E2, E3, E4, E5;
> +@@
> +
> +- address_space_rw(E1, E2, E3, E4, E5, 1)
> ++ address_space_write(E1, E2, E3, E4, E5)
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] Avoid address_space_rw() with a constant is_write argument
2020-02-18 13:13 ` Laurent Vivier
@ 2020-02-18 13:20 ` Peter Maydell
2020-02-20 11:28 ` Paolo Bonzini
1 sibling, 0 replies; 14+ messages in thread
From: Peter Maydell @ 2020-02-18 13:20 UTC (permalink / raw)
To: Laurent Vivier
Cc: Thomas Huth, Cornelia Huck, Eduardo Habkost, Alistair Francis,
QEMU Developers, Halil Pasic, Christian Borntraeger,
Cédric Le Goater, Edgar E. Iglesias, Paolo Bonzini,
David Gibson
On Tue, 18 Feb 2020 at 13:14, Laurent Vivier <lvivier@redhat.com> wrote:
> There is one in target/i386/hvf/vmx.h: macvm_set_cr0() you didn't change.
It turns out that "spatch --dir ." only looks at .c files,
and you need to add --include-headers to get it to look at
.h files too. The only extra change is this one:
diff --git a/target/i386/hvf/vmx.h b/target/i386/hvf/vmx.h
index eb8894cd589..c245dc6d5bb 100644
--- a/target/i386/hvf/vmx.h
+++ b/target/i386/hvf/vmx.h
@@ -125,10 +125,9 @@ static inline void macvm_set_cr0(hv_vcpuid_t
vcpu, uint64_t cr0)
if ((cr0 & CR0_PG) && (rvmcs(vcpu, VMCS_GUEST_CR4) & CR4_PAE) &&
!(efer & MSR_EFER_LME)) {
- address_space_rw(&address_space_memory,
- rvmcs(vcpu, VMCS_GUEST_CR3) & ~0x1f,
- MEMTXATTRS_UNSPECIFIED,
- (uint8_t *)pdpte, 32, 0);
+ address_space_read(&address_space_memory,
+ rvmcs(vcpu, VMCS_GUEST_CR3) & ~0x1f,
+ MEMTXATTRS_UNSPECIFIED, (uint8_t *)pdpte, 32);
/* Only set PDPTE when appropriate. */
for (i = 0; i < 4; i++) {
wvmcs(vcpu, VMCS_GUEST_PDPTE0 + i * 2, pdpte[i]);
which I propose to squash in, together with the fixes to use
the right filename in the commit message and comment.
thanks
-- PMM
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2] Avoid address_space_rw() with a constant is_write argument
2020-02-18 11:24 [PATCH v2] Avoid address_space_rw() with a constant is_write argument Peter Maydell
` (2 preceding siblings ...)
2020-02-18 13:13 ` Laurent Vivier
@ 2020-02-18 13:30 ` Cédric Le Goater
2020-02-18 14:54 ` Christian Borntraeger
` (3 subsequent siblings)
7 siblings, 0 replies; 14+ messages in thread
From: Cédric Le Goater @ 2020-02-18 13:30 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: Laurent Vivier, Thomas Huth, Alistair Francis, Eduardo Habkost,
Cornelia Huck, Halil Pasic, Christian Borntraeger,
Edgar E. Iglesias, Paolo Bonzini, David Gibson
On 2/18/20 12:24 PM, Peter Maydell wrote:
> The address_space_rw() function allows either reads or writes
> depending on the is_write argument passed to it; this is useful
> when the direction of the access is determined programmatically
> (as for instance when handling the KVM_EXIT_MMIO exit reason).
> Under the hood it just calls either address_space_write() or
> address_space_read_full().
>
> We also use it a lot with a constant is_write argument, though,
> which has two issues:
> * when reading "address_space_rw(..., 1)" this is less
> immediately clear to the reader as being a write than
> "address_space_write(...)"
> * calling address_space_rw() bypasses the optimization
> in address_space_read() that fast-paths reads of a
> fixed length
>
> This commit was produced with the included Coccinelle script
> scripts/coccinelle/as-rw-const.patch.
>
> Two lines in hw/net/dp8393x.c that Coccinelle produced that
> were over 80 characters were re-wrapped by hand.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
> ---
> I could break this down into separate patches by submaintainer,
> but the patch is not that large and I would argue that it's
> better for the project if we can try to avoid introducing too
> much friction into the process of doing 'safe' tree-wide
> minor refactorings.
>
> v1->v2: put the coccinelle script in scripts/coccinelle rather
> than just in the commit message.
> ---
> accel/kvm/kvm-all.c | 6 +--
> dma-helpers.c | 4 +-
> exec.c | 4 +-
> hw/dma/xlnx-zdma.c | 11 ++---
> hw/net/dp8393x.c | 68 ++++++++++++++--------------
> hw/net/i82596.c | 25 +++++-----
> hw/net/lasi_i82596.c | 5 +-
> hw/ppc/pnv_lpc.c | 8 ++--
> hw/s390x/css.c | 12 ++---
> qtest.c | 52 ++++++++++-----------
> target/i386/hvf/x86_mmu.c | 12 ++---
> scripts/coccinelle/as_rw_const.cocci | 30 ++++++++++++
> 12 files changed, 133 insertions(+), 104 deletions(-)
> create mode 100644 scripts/coccinelle/as_rw_const.cocci
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index c111312dfdd..0cfe6fd8ded 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2178,9 +2178,9 @@ void kvm_flush_coalesced_mmio_buffer(void)
> ent = &ring->coalesced_mmio[ring->first];
>
> if (ent->pio == 1) {
> - address_space_rw(&address_space_io, ent->phys_addr,
> - MEMTXATTRS_UNSPECIFIED, ent->data,
> - ent->len, true);
> + address_space_write(&address_space_io, ent->phys_addr,
> + MEMTXATTRS_UNSPECIFIED, ent->data,
> + ent->len);
> } else {
> cpu_physical_memory_write(ent->phys_addr, ent->data, ent->len);
> }
> diff --git a/dma-helpers.c b/dma-helpers.c
> index d3871dc61ea..e8a26e81e16 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 8e9cc3b47cf..baefe582393 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -3810,8 +3810,8 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
> address_space_write_rom(cpu->cpu_ases[asidx].as, phys_addr,
> attrs, buf, l);
> } else {
> - address_space_rw(cpu->cpu_ases[asidx].as, phys_addr,
> - attrs, buf, l, 0);
> + address_space_read(cpu->cpu_ases[asidx].as, phys_addr, attrs, buf,
> + l);
> }
> len -= l;
> buf += l;
> diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c
> index 8fb83f5b078..31936061e21 100644
> --- a/hw/dma/xlnx-zdma.c
> +++ b/hw/dma/xlnx-zdma.c
> @@ -311,8 +311,7 @@ static bool zdma_load_descriptor(XlnxZDMA *s, uint64_t addr, void *buf)
> return false;
> }
>
> - address_space_rw(s->dma_as, addr, s->attr,
> - buf, sizeof(XlnxZDMADescr), false);
> + address_space_read(s->dma_as, addr, s->attr, buf, sizeof(XlnxZDMADescr));
> return true;
> }
>
> @@ -364,7 +363,7 @@ static uint64_t zdma_update_descr_addr(XlnxZDMA *s, bool type,
> } else {
> addr = zdma_get_regaddr64(s, basereg);
> addr += sizeof(s->dsc_dst);
> - address_space_rw(s->dma_as, addr, s->attr, (void *) &next, 8, false);
> + address_space_read(s->dma_as, addr, s->attr, (void *)&next, 8);
> zdma_put_regaddr64(s, basereg, next);
> }
> return next;
> @@ -416,8 +415,7 @@ static void zdma_write_dst(XlnxZDMA *s, uint8_t *buf, uint32_t len)
> }
> }
>
> - address_space_rw(s->dma_as, s->dsc_dst.addr, s->attr, buf, dlen,
> - true);
> + address_space_write(s->dma_as, s->dsc_dst.addr, s->attr, buf, dlen);
> if (burst_type == AXI_BURST_INCR) {
> s->dsc_dst.addr += dlen;
> }
> @@ -493,8 +491,7 @@ static void zdma_process_descr(XlnxZDMA *s)
> len = s->cfg.bus_width / 8;
> }
> } else {
> - address_space_rw(s->dma_as, src_addr, s->attr, s->buf, len,
> - false);
> + address_space_read(s->dma_as, src_addr, s->attr, s->buf, len);
> if (burst_type == AXI_BURST_INCR) {
> src_addr += len;
> }
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index a134d431ae3..f5f1c669e80 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -275,8 +275,8 @@ static void dp8393x_do_load_cam(dp8393xState *s)
>
> while (s->regs[SONIC_CDC] & 0x1f) {
> /* Fill current entry */
> - address_space_rw(&s->as, dp8393x_cdp(s),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, dp8393x_cdp(s), MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
> s->cam[index][0] = dp8393x_get(s, width, 1) & 0xff;
> s->cam[index][1] = dp8393x_get(s, width, 1) >> 8;
> s->cam[index][2] = dp8393x_get(s, width, 2) & 0xff;
> @@ -293,8 +293,8 @@ static void dp8393x_do_load_cam(dp8393xState *s)
> }
>
> /* Read CAM enable */
> - address_space_rw(&s->as, dp8393x_cdp(s),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, dp8393x_cdp(s), MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
> s->regs[SONIC_CE] = dp8393x_get(s, width, 0);
> DPRINTF("load cam done. cam enable mask 0x%04x\n", s->regs[SONIC_CE]);
>
> @@ -311,8 +311,8 @@ 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, dp8393x_rrp(s),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, dp8393x_rrp(s), MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
>
> /* Update SONIC registers */
> s->regs[SONIC_CRBA0] = dp8393x_get(s, width, 0);
> @@ -426,8 +426,8 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
> size = sizeof(uint16_t) * 6 * width;
> s->regs[SONIC_TTDA] = s->regs[SONIC_CTDA];
> DPRINTF("Transmit packet at %08x\n", dp8393x_ttda(s));
> - address_space_rw(&s->as, dp8393x_ttda(s) + sizeof(uint16_t) * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, dp8393x_ttda(s) + sizeof(uint16_t) * width,
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size);
> tx_len = 0;
>
> /* Update registers */
> @@ -451,17 +451,19 @@ 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, dp8393x_tsa(s),
> - MEMTXATTRS_UNSPECIFIED, &s->tx_buffer[tx_len], len, 0);
> + address_space_read(&s->as, dp8393x_tsa(s), 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,
> - dp8393x_ttda(s) + sizeof(uint16_t) * (4 + 3 * i) * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as,
> + dp8393x_ttda(s)
> + + sizeof(uint16_t) * (4 + 3 * i) * width,
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data,
> + size);
> s->regs[SONIC_TSA0] = dp8393x_get(s, width, 0);
> s->regs[SONIC_TSA1] = dp8393x_get(s, width, 1);
> s->regs[SONIC_TFS] = dp8393x_get(s, width, 2);
> @@ -494,18 +496,18 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
> dp8393x_put(s, width, 0,
> s->regs[SONIC_TCR] & 0x0fff); /* status */
> size = sizeof(uint16_t) * width;
> - address_space_rw(&s->as,
> - dp8393x_ttda(s),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 1);
> + address_space_write(&s->as, dp8393x_ttda(s), MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
>
> if (!(s->regs[SONIC_CR] & SONIC_CR_HTX)) {
> /* Read footer of packet */
> size = sizeof(uint16_t) * width;
> - address_space_rw(&s->as,
> - dp8393x_ttda(s) +
> - sizeof(uint16_t) *
> - (4 + 3 * s->regs[SONIC_TFC]) * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as,
> + dp8393x_ttda(s)
> + + sizeof(uint16_t) * (4 + 3 * s->regs[SONIC_TFC])
> + * width,
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data,
> + size);
> s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0) & ~0x1;
> if (dp8393x_get(s, width, 0) & 0x1) {
> /* EOL detected */
> @@ -767,8 +769,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 = dp8393x_crda(s) + sizeof(uint16_t) * 5 * width;
> - address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
> if (dp8393x_get(s, width, 0) & 0x1) {
> /* Still EOL ; stop reception */
> return -1;
> @@ -787,11 +789,11 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> /* Put packet into RBA */
> DPRINTF("Receive packet at %08x\n", dp8393x_crba(s));
> address = dp8393x_crba(s);
> - 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;
> @@ -819,13 +821,13 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> dp8393x_put(s, width, 3, s->regs[SONIC_TRBA1]); /* pkt_ptr1 */
> dp8393x_put(s, width, 4, s->regs[SONIC_RSC]); /* seq_no */
> size = sizeof(uint16_t) * 5 * width;
> - address_space_rw(&s->as, dp8393x_crda(s),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 1);
> + address_space_write(&s->as, dp8393x_crda(s), MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
>
> /* Move to next descriptor */
> size = sizeof(uint16_t) * width;
> - address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 5 * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 5 * width,
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size);
> s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0);
> if (s->regs[SONIC_LLFA] & 0x1) {
> /* EOL detected */
> @@ -838,8 +840,8 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> offset += sizeof(uint16_t);
> }
> s->data[0] = 0;
> - address_space_rw(&s->as, offset, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *)s->data, sizeof(uint16_t), 1);
> + address_space_write(&s->as, offset, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->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/net/i82596.c b/hw/net/i82596.c
> index 3a0e1ec4c05..6a80c24af23 100644
> --- a/hw/net/i82596.c
> +++ b/hw/net/i82596.c
> @@ -148,8 +148,8 @@ static void i82596_transmit(I82596State *s, uint32_t addr)
>
> if (s->nic && len) {
> assert(len <= sizeof(s->tx_buffer));
> - address_space_rw(&address_space_memory, tba,
> - MEMTXATTRS_UNSPECIFIED, s->tx_buffer, len, 0);
> + address_space_read(&address_space_memory, tba,
> + MEMTXATTRS_UNSPECIFIED, s->tx_buffer, len);
> DBG(PRINT_PKTHDR("Send", &s->tx_buffer));
> DBG(printf("Sending %d bytes\n", len));
> qemu_send_packet(qemu_get_queue(s->nic), s->tx_buffer, len);
> @@ -172,8 +172,8 @@ static void set_individual_address(I82596State *s, uint32_t addr)
>
> nc = qemu_get_queue(s->nic);
> m = s->conf.macaddr.a;
> - address_space_rw(&address_space_memory, addr + 8,
> - MEMTXATTRS_UNSPECIFIED, m, ETH_ALEN, 0);
> + address_space_read(&address_space_memory, addr + 8,
> + MEMTXATTRS_UNSPECIFIED, m, ETH_ALEN);
> qemu_format_nic_info_str(nc, m);
> trace_i82596_new_mac(nc->info_str);
> }
> @@ -190,9 +190,8 @@ static void set_multicast_list(I82596State *s, uint32_t addr)
> }
> for (i = 0; i < mc_count; i++) {
> uint8_t multicast_addr[ETH_ALEN];
> - address_space_rw(&address_space_memory,
> - addr + i * ETH_ALEN, MEMTXATTRS_UNSPECIFIED,
> - multicast_addr, ETH_ALEN, 0);
> + address_space_read(&address_space_memory, addr + i * ETH_ALEN,
> + MEMTXATTRS_UNSPECIFIED, multicast_addr, ETH_ALEN);
> DBG(printf("Add multicast entry " MAC_FMT "\n",
> MAC_ARG(multicast_addr)));
> unsigned mcast_idx = (net_crc32(multicast_addr, ETH_ALEN) &
> @@ -260,8 +259,8 @@ static void command_loop(I82596State *s)
> byte_cnt = MAX(byte_cnt, 4);
> byte_cnt = MIN(byte_cnt, sizeof(s->config));
> /* copy byte_cnt max. */
> - address_space_rw(&address_space_memory, s->cmd_p + 8,
> - MEMTXATTRS_UNSPECIFIED, s->config, byte_cnt, 0);
> + address_space_read(&address_space_memory, s->cmd_p + 8,
> + MEMTXATTRS_UNSPECIFIED, s->config, byte_cnt);
> /* config byte according to page 35ff */
> s->config[2] &= 0x82; /* mask valid bits */
> s->config[2] |= 0x40;
> @@ -640,14 +639,14 @@ ssize_t i82596_receive(NetClientState *nc, const uint8_t *buf, size_t sz)
> }
> rba = get_uint32(rbd + 8);
> /* printf("rba is 0x%x\n", rba); */
> - address_space_rw(&address_space_memory, rba,
> - MEMTXATTRS_UNSPECIFIED, (void *)buf, num, 1);
> + address_space_write(&address_space_memory, rba,
> + MEMTXATTRS_UNSPECIFIED, (void *)buf, num);
> rba += num;
> buf += num;
> len -= num;
> if (len == 0) { /* copy crc */
> - address_space_rw(&address_space_memory, rba - 4,
> - MEMTXATTRS_UNSPECIFIED, crc_ptr, 4, 1);
> + address_space_write(&address_space_memory, rba - 4,
> + MEMTXATTRS_UNSPECIFIED, crc_ptr, 4);
> }
>
> num |= 0x4000; /* set F BIT */
> diff --git a/hw/net/lasi_i82596.c b/hw/net/lasi_i82596.c
> index 427b3fbf701..52637a562d8 100644
> --- a/hw/net/lasi_i82596.c
> +++ b/hw/net/lasi_i82596.c
> @@ -55,8 +55,9 @@ static void lasi_82596_mem_write(void *opaque, hwaddr addr,
> * Provided for SeaBIOS only. Write MAC of Network card to addr @val.
> * Needed for the PDC_LAN_STATION_ID_READ PDC call.
> */
> - address_space_rw(&address_space_memory, val,
> - MEMTXATTRS_UNSPECIFIED, d->state.conf.macaddr.a, ETH_ALEN, 1);
> + address_space_write(&address_space_memory, val,
> + MEMTXATTRS_UNSPECIFIED, d->state.conf.macaddr.a,
> + ETH_ALEN);
> break;
> }
> }
> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
> index 5989d723c50..f150deca340 100644
> --- a/hw/ppc/pnv_lpc.c
> +++ b/hw/ppc/pnv_lpc.c
> @@ -238,16 +238,16 @@ static bool opb_read(PnvLpcController *lpc, uint32_t addr, uint8_t *data,
> int sz)
> {
> /* XXX Handle access size limits and FW read caching here */
> - return !address_space_rw(&lpc->opb_as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, sz, false);
> + return !address_space_read(&lpc->opb_as, addr, MEMTXATTRS_UNSPECIFIED,
> + data, sz);
> }
>
> static bool opb_write(PnvLpcController *lpc, uint32_t addr, uint8_t *data,
> int sz)
> {
> /* XXX Handle access size limits here */
> - return !address_space_rw(&lpc->opb_as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, sz, true);
> + return !address_space_write(&lpc->opb_as, addr, MEMTXATTRS_UNSPECIFIED,
> + data, sz);
> }
>
> #define ECCB_CTL_READ PPC_BIT(15)
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 844caab4082..0e0fccd050e 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -874,18 +874,18 @@ static inline int ida_read_next_idaw(CcwDataStream *cds)
> if (idaw_addr & 0x07 || !cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
> return -EINVAL; /* channel program check */
> }
> - ret = address_space_rw(&address_space_memory, idaw_addr,
> - MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
> - sizeof(idaw.fmt2), false);
> + ret = address_space_read(&address_space_memory, idaw_addr,
> + MEMTXATTRS_UNSPECIFIED, (void *)&idaw.fmt2,
> + sizeof(idaw.fmt2));
> cds->cda = be64_to_cpu(idaw.fmt2);
> } else {
> idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
> if (idaw_addr & 0x03 || !cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
> return -EINVAL; /* channel program check */
> }
> - ret = address_space_rw(&address_space_memory, idaw_addr,
> - MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,
> - sizeof(idaw.fmt1), false);
> + ret = address_space_read(&address_space_memory, idaw_addr,
> + MEMTXATTRS_UNSPECIFIED, (void *)&idaw.fmt1,
> + sizeof(idaw.fmt1));
> cds->cda = be64_to_cpu(idaw.fmt1);
> if (cds->cda & 0x80000000) {
> return -EINVAL; /* channel program check */
> diff --git a/qtest.c b/qtest.c
> index 12432f99cf4..328d674bcc8 100644
> --- a/qtest.c
> +++ b/qtest.c
> @@ -429,23 +429,23 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
>
> if (words[0][5] == 'b') {
> uint8_t data = value;
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - &data, 1, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + &data, 1);
> } else if (words[0][5] == 'w') {
> uint16_t data = value;
> tswap16s(&data);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &data, 2, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&data, 2);
> } else if (words[0][5] == 'l') {
> uint32_t data = value;
> tswap32s(&data);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &data, 4, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&data, 4);
> } else if (words[0][5] == 'q') {
> uint64_t data = value;
> tswap64s(&data);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &data, 8, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&data, 8);
> }
> qtest_send_prefix(chr);
> qtest_send(chr, "OK\n");
> @@ -463,22 +463,22 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
>
> if (words[0][4] == 'b') {
> uint8_t data;
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - &data, 1, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + &data, 1);
> value = data;
> } else if (words[0][4] == 'w') {
> uint16_t data;
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &data, 2, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&data, 2);
> value = tswap16(data);
> } else if (words[0][4] == 'l') {
> uint32_t data;
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &data, 4, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&data, 4);
> value = tswap32(data);
> } else if (words[0][4] == 'q') {
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &value, 8, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&value, 8);
> tswap64s(&value);
> }
> qtest_send_prefix(chr);
> @@ -498,8 +498,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
> g_assert(len);
>
> data = g_malloc(len);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, len, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
> + len);
>
> enc = g_malloc(2 * len + 1);
> for (i = 0; i < len; i++) {
> @@ -524,8 +524,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
> g_assert(ret == 0);
>
> data = g_malloc(len);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, len, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
> + len);
> b64_data = g_base64_encode(data, len);
> qtest_send_prefix(chr);
> qtest_sendf(chr, "OK %s\n", b64_data);
> @@ -559,8 +559,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
> data[i] = 0;
> }
> }
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, len, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
> + len);
> g_free(data);
>
> qtest_send_prefix(chr);
> @@ -582,8 +582,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
> if (len) {
> data = g_malloc(len);
> memset(data, pattern, len);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, len, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + data, len);
> g_free(data);
> }
>
> @@ -616,8 +616,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
> out_len = MIN(out_len, len);
> }
>
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, len, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
> + len);
>
> qtest_send_prefix(chr);
> qtest_send(chr, "OK\n");
> diff --git a/target/i386/hvf/x86_mmu.c b/target/i386/hvf/x86_mmu.c
> index d5a0efe7188..ff016fc0145 100644
> --- a/target/i386/hvf/x86_mmu.c
> +++ b/target/i386/hvf/x86_mmu.c
> @@ -88,8 +88,8 @@ static bool get_pt_entry(struct CPUState *cpu, struct gpt_translation *pt,
> }
>
> index = gpt_entry(pt->gva, level, pae);
> - address_space_rw(&address_space_memory, gpa + index * pte_size(pae),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)&pte, pte_size(pae), 0);
> + address_space_read(&address_space_memory, gpa + index * pte_size(pae),
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)&pte, pte_size(pae));
>
> pt->pte[level - 1] = pte;
>
> @@ -238,8 +238,8 @@ void vmx_write_mem(struct CPUState *cpu, target_ulong gva, void *data, int bytes
> if (!mmu_gva_to_gpa(cpu, gva, &gpa)) {
> VM_PANIC_EX("%s: mmu_gva_to_gpa %llx failed\n", __func__, gva);
> } else {
> - address_space_rw(&address_space_memory, gpa, MEMTXATTRS_UNSPECIFIED,
> - data, copy, 1);
> + address_space_write(&address_space_memory, gpa,
> + MEMTXATTRS_UNSPECIFIED, data, copy);
> }
>
> bytes -= copy;
> @@ -259,8 +259,8 @@ void vmx_read_mem(struct CPUState *cpu, void *data, target_ulong gva, int bytes)
> if (!mmu_gva_to_gpa(cpu, gva, &gpa)) {
> VM_PANIC_EX("%s: mmu_gva_to_gpa %llx failed\n", __func__, gva);
> }
> - address_space_rw(&address_space_memory, gpa, MEMTXATTRS_UNSPECIFIED,
> - data, copy, 0);
> + address_space_read(&address_space_memory, gpa, MEMTXATTRS_UNSPECIFIED,
> + data, copy);
>
> bytes -= copy;
> gva += copy;
> diff --git a/scripts/coccinelle/as_rw_const.cocci b/scripts/coccinelle/as_rw_const.cocci
> new file mode 100644
> index 00000000000..30da707701b
> --- /dev/null
> +++ b/scripts/coccinelle/as_rw_const.cocci
> @@ -0,0 +1,30 @@
> +// Avoid uses of address_space_rw() with a constant is_write argument.
> +// Usage:
> +// spatch --sp-file as-rw-const.spatch --dir . --in-place
> +
> +@@
> +expression E1, E2, E3, E4, E5;
> +symbol false;
> +@@
> +
> +- address_space_rw(E1, E2, E3, E4, E5, false)
> ++ address_space_read(E1, E2, E3, E4, E5)
> +@@
> +expression E1, E2, E3, E4, E5;
> +@@
> +
> +- address_space_rw(E1, E2, E3, E4, E5, 0)
> ++ address_space_read(E1, E2, E3, E4, E5)
> +@@
> +expression E1, E2, E3, E4, E5;
> +symbol true;
> +@@
> +
> +- address_space_rw(E1, E2, E3, E4, E5, true)
> ++ address_space_write(E1, E2, E3, E4, E5)
> +@@
> +expression E1, E2, E3, E4, E5;
> +@@
> +
> +- address_space_rw(E1, E2, E3, E4, E5, 1)
> ++ address_space_write(E1, E2, E3, E4, E5)
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] Avoid address_space_rw() with a constant is_write argument
2020-02-18 12:56 ` Philippe Mathieu-Daudé
@ 2020-02-18 13:33 ` Eric Blake
2020-02-18 13:41 ` Peter Maydell
2020-02-20 9:27 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 14+ messages in thread
From: Eric Blake @ 2020-02-18 13:33 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Peter Maydell, qemu-devel
Cc: Laurent Vivier, Thomas Huth, Alistair Francis, Eduardo Habkost,
Cornelia Huck, Halil Pasic, Christian Borntraeger,
Cédric Le Goater, Edgar E. Iglesias, Paolo Bonzini,
David Gibson
On 2/18/20 6:56 AM, Philippe Mathieu-Daudé wrote:
>> +++ b/scripts/coccinelle/as_rw_const.cocci
>> @@ -0,0 +1,30 @@
>> +// Avoid uses of address_space_rw() with a constant is_write argument.
>> +// Usage:
>> +// spatch --sp-file as-rw-const.spatch --dir . --in-place
>
> Nitpick, script is now scripts/coccinelle/as_rw_const.cocci.
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
>> +
>> +@@
>> +expression E1, E2, E3, E4, E5;
>> +symbol false;
>> +@@
>> +
>> +- address_space_rw(E1, E2, E3, E4, E5, false)
>> ++ address_space_read(E1, E2, E3, E4, E5)
>> +@@
>> +expression E1, E2, E3, E4, E5;
>> +@@
>> +
>> +- address_space_rw(E1, E2, E3, E4, E5, 0)
>> ++ address_space_read(E1, E2, E3, E4, E5)
This feels a bit redundant. Doesn't coccinelle have enough smarts about
isomorphisms (such as 0 == false, 1 == true) that it could get by with
one @@ hunk instead of 2, if we come up with the right way to represent
any isomorphism to a constant value? But admittedly, I don't know what
that representation would actually be, and your verbose patch works even
if it is not the most concise possible. So don't let my remarks hold
this patch up.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] Avoid address_space_rw() with a constant is_write argument
2020-02-18 13:33 ` Eric Blake
@ 2020-02-18 13:41 ` Peter Maydell
0 siblings, 0 replies; 14+ messages in thread
From: Peter Maydell @ 2020-02-18 13:41 UTC (permalink / raw)
To: Eric Blake
Cc: Laurent Vivier, Thomas Huth, Alistair Francis, Eduardo Habkost,
Cornelia Huck, QEMU Developers, Halil Pasic,
Christian Borntraeger, Cédric Le Goater, Edgar E. Iglesias,
Paolo Bonzini, Philippe Mathieu-Daudé,
David Gibson
On Tue, 18 Feb 2020 at 13:33, Eric Blake <eblake@redhat.com> wrote:
>
> On 2/18/20 6:56 AM, Philippe Mathieu-Daudé wrote:
>
> >> +++ b/scripts/coccinelle/as_rw_const.cocci
> >> @@ -0,0 +1,30 @@
> >> +// Avoid uses of address_space_rw() with a constant is_write argument.
> >> +// Usage:
> >> +// spatch --sp-file as-rw-const.spatch --dir . --in-place
> >
> > Nitpick, script is now scripts/coccinelle/as_rw_const.cocci.
> >
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >
> >> +
> >> +@@
> >> +expression E1, E2, E3, E4, E5;
> >> +symbol false;
> >> +@@
> >> +
> >> +- address_space_rw(E1, E2, E3, E4, E5, false)
> >> ++ address_space_read(E1, E2, E3, E4, E5)
> >> +@@
> >> +expression E1, E2, E3, E4, E5;
> >> +@@
> >> +
> >> +- address_space_rw(E1, E2, E3, E4, E5, 0)
> >> ++ address_space_read(E1, E2, E3, E4, E5)
>
> This feels a bit redundant. Doesn't coccinelle have enough smarts about
> isomorphisms (such as 0 == false, 1 == true) that it could get by with
> one @@ hunk instead of 2, if we come up with the right way to represent
> any isomorphism to a constant value? But admittedly, I don't know what
> that representation would actually be, and your verbose patch works even
> if it is not the most concise possible. So don't let my remarks hold
> this patch up.
My experience with Coccinelle has generally been that trying
to make semantic patches smaller and less redundant is futile
and a massive timesink. In this case as far as I can tell
Coccinelle has no idea at all about the existence of the 'bool'
type and that 'true' and 'false' are equivalent to 1 and 0.
Thus the 'symbol' declaration, otherwise it thinks that 'false'
is a random semantic identifier and doesn't look for a literal
match of it.
thanks
-- PMM
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] Avoid address_space_rw() with a constant is_write argument
2020-02-18 11:24 [PATCH v2] Avoid address_space_rw() with a constant is_write argument Peter Maydell
` (3 preceding siblings ...)
2020-02-18 13:30 ` Cédric Le Goater
@ 2020-02-18 14:54 ` Christian Borntraeger
2020-02-18 17:34 ` Cornelia Huck
` (2 subsequent siblings)
7 siblings, 0 replies; 14+ messages in thread
From: Christian Borntraeger @ 2020-02-18 14:54 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: Laurent Vivier, Thomas Huth, Alistair Francis, Eduardo Habkost,
Cornelia Huck, Halil Pasic, Cédric Le Goater,
Edgar E. Iglesias, Paolo Bonzini, David Gibson
On 18.02.20 12:24, Peter Maydell wrote:
> The address_space_rw() function allows either reads or writes
> depending on the is_write argument passed to it; this is useful
> when the direction of the access is determined programmatically
> (as for instance when handling the KVM_EXIT_MMIO exit reason).
> Under the hood it just calls either address_space_write() or
> address_space_read_full().
>
> We also use it a lot with a constant is_write argument, though,
> which has two issues:
> * when reading "address_space_rw(..., 1)" this is less
> immediately clear to the reader as being a write than
> "address_space_write(...)"
> * calling address_space_rw() bypasses the optimization
> in address_space_read() that fast-paths reads of a
> fixed length
>
> This commit was produced with the included Coccinelle script
> scripts/coccinelle/as-rw-const.patch.
>
> Two lines in hw/net/dp8393x.c that Coccinelle produced that
> were over 80 characters were re-wrapped by hand.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
s390 part
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
> I could break this down into separate patches by submaintainer,
> but the patch is not that large and I would argue that it's
> better for the project if we can try to avoid introducing too
> much friction into the process of doing 'safe' tree-wide
> minor refactorings.
>
> v1->v2: put the coccinelle script in scripts/coccinelle rather
> than just in the commit message.
[...]
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -874,18 +874,18 @@ static inline int ida_read_next_idaw(CcwDataStream *cds)
> if (idaw_addr & 0x07 || !cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
> return -EINVAL; /* channel program check */
> }
> - ret = address_space_rw(&address_space_memory, idaw_addr,
> - MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
> - sizeof(idaw.fmt2), false);
> + ret = address_space_read(&address_space_memory, idaw_addr,
> + MEMTXATTRS_UNSPECIFIED, (void *)&idaw.fmt2,
> + sizeof(idaw.fmt2));
> cds->cda = be64_to_cpu(idaw.fmt2);
> } else {
> idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
> if (idaw_addr & 0x03 || !cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
> return -EINVAL; /* channel program check */
> }
> - ret = address_space_rw(&address_space_memory, idaw_addr,
> - MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,
> - sizeof(idaw.fmt1), false);
> + ret = address_space_read(&address_space_memory, idaw_addr,
> + MEMTXATTRS_UNSPECIFIED, (void *)&idaw.fmt1,
> + sizeof(idaw.fmt1));
> cds->cda = be64_to_cpu(idaw.fmt1);
> if (cds->cda & 0x80000000) {
> return -EINVAL; /* channel program check */
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] Avoid address_space_rw() with a constant is_write argument
2020-02-18 11:24 [PATCH v2] Avoid address_space_rw() with a constant is_write argument Peter Maydell
` (4 preceding siblings ...)
2020-02-18 14:54 ` Christian Borntraeger
@ 2020-02-18 17:34 ` Cornelia Huck
2020-02-18 22:30 ` Alistair Francis
2020-02-18 23:00 ` David Gibson
7 siblings, 0 replies; 14+ messages in thread
From: Cornelia Huck @ 2020-02-18 17:34 UTC (permalink / raw)
To: Peter Maydell
Cc: Laurent Vivier, Thomas Huth, Eduardo Habkost, Alistair Francis,
qemu-devel, Halil Pasic, Christian Borntraeger,
Cédric Le Goater, Edgar E. Iglesias, Paolo Bonzini,
David Gibson
On Tue, 18 Feb 2020 11:24:57 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:
> The address_space_rw() function allows either reads or writes
> depending on the is_write argument passed to it; this is useful
> when the direction of the access is determined programmatically
> (as for instance when handling the KVM_EXIT_MMIO exit reason).
> Under the hood it just calls either address_space_write() or
> address_space_read_full().
>
> We also use it a lot with a constant is_write argument, though,
> which has two issues:
> * when reading "address_space_rw(..., 1)" this is less
> immediately clear to the reader as being a write than
> "address_space_write(...)"
> * calling address_space_rw() bypasses the optimization
> in address_space_read() that fast-paths reads of a
> fixed length
>
> This commit was produced with the included Coccinelle script
> scripts/coccinelle/as-rw-const.patch.
>
> Two lines in hw/net/dp8393x.c that Coccinelle produced that
> were over 80 characters were re-wrapped by hand.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I could break this down into separate patches by submaintainer,
> but the patch is not that large and I would argue that it's
> better for the project if we can try to avoid introducing too
> much friction into the process of doing 'safe' tree-wide
> minor refactorings.
>
> v1->v2: put the coccinelle script in scripts/coccinelle rather
> than just in the commit message.
> ---
> accel/kvm/kvm-all.c | 6 +--
> dma-helpers.c | 4 +-
> exec.c | 4 +-
> hw/dma/xlnx-zdma.c | 11 ++---
> hw/net/dp8393x.c | 68 ++++++++++++++--------------
> hw/net/i82596.c | 25 +++++-----
> hw/net/lasi_i82596.c | 5 +-
> hw/ppc/pnv_lpc.c | 8 ++--
> hw/s390x/css.c | 12 ++---
s390 part:
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> qtest.c | 52 ++++++++++-----------
> target/i386/hvf/x86_mmu.c | 12 ++---
> scripts/coccinelle/as_rw_const.cocci | 30 ++++++++++++
> 12 files changed, 133 insertions(+), 104 deletions(-)
> create mode 100644 scripts/coccinelle/as_rw_const.cocci
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] Avoid address_space_rw() with a constant is_write argument
2020-02-18 11:24 [PATCH v2] Avoid address_space_rw() with a constant is_write argument Peter Maydell
` (5 preceding siblings ...)
2020-02-18 17:34 ` Cornelia Huck
@ 2020-02-18 22:30 ` Alistair Francis
2020-02-18 23:00 ` David Gibson
7 siblings, 0 replies; 14+ messages in thread
From: Alistair Francis @ 2020-02-18 22:30 UTC (permalink / raw)
To: Peter Maydell
Cc: Laurent Vivier, Thomas Huth, Cornelia Huck, Eduardo Habkost,
Alistair Francis, qemu-devel@nongnu.org Developers, Halil Pasic,
Christian Borntraeger, Cédric Le Goater, Paolo Bonzini,
Edgar E. Iglesias, David Gibson
On Tue, Feb 18, 2020 at 3:25 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> The address_space_rw() function allows either reads or writes
> depending on the is_write argument passed to it; this is useful
> when the direction of the access is determined programmatically
> (as for instance when handling the KVM_EXIT_MMIO exit reason).
> Under the hood it just calls either address_space_write() or
> address_space_read_full().
>
> We also use it a lot with a constant is_write argument, though,
> which has two issues:
> * when reading "address_space_rw(..., 1)" this is less
> immediately clear to the reader as being a write than
> "address_space_write(...)"
> * calling address_space_rw() bypasses the optimization
> in address_space_read() that fast-paths reads of a
> fixed length
>
> This commit was produced with the included Coccinelle script
> scripts/coccinelle/as-rw-const.patch.
>
> Two lines in hw/net/dp8393x.c that Coccinelle produced that
> were over 80 characters were re-wrapped by hand.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> I could break this down into separate patches by submaintainer,
> but the patch is not that large and I would argue that it's
> better for the project if we can try to avoid introducing too
> much friction into the process of doing 'safe' tree-wide
> minor refactorings.
>
> v1->v2: put the coccinelle script in scripts/coccinelle rather
> than just in the commit message.
> ---
> accel/kvm/kvm-all.c | 6 +--
> dma-helpers.c | 4 +-
> exec.c | 4 +-
> hw/dma/xlnx-zdma.c | 11 ++---
> hw/net/dp8393x.c | 68 ++++++++++++++--------------
> hw/net/i82596.c | 25 +++++-----
> hw/net/lasi_i82596.c | 5 +-
> hw/ppc/pnv_lpc.c | 8 ++--
> hw/s390x/css.c | 12 ++---
> qtest.c | 52 ++++++++++-----------
> target/i386/hvf/x86_mmu.c | 12 ++---
> scripts/coccinelle/as_rw_const.cocci | 30 ++++++++++++
> 12 files changed, 133 insertions(+), 104 deletions(-)
> create mode 100644 scripts/coccinelle/as_rw_const.cocci
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index c111312dfdd..0cfe6fd8ded 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2178,9 +2178,9 @@ void kvm_flush_coalesced_mmio_buffer(void)
> ent = &ring->coalesced_mmio[ring->first];
>
> if (ent->pio == 1) {
> - address_space_rw(&address_space_io, ent->phys_addr,
> - MEMTXATTRS_UNSPECIFIED, ent->data,
> - ent->len, true);
> + address_space_write(&address_space_io, ent->phys_addr,
> + MEMTXATTRS_UNSPECIFIED, ent->data,
> + ent->len);
> } else {
> cpu_physical_memory_write(ent->phys_addr, ent->data, ent->len);
> }
> diff --git a/dma-helpers.c b/dma-helpers.c
> index d3871dc61ea..e8a26e81e16 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 8e9cc3b47cf..baefe582393 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -3810,8 +3810,8 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
> address_space_write_rom(cpu->cpu_ases[asidx].as, phys_addr,
> attrs, buf, l);
> } else {
> - address_space_rw(cpu->cpu_ases[asidx].as, phys_addr,
> - attrs, buf, l, 0);
> + address_space_read(cpu->cpu_ases[asidx].as, phys_addr, attrs, buf,
> + l);
> }
> len -= l;
> buf += l;
> diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c
> index 8fb83f5b078..31936061e21 100644
> --- a/hw/dma/xlnx-zdma.c
> +++ b/hw/dma/xlnx-zdma.c
> @@ -311,8 +311,7 @@ static bool zdma_load_descriptor(XlnxZDMA *s, uint64_t addr, void *buf)
> return false;
> }
>
> - address_space_rw(s->dma_as, addr, s->attr,
> - buf, sizeof(XlnxZDMADescr), false);
> + address_space_read(s->dma_as, addr, s->attr, buf, sizeof(XlnxZDMADescr));
> return true;
> }
>
> @@ -364,7 +363,7 @@ static uint64_t zdma_update_descr_addr(XlnxZDMA *s, bool type,
> } else {
> addr = zdma_get_regaddr64(s, basereg);
> addr += sizeof(s->dsc_dst);
> - address_space_rw(s->dma_as, addr, s->attr, (void *) &next, 8, false);
> + address_space_read(s->dma_as, addr, s->attr, (void *)&next, 8);
> zdma_put_regaddr64(s, basereg, next);
> }
> return next;
> @@ -416,8 +415,7 @@ static void zdma_write_dst(XlnxZDMA *s, uint8_t *buf, uint32_t len)
> }
> }
>
> - address_space_rw(s->dma_as, s->dsc_dst.addr, s->attr, buf, dlen,
> - true);
> + address_space_write(s->dma_as, s->dsc_dst.addr, s->attr, buf, dlen);
> if (burst_type == AXI_BURST_INCR) {
> s->dsc_dst.addr += dlen;
> }
> @@ -493,8 +491,7 @@ static void zdma_process_descr(XlnxZDMA *s)
> len = s->cfg.bus_width / 8;
> }
> } else {
> - address_space_rw(s->dma_as, src_addr, s->attr, s->buf, len,
> - false);
> + address_space_read(s->dma_as, src_addr, s->attr, s->buf, len);
> if (burst_type == AXI_BURST_INCR) {
> src_addr += len;
> }
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index a134d431ae3..f5f1c669e80 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -275,8 +275,8 @@ static void dp8393x_do_load_cam(dp8393xState *s)
>
> while (s->regs[SONIC_CDC] & 0x1f) {
> /* Fill current entry */
> - address_space_rw(&s->as, dp8393x_cdp(s),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, dp8393x_cdp(s), MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
> s->cam[index][0] = dp8393x_get(s, width, 1) & 0xff;
> s->cam[index][1] = dp8393x_get(s, width, 1) >> 8;
> s->cam[index][2] = dp8393x_get(s, width, 2) & 0xff;
> @@ -293,8 +293,8 @@ static void dp8393x_do_load_cam(dp8393xState *s)
> }
>
> /* Read CAM enable */
> - address_space_rw(&s->as, dp8393x_cdp(s),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, dp8393x_cdp(s), MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
> s->regs[SONIC_CE] = dp8393x_get(s, width, 0);
> DPRINTF("load cam done. cam enable mask 0x%04x\n", s->regs[SONIC_CE]);
>
> @@ -311,8 +311,8 @@ 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, dp8393x_rrp(s),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, dp8393x_rrp(s), MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
>
> /* Update SONIC registers */
> s->regs[SONIC_CRBA0] = dp8393x_get(s, width, 0);
> @@ -426,8 +426,8 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
> size = sizeof(uint16_t) * 6 * width;
> s->regs[SONIC_TTDA] = s->regs[SONIC_CTDA];
> DPRINTF("Transmit packet at %08x\n", dp8393x_ttda(s));
> - address_space_rw(&s->as, dp8393x_ttda(s) + sizeof(uint16_t) * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, dp8393x_ttda(s) + sizeof(uint16_t) * width,
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size);
> tx_len = 0;
>
> /* Update registers */
> @@ -451,17 +451,19 @@ 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, dp8393x_tsa(s),
> - MEMTXATTRS_UNSPECIFIED, &s->tx_buffer[tx_len], len, 0);
> + address_space_read(&s->as, dp8393x_tsa(s), 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,
> - dp8393x_ttda(s) + sizeof(uint16_t) * (4 + 3 * i) * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as,
> + dp8393x_ttda(s)
> + + sizeof(uint16_t) * (4 + 3 * i) * width,
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data,
> + size);
> s->regs[SONIC_TSA0] = dp8393x_get(s, width, 0);
> s->regs[SONIC_TSA1] = dp8393x_get(s, width, 1);
> s->regs[SONIC_TFS] = dp8393x_get(s, width, 2);
> @@ -494,18 +496,18 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
> dp8393x_put(s, width, 0,
> s->regs[SONIC_TCR] & 0x0fff); /* status */
> size = sizeof(uint16_t) * width;
> - address_space_rw(&s->as,
> - dp8393x_ttda(s),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 1);
> + address_space_write(&s->as, dp8393x_ttda(s), MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
>
> if (!(s->regs[SONIC_CR] & SONIC_CR_HTX)) {
> /* Read footer of packet */
> size = sizeof(uint16_t) * width;
> - address_space_rw(&s->as,
> - dp8393x_ttda(s) +
> - sizeof(uint16_t) *
> - (4 + 3 * s->regs[SONIC_TFC]) * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as,
> + dp8393x_ttda(s)
> + + sizeof(uint16_t) * (4 + 3 * s->regs[SONIC_TFC])
> + * width,
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data,
> + size);
> s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0) & ~0x1;
> if (dp8393x_get(s, width, 0) & 0x1) {
> /* EOL detected */
> @@ -767,8 +769,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 = dp8393x_crda(s) + sizeof(uint16_t) * 5 * width;
> - address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
> if (dp8393x_get(s, width, 0) & 0x1) {
> /* Still EOL ; stop reception */
> return -1;
> @@ -787,11 +789,11 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> /* Put packet into RBA */
> DPRINTF("Receive packet at %08x\n", dp8393x_crba(s));
> address = dp8393x_crba(s);
> - 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;
> @@ -819,13 +821,13 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> dp8393x_put(s, width, 3, s->regs[SONIC_TRBA1]); /* pkt_ptr1 */
> dp8393x_put(s, width, 4, s->regs[SONIC_RSC]); /* seq_no */
> size = sizeof(uint16_t) * 5 * width;
> - address_space_rw(&s->as, dp8393x_crda(s),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 1);
> + address_space_write(&s->as, dp8393x_crda(s), MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
>
> /* Move to next descriptor */
> size = sizeof(uint16_t) * width;
> - address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 5 * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 5 * width,
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size);
> s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0);
> if (s->regs[SONIC_LLFA] & 0x1) {
> /* EOL detected */
> @@ -838,8 +840,8 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> offset += sizeof(uint16_t);
> }
> s->data[0] = 0;
> - address_space_rw(&s->as, offset, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *)s->data, sizeof(uint16_t), 1);
> + address_space_write(&s->as, offset, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->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/net/i82596.c b/hw/net/i82596.c
> index 3a0e1ec4c05..6a80c24af23 100644
> --- a/hw/net/i82596.c
> +++ b/hw/net/i82596.c
> @@ -148,8 +148,8 @@ static void i82596_transmit(I82596State *s, uint32_t addr)
>
> if (s->nic && len) {
> assert(len <= sizeof(s->tx_buffer));
> - address_space_rw(&address_space_memory, tba,
> - MEMTXATTRS_UNSPECIFIED, s->tx_buffer, len, 0);
> + address_space_read(&address_space_memory, tba,
> + MEMTXATTRS_UNSPECIFIED, s->tx_buffer, len);
> DBG(PRINT_PKTHDR("Send", &s->tx_buffer));
> DBG(printf("Sending %d bytes\n", len));
> qemu_send_packet(qemu_get_queue(s->nic), s->tx_buffer, len);
> @@ -172,8 +172,8 @@ static void set_individual_address(I82596State *s, uint32_t addr)
>
> nc = qemu_get_queue(s->nic);
> m = s->conf.macaddr.a;
> - address_space_rw(&address_space_memory, addr + 8,
> - MEMTXATTRS_UNSPECIFIED, m, ETH_ALEN, 0);
> + address_space_read(&address_space_memory, addr + 8,
> + MEMTXATTRS_UNSPECIFIED, m, ETH_ALEN);
> qemu_format_nic_info_str(nc, m);
> trace_i82596_new_mac(nc->info_str);
> }
> @@ -190,9 +190,8 @@ static void set_multicast_list(I82596State *s, uint32_t addr)
> }
> for (i = 0; i < mc_count; i++) {
> uint8_t multicast_addr[ETH_ALEN];
> - address_space_rw(&address_space_memory,
> - addr + i * ETH_ALEN, MEMTXATTRS_UNSPECIFIED,
> - multicast_addr, ETH_ALEN, 0);
> + address_space_read(&address_space_memory, addr + i * ETH_ALEN,
> + MEMTXATTRS_UNSPECIFIED, multicast_addr, ETH_ALEN);
> DBG(printf("Add multicast entry " MAC_FMT "\n",
> MAC_ARG(multicast_addr)));
> unsigned mcast_idx = (net_crc32(multicast_addr, ETH_ALEN) &
> @@ -260,8 +259,8 @@ static void command_loop(I82596State *s)
> byte_cnt = MAX(byte_cnt, 4);
> byte_cnt = MIN(byte_cnt, sizeof(s->config));
> /* copy byte_cnt max. */
> - address_space_rw(&address_space_memory, s->cmd_p + 8,
> - MEMTXATTRS_UNSPECIFIED, s->config, byte_cnt, 0);
> + address_space_read(&address_space_memory, s->cmd_p + 8,
> + MEMTXATTRS_UNSPECIFIED, s->config, byte_cnt);
> /* config byte according to page 35ff */
> s->config[2] &= 0x82; /* mask valid bits */
> s->config[2] |= 0x40;
> @@ -640,14 +639,14 @@ ssize_t i82596_receive(NetClientState *nc, const uint8_t *buf, size_t sz)
> }
> rba = get_uint32(rbd + 8);
> /* printf("rba is 0x%x\n", rba); */
> - address_space_rw(&address_space_memory, rba,
> - MEMTXATTRS_UNSPECIFIED, (void *)buf, num, 1);
> + address_space_write(&address_space_memory, rba,
> + MEMTXATTRS_UNSPECIFIED, (void *)buf, num);
> rba += num;
> buf += num;
> len -= num;
> if (len == 0) { /* copy crc */
> - address_space_rw(&address_space_memory, rba - 4,
> - MEMTXATTRS_UNSPECIFIED, crc_ptr, 4, 1);
> + address_space_write(&address_space_memory, rba - 4,
> + MEMTXATTRS_UNSPECIFIED, crc_ptr, 4);
> }
>
> num |= 0x4000; /* set F BIT */
> diff --git a/hw/net/lasi_i82596.c b/hw/net/lasi_i82596.c
> index 427b3fbf701..52637a562d8 100644
> --- a/hw/net/lasi_i82596.c
> +++ b/hw/net/lasi_i82596.c
> @@ -55,8 +55,9 @@ static void lasi_82596_mem_write(void *opaque, hwaddr addr,
> * Provided for SeaBIOS only. Write MAC of Network card to addr @val.
> * Needed for the PDC_LAN_STATION_ID_READ PDC call.
> */
> - address_space_rw(&address_space_memory, val,
> - MEMTXATTRS_UNSPECIFIED, d->state.conf.macaddr.a, ETH_ALEN, 1);
> + address_space_write(&address_space_memory, val,
> + MEMTXATTRS_UNSPECIFIED, d->state.conf.macaddr.a,
> + ETH_ALEN);
> break;
> }
> }
> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
> index 5989d723c50..f150deca340 100644
> --- a/hw/ppc/pnv_lpc.c
> +++ b/hw/ppc/pnv_lpc.c
> @@ -238,16 +238,16 @@ static bool opb_read(PnvLpcController *lpc, uint32_t addr, uint8_t *data,
> int sz)
> {
> /* XXX Handle access size limits and FW read caching here */
> - return !address_space_rw(&lpc->opb_as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, sz, false);
> + return !address_space_read(&lpc->opb_as, addr, MEMTXATTRS_UNSPECIFIED,
> + data, sz);
> }
>
> static bool opb_write(PnvLpcController *lpc, uint32_t addr, uint8_t *data,
> int sz)
> {
> /* XXX Handle access size limits here */
> - return !address_space_rw(&lpc->opb_as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, sz, true);
> + return !address_space_write(&lpc->opb_as, addr, MEMTXATTRS_UNSPECIFIED,
> + data, sz);
> }
>
> #define ECCB_CTL_READ PPC_BIT(15)
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 844caab4082..0e0fccd050e 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -874,18 +874,18 @@ static inline int ida_read_next_idaw(CcwDataStream *cds)
> if (idaw_addr & 0x07 || !cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
> return -EINVAL; /* channel program check */
> }
> - ret = address_space_rw(&address_space_memory, idaw_addr,
> - MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
> - sizeof(idaw.fmt2), false);
> + ret = address_space_read(&address_space_memory, idaw_addr,
> + MEMTXATTRS_UNSPECIFIED, (void *)&idaw.fmt2,
> + sizeof(idaw.fmt2));
> cds->cda = be64_to_cpu(idaw.fmt2);
> } else {
> idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
> if (idaw_addr & 0x03 || !cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
> return -EINVAL; /* channel program check */
> }
> - ret = address_space_rw(&address_space_memory, idaw_addr,
> - MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,
> - sizeof(idaw.fmt1), false);
> + ret = address_space_read(&address_space_memory, idaw_addr,
> + MEMTXATTRS_UNSPECIFIED, (void *)&idaw.fmt1,
> + sizeof(idaw.fmt1));
> cds->cda = be64_to_cpu(idaw.fmt1);
> if (cds->cda & 0x80000000) {
> return -EINVAL; /* channel program check */
> diff --git a/qtest.c b/qtest.c
> index 12432f99cf4..328d674bcc8 100644
> --- a/qtest.c
> +++ b/qtest.c
> @@ -429,23 +429,23 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
>
> if (words[0][5] == 'b') {
> uint8_t data = value;
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - &data, 1, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + &data, 1);
> } else if (words[0][5] == 'w') {
> uint16_t data = value;
> tswap16s(&data);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &data, 2, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&data, 2);
> } else if (words[0][5] == 'l') {
> uint32_t data = value;
> tswap32s(&data);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &data, 4, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&data, 4);
> } else if (words[0][5] == 'q') {
> uint64_t data = value;
> tswap64s(&data);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &data, 8, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&data, 8);
> }
> qtest_send_prefix(chr);
> qtest_send(chr, "OK\n");
> @@ -463,22 +463,22 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
>
> if (words[0][4] == 'b') {
> uint8_t data;
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - &data, 1, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + &data, 1);
> value = data;
> } else if (words[0][4] == 'w') {
> uint16_t data;
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &data, 2, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&data, 2);
> value = tswap16(data);
> } else if (words[0][4] == 'l') {
> uint32_t data;
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &data, 4, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&data, 4);
> value = tswap32(data);
> } else if (words[0][4] == 'q') {
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &value, 8, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&value, 8);
> tswap64s(&value);
> }
> qtest_send_prefix(chr);
> @@ -498,8 +498,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
> g_assert(len);
>
> data = g_malloc(len);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, len, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
> + len);
>
> enc = g_malloc(2 * len + 1);
> for (i = 0; i < len; i++) {
> @@ -524,8 +524,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
> g_assert(ret == 0);
>
> data = g_malloc(len);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, len, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
> + len);
> b64_data = g_base64_encode(data, len);
> qtest_send_prefix(chr);
> qtest_sendf(chr, "OK %s\n", b64_data);
> @@ -559,8 +559,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
> data[i] = 0;
> }
> }
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, len, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
> + len);
> g_free(data);
>
> qtest_send_prefix(chr);
> @@ -582,8 +582,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
> if (len) {
> data = g_malloc(len);
> memset(data, pattern, len);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, len, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + data, len);
> g_free(data);
> }
>
> @@ -616,8 +616,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
> out_len = MIN(out_len, len);
> }
>
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, len, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
> + len);
>
> qtest_send_prefix(chr);
> qtest_send(chr, "OK\n");
> diff --git a/target/i386/hvf/x86_mmu.c b/target/i386/hvf/x86_mmu.c
> index d5a0efe7188..ff016fc0145 100644
> --- a/target/i386/hvf/x86_mmu.c
> +++ b/target/i386/hvf/x86_mmu.c
> @@ -88,8 +88,8 @@ static bool get_pt_entry(struct CPUState *cpu, struct gpt_translation *pt,
> }
>
> index = gpt_entry(pt->gva, level, pae);
> - address_space_rw(&address_space_memory, gpa + index * pte_size(pae),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)&pte, pte_size(pae), 0);
> + address_space_read(&address_space_memory, gpa + index * pte_size(pae),
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)&pte, pte_size(pae));
>
> pt->pte[level - 1] = pte;
>
> @@ -238,8 +238,8 @@ void vmx_write_mem(struct CPUState *cpu, target_ulong gva, void *data, int bytes
> if (!mmu_gva_to_gpa(cpu, gva, &gpa)) {
> VM_PANIC_EX("%s: mmu_gva_to_gpa %llx failed\n", __func__, gva);
> } else {
> - address_space_rw(&address_space_memory, gpa, MEMTXATTRS_UNSPECIFIED,
> - data, copy, 1);
> + address_space_write(&address_space_memory, gpa,
> + MEMTXATTRS_UNSPECIFIED, data, copy);
> }
>
> bytes -= copy;
> @@ -259,8 +259,8 @@ void vmx_read_mem(struct CPUState *cpu, void *data, target_ulong gva, int bytes)
> if (!mmu_gva_to_gpa(cpu, gva, &gpa)) {
> VM_PANIC_EX("%s: mmu_gva_to_gpa %llx failed\n", __func__, gva);
> }
> - address_space_rw(&address_space_memory, gpa, MEMTXATTRS_UNSPECIFIED,
> - data, copy, 0);
> + address_space_read(&address_space_memory, gpa, MEMTXATTRS_UNSPECIFIED,
> + data, copy);
>
> bytes -= copy;
> gva += copy;
> diff --git a/scripts/coccinelle/as_rw_const.cocci b/scripts/coccinelle/as_rw_const.cocci
> new file mode 100644
> index 00000000000..30da707701b
> --- /dev/null
> +++ b/scripts/coccinelle/as_rw_const.cocci
> @@ -0,0 +1,30 @@
> +// Avoid uses of address_space_rw() with a constant is_write argument.
> +// Usage:
> +// spatch --sp-file as-rw-const.spatch --dir . --in-place
> +
> +@@
> +expression E1, E2, E3, E4, E5;
> +symbol false;
> +@@
> +
> +- address_space_rw(E1, E2, E3, E4, E5, false)
> ++ address_space_read(E1, E2, E3, E4, E5)
> +@@
> +expression E1, E2, E3, E4, E5;
> +@@
> +
> +- address_space_rw(E1, E2, E3, E4, E5, 0)
> ++ address_space_read(E1, E2, E3, E4, E5)
> +@@
> +expression E1, E2, E3, E4, E5;
> +symbol true;
> +@@
> +
> +- address_space_rw(E1, E2, E3, E4, E5, true)
> ++ address_space_write(E1, E2, E3, E4, E5)
> +@@
> +expression E1, E2, E3, E4, E5;
> +@@
> +
> +- address_space_rw(E1, E2, E3, E4, E5, 1)
> ++ address_space_write(E1, E2, E3, E4, E5)
> --
> 2.20.1
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] Avoid address_space_rw() with a constant is_write argument
2020-02-18 11:24 [PATCH v2] Avoid address_space_rw() with a constant is_write argument Peter Maydell
` (6 preceding siblings ...)
2020-02-18 22:30 ` Alistair Francis
@ 2020-02-18 23:00 ` David Gibson
7 siblings, 0 replies; 14+ messages in thread
From: David Gibson @ 2020-02-18 23:00 UTC (permalink / raw)
To: Peter Maydell
Cc: Laurent Vivier, Thomas Huth, Alistair Francis, Eduardo Habkost,
Cornelia Huck, qemu-devel, Halil Pasic, Christian Borntraeger,
Cédric Le Goater, Edgar E. Iglesias, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 30920 bytes --]
On Tue, Feb 18, 2020 at 11:24:57AM +0000, Peter Maydell wrote:
> The address_space_rw() function allows either reads or writes
> depending on the is_write argument passed to it; this is useful
> when the direction of the access is determined programmatically
> (as for instance when handling the KVM_EXIT_MMIO exit reason).
> Under the hood it just calls either address_space_write() or
> address_space_read_full().
>
> We also use it a lot with a constant is_write argument, though,
> which has two issues:
> * when reading "address_space_rw(..., 1)" this is less
> immediately clear to the reader as being a write than
> "address_space_write(...)"
> * calling address_space_rw() bypasses the optimization
> in address_space_read() that fast-paths reads of a
> fixed length
>
> This commit was produced with the included Coccinelle script
> scripts/coccinelle/as-rw-const.patch.
>
> Two lines in hw/net/dp8393x.c that Coccinelle produced that
> were over 80 characters were re-wrapped by hand.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
ppc parts
Acked-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> I could break this down into separate patches by submaintainer,
> but the patch is not that large and I would argue that it's
> better for the project if we can try to avoid introducing too
> much friction into the process of doing 'safe' tree-wide
> minor refactorings.
>
> v1->v2: put the coccinelle script in scripts/coccinelle rather
> than just in the commit message.
> ---
> accel/kvm/kvm-all.c | 6 +--
> dma-helpers.c | 4 +-
> exec.c | 4 +-
> hw/dma/xlnx-zdma.c | 11 ++---
> hw/net/dp8393x.c | 68 ++++++++++++++--------------
> hw/net/i82596.c | 25 +++++-----
> hw/net/lasi_i82596.c | 5 +-
> hw/ppc/pnv_lpc.c | 8 ++--
> hw/s390x/css.c | 12 ++---
> qtest.c | 52 ++++++++++-----------
> target/i386/hvf/x86_mmu.c | 12 ++---
> scripts/coccinelle/as_rw_const.cocci | 30 ++++++++++++
> 12 files changed, 133 insertions(+), 104 deletions(-)
> create mode 100644 scripts/coccinelle/as_rw_const.cocci
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index c111312dfdd..0cfe6fd8ded 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2178,9 +2178,9 @@ void kvm_flush_coalesced_mmio_buffer(void)
> ent = &ring->coalesced_mmio[ring->first];
>
> if (ent->pio == 1) {
> - address_space_rw(&address_space_io, ent->phys_addr,
> - MEMTXATTRS_UNSPECIFIED, ent->data,
> - ent->len, true);
> + address_space_write(&address_space_io, ent->phys_addr,
> + MEMTXATTRS_UNSPECIFIED, ent->data,
> + ent->len);
> } else {
> cpu_physical_memory_write(ent->phys_addr, ent->data, ent->len);
> }
> diff --git a/dma-helpers.c b/dma-helpers.c
> index d3871dc61ea..e8a26e81e16 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 8e9cc3b47cf..baefe582393 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -3810,8 +3810,8 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
> address_space_write_rom(cpu->cpu_ases[asidx].as, phys_addr,
> attrs, buf, l);
> } else {
> - address_space_rw(cpu->cpu_ases[asidx].as, phys_addr,
> - attrs, buf, l, 0);
> + address_space_read(cpu->cpu_ases[asidx].as, phys_addr, attrs, buf,
> + l);
> }
> len -= l;
> buf += l;
> diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c
> index 8fb83f5b078..31936061e21 100644
> --- a/hw/dma/xlnx-zdma.c
> +++ b/hw/dma/xlnx-zdma.c
> @@ -311,8 +311,7 @@ static bool zdma_load_descriptor(XlnxZDMA *s, uint64_t addr, void *buf)
> return false;
> }
>
> - address_space_rw(s->dma_as, addr, s->attr,
> - buf, sizeof(XlnxZDMADescr), false);
> + address_space_read(s->dma_as, addr, s->attr, buf, sizeof(XlnxZDMADescr));
> return true;
> }
>
> @@ -364,7 +363,7 @@ static uint64_t zdma_update_descr_addr(XlnxZDMA *s, bool type,
> } else {
> addr = zdma_get_regaddr64(s, basereg);
> addr += sizeof(s->dsc_dst);
> - address_space_rw(s->dma_as, addr, s->attr, (void *) &next, 8, false);
> + address_space_read(s->dma_as, addr, s->attr, (void *)&next, 8);
> zdma_put_regaddr64(s, basereg, next);
> }
> return next;
> @@ -416,8 +415,7 @@ static void zdma_write_dst(XlnxZDMA *s, uint8_t *buf, uint32_t len)
> }
> }
>
> - address_space_rw(s->dma_as, s->dsc_dst.addr, s->attr, buf, dlen,
> - true);
> + address_space_write(s->dma_as, s->dsc_dst.addr, s->attr, buf, dlen);
> if (burst_type == AXI_BURST_INCR) {
> s->dsc_dst.addr += dlen;
> }
> @@ -493,8 +491,7 @@ static void zdma_process_descr(XlnxZDMA *s)
> len = s->cfg.bus_width / 8;
> }
> } else {
> - address_space_rw(s->dma_as, src_addr, s->attr, s->buf, len,
> - false);
> + address_space_read(s->dma_as, src_addr, s->attr, s->buf, len);
> if (burst_type == AXI_BURST_INCR) {
> src_addr += len;
> }
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index a134d431ae3..f5f1c669e80 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -275,8 +275,8 @@ static void dp8393x_do_load_cam(dp8393xState *s)
>
> while (s->regs[SONIC_CDC] & 0x1f) {
> /* Fill current entry */
> - address_space_rw(&s->as, dp8393x_cdp(s),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, dp8393x_cdp(s), MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
> s->cam[index][0] = dp8393x_get(s, width, 1) & 0xff;
> s->cam[index][1] = dp8393x_get(s, width, 1) >> 8;
> s->cam[index][2] = dp8393x_get(s, width, 2) & 0xff;
> @@ -293,8 +293,8 @@ static void dp8393x_do_load_cam(dp8393xState *s)
> }
>
> /* Read CAM enable */
> - address_space_rw(&s->as, dp8393x_cdp(s),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, dp8393x_cdp(s), MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
> s->regs[SONIC_CE] = dp8393x_get(s, width, 0);
> DPRINTF("load cam done. cam enable mask 0x%04x\n", s->regs[SONIC_CE]);
>
> @@ -311,8 +311,8 @@ 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, dp8393x_rrp(s),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, dp8393x_rrp(s), MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
>
> /* Update SONIC registers */
> s->regs[SONIC_CRBA0] = dp8393x_get(s, width, 0);
> @@ -426,8 +426,8 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
> size = sizeof(uint16_t) * 6 * width;
> s->regs[SONIC_TTDA] = s->regs[SONIC_CTDA];
> DPRINTF("Transmit packet at %08x\n", dp8393x_ttda(s));
> - address_space_rw(&s->as, dp8393x_ttda(s) + sizeof(uint16_t) * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, dp8393x_ttda(s) + sizeof(uint16_t) * width,
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size);
> tx_len = 0;
>
> /* Update registers */
> @@ -451,17 +451,19 @@ 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, dp8393x_tsa(s),
> - MEMTXATTRS_UNSPECIFIED, &s->tx_buffer[tx_len], len, 0);
> + address_space_read(&s->as, dp8393x_tsa(s), 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,
> - dp8393x_ttda(s) + sizeof(uint16_t) * (4 + 3 * i) * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as,
> + dp8393x_ttda(s)
> + + sizeof(uint16_t) * (4 + 3 * i) * width,
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data,
> + size);
> s->regs[SONIC_TSA0] = dp8393x_get(s, width, 0);
> s->regs[SONIC_TSA1] = dp8393x_get(s, width, 1);
> s->regs[SONIC_TFS] = dp8393x_get(s, width, 2);
> @@ -494,18 +496,18 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
> dp8393x_put(s, width, 0,
> s->regs[SONIC_TCR] & 0x0fff); /* status */
> size = sizeof(uint16_t) * width;
> - address_space_rw(&s->as,
> - dp8393x_ttda(s),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 1);
> + address_space_write(&s->as, dp8393x_ttda(s), MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
>
> if (!(s->regs[SONIC_CR] & SONIC_CR_HTX)) {
> /* Read footer of packet */
> size = sizeof(uint16_t) * width;
> - address_space_rw(&s->as,
> - dp8393x_ttda(s) +
> - sizeof(uint16_t) *
> - (4 + 3 * s->regs[SONIC_TFC]) * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as,
> + dp8393x_ttda(s)
> + + sizeof(uint16_t) * (4 + 3 * s->regs[SONIC_TFC])
> + * width,
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data,
> + size);
> s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0) & ~0x1;
> if (dp8393x_get(s, width, 0) & 0x1) {
> /* EOL detected */
> @@ -767,8 +769,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 = dp8393x_crda(s) + sizeof(uint16_t) * 5 * width;
> - address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
> if (dp8393x_get(s, width, 0) & 0x1) {
> /* Still EOL ; stop reception */
> return -1;
> @@ -787,11 +789,11 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> /* Put packet into RBA */
> DPRINTF("Receive packet at %08x\n", dp8393x_crba(s));
> address = dp8393x_crba(s);
> - 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;
> @@ -819,13 +821,13 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> dp8393x_put(s, width, 3, s->regs[SONIC_TRBA1]); /* pkt_ptr1 */
> dp8393x_put(s, width, 4, s->regs[SONIC_RSC]); /* seq_no */
> size = sizeof(uint16_t) * 5 * width;
> - address_space_rw(&s->as, dp8393x_crda(s),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 1);
> + address_space_write(&s->as, dp8393x_crda(s), MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
>
> /* Move to next descriptor */
> size = sizeof(uint16_t) * width;
> - address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 5 * width,
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> + address_space_read(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 5 * width,
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size);
> s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0);
> if (s->regs[SONIC_LLFA] & 0x1) {
> /* EOL detected */
> @@ -838,8 +840,8 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> offset += sizeof(uint16_t);
> }
> s->data[0] = 0;
> - address_space_rw(&s->as, offset, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *)s->data, sizeof(uint16_t), 1);
> + address_space_write(&s->as, offset, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->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/net/i82596.c b/hw/net/i82596.c
> index 3a0e1ec4c05..6a80c24af23 100644
> --- a/hw/net/i82596.c
> +++ b/hw/net/i82596.c
> @@ -148,8 +148,8 @@ static void i82596_transmit(I82596State *s, uint32_t addr)
>
> if (s->nic && len) {
> assert(len <= sizeof(s->tx_buffer));
> - address_space_rw(&address_space_memory, tba,
> - MEMTXATTRS_UNSPECIFIED, s->tx_buffer, len, 0);
> + address_space_read(&address_space_memory, tba,
> + MEMTXATTRS_UNSPECIFIED, s->tx_buffer, len);
> DBG(PRINT_PKTHDR("Send", &s->tx_buffer));
> DBG(printf("Sending %d bytes\n", len));
> qemu_send_packet(qemu_get_queue(s->nic), s->tx_buffer, len);
> @@ -172,8 +172,8 @@ static void set_individual_address(I82596State *s, uint32_t addr)
>
> nc = qemu_get_queue(s->nic);
> m = s->conf.macaddr.a;
> - address_space_rw(&address_space_memory, addr + 8,
> - MEMTXATTRS_UNSPECIFIED, m, ETH_ALEN, 0);
> + address_space_read(&address_space_memory, addr + 8,
> + MEMTXATTRS_UNSPECIFIED, m, ETH_ALEN);
> qemu_format_nic_info_str(nc, m);
> trace_i82596_new_mac(nc->info_str);
> }
> @@ -190,9 +190,8 @@ static void set_multicast_list(I82596State *s, uint32_t addr)
> }
> for (i = 0; i < mc_count; i++) {
> uint8_t multicast_addr[ETH_ALEN];
> - address_space_rw(&address_space_memory,
> - addr + i * ETH_ALEN, MEMTXATTRS_UNSPECIFIED,
> - multicast_addr, ETH_ALEN, 0);
> + address_space_read(&address_space_memory, addr + i * ETH_ALEN,
> + MEMTXATTRS_UNSPECIFIED, multicast_addr, ETH_ALEN);
> DBG(printf("Add multicast entry " MAC_FMT "\n",
> MAC_ARG(multicast_addr)));
> unsigned mcast_idx = (net_crc32(multicast_addr, ETH_ALEN) &
> @@ -260,8 +259,8 @@ static void command_loop(I82596State *s)
> byte_cnt = MAX(byte_cnt, 4);
> byte_cnt = MIN(byte_cnt, sizeof(s->config));
> /* copy byte_cnt max. */
> - address_space_rw(&address_space_memory, s->cmd_p + 8,
> - MEMTXATTRS_UNSPECIFIED, s->config, byte_cnt, 0);
> + address_space_read(&address_space_memory, s->cmd_p + 8,
> + MEMTXATTRS_UNSPECIFIED, s->config, byte_cnt);
> /* config byte according to page 35ff */
> s->config[2] &= 0x82; /* mask valid bits */
> s->config[2] |= 0x40;
> @@ -640,14 +639,14 @@ ssize_t i82596_receive(NetClientState *nc, const uint8_t *buf, size_t sz)
> }
> rba = get_uint32(rbd + 8);
> /* printf("rba is 0x%x\n", rba); */
> - address_space_rw(&address_space_memory, rba,
> - MEMTXATTRS_UNSPECIFIED, (void *)buf, num, 1);
> + address_space_write(&address_space_memory, rba,
> + MEMTXATTRS_UNSPECIFIED, (void *)buf, num);
> rba += num;
> buf += num;
> len -= num;
> if (len == 0) { /* copy crc */
> - address_space_rw(&address_space_memory, rba - 4,
> - MEMTXATTRS_UNSPECIFIED, crc_ptr, 4, 1);
> + address_space_write(&address_space_memory, rba - 4,
> + MEMTXATTRS_UNSPECIFIED, crc_ptr, 4);
> }
>
> num |= 0x4000; /* set F BIT */
> diff --git a/hw/net/lasi_i82596.c b/hw/net/lasi_i82596.c
> index 427b3fbf701..52637a562d8 100644
> --- a/hw/net/lasi_i82596.c
> +++ b/hw/net/lasi_i82596.c
> @@ -55,8 +55,9 @@ static void lasi_82596_mem_write(void *opaque, hwaddr addr,
> * Provided for SeaBIOS only. Write MAC of Network card to addr @val.
> * Needed for the PDC_LAN_STATION_ID_READ PDC call.
> */
> - address_space_rw(&address_space_memory, val,
> - MEMTXATTRS_UNSPECIFIED, d->state.conf.macaddr.a, ETH_ALEN, 1);
> + address_space_write(&address_space_memory, val,
> + MEMTXATTRS_UNSPECIFIED, d->state.conf.macaddr.a,
> + ETH_ALEN);
> break;
> }
> }
> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
> index 5989d723c50..f150deca340 100644
> --- a/hw/ppc/pnv_lpc.c
> +++ b/hw/ppc/pnv_lpc.c
> @@ -238,16 +238,16 @@ static bool opb_read(PnvLpcController *lpc, uint32_t addr, uint8_t *data,
> int sz)
> {
> /* XXX Handle access size limits and FW read caching here */
> - return !address_space_rw(&lpc->opb_as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, sz, false);
> + return !address_space_read(&lpc->opb_as, addr, MEMTXATTRS_UNSPECIFIED,
> + data, sz);
> }
>
> static bool opb_write(PnvLpcController *lpc, uint32_t addr, uint8_t *data,
> int sz)
> {
> /* XXX Handle access size limits here */
> - return !address_space_rw(&lpc->opb_as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, sz, true);
> + return !address_space_write(&lpc->opb_as, addr, MEMTXATTRS_UNSPECIFIED,
> + data, sz);
> }
>
> #define ECCB_CTL_READ PPC_BIT(15)
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 844caab4082..0e0fccd050e 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -874,18 +874,18 @@ static inline int ida_read_next_idaw(CcwDataStream *cds)
> if (idaw_addr & 0x07 || !cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
> return -EINVAL; /* channel program check */
> }
> - ret = address_space_rw(&address_space_memory, idaw_addr,
> - MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
> - sizeof(idaw.fmt2), false);
> + ret = address_space_read(&address_space_memory, idaw_addr,
> + MEMTXATTRS_UNSPECIFIED, (void *)&idaw.fmt2,
> + sizeof(idaw.fmt2));
> cds->cda = be64_to_cpu(idaw.fmt2);
> } else {
> idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
> if (idaw_addr & 0x03 || !cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
> return -EINVAL; /* channel program check */
> }
> - ret = address_space_rw(&address_space_memory, idaw_addr,
> - MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,
> - sizeof(idaw.fmt1), false);
> + ret = address_space_read(&address_space_memory, idaw_addr,
> + MEMTXATTRS_UNSPECIFIED, (void *)&idaw.fmt1,
> + sizeof(idaw.fmt1));
> cds->cda = be64_to_cpu(idaw.fmt1);
> if (cds->cda & 0x80000000) {
> return -EINVAL; /* channel program check */
> diff --git a/qtest.c b/qtest.c
> index 12432f99cf4..328d674bcc8 100644
> --- a/qtest.c
> +++ b/qtest.c
> @@ -429,23 +429,23 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
>
> if (words[0][5] == 'b') {
> uint8_t data = value;
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - &data, 1, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + &data, 1);
> } else if (words[0][5] == 'w') {
> uint16_t data = value;
> tswap16s(&data);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &data, 2, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&data, 2);
> } else if (words[0][5] == 'l') {
> uint32_t data = value;
> tswap32s(&data);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &data, 4, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&data, 4);
> } else if (words[0][5] == 'q') {
> uint64_t data = value;
> tswap64s(&data);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &data, 8, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&data, 8);
> }
> qtest_send_prefix(chr);
> qtest_send(chr, "OK\n");
> @@ -463,22 +463,22 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
>
> if (words[0][4] == 'b') {
> uint8_t data;
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - &data, 1, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + &data, 1);
> value = data;
> } else if (words[0][4] == 'w') {
> uint16_t data;
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &data, 2, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&data, 2);
> value = tswap16(data);
> } else if (words[0][4] == 'l') {
> uint32_t data;
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &data, 4, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&data, 4);
> value = tswap32(data);
> } else if (words[0][4] == 'q') {
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *) &value, 8, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)&value, 8);
> tswap64s(&value);
> }
> qtest_send_prefix(chr);
> @@ -498,8 +498,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
> g_assert(len);
>
> data = g_malloc(len);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, len, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
> + len);
>
> enc = g_malloc(2 * len + 1);
> for (i = 0; i < len; i++) {
> @@ -524,8 +524,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
> g_assert(ret == 0);
>
> data = g_malloc(len);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, len, false);
> + address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
> + len);
> b64_data = g_base64_encode(data, len);
> qtest_send_prefix(chr);
> qtest_sendf(chr, "OK %s\n", b64_data);
> @@ -559,8 +559,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
> data[i] = 0;
> }
> }
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, len, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
> + len);
> g_free(data);
>
> qtest_send_prefix(chr);
> @@ -582,8 +582,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
> if (len) {
> data = g_malloc(len);
> memset(data, pattern, len);
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, len, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> + data, len);
> g_free(data);
> }
>
> @@ -616,8 +616,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
> out_len = MIN(out_len, len);
> }
>
> - address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> - data, len, true);
> + address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
> + len);
>
> qtest_send_prefix(chr);
> qtest_send(chr, "OK\n");
> diff --git a/target/i386/hvf/x86_mmu.c b/target/i386/hvf/x86_mmu.c
> index d5a0efe7188..ff016fc0145 100644
> --- a/target/i386/hvf/x86_mmu.c
> +++ b/target/i386/hvf/x86_mmu.c
> @@ -88,8 +88,8 @@ static bool get_pt_entry(struct CPUState *cpu, struct gpt_translation *pt,
> }
>
> index = gpt_entry(pt->gva, level, pae);
> - address_space_rw(&address_space_memory, gpa + index * pte_size(pae),
> - MEMTXATTRS_UNSPECIFIED, (uint8_t *)&pte, pte_size(pae), 0);
> + address_space_read(&address_space_memory, gpa + index * pte_size(pae),
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)&pte, pte_size(pae));
>
> pt->pte[level - 1] = pte;
>
> @@ -238,8 +238,8 @@ void vmx_write_mem(struct CPUState *cpu, target_ulong gva, void *data, int bytes
> if (!mmu_gva_to_gpa(cpu, gva, &gpa)) {
> VM_PANIC_EX("%s: mmu_gva_to_gpa %llx failed\n", __func__, gva);
> } else {
> - address_space_rw(&address_space_memory, gpa, MEMTXATTRS_UNSPECIFIED,
> - data, copy, 1);
> + address_space_write(&address_space_memory, gpa,
> + MEMTXATTRS_UNSPECIFIED, data, copy);
> }
>
> bytes -= copy;
> @@ -259,8 +259,8 @@ void vmx_read_mem(struct CPUState *cpu, void *data, target_ulong gva, int bytes)
> if (!mmu_gva_to_gpa(cpu, gva, &gpa)) {
> VM_PANIC_EX("%s: mmu_gva_to_gpa %llx failed\n", __func__, gva);
> }
> - address_space_rw(&address_space_memory, gpa, MEMTXATTRS_UNSPECIFIED,
> - data, copy, 0);
> + address_space_read(&address_space_memory, gpa, MEMTXATTRS_UNSPECIFIED,
> + data, copy);
>
> bytes -= copy;
> gva += copy;
> diff --git a/scripts/coccinelle/as_rw_const.cocci b/scripts/coccinelle/as_rw_const.cocci
> new file mode 100644
> index 00000000000..30da707701b
> --- /dev/null
> +++ b/scripts/coccinelle/as_rw_const.cocci
> @@ -0,0 +1,30 @@
> +// Avoid uses of address_space_rw() with a constant is_write argument.
> +// Usage:
> +// spatch --sp-file as-rw-const.spatch --dir . --in-place
> +
> +@@
> +expression E1, E2, E3, E4, E5;
> +symbol false;
> +@@
> +
> +- address_space_rw(E1, E2, E3, E4, E5, false)
> ++ address_space_read(E1, E2, E3, E4, E5)
> +@@
> +expression E1, E2, E3, E4, E5;
> +@@
> +
> +- address_space_rw(E1, E2, E3, E4, E5, 0)
> ++ address_space_read(E1, E2, E3, E4, E5)
> +@@
> +expression E1, E2, E3, E4, E5;
> +symbol true;
> +@@
> +
> +- address_space_rw(E1, E2, E3, E4, E5, true)
> ++ address_space_write(E1, E2, E3, E4, E5)
> +@@
> +expression E1, E2, E3, E4, E5;
> +@@
> +
> +- address_space_rw(E1, E2, E3, E4, E5, 1)
> ++ address_space_write(E1, E2, E3, E4, E5)
--
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: 833 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] Avoid address_space_rw() with a constant is_write argument
2020-02-18 12:56 ` Philippe Mathieu-Daudé
2020-02-18 13:33 ` Eric Blake
@ 2020-02-20 9:27 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 14+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-02-20 9:27 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: Laurent Vivier, Thomas Huth, Cornelia Huck, Eduardo Habkost,
Alistair Francis, Halil Pasic, Christian Borntraeger,
Cédric Le Goater, Paolo Bonzini, Edgar E. Iglesias,
David Gibson
On 2/18/20 1:56 PM, Philippe Mathieu-Daudé wrote:
> On 2/18/20 12:24 PM, Peter Maydell wrote:
>> The address_space_rw() function allows either reads or writes
>> depending on the is_write argument passed to it; this is useful
>> when the direction of the access is determined programmatically
>> (as for instance when handling the KVM_EXIT_MMIO exit reason).
>> Under the hood it just calls either address_space_write() or
>> address_space_read_full().
>>
>> We also use it a lot with a constant is_write argument, though,
>> which has two issues:
>> * when reading "address_space_rw(..., 1)" this is less
>> immediately clear to the reader as being a write than
>> "address_space_write(...)"
>> * calling address_space_rw() bypasses the optimization
>> in address_space_read() that fast-paths reads of a
>> fixed length
>>
>> This commit was produced with the included Coccinelle script
>> scripts/coccinelle/as-rw-const.patch.
Script is "scripts/coccinelle/as_rw_const.cocci".
I plan to respin this patch (fixed) in a larger series.
>>
>> Two lines in hw/net/dp8393x.c that Coccinelle produced that
>> were over 80 characters were re-wrapped by hand.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> I could break this down into separate patches by submaintainer,
>> but the patch is not that large and I would argue that it's
>> better for the project if we can try to avoid introducing too
>> much friction into the process of doing 'safe' tree-wide
>> minor refactorings.
>>
>> v1->v2: put the coccinelle script in scripts/coccinelle rather
>> than just in the commit message.
>> ---
>> accel/kvm/kvm-all.c | 6 +--
>> dma-helpers.c | 4 +-
>> exec.c | 4 +-
>> hw/dma/xlnx-zdma.c | 11 ++---
>> hw/net/dp8393x.c | 68 ++++++++++++++--------------
>> hw/net/i82596.c | 25 +++++-----
>> hw/net/lasi_i82596.c | 5 +-
>> hw/ppc/pnv_lpc.c | 8 ++--
>> hw/s390x/css.c | 12 ++---
>> qtest.c | 52 ++++++++++-----------
>> target/i386/hvf/x86_mmu.c | 12 ++---
>> scripts/coccinelle/as_rw_const.cocci | 30 ++++++++++++
>> 12 files changed, 133 insertions(+), 104 deletions(-)
>> create mode 100644 scripts/coccinelle/as_rw_const.cocci
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] Avoid address_space_rw() with a constant is_write argument
2020-02-18 13:13 ` Laurent Vivier
2020-02-18 13:20 ` Peter Maydell
@ 2020-02-20 11:28 ` Paolo Bonzini
1 sibling, 0 replies; 14+ messages in thread
From: Paolo Bonzini @ 2020-02-20 11:28 UTC (permalink / raw)
To: Laurent Vivier, Peter Maydell, qemu-devel
Cc: Thomas Huth, Alistair Francis, Eduardo Habkost, Cornelia Huck,
Halil Pasic, Christian Borntraeger, Cédric Le Goater,
Edgar E. Iglesias, David Gibson
On 18/02/20 14:13, Laurent Vivier wrote:
> There is one in target/i386/hvf/vmx.h: macvm_set_cr0() you didn't change.
>
> You must update the script name in the script comment (as suggested by
> Philippe) and in the commit message.
>
> Anyway:
>
> Reviewed-by: Laurent Vivier <lvivier@redhat.com>
Thanks, I squashed the change to vmx.h.
Paolo
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-02-20 11:29 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-18 11:24 [PATCH v2] Avoid address_space_rw() with a constant is_write argument Peter Maydell
2020-02-16 17:24 ` Edgar E. Iglesias
2020-02-18 12:56 ` Philippe Mathieu-Daudé
2020-02-18 13:33 ` Eric Blake
2020-02-18 13:41 ` Peter Maydell
2020-02-20 9:27 ` Philippe Mathieu-Daudé
2020-02-18 13:13 ` Laurent Vivier
2020-02-18 13:20 ` Peter Maydell
2020-02-20 11:28 ` Paolo Bonzini
2020-02-18 13:30 ` Cédric Le Goater
2020-02-18 14:54 ` Christian Borntraeger
2020-02-18 17:34 ` Cornelia Huck
2020-02-18 22:30 ` Alistair Francis
2020-02-18 23:00 ` David Gibson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.