linuxppc-dev.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/3] powerpc/code-patching: Introduce open_patch_window()/close_patch_window()
@ 2024-03-25 22:48 Benjamin Gray
  2024-03-25 22:48 ` [PATCH v2 2/3] powerpc/code-patching: Convert to open_patch_window()/close_patch_window() Benjamin Gray
  2024-03-25 22:48 ` [PATCH v2 3/3] powerpc/code-patching: Restore 32-bit patching performance Benjamin Gray
  0 siblings, 2 replies; 4+ messages in thread
From: Benjamin Gray @ 2024-03-25 22:48 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: Benjamin Gray

The code patching capabilities have grown, and the specific quirks for
setting up and tearing down the patching window are getting duplicated.

This commit introduces an abstraction for working with this patching
window. It defines open_patch_window() to set up the writable alias
page, and close_patch_window() to remove it and flush the TLB. The
actual mechanism for providing this patching window is an implementation
detail that consumers don't need to worry about. Consumers are still
responsible for flushing/syncing any changes they make through this
alias page though.

Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>

---

v1: https://lore.kernel.org/all/20240315025937.407590-1-bgray@linux.ibm.com/

This design can be readily extended to remap the writable page to
another physical page without incurring all of the entry and exit
overhead. But that might have problems with spending too long in
an interrupts disabled context, so I've left it out for now.
---
 arch/powerpc/lib/code-patching.c | 113 +++++++++++++++++++++++++++++++
 1 file changed, 113 insertions(+)

diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index df64343b9214..7c193e19e297 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -276,6 +276,119 @@ static void unmap_patch_area(unsigned long addr)
 	flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
 }
 
+/*
+ * Represents an active patching window.
+ */
+struct patch_window {
+	/*
+	 * Page aligned patching window address. The page is a writable alias of
+	 * the configured target page.
+	 */
+	unsigned long text_poke_addr;
+	/*
+	 * Pointer to the PTE for the patching window page.
+	 */
+	pte_t *ptep;
+	/*
+	 * (Temporary MM patching only) The original MM before creating the
+	 * patching window.
+	 */
+	struct mm_struct *orig_mm;
+	/*
+	 * (Temporary MM patching only) The patching window MM.
+	 */
+	struct mm_struct *patch_mm;
+	/*
+	 * (Temporary MM patching only) Lock for the patching window PTE.
+	 */
+	spinlock_t *ptl;
+};
+
+/*
+ * Calling this function opens a 'patching window' that maps a
+ * page starting at ctx->text_poke_addr (which is set by this function)
+ * as a writable alias to the page starting at addr.
+ *
+ * Upon success, callers must invoke the corresponding close_patch_window()
+ * function to return to the original state. Callers are also responsible
+ * for syncing any changes they make inside the window.
+ *
+ * Interrupts must be disabled for the entire duration of the patching. The PIDR
+ * is potentially changed during this time.
+ */
+static int open_patch_window(void *addr, struct patch_window *ctx)
+{
+	unsigned long pfn = get_patch_pfn(addr);
+
+	lockdep_assert_irqs_disabled();
+
+	ctx->text_poke_addr = (unsigned long)__this_cpu_read(cpu_patching_context.addr);
+
+	if (!mm_patch_enabled()) {
+		ctx->ptep = __this_cpu_read(cpu_patching_context.pte);
+		__set_pte_at(&init_mm, ctx->text_poke_addr,
+			     ctx->ptep, pfn_pte(pfn, PAGE_KERNEL), 0);
+
+		/* See ptesync comment in radix__set_pte_at() */
+		if (radix_enabled())
+			asm volatile("ptesync" ::: "memory");
+
+		return 0;
+	}
+
+	ctx->orig_mm = current->active_mm;
+	ctx->patch_mm = __this_cpu_read(cpu_patching_context.mm);
+
+	ctx->ptep = get_locked_pte(ctx->patch_mm, ctx->text_poke_addr, &ctx->ptl);
+	if (!ctx->ptep)
+		return -ENOMEM;
+
+	__set_pte_at(ctx->patch_mm, ctx->text_poke_addr,
+		     ctx->ptep, pfn_pte(pfn, PAGE_KERNEL), 0);
+
+	/* order PTE update before use, also serves as the hwsync */
+	asm volatile("ptesync" ::: "memory");
+
+	/*
+	 * Changing mm requires context synchronising instructions on both sides of
+	 * the context switch, as well as a hwsync between the last instruction for
+	 * which the address of an associated storage access was translated using
+	 * the current context. switch_mm_irqs_off() performs an isync after the
+	 * context switch.
+	 */
+	isync();
+	switch_mm_irqs_off(ctx->orig_mm, ctx->patch_mm, current);
+
+	WARN_ON(!mm_is_thread_local(ctx->patch_mm));
+
+	suspend_breakpoints();
+	return 0;
+}
+
+static void close_patch_window(struct patch_window *ctx)
+{
+	lockdep_assert_irqs_disabled();
+
+	if (!mm_patch_enabled()) {
+		pte_clear(&init_mm, ctx->text_poke_addr, ctx->ptep);
+		flush_tlb_kernel_range(ctx->text_poke_addr, ctx->text_poke_addr + PAGE_SIZE);
+		return;
+	}
+
+	isync();
+	switch_mm_irqs_off(ctx->patch_mm, ctx->orig_mm, current);
+	restore_breakpoints();
+
+	pte_clear(ctx->patch_mm, ctx->text_poke_addr, ctx->ptep);
+	/*
+	 * ptesync to order PTE update before TLB invalidation done
+	 * by radix__local_flush_tlb_page_psize (in _tlbiel_va)
+	 */
+	local_flush_tlb_page_psize(ctx->patch_mm, ctx->text_poke_addr, mmu_virtual_psize);
+
+	pte_unmap_unlock(ctx->ptep, ctx->ptl);
+}
+
 static int __do_patch_instruction_mm(u32 *addr, ppc_inst_t instr)
 {
 	int err;
-- 
2.44.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH v2 2/3] powerpc/code-patching: Convert to open_patch_window()/close_patch_window()
  2024-03-25 22:48 [PATCH v2 1/3] powerpc/code-patching: Introduce open_patch_window()/close_patch_window() Benjamin Gray
