| Message ID | 1379009870-18323-1-git-send-email-sw@weilnetz.de |
|---|---|
| State | Accepted |
| Headers | show |
On 09/12/2013 11:17 AM, Stefan Weil wrote: > @@ -1093,7 +1093,6 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr) > tmp8 = helper_ldb_mmu(env, taddr, tci_read_i(&tb_ptr)); > #else > host_addr = (tcg_target_ulong)taddr; > - assert(taddr == host_addr); > tmp8 = *(uint8_t *)(host_addr + GUEST_BASE); > #endif I noticed first that g2h would be better than fiddling GUEST_BASE by hand. But then I noticed failure to handle endianness and failure to handle unaligned accesses too. You should be using tmp8 = ldub(taddr); et al. See include/exec/cpu-all.h, beginning line 253. r~
Am 12.09.2013 20:39, schrieb Richard Henderson: > On 09/12/2013 11:17 AM, Stefan Weil wrote: >> @@ -1093,7 +1093,6 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr) >> tmp8 = helper_ldb_mmu(env, taddr, tci_read_i(&tb_ptr)); >> #else >> host_addr = (tcg_target_ulong)taddr; >> - assert(taddr == host_addr); >> tmp8 = *(uint8_t *)(host_addr + GUEST_BASE); >> #endif > I noticed first that g2h would be better than fiddling GUEST_BASE > by hand. But then I noticed failure to handle endianness and > failure to handle unaligned accesses too. > > You should be using > > tmp8 = ldub(taddr); > > et al. See include/exec/cpu-all.h, beginning line 253. > > > r~ Thanks for your hint. Yes, as you can see from tcg/tci/README,the test matrix of TCI did not include big endian hosts up to now. Testing on an emulated big endian Malta system is terribly slow, and I have no access to real big endian hardware fortests. But I think that such changes are independent of this patchwhich can be applied as it is. Regards, Stefan
On 09/12/2013 11:57 AM, Stefan Weil wrote: > Thanks for your hint. Yes, as you can see from tcg/tci/README,the test > matrix of TCI > did not include big endian hosts up to now. Testing on an emulated big > endian Malta > system is terribly slow, and I have no access to real big endian > hardware fortests. Recall that we're not necessarily talking about big-endian host, but big-endian target on little-endian host. I don't see any byte swapping going on here at all. FYI, the smoke-test I use for big-endian is $ qemu-system-sparc -nographic Configuration device id QEMU version 1 machine id 32 CPUs: 1 x FMI,MB86904 UUID: 00000000-0000-0000-0000-000000000000 Welcome to OpenBIOS v1.1 built on Jul 30 2013 21:43 Type 'help' for detailed information Trying disk... No valid state has been set by load or init-program 0 > At which point you can signal the monitor to exit with ctrl-a x. > But I think that such changes are independent of this patchwhich can be > applied as it is. Fair enough. In which case Reviewed-by: Richard Henderson <rth@twiddle.net> r~
Am 12.09.2013 22:07, schrieb Richard Henderson: > On 09/12/2013 11:57 AM, Stefan Weil wrote: >> Thanks for your hint. Yes, as you can see from tcg/tci/README,the test >> matrix of TCI >> did not include big endian hosts up to now. Testing on an emulated big >> endian Malta >> system is terribly slow, and I have no access to real big endian >> hardware fortests. > Recall that we're not necessarily talking about big-endian host, > but big-endian target on little-endian host. I don't see any > byte swapping going on here at all. > > FYI, the smoke-test I use for big-endian is > > $ qemu-system-sparc -nographic > Configuration device id QEMU version 1 machine id 32 > CPUs: 1 x FMI,MB86904 > UUID: 00000000-0000-0000-0000-000000000000 > Welcome to OpenBIOS v1.1 built on Jul 30 2013 21:43 > Type 'help' for detailed information > Trying disk... > No valid state has been set by load or init-program > > 0 > > > At which point you can signal the monitor to exit with ctrl-a x. TCI passes your smoke-test on i686, x86_64 and maybe also on ARM hosts (ARM is still running, up to now it got the first line of the test done).
Am 12.09.2013 22:28, schrieb Stefan Weil: > TCI passes your smoke-test on i686, x86_64 and maybe also on ARM hosts > (ARM is still running, up to now it got the first line of the test done). ARM finished the test successfully now, too.
12.09.2013 22:17, Stefan Weil wrote: > Debian busybox-static for alpha has a load address of 0x0000000120000000 > which is mapped to 0x0000000020000000 for 32 bit hosts. > > qemu-alpha uses the TCG opcodes qemu_ld32, qemu_ld64, qemu_st32 and > qemu_st64 which all raise the assertion (taddr == host_addr). > > Remove all assertions of this type because they are either wrong or > unnecessary (when sizeof(tcg_target_ulong) >= sizeof(target_ulong)). Thanks, applied to the trivial-patches queue. I'm not sure why this needs to go there, as you're a maintainer of TCI, but since you sent it especially to -trivial I'm applying it. /mjt
diff --git a/tci.c b/tci.c index cea7234..0202ed9 100644 --- a/tci.c +++ b/tci.c @@ -1093,7 +1093,6 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr) tmp8 = helper_ldb_mmu(env, taddr, tci_read_i(&tb_ptr)); #else host_addr = (tcg_target_ulong)taddr; - assert(taddr == host_addr); tmp8 = *(uint8_t *)(host_addr + GUEST_BASE); #endif tci_write_reg8(t0, tmp8); @@ -1105,7 +1104,6 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr) tmp8 = helper_ldb_mmu(env, taddr, tci_read_i(&tb_ptr)); #else host_addr = (tcg_target_ulong)taddr; - assert(taddr == host_addr); tmp8 = *(uint8_t *)(host_addr + GUEST_BASE); #endif tci_write_reg8s(t0, tmp8); @@ -1117,7 +1115,6 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr) tmp16 = helper_ldw_mmu(env, taddr, tci_read_i(&tb_ptr)); #else host_addr = (tcg_target_ulong)taddr; - assert(taddr == host_addr); tmp16 = tswap16(*(uint16_t *)(host_addr + GUEST_BASE)); #endif tci_write_reg16(t0, tmp16); @@ -1129,7 +1126,6 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr) tmp16 = helper_ldw_mmu(env, taddr, tci_read_i(&tb_ptr)); #else host_addr = (tcg_target_ulong)taddr; - assert(taddr == host_addr); tmp16 = tswap16(*(uint16_t *)(host_addr + GUEST_BASE)); #endif tci_write_reg16s(t0, tmp16); @@ -1142,7 +1138,6 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr) tmp32 = helper_ldl_mmu(env, taddr, tci_read_i(&tb_ptr)); #else host_addr = (tcg_target_ulong)taddr; - assert(taddr == host_addr); tmp32 = tswap32(*(uint32_t *)(host_addr + GUEST_BASE)); #endif tci_write_reg32(t0, tmp32); @@ -1154,7 +1149,6 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr) tmp32 = helper_ldl_mmu(env, taddr, tci_read_i(&tb_ptr)); #else host_addr = (tcg_target_ulong)taddr; - assert(taddr == host_addr); tmp32 = tswap32(*(uint32_t *)(host_addr + GUEST_BASE)); #endif tci_write_reg32s(t0, tmp32); @@ -1167,7 +1161,6 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr) tmp32 = helper_ldl_mmu(env, taddr, tci_read_i(&tb_ptr)); #else host_addr = (tcg_target_ulong)taddr; - assert(taddr == host_addr); tmp32 = tswap32(*(uint32_t *)(host_addr + GUEST_BASE)); #endif tci_write_reg32(t0, tmp32); @@ -1182,7 +1175,6 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr) tmp64 = helper_ldq_mmu(env, taddr, tci_read_i(&tb_ptr)); #else host_addr = (tcg_target_ulong)taddr; - assert(taddr == host_addr); tmp64 = tswap64(*(uint64_t *)(host_addr + GUEST_BASE)); #endif tci_write_reg(t0, tmp64); @@ -1198,7 +1190,6 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr) helper_stb_mmu(env, taddr, t0, t2); #else host_addr = (tcg_target_ulong)taddr; - assert(taddr == host_addr); *(uint8_t *)(host_addr + GUEST_BASE) = t0; #endif break; @@ -1210,7 +1201,6 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr) helper_stw_mmu(env, taddr, t0, t2); #else host_addr = (tcg_target_ulong)taddr; - assert(taddr == host_addr); *(uint16_t *)(host_addr + GUEST_BASE) = tswap16(t0); #endif break; @@ -1222,7 +1212,6 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr) helper_stl_mmu(env, taddr, t0, t2); #else host_addr = (tcg_target_ulong)taddr; - assert(taddr == host_addr); *(uint32_t *)(host_addr + GUEST_BASE) = tswap32(t0); #endif break; @@ -1234,7 +1223,6 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr) helper_stq_mmu(env, taddr, tmp64, t2); #else host_addr = (tcg_target_ulong)taddr; - assert(taddr == host_addr); *(uint64_t *)(host_addr + GUEST_BASE) = tswap64(tmp64); #endif break;
Debian busybox-static for alpha has a load address of 0x0000000120000000 which is mapped to 0x0000000020000000 for 32 bit hosts. qemu-alpha uses the TCG opcodes qemu_ld32, qemu_ld64, qemu_st32 and qemu_st64 which all raise the assertion (taddr == host_addr). Remove all assertions of this type because they are either wrong or unnecessary (when sizeof(tcg_target_ulong) >= sizeof(target_ulong)). Cc: qemu-stable <qemu-stable@nongnu.org> Signed-off-by: Stefan Weil <sw@weilnetz.de> --- tci.c | 12 ------------ 1 file changed, 12 deletions(-)