| Message ID | 20171114092910.20399-3-kamalesh@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ppc64le: Add REL24 relocation support of livepatch symbols | expand |
Kamalesh Babulal wrote: > From: Josh Poimboeuf <jpoimboe@redhat.com> > > When attempting to load a livepatch module, I got the following error: > > module_64: patch_module: Expect noop after relocate, got 3c820000 > > The error was triggered by the following code in > unregister_netdevice_queue(): > > 14c: 00 00 00 48 b 14c <unregister_netdevice_queue+0x14c> > 14c: R_PPC64_REL24 net_set_todo > 150: 00 00 82 3c addis r4,r2,0 > > GCC didn't insert a nop after the branch to net_set_todo() because it's > a sibling call, so it never returns. The nop isn't needed after the > branch in that case. > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com> > Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com> > --- > arch/powerpc/kernel/module_64.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c > index 39b01fd..9e5391f 100644 > --- a/arch/powerpc/kernel/module_64.c > +++ b/arch/powerpc/kernel/module_64.c > @@ -489,6 +489,10 @@ static int restore_r2(u32 *instruction, struct module *me) > if (is_early_mcount_callsite(instruction - 1)) > return 1; > > + /* Sibling calls don't return, so they don't need to restore r2 */ > + if (instruction[-1] == PPC_INST_BRANCH) > + return 1; > + This looks quite fragile, unless we know for sure that gcc will _always_ emit this instruction form for sibling calls with relocations. As an alternative, does it make sense to do the following check instead? if ((instr_is_branch_iform(insn) || instr_is_branch_bform(insn)) && !(insn & 0x1)) - Naveen
On Tue, Nov 14, 2017 at 03:59:21PM +0530, Naveen N. Rao wrote: > Kamalesh Babulal wrote: > > From: Josh Poimboeuf <jpoimboe@redhat.com> > > > > When attempting to load a livepatch module, I got the following error: > > > > module_64: patch_module: Expect noop after relocate, got 3c820000 > > > > The error was triggered by the following code in > > unregister_netdevice_queue(): > > > > 14c: 00 00 00 48 b 14c <unregister_netdevice_queue+0x14c> > > 14c: R_PPC64_REL24 net_set_todo > > 150: 00 00 82 3c addis r4,r2,0 > > > > GCC didn't insert a nop after the branch to net_set_todo() because it's > > a sibling call, so it never returns. The nop isn't needed after the > > branch in that case. > > > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com> > > Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com> > > --- > > arch/powerpc/kernel/module_64.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c > > index 39b01fd..9e5391f 100644 > > --- a/arch/powerpc/kernel/module_64.c > > +++ b/arch/powerpc/kernel/module_64.c > > @@ -489,6 +489,10 @@ static int restore_r2(u32 *instruction, struct module *me) > > if (is_early_mcount_callsite(instruction - 1)) > > return 1; > > > > + /* Sibling calls don't return, so they don't need to restore r2 */ > > + if (instruction[-1] == PPC_INST_BRANCH) > > + return 1; > > + > > This looks quite fragile, unless we know for sure that gcc will _always_ > emit this instruction form for sibling calls with relocations. > > As an alternative, does it make sense to do the following check instead? > if ((instr_is_branch_iform(insn) || instr_is_branch_bform(insn)) > && !(insn & 0x1)) Yes, good point. How about something like this? (completely untested because I don't have access to a box at the moment) diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h index abef812de7f8..302e4368debc 100644 --- a/arch/powerpc/include/asm/code-patching.h +++ b/arch/powerpc/include/asm/code-patching.h @@ -33,6 +33,7 @@ int patch_branch(unsigned int *addr, unsigned long target, int flags); int patch_instruction(unsigned int *addr, unsigned int instr); int instr_is_relative_branch(unsigned int instr); +int instr_is_link_branch(unsigned int instr); int instr_is_branch_to_addr(const unsigned int *instr, unsigned long addr); unsigned long branch_target(const unsigned int *instr); unsigned int translate_branch(const unsigned int *dest, diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c index 9cb007bc7075..b5148a206b4d 100644 --- a/arch/powerpc/kernel/module_64.c +++ b/arch/powerpc/kernel/module_64.c @@ -487,11 +487,13 @@ static bool is_early_mcount_callsite(u32 *instruction) restore r2. */ static int restore_r2(u32 *instruction, struct module *me) { - if (is_early_mcount_callsite(instruction - 1)) + u32 *prev_insn = instruction - 1; + + if (is_early_mcount_callsite(prev_insn)) return 1; /* Sibling calls don't return, so they don't need to restore r2 */ - if (instruction[-1] == PPC_INST_BRANCH) + if (!instr_is_link_branch(*prev_insn)) return 1; if (*instruction != PPC_INST_NOP) { diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c index c9de03e0c1f1..4727fafd37e4 100644 --- a/arch/powerpc/lib/code-patching.c +++ b/arch/powerpc/lib/code-patching.c @@ -304,6 +304,12 @@ int instr_is_relative_branch(unsigned int instr) return instr_is_branch_iform(instr) || instr_is_branch_bform(instr); } +int instr_is_link_branch(unsigned int instr) +{ + return (instr_is_branch_iform(instr) || instr_is_branch_bform(instr)) && + (instr & BRANCH_SET_LINK); +} + static unsigned long branch_iform_target(const unsigned int *instr) { signed long imm;
On Tuesday 14 November 2017 09:23 PM, Josh Poimboeuf wrote: > On Tue, Nov 14, 2017 at 03:59:21PM +0530, Naveen N. Rao wrote: >> Kamalesh Babulal wrote: >>> From: Josh Poimboeuf <jpoimboe@redhat.com> >>> >>> When attempting to load a livepatch module, I got the following error: >>> >>> module_64: patch_module: Expect noop after relocate, got 3c820000 >>> >>> The error was triggered by the following code in >>> unregister_netdevice_queue(): >>> >>> 14c: 00 00 00 48 b 14c <unregister_netdevice_queue+0x14c> >>> 14c: R_PPC64_REL24 net_set_todo >>> 150: 00 00 82 3c addis r4,r2,0 >>> >>> GCC didn't insert a nop after the branch to net_set_todo() because it's >>> a sibling call, so it never returns. The nop isn't needed after the >>> branch in that case. >>> >>> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com> >>> Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com> >>> --- >>> arch/powerpc/kernel/module_64.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c >>> index 39b01fd..9e5391f 100644 >>> --- a/arch/powerpc/kernel/module_64.c >>> +++ b/arch/powerpc/kernel/module_64.c >>> @@ -489,6 +489,10 @@ static int restore_r2(u32 *instruction, struct module *me) >>> if (is_early_mcount_callsite(instruction - 1)) >>> return 1; >>> >>> + /* Sibling calls don't return, so they don't need to restore r2 */ >>> + if (instruction[-1] == PPC_INST_BRANCH) >>> + return 1; >>> + >> >> This looks quite fragile, unless we know for sure that gcc will _always_ >> emit this instruction form for sibling calls with relocations. >> >> As an alternative, does it make sense to do the following check instead? >> if ((instr_is_branch_iform(insn) || instr_is_branch_bform(insn)) >> && !(insn & 0x1)) > > Yes, good point. How about something like this? > > (completely untested because I don't have access to a box at the moment) Reviewed-and-tested-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com> > > > diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h > index abef812de7f8..302e4368debc 100644 > --- a/arch/powerpc/include/asm/code-patching.h > +++ b/arch/powerpc/include/asm/code-patching.h > @@ -33,6 +33,7 @@ int patch_branch(unsigned int *addr, unsigned long target, int flags); > int patch_instruction(unsigned int *addr, unsigned int instr); > > int instr_is_relative_branch(unsigned int instr); > +int instr_is_link_branch(unsigned int instr); > int instr_is_branch_to_addr(const unsigned int *instr, unsigned long addr); > unsigned long branch_target(const unsigned int *instr); > unsigned int translate_branch(const unsigned int *dest, > diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c > index 9cb007bc7075..b5148a206b4d 100644 > --- a/arch/powerpc/kernel/module_64.c > +++ b/arch/powerpc/kernel/module_64.c > @@ -487,11 +487,13 @@ static bool is_early_mcount_callsite(u32 *instruction) > restore r2. */ > static int restore_r2(u32 *instruction, struct module *me) > { > - if (is_early_mcount_callsite(instruction - 1)) > + u32 *prev_insn = instruction - 1; > + > + if (is_early_mcount_callsite(prev_insn)) > return 1; > > /* Sibling calls don't return, so they don't need to restore r2 */ > - if (instruction[-1] == PPC_INST_BRANCH) > + if (!instr_is_link_branch(*prev_insn)) > return 1; > > if (*instruction != PPC_INST_NOP) { > diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c > index c9de03e0c1f1..4727fafd37e4 100644 > --- a/arch/powerpc/lib/code-patching.c > +++ b/arch/powerpc/lib/code-patching.c > @@ -304,6 +304,12 @@ int instr_is_relative_branch(unsigned int instr) > return instr_is_branch_iform(instr) || instr_is_branch_bform(instr); > } > > +int instr_is_link_branch(unsigned int instr) > +{ > + return (instr_is_branch_iform(instr) || instr_is_branch_bform(instr)) && > + (instr & BRANCH_SET_LINK); > +} > + > static unsigned long branch_iform_target(const unsigned int *instr) > { > signed long imm; >
Josh Poimboeuf wrote: > On Tue, Nov 14, 2017 at 03:59:21PM +0530, Naveen N. Rao wrote: >> Kamalesh Babulal wrote: >> > From: Josh Poimboeuf <jpoimboe@redhat.com> >> > >> > When attempting to load a livepatch module, I got the following error: >> > >> > module_64: patch_module: Expect noop after relocate, got 3c820000 >> > >> > The error was triggered by the following code in >> > unregister_netdevice_queue(): >> > >> > 14c: 00 00 00 48 b 14c <unregister_netdevice_queue+0x14c> >> > 14c: R_PPC64_REL24 net_set_todo >> > 150: 00 00 82 3c addis r4,r2,0 >> > >> > GCC didn't insert a nop after the branch to net_set_todo() because it's >> > a sibling call, so it never returns. The nop isn't needed after the >> > branch in that case. >> > >> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com> >> > Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com> >> > --- >> > arch/powerpc/kernel/module_64.c | 4 ++++ >> > 1 file changed, 4 insertions(+) >> > >> > diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c >> > index 39b01fd..9e5391f 100644 >> > --- a/arch/powerpc/kernel/module_64.c >> > +++ b/arch/powerpc/kernel/module_64.c >> > @@ -489,6 +489,10 @@ static int restore_r2(u32 *instruction, struct module *me) >> > if (is_early_mcount_callsite(instruction - 1)) >> > return 1; >> > >> > + /* Sibling calls don't return, so they don't need to restore r2 */ >> > + if (instruction[-1] == PPC_INST_BRANCH) >> > + return 1; >> > + >> >> This looks quite fragile, unless we know for sure that gcc will _always_ >> emit this instruction form for sibling calls with relocations. >> >> As an alternative, does it make sense to do the following check instead? >> if ((instr_is_branch_iform(insn) || instr_is_branch_bform(insn)) >> && !(insn & 0x1)) > > Yes, good point. How about something like this? That looks good to me (with a very minor nit below). Acked-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> > > (completely untested because I don't have access to a box at the moment) > > > diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h > index abef812de7f8..302e4368debc 100644 > --- a/arch/powerpc/include/asm/code-patching.h > +++ b/arch/powerpc/include/asm/code-patching.h > @@ -33,6 +33,7 @@ int patch_branch(unsigned int *addr, unsigned long target, int flags); > int patch_instruction(unsigned int *addr, unsigned int instr); > > int instr_is_relative_branch(unsigned int instr); > +int instr_is_link_branch(unsigned int instr); > int instr_is_branch_to_addr(const unsigned int *instr, unsigned long addr); > unsigned long branch_target(const unsigned int *instr); > unsigned int translate_branch(const unsigned int *dest, > diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c > index 9cb007bc7075..b5148a206b4d 100644 > --- a/arch/powerpc/kernel/module_64.c > +++ b/arch/powerpc/kernel/module_64.c > @@ -487,11 +487,13 @@ static bool is_early_mcount_callsite(u32 *instruction) > restore r2. */ > static int restore_r2(u32 *instruction, struct module *me) > { > - if (is_early_mcount_callsite(instruction - 1)) > + u32 *prev_insn = instruction - 1; > + > + if (is_early_mcount_callsite(prev_insn)) > return 1; > > /* Sibling calls don't return, so they don't need to restore r2 */ > - if (instruction[-1] == PPC_INST_BRANCH) > + if (!instr_is_link_branch(*prev_insn)) > return 1; > > if (*instruction != PPC_INST_NOP) { > diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c > index c9de03e0c1f1..4727fafd37e4 100644 > --- a/arch/powerpc/lib/code-patching.c > +++ b/arch/powerpc/lib/code-patching.c > @@ -304,6 +304,12 @@ int instr_is_relative_branch(unsigned int instr) > return instr_is_branch_iform(instr) || instr_is_branch_bform(instr); > } > > +int instr_is_link_branch(unsigned int instr) > +{ > + return (instr_is_branch_iform(instr) || instr_is_branch_bform(instr)) && > + (instr & BRANCH_SET_LINK); > +} > + Nitpicking here, but since we're not considering the other branch forms, perhaps this can be renamed to instr_is_link_relative_branch() (or maybe instr_is_relative_branch_link()), just so we're clear :) - Naveen > static unsigned long branch_iform_target(const unsigned int *instr) > { > signed long imm; > -- > To unsubscribe from this list: send the line "unsubscribe live-patching" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > >
On Wed, Nov 15, 2017 at 02:58:33PM +0530, Naveen N. Rao wrote: > > +int instr_is_link_branch(unsigned int instr) > > +{ > > + return (instr_is_branch_iform(instr) || instr_is_branch_bform(instr)) && > > + (instr & BRANCH_SET_LINK); > > +} > > + > > Nitpicking here, but since we're not considering the other branch forms, > perhaps this can be renamed to instr_is_link_relative_branch() (or maybe > instr_is_relative_branch_link()), just so we're clear :) My understanding is that the absolute/relative bit isn't a "form", but rather a bit that can be set for either the b-form (conditional) or the i-form (unconditional). And the above function isn't checking the absolute bit, so it isn't necessarily a relative branch. Or did I miss something?
Josh Poimboeuf wrote: > On Wed, Nov 15, 2017 at 02:58:33PM +0530, Naveen N. Rao wrote: >> > +int instr_is_link_branch(unsigned int instr) >> > +{ >> > + return (instr_is_branch_iform(instr) || instr_is_branch_bform(instr)) && >> > + (instr & BRANCH_SET_LINK); >> > +} >> > + >> >> Nitpicking here, but since we're not considering the other branch forms, >> perhaps this can be renamed to instr_is_link_relative_branch() (or maybe >> instr_is_relative_branch_link()), just so we're clear :) > > My understanding is that the absolute/relative bit isn't a "form", but > rather a bit that can be set for either the b-form (conditional) or the > i-form (unconditional). And the above function isn't checking the > absolute bit, so it isn't necessarily a relative branch. Or did I miss > something? Ah, good point. I was coming from the fact that we are only considering the i-form and b-form branches and not the lr/ctr/tar based branches, which are always absolute branches, but can also set the link register. Thinking about this more, aren't we only interested in relative branches here (for relocations), so can we actually filter out the absolute branches? Something like this? int instr_is_relative_branch_link(unsigned int instr) { return ((instr_is_branch_iform(instr) || instr_is_branch_bform(instr)) && !(instr & BRANCH_ABSOLUTE) && (instr & BRANCH_SET_LINK)); } - Naveen
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c index 39b01fd..9e5391f 100644 --- a/arch/powerpc/kernel/module_64.c +++ b/arch/powerpc/kernel/module_64.c @@ -489,6 +489,10 @@ static int restore_r2(u32 *instruction, struct module *me) if (is_early_mcount_callsite(instruction - 1)) return 1; + /* Sibling calls don't return, so they don't need to restore r2 */ + if (instruction[-1] == PPC_INST_BRANCH) + return 1; + if (*instruction != PPC_INST_NOP) { pr_err("%s: Expect noop after relocate, got %08x\n", me->name, *instruction);