Message ID | 20220309055118.1551013-1-maskray@google.com (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | powerpc: Replace ppc64 DT_RELACOUNT usage with DT_RELASZ/24 | expand |
Context | Check | Description |
---|---|---|
snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 7 jobs. |
snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 24 jobs. |
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 >
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 > >
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
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
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?
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
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(-)