* [Qemu-devel] [PATCH] i386/translate: ignore 0x67 (PREFIX_ADR) on TARGET_X86_64 && CODE64()
@ 2013-05-24 21:37 Laszlo Ersek
2013-05-25 23:23 ` Richard Henderson
0 siblings, 1 reply; 5+ messages in thread
From: Laszlo Ersek @ 2013-05-24 21:37 UTC (permalink / raw)
To: Richard Henderson, qemu-devel
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] i386/translate: ignore 0x67 (PREFIX_ADR) on TARGET_X86_64 && CODE64()
2013-05-24 21:37 [Qemu-devel] [PATCH] i386/translate: ignore 0x67 (PREFIX_ADR) on TARGET_X86_64 && CODE64() Laszlo Ersek
@ 2013-05-25 23:23 ` Richard Henderson
2013-05-26 8:33 ` Paolo Bonzini
0 siblings, 1 reply; 5+ messages in thread
From: Richard Henderson @ 2013-05-25 23:23 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: qemu-devel
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] i386/translate: ignore 0x67 (PREFIX_ADR) on TARGET_X86_64 && CODE64()
2013-05-25 23:23 ` Richard Henderson
@ 2013-05-26 8:33 ` Paolo Bonzini
2013-05-26 23:45 ` Laszlo Ersek
0 siblings, 1 reply; 5+ messages in thread
From: Paolo Bonzini @ 2013-05-26 8:33 UTC (permalink / raw)
To: Richard Henderson; +Cc: Laszlo Ersek, qemu-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] i386/translate: ignore 0x67 (PREFIX_ADR) on TARGET_X86_64 && CODE64()
2013-05-26 8:33 ` Paolo Bonzini
@ 2013-05-26 23:45 ` Laszlo Ersek
2013-05-27 6:29 ` Paolo Bonzini
0 siblings, 1 reply; 5+ messages in thread
From: Laszlo Ersek @ 2013-05-26 23:45 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, Richard Henderson
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] i386/translate: ignore 0x67 (PREFIX_ADR) on TARGET_X86_64 && CODE64()
2013-05-26 23:45 ` Laszlo Ersek
@ 2013-05-27 6:29 ` Paolo Bonzini
0 siblings, 0 replies; 5+ messages in thread
From: Paolo Bonzini @ 2013-05-27 6:29 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: qemu-devel, Richard Henderson
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
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-05-27 6:29 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-05-24 21:37 [Qemu-devel] [PATCH] i386/translate: ignore 0x67 (PREFIX_ADR) on TARGET_X86_64 && CODE64() Laszlo Ersek
2013-05-25 23:23 ` Richard Henderson
2013-05-26 8:33 ` Paolo Bonzini
2013-05-26 23:45 ` Laszlo Ersek
2013-05-27 6:29 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).