@ 2024-03-25 22:48 ` Benjamin Gray
  2024-03-25 22:48 ` [PATCH v2 3/3] powerpc/code-patching: Restore 32-bit patching performance Benjamin Gray
  1 sibling, 0 replies; 4+ messages in thread
From: Benjamin Gray @ 2024-03-25 22:48 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: Benjamin Gray

The existing patching alias page setup and teardown sections can be
simplified to make use of the new open_patch_window() abstraction.

This eliminates the _mm variants of the helpers, consumers no longer
need to check mm_patch_enabled(), and consumers no longer need to worry
about synchronization and flushing beyond the changes they make in the
patching window.

Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>

---

v2: * Removed an outdated comment about syncing
---
 arch/powerpc/lib/code-patching.c | 179 +++----------------------------
 1 file changed, 15 insertions(+), 164 deletions(-)

diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index 7c193e19e297..b3a644290369 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -66,40 +66,6 @@ static bool mm_patch_enabled(void)
 	return IS_ENABLED(CONFIG_SMP) && radix_enabled();
 }
 
-/*
- * The following applies for Radix MMU. Hash MMU has different requirements,
- * and so is not supported.
- *
- * Changing mm requires context synchronising instructions on both sides of
- * the context switch, as well as a hwsync between the last instruction for
- * which the address of an associated storage access was translated using
- * the current context.
- *
- * switch_mm_irqs_off() performs an isync after the context switch. It is
- * the responsibility of the caller to perform the CSI and hwsync before
- * starting/stopping the temp mm.
- */
-static struct mm_struct *start_using_temp_mm(struct mm_struct *temp_mm)
-{
-	struct mm_struct *orig_mm = current->active_mm;
-
-	lockdep_assert_irqs_disabled();
-	switch_mm_irqs_off(orig_mm, temp_mm, current);
-
-	WARN_ON(!mm_is_thread_local(temp_mm));
-
-	suspend_breakpoints();
-	return orig_mm;
-}
-
-static void stop_using_temp_mm(struct mm_struct *temp_mm,
-			       struct mm_struct *orig_mm)
-{
-	lockdep_assert_irqs_disabled();
-	switch_mm_irqs_off(temp_mm, orig_mm, current);
-	restore_breakpoints();
-}
-
 static int text_area_cpu_up(unsigned int cpu)
 {
 	struct vm_struct *area;
@@ -389,73 +355,20 @@ static void close_patch_window(struct patch_window *ctx)
 	pte_unmap_unlock(ctx->ptep, ctx->ptl);
 }
 
