Re: [Qemu-devel] [PATCH v5 11/15] memory: Single byte swap along the I/O path
On 7/26/19 2:39 AM, Paolo Bonzini wrote: > Then memory_region_endianness_inverted can be: > > if (mr->ops->endianness == DEVICE_NATIVE_ENDIAN) > return (op & MO_BSWAP) != MO_TE; > else if (mr->ops->endianness == DEVICE_BIG_ENDIAN) > return (op & MO_BSWAP) != MO_BE; > else if (mr->ops->endianness == DEVICE_LITTLE_ENDIAN) > return (op & MO_BSWAP) != MO_LE; Possibly outside the scope of this patch set, I'd like to replace enum device_endian with MemOp, with exactly the above enumerator replacement. In the meantime, perhaps a conversion function static MemOp devendian_memop(enum device_endian d) { switch (d) { case DEVICE_NATIVE_ENDIAN: return MO_TE; case DEVICE_BIG_ENDIAN: return MO_BE; case DEVICE_LITTLE_ENDIAN: return MO_LE; default: g_assert_not_reached(); } } With that, this would simplify to return (op & MO_BSWAP) != devendian_memop(mr->ops->endianness); > I think the changes should be split in two parts. Before this patch, > you modify all callers of memory_region_dispatch_* so that they already > pass the right endianness op; however, you leave the existing swap in > place. So for example in load_helper you'd have in a previous patch > > +/* FIXME: io_readx ignores MO_BSWAP. */ > +op = SIZE_MEMOP(size) | (big_endian ? MO_BE : MO_LE); > res = io_readx(env, &env_tlb(env)->d[mmu_idx].iotlb[index], > - mmu_idx, addr, retaddr, access_type, > SIZE_MEMOP(size)); > + mmu_idx, addr, retaddr, access_type, op); > return handle_bswap(res, size, big_endian); > > Then, in this patch, you remove the handle_bswap call as well as the > FIXME comment. Agreed. r~
Re: [Qemu-devel] [PATCH v5 11/15] memory: Single byte swap along the I/O path
On 7/26/19 2:26 AM, Paolo Bonzini wrote: > On 26/07/19 08:47, tony.ngu...@bt.com wrote: >> + op = SIZE_MEMOP(size); >> + if (need_bswap(big_endian)) { >> + op ^= MO_BSWAP; >> + } > > And this has the same issue as the first version. It should be > > op = SIZE_MEMOP(size) | (big_endian ? MO_BE : MO_LE); > > and everything should work. If it doesn't (and indeed it doesn't :)) it > means you have bugs somewhere else. As I mentioned against patch 9, which also touches this area, it should be using the MemOp that is already passed in to this function instead of building a new one from scratch. But, yes, any failure in that would mean bugs somewhere else. ;-) r~
Re: [Qemu-devel] [PATCH v5 11/15] memory: Single byte swap along the I/O path
On 26/07/19 08:47, tony.ngu...@bt.com wrote: > +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; > + } Here it should not matter: the caller of memory_region_dispatch_read should includes one of MO_TE/MO_LE/MO_BE in the op (or nothing for host endianness). Then memory_region_endianness_inverted can be: if (mr->ops->endianness == DEVICE_NATIVE_ENDIAN) return (op & MO_BSWAP) != MO_TE; else if (mr->ops->endianness == DEVICE_BIG_ENDIAN) return (op & MO_BSWAP) != MO_BE; else if (mr->ops->endianness == DEVICE_LITTLE_ENDIAN) return (op & MO_BSWAP) != MO_LE; and adjust_endianness does if (memory_region_endianness_inverted(mr, op)) { switch (op & MO_SIZE) { ... } } I think the changes should be split in two parts. Before this patch, you modify all callers of memory_region_dispatch_* so that they already pass the right endianness op; however, you leave the existing swap in place. So for example in load_helper you'd have in a previous patch +/* FIXME: io_readx ignores MO_BSWAP. */ +op = SIZE_MEMOP(size) | (big_endian ? MO_BE : MO_LE); res = io_readx(env, &env_tlb(env)->d[mmu_idx].iotlb[index], - mmu_idx, addr, retaddr, access_type, SIZE_MEMOP(size)); + mmu_idx, addr, retaddr, access_type, op); return handle_bswap(res, size, big_endian); Then, in this patch, you remove the handle_bswap call as well as the FIXME comment. Paolo
Re: [Qemu-devel] [PATCH v5 11/15] memory: Single byte swap along the I/O path
On 26/07/19 08:47, tony.ngu...@bt.com wrote: > + op = SIZE_MEMOP(size); > + if (need_bswap(big_endian)) { > + op ^= MO_BSWAP; > + } And this has the same issue as the first version. It should be op = SIZE_MEMOP(size) | (big_endian ? MO_BE : MO_LE); and everything should work. If it doesn't (and indeed it doesn't :)) it means you have bugs somewhere else. Paolo
[Qemu-devel] [PATCH v5 11/15] memory: Single byte swap along the I/O path
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. 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 --- accel/tcg/cputlb.c | 41 +++-- memory.c | 30 +- 2 files changed, 36 insertions(+), 35 deletions(-) diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c index 5d88cec..e61b1eb 100644 --- a/accel/tcg/cputlb.c +++ b/accel/tcg/cputlb.c @@ -1209,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); } /* @@ -1259,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)) { @@ -1304,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_MEMOP(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). */ @@ -1507,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)) { @@ -1552,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_MEMOP(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, &