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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.03.2023 18:10, Oleksii wrote:
> Hello Jan,
> 
> On Thu, 2023-03-16 at 10:52 +0100, Jan Beulich wrote:
>> 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.
> As all your patches are in the staging.
> 
> Can I send a new patch vesrion with <asm/processor.h> removed in
> common/bug.c but leave <xen/debugger.h>? 

If another variant of the build issue still exists, then you want to
leave that as is, yes (but update the description to point out the
new issue that makes this necessary).

> Should I wait for xen/lib.h reworking?

No.

Jan