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 <redacted>
---
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(-)
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(-)
@@ -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_PPC64OFFSET(CFG_ICACHE_BLOCKSZ,vdso_arch_data,icache_block_size);
From: Thomas Weißschuh <hidden> Date: 2024-10-02 08:54:26
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.
[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:
quoted
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.
From: Thomas Weißschuh <hidden> Date: 2024-10-02 10:21:29
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 :
quoted
On Wed, Oct 02, 2024 at 10:39:28AM GMT, Christophe Leroy wrote:
quoted
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.
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
From: "Jason A. Donenfeld" <Jason@zx2c4.com> Date: 2024-10-03 16:33:59
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
From: Michael Ellerman <hidden> Date: 2024-10-04 10:52:46
On October 4, 2024 2:33:54 AM GMT+10:00, "Jason A. Donenfeld" [off-list ref] 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.
From: "Jason A. Donenfeld" <Jason@zx2c4.com> Date: 2024-10-04 14:03:40
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" [off-list ref] wrote:
quoted
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
From: "Jason A. Donenfeld" <Jason@zx2c4.com> Date: 2024-10-07 16:28:51
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:
quoted
On October 4, 2024 2:33:54 AM GMT+10:00, "Jason A. Donenfeld" [off-list ref] wrote:
quoted
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.
From: Thomas Weißschuh <hidden> Date: 2024-10-10 08:20:41
On Wed, Oct 02, 2024 at 10:39:29AM +0200, Christophe Leroy wrote:
quoted hunk
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(-)
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.
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:
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(-)
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
From: Thomas Weißschuh <hidden> Date: 2024-10-10 09:12:29
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(-)
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
On Thu, Oct 10, 2024 at 11:00:15AM +0200, Christophe Leroy wrote:
quoted
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(-)
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.
quoted
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.
quoted
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
From: Michael Ellerman <hidden> Date: 2024-10-11 10:39:52
"Jason A. Donenfeld" [off-list ref] writes:
On Fri, Oct 04, 2024 at 04:03:34PM +0200, Jason A. Donenfeld wrote:
quoted
On Fri, Oct 04, 2024 at 08:52:40PM +1000, Michael Ellerman wrote:
quoted
On October 4, 2024 2:33:54 AM GMT+10:00, "Jason A. Donenfeld" [off-list ref] wrote:
quoted
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
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2024-10-11 11:46:52
Christophe Leroy [off-list ref] writes:
Le 10/10/2024 à 11:12, Thomas Weißschuh a écrit :
quoted
On Thu, Oct 10, 2024 at 11:00:15AM +0200, Christophe Leroy wrote:
quoted
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(-)
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.
quoted
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.
quoted
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
PC-rel instructions only exist on very very recent powerpc CPUs (power10 ?)
Yeah power10 or later.
quoted
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. >
quoted
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
From: Michael Ellerman <hidden> Date: 2024-11-07 08:45:01
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.
[...]