Re: VMI broken on tip/master...

[Alok Kataria <akataria@vmware.com>]

On Mon, 2009-03-16 at 15:54 -0700, Jeremy Fitzhardinge wrote:
> > I don't understand why is this not printing anything at this early fault
> > though, the system just enters the hlt_loop and stays there.
> >   
> 
> It should drop something onto the vga console (and/or serial port?).
Surprisingly it doesn't, but anyways...

> 
> >> Doing what?  It's a bit hard to tell what you're actually seeing.
> >>     
> > I did some more debugging and I think i know what the problem is 
> > The objdump for trace_clock_local looks like this
> >
> > c1070c24 <trace_clock_local>:
> > c1070c24:       55                      push   %ebp
> > c1070c25:       89 e5                   mov    %esp,%ebp
> > c1070c27:       53                      push   %ebx
> > c1070c28:       83 ec 08                sub    $0x8,%esp
> > c1070c2b:       ff 15 5c 87 3d c1       call   *0xc13d875c
> > c1070c31:       ba 64 87 3d c1          mov    $0xc13d8764,%edx
> > c1070c36:       89 45 f4                mov    %eax,0xfffffff4(%ebp)
> > c1070c39:       ff 12                   call   *(%edx)		<<=====*
> > c1070c3b:       e8 cd 68 f9 ff          call   c100750d <sched_clock>
> > c1070c40:       89 c1                   mov    %eax,%ecx
> > c1070c42:       8b 45 f4                mov    0xfffffff4(%ebp),%eax
> > c1070c45:       ff 15 60 87 3d c1       call   *0xc13d8760
> >
> > Notice instruction on c1070c39 we have "call *(edx)",
> > edx was just loaded with the address for the paravirt call.
> > when we try to replace that to a call to vmi specific function, maybe we
> > hit a BUG_ON(len < 5) in vmi's patch_internal code, because now the
> > instruction length is less than 5.
> >
> > Is there is a way to get GCC to not do such fancy tricks, and instead do
> > a direct "call 0xc13d8764"  ?
> Well, indirect "call *0xc13d875c".   But yes, its hard to see what gcc 
> thinks its getting out of doing the indirect call via %edx.  We 
> definitely don't want gcc doing such things, even if it does make sense 
> (such as calling an op multiple times, and CSEing the pointer); given 
> that we generate all this in asm anyway, we could force it.  Does this help?

Yes, This does the trick 
The disassembly for trace_clock_local is now correct, and this boots
too.

c107105c <trace_clock_local>:
c107105c:       55                      push   %ebp
c107105d:       89 e5                   mov    %esp,%ebp
c107105f:       53                      push   %ebx
c1071060:       83 ec 08                sub    $0x8,%esp
c1071063:       ff 15 7c 07 3e c1       call   *0xc13e077c
c1071069:       89 45 f4                mov    %eax,0xfffffff4(%ebp)
c107106c:       ff 15 84 07 3e c1       call   *0xc13e0784
c1071072:       e8 36 65 f9 ff          call   c10075ad <sched_clock>
c1071077:       89 c1                   mov    %eax,%ecx
c1071079:       8b 45 f4                mov    0xfffffff4(%ebp),%eax
c107107c:       ff 15 80 07 3e c1       call   *0xc13e0780

Thanks,
Alok

> 
> diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
> index d4fec1f..62dfc51 100644
> --- a/arch/x86/include/asm/paravirt.h
> +++ b/arch/x86/include/asm/paravirt.h
> @@ -395,7 +395,7 @@ extern struct pv_lock_ops pv_lock_ops;
>  
>  #define paravirt_type(op)				\
>  	[paravirt_typenum] "i" (PARAVIRT_PATCH(op)),	\
> -	[paravirt_opptr] "m" (op)
> +	[paravirt_opptr] "i" (&(op))
>  #define paravirt_clobber(clobber)		\
>  	[paravirt_clobber] "i" (clobber)
>  
> @@ -449,7 +449,7 @@ int paravirt_disable_iospace(void);
>   * offset into the paravirt_patch_template structure, and can therefore be
>   * freely converted back into a structure offset.
>   */
> -#define PARAVIRT_CALL	"call *%[paravirt_opptr];"
> +#define PARAVIRT_CALL	"call *%c[paravirt_opptr];"
>  
>  /*
>   * These macros are intended to wrap calls through one of the paravirt
> 
>