[PATCH v3 04/13] arm64: debug: call step handlers statically

[Anshuman Khandual]

On 09/06/25 11:04 PM, Ada Couprie Diaz wrote:
> Software stepping checks for the correct handler by iterating over a list
> of dynamically registered handlers and calling all of them until one
> handles the exception.
> 
> This is the only generic way to handle software stepping handlers in arm64
> as the exception does not provide an immediate that could be checked,
> contrary to software breakpoints.
> 
> However, the registration mechanism is not exported and has only
> two current users : the KGDB stepping handler, and the uprobe single step
> handler.
> Given that one comes from user mode and the other from kernel mode, call
> the appropriate one by checking the source EL of the exception.
> Add a stand-in that returns DBG_HOOK_ERROR when the configuration
> options are not enabled.
> 
> Unify the naming of the handler to XXX_singlestep_handler(), making it
> clear they are related.

Makes sense.

> 
> Signed-off-by: Ada Couprie Diaz <ada.coupriediaz at arm.com>
> ---
>  arch/arm64/include/asm/kgdb.h      |  9 +++++++++
>  arch/arm64/include/asm/uprobes.h   |  9 +++++++++
>  arch/arm64/kernel/debug-monitors.c | 25 ++++++-------------------
>  arch/arm64/kernel/kgdb.c           | 17 +++--------------
>  arch/arm64/kernel/probes/uprobes.c |  9 +--------
>  5 files changed, 28 insertions(+), 41 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kgdb.h b/arch/arm64/include/asm/kgdb.h
> index 82a76b2102fb..fa0c1edb8a65 100644
> --- a/arch/arm64/include/asm/kgdb.h
> +++ b/arch/arm64/include/asm/kgdb.h
> @@ -26,6 +26,15 @@ extern int kgdb_fault_expected;
>  
>  int kgdb_brk_handler(struct pt_regs *regs, unsigned long esr);
>  int kgdb_compiled_brk_handler(struct pt_regs *regs, unsigned long esr);
> +#ifdef CONFIG_KGDB
> +int kgdb_singlestep_handler(struct pt_regs *regs, unsigned long esr);
> +#else
> +static inline int kgdb_singlestep_handler(struct pt_regs *regs,
> +	unsigned long esr)
> +{
> +	return DBG_HOOK_ERROR;
> +}
> +#endif
>  
>  #endif /* !__ASSEMBLY__ */
>  
> diff --git a/arch/arm64/include/asm/uprobes.h b/arch/arm64/include/asm/uprobes.h
> index 3659a79a9f32..5858e436a5a7 100644
> --- a/arch/arm64/include/asm/uprobes.h
> +++ b/arch/arm64/include/asm/uprobes.h
> @@ -29,5 +29,14 @@ struct arch_uprobe {
>  };
>  
>  int uprobe_brk_handler(struct pt_regs *regs, unsigned long esr);
> +#ifdef CONFIG_UPROBES
> +int uprobe_singlestep_handler(struct pt_regs *regs, unsigned long esr);
> +#else
> +static inline int uprobe_singlestep_handler(struct pt_regs *regs,
> +	unsigned long esr)
> +{
> +	return DBG_HOOK_ERROR;
> +}
> +#endif
>  
>  #endif
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index 15d7158a7548..b156bef7f61e 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -200,30 +200,17 @@ void unregister_kernel_step_hook(struct step_hook *hook)
>  }
>  
>  /*
> - * Call registered single step handlers
> + * Call single step handlers
>   * There is no Syndrome info to check for determining the handler.
> - * So we call all the registered handlers, until the right handler is
> - * found which returns zero.
> + * However, there is only one possible handler for user and kernel modes, so
> + * check and call the appropriate one.
>   */
>  static int call_step_hook(struct pt_regs *regs, unsigned long esr)
>  {
> -	struct step_hook *hook;
> -	struct list_head *list;
> -	int retval = DBG_HOOK_ERROR;
> +	if (user_mode(regs))
> +		return uprobe_singlestep_handler(regs, esr);
>  
> -	list = user_mode(regs) ? &user_step_hook : &kernel_step_hook;
> -
> -	/*
> -	 * Since single-step exception disables interrupt, this function is
> -	 * entirely not preemptible, and we can use rcu list safely here.
> -	 */
> -	list_for_each_entry_rcu(hook, list, node)	{
> -		retval = hook->fn(regs, esr);
> -		if (retval == DBG_HOOK_HANDLED)
> -			break;
> -	}
> -
> -	return retval;
> +	return kgdb_singlestep_handler(regs, esr);
>  }
>  NOKPROBE_SYMBOL(call_step_hook);
>  
> diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
> index b5a3b9c85a65..8f6ce2ea005c 100644
> --- a/arch/arm64/kernel/kgdb.c
> +++ b/arch/arm64/kernel/kgdb.c
> @@ -250,7 +250,7 @@ int kgdb_compiled_brk_handler(struct pt_regs *regs, unsigned long esr)
>  }
>  NOKPROBE_SYMBOL(kgdb_compiled_brk_handler);
>  
> -static int kgdb_step_brk_fn(struct pt_regs *regs, unsigned long esr)
> +int kgdb_singlestep_handler(struct pt_regs *regs, unsigned long esr)

This rename makes sense but as mentioned later kgdb_single_step_handler()
might save some changes in uprobes callback function.

>  {
>  	if (!kgdb_single_step)
>  		return DBG_HOOK_ERROR;
> @@ -258,11 +258,7 @@ static int kgdb_step_brk_fn(struct pt_regs *regs, unsigned long esr)
>  	kgdb_handle_exception(0, SIGTRAP, 0, regs);
>  	return DBG_HOOK_HANDLED;
>  }
> -NOKPROBE_SYMBOL(kgdb_step_brk_fn);
> -
> -static struct step_hook kgdb_step_hook = {
> -	.fn		= kgdb_step_brk_fn
> -};
> +NOKPROBE_SYMBOL(kgdb_singlestep_handler);
>  
>  static int __kgdb_notify(struct die_args *args, unsigned long cmd)
>  {
> @@ -301,13 +297,7 @@ static struct notifier_block kgdb_notifier = {
>   */
>  int kgdb_arch_init(void)
>  {
> -	int ret = register_die_notifier(&kgdb_notifier);
> -
> -	if (ret != 0)
> -		return ret;
> -
> -	register_kernel_step_hook(&kgdb_step_hook);
> -	return 0;
> +	return register_die_notifier(&kgdb_notifier);
>  }
>  
>  /*
> @@ -317,7 +307,6 @@ int kgdb_arch_init(void)
>   */
>  void kgdb_arch_exit(void)
>  {
> -	unregister_kernel_step_hook(&kgdb_step_hook);
>  	unregister_die_notifier(&kgdb_notifier);
>  }

kgdb_arch_init()/_exit() now deals only with kgdb_notifier registration
and un-registration only.
 >  
> diff --git a/arch/arm64/kernel/probes/uprobes.c b/arch/arm64/kernel/probes/uprobes.c
> index ad68b4a5974d..fefc990860bc 100644
> --- a/arch/arm64/kernel/probes/uprobes.c
> +++ b/arch/arm64/kernel/probes/uprobes.c
> @@ -182,7 +182,7 @@ int uprobe_brk_handler(struct pt_regs *regs,
>  	return DBG_HOOK_ERROR;
>  }
>  
> -static int uprobe_single_step_handler(struct pt_regs *regs,
> +int uprobe_singlestep_handler(struct pt_regs *regs,
>  				      unsigned long esr)

A small nit - if the kgdb handler be changed as kgdb_single_step_handler()
the above rename can be skipped.
 >  {
>  	struct uprobe_task *utask = current->utask;
> @@ -194,15 +194,8 @@ static int uprobe_single_step_handler(struct pt_regs *regs,
>  	return DBG_HOOK_ERROR;
>  }
>  
> -/* uprobe single step handler hook */
> -static struct step_hook uprobes_step_hook = {
> -	.fn = uprobe_single_step_handler,
> -};
> -
>  static int __init arch_init_uprobes(void)
>  {
> -	register_user_step_hook(&uprobes_step_hook);
> -
>  	return 0;
>  }
>  

The arch hook arch_init_uprobes() is redundant now and can be dropped.

static int __init arch_init_uprobes(void)
{
        return 0;
}
device_initcall(arch_init_uprobes);

git grep arch_init_uprobes
arch/arm64/kernel/probes/uprobes.c:static int __init arch_init_uprobes(void)
arch/arm64/kernel/probes/uprobes.c:device_initcall(arch_init_uprobes);

Otherwise LGTM.