Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220309055118.1551013-1-maskray@google.com> <CAKwvOdmMS4=QAoBFvhAdXWaLHOwH2252FX9i_yZyiCOpOt=3Dw@mail.gmail.com>
In-Reply-To: <CAKwvOdmMS4=QAoBFvhAdXWaLHOwH2252FX9i_yZyiCOpOt=3Dw@mail.gmail.com>
From:   =?UTF-8?B?RsSBbmctcnXDrCBTw7JuZw==?= <maskray@google.com>
Date:   Thu, 10 Mar 2022 13:08:08 -0800
Message-ID: <CAFP8O3+jJf=amTYjm6YQbJKAuK0XRNoG3Gwc6C+E0=CPd46ZAw@mail.gmail.com>
Subject: Re: [PATCH] powerpc: Replace ppc64 DT_RELACOUNT usage with DT_RELASZ/24
To:     Alexey Kardashevskiy <aik@ozlabs.ru>
Cc:     Michael Ellerman <mpe@ellerman.id.au>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        linuxppc-dev@lists.ozlabs.org,
        Nathan Chancellor <nathan@kernel.org>,
        linux-kernel@vger.kernel.org, llvm@lists.linux.dev
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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