-static int __do_patch_instruction_mm(u32 *addr, ppc_inst_t instr)
-{
-	int err;
-	u32 *patch_addr;
-	unsigned long text_poke_addr;
-	pte_t *pte;
-	unsigned long pfn = get_patch_pfn(addr);
-	struct mm_struct *patching_mm;
-	struct mm_struct *orig_mm;
-	spinlock_t *ptl;
-
-	patching_mm = __this_cpu_read(cpu_patching_context.mm);
-	text_poke_addr = __this_cpu_read(cpu_patching_context.addr);
-	patch_addr = (u32 *)(text_poke_addr + offset_in_page(addr));
-
-	pte = get_locked_pte(patching_mm, text_poke_addr, &ptl);
-	if (!pte)
-		return -ENOMEM;
-
-	__set_pte_at(patching_mm, text_poke_addr, pte, pfn_pte(pfn, PAGE_KERNEL), 0);
-
-	/* order PTE update before use, also serves as the hwsync */
-	asm volatile("ptesync": : :"memory");
-
-	/* order context switch after arbitrary prior code */
-	isync();
-
-	orig_mm = start_using_temp_mm(patching_mm);
-
-	err = __patch_instruction(addr, instr, patch_addr);
-
-	/* context synchronisation performed by __patch_instruction (isync or exception) */
-	stop_using_temp_mm(patching_mm, orig_mm);
-
-	pte_clear(patching_mm, text_poke_addr, pte);
-	/*
-	 * ptesync to order PTE update before TLB invalidation done
-	 * by radix__local_flush_tlb_page_psize (in _tlbiel_va)
-	 */
-	local_flush_tlb_page_psize(patching_mm, text_poke_addr, mmu_virtual_psize);
-
-	pte_unmap_unlock(pte, ptl);
-
-	return err;
-}
-
 static int __do_patch_instruction(u32 *addr, ppc_inst_t instr)
 {
-	int err;
+	struct patch_window ctx;
 	u32 *patch_addr;
-	unsigned long text_poke_addr;
-	pte_t *pte;
-	unsigned long pfn = get_patch_pfn(addr);
-
-	text_poke_addr = (unsigned long)__this_cpu_read(cpu_patching_context.addr) & PAGE_MASK;
-	patch_addr = (u32 *)(text_poke_addr + offset_in_page(addr));
+	int err;
 
-	pte = __this_cpu_read(cpu_patching_context.pte);
-	__set_pte_at(&init_mm, text_poke_addr, pte, pfn_pte(pfn, PAGE_KERNEL), 0);
-	/* See ptesync comment in radix__set_pte_at() */
-	if (radix_enabled())
-		asm volatile("ptesync": : :"memory");
+	err = open_patch_window(addr, &ctx);
+	if (err)
+		return err;
 
+	patch_addr = (u32 *)(ctx.text_poke_addr + offset_in_page(addr));
 	err = __patch_instruction(addr, instr, patch_addr);
 
-	pte_clear(&init_mm, text_poke_addr, pte);
-	flush_tlb_kernel_range(text_poke_addr, text_poke_addr + PAGE_SIZE);
+	close_patch_window(&ctx);
 
 	return err;
 }
@@ -475,10 +388,7 @@ int patch_instruction(u32 *addr, ppc_inst_t instr)
 		return raw_patch_instruction(addr, instr);
 
 	local_irq_save(flags);
-	if (mm_patch_enabled())
-		err = __do_patch_instruction_mm(addr, instr);
-	else
-		err = __do_patch_instruction(addr, instr);
+	err = __do_patch_instruction(addr, instr);
 	local_irq_restore(flags);
 
 	return err;
@@ -534,80 +444,24 @@ static int __patch_instructions(u32 *patch_addr, u32 *code, size_t len, bool rep
 	return err;
 }
 
