Re: [PATCH v8 5/5] xen/x86: switch x86 to use generic implemetation of bug.h
On 28.03.2023 18:55, Oleksii wrote: > On Tue, 2023-03-28 at 19:38 +0300, Oleksii wrote: >> On Tue, 2023-03-28 at 18:38 +0300, Oleksii wrote: >>> offsets.s arch/x86/x86_64/asm-offsets.c >>> In file included from ./include/public/domctl.h:21, >>> from ./include/public/sysctl.h:18, >>> from ./arch/x86/include/asm/cpuid.h:14, >>> from ./arch/x86/include/asm/cpufeature.h:10, >>> from ./arch/x86/include/asm/system.h:7, >>> 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:6, >>> from ./include/xen/bug.h:15, >>> from ./arch/x86/include/asm/alternative.h:7, >>> from ./arch/x86/include/asm/bitops.h:8, >>> from ./include/xen/bitops.h:106, >>> from ./arch/x86/include/asm/smp.h:8, >>> from ./include/xen/smp.h:4, >>> from ./include/xen/perfc.h:7, >>> from arch/x86/x86_64/asm-offsets.c:9: >> And again the problem is that x86's includes >> which somewhere inside uses BUG() which will be >> defined after we will back for x86's to where >> BUG() is defined. >> >> So it looks like we can't include to something that will >> use functionality defined in ... >> >> Thereby I don't see better option that include in >> instead of Well, to deal with this specific issue we could re-arrange xen/perfc.h a little (to skip part of it when COMPILE_OFFSETS is defined), but it seems quite likely that then the same issue would surface yet again elsewhere. So yes, for the time being I guess we need to go with what you have. Until we can sort the xen/lib.h vs xen/bug.h aspect. > Or as an option we can include in instead of > include in it will allow us to resolve an > issue... > > Do you think this option will be better? No, imo that arrangement should remain as is. Jan
Re: [PATCH v8 5/5] xen/x86: switch x86 to use generic implemetation of bug.h
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 > > * 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 ' > > > > Signed-off-by: Oleksii Kurochko > > Reviewed-by: Jan Beulich > > --- > > Changes in V8: > > * move from to 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. I tried to remove dependency between xen/lib.h and xen/bug.h but it is still the same issue but for different compilation unit: mmanual-endbr -fno-jump-tables '-D__OBJECT_LABEL__=asm-offsets.s' - mpreferred-stack-boundary=3 -S -g0 -o asm-offsets.s.new -MQ asm- offsets.s arch/x86/x86_64/asm-offsets.c In file included from ./include/public/domctl.h:21, from ./include/public/sysctl.h:18, from ./arch/x86/include/asm/cpuid.h:14, from ./arch/x86/include/asm/cpufeature.h:10, from ./arch/x86/include/asm/system.h:7, 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:6, from ./include/xen/bug.h:15, from ./arch/x86/include/asm/alternative.h:7, from ./arch/x86/include/asm/bitops.h:8, from ./include/xen/bitops.h:106, from ./arch/x86/include/asm/smp.h:8, from ./include/xen/smp.h:4, from ./include/xen/perfc.h:7, from arch/x86/x86_64/asm-offsets.c:9: ~ Oleksii
Re: [PATCH v8 5/5] xen/x86: switch x86 to use generic implemetation of bug.h
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 >>> * 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 ' >>> >>> Signed-off-by: Oleksii Kurochko >>> Reviewed-by: Jan Beulich >>> --- >>> Changes in V8: >>> * move from to 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 removed in > common/bug.c but leave ? 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
Re: [PATCH v8 5/5] xen/x86: switch x86 to use generic implemetation of bug.h
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 > > * 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 ' > > > > Signed-off-by: Oleksii Kurochko > > Reviewed-by: Jan Beulich > > --- > > Changes in V8: > > * move from to 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 removed in common/bug.c but leave ? Should I wait for xen/lib.h reworking? > > Jan > > > and the reason for that is when > > is included in :9 the "layout" > > of would be > > the following: > > #include : > > #include : > > #include : > > > > cpumask.h: > > > > ASSERT(cpu < nr_cpu_ids); > > return cpu; > > > > > > #define ASSERT ... > > > > Thereby ASSERT is defined after it was used in . > ~ Oleksii
Re: [PATCH v8 5/5] xen/x86: switch x86 to use generic implemetation of bug.h
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 > * 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 ' > > Signed-off-by: Oleksii Kurochko > Reviewed-by: Jan Beulich > --- > Changes in V8: > * move from to 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 > is included in :9 the "layout" of > would be >the following: > #include : > #include : > #include : > > cpumask.h: > > ASSERT(cpu < nr_cpu_ids); > return cpu; > > > #define ASSERT ... > >Thereby ASSERT is defined after it was used in .
