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
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.
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....@linux.vnet.ibm.com>
----8<----
From: Josh Poimboeuf <jpoim...@redhat.com>
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
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 <jpoim...@redhat.com>
Reviewed-and-tested-by: Kamalesh Babulal <kamal...@linux.vnet.ibm.com>
Thanks, Kamalesh!
- Naveen
