Re: [PATCH 2/2] xen/bug: Complete outstanding TODO

[Julien Grall <julien@xxxxxxx>]

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@xxxxxxxxxx>
> > > ---
> > > CC: George Dunlap <George.Dunlap@xxxxxxxxxx>
> > > CC: Jan Beulich <JBeulich@xxxxxxxx>
> > > CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > CC: Wei Liu <wl@xxxxxxx>
> > > CC: Julien Grall <julien@xxxxxxx>
> > > CC: Federico Serafini <federico.serafini@xxxxxxxxxxx>
> > > ---
> > >    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