From: Michael Ellerman <mpe@ellerman.id.au> To: Christophe Leroy <christophe.leroy@c-s.fr>, Benjamin Herrenschmidt <benh@kernel.crashing.org>, Paul Mackerras <paulus@samba.org>, nathanl@linux.ibm.com Cc: linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, arnd@arndb.de, tglx@linutronix.de, vincenzo.frascino@arm.com, luto@kernel.org, linux-arch@vger.kernel.org, Tulio Magno Quites Machado Filho <tuliom@linux.ibm.com> Subject: Re: [PATCH v8 5/8] powerpc/vdso: Prepare for switching VDSO to generic C implementation. Date: Wed, 15 Jul 2020 11:04:26 +1000 [thread overview] Message-ID: <878sflvbad.fsf@mpe.ellerman.id.au> (raw) In-Reply-To: <2a67c333893454868bbfda773ba4b01c20272a5d.1588079622.git.christophe.leroy@c-s.fr> Christophe Leroy <christophe.leroy@c-s.fr> 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. 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. There's also no code to load/restore the TOC pointer on BE, which I think we'll need to handle. cheers
WARNING: multiple messages have this Message-ID (diff)
From: Michael Ellerman <mpe@ellerman.id.au> To: Christophe Leroy <christophe.leroy@c-s.fr>, Benjamin Herrenschmidt <benh@kernel.crashing.org>, Paul Mackerras <paulus@samba.org>, nathanl@linux.ibm.com Cc: linux-arch@vger.kernel.org, arnd@arndb.de, linux-kernel@vger.kernel.org, Tulio Magno Quites Machado Filho <tuliom@linux.ibm.com>, luto@kernel.org, tglx@linutronix.de, vincenzo.frascino@arm.com, linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH v8 5/8] powerpc/vdso: Prepare for switching VDSO to generic C implementation. Date: Wed, 15 Jul 2020 11:04:26 +1000 [thread overview] Message-ID: <878sflvbad.fsf@mpe.ellerman.id.au> (raw) In-Reply-To: <2a67c333893454868bbfda773ba4b01c20272a5d.1588079622.git.christophe.leroy@c-s.fr> Christophe Leroy <christophe.leroy@c-s.fr> 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. 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. There's also no code to load/restore the TOC pointer on BE, which I think we'll need to handle. cheers
next prev parent reply other threads:[~2020-07-15 1:04 UTC|newest] Thread overview: 71+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-04-28 13:16 [PATCH v8 0/8] powerpc: switch VDSO to C implementation Christophe Leroy 2020-04-28 13:16 ` Christophe Leroy 2020-04-28 13:16 ` [PATCH v8 1/8] powerpc/vdso64: Switch from __get_datapage() to get_datapage inline macro Christophe Leroy 2020-04-28 13:16 ` Christophe Leroy 2020-04-28 13:16 ` [PATCH v8 2/8] powerpc/vdso: Remove __kernel_datapage_offset and simplify __get_datapage() Christophe Leroy 2020-04-28 13:16 ` Christophe Leroy 2020-07-16 2:59 ` Michael Ellerman 2020-07-16 2:59 ` Michael Ellerman 2020-08-04 11:17 ` Christophe Leroy 2020-08-04 11:17 ` Christophe Leroy 2020-08-25 14:15 ` Christophe Leroy 2020-08-26 13:58 ` Michael Ellerman 2020-08-26 13:58 ` Michael Ellerman 2020-08-27 20:34 ` Dmitry Safonov 2020-08-27 20:34 ` Dmitry Safonov 2020-08-28 2:14 ` Michael Ellerman 2020-08-28 2:14 ` Michael Ellerman 2020-09-21 11:26 ` Will Deacon 2020-09-21 11:26 ` Will Deacon 2020-09-27 7:43 ` Christophe Leroy 2020-09-27 7:43 ` Christophe Leroy 2020-09-28 15:08 ` Dmitry Safonov 2020-09-28 15:08 ` Dmitry Safonov 2020-10-23 11:22 ` Christophe Leroy 2020-10-23 11:22 ` Christophe Leroy 2020-10-23 11:25 ` Will Deacon 2020-10-23 11:25 ` Will Deacon 2020-10-23 11:57 ` Christophe Leroy 2020-10-23 11:57 ` Christophe Leroy 2020-10-23 13:29 ` Dmitry Safonov 2020-10-23 13:29 ` Dmitry Safonov 2020-04-28 13:16 ` [PATCH v8 3/8] powerpc/vdso: Remove unused \tmp param in __get_datapage() Christophe Leroy 2020-04-28 13:16 ` Christophe Leroy 2020-04-28 13:16 ` [PATCH v8 4/8] powerpc/processor: Move cpu_relax() into asm/vdso/processor.h Christophe Leroy 2020-04-28 13:16 ` Christophe Leroy 2020-04-28 13:16 ` [PATCH v8 5/8] powerpc/vdso: Prepare for switching VDSO to generic C implementation Christophe Leroy 2020-04-28 13:16 ` Christophe Leroy 2020-07-15 1:04 ` Michael Ellerman [this message] 2020-07-15 1:04 ` Michael Ellerman 2020-07-15 18:47 ` Christophe Leroy 2020-07-15 18:47 ` Christophe Leroy 2020-07-16 23:18 ` Tulio Magno Quites Machado Filho 2020-07-16 23:18 ` Tulio Magno Quites Machado Filho 2020-08-04 11:14 ` Christophe Leroy 2020-08-04 11:14 ` Christophe Leroy 2020-08-05 6:24 ` Michael Ellerman 2020-08-05 6:24 ` Michael Ellerman 2020-08-05 13:35 ` Segher Boessenkool 2020-08-05 13:35 ` Segher Boessenkool 2020-08-06 2:03 ` Michael Ellerman 2020-08-06 2:03 ` Michael Ellerman 2020-08-06 18:33 ` Segher Boessenkool 2020-08-06 18:33 ` Segher Boessenkool 2020-08-07 2:44 ` Michael Ellerman 2020-08-07 2:44 ` Michael Ellerman 2020-04-28 13:16 ` [PATCH v8 6/8] powerpc/vdso: Switch " Christophe Leroy 2020-04-28 13:16 ` Christophe Leroy 2020-04-28 13:16 ` [PATCH v8 7/8] lib/vdso: force inlining of __cvdso_clock_gettime_common() Christophe Leroy 2020-04-28 13:16 ` Christophe Leroy 2020-04-28 13:16 ` [PATCH v8 8/8] powerpc/vdso: Provide __kernel_clock_gettime64() on vdso32 Christophe Leroy 2020-04-28 13:16 ` Christophe Leroy 2020-04-28 15:03 ` Christophe Leroy 2020-04-28 16:05 ` Arnd Bergmann 2020-04-28 16:05 ` Arnd Bergmann 2020-05-09 15:54 ` Christophe Leroy 2020-05-09 15:54 ` Christophe Leroy 2020-05-09 18:48 ` Christophe Leroy 2020-05-29 18:56 ` [PATCH v8 0/8] powerpc: switch VDSO to C implementation Christophe Leroy 2020-06-03 10:04 ` Michael Ellerman 2020-07-16 12:55 ` Michael Ellerman 2020-07-16 12:55 ` Michael Ellerman
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=878sflvbad.fsf@mpe.ellerman.id.au \ --to=mpe@ellerman.id.au \ --cc=arnd@arndb.de \ --cc=benh@kernel.crashing.org \ --cc=christophe.leroy@c-s.fr \ --cc=linux-arch@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=luto@kernel.org \ --cc=nathanl@linux.ibm.com \ --cc=paulus@samba.org \ --cc=tglx@linutronix.de \ --cc=tuliom@linux.ibm.com \ --cc=vincenzo.frascino@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.