| Message ID | 1565166794966.57397@bt.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Invert Endian bit in SPARCv9 MMU TTE | expand |
On 07/08/19 10:33, tony.nguyen@bt.com wrote: > +#ifdef NEED_CPU_H > + return ctz32(size) | MO_TE; > +#else > return ctz32(size); > +#endif Please use two separate functions for this, for example size_to_memop and target_size_to_memop, or even just add MO_TE to the callers that need it (only cputlb.c?). Paolo
On 8/7/19 1:33 AM, tony.nguyen@bt.com wrote: > @@ -551,6 +551,7 @@ void virtio_address_space_write(VirtIOPCIProxy *proxy, hwaddr addr, > /* As length is under guest control, handle illegal values. */ > return; > } > + /* FIXME: memory_region_dispatch_write ignores MO_BSWAP. */ > memory_region_dispatch_write(mr, addr, val, size_memop(len), > MEMTXATTRS_UNSPECIFIED); > } Here is an example of where Paolo is quite right -- you cannot simply add MO_TE via size_memop(). In patch 22 we see > @@ -542,16 +542,15 @@ void virtio_address_space_write(VirtIOPCIProxy *proxy, hwaddr addr, > val = pci_get_byte(buf); > break; > case 2: > - val = cpu_to_le16(pci_get_word(buf)); > + val = pci_get_word(buf); > break; > case 4: > - val = cpu_to_le32(pci_get_long(buf)); > + val = pci_get_long(buf); > break; > default: > /* As length is under guest control, handle illegal values. */ > return; > } > - /* FIXME: memory_region_dispatch_write ignores MO_BSWAP. */ > memory_region_dispatch_write(mr, addr, val, size_memop(len), > MEMTXATTRS_UNSPECIFIED); This is a little-endian store -- MO_LE not MO_TE. r~
On 07/08/19 19:49, Richard Henderson wrote: > On 8/7/19 1:33 AM, tony.nguyen@bt.com wrote: >> @@ -551,6 +551,7 @@ void virtio_address_space_write(VirtIOPCIProxy *proxy, hwaddr addr, >> /* As length is under guest control, handle illegal values. */ >> return; >> } >> + /* FIXME: memory_region_dispatch_write ignores MO_BSWAP. */ >> memory_region_dispatch_write(mr, addr, val, size_memop(len), >> MEMTXATTRS_UNSPECIFIED); >> } > > Here is an example of where Paolo is quite right -- you cannot simply add MO_TE > via size_memop(). In patch 22 we see > >> @@ -542,16 +542,15 @@ void virtio_address_space_write(VirtIOPCIProxy *proxy, hwaddr addr, >> val = pci_get_byte(buf); >> break; >> case 2: >> - val = cpu_to_le16(pci_get_word(buf)); >> + val = pci_get_word(buf); >> break; >> case 4: >> - val = cpu_to_le32(pci_get_long(buf)); >> + val = pci_get_long(buf); >> break; >> default: >> /* As length is under guest control, handle illegal values. */ >> return; >> } >> - /* FIXME: memory_region_dispatch_write ignores MO_BSWAP. */ >> memory_region_dispatch_write(mr, addr, val, size_memop(len), >> MEMTXATTRS_UNSPECIFIED); > > This is a little-endian store -- MO_LE not MO_TE. Or leave the switch statement aside and request host endianness. Either is fine. Paolo
On 8/7/19 11:00 AM, Paolo Bonzini wrote: > On 07/08/19 19:49, Richard Henderson wrote: >> On 8/7/19 1:33 AM, tony.nguyen@bt.com wrote: >>> @@ -551,6 +551,7 @@ void virtio_address_space_write(VirtIOPCIProxy *proxy, hwaddr addr, >>> /* As length is under guest control, handle illegal values. */ >>> return; >>> } >>> + /* FIXME: memory_region_dispatch_write ignores MO_BSWAP. */ >>> memory_region_dispatch_write(mr, addr, val, size_memop(len), >>> MEMTXATTRS_UNSPECIFIED); >>> } >> >> Here is an example of where Paolo is quite right -- you cannot simply add MO_TE >> via size_memop(). In patch 22 we see >> >>> @@ -542,16 +542,15 @@ void virtio_address_space_write(VirtIOPCIProxy *proxy, hwaddr addr, >>> val = pci_get_byte(buf); >>> break; >>> case 2: >>> - val = cpu_to_le16(pci_get_word(buf)); >>> + val = pci_get_word(buf); >>> break; >>> case 4: >>> - val = cpu_to_le32(pci_get_long(buf)); >>> + val = pci_get_long(buf); >>> break; >>> default: >>> /* As length is under guest control, handle illegal values. */ >>> return; >>> } >>> - /* FIXME: memory_region_dispatch_write ignores MO_BSWAP. */ >>> memory_region_dispatch_write(mr, addr, val, size_memop(len), >>> MEMTXATTRS_UNSPECIFIED); >> >> This is a little-endian store -- MO_LE not MO_TE. > > Or leave the switch statement aside and request host endianness. Either > is fine. The goal is to minimize the number of places and the number of times that bswaps happen. That's why I think pushing the cpu_to_le16 into memory_region_dispatch_write is a good thing. However, leaving a host-endian might be the easiest short-term solution for the more complicated cases. It also seems like a way to split the patch further if we think that's desirable. r~
diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c index 0e92a37..d322b86 100644 --- a/hw/s390x/s390-pci-inst.c +++ b/hw/s390x/s390-pci-inst.c @@ -782,7 +782,8 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr, for (i = 0; i < len / 8; i++) { result = memory_region_dispatch_write(mr, offset + i * 8, ldq_p(buffer + i * 8), - MO_64, MEMTXATTRS_UNSPECIFIED); + MO_64 | MO_TE, + MEMTXATTRS_UNSPECIFIED); if (result != MEMTX_OK) { s390_program_interrupt(env, PGM_OPERAND, 6, ra); return 0; diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index b929e44..70eb161 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -551,6 +551,7 @@ void virtio_address_space_write(VirtIOPCIProxy *proxy, hwaddr addr, /* As length is under guest control, handle illegal values. */ return; } + /* FIXME: memory_region_dispatch_write ignores MO_BSWAP. */ memory_region_dispatch_write(mr, addr, val, size_memop(len), MEMTXATTRS_UNSPECIFIED); } @@ -575,6 +576,7 @@ virtio_address_space_read(VirtIOPCIProxy *proxy, hwaddr addr, /* Make sure caller aligned buf properly */ assert(!(((uintptr_t)buf) & (len - 1))); + /* FIXME: memory_region_dispatch_read ignores MO_BSWAP. */ memory_region_dispatch_read(mr, addr, &val, size_memop(len), MEMTXATTRS_UNSPECIFIED); switch (len) { diff --git a/include/exec/memop.h b/include/exec/memop.h index 4a4212d..47a5500 100644 --- a/include/exec/memop.h +++ b/include/exec/memop.h @@ -122,7 +122,11 @@ static inline MemOp size_memop(unsigned size) /* Power of 2 up to 8. */ assert((size & (size - 1)) == 0 && size >= 1 && size <= 8); #endif +#ifdef NEED_CPU_H + return ctz32(size) | MO_TE; +#else return ctz32(size); +#endif } #endif diff --git a/memory_ldst.inc.c b/memory_ldst.inc.c index d7e28d0..ff28b30 100644 --- a/memory_ldst.inc.c +++ b/memory_ldst.inc.c @@ -37,7 +37,8 @@ static inline uint32_t glue(address_space_ldl_internal, SUFFIX)(ARG1_DECL, release_lock |= prepare_mmio_access(mr); /* I/O case */ - r = memory_region_dispatch_read(mr, addr1, &val, MO_32, attrs); + /* FIXME: memory_region_dispatch_read ignores MO_BSWAP. */ + r = memory_region_dispatch_read(mr, addr1, &val, MO_32 | endian, attrs); #if defined(TARGET_WORDS_BIGENDIAN) if (endian == MO_LE) { val = bswap32(val); @@ -112,7 +113,8 @@ static inline uint64_t glue(address_space_ldq_internal, SUFFIX)(ARG1_DECL, release_lock |= prepare_mmio_access(mr); /* I/O case */ - r = memory_region_dispatch_read(mr, addr1, &val, MO_64, attrs); + /* FIXME: memory_region_dispatch_read ignores MO_BSWAP. */ + r = memory_region_dispatch_read(mr, addr1, &val, MO_64 | endian, attrs); #if defined(TARGET_WORDS_BIGENDIAN) if (endian == MO_LE) { val = bswap64(val); @@ -221,7 +223,8 @@ static inline uint32_t glue(address_space_lduw_internal, SUFFIX)(ARG1_DECL, release_lock |= prepare_mmio_access(mr); /* I/O case */ - r = memory_region_dispatch_read(mr, addr1, &val, MO_16, attrs); + /* FIXME: memory_region_dispatch_read ignores MO_BSWAP. */ + r = memory_region_dispatch_read(mr, addr1, &val, MO_16 | endian, attrs); #if defined(TARGET_WORDS_BIGENDIAN) if (endian == MO_LE) { val = bswap16(val); @@ -297,7 +300,7 @@ void glue(address_space_stl_notdirty, SUFFIX)(ARG1_DECL, if (l < 4 || !memory_access_is_direct(mr, true)) { release_lock |= prepare_mmio_access(mr); - r = memory_region_dispatch_write(mr, addr1, val, MO_32, attrs); + r = memory_region_dispatch_write(mr, addr1, val, MO_32 | MO_TE, attrs); } else { ptr = qemu_map_ram_ptr(mr->ram_block, addr1); stl_p(ptr, val); @@ -343,7 +346,8 @@ static inline void glue(address_space_stl_internal, SUFFIX)(ARG1_DECL, val = bswap32(val); } #endif - r = memory_region_dispatch_write(mr, addr1, val, MO_32, attrs); + /* FIXME: memory_region_dispatch_write ignores MO_BSWAP. */ + r = memory_region_dispatch_write(mr, addr1, val, MO_32 | endian, attrs); } else { /* RAM case */ ptr = qemu_map_ram_ptr(mr->ram_block, addr1); @@ -448,7 +452,8 @@ static inline void glue(address_space_stw_internal, SUFFIX)(ARG1_DECL, val = bswap16(val); } #endif - r = memory_region_dispatch_write(mr, addr1, val, MO_16, attrs); + /* FIXME: memory_region_dispatch_write ignores MO_BSWAP. */ + r = memory_region_dispatch_write(mr, addr1, val, MO_16 | endian, attrs); } else { /* RAM case */ ptr = qemu_map_ram_ptr(mr->ram_block, addr1); @@ -521,7 +526,8 @@ static void glue(address_space_stq_internal, SUFFIX)(ARG1_DECL, val = bswap64(val); } #endif - r = memory_region_dispatch_write(mr, addr1, val, MO_64, attrs); + /* FIXME: memory_region_dispatch_write ignores MO_BSWAP. */ + r = memory_region_dispatch_write(mr, addr1, val, MO_64 | endian, attrs); } else { /* RAM case */ ptr = qemu_map_ram_ptr(mr->ram_block, addr1); diff --git a/target/mips/op_helper.c b/target/mips/op_helper.c index e79f99d..1b475f3 100644 --- a/target/mips/op_helper.c +++ b/target/mips/op_helper.c @@ -4741,11 +4741,11 @@ void helper_cache(CPUMIPSState *env, target_ulong addr, uint32_t op) if (op == 9) { /* Index Store Tag */ memory_region_dispatch_write(env->itc_tag, index, env->CP0_TagLo, - MO_64, MEMTXATTRS_UNSPECIFIED); + MO_64 | MO_TE, MEMTXATTRS_UNSPECIFIED); } else if (op == 5) { /* Index Load Tag */ memory_region_dispatch_read(env->itc_tag, index, &env->CP0_TagLo, - MO_64, MEMTXATTRS_UNSPECIFIED); + MO_64 | MO_TE, MEMTXATTRS_UNSPECIFIED); } #endif }
Preparation for collapsing the two byte swaps adjust_endianness and handle_bswap into the former. Call memory_region_dispatch_{read|write} with endianness encoded into the "MemOp op" operand. This patch does not change any behaviour as memory_region_dispatch_{read|write} is yet to handle the endianness. Once it does handle endianness, callers with byte swaps need to collapse them into adjust_endianness. Signed-off-by: Tony Nguyen <tony.nguyen@bt.com> --- hw/s390x/s390-pci-inst.c | 3 ++- hw/virtio/virtio-pci.c | 2 ++ include/exec/memop.h | 4 ++++ memory_ldst.inc.c | 20 +++++++++++++------- target/mips/op_helper.c | 4 ++-- 5 files changed, 23 insertions(+), 10 deletions(-) -- 1.8.3.1 ?