diff mbox series

[RFCv1,1/4] arm64: Use static key for tracing PID in CONTEXTIDR

Message ID 20211021134530.206216-2-leo.yan@linaro.org (mailing list archive)
State New, archived
Headers show
Series arm64: Use static key for PID in CONTEXTIDR | expand

Commit Message

Leo Yan Oct. 21, 2021, 1:45 p.m. UTC
The kernel provides CONFIG_PID_IN_CONTEXTIDR for tracing PID into system
register CONTEXTIDR; we need to statically enable this configuration
when build kernel image to use this feature.

On the other hand, hardware tracing modules (e.g. Arm CoreSight, SPE,
etc) rely on this feature to provide context info in their tracing data.
If kernel has not enabled configuration CONFIG_PID_IN_CONTEXTIDR, then
tracing modules have no chance to capture PID related info.

This patch introduces static key for tracing PID in CONTEXTIDR, it
provides a possibility for device driver to dynamically enable and
disable tracing PID into CONTEXTIDR as needed.

As the first step, the kernel increases the static key if
CONFIG_PID_IN_CONTEXTIDR is enabled when booting kernel, in this case
kernel will always trace PID into CONTEXTIDR at the runtime.  This means
before and after applying this patch, the semantics for
CONFIG_PID_IN_CONTEXTIDR are consistent.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 arch/arm64/include/asm/mmu_context.h |  4 +++-
 arch/arm64/kernel/process.c          | 11 +++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

Comments

James Clark Oct. 21, 2021, 2:33 p.m. UTC | #1
On 21/10/2021 14:45, Leo Yan wrote:
> The kernel provides CONFIG_PID_IN_CONTEXTIDR for tracing PID into system
> register CONTEXTIDR; we need to statically enable this configuration
> when build kernel image to use this feature.
> 
> On the other hand, hardware tracing modules (e.g. Arm CoreSight, SPE,
> etc) rely on this feature to provide context info in their tracing data.
> If kernel has not enabled configuration CONFIG_PID_IN_CONTEXTIDR, then
> tracing modules have no chance to capture PID related info.
> 
> This patch introduces static key for tracing PID in CONTEXTIDR, it
> provides a possibility for device driver to dynamically enable and
> disable tracing PID into CONTEXTIDR as needed.
> 
> As the first step, the kernel increases the static key if
> CONFIG_PID_IN_CONTEXTIDR is enabled when booting kernel, in this case
> kernel will always trace PID into CONTEXTIDR at the runtime.  This means
> before and after applying this patch, the semantics for
> CONFIG_PID_IN_CONTEXTIDR are consistent.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  arch/arm64/include/asm/mmu_context.h |  4 +++-
>  arch/arm64/kernel/process.c          | 11 +++++++++++
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h
> index f4ba93d4ffeb..e1f33616f83a 100644
> --- a/arch/arm64/include/asm/mmu_context.h
> +++ b/arch/arm64/include/asm/mmu_context.h
> @@ -26,9 +26,11 @@
>  
>  extern bool rodata_full;
>  
> +DECLARE_STATIC_KEY_FALSE(contextidr_in_use);
> +
>  static inline void contextidr_thread_switch(struct task_struct *next)
>  {
> -	if (!IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR))
> +	if (!static_branch_unlikely(&contextidr_in_use))
>  		return;
>  
>  	write_sysreg(task_pid_nr(next), contextidr_el1);
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 40adb8cdbf5a..d744c0c7e4c4 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -61,6 +61,9 @@ unsigned long __stack_chk_guard __ro_after_init;
>  EXPORT_SYMBOL(__stack_chk_guard);
>  #endif
>  
> +DEFINE_STATIC_KEY_FALSE(contextidr_in_use);
> +EXPORT_SYMBOL_GPL(contextidr_in_use);
> +
>  /*
>   * Function pointers to optional machine specific functions
>   */
> @@ -721,3 +724,11 @@ int arch_elf_adjust_prot(int prot, const struct arch_elf_state *state,
>  	return prot;
>  }
>  #endif
> +
> +static int __init contextidr_init(void)
> +{
> +	if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR))
> +		static_branch_inc(&contextidr_in_use);
> +	return 0;
> +}
> +early_initcall(contextidr_init);

Hi Leo,

Can you skip this early_initcall() part if you do something like this:

    DECLARE_STATIC_KEY_MAYBE(CONFIG_PID_IN_CONTEXTIDR, contextidr_in_use)

It seems like there is a way to conditionally initialise it to true.

James
Leo Yan Oct. 21, 2021, 2:37 p.m. UTC | #2
Hi James,

On Thu, Oct 21, 2021 at 03:33:01PM +0100, James Clark wrote:

[...]

> > +static int __init contextidr_init(void)
> > +{
> > +	if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR))
> > +		static_branch_inc(&contextidr_in_use);
> > +	return 0;
> > +}
> > +early_initcall(contextidr_init);
> 
> Hi Leo,
> 
> Can you skip this early_initcall() part if you do something like this:
> 
>     DECLARE_STATIC_KEY_MAYBE(CONFIG_PID_IN_CONTEXTIDR, contextidr_in_use)
> 
> It seems like there is a way to conditionally initialise it to true.

