Re: [kvm-unit-tests PATCH 2/2] x86: realmode: use inline asm to get stack pointer

[Jim Mattson <jmattson@google.com>]

On Wed, Oct 16, 2019 at 12:53 PM Jim Mattson <jmattson@google.com> wrote:
>
> On Sat, Oct 12, 2019 at 4:59 PM Bill Wendling <morbo@google.com> wrote:
> >
> > It's fragile to try to retrieve the stack pointer by taking the address
> > of a variable on the stack. For instance, clang reserves more stack
> > space than gcc here, indicating that the variable may not be at the
> > start of the stack. Instead of relying upon this to work, retrieve the
> > "%rbp" value, which contains the value of "%rsp" before stack
> > allocation.
> >
> > Signed-off-by: Bill Wendling <morbo@google.com>
> > ---
> >  x86/realmode.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/x86/realmode.c b/x86/realmode.c
> > index cf45fd6..7c89dd1 100644
> > --- a/x86/realmode.c
> > +++ b/x86/realmode.c
> > @@ -518,11 +518,12 @@ extern void retf_imm(void);
> >
> >  static void test_call(void)
> >  {
> > -       u32 esp[16];
> >         u32 addr;
> >
> >         inregs = (struct regs){ 0 };
> > -       inregs.esp = (u32)esp;
> > +
> > +       // At this point the original stack pointer is in %ebp.
> > +       asm volatile ("mov %%ebp, %0" : "=rm"(inregs.esp));
>
> I don't think we should assume the existence of frame pointers.
> Moreover, I think %esp is actually the value that should be saved
> here, regardless of how large the current frame is.

Never mind. After taking a closer look, esp[] is meant to provide
stack space for the code under test, but inregs.esp should point to
the top of this stack rather than the bottom. This is apparently a
long-standing bug, similar to the one Avi fixed for  test_long_jmp()
in commit 4aa22949 ("realmode: fix esp in long jump test").

For consistency with test_long_jmp, I'd suggest changing the
inregs.esp assignment to:
       inregs.esp = (u32)(esp+16);

Note that you absolutely must preserve the esp[] array!

> >         MK_INSN(call1, "mov $test_function, %eax \n\t"
> >                        "call *%eax\n\t");
> > --
> > 2.23.0.700.g56cf767bdb-goog
> >