Re: [PATCH v2] kernel/module_64.c: Add REL24 relocation support of livepatch symbols

[Torsten Duwe <duwe@lst.de>]

On Wed, Oct 04, 2017 at 11:25:16AM -0400, Kamalesh Babulal wrote:
> 
> Both the failures with REL24 livepatch symbols relocation, can be
> resolved by constructing a new livepatch stub. The newly setup klp_stub
> mimics the functionality of entry_64.S::livepatch_handler introduced by
> commit 85baa095497f ("powerpc/livepatch: Add live patching support on
> ppc64le").

So, do I get his right that this patch is based on your June 13 proposal
"powerpc/modules: Introduce new stub code for SHN_LIVEPATCH symbols" ?
I guess you lost many of us already at that point. What is the new, much
bigger stub code needed for? Stub code should do only the very bare minimum,
all common functionality is handled in the kernel main object.

What exactly is the problem you're trying to solve, what is to be achieved?

> +
> +	/*
> +	 * Patch the code required to load the trampoline address into r11,
> +	 * function global entry point into r12, ctr.
> +	 */
> +	entry->jump[klp_stub_idx++] = (PPC_INST_ADDIS | ___PPC_RT(11) |
> +					___PPC_RA(2) | PPC_HA(reladdr));
> +
> +	entry->jump[klp_stub_idx++] = (PPC_INST_ADDI | ___PPC_RT(11) |
> +					 ___PPC_RA(11) | PPC_LO(reladdr));
> +
> +	entry->jump[klp_stub_idx++] = (PPC_INST_LD | ___PPC_RT(12) |
> +					 ___PPC_RA(11) | 128);
							 ^^^
Also, I was a bit puzzled by this constant, BTW.
Can you #define a meaning to this, perhaps?

> @@ -201,8 +204,33 @@ livepatch_handler:
>  	ori     r12, r12, STACK_END_MAGIC@l
>  	std	r12, -8(r11)
>  
> +	/*
> +	 * klp_stub_insn/klp_stub_insn_end marks the beginning/end of the
> +	 * additional instructions, which gets patched by create_klp_stub()
> +	 * for livepatch symbol relocation stub. The instructions are:
> +	 *
> +	 * Load TOC relative address into r11. module_64.c::klp_stub_for_addr()
> +	 * identifies the available free stub slot and loads the address into
> +	 * r11 with two instructions.
> +	 *
> +	 * addis r11, r2, stub_address@ha
> +	 * addi  r11, r11, stub_address@l
> +	 *
> +	 * Load global entry into r12 from entry->funcdata offset
> +	 * ld    r12, 128(r11)

Is that the same 128 as above? Then it should definitely be a #define to
avoid inconsistencies.

	Torsten