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@gmail.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
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);
It happens in case when CONFIG_CRASH_DEBUG is enabled 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>.
* Rebase the patch series on the top of the latest staging: it was a merge conflict
inisde x86/Kconfig.
---
Changes in V7:
* update the commit message
* make eip 'const void *'
* change [eip = (unsigned char *)eip + sizeof(bug_insn);] to [eip += sizeof(bug_insn);]
* add '#include <xen/debugger.h>' to <asm/bug.h>
* add Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Changes in V6:
* update the commit message
* update the type of eip to 'void *' in do_invalid_op()
* fix the logic of do_invalid_op()
* move macros BUG_DEBUGGER_TRAP_FATAL under #ifndef __ASSEMBLY__ as
it is not necessary to be in assembly code.
---
Changes in V5:
* Nothing changed
---
Changes in V4:
* Back comment /* !__ASSEMBLY__ */ for #else case in <asm/bug.h>
* Remove changes related to x86/.../asm/debuger.h as do_bug_frame() prototype
was updated and cpu_user_regs isn't const any more.
---
Changes in V3:
* As prototype and what do_bug_frame() returns was changed so patch 3 and 4
was updated to use a new version of do_bug_frame
* MODIFIER was change to BUG_ASM_CONST to align with generic implementation
---
Changes in V2:
* Remove all unnecessary things from <asm/bug.h> as they were introduced in
<xen/bug.h>.
* Define BUG_INSTR = 'ud2' and MODIFIER = 'c' ( it is needed to skip '$'
when use an imidiate in x86 assembly )
* Update do_invalid_op() to re-use handle_bug_frame() and find_bug_frame()
from generic implemetation of CONFIG_GENERIC_BUG_FRAME
* Code style fixes.
---
xen/arch/x86/Kconfig | 1 +
xen/arch/x86/include/asm/bug.h | 69 ++---------------------------
xen/arch/x86/traps.c | 81 ++++------------------------------
3 files changed, 12 insertions(+), 139 deletions(-)
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 2a5c3304e2..406445a358 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -11,6 +11,7 @@ config X86
select ARCH_MAP_DOMAIN_PAGE
select ARCH_SUPPORTS_INT128
imply CORE_PARKING
+ select GENERIC_BUG_FRAME
select HAS_ALTERNATIVE
select HAS_COMPAT
select HAS_CPUFREQ
diff --git a/xen/arch/x86/include/asm/bug.h b/xen/arch/x86/include/asm/bug.h
index 4b3e7b019d..f852cd0ee9 100644
--- a/xen/arch/x86/include/asm/bug.h
+++ b/xen/arch/x86/include/asm/bug.h
@@ -3,73 +3,10 @@
#ifndef __ASSEMBLY__
-#define BUG_FRAME_STRUCT
+#define BUG_DEBUGGER_TRAP_FATAL(regs) debugger_trap_fatal(X86_EXC_GP,regs)
-struct bug_frame {
- signed int loc_disp:BUG_DISP_WIDTH;
- unsigned int line_hi:BUG_LINE_HI_WIDTH;
- signed int ptr_disp:BUG_DISP_WIDTH;
- unsigned int line_lo:BUG_LINE_LO_WIDTH;
- signed int msg_disp[];
-};
-
-#define bug_loc(b) ((const void *)(b) + (b)->loc_disp)
-#define bug_ptr(b) ((const void *)(b) + (b)->ptr_disp)
-#define bug_line(b) (((((b)->line_hi + ((b)->loc_disp < 0)) & \
- ((1 << BUG_LINE_HI_WIDTH) - 1)) << \
- BUG_LINE_LO_WIDTH) + \
- (((b)->line_lo + ((b)->ptr_disp < 0)) & \
- ((1 << BUG_LINE_LO_WIDTH) - 1)))
-#define bug_msg(b) ((const char *)(b) + (b)->msg_disp[1])
-
-#define _ASM_BUGFRAME_TEXT(second_frame) \
- ".Lbug%=: ud2\n" \
- ".pushsection .bug_frames.%c[bf_type], \"a\", @progbits\n" \
- ".p2align 2\n" \
- ".Lfrm%=:\n" \
- ".long (.Lbug%= - .Lfrm%=) + %c[bf_line_hi]\n" \
- ".long (%c[bf_ptr] - .Lfrm%=) + %c[bf_line_lo]\n" \
- ".if " #second_frame "\n" \
- ".long 0, %c[bf_msg] - .Lfrm%=\n" \
- ".endif\n" \
- ".popsection\n" \
-
-#define _ASM_BUGFRAME_INFO(type, line, ptr, msg) \
- [bf_type] "i" (type), \
- [bf_ptr] "i" (ptr), \
- [bf_msg] "i" (msg), \
- [bf_line_lo] "i" ((line & ((1 << BUG_LINE_LO_WIDTH) - 1)) \
- << BUG_DISP_WIDTH), \
- [bf_line_hi] "i" (((line) >> BUG_LINE_LO_WIDTH) << BUG_DISP_WIDTH)
-
-#define BUG_FRAME(type, line, ptr, second_frame, msg) do { \
- BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH)); \
- BUILD_BUG_ON((type) >= BUGFRAME_NR); \
- asm volatile ( _ASM_BUGFRAME_TEXT(second_frame) \
- :: _ASM_BUGFRAME_INFO(type, line, ptr, msg) ); \
-} while (0)
-
-
-#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, NULL)
-#define BUG() do { \
- BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, 0, NULL); \
- unreachable(); \
-} while (0)
-
-/*
- * 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 ( 0 )
-
-#define assert_failed(msg) do { \
- BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg); \
- unreachable(); \
-} while (0)
+#define BUG_INSTR "ud2"
+#define BUG_ASM_CONST "c"
#else /* !__ASSEMBLY__ */
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index cade9e12f8..c36e3f855b 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -24,6 +24,7 @@
* Gareth Hughes <gareth@valinux.com>, May 2000
*/
+#include <xen/bug.h>
#include <xen/init.h>
#include <xen/sched.h>
#include <xen/lib.h>
@@ -1166,12 +1167,9 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
void do_invalid_op(struct cpu_user_regs *regs)
{
- const struct bug_frame *bug = NULL;
u8 bug_insn[2];
- const char *prefix = "", *filename, *predicate, *eip = (char *)regs->rip;
- unsigned long fixup;
- int id = -1, lineno;
- const struct virtual_region *region;
+ const void *eip = (const void *)regs->rip;
+ int id;
if ( likely(guest_mode(regs)) )
{
@@ -1185,83 +1183,20 @@ void do_invalid_op(struct cpu_user_regs *regs)
memcmp(bug_insn, "\xf\xb", sizeof(bug_insn)) )
goto die;
- region = find_text_region(regs->rip);
- if ( region )
- {
- for ( id = 0; id < BUGFRAME_NR; id++ )
- {
- const struct bug_frame *b;
- unsigned int i;
-
- for ( i = 0, b = region->frame[id].bugs;
- i < region->frame[id].n_bugs; b++, i++ )
- {
- if ( bug_loc(b) == eip )
- {
- bug = b;
- goto found;
- }
- }
- }
- }
-
- found:
- if ( !bug )
+ id = do_bug_frame(regs, regs->rip);
+ if ( id < 0 )
goto die;
- eip += sizeof(bug_insn);
- if ( id == BUGFRAME_run_fn )
- {
- void (*fn)(struct cpu_user_regs *) = bug_ptr(bug);
-
- fn(regs);
- fixup_exception_return(regs, (unsigned long)eip);
- return;
- }
- /* WARN, BUG or ASSERT: decode the filename pointer and line number. */
- filename = bug_ptr(bug);
- if ( !is_kernel(filename) && !is_patch(filename) )
- goto die;
- fixup = strlen(filename);
- if ( fixup > 50 )
- {
- filename += fixup - 47;
- prefix = "...";
- }
- lineno = bug_line(bug);
+ eip += sizeof(bug_insn);
switch ( id )
{
+ case BUGFRAME_run_fn:
case BUGFRAME_warn:
- printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno);
- show_execution_state(regs);
fixup_exception_return(regs, (unsigned long)eip);
- return;
-
case BUGFRAME_bug:
- printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
-
- if ( debugger_trap_fatal(TRAP_invalid_op, regs) )
- return;
-
- show_execution_state(regs);
- panic("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
-
case BUGFRAME_assert:
- /* ASSERT: decode the predicate string pointer. */
- predicate = bug_msg(bug);
- if ( !is_kernel(predicate) && !is_patch(predicate) )
- predicate = "<unknown>";
-
- printk("Assertion '%s' failed at %s%s:%d\n",
- predicate, prefix, filename, lineno);
-
- if ( debugger_trap_fatal(TRAP_invalid_op, regs) )
- return;
-
- show_execution_state(regs);
- panic("Assertion '%s' failed at %s%s:%d\n",
- predicate, prefix, filename, lineno);
+ return;
}
die:
--
2.39.2On 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@gmail.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> > --- > 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>.
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@gmail.com>
> > Reviewed-by: Jan Beulich <jbeulich@suse.com>
> > ---
> > 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.
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
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 <asm/bug.h> includes <xen/debugger.h> which somewhere inside uses BUG() which will be defined after we will back for x86's <asm/bug.h> to <xen/bug.h> where BUG() is defined. So it looks like we can't include to <asm/bug.h> something that will use functionality defined in <xen/bug.h>... Thereby I don't see better option that include <xen/debugger.h> in <common/bug.c> instead of <asm/bug.h> ~ Oleksii
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 <asm/bug.h> includes > <xen/debugger.h> which somewhere inside uses BUG() which will be > defined after we will back for x86's <asm/bug.h> to <xen/bug.h> where > BUG() is defined. > > So it looks like we can't include to <asm/bug.h> something that will > use functionality defined in <xen/bug.h>... > > Thereby I don't see better option that include <xen/debugger.h> in > <common/bug.c> instead of <asm/bug.h> Or as an option we can include <xen/bug.h> in <asm/bug.h> instead of include <asm/bug.h> in <xen/bug.h> it will allow us to resolve an issue... Do you think this option will be better? ~ Oleksii
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 <asm/bug.h> includes >> <xen/debugger.h> which somewhere inside uses BUG() which will be >> defined after we will back for x86's <asm/bug.h> to <xen/bug.h> where >> BUG() is defined. >> >> So it looks like we can't include to <asm/bug.h> something that will >> use functionality defined in <xen/bug.h>... >> >> Thereby I don't see better option that include <xen/debugger.h> in >> <common/bug.c> instead of <asm/bug.h> 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 <xen/bug.h> in <asm/bug.h> instead of > include <asm/bug.h> in <xen/bug.h> 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
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@gmail.com> > > Reviewed-by: Jan Beulich <jbeulich@suse.com> > > --- > > 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>? Should I wait for xen/lib.h reworking? > > 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>. > ~ Oleksii
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@gmail.com> >>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >>> --- >>> 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
© 2016 - 2024 Red Hat, Inc.