Re: [XEN RFC] xen/bug: introduce bug_fn_nonconst_t to validate run_in_exception_handle()

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 14 Dec 2023, Federico Serafini wrote:
> Introduce function type bug_fn_nonconst_t (as opposed to bug_fn_t)
> to validate the argument passed to run_in_exception_handle().
> 
> Place the definition of bug_fn_nonconst_t before of asm/bug.h inclusion
> so that arch-specific implementations of run_in_exception_handler() can
> use it (and move bug_fn_t into the same place for the same reason).
> 
> Furthermore, use bug_fn_nonconst_t to address violations of
> MISRA C:2012 Rule 8.2  ("Function types shall be in prototype form with
> named parameters").
> 
> Signed-off-by: Federico Serafini <federico.serafini@xxxxxxxxxxx>
> ---
> At the moment, bug_fn_t can not be used within run_in_exception_handler()
> because of the const qualifier on the formal parameter.
> I tried to adjust the constness of the functions passed to
> run_in_exception_handler but at a certain point I stopped:
> a lot of code churn is required and I am not sure about the correctenss of the
> result.
> So, I came up with this solution, taking inspiration from the exising 
> functions
> show_execution_state() and show_execution_state_nonconst().
> 
> I would like to ask if:
> 1) I correctly applied Julien's suggestion about the visibility [1];
> 2) this RFC can become a patch.
> 
> [1]
> https://lists.xenproject.org/archives/html/xen-devel/2023-11/msg01361.html
> ---
>  xen/common/bug.c      |  2 +-
>  xen/include/xen/bug.h | 21 +++++++++++++++------
>  2 files changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/common/bug.c b/xen/common/bug.c
> index ca166e102b..9170821ab8 100644
> --- a/xen/common/bug.c
> +++ b/xen/common/bug.c
> @@ -63,7 +63,7 @@ int do_bug_frame(struct cpu_user_regs *regs, unsigned long 
> pc)
>  
>      if ( id == BUGFRAME_run_fn )
>      {
> -        void (*fn)(struct cpu_user_regs *) = bug_ptr(bug);
> +        bug_fn_nonconst_t *fn = bug_ptr(bug);
>  
>          fn(regs);
>  
> diff --git a/xen/include/xen/bug.h b/xen/include/xen/bug.h
> index cb5138410e..c6f5594af5 100644
> --- a/xen/include/xen/bug.h
> +++ b/xen/include/xen/bug.h
> @@ -12,6 +12,18 @@
>  #define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
>  #define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
>  
> +#ifndef __ASSEMBLY__
> +
> +/*
> + * Make bug_fn_t and bug_fn_nonconst_t visible for arch-specific 
> implementation
> + * of run_in_exception_handler.
> + */
> +struct cpu_user_regs;
> +typedef void bug_fn_t(const struct cpu_user_regs *regs);
> +typedef void bug_fn_nonconst_t(struct cpu_user_regs *regs);
> +
> +#endif
> +
>  #include <asm/bug.h>
>  
>  #ifndef __ASSEMBLY__
> @@ -99,18 +111,15 @@ struct bug_frame {
>  
>  #endif
>  
> -struct cpu_user_regs;
> -typedef void bug_fn_t(const struct cpu_user_regs *regs);

I am not sure why you moved this up in the file, but everything looks
correct to me. I would ack it but I'll let Julien confirm in regards to
his older comment.


>  #ifndef run_in_exception_handler
>  
>  /*
>   * TODO: untangle header dependences, break BUILD_BUG_ON() out of xen/lib.h,
>   * and use a real static inline here to get proper type checking of fn().
>   */
> -#define run_in_exception_handler(fn) do {                   \
> -    (void)((fn) == (void (*)(struct cpu_user_regs *))NULL); \
> -    BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0, NULL);             \
> +#define run_in_exception_handler(fn) do {       \
> +    (void)((fn) == (bug_fn_nonconst_t *)NULL);  \
> +    BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0, NULL); \
>  } while ( false )
>  
>  #endif /* run_in_exception_handler */
> -- 
> 2.34.1
>