From: Nick Desaulniers <ndesaulniers@google.com>
To: Fangrui Song <maskray@google.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>,
Benjamin Herrenschmidt <benh@kernel.crashing.org>,
Alexey Kardashevskiy <aik@ozlabs.ru>,
linuxppc-dev@lists.ozlabs.org,
Nathan Chancellor <nathan@kernel.org>,
linux-kernel@vger.kernel.org, llvm@lists.linux.dev
Subject: Re: [PATCH] powerpc: Replace ppc64 DT_RELACOUNT usage with DT_RELASZ/24
Date: Thu, 10 Mar 2022 11:48:40 -0800 [thread overview]
Message-ID: <CAKwvOdmMS4=QAoBFvhAdXWaLHOwH2252FX9i_yZyiCOpOt=3Dw@mail.gmail.com> (raw)
In-Reply-To: <20220309055118.1551013-1-maskray@google.com>
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
next prev parent reply other threads:[~2022-03-10 19:48 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2022-03-10 21:08 ` Fāng-ruì Sòng
2022-03-11 4:15 ` Michael Ellerman
2022-03-11 4:35 ` Alexey Kardashevskiy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAKwvOdmMS4=QAoBFvhAdXWaLHOwH2252FX9i_yZyiCOpOt=3Dw@mail.gmail.com' \
--to=ndesaulniers@google.com \
--cc=aik@ozlabs.ru \
--cc=benh@kernel.crashing.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=llvm@lists.linux.dev \
--cc=maskray@google.com \
--cc=mpe@ellerman.id.au \
--cc=nathan@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).