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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Wed, 2023-03-08 at 16:33 +0100, Jan Beulich wrote:
> 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.
Sure, I will change it than to:
 const void *eip = (const void *)regs->rip;
> 
> > @@ -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.
Just missed that during rebase. ( I experimented with the branch and
received conflicts that were resolved incorrectly )

I will update that. Thanks

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

~ Oleksii