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

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 :

quoted

On Wed, Oct 02, 2024 at 10:39:29AM +0200, Christophe Leroy wrote:

quoted

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