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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.03.2023 16:50, Oleksii Kurochko wrote:
> @@ -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;
> +    void *eip = (void *)regs->rip;

As said elsewhere already: "const" please whenever possible. The more that
the variable was pointer-to-const before.

> @@ -1185,83 +1183,20 @@ 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 = (unsigned char *)eip + sizeof(bug_insn);

Why don't you keep the original

    eip += sizeof(bug_insn);

? As also said before we use the GNU extension of arithmetic on pointers
to void pretty extensively elsewhere; there's no reason at all to
introduce an unnecessary and questionable cast here.

With these adjustments and with the re-basing over the changes requested
to patch 2
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan