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, _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, _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