[PATCH 1/2] powerpc/vdso: Add a page for non-time data

[PATCH 1/2] powerpc/vdso: Add a page for non-time data

[Christophe Leroy]

The page containing VDSO time data is swapped with the one containing
TIME namespace data when a process uses a non-root time namespace.
For other data like powerpc specific data and RNG data, it means
tracking whether time namespace is the root one or not to know which
page to use.

Simplify the logic behind by moving time data out of first data page
so that the first data page which contains everything else always
remains the first page. Time data is in the second or third page
depending on selected time namespace.

While we are playing with get_datapage macro, directly take into
account the data offset inside the macro instead of adding that offset
afterwards.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/include/asm/vdso_datapage.h | 24 +++++++-----------------
 arch/powerpc/kernel/vdso.c               | 16 ++++++++++------
 arch/powerpc/kernel/vdso/cacheflush.S    |  2 +-
 arch/powerpc/kernel/vdso/datapage.S      |  4 ++--
 arch/powerpc/kernel/vdso/getrandom.S     |  3 +--
 arch/powerpc/kernel/vdso/gettimeofday.S  |  5 ++---
 arch/powerpc/kernel/vdso/vdso32.lds.S    |  2 +-
 arch/powerpc/kernel/vdso/vdso64.lds.S    |  2 +-
 8 files changed, 25 insertions(+), 33 deletions(-)

diff --git a/arch/powerpc/include/asm/vdso_datapage.h b/arch/powerpc/include/asm/vdso_datapage.h
index 248dee138f7b..33069afccb3c 100644
--- a/arch/powerpc/include/asm/vdso_datapage.h
+++ b/arch/powerpc/include/asm/vdso_datapage.h
@@ -82,8 +82,9 @@ struct vdso_arch_data {
 	__u32 syscall_map[SYSCALL_MAP_SIZE];	/* Map of syscalls  */
 	__u32 compat_syscall_map[SYSCALL_MAP_SIZE];	/* Map of compat syscalls */
 
-	struct vdso_data data[CS_BASES];
 	struct vdso_rng_data rng_data;
+
+	struct vdso_data data[CS_BASES] __aligned(1 << CONFIG_PAGE_SHIFT);
 };
 
 #else /* CONFIG_PPC64 */
@@ -95,8 +96,9 @@ struct vdso_arch_data {
 	__u64 tb_ticks_per_sec;		/* Timebase tics / sec		0x38 */
 	__u32 syscall_map[SYSCALL_MAP_SIZE]; /* Map of syscalls */
 	__u32 compat_syscall_map[0];	/* No compat syscalls on PPC32 */
-	struct vdso_data data[CS_BASES];
 	struct vdso_rng_data rng_data;
+
+	struct vdso_data data[CS_BASES] __aligned(1 << CONFIG_PAGE_SHIFT);
 };
 
 #endif /* CONFIG_PPC64 */
@@ -105,29 +107,17 @@ extern struct vdso_arch_data *vdso_data;
 
 #else /* __ASSEMBLY__ */
 
-.macro get_datapage ptr
+.macro get_datapage ptr offset=0
 	bcl	20, 31, .+4
 999:
 	mflr	\ptr
-	addis	\ptr, \ptr, (_vdso_datapage - 999b)@ha
-	addi	\ptr, \ptr, (_vdso_datapage - 999b)@l
+	addis	\ptr, \ptr, (_vdso_datapage - 999b + \offset)@ha
+	addi	\ptr, \ptr, (_vdso_datapage - 999b + \offset)@l
 .endm
 
 #include <asm/asm-offsets.h>
 #include <asm/page.h>
 
-.macro get_realdatapage ptr scratch
-	get_datapage \ptr
-#ifdef CONFIG_TIME_NS
-	lwz	\scratch, VDSO_CLOCKMODE_OFFSET(\ptr)
-	xoris	\scratch, \scratch, VDSO_CLOCKMODE_TIMENS@h
-	xori	\scratch, \scratch, VDSO_CLOCKMODE_TIMENS@l
-	cntlzw	\scratch, \scratch
-	rlwinm	\scratch, \scratch, PAGE_SHIFT - 5, 1 << PAGE_SHIFT
-	add	\ptr, \ptr, \scratch
-#endif
-.endm
-
 #endif /* __ASSEMBLY__ */
 
 #endif /* __KERNEL__ */
