Re: [Qemu-devel] [PATCH 7/9] Convert address_space_rw to use MMUAccessType

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5645 bytes --]

On Thu, Jul 07, 2016 at 04:33:41PM -0700, Andrey Smirnov wrote:
> Convert address_space_rw() to use MMUAccessType following the conversion
> of cpu_memory_rw_debug().

Same concerns as the previous patch.  These paths don't actually have
anything to do with the MMU, which makes the constants oddly named.

> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  exec.c                   | 13 ++++++++-----
>  include/exec/memory.h    |  7 +++++--
>  kvm-all.c                |  8 +++++---
>  scripts/coverity-model.c |  7 +++++--
>  4 files changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 36a66e6..1fc6726 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2718,12 +2718,15 @@ MemTxResult address_space_read_full(AddressSpace *as, hwaddr addr,
>  }
>  
>  MemTxResult address_space_rw(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
> -                             uint8_t *buf, int len, bool is_write)
> +                             uint8_t *buf, int len, MMUAccessType access_type)
>  {
> -    if (is_write) {
> +    switch (access_type) {
> +    case MMU_DATA_STORE:
>          return address_space_write(as, addr, attrs, buf, len);
> -    } else {
> +    case MMU_DATA_LOAD:
>          return address_space_read(as, addr, attrs, buf, len);
> +    default:
> +        abort();
>      }
>  }
>  
> @@ -2731,7 +2734,7 @@ void cpu_physical_memory_rw(hwaddr addr, uint8_t *buf,
>                              int len, int is_write)
>  {
>      address_space_rw(&address_space_memory, addr, MEMTXATTRS_UNSPECIFIED,
> -                     buf, len, is_write);
> +                     buf, len, is_write ? MMU_DATA_STORE : MMU_DATA_LOAD);
>  }
>  
>  enum write_rom_type {
> @@ -3643,7 +3646,7 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
>          } else {
>              address_space_rw(cpu->cpu_ases[asidx].as, phys_addr,
>                               MEMTXATTRS_UNSPECIFIED,
> -                             buf, l, 0);
> +                             buf, l, access_type);

Replacing a fixed 0 with access_type looks wrong here, but I don't
have the context readily to hand to be sure.

>          }
>          len -= l;
>          buf += l;
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 51e4c2f..368263c 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -22,6 +22,7 @@
>  #define DIRTY_MEMORY_NUM       3        /* num of dirty bits */
>  
>  #include "exec/cpu-common.h"
> +#include "qom/cpu.h"
>  #ifndef CONFIG_USER_ONLY
>  #include "exec/hwaddr.h"
>  #endif
> @@ -1251,11 +1252,13 @@ void address_space_destroy(AddressSpace *as);
>   * @addr: address within that address space
>   * @attrs: memory transaction attributes
>   * @buf: buffer with the data transferred
> - * @is_write: indicates the transfer direction
> + * @access_type: indicates the transfer direction (only valid values
> + * are MMU_DATA_LOAD for data reads and MMU_DATA_STORE for data
> + * writes)
>   */
>  MemTxResult address_space_rw(AddressSpace *as, hwaddr addr,
>                               MemTxAttrs attrs, uint8_t *buf,
> -                             int len, bool is_write);
> +                             int len, MMUAccessType access_type);
>  
>  /**
>   * address_space_write: write to address space.
> diff --git a/kvm-all.c b/kvm-all.c
> index a88f917..7582275 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1767,11 +1767,12 @@ static void kvm_handle_io(uint16_t port, MemTxAttrs attrs, void *data, int direc
>  {
>      int i;
>      uint8_t *ptr = data;
> +    MMUAccessType access_type =
> +        (direction == KVM_EXIT_IO_OUT) ? MMU_DATA_STORE : MMU_DATA_LOAD;
>  
>      for (i = 0; i < count; i++) {
>          address_space_rw(&address_space_io, port, attrs,
> -                         ptr, size,
> -                         direction == KVM_EXIT_IO_OUT);
> +                         ptr, size, access_type);
>          ptr += size;
>      }
>  }
> @@ -1947,7 +1948,8 @@ int kvm_cpu_exec(CPUState *cpu)
>                               run->mmio.phys_addr, attrs,
>                               run->mmio.data,
>                               run->mmio.len,
> -                             run->mmio.is_write);
> +                             run->mmio.is_write ?
> +                             MMU_DATA_STORE : MMU_DATA_LOAD);
>              ret = 0;
>              break;
>          case KVM_EXIT_IRQ_WINDOW_OPEN:
> diff --git a/scripts/coverity-model.c b/scripts/coverity-model.c
> index ee5bf9d..be3418d 100644
> --- a/scripts/coverity-model.c
> +++ b/scripts/coverity-model.c
> @@ -68,13 +68,16 @@ static void __bufread(uint8_t *buf, ssize_t len)
>  }
>  
>  MemTxResult address_space_rw(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
> -                             uint8_t *buf, int len, bool is_write)
> +                             uint8_t *buf, int len, MMUAccessType access_type)
>  {
>      MemTxResult result;
>  
>      // TODO: investigate impact of treating reads as producing
>      // tainted data, with __coverity_tainted_data_argument__(buf).
> -    if (is_write) __bufread(buf, len); else __bufwrite(buf, len);
> +    if (access_type == MMU_DATA_STORE)
> +        __bufread(buf, len);
> +    else
> +        __bufwrite(buf, len);
>  
>      return result;
>  }

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]