Patch attached. Signed-off-by: Polykarpos Vergos [email protected]
From 0b4961a251c645e9367445f86a3783c69a824809 Mon Sep 17 00:00:00 2001 From: Polykarpos Vergos <[email protected]> Date: Mon, 10 Aug 2026 14:49:04 +0200 Subject: [PATCH] target/i386: leave BSF/BSR destination unchanged on a zero source
The Intel SDM documents the destination of BSF/BSR as undefined when the source operand is zero, but on real hardware (both Intel and AMD) the destination register is left completely unchanged, and software relies on that. For the 32-bit form in 64-bit mode this includes the upper 32 bits: unlike an ordinary 32-bit register write, a zero source does not zero-extend the destination. QEMU passed the old destination value as the "value on zero" input to ctz/clz, but the subsequent register writeback still applied the normal 32-bit zero-extension, clearing the upper half of the destination when the source was zero. Compute the bit index for a nonzero source and use a conditional move, so that a zero source leaves the whole destination register untouched. Add a test for the 16/32/64-bit forms; the existing test-i386 coverage missed this because it only exercised destinations whose upper bits were already zero. Resolves: https://gitlab.com/qemu-project/qemu/-/issues/4132 Signed-off-by: Polykarpos Vergos <[email protected]> --- target/i386/tcg/emit.c.inc | 42 +++++++++++++++------- tests/tcg/i386/Makefile.target | 2 +- tests/tcg/i386/test-i386-bsx.c | 65 ++++++++++++++++++++++++++++++++++ 3 files changed, 95 insertions(+), 14 deletions(-) create mode 100644 tests/tcg/i386/test-i386-bsx.c diff --git a/target/i386/tcg/emit.c.inc b/target/i386/tcg/emit.c.inc index 473f415766..7cf695f7ea 100644 --- a/target/i386/tcg/emit.c.inc +++ b/target/i386/tcg/emit.c.inc @@ -1396,6 +1396,8 @@ static void gen_BOUND(DisasContext *s, X86DecodedInsn *decode) static void gen_BSF(DisasContext *s, X86DecodedInsn *decode) { MemOp ot = decode->op[0].ot; + int reg = decode->op[0].n; + TCGv dest, result; /* Only the Z bit is defined and it is related to the input. */ decode->cc_dst = tcg_temp_new(); @@ -1403,18 +1405,27 @@ static void gen_BSF(DisasContext *s, X86DecodedInsn *decode) tcg_gen_mov_tl(decode->cc_dst, s->T0); /* - * The manual says that the output is undefined when the - * input is zero, but real hardware leaves it unchanged, and - * real programs appear to depend on that. Accomplish this - * by passing the output as the value to return upon zero. + * The manual says that the output is undefined when the input is zero, + * but real hardware leaves the whole destination register unchanged -- + * it is not even zero-extended for a 32-bit operation -- and real + * programs appear to depend on that. Compute the result for a nonzero + * input, then use a conditional move so that a zero input leaves the + * destination register untouched. */ - tcg_gen_ctz_tl(s->T0, s->T0, s->T1); + result = tcg_temp_new(); + tcg_gen_ctz_tl(result, s->T0, s->T0); + dest = gen_op_deposit_reg_v(s, ot, reg, result, result); + tcg_gen_movcond_tl(TCG_COND_EQ, dest, decode->cc_dst, + tcg_constant_tl(0), dest, result); + decode->op[0].unit = X86_OP_SKIP; } /* Non-standard convention - on entry T0 is zero-extended input, T1 is the output. */ static void gen_BSR(DisasContext *s, X86DecodedInsn *decode) { MemOp ot = decode->op[0].ot; + int reg = decode->op[0].n; + TCGv dest, result; /* Only the Z bit is defined and it is related to the input. */ decode->cc_dst = tcg_temp_new(); @@ -1422,15 +1433,20 @@ static void gen_BSR(DisasContext *s, X86DecodedInsn *decode) tcg_gen_mov_tl(decode->cc_dst, s->T0); /* - * The manual says that the output is undefined when the - * input is zero, but real hardware leaves it unchanged, and - * real programs appear to depend on that. Accomplish this - * by passing the output as the value to return upon zero. - * Plus, return the bit index of the first 1 bit. + * The manual says that the output is undefined when the input is zero, + * but real hardware leaves the whole destination register unchanged -- + * it is not even zero-extended for a 32-bit operation -- and real + * programs appear to depend on that. Compute the bit index of the most + * significant one bit for a nonzero input, then use a conditional move + * so that a zero input leaves the destination register untouched. */ - 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); + result = tcg_temp_new(); + tcg_gen_clz_tl(result, s->T0, tcg_constant_tl(TARGET_LONG_BITS)); + tcg_gen_xori_tl(result, result, TARGET_LONG_BITS - 1); + dest = gen_op_deposit_reg_v(s, ot, reg, result, result); + tcg_gen_movcond_tl(TCG_COND_EQ, dest, decode->cc_dst, + tcg_constant_tl(0), dest, result); + decode->op[0].unit = X86_OP_SKIP; } static void gen_BSWAP(DisasContext *s, X86DecodedInsn *decode) diff --git a/tests/tcg/i386/Makefile.target b/tests/tcg/i386/Makefile.target index f1df40411b..f8f41060f2 100644 --- a/tests/tcg/i386/Makefile.target +++ b/tests/tcg/i386/Makefile.target @@ -14,7 +14,7 @@ config-cc.mak: Makefile I386_SRCS=$(notdir $(wildcard $(I386_SRC)/*.c)) ALL_X86_TESTS=$(I386_SRCS:.c=) SKIP_I386_TESTS=test-i386-ssse3 test-avx test-3dnow test-mmx test-flags -X86_64_TESTS:=$(filter test-i386-adcox test-i386-bmi2 $(SKIP_I386_TESTS), $(ALL_X86_TESTS)) +X86_64_TESTS:=$(filter test-i386-adcox test-i386-bmi2 test-i386-bsx $(SKIP_I386_TESTS), $(ALL_X86_TESTS)) test-i386-sse-exceptions: CFLAGS += -msse4.1 -mfpmath=sse run-test-i386-sse-exceptions: QEMU_OPTS += -cpu max diff --git a/tests/tcg/i386/test-i386-bsx.c b/tests/tcg/i386/test-i386-bsx.c new file mode 100644 index 0000000000..75ec7f30a7 --- /dev/null +++ b/tests/tcg/i386/test-i386-bsx.c @@ -0,0 +1,65 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Test that BSF/BSR leave the destination register unchanged when the + * source operand is zero, as real hardware does. + * + * In particular, the 32-bit form in 64-bit mode must not zero-extend the + * destination on a zero source: the entire 64-bit register is preserved, + * which is a documented exception to the usual x86-64 rule that a 32-bit + * register write clears the upper half. For a nonzero source the normal + * rules apply and the 32-bit result is zero-extended. + * + * https://gitlab.com/qemu-project/qemu/-/issues/4132 + */ +#include <assert.h> +#include <stdint.h> + +#define DEST_INIT 0x1122334455667788ULL +/* DEST_INIT with the low 16 bits cleared, for the 16-bit writeback cases. */ +#define DEST_HI16 (DEST_INIT & ~0xffffULL) + +#define DEFINE_BSX(name, insn, sfx) \ + static uint64_t name(uint64_t dst, uint64_t src, int *zf) \ + { \ + uint8_t z; \ + asm(insn " %" sfx "2, %" sfx "0 ; setz %b1" \ + : "+r"(dst), "=q"(z) \ + : "r"(src) : "cc"); \ + *zf = z; \ + return dst; \ + } + +DEFINE_BSX(bsf_w, "bsf", "w") +DEFINE_BSX(bsr_w, "bsr", "w") +DEFINE_BSX(bsf_l, "bsf", "k") +DEFINE_BSX(bsr_l, "bsr", "k") +DEFINE_BSX(bsf_q, "bsf", "") +DEFINE_BSX(bsr_q, "bsr", "") + +int main(void) +{ + int zf; + + /* Zero source: the whole 64-bit destination is left unchanged. */ + assert(bsf_w(DEST_INIT, 0, &zf) == DEST_INIT && zf == 1); + assert(bsr_w(DEST_INIT, 0, &zf) == DEST_INIT && zf == 1); + assert(bsf_l(DEST_INIT, 0, &zf) == DEST_INIT && zf == 1); + assert(bsr_l(DEST_INIT, 0, &zf) == DEST_INIT && zf == 1); + assert(bsf_q(DEST_INIT, 0, &zf) == DEST_INIT && zf == 1); + assert(bsr_q(DEST_INIT, 0, &zf) == DEST_INIT && zf == 1); + + /* + * Nonzero source: the destination receives the bit index. The 32-bit + * form zero-extends it into the upper half (so the nonzero upper bits of + * DEST_INIT are cleared), while the 16-bit form only writes the low 16 + * bits and preserves the rest. + */ + assert(bsf_l(DEST_INIT, 0x00340120, &zf) == 5 && zf == 0); + assert(bsr_l(DEST_INIT, 0x00340120, &zf) == 21 && zf == 0); + assert(bsf_q(DEST_INIT, 0x0034012000000000ULL, &zf) == 37 && zf == 0); + assert(bsr_q(DEST_INIT, 0x0034012000000000ULL, &zf) == 53 && zf == 0); + assert(bsf_w(DEST_INIT, 0x0120, &zf) == (DEST_HI16 | 5) && zf == 0); + assert(bsr_w(DEST_INIT, 0x0120, &zf) == (DEST_HI16 | 8) && zf == 0); + + return 0; +} -- 2.43.0
