Re: [PATCH 2/2] powerpc/vdso: Implement __arch_get_vdso_rng_data()

[Christophe Leroy]

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.leroy@xxxxxxxxxx>
> > ---
> >   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 ?

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

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.

Christophe