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

From: Michael Ellerman
Date: Fri Oct 11 2024 - 07:47:04 EST

Next message: Dmitry Vyukov: "Re: [PATCH v3] mm:kasan: fix sparse warnings: Should it be static?"
Previous message: Nihar Chaithanya: "[PATCH v3] mm:kasan: fix sparse warnings: Should it be static?"
In reply to: Christophe Leroy: "Re: [PATCH 2/2] powerpc/vdso: Implement __arch_get_vdso_rng_data()"
Next in thread: Christophe Leroy: "Re: [PATCH 2/2] powerpc/vdso: Implement __arch_get_vdso_rng_data()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Christophe Leroy <christophe.leroy@xxxxxxxxxx> writes:
> 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@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.
>
> PC-rel instructions only exist on very very recent powerpc CPUs (power10 ?)

Yeah power10 or later.

> 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

Setting up r12 is only required for calls to the global entry point
(offset 0), local calls can be made to offset 8 and use/don't require
r12 to be set. That's because local calls should already have the
correct toc pointer in r2.

But that's not true in VDSO code.

> 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)

The call to __c_kernel_getrandom skips over the r2 setup because it's a
local call, even though we haven't setup r2 correctly:

0000000000000758 <__kernel_getrandom>:
758: 91 ff 21 f8 stdu r1,-112(r1)
75c: a6 02 08 7c mflr r0
760: 91 ff 21 f8 stdu r1,-112(r1)
764: 80 00 01 f8 std r0,128(r1)
768: 88 00 41 f8 std r2,136(r1)
76c: 05 00 9f 42 bcl 20,4*cr7+so,770 <__kernel_getrandom+0x18>
770: a6 02 08 7d mflr r8
774: fe ff 08 3d addis r8,r8,-2
778: 90 f8 08 39 addi r8,r8,-1904
77c: fc 00 68 81 lwz r11,252(r8)
780: ff 7f 6b 6d xoris r11,r11,32767
784: ff ff 6b 69 xori r11,r11,65535
788: 34 00 6b 7d cntlzw r11,r11
78c: de 5b 6b 55 rlwinm r11,r11,11,15,15
790: 14 5a 08 7d add r8,r8,r11
794: d8 02 08 39 addi r8,r8,728
798: 41 09 00 48 bl 10d8 <__c_kernel_getrandom+0x8>

We could setup r2, but that would only help 64-bit.

This also makes me notice that we have a mixture of ELF ABI v1 and v2
code in the VDSO, depending on whether the kernel is building itself ABI
v1 or v2:

arch/powerpc/kernel/vdso/cacheflush-64.o: ELF 64-bit LSB relocatable, 64-bit PowerPC or cisco 7500, Unspecified or Power ELF V1 ABI, version 1 (SYSV), not stripped
arch/powerpc/kernel/vdso/datapage-64.o: ELF 64-bit LSB relocatable, 64-bit PowerPC or cisco 7500, Unspecified or Power ELF V1 ABI, version 1 (SYSV), not stripped
arch/powerpc/kernel/vdso/getcpu-64.o: ELF 64-bit LSB relocatable, 64-bit PowerPC or cisco 7500, Unspecified or Power ELF V1 ABI, version 1 (SYSV), not stripped
arch/powerpc/kernel/vdso/getrandom-64.o: ELF 64-bit LSB relocatable, 64-bit PowerPC or cisco 7500, Unspecified or Power ELF V1 ABI, version 1 (SYSV), not stripped
arch/powerpc/kernel/vdso/gettimeofday-64.o: ELF 64-bit LSB relocatable, 64-bit PowerPC or cisco 7500, Unspecified or Power ELF V1 ABI, version 1 (SYSV), not stripped
arch/powerpc/kernel/vdso/note-64.o: ELF 64-bit LSB relocatable, 64-bit PowerPC or cisco 7500, Unspecified or Power ELF V1 ABI, version 1 (SYSV), not stripped
arch/powerpc/kernel/vdso/sigtramp64-64.o: ELF 64-bit LSB relocatable, 64-bit PowerPC or cisco 7500, Unspecified or Power ELF V1 ABI, version 1 (SYSV), not stripped
arch/powerpc/kernel/vdso/vgetrandom-64.o: ELF 64-bit LSB relocatable, 64-bit PowerPC or cisco 7500, OpenPOWER ELF V2 ABI, version 1 (SYSV), not stripped
arch/powerpc/kernel/vdso/vgetrandom-chacha-64.o: ELF 64-bit LSB relocatable, 64-bit PowerPC or cisco 7500, Unspecified or Power ELF V1 ABI, version 1 (SYSV), not stripped
arch/powerpc/kernel/vdso/vgettimeofday-64.o: ELF 64-bit LSB relocatable, 64-bit PowerPC or cisco 7500, OpenPOWER ELF V2 ABI, version 1 (SYSV), not stripped

All the asm files are ABI v1 because they historically were, and don't
say otherwise. The C code comes out as ABI v1 or v2 depending on what
we're building the kernel as. Which is a bit fishy.

cheers

Next message: Dmitry Vyukov: "Re: [PATCH v3] mm:kasan: fix sparse warnings: Should it be static?"
Previous message: Nihar Chaithanya: "[PATCH v3] mm:kasan: fix sparse warnings: Should it be static?"
In reply to: Christophe Leroy: "Re: [PATCH 2/2] powerpc/vdso: Implement __arch_get_vdso_rng_data()"
Next in thread: Christophe Leroy: "Re: [PATCH 2/2] powerpc/vdso: Implement __arch_get_vdso_rng_data()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]