Thanks for good point!  Will test this way in next spin.

Leo
Kees Cook Oct. 21, 2021, 3:47 p.m. UTC | #3
On Thu, Oct 21, 2021 at 03:33:01PM +0100, James Clark wrote:
> 
> 
> On 21/10/2021 14:45, Leo Yan wrote:
> > The kernel provides CONFIG_PID_IN_CONTEXTIDR for tracing PID into system
> > register CONTEXTIDR; we need to statically enable this configuration
> > when build kernel image to use this feature.
> > 
> > On the other hand, hardware tracing modules (e.g. Arm CoreSight, SPE,
> > etc) rely on this feature to provide context info in their tracing data.
> > If kernel has not enabled configuration CONFIG_PID_IN_CONTEXTIDR, then
> > tracing modules have no chance to capture PID related info.
> > 
> > This patch introduces static key for tracing PID in CONTEXTIDR, it
> > provides a possibility for device driver to dynamically enable and
> > disable tracing PID into CONTEXTIDR as needed.
> > 
> > As the first step, the kernel increases the static key if
> > CONFIG_PID_IN_CONTEXTIDR is enabled when booting kernel, in this case
> > kernel will always trace PID into CONTEXTIDR at the runtime.  This means
> > before and after applying this patch, the semantics for
> > CONFIG_PID_IN_CONTEXTIDR are consistent.
> > 
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >  arch/arm64/include/asm/mmu_context.h |  4 +++-
> >  arch/arm64/kernel/process.c          | 11 +++++++++++
> >  2 files changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h
> > index f4ba93d4ffeb..e1f33616f83a 100644
> > --- a/arch/arm64/include/asm/mmu_context.h
> > +++ b/arch/arm64/include/asm/mmu_context.h
> > @@ -26,9 +26,11 @@
> >  
> >  extern bool rodata_full;
> >  
> > +DECLARE_STATIC_KEY_FALSE(contextidr_in_use);
> > +
> >  static inline void contextidr_thread_switch(struct task_struct *next)
> >  {
> > -	if (!IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR))
> > +	if (!static_branch_unlikely(&contextidr_in_use))
> >  		return;
> >  
> >  	write_sysreg(task_pid_nr(next), contextidr_el1);
> > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> > index 40adb8cdbf5a..d744c0c7e4c4 100644
> > --- a/arch/arm64/kernel/process.c
> > +++ b/arch/arm64/kernel/process.c
> > @@ -61,6 +61,9 @@ unsigned long __stack_chk_guard __ro_after_init;
> >  EXPORT_SYMBOL(__stack_chk_guard);
> >  #endif
> >  
> > +DEFINE_STATIC_KEY_FALSE(contextidr_in_use);
> > +EXPORT_SYMBOL_GPL(contextidr_in_use);
> > +
> >  /*
> >   * Function pointers to optional machine specific functions
> >   */
> > @@ -721,3 +724,11 @@ int arch_elf_adjust_prot(int prot, const struct arch_elf_state *state,
> >  	return prot;
> >  }
> >  #endif
> > +
> > +static int __init contextidr_init(void)
> > +{
> > +	if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR))
> > +		static_branch_inc(&contextidr_in_use);
> > +	return 0;
> > +}
> > +early_initcall(contextidr_init);
> 
> Hi Leo,
> 
> Can you skip this early_initcall() part if you do something like this:
> 
>     DECLARE_STATIC_KEY_MAYBE(CONFIG_PID_IN_CONTEXTIDR, contextidr_in_use)
> 
> It seems like there is a way to conditionally initialise it to true.

I was going to suggest the same thing. :) With this change, it looks
good:

Reviewed-by: Kees Cook <keescook@chromium.org>
diff mbox series

Patch

diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h
index f4ba93d4ffeb..e1f33616f83a 100644
--- a/arch/arm64/include/asm/mmu_context.h
+++ b/arch/arm64/include/asm/mmu_context.h
@@ -26,9 +26,11 @@ 
 
 extern bool rodata_full;
 
+DECLARE_STATIC_KEY_FALSE(contextidr_in_use);
+
 static inline void contextidr_thread_switch(struct task_struct *next)
 {
-	if (!IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR))
+	if (!static_branch_unlikely(&contextidr_in_use))
 		return;
 
 	write_sysreg(task_pid_nr(next), contextidr_el1);
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 40adb8cdbf5a..d744c0c7e4c4 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -61,6 +61,9 @@  unsigned long __stack_chk_guard __ro_after_init;
 EXPORT_SYMBOL(__stack_chk_guard);
 #endif
 
+DEFINE_STATIC_KEY_FALSE(contextidr_in_use);
+EXPORT_SYMBOL_GPL(contextidr_in_use);
+
 /*
  * Function pointers to optional machine specific functions
  */
@@ -721,3 +724,11 @@  int arch_elf_adjust_prot(int prot, const struct arch_elf_state *state,
 	return prot;
 }
 #endif
+
+static int __init contextidr_init(void)
+{
+	if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR))
+		static_branch_inc(&contextidr_in_use);
+	return 0;
+}
+early_initcall(contextidr_init);