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

[Thomas Weißschuh]

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.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 ?

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