Re: [Qemu-devel] [PATCH v2 06/14] exec.c: Make address_space_rw take transaction attributes

["Alex Bennée" <alex.bennee@linaro.org>]

Peter Maydell <peter.maydell@linaro.org> writes:

> Make address_space_rw take transaction attributes, rather
> than always using the 'unspecified' attributes.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  dma-helpers.c            |  3 ++-
>  exec.c                   | 51 ++++++++++++++++++++++++++----------------------
>  hw/mips/mips_jazz.c      |  6 ++++--
>  hw/pci-host/prep.c       |  6 ++++--
>  include/exec/memory.h    | 31 ++++++++++++++++++-----------
>  include/sysemu/dma.h     |  3 ++-
>  ioport.c                 | 16 +++++++++------
>  kvm-all.c                |  3 ++-
>  scripts/coverity-model.c |  8 +++++---
>  9 files changed, 77 insertions(+), 50 deletions(-)
>
<snip>
> diff --git a/include/sysemu/dma.h b/include/sysemu/dma.h
> index 3f2f4c8..efa8b99 100644
> --- a/include/sysemu/dma.h
> +++ b/include/sysemu/dma.h
> @@ -88,7 +88,8 @@ static inline int dma_memory_rw_relaxed(AddressSpace *as, dma_addr_t addr,
>                                          void *buf, dma_addr_t len,
>                                          DMADirection dir)
>  {
> -    return address_space_rw(as, addr, buf, len, dir == DMA_DIRECTION_FROM_DEVICE);
> +    return (bool)address_space_rw(as, addr, MEMTXATTRS_UNSPECIFIED,
> +                                  buf, len, dir == DMA_DIRECTION_FROM_DEVICE);
>  }

The return does the right thing but I was wondering if it should be more
explicit:

  return MEMTX_OK==address_space_rw(as, addr, MEMTXATTRS_UNSPECIFIED,...

?

>  
>  static inline int dma_memory_read_relaxed(AddressSpace *as, dma_addr_t addr,
> diff --git a/ioport.c b/ioport.c
> index 783a3ae..b345bd9 100644
> --- a/ioport.c
> +++ b/ioport.c
> @@ -64,7 +64,8 @@ void cpu_outb(pio_addr_t addr, uint8_t val)
>  {
>      LOG_IOPORT("outb: %04"FMT_pioaddr" %02"PRIx8"\n", addr, val);
>      trace_cpu_out(addr, val);
> -    address_space_write(&address_space_io, addr, &val, 1);
> +    address_space_write(&address_space_io, addr, MEMTXATTRS_UNSPECIFIED,
> +                        &val, 1);
>  }
>  
>  void cpu_outw(pio_addr_t addr, uint16_t val)
> @@ -74,7 +75,8 @@ void cpu_outw(pio_addr_t addr, uint16_t val)
>      LOG_IOPORT("outw: %04"FMT_pioaddr" %04"PRIx16"\n", addr, val);
>      trace_cpu_out(addr, val);
>      stw_p(buf, val);
> -    address_space_write(&address_space_io, addr, buf, 2);
> +    address_space_write(&address_space_io, addr, MEMTXATTRS_UNSPECIFIED,
> +                        buf, 2);
>  }
>  
>  void cpu_outl(pio_addr_t addr, uint32_t val)
> @@ -84,14 +86,16 @@ void cpu_outl(pio_addr_t addr, uint32_t val)
>      LOG_IOPORT("outl: %04"FMT_pioaddr" %08"PRIx32"\n", addr, val);
>      trace_cpu_out(addr, val);
>      stl_p(buf, val);
> -    address_space_write(&address_space_io, addr, buf, 4);
> +    address_space_write(&address_space_io, addr, MEMTXATTRS_UNSPECIFIED,
> +                        buf, 4);
>  }
>  
>  uint8_t cpu_inb(pio_addr_t addr)
>  {
>      uint8_t val;
>  
> -    address_space_read(&address_space_io, addr, &val, 1);
> +    address_space_read(&address_space_io, addr, MEMTXATTRS_UNSPECIFIED,
> +                       &val, 1);
>      trace_cpu_in(addr, val);
>      LOG_IOPORT("inb : %04"FMT_pioaddr" %02"PRIx8"\n", addr, val);
>      return val;
> @@ -102,7 +106,7 @@ uint16_t cpu_inw(pio_addr_t addr)
>      uint8_t buf[2];
>      uint16_t val;
>  
> -    address_space_read(&address_space_io, addr, buf, 2);
> +    address_space_read(&address_space_io, addr, MEMTXATTRS_UNSPECIFIED, buf, 2);
>      val = lduw_p(buf);
>      trace_cpu_in(addr, val);
>      LOG_IOPORT("inw : %04"FMT_pioaddr" %04"PRIx16"\n", addr, val);
> @@ -114,7 +118,7 @@ uint32_t cpu_inl(pio_addr_t addr)
>      uint8_t buf[4];
>      uint32_t val;
>  
> -    address_space_read(&address_space_io, addr, buf, 4);
> +    address_space_read(&address_space_io, addr, MEMTXATTRS_UNSPECIFIED, buf, 4);
>      val = ldl_p(buf);
>      trace_cpu_in(addr, val);
>      LOG_IOPORT("inl : %04"FMT_pioaddr" %08"PRIx32"\n", addr, val);
> diff --git a/kvm-all.c b/kvm-all.c
> index dd44f8c..4ec153d 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1667,7 +1667,8 @@ static void kvm_handle_io(uint16_t port, void *data, int direction, int size,
>      uint8_t *ptr = data;
>  
>      for (i = 0; i < count; i++) {
> -        address_space_rw(&address_space_io, port, ptr, size,
> +        address_space_rw(&address_space_io, port, MEMTXATTRS_UNSPECIFIED,
> +                         ptr, size,
>                           direction == KVM_EXIT_IO_OUT);
>          ptr += size;
>      }
> diff --git a/scripts/coverity-model.c b/scripts/coverity-model.c
> index cdda259..224d2d1 100644
> --- a/scripts/coverity-model.c
> +++ b/scripts/coverity-model.c
> @@ -46,6 +46,8 @@ typedef struct va_list_str *va_list;
>  
>  typedef struct AddressSpace AddressSpace;
>  typedef uint64_t hwaddr;
> +typedef uint32_t MemTxResult;
> +typedef uint64_t MemTxAttrs;
>  
>  static void __write(uint8_t *buf, ssize_t len)
>  {
> @@ -65,10 +67,10 @@ static void __read(uint8_t *buf, ssize_t len)
>      int last = buf[len-1];
>  }
>  
> -bool address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
> -                      int len, bool is_write)
> +MemTxResult address_space_rw(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
> +                             uint8_t *buf, int len, bool is_write)
>  {
> -    bool result;
> +    MemTxResult result;
>  
>      // TODO: investigate impact of treating reads as producing
>      // tainted data, with __coverity_tainted_data_argument__(buf).

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée