| Message ID | 1440719254-12349-1-git-send-email-afaerber@suse.de |
|---|---|
| State | New |
| Headers | show |
On 08/27/2015 04:47 PM, Andreas Färber wrote: > The argument order for the !CONFIG_SOFTMMU case was jumbled up since > ffc6372851d8631a9f9fa56ec613b3244dc635b9 ("tcg/aarch64: use 32-bit > offset for 32-bit user-mode emulation"), regressing from -rc2 to v2.4.0. > Fix their order to avoid segfaults, e.g., in openSUSE's GNU coreutils 8.24. Nack. The argument order is correct, that is... > - tcg_out_qemu_ld_direct(s, memop, ext, data_reg, > - guest_base ? TCG_REG_GUEST_BASE : TCG_REG_XZR, > - otype, addr_reg); > + tcg_out_qemu_ld_direct(s, memop, ext, data_reg, addr_reg, otype, > + guest_base ? TCG_REG_GUEST_BASE : TCG_REG_XZR); TCG_REG_GUEST_BASE is definitely the "base" register, holding a 64-bit host address, while addr_reg is the "offset" register, holding a (potentially) 32-bit guest address. It is (supposed to be) the "offset" register to which the zero-extend is applied. If something's wrong, and I'm not currently in a position to verify one way or another, it's in tcg_out_insn_3310. r~
Am 27.08.2015 um 22:30 schrieb Richard Henderson: > On 08/27/2015 04:47 PM, Andreas Färber wrote: >> The argument order for the !CONFIG_SOFTMMU case was jumbled up since >> ffc6372851d8631a9f9fa56ec613b3244dc635b9 ("tcg/aarch64: use 32-bit >> offset for 32-bit user-mode emulation"), regressing from -rc2 to v2.4.0. >> Fix their order to avoid segfaults, e.g., in openSUSE's GNU coreutils >> 8.24. > > Nack. The argument order is correct, that is... > >> - tcg_out_qemu_ld_direct(s, memop, ext, data_reg, >> - guest_base ? TCG_REG_GUEST_BASE : >> TCG_REG_XZR, >> - otype, addr_reg); >> + tcg_out_qemu_ld_direct(s, memop, ext, data_reg, addr_reg, otype, >> + guest_base ? TCG_REG_GUEST_BASE : >> TCG_REG_XZR); > > TCG_REG_GUEST_BASE is definitely the "base" register, holding a 64-bit > host address, while addr_reg is the "offset" register, holding a > (potentially) 32-bit guest address. It is (supposed to be) the "offset" > register to which the zero-extend is applied. Huh? Please see http://git.qemu-project.org/?p=qemu.git;a=blobdiff;f=tcg/aarch64/tcg-target.c;h=4aca883b8e78b2cae6ec22c19a74f0fa9bc22bfe;hp=d1c550886a461581f93c34d5beead2b3ddfebde5;hb=ffc6372851d8631a9f9fa56ec613b3244dc635b9;hpb=6c0f0c0f124718650a8d682ba275044fc02f6fe2 @@ -1210,12 +1210,15 @@ static void tcg_out_qemu_ld(TCGContext *s, TCGReg data_reg, TCGReg addr_reg, tcg_insn_unit *label_ptr; tcg_out_tlb_read(s, addr_reg, s_bits, &label_ptr, mem_index, 1); - tcg_out_qemu_ld_direct(s, memop, ext, data_reg, addr_reg, TCG_REG_X1); + tcg_out_qemu_ld_direct(s, memop, ext, data_reg, addr_reg, + TCG_TYPE_I64, TCG_REG_X1); add_qemu_ldst_label(s, true, oi, ext, data_reg, addr_reg, s->code_ptr, label_ptr); #else /* !CONFIG_SOFTMMU */ - tcg_out_qemu_ld_direct(s, memop, ext, data_reg, addr_reg, - GUEST_BASE ? TCG_REG_GUEST_BASE : TCG_REG_XZR); + const TCGType otype = TARGET_LONG_BITS == 64 ? TCG_TYPE_I64 : TCG_TYPE_I32; + tcg_out_qemu_ld_direct(s, memop, ext, data_reg, + GUEST_BASE ? TCG_REG_GUEST_BASE : TCG_REG_XZR, + otype, addr_reg); #endif /* CONFIG_SOFTMMU */ } @@ -1229,12 +1232,15 @@ static void tcg_out_qemu_st(TCGContext *s, TCGReg data_reg, TCGReg addr_reg, tcg_insn_unit *label_ptr; tcg_out_tlb_read(s, addr_reg, s_bits, &label_ptr, mem_index, 0); - tcg_out_qemu_st_direct(s, memop, data_reg, addr_reg, TCG_REG_X1); + tcg_out_qemu_st_direct(s, memop, data_reg, addr_reg, + TCG_TYPE_I64, TCG_REG_X1); add_qemu_ldst_label(s, false, oi, s_bits == MO_64, data_reg, addr_reg, s->code_ptr, label_ptr); #else /* !CONFIG_SOFTMMU */ - tcg_out_qemu_st_direct(s, memop, data_reg, addr_reg, - GUEST_BASE ? TCG_REG_GUEST_BASE : TCG_REG_XZR); + const TCGType otype = TARGET_LONG_BITS == 64 ? TCG_TYPE_I64 : TCG_TYPE_I32; + tcg_out_qemu_st_direct(s, memop, data_reg, + GUEST_BASE ? TCG_REG_GUEST_BASE : TCG_REG_XZR, + otype, addr_reg); #endif /* CONFIG_SOFTMMU */ } An otype argument is being inserted as next-to-last argument for the function definitions. Same for the softmmu callsites. Only in the *-user callsites the argument order is being changed with addr_reg and off_r switching order? I don't see why that should be done in this patch. If it was wrong before, it should've been done in a separate patch. > If something's wrong, and I'm not currently in a position to verify one > way or another, it's in tcg_out_insn_3310. git-bisect pointed to the above commit. You are referring to its predecessor http://git.qemu-project.org/?p=qemu.git;a=commit;h=6c0f0c0f124718650a8d682ba275044fc02f6fe2 ? Don't spot anything obviously wrong there immediately... Regards, Andreas
On 08/28/2015 09:23 AM, Andreas Färber wrote: > An otype argument is being inserted as next-to-last argument for the > function definitions. Same for the softmmu callsites. Only in the *-user > callsites the argument order is being changed with addr_reg and off_r > switching order? I don't see why that should be done in this patch. If > it was wrong before, it should've been done in a separate patch. It wasn't "wrong" before because it didn't matter before. The whole point of the patch is that we're changing from (xD = data_reg, xA = addr_reg, xG = GUEST_BASE) ldr xD, [xA, xG] to ldr xD, [xG, wA, uxtw] i.e. zero-extending xA during the address formation. Before this patch, the order of xA and xG doesn't matter; they're both straight addition operands. After this patch, if we don't switch the argument order it'll be xG that gets zero-extending GUEST_BASE, which is obviously incorrect. You'll note that we also switch the argument order for the softmmu case in the next patch, and for the same reason. Tomorrow I'll be home and can do any further investigation in depth. r~
On 28/08/2015 01:47, Andreas Färber wrote: > The argument order for the !CONFIG_SOFTMMU case was jumbled up since > ffc6372851d8631a9f9fa56ec613b3244dc635b9 ("tcg/aarch64: use 32-bit > offset for 32-bit user-mode emulation"), regressing from -rc2 to v2.4.0. > Fix their order to avoid segfaults, e.g., in openSUSE's GNU coreutils 8.24. How does -d out_asm change? The patch was tested on aarch64. Paolo
Am 29.08.2015 um 08:50 schrieb Paolo Bonzini: > On 28/08/2015 01:47, Andreas Färber wrote: >> The argument order for the !CONFIG_SOFTMMU case was jumbled up since >> ffc6372851d8631a9f9fa56ec613b3244dc635b9 ("tcg/aarch64: use 32-bit >> offset for 32-bit user-mode emulation"), regressing from -rc2 to v2.4.0. >> Fix their order to avoid segfaults, e.g., in openSUSE's GNU coreutils 8.24. > > How does -d out_asm change? The patch was tested on aarch64. Breaking stderr log attached. Diff below. ffc6372851d8631a9f9fa56ec613b3244dc635b9^ vs. ffc6372851d8631a9f9fa56ec613b3244dc635b9: @@ -53,7 +53,7 @@ 0x006cd950: 52800000 mov w0, #0x0 0x006cd954: 147ffefd b #+0x1fffbf4 (addr 0x26cd548) 0x006cd958: d2800260 mov x0, #0x13 -0x006cd95c: f2ae81c0 movk x0, #0x740e, lsl #16 +0x006cd95c: f2b4fc60 movk x0, #0xa7e3, lsl #16 0x006cd960: f2c07fe0 movk x0, #0x3ff, lsl #32 0x006cd964: 147ffef9 b #+0x1fffbe4 (addr 0x26cd548) @@ -65,53 +65,53 @@ 0x006cd980: f9009e74 str x20, [x19, #312] 0x006cd984: d1018294 sub x20, x20, #0x60 (96) 0x006cd988: f9409675 ldr x21, [x19, #296] -0x006cd98c: f83f6a95 str x21, [x20, xzr] +0x006cd98c: f8346bf5 str x21, [sp, x20] 0x006cd990: 91002294 add x20, x20, #0x8 (8) 0x006cd994: f9409a75 ldr x21, [x19, #304] -0x006cd998: f83f6a95 str x21, [x20, xzr] +0x006cd998: f8346bf5 str x21, [sp, x20] 0x006cd99c: d1002294 sub x20, x20, #0x8 (8) 0x006cd9a0: f9009e74 str x20, [x19, #312] 0x006cd9a4: aa1403f5 mov x21, x20 0x006cd9a8: f9009675 str x21, [x19, #296] 0x006cd9ac: 91014294 add x20, x20, #0x50 (80) 0x006cd9b0: f9408e76 ldr x22, [x19, #280] -0x006cd9b4: f83f6a96 str x22, [x20, xzr] +0x006cd9b4: f8346bf6 str x22, [sp, x20] 0x006cd9b8: 91002294 add x20, x20, #0x8 (8) 0x006cd9bc: f9409276 ldr x22, [x19, #288] -0x006cd9c0: f83f6a96 str x22, [x20, xzr] +0x006cd9c0: f8346bf6 str x22, [sp, x20] 0x006cd9c4: 9101e2b4 add x20, x21, #0x78 (120) 0x006cd9c8: f9008e74 str x20, [x19, #280] 0x006cd9cc: 910042b6 add x22, x21, #0x10 (16) 0x006cd9d0: f9406e77 ldr x23, [x19, #216] -0x006cd9d4: f83f6ad7 str x23, [x22, xzr] +0x006cd9d4: f8366bf7 str x23, [sp, x22] 0x006cd9d8: 910022d6 add x22, x22, #0x8 (8) 0x006cd9dc: f9407277 ldr x23, [x19, #224] -0x006cd9e0: f83f6ad7 str x23, [x22, xzr] +0x006cd9e0: f8366bf7 str x23, [sp, x22] 0x006cd9e4: 910082b6 add x22, x21, #0x20 (32) 0x006cd9e8: f9407677 ldr x23, [x19, #232] -0x006cd9ec: f83f6ad7 str x23, [x22, xzr] +0x006cd9ec: f8366bf7 str x23, [sp, x22] 0x006cd9f0: 910022d6 add x22, x22, #0x8 (8) 0x006cd9f4: f9407a77 ldr x23, [x19, #240] -0x006cd9f8: f83f6ad7 str x23, [x22, xzr] +0x006cd9f8: f8366bf7 str x23, [sp, x22] 0x006cd9fc: f9402276 ldr x22, [x19, #64] 0x006cda00: f9006e76 str x22, [x19, #216] 0x006cda04: 910a8296 add x22, x20, #0x2a0 (672) 0x006cda08: f9002676 str x22, [x19, #72] 0x006cda0c: 9100c2b7 add x23, x21, #0x30 (48) 0x006cda10: f9407e78 ldr x24, [x19, #248] -0x006cda14: f83f6af8 str x24, [x23, xzr] +0x006cda14: f8376bf8 str x24, [sp, x23] 0x006cda18: 910022f7 add x23, x23, #0x8 (8) 0x006cda1c: f9408278 ldr x24, [x19, #256] -0x006cda20: f83f6af8 str x24, [x23, xzr] +0x006cda20: f8376bf8 str x24, [sp, x23] 0x006cda24: 910102b5 add x21, x21, #0x40 (64) 0x006cda28: f9408677 ldr x23, [x19, #264] -0x006cda2c: f83f6ab7 str x23, [x21, xzr] +0x006cda2c: f8356bf7 str x23, [sp, x21] 0x006cda30: 910022b5 add x21, x21, #0x8 (8) 0x006cda34: f9408a77 ldr x23, [x19, #272] -0x006cda38: f83f6ab7 str x23, [x21, xzr] +0x006cda38: f8356bf7 str x23, [sp, x21] 0x006cda3c: 91010294 add x20, x20, #0x40 (64) 0x006cda40: f9002274 str x20, [x19, #64] -0x006cda44: f83f6a9f str xzr, [x20, xzr] +0x006cda44: f8346bff str xzr, [sp, x20] 0x006cda48: 91002294 add x20, x20, #0x8 (8) 0x006cda4c: f9002274 str x20, [x19, #64] 0x006cda50: cb160295 sub x21, x20, x22 @@ -136,7 +136,7 @@ 0x006cda9c: f2c00814 movk x20, #0x40, lsl #32 0x006cdaa0: f900a274 str x20, [x19, #320] 0x006cdaa4: d2801000 mov x0, #0x80 -0x006cdaa8: f2ae81c0 movk x0, #0x740e, lsl #16 +0x006cdaa8: f2b4fc60 movk x0, #0xa7e3, lsl #16 0x006cdaac: f2c07fe0 movk x0, #0x3ff, lsl #32 0x006cdab0: 147ffea6 b #+0x1fffa98 (addr 0x26cd548) 0x006cdab4: 14000000 b #+0x0 (addr 0x6cdab4) @@ -145,135024 +145,12 @@ 0x006cdac0: f2c00814 movk x20, #0x40, lsl #32 0x006cdac4: f900a274 str x20, [x19, #320] 0x006cdac8: d2801020 mov x0, #0x81 -0x006cdacc: f2ae81c0 movk x0, #0x740e, lsl #16 +0x006cdacc: f2b4fc60 movk x0, #0xa7e3, lsl #16 0x006cdad0: f2c07fe0 movk x0, #0x3ff, lsl #32 0x006cdad4: 147ffe9d b #+0x1fffa74 (addr 0x26cd548) 0x006cdad8: d2801060 mov x0, #0x83 -0x006cdadc: f2ae81c0 movk x0, #0x740e, lsl #16 +0x006cdadc: f2b4fc60 movk x0, #0xa7e3, lsl #16 0x006cdae0: f2c07fe0 movk x0, #0x3ff, lsl #32 0x006cdae4: 147ffe99 b #+0x1fffa64 (addr 0x26cd548) -OUT: [size=180] -0x006cdaf0: b85fc274 ldur w20, [x19, #-4] -0x006cdaf4: 35000514 cbnz w20, #+0xa0 (addr 0x6cdb94) [...] +qemu: uncaught target signal 11 (Segmentation fault) - core dumped Regards, Andreas
On 09/01/2015 09:31 AM, Andreas Färber wrote: > -0x006cd98c: f83f6a95 str x21, [x20, xzr] > +0x006cd98c: f8346bf5 str x21, [sp, x20] Thanks. Sorry it took me so long to get back to this. I've sent a patch to the list. I intended to cc you, but somehow forgot at the last second. r~
On 01/09/2015 18:31, Andreas Färber wrote: > Breaking stderr log attached. Diff below. > > ffc6372851d8631a9f9fa56ec613b3244dc635b9^ vs. > ffc6372851d8631a9f9fa56ec613b3244dc635b9: Richard already answered (and sent a patch), but I also have a question: are you using --disable-guest-base, and if so why? Paolo
On 09/02/2015 02:02 AM, Paolo Bonzini wrote: > > > On 01/09/2015 18:31, Andreas Färber wrote: >> Breaking stderr log attached. Diff below. >> >> ffc6372851d8631a9f9fa56ec613b3244dc635b9^ vs. >> ffc6372851d8631a9f9fa56ec613b3244dc635b9: > > Richard already answered (and sent a patch), but I also have a question: > are you using --disable-guest-base, and if so why? Depending on the (non) overlap of host and guest addresses, 64-bit guests tend to auto-select guest_base == 0. r~
Am 02.09.2015 um 11:02 schrieb Paolo Bonzini: > On 01/09/2015 18:31, Andreas Färber wrote: >> Breaking stderr log attached. Diff below. >> >> ffc6372851d8631a9f9fa56ec613b3244dc635b9^ vs. >> ffc6372851d8631a9f9fa56ec613b3244dc635b9: > > Richard already answered (and sent a patch), but I also have a question: > are you using --disable-guest-base, and if so why? In my test builds I was using only a target list as parameter and defaults otherwise. Regards, Andreas
diff --git a/tcg/aarch64/tcg-target.c b/tcg/aarch64/tcg-target.c index 01ae610..6f7dd4e 100644 --- a/tcg/aarch64/tcg-target.c +++ b/tcg/aarch64/tcg-target.c @@ -1224,9 +1224,8 @@ static void tcg_out_qemu_ld(TCGContext *s, TCGReg data_reg, TCGReg addr_reg, add_qemu_ldst_label(s, true, oi, ext, data_reg, addr_reg, s->code_ptr, label_ptr); #else /* !CONFIG_SOFTMMU */ - tcg_out_qemu_ld_direct(s, memop, ext, data_reg, - guest_base ? TCG_REG_GUEST_BASE : TCG_REG_XZR, - otype, addr_reg); + tcg_out_qemu_ld_direct(s, memop, ext, data_reg, addr_reg, otype, + guest_base ? TCG_REG_GUEST_BASE : TCG_REG_XZR); #endif /* CONFIG_SOFTMMU */ } @@ -1245,9 +1244,8 @@ static void tcg_out_qemu_st(TCGContext *s, TCGReg data_reg, TCGReg addr_reg, add_qemu_ldst_label(s, false, oi, (memop & MO_SIZE)== MO_64, data_reg, addr_reg, s->code_ptr, label_ptr); #else /* !CONFIG_SOFTMMU */ - tcg_out_qemu_st_direct(s, memop, data_reg, - guest_base ? TCG_REG_GUEST_BASE : TCG_REG_XZR, - otype, addr_reg); + tcg_out_qemu_st_direct(s, memop, data_reg, addr_reg, otype, + guest_base ? TCG_REG_GUEST_BASE : TCG_REG_XZR); #endif /* CONFIG_SOFTMMU */ }
The argument order for the !CONFIG_SOFTMMU case was jumbled up since ffc6372851d8631a9f9fa56ec613b3244dc635b9 ("tcg/aarch64: use 32-bit offset for 32-bit user-mode emulation"), regressing from -rc2 to v2.4.0. Fix their order to avoid segfaults, e.g., in openSUSE's GNU coreutils 8.24. Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Aurelien Jarno <aurelien@aurel32.net> Cc: Richard Henderson <rth@twiddle.net> Cc: qemu-stable@nongnu.org (v2.4.0) Signed-off-by: Andreas Färber <afaerber@suse.de> --- tcg/aarch64/tcg-target.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)