On 03.03.2023 11:38, 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>
> * Change prototype of debugger_trap_fatal() in asm/debugger.h
> to align it with generic prototype in <xen/debugger.h>.
Is this part of the description stale? The patch ...
> ---
> xen/arch/x86/Kconfig | 1 +
> xen/arch/x86/include/asm/bug.h | 73 ++----------------------------
> xen/arch/x86/traps.c | 81 +++-------------------------------
> 3 files changed, 11 insertions(+), 144 deletions(-)
... doesn't touch asm/debugger.h at all.
> --- a/xen/arch/x86/include/asm/bug.h
> +++ b/xen/arch/x86/include/asm/bug.h
> @@ -1,79 +1,12 @@
> #ifndef __X86_BUG_H__
> #define __X86_BUG_H__
>
> -#define BUG_DISP_WIDTH 24
> -#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
> -#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
> +#define BUG_DEBUGGER_TRAP_FATAL(regs) debugger_trap_fatal(X86_EXC_GP,regs)
Along the lines of a comment on an earlier patch, please move this ...
> #ifndef __ASSEMBLY__
... into the thus guarded section.
> @@ -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 char *eip = (char *)regs->rip;
I realize "char" was used before, but now that this is all on its own,
can this at least become "unsigned char", but yet better "void"?
> @@ -1185,83 +1183,18 @@ 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;
This and ...
> - 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;
... this return look to have no proper representation in the new
logic; both cases fall through to ...
> - show_execution_state(regs);
> - panic("Assertion '%s' failed at %s%s:%d\n",
> - predicate, prefix, filename, lineno);
> }
>
> die:
... here now, which is an unwanted functional change.
Jan
