[Qemu-devel] Bug report about x86 'bt' insn

[Qemu-devel] Bug report about x86 'bt' insn

[Torbjorn Granlund]

[This is a bug report, reported here since I was redirected to some
corporate site when attempting to follow the recommended bug report
practice.  If I am really required to register an account with a
corporation in order to report a qemu bug, and that bug reports are not
accepted here, then please accept my apology; I suggest that you simply
ignore this bug report.]

Bug report:

Qemu version: 0.13.0 (installed via FreeBSD's ports system).

Host: FreeBSD 8.1 amd64 (without any relevant kernel modules)

Guest environments where problem has been reproduced: (1) Debian GNU/Linux
5, (2) Debian GNU/kFreeBSD snapshot, (3) FreeBSD 8.1

Problem: The bt insn works incorrectly under qemu.  It incorrectly sets
the Z flag.  The bt insn should only affect the carry flag.  (This
causes the GMP library to work incorrectly.)

Test code snippet:

.text
	.globl	main
main:	mov	$1, %eax
	and	%eax, %eax
	bt	$0, %eax
	jz	1f
	xor	%eax, %eax
1:	ret

Assembly and execute.  Correct behaviour is to exit(0).  Under qemu it
it exits with value 1.

(I thought qemu wouldn't interfere with plain user mode instructions
when the host system can handle all the guest systems's instructions,
and that they would execute natively and at full speed.  This bug
suggests that buggy emulation is taking place.)

-- 
Torbjörn

Re: [Qemu-devel] Bug report about x86 'bt' insn

[malc]

On Sat, 6 Nov 2010, Torbjorn Granlund wrote:

> [This is a bug report, reported here since I was redirected to some
> corporate site when attempting to follow the recommended bug report
> practice.  If I am really required to register an account with a
> corporation in order to report a qemu bug, and that bug reports are not
> accepted here, then please accept my apology; I suggest that you simply
> ignore this bug report.]
> 
> Bug report:
> 
> Qemu version: 0.13.0 (installed via FreeBSD's ports system).
> 
> Host: FreeBSD 8.1 amd64 (without any relevant kernel modules)
> 
> Guest environments where problem has been reproduced: (1) Debian GNU/Linux
> 5, (2) Debian GNU/kFreeBSD snapshot, (3) FreeBSD 8.1
> 
> Problem: The bt insn works incorrectly under qemu.  It incorrectly sets
> the Z flag.  The bt insn should only affect the carry flag.  (This
> causes the GMP library to work incorrectly.)
> 
> Test code snippet:
> 
> .text
> 	.globl	main
> main:	mov	$1, %eax
> 	and	%eax, %eax
> 	bt	$0, %eax
> 	jz	1f
> 	xor	%eax, %eax
> 1:	ret
> 
> Assembly and execute.  Correct behaviour is to exit(0).  Under qemu it
> it exits with value 1.

ZF is undefined according to AMD's 24594.pdf page 69.

> 
> (I thought qemu wouldn't interfere with plain user mode instructions
> when the host system can handle all the guest systems's instructions,
> and that they would execute natively and at full speed.  This bug
> suggests that buggy emulation is taking place.)
> 
> 

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] Bug report about x86 'bt' insn

[Torbjorn Granlund]

malc <av1474@comtv.ru> writes:

  ZF is undefined according to AMD's 24594.pdf page 69.
  
Ah, you're right.  It seems that all existing x86 implementations leave
ZF alone, though.  (I am not arguing that qeum is broken, the bug is in
my code.)

I apologize for the false alarm!

-- 
Torbjörn

Re: [Qemu-devel] Bug report about x86 'bt' insn

[malc]

On Sat, 6 Nov 2010, Torbjorn Granlund wrote:

> malc <av1474@comtv.ru> writes:
> 
>   ZF is undefined according to AMD's 24594.pdf page 69.
>   
> Ah, you're right.  It seems that all existing x86 implementations leave
> ZF alone, though.  (I am not arguing that qeum is broken, the bug is in
> my code.)
> 
> I apologize for the false alarm!

Well, it's not as false as one would imagine, since QEMU sets lazy
eflags evaluation function to SAR[BWL] in BT case which is not so
good from performance standpoint.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] Bug report about x86 'bt' insn

[Nathan Froyd]

On Sat, Nov 06, 2010 at 07:24:39PM +0100, Torbjorn Granlund wrote:
> malc <av1474@comtv.ru> writes:
> 
>   ZF is undefined according to AMD's 24594.pdf page 69.
>   
> Ah, you're right.  It seems that all existing x86 implementations leave
> ZF alone, though.  (I am not arguing that qeum is broken, the bug is in
> my code.)