diff --git a/arch/powerpc/kernel/vdso.c b/arch/powerpc/kernel/vdso.c
index ee4b9d676cff..6166c1862c06 100644
--- a/arch/powerpc/kernel/vdso.c
+++ b/arch/powerpc/kernel/vdso.c
@@ -48,12 +48,13 @@ long sys_ni_syscall(void);
  */
 static union {
 	struct vdso_arch_data	data;
-	u8			page[PAGE_SIZE];
+	u8			page[2 * PAGE_SIZE];
 } vdso_data_store __page_aligned_data;
 struct vdso_arch_data *vdso_data = &vdso_data_store.data;
 
 enum vvar_pages {
-	VVAR_DATA_PAGE_OFFSET,
+	VVAR_BASE_PAGE_OFFSET,
+	VVAR_TIME_PAGE_OFFSET,
 	VVAR_TIMENS_PAGE_OFFSET,
 	VVAR_NR_PAGES,
 };
@@ -119,7 +120,7 @@ static struct vm_special_mapping vdso64_spec __ro_after_init = {
 #ifdef CONFIG_TIME_NS
 struct vdso_data *arch_get_vdso_data(void *vvar_page)
 {
-	return ((struct vdso_arch_data *)vvar_page)->data;
+	return vvar_page;
 }
 
 /*
@@ -153,11 +154,14 @@ static vm_fault_t vvar_fault(const struct vm_special_mapping *sm,
 	unsigned long pfn;
 
 	switch (vmf->pgoff) {
-	case VVAR_DATA_PAGE_OFFSET:
+	case VVAR_BASE_PAGE_OFFSET:
+		pfn = virt_to_pfn(vdso_data);
+		break;
+	case VVAR_TIME_PAGE_OFFSET:
 		if (timens_page)
 			pfn = page_to_pfn(timens_page);
 		else
-			pfn = virt_to_pfn(vdso_data);
+			pfn = virt_to_pfn(vdso_data->data);
 		break;
 #ifdef CONFIG_TIME_NS
 	case VVAR_TIMENS_PAGE_OFFSET:
@@ -170,7 +174,7 @@ static vm_fault_t vvar_fault(const struct vm_special_mapping *sm,
 		 */
 		if (!timens_page)
 			return VM_FAULT_SIGBUS;
-		pfn = virt_to_pfn(vdso_data);
+		pfn = virt_to_pfn(vdso_data->data);
 		break;
 #endif /* CONFIG_TIME_NS */
 	default:
diff --git a/arch/powerpc/kernel/vdso/cacheflush.S b/arch/powerpc/kernel/vdso/cacheflush.S
index 3b2479bd2f9a..0085ae464dac 100644
--- a/arch/powerpc/kernel/vdso/cacheflush.S
+++ b/arch/powerpc/kernel/vdso/cacheflush.S
@@ -30,7 +30,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
 #ifdef CONFIG_PPC64
 	mflr	r12
   .cfi_register lr,r12
-	get_realdatapage	r10, r11
+	get_datapage	r10
 	mtlr	r12
   .cfi_restore	lr
 #endif
diff --git a/arch/powerpc/kernel/vdso/datapage.S b/arch/powerpc/kernel/vdso/datapage.S
index 2b19b6201a33..db8e167f0166 100644
--- a/arch/powerpc/kernel/vdso/datapage.S
+++ b/arch/powerpc/kernel/vdso/datapage.S
@@ -28,7 +28,7 @@ V_FUNCTION_BEGIN(__kernel_get_syscall_map)
 	mflr	r12
   .cfi_register lr,r12
 	mr.	r4,r3
-	get_realdatapage	r3, r11
+	get_datapage	r3
 	mtlr	r12
 #ifdef __powerpc64__
 	addi	r3,r3,CFG_SYSCALL_MAP64
@@ -52,7 +52,7 @@ V_FUNCTION_BEGIN(__kernel_get_tbfreq)
   .cfi_startproc
 	mflr	r12
   .cfi_register lr,r12
-	get_realdatapage	r3, r11
+	get_datapage	r3
 #ifndef __powerpc64__
 	lwz	r4,(CFG_TB_TICKS_PER_SEC + 4)(r3)
 #endif
diff --git a/arch/powerpc/kernel/vdso/getrandom.S b/arch/powerpc/kernel/vdso/getrandom.S
index f3bbf931931c..3deddcf89f99 100644
--- a/arch/powerpc/kernel/vdso/getrandom.S
+++ b/arch/powerpc/kernel/vdso/getrandom.S
@@ -31,8 +31,7 @@
 	PPC_STL		r2, PPC_MIN_STKFRM + STK_GOT(r1)
   .cfi_rel_offset r2, PPC_MIN_STKFRM + STK_GOT
 #endif
-	get_realdatapage	r8, r11
-	addi		r8, r8, VDSO_RNG_DATA_OFFSET
+	get_datapage	r8 VDSO_RNG_DATA_OFFSET
 	bl		CFUNC(DOTSYM(\funct))
 	PPC_LL		r0, PPC_MIN_STKFRM + PPC_LR_STKOFF(r1)
 #ifdef __powerpc64__
diff --git a/arch/powerpc/kernel/vdso/gettimeofday.S b/arch/powerpc/kernel/vdso/gettimeofday.S
index 5540d7021fa2..5333848322ca 100644
--- a/arch/powerpc/kernel/vdso/gettimeofday.S
+++ b/arch/powerpc/kernel/vdso/gettimeofday.S
@@ -32,11 +32,10 @@
 	PPC_STL		r2, PPC_MIN_STKFRM + STK_GOT(r1)
   .cfi_rel_offset r2, PPC_MIN_STKFRM + STK_GOT
 #endif
-	get_datapage	r5
 	.ifeq	\call_time
-	addi		r5, r5, VDSO_DATA_OFFSET
+		get_datapage	r5 VDSO_DATA_OFFSET
 	.else
-	addi		r4, r5, VDSO_DATA_OFFSET
+		get_datapage	r4 VDSO_DATA_OFFSET
 	.endif
 	bl		CFUNC(DOTSYM(\funct))
 	PPC_LL		r0, PPC_MIN_STKFRM + PPC_LR_STKOFF(r1)
diff --git a/arch/powerpc/kernel/vdso/vdso32.lds.S b/arch/powerpc/kernel/vdso/vdso32.lds.S
index 7b41d5d256e8..1a1b0b6d681a 100644
--- a/arch/powerpc/kernel/vdso/vdso32.lds.S
+++ b/arch/powerpc/kernel/vdso/vdso32.lds.S
@@ -16,7 +16,7 @@ OUTPUT_ARCH(powerpc:common)
 
 SECTIONS
 {
-	PROVIDE(_vdso_datapage = . - 2 * PAGE_SIZE);
+	PROVIDE(_vdso_datapage = . - 3 * PAGE_SIZE);
 	. = SIZEOF_HEADERS;
 
 	.hash          	: { *(.hash) }			:text
diff --git a/arch/powerpc/kernel/vdso/vdso64.lds.S b/arch/powerpc/kernel/vdso/vdso64.lds.S
index 9481e4b892ed..e21b5506cad6 100644
--- a/arch/powerpc/kernel/vdso/vdso64.lds.S
+++ b/arch/powerpc/kernel/vdso/vdso64.lds.S
@@ -16,7 +16,7 @@ OUTPUT_ARCH(powerpc:common64)
 
 SECTIONS
 {
-	PROVIDE(_vdso_datapage = . - 2 * PAGE_SIZE);
+	PROVIDE(_vdso_datapage = . - 3 * PAGE_SIZE);
 	. = SIZEOF_HEADERS;
 
 	.hash		: { *(.hash) }			:text
-- 
2.44.0

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

[Christophe Leroy]

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@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;
 }
 
 ssize_t __c_kernel_getrandom(void *buffer, size_t len, unsigned int flags, void *opaque_state,
-			     size_t opaque_len, const struct vdso_rng_data *vd);
+			     size_t opaque_len);
 
 #endif /* !__ASSEMBLY__ */
 
diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index 131a8cc10dbe..7b3feb6bc210 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -335,7 +335,6 @@ int main(void)
 
 	/* datapage offsets for use by vdso */
 	OFFSET(VDSO_DATA_OFFSET, vdso_arch_data, data);
-	OFFSET(VDSO_RNG_DATA_OFFSET, vdso_arch_data, rng_data);
 	OFFSET(CFG_TB_TICKS_PER_SEC, vdso_arch_data, tb_ticks_per_sec);
 #ifdef CONFIG_PPC64
 	OFFSET(CFG_ICACHE_BLOCKSZ, vdso_arch_data, icache_block_size);
diff --git a/arch/powerpc/kernel/vdso/getrandom.S b/arch/powerpc/kernel/vdso/getrandom.S
index 3deddcf89f99..a80d9fb436f7 100644
--- a/arch/powerpc/kernel/vdso/getrandom.S
+++ b/arch/powerpc/kernel/vdso/getrandom.S
@@ -31,7 +31,6 @@
 	PPC_STL		r2, PPC_MIN_STKFRM + STK_GOT(r1)
   .cfi_rel_offset r2, PPC_MIN_STKFRM + STK_GOT
 #endif
-	get_datapage	r8 VDSO_RNG_DATA_OFFSET
 	bl		CFUNC(DOTSYM(\funct))
 	PPC_LL		r0, PPC_MIN_STKFRM + PPC_LR_STKOFF(r1)
 #ifdef __powerpc64__
diff --git a/arch/powerpc/kernel/vdso/vgetrandom.c b/arch/powerpc/kernel/vdso/vgetrandom.c
index 5f855d45fb7b..cc79b960a541 100644
--- a/arch/powerpc/kernel/vdso/vgetrandom.c
+++ b/arch/powerpc/kernel/vdso/vgetrandom.c
@@ -8,7 +8,7 @@
 #include <linux/types.h>
 
 ssize_t __c_kernel_getrandom(void *buffer, size_t len, unsigned int flags, void *opaque_state,
-			     size_t opaque_len, const struct vdso_rng_data *vd)
+			     size_t opaque_len)
 {
-	return __cvdso_getrandom_data(vd, buffer, len, flags, opaque_state, opaque_len);
+	return __cvdso_getrandom(buffer, len, flags, opaque_state, opaque_len);
 }
