Re: [PATCH v8 1/5] xen: introduce CONFIG_GENERIC_BUG_FRAME

[Jan Beulich <jbeulich@suse.com>]

On 21.03.2023 12:18, Oleksii wrote:
> On Mon, 2023-03-20 at 13:36 +0200, Oleksii wrote:
>> On Fri, 2023-03-17 at 15:59 +0100, Jan Beulich wrote:
>>> On 17.03.2023 10:23, Oleksii wrote:
>>>> On Thu, 2023-03-16 at 12:26 +0100, Jan Beulich wrote:
>>>>> On 15.03.2023 18:21, Oleksii Kurochko wrote:
>>>>>> --- /dev/null
>>>>>> +++ b/xen/common/bug.c
>>>>>> @@ -0,0 +1,108 @@
>>>>>> +#include <xen/bug.h>
>>>>>> +#include <xen/debugger.h>
>>>>>> +#include <xen/errno.h>
>>>>>> +#include <xen/kernel.h>
>>>>>> +#include <xen/livepatch.h>
>>>>>> +#include <xen/string.h>
>>>>>> +#include <xen/types.h>
>>>>>> +#include <xen/virtual_region.h>
>>>>>> +
>>>>>> +#include <asm/processor.h>
>>>>>
>>>>> I actually meant to also ask: What is this needed for? Glancing
>>>>> over
>>>>> the
>>>>> code ...
>>>>>
>>>>>> +/*
>>>>>> + * Returns a negative value in case of an error otherwise
>>>>>> + * BUGFRAME_{run_fn, warn, bug, assert}
>>>>>> + */
>>>>>> +int do_bug_frame(struct cpu_user_regs *regs, unsigned long
>>>>>> pc)
>>>>>> +{
>>>>>> +    const struct bug_frame *bug = NULL;
>>>>>> +    const struct virtual_region *region;
>>>>>> +    const char *prefix = "", *filename, *predicate;
>>>>>> +    unsigned long fixup;
>>>>>> +    unsigned int id, lineno;
>>>>>> +
>>>>>> +    region = find_text_region(pc);
>>>>>> +    if ( !region )
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    for ( id = 0; id < BUGFRAME_NR; id++ )
>>>>>> +    {
>>>>>> +        const struct bug_frame *b;
>>>>>> +        size_t i;
>>>>>> +
>>>>>> +        for ( i = 0, b = region->frame[id].bugs;
>>>>>> +              i < region->frame[id].n_bugs; b++, i++ )
>>>>>> +        {
>>>>>> +            if ( bug_loc(b) == pc )
>>>>>> +            {
>>>>>> +                bug = b;
>>>>>> +                goto found;
>>>>>> +            }
>>>>>> +        }
>>>>>> +    }
>>>>>> +
>>>>>> + found:
>>>>>> +    if ( !bug )
>>>>>> +        return -ENOENT;
>>>>>> +
>>>>>> +    if ( id == BUGFRAME_run_fn )
>>>>>> +    {
>>>>>> +        void (*fn)(struct cpu_user_regs *) = bug_ptr(bug);
>>>>>> +
>>>>>> +        fn(regs);
>>>>>> +
>>>>>> +        /* Re-enforce consistent types, because of the casts
>>>>>> involved. */
>>>>>> +        if ( false )
>>>>>> +            run_in_exception_handler(fn);
>>>>>> +
>>>>>> +        return id;
>>>>>> +    }
>>>>>> +
>>>>>> +    /* WARN, BUG or ASSERT: decode the filename pointer and
>>>>>> line
>>>>>> number. */
>>>>>> +    filename = bug_ptr(bug);
>>>>>> +    if ( !is_kernel(filename) && !is_patch(filename) )
>>>>>> +        return -EINVAL;
>>>>>> +    fixup = strlen(filename);
>>>>>> +    if ( fixup > 50 )
>>>>>> +    {
>>>>>> +        filename += fixup - 47;
>>>>>> +        prefix = "...";
>>>>>> +    }
>>>>>> +    lineno = bug_line(bug);
>>>>>> +
>>>>>> +    switch ( id )
>>>>>> +    {
>>>>>> +    case BUGFRAME_warn:
>>>>>> +        printk("Xen WARN at %s%s:%d\n", prefix, filename,
>>>>>> lineno);
>>>>>> +        show_execution_state(regs);
>>>>>> +
>>>>>> +        break;
>>>>>> +
>>>>>> +    case BUGFRAME_bug:
>>>>>> +        printk("Xen BUG at %s%s:%d\n", prefix, filename,
>>>>>> lineno);
>>>>>> +
>>>>>> +        if ( BUG_DEBUGGER_TRAP_FATAL(regs) )
>>>>>> +            break;
>>>>>> +
>>>>>> +        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 ( BUG_DEBUGGER_TRAP_FATAL(regs) )
>>>>>> +            break;
>>>>>> +
>>>>>> +        show_execution_state(regs);
>>>>>> +        panic("Assertion '%s' failed at %s%s:%d\n",
>>>>>> +              predicate, prefix, filename, lineno);
>>>>>> +    }
>>>>>> +
>>>>>> +    return id;
>>>>>> +}
>>>>>
>>>>> ... I can't really spot what it might be that comes from that
>>>>> header.
>>>>> Oh, on the N+1st run I've spotted it - it's
>>>>> show_execution_state().
>>>>> The declaration of which, already being used from common code
>>>>> ahead
>>>>> of this series, should imo be moved to a common header. I guess
>>>>> I'll
>>>>> make yet another patch ...
>>>> As mentioned above. Not only show_execution_state() but also
>>>> cpu_user_regs structure. ( at lest, for ARM & RISCV )
>>>
>>> Do we deref "regs" anywhere? I can't seem to be able to spot an
>>> instance.
>>> Without a deref (or alike) a forward decl is all that's needed for
>>> this
>>> code to compile.
>> You are there is no a deref so let's swich to a forward decl.
>>
>> I'll add it to a new version of the patch series.
> I just realized that show_execution_state() is declared in
> <asm/processor.h>.

Not anymore with "move {,vcpu_}show_execution_state() declarations
to common header", which was specifically made ...

> So we have to leave an inclusion of the header or declare the function
> explicitly.

... to eliminate this dependency, but which sadly is still pending an
Arm side ack.

Jan