diff mbox series

[12/17] arm64: debug-monitors: refactor MDSCR manipulation

Message ID 20200108185634.1163-13-mark.rutland@arm.com (mailing list archive)
State New, archived
Headers show
Series arm64: entry deasmification and cleanup | expand

Commit Message

Mark Rutland Jan. 8, 2020, 6:56 p.m. UTC 
When we convert the ret_to_user/work_pending code to C, we're going to
want to poke the MDSCR to enable/disable single-step. Let's factor out
the existing code for this from debug-monitors.c.

At the same time, we can make use of {read,write}_sysreg() directly, and
get rid of the mdscr_{read,write} wrappers.

The existing code masked DAIF when manipulating MDSCR_EL1, but this
should not be necessary. Exceptions can be taken immediately before DAIF
is masked, and given the lack of an ISB can also be taken after DAIF is
unmasked as writes to DAIF are only self-synchronizing and not
context-synchronizing in general. We may want to add an ISB to ensure
that updates to MDSCR have taken effect, however.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/debug-monitors.h | 10 ++++++++++
 arch/arm64/kernel/debug-monitors.c      | 32 +++++++-------------------------
 2 files changed, 17 insertions(+), 25 deletions(-)

Comments

Anshuman Khandual Jan. 10, 2020, 4:35 a.m. UTC | #1 
On 01/09/2020 12:26 AM, Mark Rutland wrote:
> When we convert the ret_to_user/work_pending code to C, we're going to
> want to poke the MDSCR to enable/disable single-step. Let's factor out
> the existing code for this from debug-monitors.c.
> 
> At the same time, we can make use of {read,write}_sysreg() directly, and
> get rid of the mdscr_{read,write} wrappers.
> 
> The existing code masked DAIF when manipulating MDSCR_EL1, but this
> should not be necessary. Exceptions can be taken immediately before DAIF
> is masked, and given the lack of an ISB can also be taken after DAIF is
> unmasked as writes to DAIF are only self-synchronizing and not
> context-synchronizing in general. We may want to add an ISB to ensure
> that updates to MDSCR have taken effect, however.

Any reason this patch choose not add that ISB for now after writing
mdscr_el1 register via sysreg_clear_set().

> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/include/asm/debug-monitors.h | 10 ++++++++++
>  arch/arm64/kernel/debug-monitors.c      | 32 +++++++-------------------------
>  2 files changed, 17 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
> index 7619f473155f..342867e50c54 100644
> --- a/arch/arm64/include/asm/debug-monitors.h
> +++ b/arch/arm64/include/asm/debug-monitors.h
> @@ -107,6 +107,16 @@ enum dbg_active_el {
>  void enable_debug_monitors(enum dbg_active_el el);
>  void disable_debug_monitors(enum dbg_active_el el);
>  
> +static __always_inline void __enable_single_step_nosync(void)
> +{
> +	sysreg_clear_set(mdscr_el1, 0, DBG_MDSCR_SS);
> +}
> +
> +static __always_inline void __disable_single_step_nosync(void)
> +{
> +	sysreg_clear_set(mdscr_el1, DBG_MDSCR_SS, 0);
> +}
> +
>  void user_rewind_single_step(struct task_struct *task);
>  void user_fastforward_single_step(struct task_struct *task);
>  
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index 48222a4760c2..fa2d4145bd07 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -32,24 +32,6 @@ u8 debug_monitors_arch(void)
>  }
>  
>  /*
> - * MDSCR access routines.
> - */
> -static void mdscr_write(u32 mdscr)
> -{
> -	unsigned long flags;
> -	flags = local_daif_save();
> -	write_sysreg(mdscr, mdscr_el1);
> -	local_daif_restore(flags);
> -}
> -NOKPROBE_SYMBOL(mdscr_write);
> -
> -static u32 mdscr_read(void)
> -{
> -	return read_sysreg(mdscr_el1);
> -}
> -NOKPROBE_SYMBOL(mdscr_read);
> -
> -/*
>   * Allow root to disable self-hosted debug from userspace.
>   * This is useful if you want to connect an external JTAG debugger.
>   */
> @@ -91,9 +73,9 @@ void enable_debug_monitors(enum dbg_active_el el)
>  		enable |= DBG_MDSCR_KDE;
>  
>  	if (enable && debug_enabled) {
> -		mdscr = mdscr_read();
> +		mdscr = read_sysreg(mdscr_el1);
>  		mdscr |= enable;
> -		mdscr_write(mdscr);
> +		write_sysreg(mdscr, mdscr_el1);
>  	}
>  }
>  NOKPROBE_SYMBOL(enable_debug_monitors);
> @@ -112,9 +94,9 @@ void disable_debug_monitors(enum dbg_active_el el)
>  		disable &= ~DBG_MDSCR_KDE;
>  
>  	if (disable) {
> -		mdscr = mdscr_read();
> +		mdscr = read_sysreg(mdscr_el1);
>  		mdscr &= disable;
> -		mdscr_write(mdscr);
> +		write_sysreg(mdscr, mdscr_el1);
>  	}
>  }
>  NOKPROBE_SYMBOL(disable_debug_monitors);
> @@ -409,7 +391,7 @@ void kernel_enable_single_step(struct pt_regs *regs)
>  {
>  	WARN_ON(!irqs_disabled());
>  	set_regs_spsr_ss(regs);
> -	mdscr_write(mdscr_read() | DBG_MDSCR_SS);
> +	__enable_single_step_nosync();
>  	enable_debug_monitors(DBG_ACTIVE_EL1);
>  }
>  NOKPROBE_SYMBOL(kernel_enable_single_step);
> @@ -417,7 +399,7 @@ NOKPROBE_SYMBOL(kernel_enable_single_step);
>  void kernel_disable_single_step(void)
>  {
>  	WARN_ON(!irqs_disabled());
> -	mdscr_write(mdscr_read() & ~DBG_MDSCR_SS);
> +	__disable_single_step_nosync();
>  	disable_debug_monitors(DBG_ACTIVE_EL1);
>  }
>  NOKPROBE_SYMBOL(kernel_disable_single_step);
> @@ -425,7 +407,7 @@ NOKPROBE_SYMBOL(kernel_disable_single_step);
>  int kernel_active_single_step(void)
>  {
>  	WARN_ON(!irqs_disabled());
> -	return mdscr_read() & DBG_MDSCR_SS;
> +	return read_sysreg(mdscr_el1) & DBG_MDSCR_SS;
>  }
>  NOKPROBE_SYMBOL(kernel_active_single_step);
>  
>
Mark Rutland Jan. 10, 2020, 4:09 p.m. UTC | #2 
On Fri, Jan 10, 2020 at 10:05:48AM +0530, Anshuman Khandual wrote:
> 
> 
> On 01/09/2020 12:26 AM, Mark Rutland wrote:
> > When we convert the ret_to_user/work_pending code to C, we're going to
> > want to poke the MDSCR to enable/disable single-step. Let's factor out
> > the existing code for this from debug-monitors.c.
> > 
> > At the same time, we can make use of {read,write}_sysreg() directly, and
> > get rid of the mdscr_{read,write} wrappers.
> > 
> > The existing code masked DAIF when manipulating MDSCR_EL1, but this
> > should not be necessary. Exceptions can be taken immediately before DAIF
> > is masked, and given the lack of an ISB can also be taken after DAIF is
> > unmasked as writes to DAIF are only self-synchronizing and not
> > context-synchronizing in general. We may want to add an ISB to ensure
> > that updates to MDSCR have taken effect, however.
> 
> Any reason this patch choose not add that ISB for now after writing
> mdscr_el1 register via sysreg_clear_set().

I didn't want to make that functional change without justification. For
example, the ISB wouldn't be needed for changes that only affect
userspace.

Thanks,
Mark.