-- 
2.44.0

Re: [PATCH 1/2] powerpc/vdso: Add a page for non-time data

[Thomas Weißschuh]

Hi Christophe,

On Wed, Oct 02, 2024 at 10:39:28AM GMT, Christophe Leroy wrote:
> The page containing VDSO time data is swapped with the one containing
> TIME namespace data when a process uses a non-root time namespace.
> For other data like powerpc specific data and RNG data, it means
> tracking whether time namespace is the root one or not to know which
> page to use.
> 
> Simplify the logic behind by moving time data out of first data page
> so that the first data page which contains everything else always
> remains the first page. Time data is in the second or third page
> depending on selected time namespace.
> 
> While we are playing with get_datapage macro, directly take into
> account the data offset inside the macro instead of adding that offset
> afterwards.

FYI

I am currently working on a series to unify the storage of the
VDSO data for most architectures, including powerpc.
This will also include a dedicated rng data page.

That generic infrastructure would replace the need for Patch 1.
Obviously, if your series gets applied, I can adapt mine for that.

If you are about to also modify other architectures in a similar way,
we may want to coordinate.

> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  arch/powerpc/include/asm/vdso_datapage.h | 24 +++++++-----------------
>  arch/powerpc/kernel/vdso.c               | 16 ++++++++++------
>  arch/powerpc/kernel/vdso/cacheflush.S    |  2 +-
>  arch/powerpc/kernel/vdso/datapage.S      |  4 ++--
>  arch/powerpc/kernel/vdso/getrandom.S     |  3 +--
>  arch/powerpc/kernel/vdso/gettimeofday.S  |  5 ++---
>  arch/powerpc/kernel/vdso/vdso32.lds.S    |  2 +-
>  arch/powerpc/kernel/vdso/vdso64.lds.S    |  2 +-
>  8 files changed, 25 insertions(+), 33 deletions(-)


