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> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com 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 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
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 >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/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 >>_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel