Since this TODO was written, BUILD_BUG_ON() has been moved out of xen/lib.h
into xen/macros.h, which has done all the hard work.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <George.Dunlap@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Wei Liu <wl@xen.org>
CC: Julien Grall <julien@xen.org>
CC: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/include/xen/bug.h | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/xen/include/xen/bug.h b/xen/include/xen/bug.h
index cb5138410ea7..8cca4486a477 100644
--- a/xen/include/xen/bug.h
+++ b/xen/include/xen/bug.h
@@ -20,7 +20,8 @@
 #define BUG_DEBUGGER_TRAP_FATAL(regs) 0
 #endif
 
-#include <xen/lib.h>
+#include <xen/macros.h>
+#include <xen/types.h>
 
 #ifndef BUG_FRAME_STRUCT
 
@@ -104,14 +105,11 @@ typedef void bug_fn_t(const struct cpu_user_regs *regs);
 
 #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);             \
-} while ( false )
+static void always_inline run_in_exception_handler(
+    void (*fn)(struct cpu_user_regs *regs))
+{
+    BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0, NULL);
+}
 
 #endif /* run_in_exception_handler */
 
-- 
2.30.2

Hi Andrew,

On 15/12/2023 18:14, Andrew Cooper wrote:
> Since this TODO was written, BUILD_BUG_ON() has been moved out of xen/lib.h
> into xen/macros.h, which has done all the hard work.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: George Dunlap <George.Dunlap@citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Wei Liu <wl@xen.org>
> CC: Julien Grall <julien@xen.org>
> CC: Federico Serafini <federico.serafini@bugseng.com>
> ---
>   xen/include/xen/bug.h | 16 +++++++---------
>   1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/include/xen/bug.h b/xen/include/xen/bug.h
> index cb5138410ea7..8cca4486a477 100644
> --- a/xen/include/xen/bug.h
> +++ b/xen/include/xen/bug.h
> @@ -20,7 +20,8 @@
>   #define BUG_DEBUGGER_TRAP_FATAL(regs) 0
>   #endif
>   
> -#include <xen/lib.h>
> +#include <xen/macros.h>
> +#include <xen/types.h>
>   
>   #ifndef BUG_FRAME_STRUCT
>   
> @@ -104,14 +105,11 @@ typedef void bug_fn_t(const struct cpu_user_regs *regs);
>   
>   #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);             \
> -} while ( false )
> +static void always_inline run_in_exception_handler(
> +    void (*fn)(struct cpu_user_regs *regs))

Based on the other threads, shouldn't this be using bug_fn_t?

Also coding style NIT: I was under the impression we would format 
prototype like that:

static void always_inline
run_in_exception_handler(void (*fn)(struct cpu_user_regs *regs))

Anyway, that comment would be moot if we were using bug_fn_t.

Cheers,

-- 
Julien Grall

On Fri, 15 Dec 2023, Julien Grall wrote:
> Hi Andrew,
> 
> On 15/12/2023 18:14, Andrew Cooper wrote:
> > Since this TODO was written, BUILD_BUG_ON() has been moved out of xen/lib.h
> > into xen/macros.h, which has done all the hard work.
> > 
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > ---
> > CC: George Dunlap <George.Dunlap@citrix.com>
> > CC: Jan Beulich <JBeulich@suse.com>
> > CC: Stefano Stabellini <sstabellini@kernel.org>
> > CC: Wei Liu <wl@xen.org>
> > CC: Julien Grall <julien@xen.org>
> > CC: Federico Serafini <federico.serafini@bugseng.com>
> > ---
> >   xen/include/xen/bug.h | 16 +++++++---------
> >   1 file changed, 7 insertions(+), 9 deletions(-)
> > 
> > diff --git a/xen/include/xen/bug.h b/xen/include/xen/bug.h
> > index cb5138410ea7..8cca4486a477 100644
> > --- a/xen/include/xen/bug.h
> > +++ b/xen/include/xen/bug.h
> > @@ -20,7 +20,8 @@
> >   #define BUG_DEBUGGER_TRAP_FATAL(regs) 0
> >   #endif
> >   -#include <xen/lib.h>
> > +#include <xen/macros.h>
> > +#include <xen/types.h>
> >     #ifndef BUG_FRAME_STRUCT
> >   @@ -104,14 +105,11 @@ typedef void bug_fn_t(const struct cpu_user_regs
> > *regs);
> >     #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);             \
> > -} while ( false )
> > +static void always_inline run_in_exception_handler(
> > +    void (*fn)(struct cpu_user_regs *regs))
> 
> Based on the other threads, shouldn't this be using bug_fn_t?

Unfortunately it doesn't compile:

common/bug.c: In function ‘do_bug_frame’:
common/bug.c:72:38: error: passing argument 1 of ‘run_in_exception_handler’ from incompatible pointer type [-Werror=incompatible-pointer-types]
   72 |             run_in_exception_handler(fn);
      |                                      ^~
      |                                      |
      |                                      void (*)(struct cpu_user_regs *)

due to the missing const in common/bug.c:72.


Not to make things more complicated in this patch, I think we should
take the patch as is as a simple cleanup (and do further cleanups in the
future):

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Hi Stefano,

On 20/12/2023 18:58, Stefano Stabellini wrote:
> On Fri, 15 Dec 2023, Julien Grall wrote:
>> Hi Andrew,
>>
>> On 15/12/2023 18:14, Andrew Cooper wrote:
>>> Since this TODO was written, BUILD_BUG_ON() has been moved out of xen/lib.h
>>> into xen/macros.h, which has done all the hard work.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> CC: George Dunlap <George.Dunlap@citrix.com>
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>> CC: Wei Liu <wl@xen.org>
>>> CC: Julien Grall <julien@xen.org>
>>> CC: Federico Serafini <federico.serafini@bugseng.com>
>>> ---
>>>    xen/include/xen/bug.h | 16 +++++++---------
>>>    1 file changed, 7 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/xen/include/xen/bug.h b/xen/include/xen/bug.h
>>> index cb5138410ea7..8cca4486a477 100644
>>> --- a/xen/include/xen/bug.h
>>> +++ b/xen/include/xen/bug.h
>>> @@ -20,7 +20,8 @@
>>>    #define BUG_DEBUGGER_TRAP_FATAL(regs) 0
>>>    #endif
>>>    -#include <xen/lib.h>
>>> +#include <xen/macros.h>
>>> +#include <xen/types.h>
>>>      #ifndef BUG_FRAME_STRUCT
>>>    @@ -104,14 +105,11 @@ typedef void bug_fn_t(const struct cpu_user_regs
>>> *regs);
>>>      #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);             \
>>> -} while ( false )
>>> +static void always_inline run_in_exception_handler(
>>> +    void (*fn)(struct cpu_user_regs *regs))
>>
>> Based on the other threads, shouldn't this be using bug_fn_t?
> 
> Unfortunately it doesn't compile:
> 
> common/bug.c: In function ‘do_bug_frame’:
> common/bug.c:72:38: error: passing argument 1 of ‘run_in_exception_handler’ from incompatible pointer type [-Werror=incompatible-pointer-types]
>     72 |             run_in_exception_handler(fn);
>        |                                      ^~
>        |                                      |
>        |                                      void (*)(struct cpu_user_regs *)
> 
> due to the missing const in common/bug.c:72.
> 
> 
> Not to make things more complicated in this patch, I think we should
> take the patch as is as a simple cleanup (and do further cleanups in the
> future):

I was sort of expecting an error and I should have proposed a solution, 
sorry. I don't think we need to go all the way of adding 'const' to all 
the callbacks.

Instead, we can remove the const in the bug_fn_t (I realize this is a 
pre-existing bug for Arm). I understand this is not the way we want to 
go in the longer term, but this would be accurate with the current code 
base.

Today we have a weird situation:

    -> run_in_exception_handler() is called with non-const regs.
    -> On arm, the callback is cast to a const regs
    -> The callback will use a non-const regs

This most likely violate MISRA (I think rule 11.8) in a very subtle way. 
And none of the analyzer would be able to catch it because how we build 
the BUG frame.

So I think for this patch we should drop the const in bug_fn_t for 
consistency. We can then update all the callback to const in a future work.

I would be ok to send a separate patch to update bug_fn_t if this is 
preferred.

Cheers,

-- 
Julien Grall