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

Thu Oct 10 20:28:16 AEDT 2024

Le 10/10/2024 à 11:12, Thomas Weißschuh a écrit :
> 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 at 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.

PC-rel instructions only exist on very very recent powerpc CPUs (power10 ?)

On PPC64, ELF ABI v2 requires caller to put called function address in 
r12 and it looks like GCC uses that:

0000000000000000 <__c_kernel_getrandom>:
    0:	3c 4c 00 00 	addis   r2,r12,0
			2: R_PPC64_REL16_HA	.TOC.+0x2
    4:	38 42 00 00 	addi    r2,r2,0
			6: R_PPC64_REL16_LO	.TOC.+0x6
...
   64:	3d 22 00 00 	addis   r9,r2,0
			66: R_PPC64_TOC16_HA	_vdso_datapage+0x100
   68:	89 29 00 00 	lbz     r9,0(r9)
			6a: R_PPC64_TOC16_LO	_vdso_datapage+0x100

Which after final link results in:

0000000000001060 <__c_kernel_getrandom>:
     1060:	3c 4c 00 01 	addis   r2,r12,1
     1064:	38 42 8e a0 	addi    r2,r2,-29024
...
     10c4:	3d 22 ff fc 	addis   r9,r2,-4
     10c8:	89 29 62 00 	lbz     r9,25088(r9)

We don't have such a convention in the 32 bits ABI and GCC doesn't seem 
to generate necessary code for it unless you use -fPIC. But fPIC is 
another story.

Christophe