[Qemu-devel] [PATCH] i386/translate: ignore 0x67 (PREFIX_ADR) on TARGET_X86_64 && CODE64()

[Qemu-devel] [PATCH] i386/translate: ignore 0x67 (PREFIX_ADR) on TARGET_X86_64 && CODE64()

[Laszlo Ersek]

The code reorganization in commit 4a6fd938 broke handling of PREFIX_ADR.
Restore the previous behavior:

If TARGET_X86_64 *and* CODE64():
  (a) PREFIX_ADR set: no effect, "aflag" should stay at the original
      "s->code32" value,
  (b) PREFIX_ADR clear: "aflag" should be set to constant 2.

Otherwise:
  (c) PREFIX_ADR set: the least significant bit in "aflag" (originally
      initialized form "s->code32") should be negated,
  (d) PREFIX_ADR clear: no effect, "aflag" should stay at the original
      "s->code32" value.

Currently branch (a) is mishandled as branch (c).

Please-review: Richard Henderson <rth@twiddle.net>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 target-i386/translate.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/target-i386/translate.c b/target-i386/translate.c
index 0aeccdb..86f2678 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -4813,7 +4813,11 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
             /* 0x66 is ignored if rex.w is set */
             dflag = 2;
         }
-        if (!(prefixes & PREFIX_ADR)) {
+        if (prefixes & PREFIX_ADR) {
+            /* flip it back, 0x67 should have no effect */
+            aflag ^= 1;
+        }
+        else {
             aflag = 2;
         }
     }
-- 
1.7.1

Re: [Qemu-devel] [PATCH] i386/translate: ignore 0x67 (PREFIX_ADR) on TARGET_X86_64 && CODE64()

[Richard Henderson]

On 2013-05-24 14:37, Laszlo Ersek wrote:
> @@ -4813,7 +4813,11 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
>               /* 0x66 is ignored if rex.w is set */
>               dflag = 2;
>           }
> -        if (!(prefixes & PREFIX_ADR)) {
> +        if (prefixes & PREFIX_ADR) {
> +            /* flip it back, 0x67 should have no effect */
> +            aflag ^= 1;
> +        }
> +        else {
>               aflag = 2;
>           }
>       }
>

Agreed that there's a bug here.  I'm thinking it would be clearer to not write 
this as yet another flip, but understand that unlike dflag, aflag can only be
either 1 or 2 in 64-bit mode.

I'm thinking of something more like this:


r~


diff --git a/target-i386/translate.c b/target-i386/translate.c
index 0aeccdb..bf772aa 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -4813,9 +4813,8 @@ static target_ulong disas_insn(CPUX86State *env, 
DisasContext *s,
              /* 0x66 is ignored if rex.w is set */
              dflag = 2;
          }
-        if (!(prefixes & PREFIX_ADR)) {
-            aflag = 2;
-        }
+        /* 0x67 toggles between 64-bit and 32-bit addressing.  */
+        aflag = (prefixes & PREFIX_ADR ? 1 : 2);
      }
  #endif

Re: [Qemu-devel] [PATCH] i386/translate: ignore 0x67 (PREFIX_ADR) on TARGET_X86_64 && CODE64()

[Paolo Bonzini]

Il 26/05/2013 01:23, Richard Henderson ha scritto:
> On 2013-05-24 14:37, Laszlo Ersek wrote:
>> @@ -4813,7 +4813,11 @@ static target_ulong disas_insn(CPUX86State
>> *env, DisasContext *s,
>>               /* 0x66 is ignored if rex.w is set */
>>               dflag = 2;
>>           }
>> -        if (!(prefixes & PREFIX_ADR)) {
>> +        if (prefixes & PREFIX_ADR) {
>> +            /* flip it back, 0x67 should have no effect */
>> +            aflag ^= 1;
>> +        }
>> +        else {
>>               aflag = 2;
>>           }
>>       }
>>
> 
> Agreed that there's a bug here.  I'm thinking it would be clearer to not
> write this as yet another flip, but understand that unlike dflag, aflag
> can only be either 1 or 2 in 64-bit mode.
> 
> I'm thinking of something more like this:
> 
> diff --git a/target-i386/translate.c b/target-i386/translate.c
> index 0aeccdb..bf772aa 100644
> --- a/target-i386/translate.c
> +++ b/target-i386/translate.c
> @@ -4813,9 +4813,8 @@ static target_ulong disas_insn(CPUX86State *env,
> DisasContext *s,
>              /* 0x66 is ignored if rex.w is set */
>              dflag = 2;
>          }
> -        if (!(prefixes & PREFIX_ADR)) {
> -            aflag = 2;
> -        }
> +        /* 0x67 toggles between 64-bit and 32-bit addressing.  */
> +        aflag = (prefixes & PREFIX_ADR ? 1 : 2);

Isn't that just "aflag++"?  Needs a comment of course ("toggle between
32- and 64-bit, not 16- and 32-bit.").

Paolo

Re: [Qemu-devel] [PATCH] i386/translate: ignore 0x67 (PREFIX_ADR) on TARGET_X86_64 && CODE64()