FWIW, the Intel manuals (253666, June 2010) state that the ZF flag is
unaffected, not just undefined.

-Nathan

Re: [Qemu-devel] Bug report about x86 'bt' insn

[malc]

On Sat, 6 Nov 2010, Nathan Froyd wrote:

> On Sat, Nov 06, 2010 at 07:24:39PM +0100, Torbjorn Granlund wrote:
> > malc <av1474@comtv.ru> writes:
> > 
> >   ZF is undefined according to AMD's 24594.pdf page 69.
> >   
> > Ah, you're right.  It seems that all existing x86 implementations leave
> > ZF alone, though.  (I am not arguing that qeum is broken, the bug is in
> > my code.)
> 
> FWIW, the Intel manuals (253666, June 2010) state that the ZF flag is
> unaffected, not just undefined.

(3-94 Vol. 2A BT it Test)

Flags Affected
The CF flag contains the value of the selected bit. The OF, SF, ZF, AF, 
and PF flags are undefined.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] Bug report about x86 'bt' insn

[malc]

On Sat, 6 Nov 2010, malc wrote:

> On Sat, 6 Nov 2010, Nathan Froyd wrote:
> 
> > On Sat, Nov 06, 2010 at 07:24:39PM +0100, Torbjorn Granlund wrote:
> > > malc <av1474@comtv.ru> writes:
> > > 
> > >   ZF is undefined according to AMD's 24594.pdf page 69.
> > >   
> > > Ah, you're right.  It seems that all existing x86 implementations leave
> > > ZF alone, though.  (I am not arguing that qeum is broken, the bug is in
> > > my code.)
> > 
> > FWIW, the Intel manuals (253666, June 2010) state that the ZF flag is
> > unaffected, not just undefined.
> 
> (3-94 Vol. 2A BT it Test)
> 
> Flags Affected
> The CF flag contains the value of the selected bit. The OF, SF, ZF, AF, 
> and PF flags are undefined.
> 

And this one was March 2009 (conveniently having the same file name
253666), the September 2010 issue reads differently:

Flags Affected
The CF flag contains the value of the selected bit. The ZF flag is 
unaffected. The OF, SF, AF, and PF flags are undefined.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] Bug report about x86 'bt' insn

[Torbjorn Granlund]

malc <av1474@comtv.ru> writes:

  And this one was March 2009 (conveniently having the same file name
  253666), the September 2010 issue reads differently:
  
  Flags Affected
  The CF flag contains the value of the selected bit. The ZF flag is 
  unaffected. The OF, SF, AF, and PF flags are undefined.

Have the qemu developers come to any decision on this one?

I haven't tested many 32-bit x86 implementations, but I think I have
tested all 64-bit x86 implementations, and they all seem to leave ZF
unaffected.  (Clearly, it is possible that they do write it for some
values of the operands; there are too many possible values to test.)

I actually think qemu for both x86-32 and x86-64 should be changed to
make the bt insn not affect ZF.  Considering that Intel have changed
their manuals (I have confirmed this) to say that ZF is unaffected,
there are even stronger reasons to change qemu.  Even without that
documentation change, I would argue that compatibility with all defacto
hardware is important.

(I am not trying to say that my bug report was correct.  It wasn't.  I
have fixed the GMP code.)

-- 
Torbjörn

Re: [Qemu-devel] Bug report about x86 'bt' insn

[malc]

On Wed, 10 Nov 2010, Torbjorn Granlund wrote:

> malc <av1474@comtv.ru> writes:
> 
>   And this one was March 2009 (conveniently having the same file name
>   253666), the September 2010 issue reads differently:
>   
>   Flags Affected
>   The CF flag contains the value of the selected bit. The ZF flag is 
>   unaffected. The OF, SF, AF, and PF flags are undefined.
> 
> Have the qemu developers come to any decision on this one?
> 
> I haven't tested many 32-bit x86 implementations, but I think I have
> tested all 64-bit x86 implementations, and they all seem to leave ZF
> unaffected.  (Clearly, it is possible that they do write it for some
> values of the operands; there are too many possible values to test.)
> 
> I actually think qemu for both x86-32 and x86-64 should be changed to
> make the bt insn not affect ZF.  Considering that Intel have changed
> their manuals (I have confirmed this) to say that ZF is unaffected,
> there are even stronger reasons to change qemu.  Even without that
> documentation change, I would argue that compatibility with all defacto
> hardware is important.
> 

All is the keyword here, i doubt that exhaustive search was performed
furthermore, AMDs documentation (to the best of my knowledge still)
still maintains that ZF is undefined.

