Re: [PATCH] consolidate do_bug_frame() / bug_fn_t

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

Hi Jan, Andrew,

I managed to get back to read the mailing list and noticed this patch.

Is it still relevant and needs to be reviewed?

Are there any outstanding disagreements between maintainers on the
approach to take here?  Or should I just go ahead and review it?


On Tue, 9 Jan 2024, Jan Beulich wrote:
> The type not being used in do_bug_frame() is suspicious. Apparently
> that's solely because the type uses a pointer-to-const parameter,
> when run_in_exception_handler() wants functions taking pointer-to-non-
> const. Drop the const, in turn requiring Arm's do_bug_frame() to also
> have its const dropped. This then brings that function also closer to
> the common one, with Arm's use of vaddr_t remaining as a difference.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> This is an alternative proposal to
> https://lists.xen.org/archives/html/xen-devel/2023-12/msg01385.html,
> albeit without paving a road towards Andrew's desire of getting rid of
> show_execution_state_nonconst() again. Retaining (and propagating) the
> const would imply the need to cast away the const-ness somewhere on (at
> least) the path to invoking gdb stub code. Personally I'm averse to such
> casting away of const-ness ...
> 
> --- a/xen/arch/arm/include/asm/traps.h
> +++ b/xen/arch/arm/include/asm/traps.h
> @@ -69,7 +69,7 @@ void do_cp(struct cpu_user_regs *regs, c
>  void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr);
>  void do_trap_hvc_smccc(struct cpu_user_regs *regs);
>  
> -int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc);
> +int do_bug_frame(struct cpu_user_regs *regs, vaddr_t pc);
>  
>  void noreturn do_unexpected_trap(const char *msg,
>                                   const struct cpu_user_regs *regs);
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1202,7 +1202,7 @@ void do_unexpected_trap(const char *msg,
>      panic("CPU%d: Unexpected Trap: %s\n", smp_processor_id(), msg);
>  }
>  
> -int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
> +int do_bug_frame(struct cpu_user_regs *regs, vaddr_t pc)
>  {
>      const struct bug_frame *bug = NULL;
>      const char *prefix = "", *filename, *predicate;
> --- a/xen/common/bug.c
> +++ b/xen/common/bug.c
> @@ -63,14 +63,10 @@ int do_bug_frame(struct cpu_user_regs *r
>  
>      if ( id == BUGFRAME_run_fn )
>      {
> -        void (*fn)(struct cpu_user_regs *) = bug_ptr(bug);
> +        bug_fn_t *fn = bug_ptr(bug);
>  
>          fn(regs);
>  
> -        /* Re-enforce consistent types, because of the casts involved. */
> -        if ( false )
> -            run_in_exception_handler(fn);
> -
>          return id;
>      }
>  
> --- a/xen/include/xen/bug.h
> +++ b/xen/include/xen/bug.h
> @@ -101,12 +101,11 @@ struct bug_frame {
>  #endif
>  
>  struct cpu_user_regs;
> -typedef void bug_fn_t(const struct cpu_user_regs *regs);
> +typedef void bug_fn_t(struct cpu_user_regs *regs);
>  
>  #ifndef run_in_exception_handler
>  
> -static void always_inline run_in_exception_handler(
> -    void (*fn)(struct cpu_user_regs *regs))
> +static void always_inline run_in_exception_handler(bug_fn_t *fn)
>  {
>      BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0, NULL);
>  }
>