[07/14] exec.c: Add new address_space_ld*/st* functions

Add new address_space_ld*/st* functions which allow transaction
attributes and error reporting for basic load and stores. These
are named to be in line with the address_space_read/write/rw
buffer operations.

The existing ld/st*_phys functions are now wrappers around
the new functions.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 exec.c                | 291 ++++++++++++++++++++++++++++++++++++++++++--------
 include/exec/memory.h |  63 +++++++++++
 2 files changed, 310 insertions(+), 44 deletions(-)

On 07/04/2015 22:09, Peter Maydell wrote:
> +#ifdef NEED_CPU_H
> +uint32_t address_space_lduw(AddressSpace *as, hwaddr addr,
> +                            MemTxAttrs attrs, MemTxResult *result);
> +uint32_t address_space_ldl(AddressSpace *as, hwaddr addr,
> +                            MemTxAttrs attrs, MemTxResult *result);
> +uint64_t address_space_ldq(AddressSpace *as, hwaddr addr,
> +                            MemTxAttrs attrs, MemTxResult *result);
> +void address_space_stl_notdirty(AddressSpace *as, hwaddr addr, uint32_t val,
> +                            MemTxAttrs attrs, MemTxResult *result);
> +void address_space_stw(AddressSpace *as, hwaddr addr, uint32_t val,
> +                            MemTxAttrs attrs, MemTxResult *result);
> +void address_space_stl(AddressSpace *as, hwaddr addr, uint32_t val,
> +                            MemTxAttrs attrs, MemTxResult *result);
> +void address_space_stq(AddressSpace *as, hwaddr addr, uint64_t val,
> +                            MemTxAttrs attrs, MemTxResult *result);
> +#endif

I think we do not want to expose these at all (or at least, all users
should really be CPUs and hence use *_phys functions).

S390 is always big-endian, and watch_mem_read/write can use the same
buffer trick as subpages (and in fact should probably use memattrs as well).

So, please at least add a comment that these functions are deprecated,
and check if watch_mem_read/write should be handled like subpages.

Thanks,

Paolo