That said the bt operation should be changed but for different reasons.

> (I am not trying to say that my bug report was correct.  It wasn't.  I
> have fixed the GMP code.)
> 
> 

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] Bug report about x86 'bt' insn

[Torbjorn Granlund]

malc <av1474@comtv.ru> writes:

  All is the keyword here, i doubt that exhaustive search was performed
  furthermore, AMDs documentation (to the best of my knowledge still)
  still maintains that ZF is undefined.

AMD indeed still says undefined in their documentation (as of two hours
ago).  I have asked AMD for a clarification, mainly to make sure they
are aware of Intel's documentation change.

(I am not certain about what you doubt.  As I said, I did not check all
2^64 operand values combined all possible bt bit position arguments.  I
believe I have checked all 64-bit x86 processors implementations [two
generations of Core2, NHM and WSM based Core ix, Via Nano, as well as all
Athlon64/Opteron generations).)

-- 
Torbjörn

Re: [Qemu-devel] Bug report about x86 'bt' insn

[malc]

On Wed, 10 Nov 2010, Torbjorn Granlund wrote:

> malc <av1474@comtv.ru> writes:
> 
>   All is the keyword here, i doubt that exhaustive search was performed
>   furthermore, AMDs documentation (to the best of my knowledge still)
>   still maintains that ZF is undefined.
> 
> AMD indeed still says undefined in their documentation (as of two hours
> ago).  I have asked AMD for a clarification, mainly to make sure they
> are aware of Intel's documentation change.
> 
> (I am not certain about what you doubt.  As I said, I did not check all
> 2^64 operand values combined all possible bt bit position arguments.  I
> believe I have checked all 64-bit x86 processors implementations [two
> generations of Core2, NHM and WSM based Core ix, Via Nano, as well as all
> Athlon64/Opteron generations).)

I meant all CPU variants.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] Bug report about x86 'bt' insn

[malc]

On Wed, 10 Nov 2010, malc wrote:

> On Wed, 10 Nov 2010, Torbjorn Granlund wrote:
> 
> > malc <av1474@comtv.ru> writes:
> > 
> >   All is the keyword here, i doubt that exhaustive search was performed
> >   furthermore, AMDs documentation (to the best of my knowledge still)
> >   still maintains that ZF is undefined.
> > 
> > AMD indeed still says undefined in their documentation (as of two hours
> > ago).  I have asked AMD for a clarification, mainly to make sure they
> > are aware of Intel's documentation change.
> > 
> > (I am not certain about what you doubt.  As I said, I did not check all
> > 2^64 operand values combined all possible bt bit position arguments.  I
> > believe I have checked all 64-bit x86 processors implementations [two
> > generations of Core2, NHM and WSM based Core ix, Via Nano, as well as all
> > Athlon64/Opteron generations).)
> 
> I meant all CPU variants.

Anyways:

diff --git a/target-i386/translate.c b/target-i386/translate.c
index 7b6e3c2..f416dce 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -6536,8 +6536,7 @@ static target_ulong disas_insn(DisasContext *s, 
target_ulong pc_start)
         tcg_gen_andi_tl(cpu_T[1], cpu_T[1], (1 << (3 + ot)) - 1);
         switch(op) {
         case 0:
-            tcg_gen_shr_tl(cpu_cc_src, cpu_T[0], cpu_T[1]);
-            tcg_gen_movi_tl(cpu_cc_dst, 0);
+            tcg_gen_shr_tl(cpu_tmp4, cpu_T[0], cpu_T[1]);
             break;
         case 1:
             tcg_gen_shr_tl(cpu_tmp4, cpu_T[0], cpu_T[1]);
@@ -6560,7 +6559,14 @@ static target_ulong disas_insn(DisasContext *s, 
target_ulong pc_start)
             tcg_gen_xor_tl(cpu_T[0], cpu_T[0], cpu_tmp0);
             break;
         }
-        s->cc_op = CC_OP_SARB + ot;
+
+        if (s->cc_op != CC_OP_DYNAMIC)
+            gen_op_set_cc_op(s->cc_op);
+        gen_compute_eflags(cpu_cc_src);
+        tcg_gen_andi_tl(cpu_tmp4, cpu_tmp4, CC_C);
+        tcg_gen_or_tl(cpu_cc_src, cpu_cc_src, cpu_tmp4);
+        s->cc_op = CC_OP_EFLAGS;
+
         if (op != 0) {
             if (mod != 3)
                 gen_op_st_T0_A0(ot + s->mem_index);


-- 
mailto:av1474@comtv.ru