Re: [qemu-s390x] [Qemu-devel] [PATCH v4 11/15] memory: Single byte swap along the I/O path

[Philippe Mathieu-Daudé]

On 7/25/19 11:56 AM, address@hidden wrote:
> Now that MemOp has been pushed down into the memory API, we can
> collapse the two byte swaps adjust_endianness and handle_bswap into
> the former.

Nice cleanup :)

> 
> Collapsing byte swaps along the I/O path enables additional endian
> inversion logic, e.g. SPARC64 Invert Endian TTE bit, with redundant
> byte swaps cancelling out.
> 
> Signed-off-by: Tony Nguyen <address@hidden>
> ---
>  accel/tcg/cputlb.c | 58 
> +++++++++++++++++++++++++-----------------------------
>  memory.c           | 30 ++++++++++++++++------------
>  2 files changed, 44 insertions(+), 44 deletions(-)
> 
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index a4a0bf7..e61b1eb 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -881,7 +881,7 @@ static void tlb_fill(CPUState *cpu, target_ulong addr, 
> int size,
> 
>  static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>                           int mmu_idx, target_ulong addr, uintptr_t retaddr,
> -                         MMUAccessType access_type, int size)
> +                         MMUAccessType access_type, MemOp op)
>  {
>      CPUState *cpu = env_cpu(env);
>      hwaddr mr_offset;
> @@ -906,14 +906,13 @@ static uint64_t io_readx(CPUArchState *env, 
> CPUIOTLBEntry *iotlbentry,
>          qemu_mutex_lock_iothread();
>          locked = true;
>      }
> -    r = memory_region_dispatch_read(mr, mr_offset, &val, SIZE_MEMOP(size),
> -                                    iotlbentry->attrs);
> +    r = memory_region_dispatch_read(mr, mr_offset, &val, op, 
> iotlbentry->attrs);
>      if (r != MEMTX_OK) {
>          hwaddr physaddr = mr_offset +
>              section->offset_within_address_space -
>              section->offset_within_region;
> 
> -        cpu_transaction_failed(cpu, physaddr, addr, size, access_type,
> +        cpu_transaction_failed(cpu, physaddr, addr, MEMOP_SIZE(op), 
> access_type,
>                                 mmu_idx, iotlbentry->attrs, r, retaddr);

Please move this change in "cputlb: Access MemoryRegion with MemOp"
(patch #9 of this series).

>      }
>      if (locked) {
> @@ -925,7 +924,7 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry 
> *iotlbentry,
> 
>  static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>                        int mmu_idx, uint64_t val, target_ulong addr,
> -                      uintptr_t retaddr, int size)
> +                      uintptr_t retaddr, MemOp op)
>  {
>      CPUState *cpu = env_cpu(env);
>      hwaddr mr_offset;
> @@ -947,15 +946,15 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry 
> *iotlbentry,
>          qemu_mutex_lock_iothread();
>          locked = true;
>      }
> -    r = memory_region_dispatch_write(mr, mr_offset, val, SIZE_MEMOP(size),
> -                                    iotlbentry->attrs);
> +    r = memory_region_dispatch_write(mr, mr_offset, val, op, 
> iotlbentry->attrs);
>      if (r != MEMTX_OK) {
>          hwaddr physaddr = mr_offset +
>              section->offset_within_address_space -
>              section->offset_within_region;
> 
> -        cpu_transaction_failed(cpu, physaddr, addr, size, MMU_DATA_STORE,
> -                               mmu_idx, iotlbentry->attrs, r, retaddr);
> +        cpu_transaction_failed(cpu, physaddr, addr, MEMOP_SIZE(op),
> +                               MMU_DATA_STORE, mmu_idx, iotlbentry->attrs, r,
> +                               retaddr);

Ditto.

>      }
>      if (locked) {
>          qemu_mutex_unlock_iothread();
> @@ -1210,26 +1209,13 @@ static void *atomic_mmu_lookup(CPUArchState *env, 
> target_ulong addr,
>  #endif
> 
>  /*
> - * Byte Swap Helper
> + * Byte Swap Checker
>   *
> - * This should all dead code away depending on the build host and
> - * access type.
> + * Dead code should all go away depending on the build host and access type.
>   */
> -
> -static inline uint64_t handle_bswap(uint64_t val, int size, bool big_endian)
> +static inline bool need_bswap(bool big_endian)
>  {
> -    if ((big_endian && NEED_BE_BSWAP) || (!big_endian && NEED_LE_BSWAP)) {
> -        switch (size) {
> -        case 1: return val;
> -        case 2: return bswap16(val);
> -        case 4: return bswap32(val);
> -        case 8: return bswap64(val);
> -        default:
> -            g_assert_not_reached();
> -        }
> -    } else {
> -        return val;
> -    }
> +    return (big_endian && NEED_BE_BSWAP) || (!big_endian && NEED_LE_BSWAP);

:)

>  }
> 
>  /*
> @@ -1260,6 +1246,7 @@ load_helper(CPUArchState *env, target_ulong addr, 
> TCGMemOpIdx oi,
>      unsigned a_bits = get_alignment_bits(get_memop(oi));
>      void *haddr;
>      uint64_t res;
> +    MemOp op;
> 
>      /* Handle CPU specific unaligned behaviour */
>      if (addr & ((1 << a_bits) - 1)) {
> @@ -1305,9 +1292,13 @@ load_helper(CPUArchState *env, target_ulong addr, 
> TCGMemOpIdx oi,
>              }
>          }
> 
> -        res = io_readx(env, &env_tlb(env)->d[mmu_idx].iotlb[index],
> -                       mmu_idx, addr, retaddr, access_type, size);
> -        return handle_bswap(res, size, big_endian);
> +        op = SIZE_MEMOP(size);
> +        if (need_bswap(big_endian)) {
> +            op ^= MO_BSWAP;
> +        }
> +
> +        return io_readx(env, &env_tlb(env)->d[mmu_idx].iotlb[index],
> +                       mmu_idx, addr, retaddr, access_type, op);
>      }
> 
>      /* Handle slow unaligned access (it spans two pages or IO).  */
> @@ -1508,6 +1499,7 @@ store_helper(CPUArchState *env, target_ulong addr, 
> uint64_t val,
>      const size_t tlb_off = offsetof(CPUTLBEntry, addr_write);
>      unsigned a_bits = get_alignment_bits(get_memop(oi));
>      void *haddr;
> +    MemOp op;
> 
>      /* Handle CPU specific unaligned behaviour */
>      if (addr & ((1 << a_bits) - 1)) {
> @@ -1553,9 +1545,13 @@ store_helper(CPUArchState *env, target_ulong addr, 
> uint64_t val,
>              }
>          }
> 
> +        op = SIZE_MEMOP(size);
> +        if (need_bswap(big_endian)) {
> +            op ^= MO_BSWAP;
> +        }
> +
>          io_writex(env, &env_tlb(env)->d[mmu_idx].iotlb[index], mmu_idx,
> -                  handle_bswap(val, size, big_endian),
> -                  addr, retaddr, size);
> +                  val, addr, retaddr, op);
>          return;
>      }
> 
> diff --git a/memory.c b/memory.c
> index 6982e19..0277d3d 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -352,7 +352,7 @@ static bool memory_region_big_endian(MemoryRegion *mr)
>  #endif
>  }
> 
> -static bool memory_region_wrong_endianness(MemoryRegion *mr)
> +static bool memory_region_endianness_inverted(MemoryRegion *mr)
>  {
>  #ifdef TARGET_WORDS_BIGENDIAN
>      return mr->ops->endianness == DEVICE_LITTLE_ENDIAN;
> @@ -361,23 +361,27 @@ static bool memory_region_wrong_endianness(MemoryRegion 
> *mr)
>  #endif
>  }
> 
> -static void adjust_endianness(MemoryRegion *mr, uint64_t *data, unsigned 
> size)
> +static void adjust_endianness(MemoryRegion *mr, uint64_t *data, MemOp op)
>  {
> -    if (memory_region_wrong_endianness(mr)) {
> -        switch (size) {
> -        case 1:
> +    if (memory_region_endianness_inverted(mr)) {
> +        op ^= MO_BSWAP;
> +    }
> +
> +    if (op & MO_BSWAP) {
> +        switch (op & MO_SIZE) {
> +        case MO_8:
>              break;
> -        case 2:
> +        case MO_16:
>              *data = bswap16(*data);
>              break;
> -        case 4:
> +        case MO_32:
>              *data = bswap32(*data);
>              break;
> -        case 8:
> +        case MO_64:
>              *data = bswap64(*data);
>              break;
>          default:
> -            abort();
> +            g_assert_not_reached();
>          }
>      }
>  }
> @@ -1451,7 +1455,7 @@ MemTxResult memory_region_dispatch_read(MemoryRegion 
> *mr,
>      }
> 
>      r = memory_region_dispatch_read1(mr, addr, pval, size, attrs);
> -    adjust_endianness(mr, pval, size);
> +    adjust_endianness(mr, pval, op);
>      return r;
>  }
> 
> @@ -1494,7 +1498,7 @@ MemTxResult memory_region_dispatch_write(MemoryRegion 
> *mr,
>          return MEMTX_DECODE_ERROR;
>      }
> 
> -    adjust_endianness(mr, &data, size);
> +    adjust_endianness(mr, &data, op);
> 
>      if ((!kvm_eventfds_enabled()) &&
>          memory_region_dispatch_write_eventfds(mr, addr, data, size, attrs)) {
> @@ -2340,7 +2344,7 @@ void memory_region_add_eventfd(MemoryRegion *mr,
>      }
> 
>      if (size) {
> -        adjust_endianness(mr, &mrfd.data, size);
> +        adjust_endianness(mr, &mrfd.data, SIZE_MEMOP(size));
>      }
>      memory_region_transaction_begin();
>      for (i = 0; i < mr->ioeventfd_nb; ++i) {
> @@ -2375,7 +2379,7 @@ void memory_region_del_eventfd(MemoryRegion *mr,
>      unsigned i;
> 
>      if (size) {
> -        adjust_endianness(mr, &mrfd.data, size);
> +        adjust_endianness(mr, &mrfd.data, SIZE_MEMOP(size));
>      }
>      memory_region_transaction_begin();
>      for (i = 0; i < mr->ioeventfd_nb; ++i) {
> --
> 1.8.3.1
> 

LGTM!