Hi Balbir, Some comments inline ... On Thu, 2016-02-25 at 23:11 +1100, Balbir Singh wrote: > This applies on top of the patches posted by Michael today > Enable livepatching. This takes patch 6/8 and 7/8 of v8 as the base. > Removes the extra strict check in gcc-profile-kernel-notrace.sh > and adds logic for checking offsets in livepatch. The patch > for HAVE_C_RECORDMCOUNT is not required and not used here. > > Depending on whether or not a TOC is generated, the offset > for _mcount can be +16 or +8. The changes are such that the > offset checks are specific to powerpc. > > Comments? Testing? I tested the sample in the livepatch > directory You forgot to CC linuxppc-dev :) > References > > 1. https://patchwork.ozlabs.org/patch/581521/ > 2. https://patchwork.ozlabs.org/patch/587464/ > > Signed-off-by: Torsten Duwe <duwe@xxxxxxx> > Signed-off-by: Balbir Singh <bsingharora@xxxxxxxxx> > --- > arch/powerpc/Kconfig | 3 ++ > arch/powerpc/gcc-mprofile-kernel-notrace.sh | 7 ---- > arch/powerpc/include/asm/livepatch.h | 61 +++++++++++++++++++++++++++++ > arch/powerpc/kernel/Makefile | 1 + > arch/powerpc/kernel/entry_64.S | 46 ++++++++++++++++++++++ > arch/powerpc/kernel/livepatch.c | 38 ++++++++++++++++++ > include/linux/livepatch.h | 13 ++++++ > kernel/livepatch/core.c | 4 +- > 8 files changed, 164 insertions(+), 9 deletions(-) > > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig > index 9f72565..72e46b0 100644 > --- a/arch/powerpc/Kconfig > +++ b/arch/powerpc/Kconfig > @@ -160,6 +160,7 @@ config PPC > select ARCH_HAS_DEVMEM_IS_ALLOWED > select HAVE_ARCH_SECCOMP_FILTER > select ARCH_HAS_UBSAN_SANITIZE_ALL > + select HAVE_LIVEPATCH if PPC64 && CPU_LITTLE_ENDIAN I think this should be "if HAVE_DYNAMIC_FTRACE_WITH_REGS", that way if/when we add support for that on BE/32-bit etc. we won't need to update this line. > diff --git a/arch/powerpc/gcc-mprofile-kernel-notrace.sh b/arch/powerpc/gcc-mprofile-kernel-notrace.sh > index 68d6482..6dafff6 100755 > --- a/arch/powerpc/gcc-mprofile-kernel-notrace.sh > +++ b/arch/powerpc/gcc-mprofile-kernel-notrace.sh > @@ -12,12 +12,6 @@ echo "int func() { return 0; }" | \ > > trace_result=$? > > -echo "int func() { return 0; }" | \ > - $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \ > - sed -n -e '/func:/,/bl _mcount/p' | grep -q TOC > - > -leaf_toc_result=$? > - > /bin/echo -e "#include <linux/compiler.h>\nnotrace int func() { return 0; }" | \ > $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \ > grep -q "mcount" > @@ -25,7 +19,6 @@ leaf_toc_result=$? > notrace_result=$? > > if [ "$trace_result" -eq "0" -a \ > - "$leaf_toc_result" -eq "0" -a \ > "$notrace_result" -eq "1" ]; then > echo y > else You can drop that hunk as I've reworked that script completely in my version. > diff --git a/arch/powerpc/include/asm/livepatch.h b/arch/powerpc/include/asm/livepatch.h > new file mode 100644 > index 0000000..6abb69c > --- /dev/null > +++ b/arch/powerpc/include/asm/livepatch.h > @@ -0,0 +1,61 @@ > +/* > + * livepatch.h - powerpc-specific Kernel Live Patching Core > + * > + * Copyright (C) 2015 SUSE > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License > + * as published by the Free Software Foundation; either version 2 > + * of the License, or (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, see <http://www.gnu.org/licenses/>. > + */ > +#ifndef _ASM_POWERPC64_LIVEPATCH_H > +#define _ASM_POWERPC64_LIVEPATCH_H > + > +#include <linux/module.h> > +#include <linux/ftrace.h> > + > +#ifdef CONFIG_LIVEPATCH > +static inline int klp_check_compiler_support(void) > +{ > +#if !defined(_CALL_ELF) || _CALL_ELF != 2 || !defined(CC_USING_MPROFILE_KERNEL) > + return 1; > +#endif > + return 0; > +} I don't understand why we need that. If your compiler is not supported then you can't even compile the live patch code. I guess we have to implement it to keep the livepatch core happy, but it should just return 1. > +#define ARCH_HAVE_KLP_MATCHADDR I think the consensus these days is that we do that either via Kconfig, or using the #define foo foo pattern. Or just make it a weak function. > +static inline int klp_matchaddr(struct ftrace_ops *ops, unsigned long ip, > + int remove, int reset) > +{ > + int offsets[] = {8, 16}; Because of the two versions of mprofile-kernel (2 or 3 instruction sequence) and the presence or absense of the global entry point, the full set of offsets is: 4, 8, 12, 16. > + int i; > + int ret = 1; > + > + for (i = 0; i < ARRAY_SIZE(offsets); i++) { > + ret = ftrace_set_filter_ip(ops, ip+offsets[i], remove, reset); > + if (!ret) > + break; > + } > + return ret; > +} > + > +extern int klp_write_module_reloc(struct module *mod, unsigned long type, > + unsigned long loc, unsigned long value); That could be static inline for the moment, all it does is return ENOSYS. > + > +static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip) > +{ > + regs->nip = ip; > +} ptrace already defines instruction_pointer_set() to do this. But I guess that's a cleanup for later. > +#else > +#error Live patching support is disabled; check CONFIG_LIVEPATCH > +#endif I don't think you need that here in the header, the kconfig logic already handles this for us. > + > +#endif /* _ASM_POWERPC64_LIVEPATCH_H */ > diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile > index 44667fd..405efce 100644 > --- a/arch/powerpc/kernel/Makefile > +++ b/arch/powerpc/kernel/Makefile > @@ -119,6 +119,7 @@ obj-$(CONFIG_DYNAMIC_FTRACE) += ftrace.o > obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o > obj-$(CONFIG_FTRACE_SYSCALLS) += ftrace.o > obj-$(CONFIG_TRACING) += trace_clock.o > +obj-$(CONFIG_LIVEPATCH) += livepatch.o > > ifneq ($(CONFIG_PPC_INDIRECT_PIO),y) > obj-y += iomap.o > diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S > index f347f50..853717f 100644 > --- a/arch/powerpc/kernel/entry_64.S > +++ b/arch/powerpc/kernel/entry_64.S > @@ -1225,6 +1225,9 @@ _GLOBAL(ftrace_caller) > > /* Calculate ip from nip-4 into r3 for call below */ > subi r3, r7, MCOUNT_INSN_SIZE > +#ifdef CONFIG_LIVEPATCH > + mr r14,r3 /* remember old NIP */ > +#endif > > /* Put the original return address in r4 as parent_ip */ > mr r4, r0 > @@ -1247,6 +1250,9 @@ ftrace_call: > /* Load ctr with the possibly modified NIP */ > ld r3, _NIP(r1) > mtctr r3 > +#ifdef CONFIG_LIVEPATCH > + cmpd r14,r3 /* has NIP been altered? */ > +#endif > > /* Restore gprs */ > REST_8GPRS(0,r1) > @@ -1264,6 +1270,27 @@ ftrace_call: > ld r0, LRSAVE(r1) > mtlr r0 > > +#ifdef CONFIG_LIVEPATCH > + beq+ 4f /* likely(old_NIP == new_NIP) */ > + > + /* For a local call, restore this TOC after calling the patch function. > + * For a global call, it does not matter what we restore here, > + * since the global caller does its own restore right afterwards, > + * anyway. Just insert a KLP_return_helper frame in any case, > + * so a patch function can always count on the changed stack offsets. > + */ That comment could use rewording. I'm not sure what the last sentence is trying to say. And normal comment formatting would be good. > + stdu r1,-32(r1) /* open new mini stack frame */ > + std r0,24(r1) /* save TOC now, unconditionally. */ The toc isn't in r0 in my version, use can just save r2 directly. > + bl 5f > +5: mflr r12 > + addi r12,r12,(KLP_return_helper+4-.)@l It's a pity we need to do this. Just a few instructions ago we had the kernel toc in r2, which would allow us to just do this normally. I guess we'll go with this for now, but I think we can do better in the medium term. > + std r12,LRSAVE(r1) That's the callee's LRSAVE slot, not ours, I think we can just drop that line. > + mtlr r12 > + mfctr r12 /* allow for TOC calculation in newfunc */ A comment here on the content of ctr would be helpful I think. > + bctr > +4: > +#endif > + > #ifdef CONFIG_FUNCTION_GRAPH_TRACER > stdu r1, -112(r1) > .globl ftrace_graph_call > @@ -1279,6 +1306,25 @@ _GLOBAL(ftrace_graph_stub) > #endif /* CC_USING_MPROFILE_KERNEL */ > _GLOBAL(ftrace_stub) > blr > +#ifdef CONFIG_LIVEPATCH > +/* Helper function for local calls that are becoming global > + due to live patching. > + We can't simply patch the NOP after the original call, > + because, depending on the consistency model, some kernel > + threads may still have called the original, local function > + *without* saving their TOC in the respective stack frame slot, > + so the decision is made per-thread during function return by > + maybe inserting a KLP_return_helper frame or not. > +*/ Can you fix the formatting on that. Also it's wrong, we always return via KLP_return_helper() if we're livepatching AFAICS. > +KLP_return_helper: Can we call it klp_return_helper please. > + ld r2,24(r1) /* restore TOC (saved by ftrace_caller) */ > + addi r1, r1, 32 /* destroy mini stack frame */ > + ld r0,LRSAVE(r1) /* get the real return address */ > + mtlr r0 > + blr > +#endif > + > + > #else > _GLOBAL_TOC(_mcount) > /* Taken from output of objdump from lib64/glibc */ cheers -- To unsubscribe from this list: send the line "unsubscribe live-patching" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