-/*
- * A page is mapped and instructions that fit the page are patched.
- * Assumes 'len' to be (PAGE_SIZE - offset_in_page(addr)) or below.
- */
-static int __do_patch_instructions_mm(u32 *addr, u32 *code, size_t len, bool repeat_instr)
-{
-	struct mm_struct *patching_mm, *orig_mm;
-	unsigned long pfn = get_patch_pfn(addr);
-	unsigned long text_poke_addr;
-	spinlock_t *ptl;
-	u32 *patch_addr;
-	pte_t *pte;
-	int err;
-
-	patching_mm = __this_cpu_read(cpu_patching_context.mm);
-	text_poke_addr = __this_cpu_read(cpu_patching_context.addr);
-	patch_addr = (u32 *)(text_poke_addr + offset_in_page(addr));
-
-	pte = get_locked_pte(patching_mm, text_poke_addr, &ptl);
-	if (!pte)
-		return -ENOMEM;
-
-	__set_pte_at(patching_mm, text_poke_addr, pte, pfn_pte(pfn, PAGE_KERNEL), 0);
-
-	/* order PTE update before use, also serves as the hwsync */
-	asm volatile("ptesync" ::: "memory");
-
-	/* order context switch after arbitrary prior code */
-	isync();
-
-	orig_mm = start_using_temp_mm(patching_mm);
-
-	err = __patch_instructions(patch_addr, code, len, repeat_instr);
-
-	/* context synchronisation performed by __patch_instructions */
-	stop_using_temp_mm(patching_mm, orig_mm);
-
-	pte_clear(patching_mm, text_poke_addr, pte);
-	/*
-	 * ptesync to order PTE update before TLB invalidation done
-	 * by radix__local_flush_tlb_page_psize (in _tlbiel_va)
-	 */
-	local_flush_tlb_page_psize(patching_mm, text_poke_addr, mmu_virtual_psize);
-
-	pte_unmap_unlock(pte, ptl);
-
-	return err;
-}
-
 /*
  * A page is mapped and instructions that fit the page are patched.
  * Assumes 'len' to be (PAGE_SIZE - offset_in_page(addr)) or below.
  */
 static int __do_patch_instructions(u32 *addr, u32 *code, size_t len, bool repeat_instr)
 {
-	unsigned long pfn = get_patch_pfn(addr);
-	unsigned long text_poke_addr;
+	struct patch_window ctx;
 	u32 *patch_addr;
-	pte_t *pte;
 	int err;
 
-	text_poke_addr = (unsigned long)__this_cpu_read(cpu_patching_context.addr) & PAGE_MASK;
-	patch_addr = (u32 *)(text_poke_addr + offset_in_page(addr));
-
-	pte = __this_cpu_read(cpu_patching_context.pte);
-	__set_pte_at(&init_mm, text_poke_addr, pte, pfn_pte(pfn, PAGE_KERNEL), 0);
-	/* See ptesync comment in radix__set_pte_at() */
-	if (radix_enabled())
-		asm volatile("ptesync" ::: "memory");
+	err = open_patch_window(addr, &ctx);
+	if (err)
+		return err;
 
+	patch_addr = (u32 *)(ctx.text_poke_addr + offset_in_page(addr));
 	err = __patch_instructions(patch_addr, code, len, repeat_instr);
 
-	pte_clear(&init_mm, text_poke_addr, pte);
-	flush_tlb_kernel_range(text_poke_addr, text_poke_addr + PAGE_SIZE);
+	close_patch_window(&ctx);
 
 	return err;
 }
@@ -628,10 +482,7 @@ int patch_instructions(u32 *addr, u32 *code, size_t len, bool repeat_instr)
 		plen = min_t(size_t, PAGE_SIZE - offset_in_page(addr), len);
 
 		local_irq_save(flags);
-		if (mm_patch_enabled())
-			err = __do_patch_instructions_mm(addr, code, plen, repeat_instr);
-		else
-			err = __do_patch_instructions(addr, code, plen, repeat_instr);
+		err = __do_patch_instructions(addr, code, plen, repeat_instr);
 		local_irq_restore(flags);
 		if (err)
 			return err;
