On Thu, Oct 10, 2024 at 11:00:15AM +0200, Christophe Leroy wrote:
> Hi Thomas,
>
> Le 10/10/2024 à 10:20, Thomas Weißschuh a écrit :
> > On Wed, Oct 02, 2024 at 10:39:29AM +0200, Christophe Leroy wrote:
> > > VDSO time functions do not call any other function, so they don't
> > > need to save/restore LR. However, retrieving the address of VDSO data
> > > page requires using LR hence saving then restoring it, which can be
> > > heavy on some CPUs. On the other hand, VDSO functions on powerpc are
> > > not standard functions and require a wrapper function to call C VDSO
> > > functions. And that wrapper has to save and restore LR in order to
> > > call the C VDSO function, so retrieving VDSO data page address in that
> > > wrapper doesn't require additional save/restore of LR.
> > >
> > > For random VDSO functions it is a bit different. Because the function
> > > calls __arch_chacha20_blocks_nostack(), it saves and restores LR.
> > > Retrieving VDSO data page address can then be done there without
> > > additional save/restore of LR.
> > >
> > > So lets implement __arch_get_vdso_rng_data() and simplify the wrapper.
> > >
> > > It starts paving the way for the day powerpc will implement a more
> > > standard ABI for VDSO functions.
> > >
> > > Signed-off-by: Christophe Leroy <christophe.le...@csgroup.eu>
> > > ---
> > > arch/powerpc/include/asm/vdso/getrandom.h | 15 +++++++++++++--
> > > arch/powerpc/kernel/asm-offsets.c | 1 -
> > > arch/powerpc/kernel/vdso/getrandom.S | 1 -
> > > arch/powerpc/kernel/vdso/vgetrandom.c | 4 ++--
> > > 4 files changed, 15 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/arch/powerpc/include/asm/vdso/getrandom.h
> > > b/arch/powerpc/include/asm/vdso/getrandom.h
> > > index 501d6bb14e8a..4302e7c67aa5 100644
> > > --- a/arch/powerpc/include/asm/vdso/getrandom.h
> > > +++ b/arch/powerpc/include/asm/vdso/getrandom.h
> > > @@ -7,6 +7,8 @@
> > > #ifndef __ASSEMBLY__
> > > +#include <asm/vdso_datapage.h>
> > > +
> > > static __always_inline int do_syscall_3(const unsigned long _r0, const
> > > unsigned long _r3,
> > > const unsigned long _r4, const
> > > unsigned long _r5)
> > > {
> > > @@ -43,11 +45,20 @@ static __always_inline ssize_t getrandom_syscall(void
> > > *buffer, size_t len, unsig
> > > static __always_inline struct vdso_rng_data
> > > *__arch_get_vdso_rng_data(void)
> > > {
> > > - return NULL;
> > > + struct vdso_arch_data *data;
> > > +
> > > + asm(
> > > + " bcl 20, 31, .+4\n"
> > > + "0: mflr %0\n"
> > > + " addis %0, %0, (_vdso_datapage - 0b)@ha\n"
> > > + " addi %0, %0, (_vdso_datapage - 0b)@l\n"
> > > + : "=r" (data) :: "lr");
> > > +
> > > + return &data->rng_data;
> > > }
> >
> > Did you also try something like this:
> >
> > extern struct vdso_arch_data _vdso_datapage
> > __attribute__((visibility("hidden")));
> >
> > static __always_inline struct vdso_rng_data *__arch_get_vdso_rng_data(void)
> > {
> > return &_vdso_datapage.rng_data;
> > }
> >
> > Not knowing much about ppc asm the resulting assembly looks simpler.
> > And it would be more in line with what other archs are doing.
>
> Did you build it ?
Yes, I couldn't have looked at the asm otherwise.
> I get :
>
> VDSO32C arch/powerpc/kernel/vdso/vgetrandom-32.o
> VDSO32L arch/powerpc/kernel/vdso/vdso32.so.dbg
> arch/powerpc/kernel/vdso/vdso32.so.dbg: dynamic relocations are not
> supported
> make[2]: *** [arch/powerpc/kernel/vdso/Makefile:75:
> arch/powerpc/kernel/vdso/vdso32.so.dbg] Error 1
I forgot to enable CONFIG_COMPAT.
It's only broken for the 32 bit case.
> Current solution gives:
>
> 24: 42 9f 00 05 bcl 20,4*cr7+so,28 <__c_kernel_getrandom+0x28>
> 28: 7e a8 02 a6 mflr r21
> 2c: 3e b5 00 00 addis r21,r21,0
> 2e: R_PPC_REL16_HA _vdso_datapage+0x6
> 30: 3a b5 00 00 addi r21,r21,0
> 32: R_PPC_REL16_LO _vdso_datapage+0xa
>
>
> Your solution gives:
>
> 60: 3e e0 00 00 lis r23,0
> 62: R_PPC_ADDR16_HA _vdso_datapage
> 64: 3a f7 00 00 addi r23,r23,0
> 66: R_PPC_ADDR16_LO _vdso_datapage
>
>
> So yes your solution looks simpler, but relies on absolute addresses set up
> through dynamic relocation which is not possible because different processes
> map the same VDSO datapage at different addresses.
Due to visibility("hidden"), the compiler should not emit absolute
references but PC-relative ones.
This is how it works for most other architectures, see
include/vdso/datapage.h.
I'll try to see why this doesn't work for ppc32.
Thomas
