| Message ID | 20231215181433.1588532-3-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen: run_in_exception_handler() cleanup | expand |
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,
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,
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 */
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(-)