Thomas

Re: [PATCH 1/2] powerpc/vdso: Add a page for non-time data

[Christophe Leroy]

Le 02/10/2024 à 10:54, Thomas Weißschuh a écrit :
> [Vous ne recevez pas souvent de courriers de thomas.weissschuh@linutronix.de. D?couvrez pourquoi ceci est important ? https://aka.ms/LearnAboutSenderIdentification ]
> 
> Hi Christophe,
> 
> On Wed, Oct 02, 2024 at 10:39:28AM GMT, Christophe Leroy wrote:
>> The page containing VDSO time data is swapped with the one containing
>> TIME namespace data when a process uses a non-root time namespace.
>> For other data like powerpc specific data and RNG data, it means
>> tracking whether time namespace is the root one or not to know which
>> page to use.
>>
>> Simplify the logic behind by moving time data out of first data page
>> so that the first data page which contains everything else always
>> remains the first page. Time data is in the second or third page
>> depending on selected time namespace.
>>
>> While we are playing with get_datapage macro, directly take into
>> account the data offset inside the macro instead of adding that offset
>> afterwards.
> 
> FYI
> 
> I am currently working on a series to unify the storage of the
> VDSO data for most architectures, including powerpc.
> This will also include a dedicated rng data page.
> 
> That generic infrastructure would replace the need for Patch 1.
> Obviously, if your series gets applied, I can adapt mine for that.
> 
> If you are about to also modify other architectures in a similar way,
> we may want to coordinate.
> 

I'm not going to do anything on other architectures.

Indeed my patch is an outcome of the discussion at 
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/ffd7fc255e194d1e2b0aa3d9d129e826c53219d4.1725611321.git.christophe.leroy@csgroup.eu/

I'm all fine if you are doing something generic for all architectures. 
For powerpc will it handle the entire non-time data, not only rng ? The 
purpose being to revert 
https://github.com/torvalds/linux/commit/c73049389e58c01e2e3bbfae900c8daeee177191

Christophe

Re: [PATCH 1/2] powerpc/vdso: Add a page for non-time data

[Thomas Weißschuh]

Hi Christophe,

On Wed, Oct 02, 2024 at 12:10:08PM GMT, Christophe Leroy wrote:
> Le 02/10/2024 à 10:54, Thomas Weißschuh a écrit :
> > On Wed, Oct 02, 2024 at 10:39:28AM GMT, Christophe Leroy wrote:
> > > The page containing VDSO time data is swapped with the one containing
> > > TIME namespace data when a process uses a non-root time namespace.
> > > For other data like powerpc specific data and RNG data, it means
> > > tracking whether time namespace is the root one or not to know which
> > > page to use.
> > > 
> > > Simplify the logic behind by moving time data out of first data page
> > > so that the first data page which contains everything else always
> > > remains the first page. Time data is in the second or third page
> > > depending on selected time namespace.
> > > 
> > > While we are playing with get_datapage macro, directly take into
> > > account the data offset inside the macro instead of adding that offset
> > > afterwards.
> > 
> > FYI
> > 
> > I am currently working on a series to unify the storage of the
> > VDSO data for most architectures, including powerpc.
> > This will also include a dedicated rng data page.
> > 
> > That generic infrastructure would replace the need for Patch 1.
> > Obviously, if your series gets applied, I can adapt mine for that.
> > 
> > If you are about to also modify other architectures in a similar way,
> > we may want to coordinate.
> > 
> 
> I'm not going to do anything on other architectures.

Ack.

> Indeed my patch is an outcome of the discussion at
> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/ffd7fc255e194d1e2b0aa3d9d129e826c53219d4.1725611321.git.christophe.leroy@csgroup.eu/
>
> I'm all fine if you are doing something generic for all architectures. For
> powerpc will it handle the entire non-time data, not only rng ? The purpose
> being to revert https://github.com/torvalds/linux/commit/c73049389e58c01e2e3bbfae900c8daeee177191

Yes, it can handle arbitrary arch-specific non-time-related data in
addition to the rng data.
(In addition it also handles arch-specific time-related data)
The code introduced by the linked patch is gone in my series.


Thomas

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

[Jason A. Donenfeld]

Hey Christophe, Michael,

This series actually looks pretty okay to me. I realize ThomasW is
working on more generic cleanups that might obliterate the need for
this, and that may or may not wind up in 6.13. But, I was thinking, this
seems like a good correct thing to do, and to do it now for 6.12, maybe
as a fix through the powerpc tree. Then ThomasW can base his work atop
this, which might wind up including the nice lr optimizations you've
made. And then also if ThomasW's work doesn't land or gets reverted or
whatever, at least we'll have this in tree for 6.12.

Michael - what do you think of that? Worth taking these two patches into
your fixes?

Jason

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

[Michael Ellerman]

On October 4, 2024 2:33:54 AM GMT+10:00, "Jason A. Donenfeld" <Jason@zx2c4.com> wrote:
>Hey Christophe, Michael,
>
>This series actually looks pretty okay to me. I realize ThomasW is
>working on more generic cleanups that might obliterate the need for
>this, and that may or may not wind up in 6.13. But, I was thinking, this
>seems like a good correct thing to do, and to do it now for 6.12, maybe
>as a fix through the powerpc tree. Then ThomasW can base his work atop
>this, which might wind up including the nice lr optimizations you've
>made. And then also if ThomasW's work doesn't land or gets reverted or
>whatever, at least we'll have this in tree for 6.12.
>
>Michael - what do you think of that? Worth taking these two patches into
>your fixes?

I agree the series looks good. But they're not fixes by my reading, so I'd be inclined to put them in next for v6.13?

cheers
-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

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

[Jason A. Donenfeld]

On Fri, Oct 04, 2024 at 08:52:40PM +1000, Michael Ellerman wrote:
> 
> 
> On October 4, 2024 2:33:54 AM GMT+10:00, "Jason A. Donenfeld" <Jason@zx2c4.com> wrote:
> >Hey Christophe, Michael,
> >
> >This series actually looks pretty okay to me. I realize ThomasW is
> >working on more generic cleanups that might obliterate the need for
> >this, and that may or may not wind up in 6.13. But, I was thinking, this
> >seems like a good correct thing to do, and to do it now for 6.12, maybe
> >as a fix through the powerpc tree. Then ThomasW can base his work atop
> >this, which might wind up including the nice lr optimizations you've
> >made. And then also if ThomasW's work doesn't land or gets reverted or
> >whatever, at least we'll have this in tree for 6.12.
> >
> >Michael - what do you think of that? Worth taking these two patches into
> >your fixes?
> 
> I agree the series looks good. But they're not fixes by my reading, so I'd be inclined to put them in next for v6.13?

They're "close enough" to fixes. The get_realdatapage stuff is super
wonky and weird and it's quite good Christophe has gotten rid of it.
Returning NULL from the generic accesor function never really sat right
and looks buggy even if it does work. But more to the point, given the
other scheduled churn for 6.13, it's going to be a tree-clashing
nightmare to get this in later. And this Sunday is rc2 only, so why not.

Jason

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

[Jason A. Donenfeld]

On Fri, Oct 04, 2024 at 04:03:34PM +0200, Jason A. Donenfeld wrote:
> On Fri, Oct 04, 2024 at 08:52:40PM +1000, Michael Ellerman wrote:
> > 
> > 
> > On October 4, 2024 2:33:54 AM GMT+10:00, "Jason A. Donenfeld" <Jason@zx2c4.com> wrote:
> > >Hey Christophe, Michael,
> > >
> > >This series actually looks pretty okay to me. I realize ThomasW is
> > >working on more generic cleanups that might obliterate the need for
> > >this, and that may or may not wind up in 6.13. But, I was thinking, this
> > >seems like a good correct thing to do, and to do it now for 6.12, maybe
> > >as a fix through the powerpc tree. Then ThomasW can base his work atop
> > >this, which might wind up including the nice lr optimizations you've
> > >made. And then also if ThomasW's work doesn't land or gets reverted or
> > >whatever, at least we'll have this in tree for 6.12.
> > >
> > >Michael - what do you think of that? Worth taking these two patches into
> > >your fixes?
> > 
> > I agree the series looks good. But they're not fixes by my reading, so I'd be inclined to put them in next for v6.13?
> 
> They're "close enough" to fixes. The get_realdatapage stuff is super
> wonky and weird and it's quite good Christophe has gotten rid of it.
> Returning NULL from the generic accesor function never really sat right
> and looks buggy even if it does work. But more to the point, given the
> other scheduled churn for 6.13, it's going to be a tree-clashing
> nightmare to get this in later. And this Sunday is rc2 only, so why not.

Bumping to top of the box.

Jason

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

[Thomas Weißschuh]

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

> [..]

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

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

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


Thomas

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

[Christophe Leroy]

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

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

[Michael Ellerman]

"Jason A. Donenfeld" <Jason@zx2c4.com> writes:
> On Fri, Oct 04, 2024 at 04:03:34PM +0200, Jason A. Donenfeld wrote:
>> On Fri, Oct 04, 2024 at 08:52:40PM +1000, Michael Ellerman wrote:
>> > 
>> > 
>> > On October 4, 2024 2:33:54 AM GMT+10:00, "Jason A. Donenfeld" <Jason@zx2c4.com> wrote:
>> > >Hey Christophe, Michael,
>> > >
>> > >This series actually looks pretty okay to me. I realize ThomasW is
>> > >working on more generic cleanups that might obliterate the need for
>> > >this, and that may or may not wind up in 6.13. But, I was thinking, this
>> > >seems like a good correct thing to do, and to do it now for 6.12, maybe
>> > >as a fix through the powerpc tree. Then ThomasW can base his work atop
>> > >this, which might wind up including the nice lr optimizations you've
>> > >made. And then also if ThomasW's work doesn't land or gets reverted or
>> > >whatever, at least we'll have this in tree for 6.12.
>> > >
>> > >Michael - what do you think of that? Worth taking these two patches into
>> > >your fixes?
>> > 
>> > I agree the series looks good. But they're not fixes by my reading, so I'd be inclined to put them in next for v6.13?
>> 
>> They're "close enough" to fixes. The get_realdatapage stuff is super
>> wonky and weird and it's quite good Christophe has gotten rid of it.
>> Returning NULL from the generic accesor function never really sat right
>> and looks buggy even if it does work. But more to the point, given the
>> other scheduled churn for 6.13, it's going to be a tree-clashing
>> nightmare to get this in later. And this Sunday is rc2 only, so why not.
>
> Bumping to top of the box.

Ack. It was too late for rc2, and I'm naturally cautious, so I decided
these would go into next.

We can handle any merge conflicts with a topic branch.

cheers

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

[Michael Ellerman]

Christophe Leroy <christophe.leroy@csgroup.eu> 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@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 ?)
 
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

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

