Re: [PATCH v8 5/5] xen/x86: switch x86 to use generic implemetation of bug.h

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.03.2023 18:21, Oleksii Kurochko wrote:
> The following changes were made:
> * Make GENERIC_BUG_FRAME mandatory for X86
> * Update asm/bug.h using generic implementation in <xen/bug.h>
> * Update do_invalid_op using generic do_bug_frame()
> * Define BUG_DEBUGGER_TRAP_FATAL to debugger_trap_fatal(X86_EXC_GP,regs)
> * type of eip variable was changed to 'const void *'
> * add '#include <xen/debugger.h>'
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> Changes in V8:
>  * move <xen/debuger.h> from <asm/bug.h> to <common/bug.c> to fix compilation 
> issue.
>    The following compilation issue occurs:
>      In file included from ./arch/x86/include/asm/smp.h:10,
>                  from ./include/xen/smp.h:4,
>                  from ./arch/x86/include/asm/processor.h:10,
>                  from ./arch/x86/include/asm/system.h:6,
>                  from ./arch/x86/include/asm/atomic.h:5,
>                  from ./include/xen/gdbstub.h:24,
>                  from ./arch/x86/include/asm/debugger.h:10,
>                  from ./include/xen/debugger.h:24,
>                  from ./arch/x86/include/asm/bug.h:7,
>                  from ./include/xen/bug.h:15,
>                  from ./include/xen/lib.h:27,
>                  from ./include/xen/perfc.h:6,
>                  from arch/x86/x86_64/asm-offsets.c:9:
>      ./include/xen/cpumask.h: In function 'cpumask_check':
>      ./include/xen/cpumask.h:84:9: error: implicit declaration of function 
> 'ASSERT' [-Werror=implicit-function-declaration]
>                     84 |         ASSERT(cpu < nr_cpu_ids);

I'm going to post a patch to address this specific failure. But something
similar may then surface elsewhere.

>    It happens in case when CONFIG_CRASH_DEBUG is enabled

I have to admit that I don't see the connection to CRASH_DEBUG: It's the
asm/atomic.h inclusion that's problematic afaics, yet that (needlessly)
happens outside the respective #ifdef in xen/gdbstub.h.

If another instance of this header interaction issue would surface despite
my to-be-posted patch, I'd be okay with going this route for the moment.
But I think the real issue here is xen/lib.h including xen/bug.h. Instead
of that, some stuff that's presently in xen/lib.h should instead move to
xen/bug.h, and the inclusion there be dropped. Any parties actually using
stuff from xen/bug.h (xen/lib.h then won't anymore) should then include
that header themselves.

Jan

> and the reason for that is when
>    <xen/lib.h> is included in <x86_64/asm-offsets.c>:9 the "layout" of 
> <xen/lib.h> would be
>    the following:
>      #include <xen/bug.h>:
>      #include <asm/bug.h>:
>      #include <xen/debugger.h>:
>          ....
>               cpumask.h:
>                      ....
>                     ASSERT(cpu < nr_cpu_ids);
>                   return cpu;
>                      .... 
>      ....
>      #define ASSERT ...
>      ....
>    Thereby ASSERT is defined after it was used in <xen/cpumask.h>.