x86_emulate fix

The x86_emulate code uses the _PRE_EFLAGS macro to setup eflags immediately before executing (an emulated version of) the instruction. But _PRE_EFLAGS ends in a "andl" instruction, which clobbers the real eflags we've just carefully set up. This fix simply leaves the new eflags value on the stack until the final "popf" into eflags.

Signed-off-by David Lively <[email protected]>

diff -r 85791ff698bd xen/arch/x86/x86_emulate.c

xen/arch/x86/x86_emulate.c Fri Oct 19 11:31:38 2007 -0400 +4 -3

@@ -300,7 +300,7 @@ struct operand {
 
 /* Before executing instruction: restore necessary bits in EFLAGS. */
 #define _PRE_EFLAGS(_sav, _msk, _tmp)           \
-/* EFLAGS = (_sav & _msk) | (EFLAGS & ~_msk); */\
+/* push (_sav & _msk) | (EFLAGS & ~_msk); */\
 "push %"_sav"; "                                \
 "movl %"_msk",%"_LO32 _tmp"; "                  \
 "andl %"_LO32 _tmp",("_STK"); "                 \

@@ -309,11 +309,12 @@ struct operand {
 "andl %"_LO32 _tmp",("_STK"); "                 \
 "pop  %"_tmp"; "                                \
 "orl  %"_LO32 _tmp",("_STK"); "                 \
-"popf; "                                        \
 /* _sav &= ~msk; */                             \
 "movl %"_msk",%"_LO32 _tmp"; "                  \
 "notl %"_LO32 _tmp"; "                          \
-"andl %"_LO32 _tmp",%"_sav"; "
+"andl %"_LO32 _tmp",%"_sav"; "          \
+/* pop EFLAGS */    \
+"popf; "
 
 /* After executing instruction: write-back necessary bits in EFLAGS. */
 #define _POST_EFLAGS(_sav, _msk, _tmp)          \

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Good point! Unfortunately the patch is also subtly wrong. We cannot access the _sav argument while we have unpopped items on the stack. This is because _sav is a memory argument referencing a local variable (hence is on-stack). Hence gcc will probably emit a stack-pointer-relative effective address, which will be incorrect because the stack pointer is different from what gcc expects.

I'll have a think about how to fix this one.

-- Keir

Quoted 35 lines · David Lively

The x86_emulate code uses the _PRE_EFLAGS macro to setup eflags immediately before executing (an emulated version of) the instruction. But _PRE_EFLAGS ends in a "andl" instruction, which clobbers the real eflags we've just carefully set up. This fix simply leaves the new eflags value on the stack until the final "popf" into eflags.

Signed-off-by David Lively <[email protected]>

diff -r 85791ff698bd xen/arch/x86/x86_emulate.c

xen/arch/x86/x86_emulate.c Fri Oct 19 11:31:38 2007 -0400 +4 -3

@@ -300,7 +300,7 @@ struct operand {
 
 /* Before executing instruction: restore necessary bits in EFLAGS. */
 #define _PRE_EFLAGS(_sav, _msk, _tmp)           \
-/* EFLAGS = (_sav & _msk) | (EFLAGS & ~_msk); */\
+/* push (_sav & _msk) | (EFLAGS & ~_msk); */\
 "push %"_sav"; "                                \
 "movl %"_msk",%"_LO32 _tmp"; "                  \
 "andl %"_LO32 _tmp",("_STK"); "                 \

@@ -309,11 +309,12 @@ struct operand {
 "andl %"_LO32 _tmp",("_STK"); "                 \
 "pop  %"_tmp"; "                                \
 "orl  %"_LO32 _tmp",("_STK"); "                 \
-"popf; "                                        \
 /* _sav &= ~msk; */                             \
 "movl %"_msk",%"_LO32 _tmp"; "                  \
 "notl %"_LO32 _tmp"; "                          \
-"andl %"_LO32 _tmp",%"_sav"; "
+"andl %"_LO32 _tmp",%"_sav"; "          \
+/* pop EFLAGS */    \
+"popf; "
 
 /* After executing instruction: write-back necessary bits in EFLAGS. */
 #define _POST_EFLAGS(_sav, _msk, _tmp)          \

Quoted 4 lines

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

You're right. We don't currently see this behavior since we build with frame pointers, so those local variable references are %bp-relative instead of %sp-relative. (But -fomit-frame-pointers is the default, and this definitely won't work there.)

it seems fairly easy to fix with another stack temp or (better) register. But it would be nice to find a solution that doesn't require another temp. (I'm looking too ...)

Dave

Quoted 60 lines · Keir Fraser

Good point! Unfortunately the patch is also subtly wrong. We cannot access the _sav argument while we have unpopped items on the stack. This is because _sav is a memory argument referencing a local variable (hence is on-stack). Hence gcc will probably emit a stack-pointer-relative effective address, which will be incorrect because the stack pointer is different from what gcc expects.

I'll have a think about how to fix this one.

-- Keir

Quoted 35 lines · David Lively

The x86_emulate code uses the _PRE_EFLAGS macro to setup eflags immediately before executing (an emulated version of) the instruction. But _PRE_EFLAGS ends in a "andl" instruction, which clobbers the real eflags we've just carefully set up. This fix simply leaves the new eflags value on the stack until the final "popf" into eflags.

Signed-off-by David Lively <[email protected]>

diff -r 85791ff698bd xen/arch/x86/x86_emulate.c

xen/arch/x86/x86_emulate.c Fri Oct 19 11:31:38 2007 -0400 +4 -3

@@ -300,7 +300,7 @@ struct operand {
 
 /* Before executing instruction: restore necessary bits in EFLAGS. */
 #define _PRE_EFLAGS(_sav, _msk, _tmp)           \
-/* EFLAGS = (_sav & _msk) | (EFLAGS & ~_msk); */\
+/* push (_sav & _msk) | (EFLAGS & ~_msk); */\
 "push %"_sav"; "                                \
 "movl %"_msk",%"_LO32 _tmp"; "                  \
 "andl %"_LO32 _tmp",("_STK"); "                 \

@@ -309,11 +309,12 @@ struct operand {
 "andl %"_LO32 _tmp",("_STK"); "                 \
 "pop  %"_tmp"; "                                \
 "orl  %"_LO32 _tmp",("_STK"); "                 \
-"popf; "                                        \
 /* _sav &= ~msk; */                             \
 "movl %"_msk",%"_LO32 _tmp"; "                  \
 "notl %"_LO32 _tmp"; "                          \
-"andl %"_LO32 _tmp",%"_sav"; "
+"andl %"_LO32 _tmp",%"_sav"; "          \
+/* pop EFLAGS */    \
+"popf; "
 
 /* After executing instruction: write-back necessary bits in EFLAGS. */
 #define _POST_EFLAGS(_sav, _msk, _tmp)          \

Quoted 4 lines

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel