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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

>>> @@ -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.

>>>      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.

Jan