On 11.08.2023 16:32, Oleksii Kurochko wrote:
> The patch introduces the temporary implementation of do_bug_frame().
> When common code is merged, CONFIG_GENERIC_BUG_FRAME should
> be used instead.
>
> Signed-off-by: Oleksii Kurochko <oleksii.kuroc...@gmail.com>
> ---
> Changes in V8:
> - remove Pointless initializer of id.
> - make bug_frames[] array constant.
> - remove cast_to_bug_frame(addr).
> - rename is_valig_bugaddr to is_valid_bug_insn().
> - add check that read_instr is used only on xen code
> - update the commit message.
Looks largely okay to me now, yet still a couple of remarks / suggestions:
> +int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
> +{
> + const struct bug_frame *start, *end;
> + const struct bug_frame *bug = NULL;
> + unsigned int id;
> + const char *filename, *predicate;
> + int lineno;
> +
> + static const struct bug_frame * const bug_frames[] = {
> + &__start_bug_frames[0],
> + &__stop_bug_frames_0[0],
> + &__stop_bug_frames_1[0],
> + &__stop_bug_frames_2[0],
> + &__stop_bug_frames_3[0],
> + };
> +
> + for ( id = 0; id < BUGFRAME_NR; id++ )
> + {
> + start = bug_frames[id];
> + end = bug_frames[id + 1];
> +
> + while ( start != end )
> + {
> + if ( (vaddr_t)bug_loc(start) == pc )
> + {
> + bug = start;
> + goto found;
> + }
> +
> + start++;
> + }
> + }
> +
> + found:
> + if ( bug == NULL )
> + return -ENOENT;
> +
> + if ( id == BUGFRAME_run_fn )
> + {
> + void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug);
> +
> + fn(regs);
> +
> + goto end;
I'd like to suggest that you avoid "goto" when at the label there's just
a single, simple statement (like return in the case here).
> + }
> +
> + /* WARN, BUG or ASSERT: decode the filename pointer and line number. */
> + filename = bug_ptr(bug);
> + lineno = bug_line(bug);
> +
> + switch ( id )
> + {
> + case BUGFRAME_warn:
> + printk("Xen WARN at %s:%d\n", filename, lineno);
> +
> + show_execution_state(regs);
> +
> + goto end;
> +
> + case BUGFRAME_bug:
> + printk("Xen BUG at %s:%d\n", filename, lineno);
> +
> + show_execution_state(regs);
> +
> + printk("change wait_for_interrupt to panic() when common is
> available\n");
> + die();
The log message here ...
> + case BUGFRAME_assert:
> + /* ASSERT: decode the predicate string pointer. */
> + predicate = bug_msg(bug);
> +
> + printk("Assertion %s failed at %s:%d\n", predicate, filename,
> lineno);
> +
> + show_execution_state(regs);
> +
> + printk("change wait_for_interrupt to panic() when common is
> available\n");
> + die();
... and here doesn't really fit with the following code. There's no
wait_for_interrupt() in sight.
> + }
> +
> + return -EINVAL;
> +
> + end:
> + return 0;
> +}
> +
> +static bool is_valid_bug_insn(uint32_t insn)
> +{
> + return insn == BUG_INSN_32 ||
> + (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16;
> +}
> +
> +/* There are no guests for now so the func has to return alwasys false */
> +static inline bool guest_mode(void)
> +{
> + return false;
> +}
I think this makes little sense to have, and ...
> +/* Should be used only on Xen code */
... just the comment is good enough. First and foremost I don't think
a function with name and purpose of the above can get away without
any parameters. Furthermore, as said before, ...
> +static uint32_t read_instr(unsigned long pc)
> +{
> + uint16_t instr16 = *(uint16_t *)pc;
... such casting can't be legitimate for guest addresses anyway; you
would need to map guest memory instead. What you might do ...
> + if ( guest_mode() )
> + {
> + printk("%s should be called only on Xen code\n", __func__);
> + die();
> + }
... instead of this, as some minimalistic checking, is
ASSERT(is_kernel_text(pc + 1) || is_kernel_inittext(pc + 1));
> + if ( GET_INSN_LENGTH(instr16) == 2 )
> + return instr16;
> + else
> + return *(uint32_t *)pc;
and then
if ( GET_INSN_LENGTH(instr16) == 2 )
return instr16;
ASSERT(is_kernel_text(pc + 3) || is_kernel_inittext(pc + 3));
return *(uint32_t *)pc;
Yet then again asserting in exception handlers is of limited use
only, as then you risk recursive exceptions.
> void do_trap(struct cpu_user_regs *cpu_regs)
> {
> + register_t pc = cpu_regs->sepc;
> + uint32_t instr = read_instr(pc);
> +
> + if ( is_valid_bug_insn(instr) )
> + {
> + if ( !do_bug_frame(cpu_regs, pc) )
Two if()-s like these typically want folding into one.
Jan