-- 
2.44.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH v2 3/3] powerpc/code-patching: Restore 32-bit patching performance
  2024-03-25 22:48 [PATCH v2 1/3] powerpc/code-patching: Introduce open_patch_window()/close_patch_window() Benjamin Gray
  2024-03-25 22:48 ` [PATCH v2 2/3] powerpc/code-patching: Convert to open_patch_window()/close_patch_window() Benjamin Gray
@ 2024-03-25 22:48 ` Benjamin Gray
  2024-03-26  7:15   ` Christophe Leroy
  1 sibling, 1 reply; 4+ messages in thread
From: Benjamin Gray @ 2024-03-25 22:48 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: Benjamin Gray

The new open/close abstraction makes it more difficult for a
compiler to optimise. This causes 10% worse performance on
ppc32 as in [1]. Restoring the page alignment mask and inlining
the helpers allows the compiler to better reason about the address
alignment, allowing more optimised cache flushing selection.

[1]: https://lore.kernel.org/all/77fdcdeb-4af5-4ad0-a4c6-57bf0762dc65@csgroup.eu/

Suggested-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>

---

v2: * New in v2

I think Suggested-by is an appropriate tag. The patch is Christophe's
from the link, I just added the commit description, so it could well
be better to change the author to Christophe completely.
---
 arch/powerpc/lib/code-patching.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index b3a644290369..d089da115987 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -282,13 +282,13 @@ struct patch_window {
  * Interrupts must be disabled for the entire duration of the patching. The PIDR
  * is potentially changed during this time.
  */
-static int open_patch_window(void *addr, struct patch_window *ctx)
+static __always_inline int open_patch_window(void *addr, struct patch_window *ctx)
 {
 	unsigned long pfn = get_patch_pfn(addr);
 
 	lockdep_assert_irqs_disabled();
 
-	ctx->text_poke_addr = (unsigned long)__this_cpu_read(cpu_patching_context.addr);
+	ctx->text_poke_addr = (unsigned long)__this_cpu_read(cpu_patching_context.addr) & PAGE_MASK;
 
 	if (!mm_patch_enabled()) {
 		ctx->ptep = __this_cpu_read(cpu_patching_context.pte);
@@ -331,7 +331,7 @@ static int open_patch_window(void *addr, struct patch_window *ctx)
 	return 0;
 }
 
-static void close_patch_window(struct patch_window *ctx)
+static __always_inline void close_patch_window(struct patch_window *ctx)
 {
 	lockdep_assert_irqs_disabled();
 
-- 
2.44.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v2 3/3] powerpc/code-patching: Restore 32-bit patching performance
  2024-03-25 22:48 ` [PATCH v2 3/3] powerpc/code-patching: Restore 32-bit patching performance Benjamin Gray
@ 2024-03-26  7:15   ` Christophe Leroy
  0 siblings, 0 replies; 4+ messages in thread
From: Christophe Leroy @ 2024-03-26  7:15 UTC (permalink / raw)
  To: Benjamin Gray, linuxppc-dev



Le 25/03/2024 à 23:48, Benjamin Gray a écrit :
> The new open/close abstraction makes it more difficult for a
> compiler to optimise. This causes 10% worse performance on
> ppc32 as in [1]. Restoring the page alignment mask and inlining
> the helpers allows the compiler to better reason about the address
> alignment, allowing more optimised cache flushing selection.

This should be squashed into patch 1. There is no point in having that 
as a separate patch when in the same series.

> 
> [1]: https://lore.kernel.org/all/77fdcdeb-4af5-4ad0-a4c6-57bf0762dc65@csgroup.eu/
> 
> Suggested-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
> 
> ---
> 
> v2: * New in v2
> 
> I think Suggested-by is an appropriate tag. The patch is Christophe's
> from the link, I just added the commit description, so it could well
> be better to change the author to Christophe completely.
> ---
>   arch/powerpc/lib/code-patching.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> index b3a644290369..d089da115987 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -282,13 +282,13 @@ struct patch_window {
>    * Interrupts must be disabled for the entire duration of the patching. The PIDR
>    * is potentially changed during this time.
>    */
> -static int open_patch_window(void *addr, struct patch_window *ctx)
> +static __always_inline int open_patch_window(void *addr, struct patch_window *ctx)
>   {
>   	unsigned long pfn = get_patch_pfn(addr);
>   
>   	lockdep_assert_irqs_disabled();
>   
> -	ctx->text_poke_addr = (unsigned long)__this_cpu_read(cpu_patching_context.addr);
> +	ctx->text_poke_addr = (unsigned long)__this_cpu_read(cpu_patching_context.addr) & PAGE_MASK;
>   
>   	if (!mm_patch_enabled()) {
>   		ctx->ptep = __this_cpu_read(cpu_patching_context.pte);
> @@ -331,7 +331,7 @@ static int open_patch_window(void *addr, struct patch_window *ctx)
>   	return 0;
>   }
>   
> -static void close_patch_window(struct patch_window *ctx)
> +static __always_inline void close_patch_window(struct patch_window *ctx)
>   {
>   	lockdep_assert_irqs_disabled();
>   

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2024-03-26  7:16 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-25 22:48 [PATCH v2 1/3] powerpc/code-patching: Introduce open_patch_window()/close_patch_window() Benjamin Gray
2024-03-25 22:48 ` [PATCH v2 2/3] powerpc/code-patching: Convert to open_patch_window()/close_patch_window() Benjamin Gray
2024-03-25 22:48 ` [PATCH v2 3/3] powerpc/code-patching: Restore 32-bit patching performance Benjamin Gray
2024-03-26  7:15   ` Christophe Leroy

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