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
