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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Tue, 2023-03-07 at 13:59 +0100, Jan Beulich wrote:
> On 07.03.2023 13:52, Oleksii wrote:
> > On Mon, 2023-03-06 at 11:36 +0100, Jan Beulich wrote:
> > > On 03.03.2023 11:38, Oleksii Kurochko wrote:
> > > > --- 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__ */"
> 
> Since you use plural, maybe a misunderstanding? My comment was solely
> on the line you add; the removed lines are there merely as context.
Really. I misunderstood you.
> 
> > > > @@ -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);
> 
> Arithmetic on pointers to void is a GNU extension which we make
> heavy use of all over the hypervisor.
Got it. Than I'll rework it with 'void *'.
> 
> > > >      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;
> 
> Except the "return" needs to remain inside the switch; unrecognized
> id
> values should still fall through to the "die" label.
Yeah, it is still possible to get unrecognized id.
Thanks.
> 

~ Oleksii