[Christophe Leroy]

Le 11/10/2024 à 13:46, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>> Le 10/10/2024 à 11:12, Thomas Weißschuh a écrit :
>>>
>>> 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:

Yes indeed I forgot that. So even if the final check doesn't complain, 
it won't work at the end.

Don't know if we could find a way to detect that and fail the build.

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

That's not related to VDSO it seems. There is the same thing in 
arch/powerpc/lib for instance:

$ file arch/powerpc/lib/*.o
arch/powerpc/lib/checksum_64.o:         ELF 64-bit MSB relocatable, 
64-bit PowerPC or cisco 7500, Unspecified or Power ELF V1 ABI, version 1 
(SYSV), not stripped
arch/powerpc/lib/checksum_wrappers.o:   ELF 64-bit MSB relocatable, 
64-bit PowerPC or cisco 7500, OpenPOWER ELF V2 ABI, version 1 (SYSV), 
not stripped
arch/powerpc/lib/code-patching.o:       ELF 64-bit MSB relocatable, 
64-bit PowerPC or cisco 7500, OpenPOWER ELF V2 ABI, version 1 (SYSV), 
not stripped
arch/powerpc/lib/copy_mc_64.o:          ELF 64-bit MSB relocatable, 
64-bit PowerPC or cisco 7500, Unspecified or Power ELF V1 ABI, version 1 
(SYSV), not stripped
arch/powerpc/lib/copypage_64.o:         ELF 64-bit MSB relocatable, 
64-bit PowerPC or cisco 7500, OpenPOWER ELF V2 ABI, version 1 (SYSV), 
not stripped
...

Seems like all .c files result in a ELF V2 while some of .S files are V1 
et some are V2. That's odd because the build arguments seems to be the same:

# AS      arch/powerpc/lib/checksum_64.o
   powerpc64-linux-gcc -Wp,-MMD,arch/powerpc/lib/.checksum_64.o.d 
-nostdinc -I./arch/powerpc/include -I./arch/powerpc/include/generated 
-I./include -I./arch/powerpc/include/uapi 
-I./arch/powerpc/include/generated/uapi -I./include/uapi 
-I./include/generated/uapi -include ./include/linux/compiler-version.h 
-include ./include/linux/kconfig.h -D__KERNEL__ -I ./arch/powerpc 
-DHAVE_AS_ATHIGH=1 -D__ASSEMBLY__ -fno-PIE -m64 -mcpu=power8 -mabi=elfv2 
-mlittle-endian -Wa,--fatal-warnings 
-DKBUILD_MODFILE='"arch/powerpc/lib/checksum_64"' 
-DKBUILD_MODNAME='"checksum_64"' -D__KBUILD_MODNAME=kmod_checksum_64 -c 
-o arch/powerpc/lib/checksum_64.o arch/powerpc/lib/checksum_64.S  ; 
./tools/objtool/objtool --mcount   arch/powerpc/lib/checksum_64.o

# AS      arch/powerpc/lib/copypage_64.o
   powerpc64-linux-gcc -Wp,-MMD,arch/powerpc/lib/.copypage_64.o.d 
-nostdinc -I./arch/powerpc/include -I./arch/powerpc/include/generated 
-I./include -I./arch/powerpc/include/uapi 
-I./arch/powerpc/include/generated/uapi -I./include/uapi 
-I./include/generated/uapi -include ./include/linux/compiler-version.h 
-include ./include/linux/kconfig.h -D__KERNEL__ -I ./arch/powerpc 
-DHAVE_AS_ATHIGH=1 -D__ASSEMBLY__ -fno-PIE -m64 -mcpu=power8 -mabi=elfv2 
-mlittle-endian -Wa,--fatal-warnings 
-DKBUILD_MODFILE='"arch/powerpc/lib/copypage_64"' 
-DKBUILD_MODNAME='"copypage_64"' -D__KBUILD_MODNAME=kmod_copypage_64 -c 
-o arch/powerpc/lib/copypage_64.o arch/powerpc/lib/copypage_64.S  ; 
./tools/objtool/objtool --mcount   arch/powerpc/lib/copypage_64.o

Re: [PATCH 1/2] powerpc/vdso: Add a page for non-time data

[Michael Ellerman]

On Wed, 02 Oct 2024 10:39:28 +0200, Christophe Leroy wrote:
> The page containing VDSO time data is swapped with the one containing
> TIME namespace data when a process uses a non-root time namespace.
> For other data like powerpc specific data and RNG data, it means
> tracking whether time namespace is the root one or not to know which
> page to use.
> 
> Simplify the logic behind by moving time data out of first data page
> so that the first data page which contains everything else always
> remains the first page. Time data is in the second or third page
> depending on selected time namespace.
> 
> [...]

Applied to powerpc/next.

[1/2] powerpc/vdso: Add a page for non-time data
      https://git.kernel.org/powerpc/c/c39b1dcf055d420a498d1047c645b776e4d1a7aa
[2/2] powerpc/vdso: Implement __arch_get_vdso_rng_data()
      https://git.kernel.org/powerpc/c/4e3fa1aecb2c1f128f7289272fe2947e4396f1ce

cheers