For the bsr and bsf instructions per the Intel SDM: "If the content of the source operand is 0, the content of the destination operand is undefined." The AMD64 Architecture Programmer's Manual is more specific: it states the destination operand remains unchanged when the source is zero. Testing on real hardware (multiple Intel and AMD systems) confirms that when the source operand is zero, the CPU leaves the entire 64-bit destination register untouched, including the upper 32 bits, even when executing the 32-bit form of the instruction (e.g. "bsr edx, ecx") in 64-bit mode. gen_BSF()/gen_BSR() already encode this intent (see the existing comment) by arranging for T0 to hold the correct full-width passthrough value when the source is zero. However, that correct value was then handed to the generic register writeback path (gen_writeback), which for a 32-bit destination unconditionally applies tcg_gen_ext32u_tl() and clears the upper 32 bits regardless of what gen_BSF()/gen_BSR() had just computed. This patch bypasses the generic writeback for this specific case (64-bit mode, 32-bit operand size) and writes the already-correct value directly to the register instead. Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4132 Signed-off-by: Simon Scherer <scherer.simon89@gmail.com> --- target/i386/tcg/emit.c.inc | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/target/i386/tcg/emit.c.inc b/target/i386/tcg/emit.c.inc index XXXXXXX..XXXXXXX 100644 --- a/target/i386/tcg/emit.c.inc +++ b/target/i386/tcg/emit.c.inc @@ -XXX,XX +XXX,XX @@ static void gen_BSF(DisasContext *s, X86DecodedInsn *decode) * by passing the output as the value to return upon zero. */ tcg_gen_ctz_tl(s->T0, s->T0, s->T1); + + /* + * Bypass the gen_writeback: for a 32-bit destination in 64-bit + * mode it would zero-extend the upper 32 bits, but T0 already holds + * the exact final register value for both the zero- and non-zero- + * source cases (see comment above). + */ + if (CODE64(s) && ot == MO_32) { + tcg_gen_mov_tl(cpu_regs[decode->op[0].n], s->T0); + decode->op[0].unit = X86_OP_SKIP; + } } /* Non-standard convention - on entry T0 is zero-extended input, T1 is the output. */ @@ -XXX,XX +XXX,XX @@ static void gen_BSR(DisasContext *s, X86DecodedInsn *decode) tcg_gen_xori_tl(s->T1, s->T1, TARGET_LONG_BITS - 1); tcg_gen_clz_tl(s->T0, s->T0, s->T1); tcg_gen_xori_tl(s->T0, s->T0, TARGET_LONG_BITS - 1); + + /* See gen_BSF() above. */ + if (CODE64(s) && ot == MO_32) { + tcg_gen_mov_tl(cpu_regs[decode->op[0].n], s->T0); + decode->op[0].unit = X86_OP_SKIP; + } } static void gen_BSWAP(DisasContext *s, X86DecodedInsn *decode) -- 2.53.0 For the bsr and bsf instructions per the Intel SDM: "If the content of the source operand is 0, the content of the destination operand is undefined." The AMD64 Architecture Programmer's Manual is more specific: it states the destination operand remains unchanged when the source is zero. Testing on real hardware (multiple Intel and AMD systems) confirms that when the source operand is zero, the CPU leaves the entire 64-bit destination register untouched, including the upper 32 bits, even when executing the 32-bit form of the instruction (e.g. "bsr edx, ecx") in 64-bit mode. gen_BSF()/gen_BSR() already encode this intent (see the existing comment) by arranging for T0 to hold the correct full-width passthrough value when the source is zero. However, that correct value was then handed to the generic register writeback path (gen_writeback), which for a 32-bit destination unconditionally applies tcg_gen_ext32u_tl() and clears the upper 32 bits regardless of what gen_BSF()/gen_BSR() had just computed. Fix this at the operand-decode level instead: give the destination (G) and its 2op copy the d64 size class instead of v, so gen_writeback treats the write as full-width in the one case that was buggy (64-bit mode, 32-bit operand size), leaving every other case unchanged. The source (E) must stay v, or the decoder would scan the full 64-bit source register instead of just its low 32 bits. No changes to gen_BSF()/gen_BSR() are needed. Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4132 Signed-off-by: Simon Scherer <scherer.simon89@gmail.com> --- target/i386/tcg/decode-new.c.inc | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/target/i386/tcg/decode-new.c.inc b/target/i386/tcg/decode-new.c.inc index XXXXXXX..XXXXXXX 100644 --- a/target/i386/tcg/decode-new.c.inc +++ b/target/i386/tcg/decode-new.c.inc @@ -XXX,XX +XXX,XX @@ static void decode_0FB8(DisasContext *s, CPUX86State *env, X86OpEntry *entry, ui static void decode_0FBC(DisasContext *s, CPUX86State *env, X86OpEntry *entry, uint8_t *b) { - /* For BSF, pass 2op as the third operand so that we can use zextT0 */ + /* + * For BSF, pass 2op as the third operand so that we can use zextT0. + * G/2op use d64 because ctz already zero-extends the full 64-bit result, + * and v would zero-extend the output register if the input is zero. + * E stays v: forcing it to d64 too would scan the full 64-bit register + * instead of just the 32-bit source. + */ static const X86OpEntry opcodes_0FBC[4] = { - X86_OP_ENTRY3(BSF, G,v, E,v, 2op,v, zextT0), - X86_OP_ENTRY3(BSF, G,v, E,v, 2op,v, zextT0), /* 0x66 */ - X86_OP_ENTRYwr(TZCNT, G,v, E,v, zextT0), /* 0xf3 */ - X86_OP_ENTRY3(BSF, G,v, E,v, 2op,v, zextT0), /* 0xf2 */ + X86_OP_ENTRY3(BSF, G,d64, E,v, 2op,d64, zextT0), + X86_OP_ENTRY3(BSF, G,d64, E,v, 2op,d64, zextT0), /* 0x66 */ + X86_OP_ENTRYwr(TZCNT, G,v, E,v, zextT0), /* 0xf3 */ + X86_OP_ENTRY3(BSF, G,d64, E,v, 2op,d64, zextT0), /* 0xf2 */ }; if (!(s->cpuid_ext3_features & CPUID_EXT3_ABM)) { *entry = opcodes_0FBC[0]; @@ -XXX,XX +XXX,XX @@ static void decode_0FBC(DisasContext *s, CPUX86State *env, X86OpEntry *entry, ui static void decode_0FBD(DisasContext *s, CPUX86State *env, X86OpEntry *entry, uint8_t *b) { - /* For BSR, pass 2op as the third operand so that we can use zextT0 */ + /* + * For BSR, pass 2op as the third operand so that we can use zextT0. + * G/2op use d64 because clz already zero-extends the full 64-bit result, + * and v would zero-extend the output register if the input is zero. + * E stays v: forcing it to d64 too would scan the full 64-bit register + * instead of just the 32-bit source. + */ static const X86OpEntry opcodes_0FBD[4] = { - X86_OP_ENTRY3(BSR, G,v, E,v, 2op,v, zextT0), - X86_OP_ENTRY3(BSR, G,v, E,v, 2op,v, zextT0), /* 0x66 */ - X86_OP_ENTRYwr(LZCNT, G,v, E,v, zextT0), /* 0xf3 */ - X86_OP_ENTRY3(BSR, G,v, E,v, 2op,v, zextT0), /* 0xf2 */ + X86_OP_ENTRY3(BSR, G,d64, E,v, 2op,d64, zextT0), + X86_OP_ENTRY3(BSR, G,d64, E,v, 2op,d64, zextT0), /* 0x66 */ + X86_OP_ENTRYwr(LZCNT, G,v, E,v, zextT0), /* 0xf3 */ + X86_OP_ENTRY3(BSR, G,d64, E,v, 2op,d64, zextT0), /* 0xf2 */ }; if (!(s->cpuid_7_0_ebx_features & CPUID_7_0_EBX_BMI1)) { *entry = opcodes_0FBD[0]; -- 2.53.0