> 
> > 
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > ---
> >  arch/arm64/include/asm/debug-monitors.h | 10 ++++++++++
> >  arch/arm64/kernel/debug-monitors.c      | 32 +++++++-------------------------
> >  2 files changed, 17 insertions(+), 25 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
> > index 7619f473155f..342867e50c54 100644
> > --- a/arch/arm64/include/asm/debug-monitors.h
> > +++ b/arch/arm64/include/asm/debug-monitors.h
> > @@ -107,6 +107,16 @@ enum dbg_active_el {
> >  void enable_debug_monitors(enum dbg_active_el el);
> >  void disable_debug_monitors(enum dbg_active_el el);
> >  
> > +static __always_inline void __enable_single_step_nosync(void)
> > +{
> > +	sysreg_clear_set(mdscr_el1, 0, DBG_MDSCR_SS);
> > +}
> > +
> > +static __always_inline void __disable_single_step_nosync(void)
> > +{
> > +	sysreg_clear_set(mdscr_el1, DBG_MDSCR_SS, 0);
> > +}
> > +
> >  void user_rewind_single_step(struct task_struct *task);
> >  void user_fastforward_single_step(struct task_struct *task);
> >  
> > diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> > index 48222a4760c2..fa2d4145bd07 100644
> > --- a/arch/arm64/kernel/debug-monitors.c
> > +++ b/arch/arm64/kernel/debug-monitors.c
> > @@ -32,24 +32,6 @@ u8 debug_monitors_arch(void)
> >  }
> >  
> >  /*
> > - * MDSCR access routines.
> > - */
> > -static void mdscr_write(u32 mdscr)
> > -{
> > -	unsigned long flags;
> > -	flags = local_daif_save();
> > -	write_sysreg(mdscr, mdscr_el1);
> > -	local_daif_restore(flags);
> > -}
> > -NOKPROBE_SYMBOL(mdscr_write);
> > -
> > -static u32 mdscr_read(void)
> > -{
> > -	return read_sysreg(mdscr_el1);
> > -}
> > -NOKPROBE_SYMBOL(mdscr_read);
> > -
> > -/*
> >   * Allow root to disable self-hosted debug from userspace.
> >   * This is useful if you want to connect an external JTAG debugger.
> >   */
> > @@ -91,9 +73,9 @@ void enable_debug_monitors(enum dbg_active_el el)
> >  		enable |= DBG_MDSCR_KDE;
> >  
> >  	if (enable && debug_enabled) {
> > -		mdscr = mdscr_read();
> > +		mdscr = read_sysreg(mdscr_el1);
> >  		mdscr |= enable;
> > -		mdscr_write(mdscr);
> > +		write_sysreg(mdscr, mdscr_el1);
> >  	}
> >  }
> >  NOKPROBE_SYMBOL(enable_debug_monitors);
> > @@ -112,9 +94,9 @@ void disable_debug_monitors(enum dbg_active_el el)
> >  		disable &= ~DBG_MDSCR_KDE;
> >  
> >  	if (disable) {
> > -		mdscr = mdscr_read();
> > +		mdscr = read_sysreg(mdscr_el1);
> >  		mdscr &= disable;
> > -		mdscr_write(mdscr);
> > +		write_sysreg(mdscr, mdscr_el1);
> >  	}
> >  }
> >  NOKPROBE_SYMBOL(disable_debug_monitors);
> > @@ -409,7 +391,7 @@ void kernel_enable_single_step(struct pt_regs *regs)
> >  {
> >  	WARN_ON(!irqs_disabled());
> >  	set_regs_spsr_ss(regs);
> > -	mdscr_write(mdscr_read() | DBG_MDSCR_SS);
> > +	__enable_single_step_nosync();
> >  	enable_debug_monitors(DBG_ACTIVE_EL1);
> >  }
> >  NOKPROBE_SYMBOL(kernel_enable_single_step);
> > @@ -417,7 +399,7 @@ NOKPROBE_SYMBOL(kernel_enable_single_step);
> >  void kernel_disable_single_step(void)
> >  {
> >  	WARN_ON(!irqs_disabled());
> > -	mdscr_write(mdscr_read() & ~DBG_MDSCR_SS);
> > +	__disable_single_step_nosync();
> >  	disable_debug_monitors(DBG_ACTIVE_EL1);
> >  }
> >  NOKPROBE_SYMBOL(kernel_disable_single_step);
> > @@ -425,7 +407,7 @@ NOKPROBE_SYMBOL(kernel_disable_single_step);
> >  int kernel_active_single_step(void)
> >  {
> >  	WARN_ON(!irqs_disabled());
> > -	return mdscr_read() & DBG_MDSCR_SS;
> > +	return read_sysreg(mdscr_el1) & DBG_MDSCR_SS;
> >  }
> >  NOKPROBE_SYMBOL(kernel_active_single_step);
> >  
> > 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff mbox series

