Re: [PATCH v8 5/8] powerpc/vdso: Prepare for switching VDSO to generic C implementation.

[Christophe Leroy]

Michael Ellerman <mpe@xxxxxxxxxxxxxx> a ÃcritÂ:

> Christophe Leroy <christophe.leroy@xxxxxx> writes:
> > Prepare for switching VDSO to generic C implementation in following
> > patch. Here, we:
> > - Modify __get_datapage() to take an offset
> > - Prepare the helpers to call the C VDSO functions
> > - Prepare the required callbacks for the C VDSO functions
> > - Prepare the clocksource.h files to define VDSO_ARCH_CLOCKMODES
> > - Add the C trampolines to the generic C VDSO functions
> >
> > powerpc is a bit special for VDSO as well as system calls in the
> > way that it requires setting CR SO bit which cannot be done in C.
> > Therefore, entry/exit needs to be performed in ASM.
> >
> > Implementing __arch_get_vdso_data() would clobber the link register,
> > requiring the caller to save it. As the ASM calling function already
> > has to set a stack frame and saves the link register before calling
> > the C vdso function, retriving the vdso data pointer there is lighter.
> ...
>
> > diff --git a/arch/powerpc/include/asm/vdso/gettimeofday.h  
> > b/arch/powerpc/include/asm/vdso/gettimeofday.h
> > new file mode 100644
> > index 000000000000..4452897f9bd8
> > --- /dev/null
> > +++ b/arch/powerpc/include/asm/vdso/gettimeofday.h
> > @@ -0,0 +1,175 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __ASM_VDSO_GETTIMEOFDAY_H
> > +#define __ASM_VDSO_GETTIMEOFDAY_H
> > +
> > +#include <asm/ptrace.h>
> > +
> > +#ifdef __ASSEMBLY__
> > +
> > +.macro cvdso_call funct
> > +  .cfi_startproc
> > +	PPC_STLU	r1, -STACK_FRAME_OVERHEAD(r1)
> > +	mflr		r0
> > +  .cfi_register lr, r0
> > +	PPC_STL		r0, STACK_FRAME_OVERHEAD + PPC_LR_STKOFF(r1)
>
> This doesn't work for me on ppc64(le) with glibc.
>
> glibc doesn't create a stack frame before making the VDSO call, so the
> store of r0 (LR) goes into the caller's frame, corrupting the saved LR,
> leading to an infinite loop.

Where should it be saved if it can't be saved in the standard location ?

> This is an example from a statically built program that calls
> clock_gettime():
>
> 0000000010030cb0 <__clock_gettime>:
>     10030cb0:   0e 10 40 3c     lis     r2,4110
>     10030cb4:   00 7a 42 38     addi    r2,r2,31232
>     10030cb8:   a6 02 08 7c     mflr    r0
>     10030cbc:   ff ff 22 3d     addis   r9,r2,-1
>     10030cc0:   58 6d 29 39     addi    r9,r9,27992
>     10030cc4:   f0 ff c1 fb     std     r30,-16(r1)			<-- redzone store
>     10030cc8:   78 23 9e 7c     mr      r30,r4
>     10030ccc:   f8 ff e1 fb     std     r31,-8(r1)			<-- redzone store
>     10030cd0:   78 1b 7f 7c     mr      r31,r3
>     10030cd4:   10 00 01 f8     std     r0,16(r1)			<-- save LR to  
> caller's frame
>     10030cd8:   00 00 09 e8     ld      r0,0(r9)
>     10030cdc:   00 00 20 2c     cmpdi   r0,0
>     10030ce0:   50 00 82 41     beq     10030d30 <__clock_gettime+0x80>
>     10030ce4:   a6 03 09 7c     mtctr   r0
>     10030ce8:   21 04 80 4e     bctrl					<-- vdso call
>     10030cec:   26 00 00 7c     mfcr    r0
>     10030cf0:   00 10 09 74     andis.  r9,r0,4096
>     10030cf4:   78 1b 69 7c     mr      r9,r3
>     10030cf8:   28 00 82 40     bne     10030d20 <__clock_gettime+0x70>
>     10030cfc:   b4 07 23 7d     extsw   r3,r9
>     10030d00:   10 00 01 e8     ld      r0,16(r1)			<-- load saved  
> LR, since clobbered by the VDSO
>     10030d04:   f0 ff c1 eb     ld      r30,-16(r1)
>     10030d08:   f8 ff e1 eb     ld      r31,-8(r1)
>     10030d0c:   a6 03 08 7c     mtlr    r0				<-- restore LR
>     10030d10:   20 00 80 4e     blr					<-- jumps to 10030cec
>
>
> I'm kind of confused how it worked for you on 32-bit.

So am I then. I'm away for 3 weeks, summer break. I'll check when I'm back.

> There's also no code to load/restore the TOC pointer on BE, which I
> think we'll need to handle.

What does it means exactly ? Just saving r2 all the time ? Is there a  
dedicated location in the stack frame for it ? Is that only for 64 be ?

Christophe