On Tue, Apr 07, 2015 at 09:09:53PM +0100, Peter Maydell wrote:
> Add new address_space_ld*/st* functions which allow transaction
> attributes and error reporting for basic load and stores. These
> are named to be in line with the address_space_read/write/rw
> buffer operations.
> 
> The existing ld/st*_phys functions are now wrappers around
> the new functions.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  exec.c                | 291 ++++++++++++++++++++++++++++++++++++++++++--------
>  include/exec/memory.h |  63 +++++++++++
>  2 files changed, 310 insertions(+), 44 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 29a12fa..013032a 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2671,19 +2671,22 @@ void cpu_physical_memory_unmap(void *buffer, hwaddr len,
>  }
>  
>  /* warning: addr must be aligned */
> -static inline uint32_t ldl_phys_internal(AddressSpace *as, hwaddr addr,
> -                                         enum device_endian endian)
> +static inline uint32_t address_space_ldl_internal(AddressSpace *as, hwaddr addr,
> +                                                  MemTxAttrs attrs,
> +                                                  MemTxResult *result,
> +                                                  enum device_endian endian)
>  {
>      uint8_t *ptr;
>      uint64_t val;
>      MemoryRegion *mr;
>      hwaddr l = 4;
>      hwaddr addr1;
> +    MemTxResult r;
>  
>      mr = address_space_translate(as, addr, &addr1, &l, false);
>      if (l < 4 || !memory_access_is_direct(mr, false)) {
>          /* I/O case */
> -        io_mem_read(mr, addr1, &val, 4, MEMTXATTRS_UNSPECIFIED);
> +        r = io_mem_read(mr, addr1, &val, 4, attrs);
>  #if defined(TARGET_WORDS_BIGENDIAN)
>          if (endian == DEVICE_LITTLE_ENDIAN) {
>              val = bswap32(val);
> @@ -2709,40 +2712,68 @@ static inline uint32_t ldl_phys_internal(AddressSpace *as, hwaddr addr,
>              val = ldl_p(ptr);
>              break;
>          }
> +        r = MEMTX_OK;
> +    }
> +    if (result) {
> +        *result = r;
>      }
>      return val;
>  }
>  
> +uint32_t address_space_ldl(AddressSpace *as, hwaddr addr,
> +                           MemTxAttrs attrs, MemTxResult *result)
> +{
> +    return address_space_ldl_internal(as, addr, attrs, result,
> +                                      DEVICE_NATIVE_ENDIAN);
> +}
> +
> +uint32_t address_space_ldl_le(AddressSpace *as, hwaddr addr,
> +                              MemTxAttrs attrs, MemTxResult *result)
> +{
> +    return address_space_ldl_internal(as, addr, attrs, result,
> +                                      DEVICE_LITTLE_ENDIAN);
> +}
> +
> +uint32_t address_space_ldl_be(AddressSpace *as, hwaddr addr,
> +                              MemTxAttrs attrs, MemTxResult *result)
> +{
> +    return address_space_ldl_internal(as, addr, attrs, result,
> +                                      DEVICE_BIG_ENDIAN);
> +}
> +
>  uint32_t ldl_phys(AddressSpace *as, hwaddr addr)
>  {
> -    return ldl_phys_internal(as, addr, DEVICE_NATIVE_ENDIAN);
> +    return address_space_ldl(as, addr, MEMTXATTRS_UNSPECIFIED, NULL);
>  }
>  
>  uint32_t ldl_le_phys(AddressSpace *as, hwaddr addr)
>  {
> -    return ldl_phys_internal(as, addr, DEVICE_LITTLE_ENDIAN);
> +    return address_space_ldl_le(as, addr, MEMTXATTRS_UNSPECIFIED, NULL);
>  }
>  
>  uint32_t ldl_be_phys(AddressSpace *as, hwaddr addr)
>  {
> -    return ldl_phys_internal(as, addr, DEVICE_BIG_ENDIAN);
> +    return address_space_ldl_be(as, addr, MEMTXATTRS_UNSPECIFIED, NULL);
>  }
>  
>  /* warning: addr must be aligned */
> -static inline uint64_t ldq_phys_internal(AddressSpace *as, hwaddr addr,
> -                                         enum device_endian endian)
> +static inline uint64_t address_space_ldq_internal(AddressSpace *as, hwaddr addr,
> +                                                  MemTxAttrs attrs,
> +                                                  MemTxResult *result,
> +                                                  enum device_endian endian)
>  {
>      uint8_t *ptr;
>      uint64_t val;
>      MemoryRegion *mr;
>      hwaddr l = 8;
>      hwaddr addr1;
> +    MemTxResult r;
>  
>      mr = address_space_translate(as, addr, &addr1, &l,
>                                   false);
>      if (l < 8 || !memory_access_is_direct(mr, false)) {
>          /* I/O case */
> -        io_mem_read(mr, addr1, &val, 8, MEMTXATTRS_UNSPECIFIED);
> +        r = io_mem_read(mr, addr1, &val, 8, attrs);
>  #if defined(TARGET_WORDS_BIGENDIAN)
>          if (endian == DEVICE_LITTLE_ENDIAN) {
>              val = bswap64(val);
> @@ -2768,48 +2799,88 @@ static inline uint64_t ldq_phys_internal(AddressSpace *as, hwaddr addr,
>              val = ldq_p(ptr);
>              break;
>          }
> +        r = MEMTX_OK;
> +    }
> +    if (result) {
> +        *result = r;
>      }
>      return val;
>  }
>  
> +uint64_t address_space_ldq(AddressSpace *as, hwaddr addr,
> +                           MemTxAttrs attrs, MemTxResult *result)
> +{
> +    return address_space_ldq_internal(as, addr, attrs, result,
> +                                      DEVICE_NATIVE_ENDIAN);
> +}
> +
> +uint64_t address_space_ldq_le(AddressSpace *as, hwaddr addr,
> +                           MemTxAttrs attrs, MemTxResult *result)
> +{
> +    return address_space_ldq_internal(as, addr, attrs, result,
> +                                      DEVICE_LITTLE_ENDIAN);
> +}
> +
> +uint64_t address_space_ldq_be(AddressSpace *as, hwaddr addr,
> +                           MemTxAttrs attrs, MemTxResult *result)
> +{
> +    return address_space_ldq_internal(as, addr, attrs, result,
> +                                      DEVICE_BIG_ENDIAN);
> +}
> +
>  uint64_t ldq_phys(AddressSpace *as, hwaddr addr)
>  {
> -    return ldq_phys_internal(as, addr, DEVICE_NATIVE_ENDIAN);
> +    return address_space_ldq(as, addr, MEMTXATTRS_UNSPECIFIED, NULL);
>  }
>  
>  uint64_t ldq_le_phys(AddressSpace *as, hwaddr addr)
>  {
> -    return ldq_phys_internal(as, addr, DEVICE_LITTLE_ENDIAN);
> +    return address_space_ldq_le(as, addr, MEMTXATTRS_UNSPECIFIED, NULL);
>  }
>  
>  uint64_t ldq_be_phys(AddressSpace *as, hwaddr addr)
>  {
> -    return ldq_phys_internal(as, addr, DEVICE_BIG_ENDIAN);
> +    return address_space_ldq_be(as, addr, MEMTXATTRS_UNSPECIFIED, NULL);
>  }
>  
>  /* XXX: optimize */
> -uint32_t ldub_phys(AddressSpace *as, hwaddr addr)
> +uint32_t address_space_ldub(AddressSpace *as, hwaddr addr,
> +                            MemTxAttrs attrs, MemTxResult *result)
>  {
>      uint8_t val;
> -    address_space_rw(as, addr, MEMTXATTRS_UNSPECIFIED, &val, 1, 0);
> +    MemTxResult r;
> +
> +    r = address_space_rw(as, addr, attrs, &val, 1, 0);
> +    if (result) {
> +        *result = r;
> +    }
>      return val;
>  }
>  
> +uint32_t ldub_phys(AddressSpace *as, hwaddr addr)
> +{
> +    return address_space_ldub(as, addr, MEMTXATTRS_UNSPECIFIED, NULL);
> +}
> +
>  /* warning: addr must be aligned */
> -static inline uint32_t lduw_phys_internal(AddressSpace *as, hwaddr addr,
> -                                          enum device_endian endian)
> +static inline uint32_t address_space_lduw_internal(AddressSpace *as,
> +                                                   hwaddr addr,
> +                                                   MemTxAttrs attrs,
> +                                                   MemTxResult *result,
> +                                                   enum device_endian endian)
>  {
>      uint8_t *ptr;
>      uint64_t val;
>      MemoryRegion *mr;
>      hwaddr l = 2;
>      hwaddr addr1;
> +    MemTxResult r;
>  
>      mr = address_space_translate(as, addr, &addr1, &l,
>                                   false);
>      if (l < 2 || !memory_access_is_direct(mr, false)) {
>          /* I/O case */
> -        io_mem_read(mr, addr1, &val, 2, MEMTXATTRS_UNSPECIFIED);
> +        r = io_mem_read(mr, addr1, &val, 2, attrs);
>  #if defined(TARGET_WORDS_BIGENDIAN)
>          if (endian == DEVICE_LITTLE_ENDIAN) {
>              val = bswap16(val);
> @@ -2835,39 +2906,66 @@ static inline uint32_t lduw_phys_internal(AddressSpace *as, hwaddr addr,
>              val = lduw_p(ptr);
>              break;
>          }
> +        r = MEMTX_OK;
> +    }
> +    if (result) {
> +        *result = r;
>      }
>      return val;
>  }
>  
> +uint32_t address_space_lduw(AddressSpace *as, hwaddr addr,
> +                           MemTxAttrs attrs, MemTxResult *result)
> +{
> +    return address_space_lduw_internal(as, addr, attrs, result,
> +                                       DEVICE_NATIVE_ENDIAN);
> +}
> +
> +uint32_t address_space_lduw_le(AddressSpace *as, hwaddr addr,
> +                           MemTxAttrs attrs, MemTxResult *result)
> +{
> +    return address_space_lduw_internal(as, addr, attrs, result,
> +                                       DEVICE_LITTLE_ENDIAN);
> +}
> +
> +uint32_t address_space_lduw_be(AddressSpace *as, hwaddr addr,
> +                           MemTxAttrs attrs, MemTxResult *result)
> +{
> +    return address_space_lduw_internal(as, addr, attrs, result,
> +                                       DEVICE_BIG_ENDIAN);
> +}
> +
>  uint32_t lduw_phys(AddressSpace *as, hwaddr addr)
>  {
> -    return lduw_phys_internal(as, addr, DEVICE_NATIVE_ENDIAN);
> +    return address_space_lduw(as, addr, MEMTXATTRS_UNSPECIFIED, NULL);
>  }
>  
>  uint32_t lduw_le_phys(AddressSpace *as, hwaddr addr)
>  {
> -    return lduw_phys_internal(as, addr, DEVICE_LITTLE_ENDIAN);
> +    return address_space_lduw_le(as, addr, MEMTXATTRS_UNSPECIFIED, NULL);
>  }
>  
>  uint32_t lduw_be_phys(AddressSpace *as, hwaddr addr)
>  {
> -    return lduw_phys_internal(as, addr, DEVICE_BIG_ENDIAN);
> +    return address_space_lduw_be(as, addr, MEMTXATTRS_UNSPECIFIED, NULL);
>  }
>  
>  /* warning: addr must be aligned. The ram page is not masked as dirty
>     and the code inside is not invalidated. It is useful if the dirty
>     bits are used to track modified PTEs */
> -void stl_phys_notdirty(AddressSpace *as, hwaddr addr, uint32_t val)
> +void address_space_stl_notdirty(AddressSpace *as, hwaddr addr, uint32_t val,
> +                                MemTxAttrs attrs, MemTxResult *result)
>  {
>      uint8_t *ptr;
>      MemoryRegion *mr;
>      hwaddr l = 4;
>      hwaddr addr1;
> +    MemTxResult r;
>  
>      mr = address_space_translate(as, addr, &addr1, &l,
>                                   true);
>      if (l < 4 || !memory_access_is_direct(mr, true)) {
> -        io_mem_write(mr, addr1, val, 4, MEMTXATTRS_UNSPECIFIED);
> +        r = io_mem_write(mr, addr1, val, 4, attrs);
>      } else {
>          addr1 += memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK;
>          ptr = qemu_get_ram_ptr(addr1);
> @@ -2881,18 +2979,30 @@ void stl_phys_notdirty(AddressSpace *as, hwaddr addr, uint32_t val)
>                  cpu_physical_memory_set_dirty_range_nocode(addr1, 4);
>              }
>          }
> +        r = MEMTX_OK;
> +    }
> +    if (result) {
> +        *result = r;
>      }
>  }
>  
> +void stl_phys_notdirty(AddressSpace *as, hwaddr addr, uint32_t val)
> +{
> +    address_space_stl_notdirty(as, addr, val, MEMTXATTRS_UNSPECIFIED, NULL);
> +}
> +
>  /* warning: addr must be aligned */
> -static inline void stl_phys_internal(AddressSpace *as,
> -                                     hwaddr addr, uint32_t val,
> -                                     enum device_endian endian)
> +static inline void address_space_stl_internal(AddressSpace *as,
> +                                              hwaddr addr, uint32_t val,
> +                                              MemTxAttrs attrs,
> +                                              MemTxResult *result,
> +                                              enum device_endian endian)
>  {
>      uint8_t *ptr;
>      MemoryRegion *mr;
>      hwaddr l = 4;
>      hwaddr addr1;
> +    MemTxResult r;
>  
>      mr = address_space_translate(as, addr, &addr1, &l,
>                                   true);
> @@ -2906,7 +3016,7 @@ static inline void stl_phys_internal(AddressSpace *as,
>              val = bswap32(val);
>          }
>  #endif
> -        io_mem_write(mr, addr1, val, 4, MEMTXATTRS_UNSPECIFIED);
> +        r = io_mem_write(mr, addr1, val, 4, attrs);
>      } else {
>          /* RAM case */
>          addr1 += memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK;
> @@ -2923,40 +3033,79 @@ static inline void stl_phys_internal(AddressSpace *as,
>              break;
>          }
>          invalidate_and_set_dirty(addr1, 4);
> +        r = MEMTX_OK;
> +    }
> +    if (result) {
> +        *result = r;
>      }
>  }
>  
> +void address_space_stl(AddressSpace *as, hwaddr addr, uint32_t val,
> +                       MemTxAttrs attrs, MemTxResult *result)
> +{
> +    address_space_stl_internal(as, addr, val, attrs, result,
> +                               DEVICE_NATIVE_ENDIAN);
> +}
> +
> +void address_space_stl_le(AddressSpace *as, hwaddr addr, uint32_t val,
> +                       MemTxAttrs attrs, MemTxResult *result)
> +{
> +    address_space_stl_internal(as, addr, val, attrs, result,
> +                               DEVICE_LITTLE_ENDIAN);
> +}
> +
> +void address_space_stl_be(AddressSpace *as, hwaddr addr, uint32_t val,
> +                       MemTxAttrs attrs, MemTxResult *result)
> +{
> +    address_space_stl_internal(as, addr, val, attrs, result,
> +                               DEVICE_BIG_ENDIAN);
> +}
> +
>  void stl_phys(AddressSpace *as, hwaddr addr, uint32_t val)
>  {
> -    stl_phys_internal(as, addr, val, DEVICE_NATIVE_ENDIAN);
> +    address_space_stl(as, addr, val, MEMTXATTRS_UNSPECIFIED, NULL);
>  }
>  
>  void stl_le_phys(AddressSpace *as, hwaddr addr, uint32_t val)
>  {
> -    stl_phys_internal(as, addr, val, DEVICE_LITTLE_ENDIAN);
> +    address_space_stl_le(as, addr, val, MEMTXATTRS_UNSPECIFIED, NULL);
>  }
>  
>  void stl_be_phys(AddressSpace *as, hwaddr addr, uint32_t val)
>  {
> -    stl_phys_internal(as, addr, val, DEVICE_BIG_ENDIAN);
> +    address_space_stl_be(as, addr, val, MEMTXATTRS_UNSPECIFIED, NULL);
>  }
>  
>  /* XXX: optimize */
> -void stb_phys(AddressSpace *as, hwaddr addr, uint32_t val)
> +void address_space_stb(AddressSpace *as, hwaddr addr, uint32_t val,
> +                       MemTxAttrs attrs, MemTxResult *result)
>  {
>      uint8_t v = val;
> -    address_space_rw(as, addr, MEMTXATTRS_UNSPECIFIED, &v, 1, 1);
> +    MemTxResult r;
> +
> +    r = address_space_rw(as, addr, attrs, &v, 1, 1);
> +    if (result) {
> +        *result = r;
> +    }
> +}
> +
> +void stb_phys(AddressSpace *as, hwaddr addr, uint32_t val)
> +{
> +    address_space_stb(as, addr, val, MEMTXATTRS_UNSPECIFIED, NULL);
>  }
>  
>  /* warning: addr must be aligned */
> -static inline void stw_phys_internal(AddressSpace *as,
> -                                     hwaddr addr, uint32_t val,
> -                                     enum device_endian endian)
> +static inline void address_space_stw_internal(AddressSpace *as,
> +                                              hwaddr addr, uint32_t val,
> +                                              MemTxAttrs attrs,
> +                                              MemTxResult *result,
> +                                              enum device_endian endian)
>  {
>      uint8_t *ptr;
>      MemoryRegion *mr;
>      hwaddr l = 2;
>      hwaddr addr1;
> +    MemTxResult r;
>  
>      mr = address_space_translate(as, addr, &addr1, &l, true);
>      if (l < 2 || !memory_access_is_direct(mr, true)) {
> @@ -2969,7 +3118,7 @@ static inline void stw_phys_internal(AddressSpace *as,
>              val = bswap16(val);
>          }
>  #endif
> -        io_mem_write(mr, addr1, val, 2, MEMTXATTRS_UNSPECIFIED);
> +        r = io_mem_write(mr, addr1, val, 2, attrs);
>      } else {
>          /* RAM case */
>          addr1 += memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK;
> @@ -2986,41 +3135,95 @@ static inline void stw_phys_internal(AddressSpace *as,
>              break;
>          }
>          invalidate_and_set_dirty(addr1, 2);
> +        r = MEMTX_OK;
> +    }
> +    if (result) {
> +        *result = r;
>      }
>  }
>  
> +void address_space_stw(AddressSpace *as, hwaddr addr, uint32_t val,
> +                       MemTxAttrs attrs, MemTxResult *result)
> +{
> +    address_space_stw_internal(as, addr, val, attrs, result,
> +                               DEVICE_NATIVE_ENDIAN);
> +}
> +
> +void address_space_stw_le(AddressSpace *as, hwaddr addr, uint32_t val,
> +                       MemTxAttrs attrs, MemTxResult *result)
> +{
> +    address_space_stw_internal(as, addr, val, attrs, result,
> +                               DEVICE_LITTLE_ENDIAN);
> +}
> +
> +void address_space_stw_be(AddressSpace *as, hwaddr addr, uint32_t val,
> +                       MemTxAttrs attrs, MemTxResult *result)
> +{
> +    address_space_stw_internal(as, addr, val, attrs, result,
> +                               DEVICE_BIG_ENDIAN);
> +}
> +
>  void stw_phys(AddressSpace *as, hwaddr addr, uint32_t val)
>  {
> -    stw_phys_internal(as, addr, val, DEVICE_NATIVE_ENDIAN);
> +    address_space_stw(as, addr, val, MEMTXATTRS_UNSPECIFIED, NULL);
>  }
>  
>  void stw_le_phys(AddressSpace *as, hwaddr addr, uint32_t val)
>  {
> -    stw_phys_internal(as, addr, val, DEVICE_LITTLE_ENDIAN);
> +    address_space_stw_le(as, addr, val, MEMTXATTRS_UNSPECIFIED, NULL);
>  }
>  
>  void stw_be_phys(AddressSpace *as, hwaddr addr, uint32_t val)
>  {
> -    stw_phys_internal(as, addr, val, DEVICE_BIG_ENDIAN);
> +    address_space_stw_be(as, addr, val, MEMTXATTRS_UNSPECIFIED, NULL);
>  }
>  
>  /* XXX: optimize */
> -void stq_phys(AddressSpace *as, hwaddr addr, uint64_t val)
> +void address_space_stq(AddressSpace *as, hwaddr addr, uint64_t val,
> +                       MemTxAttrs attrs, MemTxResult *result)
>  {
> +    MemTxResult r;
>      val = tswap64(val);
> -    address_space_rw(as, addr, MEMTXATTRS_UNSPECIFIED, (void *) &val, 8, 1);
> +    r = address_space_rw(as, addr, attrs, (void *) &val, 8, 1);
> +    if (result) {
> +        *result = r;
> +    }
>  }
>  
> -void stq_le_phys(AddressSpace *as, hwaddr addr, uint64_t val)
> +void address_space_stq_le(AddressSpace *as, hwaddr addr, uint64_t val,
> +                       MemTxAttrs attrs, MemTxResult *result)
>  {
> +    MemTxResult r;
>      val = cpu_to_le64(val);
> -    address_space_rw(as, addr, MEMTXATTRS_UNSPECIFIED, (void *) &val, 8, 1);
> +    r = address_space_rw(as, addr, attrs, (void *) &val, 8, 1);
> +    if (result) {
> +        *result = r;
> +    }
> +}
> +void address_space_stq_be(AddressSpace *as, hwaddr addr, uint64_t val,
> +                       MemTxAttrs attrs, MemTxResult *result)
> +{
> +    MemTxResult r;
> +    val = cpu_to_be64(val);
> +    r = address_space_rw(as, addr, attrs, (void *) &val, 8, 1);
> +    if (result) {
> +        *result = r;
> +    }
> +}
> +
> +void stq_phys(AddressSpace *as, hwaddr addr, uint64_t val)
> +{
> +    address_space_stq(as, addr, val, MEMTXATTRS_UNSPECIFIED, NULL);
> +}
> +
> +void stq_le_phys(AddressSpace *as, hwaddr addr, uint64_t val)
> +{
> +    address_space_stq_le(as, addr, val, MEMTXATTRS_UNSPECIFIED, NULL);
>  }
>  
>  void stq_be_phys(AddressSpace *as, hwaddr addr, uint64_t val)
>  {
> -    val = cpu_to_be64(val);
> -    address_space_rw(as, addr, MEMTXATTRS_UNSPECIFIED, (void *) &val, 8, 1);
> +    address_space_stq_be(as, addr, val, MEMTXATTRS_UNSPECIFIED, NULL);
>  }
>  
>  /* virtual memory access for debug (includes writing to ROM) */
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 4d6afc8..0a048ab 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1134,6 +1134,69 @@ MemTxResult address_space_write(AddressSpace *as, hwaddr addr,
>  MemTxResult address_space_read(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
>                                 uint8_t *buf, int len);
>  
> +/**
> + * address_space_ld*: load from an address space
> + * address_space_st*: store to an address space
> + *
> + * These functions perform a load or store of the byte, word,
> + * longword or quad to the specified address within the AddressSpace.
> + * The _le suffixed functions treat the data as little endian;
> + * _be indicates big endian; no suffix indicates "same endianness
> + * as guest CPU".
> + *
> + * @as #AddressSpace to be accessed
> + * @addr: address within that address space
> + * @val: data value, for stores
> + * @attrs: memory transaction attributes
> + * @result: location to write the success/failure of the transaction;
> + *   if NULL, this information is discarded
> + */
> +uint32_t address_space_ldub(AddressSpace *as, hwaddr addr,
> +                            MemTxAttrs attrs, MemTxResult *result);
> +uint32_t address_space_lduw_le(AddressSpace *as, hwaddr addr,
> +                            MemTxAttrs attrs, MemTxResult *result);
> +uint32_t address_space_lduw_be(AddressSpace *as, hwaddr addr,
> +                            MemTxAttrs attrs, MemTxResult *result);
> +uint32_t address_space_ldl_le(AddressSpace *as, hwaddr addr,
> +                            MemTxAttrs attrs, MemTxResult *result);
> +uint32_t address_space_ldl_be(AddressSpace *as, hwaddr addr,
> +                            MemTxAttrs attrs, MemTxResult *result);
> +uint64_t address_space_ldq_le(AddressSpace *as, hwaddr addr,
> +                            MemTxAttrs attrs, MemTxResult *result);
> +uint64_t address_space_ldq_be(AddressSpace *as, hwaddr addr,
> +                            MemTxAttrs attrs, MemTxResult *result);
> +void address_space_stb(AddressSpace *as, hwaddr addr, uint32_t val,
> +                            MemTxAttrs attrs, MemTxResult *result);
> +void address_space_stw_le(AddressSpace *as, hwaddr addr, uint32_t val,
> +                            MemTxAttrs attrs, MemTxResult *result);
> +void address_space_stw_be(AddressSpace *as, hwaddr addr, uint32_t val,
> +                            MemTxAttrs attrs, MemTxResult *result);
> +void address_space_stl_le(AddressSpace *as, hwaddr addr, uint32_t val,
> +                            MemTxAttrs attrs, MemTxResult *result);
> +void address_space_stl_be(AddressSpace *as, hwaddr addr, uint32_t val,
> +                            MemTxAttrs attrs, MemTxResult *result);
> +void address_space_stq_le(AddressSpace *as, hwaddr addr, uint64_t val,
> +                            MemTxAttrs attrs, MemTxResult *result);
> +void address_space_stq_be(AddressSpace *as, hwaddr addr, uint64_t val,
> +                            MemTxAttrs attrs, MemTxResult *result);
> +
> +#ifdef NEED_CPU_H
> +uint32_t address_space_lduw(AddressSpace *as, hwaddr addr,
> +                            MemTxAttrs attrs, MemTxResult *result);
> +uint32_t address_space_ldl(AddressSpace *as, hwaddr addr,
> +                            MemTxAttrs attrs, MemTxResult *result);
> +uint64_t address_space_ldq(AddressSpace *as, hwaddr addr,
> +                            MemTxAttrs attrs, MemTxResult *result);
> +void address_space_stl_notdirty(AddressSpace *as, hwaddr addr, uint32_t val,
> +                            MemTxAttrs attrs, MemTxResult *result);
> +void address_space_stw(AddressSpace *as, hwaddr addr, uint32_t val,
> +                            MemTxAttrs attrs, MemTxResult *result);
> +void address_space_stl(AddressSpace *as, hwaddr addr, uint32_t val,
> +                            MemTxAttrs attrs, MemTxResult *result);
> +void address_space_stq(AddressSpace *as, hwaddr addr, uint64_t val,
> +                            MemTxAttrs attrs, MemTxResult *result);
> +#endif
> +
>  /* address_space_translate: translate an address range into an address space
>   * into a MemoryRegion and an address range into that section
>   *
> -- 
> 1.9.1
>

On 8 April 2015 at 12:03, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 07/04/2015 22:09, Peter Maydell wrote:
>> +#ifdef NEED_CPU_H
>> +uint32_t address_space_lduw(AddressSpace *as, hwaddr addr,
>> +                            MemTxAttrs attrs, MemTxResult *result);
>> +uint32_t address_space_ldl(AddressSpace *as, hwaddr addr,
>> +                            MemTxAttrs attrs, MemTxResult *result);
>> +uint64_t address_space_ldq(AddressSpace *as, hwaddr addr,
>> +                            MemTxAttrs attrs, MemTxResult *result);
>> +void address_space_stl_notdirty(AddressSpace *as, hwaddr addr, uint32_t val,
>> +                            MemTxAttrs attrs, MemTxResult *result);
>> +void address_space_stw(AddressSpace *as, hwaddr addr, uint32_t val,
>> +                            MemTxAttrs attrs, MemTxResult *result);
>> +void address_space_stl(AddressSpace *as, hwaddr addr, uint32_t val,
>> +                            MemTxAttrs attrs, MemTxResult *result);
>> +void address_space_stq(AddressSpace *as, hwaddr addr, uint64_t val,
>> +                            MemTxAttrs attrs, MemTxResult *result);
>> +#endif
>
> I think we do not want to expose these at all (or at least, all users
> should really be CPUs and hence use *_phys functions).
>
> S390 is always big-endian, and watch_mem_read/write can use the same
> buffer trick as subpages (and in fact should probably use memattrs as well).
>
> So, please at least add a comment that these functions are deprecated,
> and check if watch_mem_read/write should be handled like subpages.

I looked at the subpages code, and it seems to me that it's the
other way around -- the subpages code should use these new functions.
At the moment the subpage handlers use address_space_read/write
to pull the data into a buffer, and then use the ldl_p/stl_p functions
to do "read data from target-CPU order buffer into host variable".
It would be better for them to just directly be able to say "do
a ld/st in target-CPU order into this host variable", which is
the purpose of these new functions. Indirecting via a buffer seems
like an ugly workaround for not having the direct operation.

-- PMM

On 09/04/2015 13:49, Peter Maydell wrote:
>> > I think we do not want to expose these at all (or at least, all users
>> > should really be CPUs and hence use *_phys functions).
>> >
>> > S390 is always big-endian, and watch_mem_read/write can use the same
>> > buffer trick as subpages (and in fact should probably use memattrs as well).
>> >
>> > So, please at least add a comment that these functions are deprecated,
>> > and check if watch_mem_read/write should be handled like subpages.
> I looked at the subpages code, and it seems to me that it's the
> other way around -- the subpages code should use these new functions.
> At the moment the subpage handlers use address_space_read/write
> to pull the data into a buffer, and then use the ldl_p/stl_p functions
> to do "read data from target-CPU order buffer into host variable".
> It would be better for them to just directly be able to say "do
> a ld/st in target-CPU order into this host variable", which is
> the purpose of these new functions. Indirecting via a buffer seems
> like an ugly workaround for not having the direct operation.

Using them in subpage code is fine, but then the subpage code is in
exec.c and can use the _internal version directly (and pass
DEVICE_NATIVE_ENDIAN).  Still, usage of these outside exec.c is probably
suspicious.  It's at least worth pulling these in cpu-all.h; the whole
contents of cpu-common.h look like a sundry of functions that either are
deprecated or should be declared elsewhere.

Paolo

On 9 April 2015 at 13:00, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 09/04/2015 13:49, Peter Maydell wrote:
>>> > I think we do not want to expose these at all (or at least, all users
>>> > should really be CPUs and hence use *_phys functions).
>>> >
>>> > S390 is always big-endian, and watch_mem_read/write can use the same
>>> > buffer trick as subpages (and in fact should probably use memattrs as well).
>>> >
>>> > So, please at least add a comment that these functions are deprecated,
>>> > and check if watch_mem_read/write should be handled like subpages.
>> I looked at the subpages code, and it seems to me that it's the
>> other way around -- the subpages code should use these new functions.
>> At the moment the subpage handlers use address_space_read/write
>> to pull the data into a buffer, and then use the ldl_p/stl_p functions
>> to do "read data from target-CPU order buffer into host variable".
>> It would be better for them to just directly be able to say "do
>> a ld/st in target-CPU order into this host variable", which is
>> the purpose of these new functions. Indirecting via a buffer seems
>> like an ugly workaround for not having the direct operation.
>
> Using them in subpage code is fine, but then the subpage code is in
> exec.c and can use the _internal version directly (and pass
> DEVICE_NATIVE_ENDIAN).  Still, usage of these outside exec.c is probably
> suspicious.

I use them (later in the series) in target-arm/ as well, when I
want a CPU-target-endian access which lets me specify memory
attributes.

> It's at least worth pulling these in cpu-all.h; the whole
> contents of cpu-common.h look like a sundry of functions that either are
> deprecated or should be declared elsewhere.

They need to be in memory.h because cpu-all.h doesn't have
all the typedefs. Also, memory.h seems to me clearly the
best place, since it's where we declare the MemoryRegion
and AddressSpace related functions.

It's certainly true that using target-cpu-endian functions
in a device is somewhat suspicious, but that's generally
true of all those functions, not just these new
address_space_* ones.

So I think I'm happy to add a comment that the functions
are deprecated for use outside target-*/, but I think they
are still necessary.

-- PMM

On 09/04/2015 14:38, Peter Maydell wrote:
> They need to be in memory.h because cpu-all.h doesn't have
> all the typedefs. Also, memory.h seems to me clearly the
> best place, since it's where we declare the MemoryRegion
> and AddressSpace related functions.
> 
> It's certainly true that using target-cpu-endian functions
> in a device is somewhat suspicious, but that's generally
> true of all those functions, not just these new
> address_space_* ones.
> 
> So I think I'm happy to add a comment that the functions
> are deprecated for use outside target-*/, but I think they
> are still necessary.

Fair enough, that will do.

Paolo