Patch

diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index 7619f473155f..342867e50c54 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -107,6 +107,16 @@  enum dbg_active_el {
 void enable_debug_monitors(enum dbg_active_el el);
 void disable_debug_monitors(enum dbg_active_el el);
 
+static __always_inline void __enable_single_step_nosync(void)
+{
+	sysreg_clear_set(mdscr_el1, 0, DBG_MDSCR_SS);
+}
+
+static __always_inline void __disable_single_step_nosync(void)
+{
+	sysreg_clear_set(mdscr_el1, DBG_MDSCR_SS, 0);
+}
+
 void user_rewind_single_step(struct task_struct *task);
 void user_fastforward_single_step(struct task_struct *task);
 
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index 48222a4760c2..fa2d4145bd07 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -32,24 +32,6 @@  u8 debug_monitors_arch(void)
 }
 
 /*
- * MDSCR access routines.
- */
-static void mdscr_write(u32 mdscr)
-{
-	unsigned long flags;
-	flags = local_daif_save();
-	write_sysreg(mdscr, mdscr_el1);
-	local_daif_restore(flags);
-}
-NOKPROBE_SYMBOL(mdscr_write);
-
-static u32 mdscr_read(void)
-{
-	return read_sysreg(mdscr_el1);
-}
-NOKPROBE_SYMBOL(mdscr_read);
-
-/*
  * Allow root to disable self-hosted debug from userspace.
  * This is useful if you want to connect an external JTAG debugger.
  */
@@ -91,9 +73,9 @@  void enable_debug_monitors(enum dbg_active_el el)
 		enable |= DBG_MDSCR_KDE;
 
 	if (enable && debug_enabled) {
-		mdscr = mdscr_read();
+		mdscr = read_sysreg(mdscr_el1);
 		mdscr |= enable;
-		mdscr_write(mdscr);
+		write_sysreg(mdscr, mdscr_el1);
 	}
 }
 NOKPROBE_SYMBOL(enable_debug_monitors);
@@ -112,9 +94,9 @@  void disable_debug_monitors(enum dbg_active_el el)
 		disable &= ~DBG_MDSCR_KDE;
 
 	if (disable) {
-		mdscr = mdscr_read();
+		mdscr = read_sysreg(mdscr_el1);
 		mdscr &= disable;
-		mdscr_write(mdscr);
+		write_sysreg(mdscr, mdscr_el1);
 	}
 }
 NOKPROBE_SYMBOL(disable_debug_monitors);
@@ -409,7 +391,7 @@  void kernel_enable_single_step(struct pt_regs *regs)
 {
 	WARN_ON(!irqs_disabled());
 	set_regs_spsr_ss(regs);
-	mdscr_write(mdscr_read() | DBG_MDSCR_SS);
+	__enable_single_step_nosync();
 	enable_debug_monitors(DBG_ACTIVE_EL1);
 }
 NOKPROBE_SYMBOL(kernel_enable_single_step);
@@ -417,7 +399,7 @@  NOKPROBE_SYMBOL(kernel_enable_single_step);
 void kernel_disable_single_step(void)
 {
 	WARN_ON(!irqs_disabled());
-	mdscr_write(mdscr_read() & ~DBG_MDSCR_SS);
+	__disable_single_step_nosync();
 	disable_debug_monitors(DBG_ACTIVE_EL1);
 }
 NOKPROBE_SYMBOL(kernel_disable_single_step);
@@ -425,7 +407,7 @@  NOKPROBE_SYMBOL(kernel_disable_single_step);
 int kernel_active_single_step(void)
 {
 	WARN_ON(!irqs_disabled());
-	return mdscr_read() & DBG_MDSCR_SS;
+	return read_sysreg(mdscr_el1) & DBG_MDSCR_SS;
 }
 NOKPROBE_SYMBOL(kernel_active_single_step);