Re: [PATCH v4.2] powerpc/modules: Don't try to restore r2 after a sibling call

From: Naveen N. Rao
Date: Sat Nov 18 2017 - 03:33:56 EST

Kamalesh Babulal wrote:
On Thursday 16 November 2017 11:15 PM, Josh Poimboeuf wrote:
On Thu, Nov 16, 2017 at 06:39:03PM +0530, Naveen N. Rao wrote:
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

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.

Hm, RISC is more complicated than I realized ;-)

As long as 'RISC' gets people to take a look ;D

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));

Yeah, makes sense to me. Here's another try (also untested). If this
looks ok, Kamalesh would you mind testing again?

Thanks. That looks good to me.
Acked-by: Naveen N. Rao <naveen.n.rao@xxxxxxxxxxxxxxxxxx>


From: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
Subject: [PATCH v4.2] powerpc/modules: Don't try to restore r2 after a sibling call

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

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@xxxxxxxxxx>

Reviewed-and-tested-by: Kamalesh Babulal <kamalesh@xxxxxxxxxxxxxxxxxx>

Thanks, Kamalesh!

- Naveen