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

[Jan Beulich <jbeulich@xxxxxxxx>]

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