MIME-Version: 1.0
References: <cover.1674617130.git.jpoimboe@kernel.org> <2f6329ffd9674df6ff57e03edeb2ca54414770ab.1674617130.git.jpoimboe@kernel.org>
 <CAPhsuW40jEiyp0ogsO6oH_frpFCmiioSHrMOKkwGcZ8_6w5dZA@mail.gmail.com>
 <20230125164609.wvuarciciyoqa3tb@treble> <CAPhsuW45k8Avx=Zfid1pxaeHAbLGgOcxbN_=DQOb8WdPx7fB+Q@mail.gmail.com>
 <20230125185316.ebvxecd7gsvgtudr@treble>
In-Reply-To: <20230125185316.ebvxecd7gsvgtudr@treble>
From:   Song Liu <song@kernel.org>
Date:   Wed, 25 Jan 2023 10:58:57 -0800
Message-ID: <CAPhsuW56Ztn7JGci7=tPO__EFTNP0Ccp-WSNRUnQ2e81kBZMNw@mail.gmail.com>
Subject: Re: [PATCH 2/2] powerpc/module_64: Fix "expected nop" error on module re-patching
To:     Josh Poimboeuf <jpoimboe@kernel.org>
Cc:     live-patching@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
        linux-kernel@vger.kernel.org, Nicholas Piggin <npiggin@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Jan 25, 2023 at 10:53 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> On Wed, Jan 25, 2023 at 09:36:02AM -0800, Song Liu wrote:
> > On Wed, Jan 25, 2023 at 8:46 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> > >
> > > On Tue, Jan 24, 2023 at 10:09:56PM -0800, Song Liu wrote:
> > > > > @@ -514,9 +515,18 @@ static int restore_r2(const char *name, u32 *instruction, struct module *me)
> > > > >         if (!instr_is_relative_link_branch(ppc_inst(*prev_insn)))
> > > > >                 return 0;
> > > > >
> > > > > -       if (*instruction != PPC_RAW_NOP()) {
> > > > > +       /*
> > > > > +        * For livepatch, the restore r2 instruction might have already been
> > > > > +        * written previously, if the referenced symbol is in a previously
> > > > > +        * unloaded module which is now being loaded again.  In that case, skip
> > > > > +        * the warning and the instruction write.
> > > > > +        */
> > > > > +       if (insn_val == PPC_INST_LD_TOC)
> > > > > +               return 0;
> > > >
> > > > Do we need "sym->st_shndx == SHN_LIVEPATCH" here?
> > >
> > > My original patch had that check, but I dropped it for simplicity.
> > >
> > > In the non-livepatch case, the condition should never be true, but it
> > > doesn't hurt to check it anyway.
> >
> > While this is the only place we use PPC_INST_LD_TOC, there is another
> > place we use "PPC_RAW_STD(_R2, _R1, R2_STACK_OFFSET)", which
> > is identical to PPC_INST_LD_TOC. So I am not quite sure whether this
> > happens for non-livepatch.
>
> It's not actually identical.  That's the "store r2 to the stack"
> counterpart to the load in PPC_INST_LD_TOC, which loads r2 from the
> stack.

Ooops.. I misread the code.

>
> For R_PPC_REL24 relocations, when calling a function which lives outside
> the module, 24 bits isn't enough to encode the relative branch target
> address.  So it has to save r2 (TOC pointer) to the stack, and branch to
> a stub, which then branches to the external function.
>
> When the external function returns execution to the instruction after
> the original branch, that instruction needs to restore the TOC pointer
> from the stack to r2.
>
> The compiler knows this, and emits the instruction after the branch as a
> NOP.  The module code replaces that NOP with a "restore r2 from the
> stack".  That's what restore_r2() does.
>
> Long story short, restore_r2() needs to ensure the instruction after the
> branch restores r2 from the stack.  If that instruction is already
> there, it doesn't need to do anything.

Thanks for the explanation!

Acked-by: Song Liu <song@kernel.org>