[Laszlo Ersek]

On 05/26/13 10:33, Paolo Bonzini wrote:
> Il 26/05/2013 01:23, Richard Henderson ha scritto:
>> On 2013-05-24 14:37, Laszlo Ersek wrote:
>>> @@ -4813,7 +4813,11 @@ static target_ulong disas_insn(CPUX86State
>>> *env, DisasContext *s,
>>>               /* 0x66 is ignored if rex.w is set */
>>>               dflag = 2;
>>>           }
>>> -        if (!(prefixes & PREFIX_ADR)) {
>>> +        if (prefixes & PREFIX_ADR) {
>>> +            /* flip it back, 0x67 should have no effect */
>>> +            aflag ^= 1;
>>> +        }
>>> +        else {
>>>               aflag = 2;
>>>           }
>>>       }
>>>
>>
>> Agreed that there's a bug here.  I'm thinking it would be clearer to not
>> write this as yet another flip, but understand that unlike dflag, aflag
>> can only be either 1 or 2 in 64-bit mode.
>>
>> I'm thinking of something more like this:
>>
>> diff --git a/target-i386/translate.c b/target-i386/translate.c
>> index 0aeccdb..bf772aa 100644
>> --- a/target-i386/translate.c
>> +++ b/target-i386/translate.c
>> @@ -4813,9 +4813,8 @@ static target_ulong disas_insn(CPUX86State *env,
>> DisasContext *s,
>>              /* 0x66 is ignored if rex.w is set */
>>              dflag = 2;
>>          }
>> -        if (!(prefixes & PREFIX_ADR)) {
>> -            aflag = 2;
>> -        }
>> +        /* 0x67 toggles between 64-bit and 32-bit addressing.  */
>> +        aflag = (prefixes & PREFIX_ADR ? 1 : 2);
> 
> Isn't that just "aflag++"?  Needs a comment of course ("toggle between
> 32- and 64-bit, not 16- and 32-bit.").

I finally looked up in the SDM what the 0x67 (address-size override)
prefix does. Apparently,

  2.1.1 Instruction Prefixes

    [...] The address-size override prefix (67H) allows programs to
    switch between 16- and 32-bit addressing. Either size can be the
    default; the prefix selects the non-default size. [...]

  CMPS/CMPSB/CMPSW/CMPSD/CMPSQ -- Compare String Operands

    [...] In 64-bit mode, the instruction's default address size is 64
    bits, 32 bit address size is supported using the prefix 67H. [...]

Assuming that
- aflag==0 means 16-bit address,
- aflag==1 means 32-bit address,
- aflag==2 means 64-bit address,
- and bit0 in "s->code32" carries "aflag" corresponding to the default
  address size in 32-bit mode,

I think Richard's patch is correct (my approach was only to restore the
pre-patch logic without having any clue about the variables' contents).

I believe aflag++ is incorrect if the current default address size for
32-bit is 16-bit (ie. (s->code32 & 1) == 0). In this case the first XOR
(seeing the 0x67 prefix) flips it to 1, and the increment would change
it to 2. aflag==2 corresponds to 64-bit address, but in 64-bit mode with
the 0x67 prefix we must choose 32-bit.

(IOW in 32-bit mode the meaning of the 0x67 prefix is not absolute but
relative.)

Laszlo

Re: [Qemu-devel] [PATCH] i386/translate: ignore 0x67 (PREFIX_ADR) on TARGET_X86_64 && CODE64()

[Paolo Bonzini]

Il 27/05/2013 01:45, Laszlo Ersek ha scritto:
> I believe aflag++ is incorrect if the current default address size for
> 32-bit is 16-bit (ie. (s->code32 & 1) == 0).

... which cannot happen. :)

(Sorry, should have been more verbose).

See cpu_x86_load_seg_cache:

#ifdef TARGET_X86_64
            if ((env->hflags & HF_LMA_MASK) && (flags & DESC_L_MASK)) {
                /* long mode */
                env->hflags |= HF_CS32_MASK | HF_SS32_MASK | HF_CS64_MASK;
                env->hflags &= ~(HF_ADDSEG_MASK);
            } else
#endif
            {
                /* legacy / compatibility case */
                new_hflags = (env->segs[R_CS].flags & DESC_B_MASK)
                    >> (DESC_B_SHIFT - HF_CS32_SHIFT);
                env->hflags = (env->hflags & ~(HF_CS32_MASK | HF_CS64_MASK)) |
                    new_hflags;
            }

This is the only place where HF_CS64_MASK is added to env->hflags.  Then:

    dc->code64 = (flags >> HF_CS64_SHIFT) & 1;

#define CODE64(s) ((s)->code64)

Paolo

 In this case the first XOR
> (seeing the 0x67 prefix) flips it to 1, and the increment would change
> it to 2. aflag==2 corresponds to 64-bit address, but in 64-bit mode with
> the 0x67 prefix we must choose 32-bit.
> 
> (IOW in 32-bit mode the meaning of the 0x67 prefix is not absolute but
> relative.)
> 
> Laszlo
> 
>