From: Andrew Donnellan <ajd@linux.ibm.com>
To: Benjamin Gray <bgray@linux.ibm.com>, linuxppc-dev@lists.ozlabs.org
Cc: peterz@infradead.org, npiggin@gmail.com, ardb@kernel.org,
jbaron@akamai.com, rostedt@goodmis.org, jpoimboe@kernel.org
Subject: Re: [PATCH v4 1/6] powerpc/code-patching: Implement generic text patching function
Date: Mon, 10 Oct 2022 15:45:39 +1100 [thread overview]
Message-ID: <71c92dc05023b235e87489062ff6f23a2af5fd3c.camel@linux.ibm.com> (raw)
In-Reply-To: <20221010002957.128276-2-bgray@linux.ibm.com>
On Mon, 2022-10-10 at 11:29 +1100, Benjamin Gray wrote:
> Adds a generic memory patching mechanism for patches of size int or
> long
> bytes.
>
> The patch_instruction function is reimplemented in terms of this
> more generic function. This generic implementation allows patching of
> arbitrary long data, such as pointers on 64-bit. As a performance
> optimisation the patch will unconditionally flush the icache, as
> patch_instruction is used significantly more often and in more
> performance sensitive contexts (e.g., ftrace activation).
>
> On 32-bit patch_int is marked noinline to prevent a mis-optimisation.
> Without noinline, inside patch_branch the compiler may inline all the
> way to do_patch_memory, preventing the compiler from inlining
> do_patch_memory into patch_int. This would needlessly force patch_int
> to be a branch to do_patch_memory.
>
> The 'IS_ENABLED(CONFIG_PPC64) && ' part of the patch size condition
> helps GCC inline __patch_memory properly. Only 64-bit requires
> doubleword writes anyway, as ints and longs are the same size on 32-
> bit.
>
> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
It looks like this needs rebasing.
Otherwise I gave this a read over and didn't spot anything, but I'll
hold off on a reviewed-by tag until it's rebased.
> ---
> arch/powerpc/include/asm/code-patching.h | 29 ++++++++++
> arch/powerpc/lib/code-patching.c | 73 ++++++++++++++++++----
> --
> 2 files changed, 85 insertions(+), 17 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/code-patching.h
> b/arch/powerpc/include/asm/code-patching.h
> index 3f881548fb61..170bfa848c7c 100644
> --- a/arch/powerpc/include/asm/code-patching.h
> +++ b/arch/powerpc/include/asm/code-patching.h
> @@ -72,7 +72,36 @@ static inline int create_branch(ppc_inst_t *instr,
> const u32 *addr,
> int create_cond_branch(ppc_inst_t *instr, const u32 *addr,
> unsigned long target, int flags);
> int patch_branch(u32 *addr, unsigned long target, int flags);
> +
> +/* patch_uint and patch_ulong must only be called on addresses where
> the patch
> + * does not cross a cacheline, otherwise it may not be flushed
> properly and
> + * mixes of new and stale data may be observed.
> + *
> + * patch_instruction and other instruction patchers automatically
> satisfy this
> + * requirement due to instruction alignment requirements.
> + */
> +
> +int patch_uint(void *addr, unsigned int val);
> +
> +#ifdef CONFIG_PPC64
> +
> +int patch_ulong(void *addr, unsigned long val);
> int patch_instruction(u32 *addr, ppc_inst_t instr);
> +
> +#else
> +
> +static inline int patch_ulong(void *addr, unsigned long val)
> +{
> + return patch_uint(addr, val);
> +}
> +
> +static inline int patch_instruction(u32 *addr, ppc_inst_t instr)
> +{
> + return patch_uint(addr, ppc_inst_val(instr));
> +}
> +
> +#endif
> +
> int raw_patch_instruction(u32 *addr, ppc_inst_t instr);
>
> static inline unsigned long patch_site_addr(s32 *site)
> diff --git a/arch/powerpc/lib/code-patching.c
> b/arch/powerpc/lib/code-patching.c
> index 125c55e3e148..e41c0ccec79f 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -15,20 +15,24 @@
> #include <asm/code-patching.h>
> #include <asm/inst.h>
>
> -static int __patch_instruction(u32 *exec_addr, ppc_inst_t instr, u32
> *patch_addr)
> +static int __patch_memory(void *patch_addr, unsigned long val, void
> *exec_addr,
> + bool is_dword)
> {
> - if (!ppc_inst_prefixed(instr)) {
> - u32 val = ppc_inst_val(instr);
> -
> - __put_kernel_nofault(patch_addr, &val, u32, failed);
> - } else {
> - u64 val = ppc_inst_as_ulong(instr);
> + /* Prefixed instruction may cross cacheline if cacheline
> smaller than 64 bytes */
> + BUILD_BUG_ON(IS_ENABLED(CONFIG_PPC64) && L1_CACHE_BYTES <
> 64);
>
> + if (IS_ENABLED(CONFIG_PPC64) && unlikely(is_dword))
> __put_kernel_nofault(patch_addr, &val, u64, failed);
> - }
> + else
> + __put_kernel_nofault(patch_addr, &val, u32, failed);
>
> - asm ("dcbst 0, %0; sync; icbi 0,%1; sync; isync" :: "r"
> (patch_addr),
> - "r"
> (exec_addr));
> + /* Assume data is inside a single cacheline */
> + dcbst(patch_addr);
> + mb(); /* sync */
> + /* Flush on the EA that may be executed in case of a non-
> coherent icache */
> + icbi(exec_addr);
> + mb(); /* sync */
> + isync();
>
> return 0;
>
> @@ -38,7 +42,10 @@ static int __patch_instruction(u32 *exec_addr,
> ppc_inst_t instr, u32 *patch_addr
>
> int raw_patch_instruction(u32 *addr, ppc_inst_t instr)
> {
> - return __patch_instruction(addr, instr, addr);
> + if (ppc_inst_prefixed(instr))
> + return __patch_memory(addr, ppc_inst_as_ulong(instr),
> addr, true);
> + else
> + return __patch_memory(addr, ppc_inst_val(instr),
> addr, false);
> }
>
> static DEFINE_PER_CPU(struct vm_struct *, text_poke_area);
> @@ -149,7 +156,7 @@ static void unmap_patch_area(unsigned long addr)
> flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
> }
>
> -static int __do_patch_instruction(u32 *addr, ppc_inst_t instr)
> +static int __do_patch_memory(void *addr, unsigned long val, bool
> is_dword)
> {
> int err;
> u32 *patch_addr;
> @@ -166,7 +173,7 @@ static int __do_patch_instruction(u32 *addr,
> ppc_inst_t instr)
> if (radix_enabled())
> asm volatile("ptesync": : :"memory");
>
> - err = __patch_instruction(addr, instr, patch_addr);
> + err = __patch_memory(patch_addr, val, addr, is_dword);
>
> pte_clear(&init_mm, text_poke_addr, pte);
> flush_tlb_kernel_range(text_poke_addr, text_poke_addr +
> PAGE_SIZE);
> @@ -174,7 +181,7 @@ static int __do_patch_instruction(u32 *addr,
> ppc_inst_t instr)
> return err;
> }
>
> -int patch_instruction(u32 *addr, ppc_inst_t instr)
> +static int do_patch_memory(void *addr, unsigned long val, bool
> is_dword)
> {
> int err;
> unsigned long flags;
> @@ -186,15 +193,47 @@ int patch_instruction(u32 *addr, ppc_inst_t
> instr)
> */
> if (!IS_ENABLED(CONFIG_STRICT_KERNEL_RWX) ||
> !static_branch_likely(&poking_init_done))
> - return raw_patch_instruction(addr, instr);
> + return __patch_memory(addr, val, addr, is_dword);
>
> local_irq_save(flags);
> - err = __do_patch_instruction(addr, instr);
> + err = __do_patch_memory(addr, val, is_dword);
> local_irq_restore(flags);
>
> return err;
> }
> -NOKPROBE_SYMBOL(patch_instruction);
> +
> +#ifdef CONFIG_PPC64
> +
> +int patch_uint(void *addr, unsigned int val)
> +{
> + return do_patch_memory(addr, val, false);
> +}
> +NOKPROBE_SYMBOL(patch_uint)
> +
> +int patch_ulong(void *addr, unsigned long val)
> +{
> + return do_patch_memory(addr, val, true);
> +}
> +NOKPROBE_SYMBOL(patch_ulong)
> +
> +int patch_instruction(u32 *addr, ppc_inst_t instr)
> +{
> + if (ppc_inst_prefixed(instr))
> + return patch_ulong(addr, ppc_inst_as_ulong(instr));
> + else
> + return patch_uint(addr, ppc_inst_val(instr));
> +}
> +NOKPROBE_SYMBOL(patch_instruction)
> +
> +#else
> +
> +noinline int patch_uint(void *addr, unsigned int val)
> +{
> + return do_patch_memory(addr, val, false);
> +}
> +NOKPROBE_SYMBOL(patch_uint)
> +
> +#endif
>
> int patch_branch(u32 *addr, unsigned long target, int flags)
> {
--
Andrew Donnellan OzLabs, ADL Canberra
ajd@linux.ibm.com IBM Australia Limited
next prev parent reply other threads:[~2022-10-10 4:47 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-10 0:29 [PATCH v4 0/6] Out-of-line static calls for powerpc64 ELF V2 Benjamin Gray
2022-10-10 0:29 ` [PATCH v4 1/6] powerpc/code-patching: Implement generic text patching function Benjamin Gray
2022-10-10 4:45 ` Andrew Donnellan [this message]
2022-10-10 7:00 ` Andrew Donnellan
2022-10-10 0:29 ` [PATCH v4 2/6] powerpc/module: Handle caller-saved TOC in module linker Benjamin Gray
2022-10-25 2:10 ` Andrew Donnellan
2022-10-25 23:41 ` Benjamin Gray
2022-10-10 0:29 ` [PATCH v4 3/6] powerpc/module: Optimise nearby branches in ELF V2 ABI stub Benjamin Gray
2022-10-10 0:29 ` [PATCH v4 4/6] static_call: Move static call selftest to static_call_selftest.c Benjamin Gray
2022-10-10 0:29 ` [PATCH v4 5/6] powerpc/64: Add support for out-of-line static calls Benjamin Gray
2022-10-10 0:29 ` [PATCH v4 6/6] powerpc: Add tests " Benjamin Gray
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=71c92dc05023b235e87489062ff6f23a2af5fd3c.camel@linux.ibm.com \
--to=ajd@linux.ibm.com \
--cc=ardb@kernel.org \
--cc=bgray@linux.ibm.com \
--cc=jbaron@akamai.com \
--cc=jpoimboe@kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=npiggin@gmail.com \
--cc=peterz@infradead.org \
--cc=rostedt@goodmis.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).