* [PATCH] x86_emulate fix
@ 2007-10-19 15:43 David Lively
2007-10-19 16:07 ` Keir Fraser
0 siblings, 1 reply; 4+ messages in thread
From: David Lively @ 2007-10-19 15:43 UTC (permalink / raw)
To: xen-devel
[-- Attachment #1: Type: text/plain, Size: 391 bytes --]
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 <dlively@virtualiron.com>
[-- Attachment #2: xen-emulate-eflags-clobber-fix.patch --]
[-- Type: text/x-patch, Size: 1218 bytes --]
diff -r 85791ff698bd xen/arch/x86/x86_emulate.c
--- a/xen/arch/x86/x86_emulate.c Fri Oct 19 11:31:38 2007 -0400
+++ b/xen/arch/x86/x86_emulate.c Fri Oct 19 11:31:38 2007 -0400
@@ -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) \
[-- Attachment #3: Type: text/plain, Size: 138 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] x86_emulate fix
2007-10-19 15:43 [PATCH] x86_emulate fix David Lively
@ 2007-10-19 16:07 ` Keir Fraser
2007-10-19 16:59 ` Dave Lively
0 siblings, 1 reply; 4+ messages in thread
From: Keir Fraser @ 2007-10-19 16:07 UTC (permalink / raw)
To: David Lively, 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
On 19/10/07 16:43, "David Lively" <dlively@virtualiron.com> wrote:
> 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 <dlively@virtualiron.com>
>
> diff -r 85791ff698bd xen/arch/x86/x86_emulate.c
> --- a/xen/arch/x86/x86_emulate.c Fri Oct 19 11:31:38 2007 -0400
> +++ b/xen/arch/x86/x86_emulate.c Fri Oct 19 11:31:38 2007 -0400
> @@ -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
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] x86_emulate fix
2007-10-19 16:07 ` Keir Fraser
@ 2007-10-19 16:59 ` Dave Lively
2007-10-19 17:10 ` Keir Fraser
0 siblings, 1 reply; 4+ messages in thread
From: Dave Lively @ 2007-10-19 16:59 UTC (permalink / raw)
To: Keir Fraser; +Cc: 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
On 10/19/07, Keir Fraser <Keir.Fraser@cl.cam.ac.uk> wrote:
> 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
>
> On 19/10/07 16:43, "David Lively" <dlively@virtualiron.com> wrote:
>
> > 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 <dlively@virtualiron.com>
> >
> > diff -r 85791ff698bd xen/arch/x86/x86_emulate.c
> > --- a/xen/arch/x86/x86_emulate.c Fri Oct 19 11:31:38 2007 -0400
> > +++ b/xen/arch/x86/x86_emulate.c Fri Oct 19 11:31:38 2007 -0400
> > @@ -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
> > Xen-devel@lists.xensource.com
> > http://lists.xensource.com/xen-devel
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] x86_emulate fix
2007-10-19 16:59 ` Dave Lively
@ 2007-10-19 17:10 ` Keir Fraser
0 siblings, 0 replies; 4+ messages in thread
From: Keir Fraser @ 2007-10-19 17:10 UTC (permalink / raw)
To: Dave Lively; +Cc: xen-devel
I checked in an alternative as changeset 16143.
K.
On 19/10/07 17:59, "Dave Lively" <dlively@virtualiron.com> wrote:
> 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
>
> On 10/19/07, Keir Fraser <Keir.Fraser@cl.cam.ac.uk> wrote:
>> 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
>>
>> On 19/10/07 16:43, "David Lively" <dlively@virtualiron.com> wrote:
>>
>>> 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 <dlively@virtualiron.com>
>>>
>>> diff -r 85791ff698bd xen/arch/x86/x86_emulate.c
>>> --- a/xen/arch/x86/x86_emulate.c Fri Oct 19 11:31:38 2007 -0400
>>> +++ b/xen/arch/x86/x86_emulate.c Fri Oct 19 11:31:38 2007 -0400
>>> @@ -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
>>> Xen-devel@lists.xensource.com
>>> http://lists.xensource.com/xen-devel
>>
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2007-10-19 17:10 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-10-19 15:43 [PATCH] x86_emulate fix David Lively
2007-10-19 16:07 ` Keir Fraser
2007-10-19 16:59 ` Dave Lively
2007-10-19 17:10 ` Keir Fraser
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.