* [PATCH] powerpc: Replace ppc64 DT_RELACOUNT usage with DT_RELASZ/24
@ 2022-03-09 5:51 Fangrui Song
2022-03-09 19:01 ` Nathan Chancellor
2022-03-10 19:48 ` Nick Desaulniers
0 siblings, 2 replies; 6+ messages in thread
From: Fangrui Song @ 2022-03-09 5:51 UTC (permalink / raw)
To: Michael Ellerman, Benjamin Herrenschmidt, Alexey Kardashevskiy,
linuxppc-dev
Cc: Nathan Chancellor, linux-kernel, llvm, Fangrui Song
DT_RELACOUNT is an ELF dynamic tag inherited from SunOS indicating the
number of R_*_RELATIVE relocations. It is optional but {ld.lld,ld.lld}
-z combreloc always creates it (if non-zero) to slightly speed up glibc
ld.so relocation resolving by avoiding R_*R_PPC64_RELATIVE type
comparison. The tag is otherwise nearly unused in the wild and I'd
recommend that software avoids using it.
lld>=14.0.0 (since commit da0e5b885b25cf4ded0fa89b965dc6979ac02ca9)
underestimates DT_RELACOUNT for ppc64 when position-independent long
branch thunks are used. Correcting it needs non-trivial arch-specific
complexity which I'd prefer to avoid. Since our code always compares the
relocation type with R_PPC64_RELATIVE, replacing every occurrence of
DT_RELACOUNT with DT_RELASZ/sizeof(Elf64_Rela)=DT_RELASZ/24 is a correct
alternative.
DT_RELASZ is in practice bounded by an uint32_t. Dividing x by 24 can be
implemented as (uint32_t)(x*0xaaaaaaab) >> 4.
Link: https://github.com/ClangBuiltLinux/linux/issues/1581
Reported-by: Nathan Chancellor <nathan@kernel.org>
Signed-off-by: Fangrui Song <maskray@google.com>
---
arch/powerpc/boot/crt0.S | 28 +++++++++++++++++-----------
arch/powerpc/kernel/reloc_64.S | 15 +++++++++------
2 files changed, 26 insertions(+), 17 deletions(-)
diff --git a/arch/powerpc/boot/crt0.S b/arch/powerpc/boot/crt0.S
index feadee18e271..1c96ebe7ef1a 100644
--- a/arch/powerpc/boot/crt0.S
+++ b/arch/powerpc/boot/crt0.S
@@ -8,7 +8,7 @@
#include "ppc_asm.h"
RELA = 7
-RELACOUNT = 0x6ffffff9
+RELASZ = 8
.data
/* A procedure descriptor used when booting this as a COFF file.
@@ -65,7 +65,7 @@ p_base: mflr r10 /* r10 now points to runtime addr of p_base */
subf r11,r11,r12 /* runtime - linktime offset */
/* The dynamic section contains a series of tagged entries.
- * We need the RELA and RELACOUNT entries. */
+ * We need the RELA and RELASZ entries. */
li r9,0
li r0,0
9: lwz r8,0(r12) /* get tag */
@@ -75,18 +75,21 @@ p_base: mflr r10 /* r10 now points to runtime addr of p_base */
bne 11f
lwz r9,4(r12) /* get RELA pointer in r9 */
b 12f
-11: addis r8,r8,(-RELACOUNT)@ha
- cmpwi r8,RELACOUNT@l
+11: cmpwi r8,RELASZ
bne 12f
- lwz r0,4(r12) /* get RELACOUNT value in r0 */
+ lwz r0,4(r12) /* get RELASZ / 24 in r0 */
+ lis r8,0xaaaa
+ ori r8,r8,0xaaab
+ mulhwu r0,r0,r8
+ srwi r0,r0,4
12: addi r12,r12,8
b 9b
/* The relocation section contains a list of relocations.
* We now do the R_PPC_RELATIVE ones, which point to words
* which need to be initialized with addend + offset.
- * The R_PPC_RELATIVE ones come first and there are RELACOUNT
- * of them. */
+ * The R_PPC_RELATIVE ones come first and there are at most
+ * RELASZ/24 of them. */
10: /* skip relocation if we don't have both */
cmpwi r0,0
beq 3f
@@ -160,14 +163,17 @@ p_base: mflr r10 /* r10 now points to runtime addr of p_base */
bne 10f
ld r13,8(r11) /* get RELA pointer in r13 */
b 11f
-10: addis r12,r12,(-RELACOUNT)@ha
- cmpdi r12,RELACOUNT@l
+10: cmpdi r12,RELASZ
bne 11f
- ld r8,8(r11) /* get RELACOUNT value in r8 */
+ ld r8,8(r11) /* get RELASZ / 24 in r8 */
+ lis r0,0xaaaa
+ ori r0,r0,0xaaab
+ mulhwu r8,r8,r0
+ srwi r8,r8,4
11: addi r11,r11,16
b 9b
12:
- cmpdi r13,0 /* check we have both RELA and RELACOUNT */
+ cmpdi r13,0 /* check we have both RELA and RELASZ */
cmpdi cr1,r8,0
beq 3f
beq cr1,3f
diff --git a/arch/powerpc/kernel/reloc_64.S b/arch/powerpc/kernel/reloc_64.S
index 02d4719bf43a..362be759609f 100644
--- a/arch/powerpc/kernel/reloc_64.S
+++ b/arch/powerpc/kernel/reloc_64.S
@@ -8,7 +8,7 @@
#include <asm/ppc_asm.h>
RELA = 7
-RELACOUNT = 0x6ffffff9
+RELASZ = 8
R_PPC64_RELATIVE = 22
/*
@@ -27,7 +27,7 @@ _GLOBAL(relocate)
add r10,r10,r12 /* r10 has runtime addr of _stext */
/*
- * Scan the dynamic section for the RELA and RELACOUNT entries.
+ * Scan the dynamic section for the RELA and RELASZ entries.
*/
li r7,0
li r8,0
@@ -38,13 +38,16 @@ _GLOBAL(relocate)
bne 2f
ld r7,8(r11) /* get RELA pointer in r7 */
b 3f
-2: addis r6,r6,(-RELACOUNT)@ha
- cmpdi r6,RELACOUNT@l
+2: cmpdi r6,RELASZ
bne 3f
- ld r8,8(r11) /* get RELACOUNT value in r8 */
+ ld r8,8(r11) /* get RELA / 24 in r8 */
+ lis r0,0xaaaa
+ ori r0,r0,0xaaab
+ mulhwu r8,r8,r0
+ srwi r8,r8,4
3: addi r11,r11,16
b 1b
-4: cmpdi r7,0 /* check we have both RELA and RELACOUNT */
+4: cmpdi r7,0 /* check we have both RELA and RELASZ */
cmpdi cr1,r8,0
beq 6f
beq cr1,6f
--
2.35.1.616.g0bdcbb4464-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc: Replace ppc64 DT_RELACOUNT usage with DT_RELASZ/24
2022-03-09 5:51 [PATCH] powerpc: Replace ppc64 DT_RELACOUNT usage with DT_RELASZ/24 Fangrui Song
@ 2022-03-09 19:01 ` Nathan Chancellor
2022-03-10 19:48 ` Nick Desaulniers
1 sibling, 0 replies; 6+ messages in thread
From: Nathan Chancellor @ 2022-03-09 19:01 UTC (permalink / raw)
To: Fangrui Song
Cc: Michael Ellerman, Benjamin Herrenschmidt, Alexey Kardashevskiy,
linuxppc-dev, linux-kernel, llvm
On Tue, Mar 08, 2022 at 09:51:18PM -0800, Fangrui Song wrote:
> DT_RELACOUNT is an ELF dynamic tag inherited from SunOS indicating the
> number of R_*_RELATIVE relocations. It is optional but {ld.lld,ld.lld}
^ ld.{bfd,lld} ?
> -z combreloc always creates it (if non-zero) to slightly speed up glibc
> ld.so relocation resolving by avoiding R_*R_PPC64_RELATIVE type
> comparison. The tag is otherwise nearly unused in the wild and I'd
> recommend that software avoids using it.
>
> lld>=14.0.0 (since commit da0e5b885b25cf4ded0fa89b965dc6979ac02ca9)
> underestimates DT_RELACOUNT for ppc64 when position-independent long
> branch thunks are used. Correcting it needs non-trivial arch-specific
> complexity which I'd prefer to avoid. Since our code always compares the
> relocation type with R_PPC64_RELATIVE, replacing every occurrence of
> DT_RELACOUNT with DT_RELASZ/sizeof(Elf64_Rela)=DT_RELASZ/24 is a correct
> alternative.
>
> DT_RELASZ is in practice bounded by an uint32_t. Dividing x by 24 can be
> implemented as (uint32_t)(x*0xaaaaaaab) >> 4.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/1581
> Reported-by: Nathan Chancellor <nathan@kernel.org>
> Signed-off-by: Fangrui Song <maskray@google.com>
Thank you a lot for the fix! I tested this with several different
configurations with tip of tree LLVM and they all booted in QEMU. I also
tested one configuration with GCC 5.4 (we support GCC 5.x as a minimum)
and it booted as well.
Tested-by: Nathan Chancellor <nathan@kernel.org>
> ---
> arch/powerpc/boot/crt0.S | 28 +++++++++++++++++-----------
> arch/powerpc/kernel/reloc_64.S | 15 +++++++++------
> 2 files changed, 26 insertions(+), 17 deletions(-)
>
> diff --git a/arch/powerpc/boot/crt0.S b/arch/powerpc/boot/crt0.S
> index feadee18e271..1c96ebe7ef1a 100644
> --- a/arch/powerpc/boot/crt0.S
> +++ b/arch/powerpc/boot/crt0.S
> @@ -8,7 +8,7 @@
> #include "ppc_asm.h"
>
> RELA = 7
> -RELACOUNT = 0x6ffffff9
> +RELASZ = 8
>
> .data
> /* A procedure descriptor used when booting this as a COFF file.
> @@ -65,7 +65,7 @@ p_base: mflr r10 /* r10 now points to runtime addr of p_base */
> subf r11,r11,r12 /* runtime - linktime offset */
>
> /* The dynamic section contains a series of tagged entries.
> - * We need the RELA and RELACOUNT entries. */
> + * We need the RELA and RELASZ entries. */
> li r9,0
> li r0,0
> 9: lwz r8,0(r12) /* get tag */
> @@ -75,18 +75,21 @@ p_base: mflr r10 /* r10 now points to runtime addr of p_base */
> bne 11f
> lwz r9,4(r12) /* get RELA pointer in r9 */
> b 12f
> -11: addis r8,r8,(-RELACOUNT)@ha
> - cmpwi r8,RELACOUNT@l
> +11: cmpwi r8,RELASZ
> bne 12f
> - lwz r0,4(r12) /* get RELACOUNT value in r0 */
> + lwz r0,4(r12) /* get RELASZ / 24 in r0 */
> + lis r8,0xaaaa
> + ori r8,r8,0xaaab
> + mulhwu r0,r0,r8
> + srwi r0,r0,4
> 12: addi r12,r12,8
> b 9b
>
> /* The relocation section contains a list of relocations.
> * We now do the R_PPC_RELATIVE ones, which point to words
> * which need to be initialized with addend + offset.
> - * The R_PPC_RELATIVE ones come first and there are RELACOUNT
> - * of them. */
> + * The R_PPC_RELATIVE ones come first and there are at most
> + * RELASZ/24 of them. */
> 10: /* skip relocation if we don't have both */
> cmpwi r0,0
> beq 3f
> @@ -160,14 +163,17 @@ p_base: mflr r10 /* r10 now points to runtime addr of p_base */
> bne 10f
> ld r13,8(r11) /* get RELA pointer in r13 */
> b 11f
> -10: addis r12,r12,(-RELACOUNT)@ha
> - cmpdi r12,RELACOUNT@l
> +10: cmpdi r12,RELASZ
> bne 11f
> - ld r8,8(r11) /* get RELACOUNT value in r8 */
> + ld r8,8(r11) /* get RELASZ / 24 in r8 */
> + lis r0,0xaaaa
> + ori r0,r0,0xaaab
> + mulhwu r8,r8,r0
> + srwi r8,r8,4
> 11: addi r11,r11,16
> b 9b
> 12:
> - cmpdi r13,0 /* check we have both RELA and RELACOUNT */
> + cmpdi r13,0 /* check we have both RELA and RELASZ */
> cmpdi cr1,r8,0
> beq 3f
> beq cr1,3f
> diff --git a/arch/powerpc/kernel/reloc_64.S b/arch/powerpc/kernel/reloc_64.S
> index 02d4719bf43a..362be759609f 100644
> --- a/arch/powerpc/kernel/reloc_64.S
> +++ b/arch/powerpc/kernel/reloc_64.S
> @@ -8,7 +8,7 @@
> #include <asm/ppc_asm.h>
>
> RELA = 7
> -RELACOUNT = 0x6ffffff9
> +RELASZ = 8
> R_PPC64_RELATIVE = 22
>
> /*
> @@ -27,7 +27,7 @@ _GLOBAL(relocate)
> add r10,r10,r12 /* r10 has runtime addr of _stext */
>
> /*
> - * Scan the dynamic section for the RELA and RELACOUNT entries.
> + * Scan the dynamic section for the RELA and RELASZ entries.
> */
> li r7,0
> li r8,0
> @@ -38,13 +38,16 @@ _GLOBAL(relocate)
> bne 2f
> ld r7,8(r11) /* get RELA pointer in r7 */
> b 3f
> -2: addis r6,r6,(-RELACOUNT)@ha
> - cmpdi r6,RELACOUNT@l
> +2: cmpdi r6,RELASZ
> bne 3f
> - ld r8,8(r11) /* get RELACOUNT value in r8 */
> + ld r8,8(r11) /* get RELA / 24 in r8 */
> + lis r0,0xaaaa
> + ori r0,r0,0xaaab
> + mulhwu r8,r8,r0
> + srwi r8,r8,4
> 3: addi r11,r11,16
> b 1b
> -4: cmpdi r7,0 /* check we have both RELA and RELACOUNT */
> +4: cmpdi r7,0 /* check we have both RELA and RELASZ */
> cmpdi cr1,r8,0
> beq 6f
> beq cr1,6f
> --
> 2.35.1.616.g0bdcbb4464-goog
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc: Replace ppc64 DT_RELACOUNT usage with DT_RELASZ/24
2022-03-09 5:51 [PATCH] powerpc: Replace ppc64 DT_RELACOUNT usage with DT_RELASZ/24 Fangrui Song
2022-03-09 19:01 ` Nathan Chancellor
@ 2022-03-10 19:48 ` Nick Desaulniers
2022-03-10 21:08 ` Fāng-ruì Sòng
1 sibling, 1 reply; 6+ messages in thread
From: Nick Desaulniers @ 2022-03-10 19:48 UTC (permalink / raw)
To: Fangrui Song
Cc: Michael Ellerman, Benjamin Herrenschmidt, Alexey Kardashevskiy,
linuxppc-dev, Nathan Chancellor, linux-kernel, llvm
On Tue, Mar 8, 2022 at 9:53 PM Fangrui Song <maskray@google.com> wrote:
>
> DT_RELACOUNT is an ELF dynamic tag inherited from SunOS indicating the
> number of R_*_RELATIVE relocations. It is optional but {ld.lld,ld.lld}
> -z combreloc always creates it (if non-zero) to slightly speed up glibc
> ld.so relocation resolving by avoiding R_*R_PPC64_RELATIVE type
> comparison. The tag is otherwise nearly unused in the wild and I'd
> recommend that software avoids using it.
>
> lld>=14.0.0 (since commit da0e5b885b25cf4ded0fa89b965dc6979ac02ca9)
> underestimates DT_RELACOUNT for ppc64 when position-independent long
> branch thunks are used. Correcting it needs non-trivial arch-specific
> complexity which I'd prefer to avoid. Since our code always compares the
> relocation type with R_PPC64_RELATIVE, replacing every occurrence of
> DT_RELACOUNT with DT_RELASZ/sizeof(Elf64_Rela)=DT_RELASZ/24 is a correct
> alternative.
checking that sizeof(Elf64_Rela) == 24, yep: https://godbolt.org/z/bb4aKbo5T
>
> DT_RELASZ is in practice bounded by an uint32_t. Dividing x by 24 can be
> implemented as (uint32_t)(x*0xaaaaaaab) >> 4.
Yep: https://godbolt.org/z/x9445ePPv
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/1581
> Reported-by: Nathan Chancellor <nathan@kernel.org>
> Signed-off-by: Fangrui Song <maskray@google.com>
> ---
> arch/powerpc/boot/crt0.S | 28 +++++++++++++++++-----------
> arch/powerpc/kernel/reloc_64.S | 15 +++++++++------
> 2 files changed, 26 insertions(+), 17 deletions(-)
>
> diff --git a/arch/powerpc/boot/crt0.S b/arch/powerpc/boot/crt0.S
> index feadee18e271..1c96ebe7ef1a 100644
> --- a/arch/powerpc/boot/crt0.S
> +++ b/arch/powerpc/boot/crt0.S
> @@ -8,7 +8,7 @@
> #include "ppc_asm.h"
>
> RELA = 7
> -RELACOUNT = 0x6ffffff9
> +RELASZ = 8
>
> .data
> /* A procedure descriptor used when booting this as a COFF file.
> @@ -65,7 +65,7 @@ p_base: mflr r10 /* r10 now points to runtime addr of p_base */
> subf r11,r11,r12 /* runtime - linktime offset */
>
> /* The dynamic section contains a series of tagged entries.
> - * We need the RELA and RELACOUNT entries. */
> + * We need the RELA and RELASZ entries. */
> li r9,0
> li r0,0
> 9: lwz r8,0(r12) /* get tag */
> @@ -75,18 +75,21 @@ p_base: mflr r10 /* r10 now points to runtime addr of p_base */
> bne 11f
> lwz r9,4(r12) /* get RELA pointer in r9 */
> b 12f
> -11: addis r8,r8,(-RELACOUNT)@ha
> - cmpwi r8,RELACOUNT@l
> +11: cmpwi r8,RELASZ
> bne 12f
> - lwz r0,4(r12) /* get RELACOUNT value in r0 */
> + lwz r0,4(r12) /* get RELASZ / 24 in r0 */
> + lis r8,0xaaaa
> + ori r8,r8,0xaaab
> + mulhwu r0,r0,r8
> + srwi r0,r0,4
Both translation units include arch/powerpc/include/asm/ppc_asm.h,
which happens to define LOAD_REG_IMMEDIATE. Let's reuse that rather
than open code lis+ori?
> 12: addi r12,r12,8
> b 9b
>
> /* The relocation section contains a list of relocations.
> * We now do the R_PPC_RELATIVE ones, which point to words
> * which need to be initialized with addend + offset.
> - * The R_PPC_RELATIVE ones come first and there are RELACOUNT
> - * of them. */
> + * The R_PPC_RELATIVE ones come first and there are at most
> + * RELASZ/24 of them. */
> 10: /* skip relocation if we don't have both */
> cmpwi r0,0
> beq 3f
> @@ -160,14 +163,17 @@ p_base: mflr r10 /* r10 now points to runtime addr of p_base */
> bne 10f
> ld r13,8(r11) /* get RELA pointer in r13 */
> b 11f
> -10: addis r12,r12,(-RELACOUNT)@ha
> - cmpdi r12,RELACOUNT@l
> +10: cmpdi r12,RELASZ
> bne 11f
> - ld r8,8(r11) /* get RELACOUNT value in r8 */
> + ld r8,8(r11) /* get RELASZ / 24 in r8 */
> + lis r0,0xaaaa
> + ori r0,r0,0xaaab
> + mulhwu r8,r8,r0
> + srwi r8,r8,4
> 11: addi r11,r11,16
> b 9b
> 12:
> - cmpdi r13,0 /* check we have both RELA and RELACOUNT */
> + cmpdi r13,0 /* check we have both RELA and RELASZ */
> cmpdi cr1,r8,0
> beq 3f
> beq cr1,3f
> diff --git a/arch/powerpc/kernel/reloc_64.S b/arch/powerpc/kernel/reloc_64.S
> index 02d4719bf43a..362be759609f 100644
> --- a/arch/powerpc/kernel/reloc_64.S
> +++ b/arch/powerpc/kernel/reloc_64.S
> @@ -8,7 +8,7 @@
> #include <asm/ppc_asm.h>
>
> RELA = 7
> -RELACOUNT = 0x6ffffff9
> +RELASZ = 8
> R_PPC64_RELATIVE = 22
>
> /*
> @@ -27,7 +27,7 @@ _GLOBAL(relocate)
> add r10,r10,r12 /* r10 has runtime addr of _stext */
>
> /*
> - * Scan the dynamic section for the RELA and RELACOUNT entries.
> + * Scan the dynamic section for the RELA and RELASZ entries.
> */
> li r7,0
> li r8,0
> @@ -38,13 +38,16 @@ _GLOBAL(relocate)
> bne 2f
> ld r7,8(r11) /* get RELA pointer in r7 */
> b 3f
> -2: addis r6,r6,(-RELACOUNT)@ha
> - cmpdi r6,RELACOUNT@l
> +2: cmpdi r6,RELASZ
> bne 3f
> - ld r8,8(r11) /* get RELACOUNT value in r8 */
> + ld r8,8(r11) /* get RELA / 24 in r8 */
> + lis r0,0xaaaa
> + ori r0,r0,0xaaab
> + mulhwu r8,r8,r0
> + srwi r8,r8,4
> 3: addi r11,r11,16
> b 1b
> -4: cmpdi r7,0 /* check we have both RELA and RELACOUNT */
> +4: cmpdi r7,0 /* check we have both RELA and RELASZ */
> cmpdi cr1,r8,0
> beq 6f
> beq cr1,6f
> --
> 2.35.1.616.g0bdcbb4464-goog
>
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc: Replace ppc64 DT_RELACOUNT usage with DT_RELASZ/24
2022-03-10 19:48 ` Nick Desaulniers
@ 2022-03-10 21:08 ` Fāng-ruì Sòng
2022-03-11 4:15 ` Michael Ellerman
0 siblings, 1 reply; 6+ messages in thread
From: Fāng-ruì Sòng @ 2022-03-10 21:08 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Michael Ellerman, Nick Desaulniers, Benjamin Herrenschmidt,
linuxppc-dev, Nathan Chancellor, linux-kernel, llvm
On Thu, Mar 10, 2022 at 11:48 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Tue, Mar 8, 2022 at 9:53 PM Fangrui Song <maskray@google.com> wrote:
> >
> > DT_RELACOUNT is an ELF dynamic tag inherited from SunOS indicating the
> > number of R_*_RELATIVE relocations. It is optional but {ld.lld,ld.lld}
> > -z combreloc always creates it (if non-zero) to slightly speed up glibc
> > ld.so relocation resolving by avoiding R_*R_PPC64_RELATIVE type
> > comparison. The tag is otherwise nearly unused in the wild and I'd
> > recommend that software avoids using it.
> >
> > lld>=14.0.0 (since commit da0e5b885b25cf4ded0fa89b965dc6979ac02ca9)
> > underestimates DT_RELACOUNT for ppc64 when position-independent long
> > branch thunks are used. Correcting it needs non-trivial arch-specific
> > complexity which I'd prefer to avoid. Since our code always compares the
> > relocation type with R_PPC64_RELATIVE, replacing every occurrence of
> > DT_RELACOUNT with DT_RELASZ/sizeof(Elf64_Rela)=DT_RELASZ/24 is a correct
> > alternative.
>
> checking that sizeof(Elf64_Rela) == 24, yep: https://godbolt.org/z/bb4aKbo5T
>
> >
> > DT_RELASZ is in practice bounded by an uint32_t. Dividing x by 24 can be
> > implemented as (uint32_t)(x*0xaaaaaaab) >> 4.
>
> Yep: https://godbolt.org/z/x9445ePPv
>
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/1581
> > Reported-by: Nathan Chancellor <nathan@kernel.org>
> > Signed-off-by: Fangrui Song <maskray@google.com>
> > ---
> > arch/powerpc/boot/crt0.S | 28 +++++++++++++++++-----------
> > arch/powerpc/kernel/reloc_64.S | 15 +++++++++------
> > 2 files changed, 26 insertions(+), 17 deletions(-)
> >
> > diff --git a/arch/powerpc/boot/crt0.S b/arch/powerpc/boot/crt0.S
> > index feadee18e271..1c96ebe7ef1a 100644
> > --- a/arch/powerpc/boot/crt0.S
> > +++ b/arch/powerpc/boot/crt0.S
> > @@ -8,7 +8,7 @@
> > #include "ppc_asm.h"
> >
> > RELA = 7
> > -RELACOUNT = 0x6ffffff9
> > +RELASZ = 8
> >
> > .data
> > /* A procedure descriptor used when booting this as a COFF file.
> > @@ -65,7 +65,7 @@ p_base: mflr r10 /* r10 now points to runtime addr of p_base */
> > subf r11,r11,r12 /* runtime - linktime offset */
> >
> > /* The dynamic section contains a series of tagged entries.
> > - * We need the RELA and RELACOUNT entries. */
> > + * We need the RELA and RELASZ entries. */
> > li r9,0
> > li r0,0
> > 9: lwz r8,0(r12) /* get tag */
> > @@ -75,18 +75,21 @@ p_base: mflr r10 /* r10 now points to runtime addr of p_base */
> > bne 11f
> > lwz r9,4(r12) /* get RELA pointer in r9 */
> > b 12f
> > -11: addis r8,r8,(-RELACOUNT)@ha
> > - cmpwi r8,RELACOUNT@l
> > +11: cmpwi r8,RELASZ
> > bne 12f
> > - lwz r0,4(r12) /* get RELACOUNT value in r0 */
> > + lwz r0,4(r12) /* get RELASZ / 24 in r0 */
> > + lis r8,0xaaaa
> > + ori r8,r8,0xaaab
> > + mulhwu r0,r0,r8
> > + srwi r0,r0,4
>
> Both translation units include arch/powerpc/include/asm/ppc_asm.h,
> which happens to define LOAD_REG_IMMEDIATE. Let's reuse that rather
> than open code lis+ori?
>
> > 12: addi r12,r12,8
> > b 9b
> >
> > /* The relocation section contains a list of relocations.
> > * We now do the R_PPC_RELATIVE ones, which point to words
> > * which need to be initialized with addend + offset.
> > - * The R_PPC_RELATIVE ones come first and there are RELACOUNT
> > - * of them. */
> > + * The R_PPC_RELATIVE ones come first and there are at most
> > + * RELASZ/24 of them. */
> > 10: /* skip relocation if we don't have both */
> > cmpwi r0,0
> > beq 3f
> > @@ -160,14 +163,17 @@ p_base: mflr r10 /* r10 now points to runtime addr of p_base */
> > bne 10f
> > ld r13,8(r11) /* get RELA pointer in r13 */
> > b 11f
> > -10: addis r12,r12,(-RELACOUNT)@ha
> > - cmpdi r12,RELACOUNT@l
> > +10: cmpdi r12,RELASZ
> > bne 11f
> > - ld r8,8(r11) /* get RELACOUNT value in r8 */
> > + ld r8,8(r11) /* get RELASZ / 24 in r8 */
> > + lis r0,0xaaaa
> > + ori r0,r0,0xaaab
> > + mulhwu r8,r8,r0
> > + srwi r8,r8,4
> > 11: addi r11,r11,16
> > b 9b
> > 12:
> > - cmpdi r13,0 /* check we have both RELA and RELACOUNT */
> > + cmpdi r13,0 /* check we have both RELA and RELASZ */
> > cmpdi cr1,r8,0
> > beq 3f
> > beq cr1,3f
> > diff --git a/arch/powerpc/kernel/reloc_64.S b/arch/powerpc/kernel/reloc_64.S
> > index 02d4719bf43a..362be759609f 100644
> > --- a/arch/powerpc/kernel/reloc_64.S
> > +++ b/arch/powerpc/kernel/reloc_64.S
> > @@ -8,7 +8,7 @@
> > #include <asm/ppc_asm.h>
> >
> > RELA = 7
> > -RELACOUNT = 0x6ffffff9
> > +RELASZ = 8
> > R_PPC64_RELATIVE = 22
> >
> > /*
> > @@ -27,7 +27,7 @@ _GLOBAL(relocate)
> > add r10,r10,r12 /* r10 has runtime addr of _stext */
> >
> > /*
> > - * Scan the dynamic section for the RELA and RELACOUNT entries.
> > + * Scan the dynamic section for the RELA and RELASZ entries.
> > */
> > li r7,0
> > li r8,0
> > @@ -38,13 +38,16 @@ _GLOBAL(relocate)
> > bne 2f
> > ld r7,8(r11) /* get RELA pointer in r7 */
> > b 3f
> > -2: addis r6,r6,(-RELACOUNT)@ha
> > - cmpdi r6,RELACOUNT@l
> > +2: cmpdi r6,RELASZ
> > bne 3f
> > - ld r8,8(r11) /* get RELACOUNT value in r8 */
> > + ld r8,8(r11) /* get RELA / 24 in r8 */
> > + lis r0,0xaaaa
> > + ori r0,r0,0xaaab
> > + mulhwu r8,r8,r0
> > + srwi r8,r8,4
> > 3: addi r11,r11,16
> > b 1b
> > -4: cmpdi r7,0 /* check we have both RELA and RELACOUNT */
> > +4: cmpdi r7,0 /* check we have both RELA and RELASZ */
> > cmpdi cr1,r8,0
> > beq 6f
> > beq cr1,6f
> > --
> > 2.35.1.616.g0bdcbb4464-goog
> >
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers
>
I rebased the patch on
git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
master and got a conflict.
Seems that https://lore.kernel.org/linuxppc-dev/20220309061822.168173-1-aik@ozlabs.ru/T/#u
("[PATCH kernel v4] powerpc/64: Add UADDR64 relocation support") fixed
the issue.
It just doesn't change arch/powerpc/boot/crt0.S
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc: Replace ppc64 DT_RELACOUNT usage with DT_RELASZ/24
2022-03-10 21:08 ` Fāng-ruì Sòng
@ 2022-03-11 4:15 ` Michael Ellerman
2022-03-11 4:35 ` Alexey Kardashevskiy
0 siblings, 1 reply; 6+ messages in thread
From: Michael Ellerman @ 2022-03-11 4:15 UTC (permalink / raw)
To: Fāng-ruì Sòng, Alexey Kardashevskiy
Cc: Nick Desaulniers, Benjamin Herrenschmidt, linuxppc-dev,
Nathan Chancellor, linux-kernel, llvm
Fāng-ruì Sòng <maskray@google.com> writes:
> On Thu, Mar 10, 2022 at 11:48 AM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
>>
>> On Tue, Mar 8, 2022 at 9:53 PM Fangrui Song <maskray@google.com> wrote:
>> >
>> > DT_RELACOUNT is an ELF dynamic tag inherited from SunOS indicating the
>> > number of R_*_RELATIVE relocations. It is optional but {ld.lld,ld.lld}
>> > -z combreloc always creates it (if non-zero) to slightly speed up glibc
>> > ld.so relocation resolving by avoiding R_*R_PPC64_RELATIVE type
>> > comparison. The tag is otherwise nearly unused in the wild and I'd
>> > recommend that software avoids using it.
>> >
>> > lld>=14.0.0 (since commit da0e5b885b25cf4ded0fa89b965dc6979ac02ca9)
>> > underestimates DT_RELACOUNT for ppc64 when position-independent long
>> > branch thunks are used. Correcting it needs non-trivial arch-specific
>> > complexity which I'd prefer to avoid. Since our code always compares the
>> > relocation type with R_PPC64_RELATIVE, replacing every occurrence of
>> > DT_RELACOUNT with DT_RELASZ/sizeof(Elf64_Rela)=DT_RELASZ/24 is a correct
>> > alternative.
>>
>> checking that sizeof(Elf64_Rela) == 24, yep: https://godbolt.org/z/bb4aKbo5T
>>
>> >
>> > DT_RELASZ is in practice bounded by an uint32_t. Dividing x by 24 can be
>> > implemented as (uint32_t)(x*0xaaaaaaab) >> 4.
>>
>> Yep: https://godbolt.org/z/x9445ePPv
>>
>> >
>> > Link: https://github.com/ClangBuiltLinux/linux/issues/1581
>> > Reported-by: Nathan Chancellor <nathan@kernel.org>
>> > Signed-off-by: Fangrui Song <maskray@google.com>
>> > ---
>> > arch/powerpc/boot/crt0.S | 28 +++++++++++++++++-----------
>> > arch/powerpc/kernel/reloc_64.S | 15 +++++++++------
>> > 2 files changed, 26 insertions(+), 17 deletions(-)
...
> I rebased the patch on
> git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> master and got a conflict.
> Seems that https://lore.kernel.org/linuxppc-dev/20220309061822.168173-1-aik@ozlabs.ru/T/#u
> ("[PATCH kernel v4] powerpc/64: Add UADDR64 relocation support") fixed
> the issue.
> It just doesn't change arch/powerpc/boot/crt0.S
Yeah sorry, I applied Alexey's v4 just before I saw your patch arrive on
the list.
If one of you can rework this so it applies on top that would be great :)
cheers
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc: Replace ppc64 DT_RELACOUNT usage with DT_RELASZ/24
2022-03-11 4:15 ` Michael Ellerman
@ 2022-03-11 4:35 ` Alexey Kardashevskiy
0 siblings, 0 replies; 6+ messages in thread
From: Alexey Kardashevskiy @ 2022-03-11 4:35 UTC (permalink / raw)
To: Michael Ellerman, Fāng-ruì Sòng
Cc: Nick Desaulniers, Benjamin Herrenschmidt, linuxppc-dev,
Nathan Chancellor, linux-kernel, llvm
On 3/11/22 15:15, Michael Ellerman wrote:
> Fāng-ruì Sòng <maskray@google.com> writes:
>> On Thu, Mar 10, 2022 at 11:48 AM Nick Desaulniers
>> <ndesaulniers@google.com> wrote:
>>>
>>> On Tue, Mar 8, 2022 at 9:53 PM Fangrui Song <maskray@google.com> wrote:
>>>>
>>>> DT_RELACOUNT is an ELF dynamic tag inherited from SunOS indicating the
>>>> number of R_*_RELATIVE relocations. It is optional but {ld.lld,ld.lld}
>>>> -z combreloc always creates it (if non-zero) to slightly speed up glibc
>>>> ld.so relocation resolving by avoiding R_*R_PPC64_RELATIVE type
>>>> comparison. The tag is otherwise nearly unused in the wild and I'd
>>>> recommend that software avoids using it.
>>>>
>>>> lld>=14.0.0 (since commit da0e5b885b25cf4ded0fa89b965dc6979ac02ca9)
>>>> underestimates DT_RELACOUNT for ppc64 when position-independent long
>>>> branch thunks are used. Correcting it needs non-trivial arch-specific
>>>> complexity which I'd prefer to avoid. Since our code always compares the
>>>> relocation type with R_PPC64_RELATIVE, replacing every occurrence of
>>>> DT_RELACOUNT with DT_RELASZ/sizeof(Elf64_Rela)=DT_RELASZ/24 is a correct
>>>> alternative.
>>>
>>> checking that sizeof(Elf64_Rela) == 24, yep: https://godbolt.org/z/bb4aKbo5T
>>>
>>>>
>>>> DT_RELASZ is in practice bounded by an uint32_t. Dividing x by 24 can be
>>>> implemented as (uint32_t)(x*0xaaaaaaab) >> 4.
>>>
>>> Yep: https://godbolt.org/z/x9445ePPv
>>>
>>>>
>>>> Link: https://github.com/ClangBuiltLinux/linux/issues/1581
>>>> Reported-by: Nathan Chancellor <nathan@kernel.org>
>>>> Signed-off-by: Fangrui Song <maskray@google.com>
>>>> ---
>>>> arch/powerpc/boot/crt0.S | 28 +++++++++++++++++-----------
>>>> arch/powerpc/kernel/reloc_64.S | 15 +++++++++------
>>>> 2 files changed, 26 insertions(+), 17 deletions(-)
> ...
>
>> I rebased the patch on
>> git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
>> master and got a conflict.
>> Seems that https://lore.kernel.org/linuxppc-dev/20220309061822.168173-1-aik@ozlabs.ru/T/#u
>> ("[PATCH kernel v4] powerpc/64: Add UADDR64 relocation support") fixed
>> the issue.
>> It just doesn't change arch/powerpc/boot/crt0.S
>
> Yeah sorry, I applied Alexey's v4 just before I saw your patch arrive on
> the list.
>
> If one of you can rework this so it applies on top that would be great :)
I guess it is me as now I have to add that UARRD64 thing to crt0.S as
well, don't I?
And also we are giving up on the llvm ld having a bug with RELACOUNT?
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-03-11 4:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-09 5:51 [PATCH] powerpc: Replace ppc64 DT_RELACOUNT usage with DT_RELASZ/24 Fangrui Song
2022-03-09 19:01 ` Nathan Chancellor
2022-03-10 19:48 ` Nick Desaulniers
2022-03-10 21:08 ` Fāng-ruì Sòng
2022-03-11 4:15 ` Michael Ellerman
2022-03-11 4:35 ` Alexey Kardashevskiy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).