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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Mon, 2023-03-06 at 11:36 +0100, Jan Beulich wrote:
> 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 ...
It is stale. Updated now.
> 
> > ---
> >  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.
But it was defined there before the patch series and these definies are
used in "#else /* !__ASSEMBLY__ */"
> 
> > @@ -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"?
If we change it to "void" shouldn't it require additional casts here (
which is not nice ):
       eip += sizeof(bug_insn);
Probably 'unsgined char' will be better.
> 
> > @@ -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.
Oh, you are right. So then in case we have correct id it is needed to
do only return:
    switch ( id )
    {
    case BUGFRAME_run_fn:
    case BUGFRAME_warn:
        fixup_exception_return(regs, (unsigned long)eip);
        break;
    }

    return;


Thanks for finding that.

~ Oleksii