|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v1 12/14] xen/riscv: introduce an implementation of macros from <asm/bug.h>
On 20.01.2023 15:59, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/bug.h
> @@ -0,0 +1,120 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2012 Regents of the University of California
> + * Copyright (C) 2021-2023 Vates
> + *
> + */
> +
> +#ifndef _ASM_RISCV_BUG_H
> +#define _ASM_RISCV_BUG_H
> +
> +#include <xen/stringify.h>
> +#include <xen/types.h>
> +
> +#ifndef __ASSEMBLY__
> +
> +struct bug_frame {
> + signed int loc_disp; /* Relative address to the bug address */
> + signed int file_disp; /* Relative address to the filename */
> + signed int msg_disp; /* Relative address to the predicate (for
> ASSERT) */
> + uint16_t line; /* Line number */
> + uint32_t pad0:16; /* Padding for 8-bytes align */
> +};
> +
> +#define bug_loc(b) ((const void *)(b) + (b)->loc_disp)
> +#define bug_file(b) ((const void *)(b) + (b)->file_disp);
> +#define bug_line(b) ((b)->line)
> +#define bug_msg(b) ((const char *)(b) + (b)->msg_disp)
> +
> +#define BUGFRAME_run_fn 0
> +#define BUGFRAME_warn 1
> +#define BUGFRAME_bug 2
> +#define BUGFRAME_assert 3
> +
> +#define BUGFRAME_NR 4
> +
> +#define __INSN_LENGTH_MASK _UL(0x3)
> +#define __INSN_LENGTH_32 _UL(0x3)
> +#define __COMPRESSED_INSN_MASK _UL(0xffff)
> +
> +#define __BUG_INSN_32 _UL(0x00100073) /* ebreak */
> +#define __BUG_INSN_16 _UL(0x9002) /* c.ebreak */
May I suggest that you avoid double-underscore (or other reserved) names
where possible?
> +#define GET_INSN_LENGTH(insn)
> \
> +({ \
> + unsigned long __len; \
> + __len = ((insn & __INSN_LENGTH_MASK) == __INSN_LENGTH_32) ? \
> + 4UL : 2UL; \
> + __len; \
> +})
> +
> +typedef u32 bug_insn_t;
This is problematic beyond the u32 instead of uint32_t. You use it once, ...
> +/* These are defined by the architecture */
> +int is_valid_bugaddr(bug_insn_t addr);
... in a call to this function, but you can't assume that you can access
32 bits when the insn you look at might be a compressed one. Just to be
on the safe side I'd like to suggest to either avoid such a type, or to
introduce two (32- and 16-bit) which then of course need using properly
in respective contexts.
> +#define BUG_FN_REG t0
> +
> +/* Many versions of GCC doesn't support the asm %c parameter which would
> + * be preferable to this unpleasantness. We use mergeable string
> + * sections to avoid multiple copies of the string appearing in the
> + * Xen image. BUGFRAME_run_fn needs to be handled separately.
> + */
> +#define BUG_FRAME(type, line, file, has_msg, msg) do { \
> + asm ("1:ebreak\n"
> \
Something's odd with the padding here; looks like there might be hard tabs.
> + ".pushsection .rodata.str, \"aMS\", %progbits, 1\n" \
> + "2:\t.asciz " __stringify(file) "\n" \
> + "3:\n" \
> + ".if " #has_msg "\n" \
> + "\t.asciz " #msg "\n" \
> + ".endif\n" \
> + ".popsection\n" \
> + ".pushsection .bug_frames." __stringify(type) ", \"a\",
> %progbits\n"\
> + "4:\n" \
> + ".p2align 2\n" \
> + ".long (1b - 4b)\n" \
> + ".long (2b - 4b)\n" \
> + ".long (3b - 4b)\n" \
> + ".hword " __stringify(line) ", 0\n" \
> + ".popsection"); \
> +} while (0)
> +
> +/*
> + * GCC will not allow to use "i" when PIE is enabled (Xen doesn't set the
> + * flag but instead rely on the default value from the compiler). So the
> + * easiest way to implement run_in_exception_handler() is to pass the to
> + * be called function in a fixed register.
> + */
> +#define run_in_exception_handler(fn) do { \
With
register void *fn_ asm(__stringify(BUG_FN_REG)) = (fn);
you should be able to avoid ...
> + asm ("mv " __stringify(BUG_FN_REG) ", %0\n"
> \
... this and simply use ...
> + "1:ebreak\n"
> \
> + ".pushsection .bug_frames." __stringify(BUGFRAME_run_fn) "," \
> + " \"a\", %%progbits\n" \
> + "2:\n" \
> + ".p2align 2\n" \
> + ".long (1b - 2b)\n" \
> + ".long 0, 0, 0\n" \
> + ".popsection" :: "r" (fn) : __stringify(BUG_FN_REG) ); \
:: "r" (fn_) );
here. See x86's alternative_callN() for similar examples.
> @@ -107,7 +108,122 @@ static void do_unexpected_trap(const struct
> cpu_user_regs *regs)
> wait_for_interrupt();
> }
>
> +void show_execution_state(const struct cpu_user_regs *regs)
> +{
> + early_printk("implement show_execution_state(regs)\n");
> +}
> +
> +int do_bug_frame(struct cpu_user_regs *regs, vaddr_t pc)
> +{
> + struct bug_frame *start, *end;
> + struct bug_frame *bug = NULL;
> + unsigned int id = 0;
> + const char *filename, *predicate;
> + int lineno;
> +
> + unsigned long bug_frames[] = {
> + (unsigned long)&__start_bug_frames[0],
> + (unsigned long)&__stop_bug_frames_0[0],
> + (unsigned long)&__stop_bug_frames_1[0],
> + (unsigned long)&__stop_bug_frames_2[0],
> + (unsigned long)&__stop_bug_frames_3[0],
> + };
> +
> + for ( id = 0; id < BUGFRAME_NR; id++ )
> + {
> + start = (struct bug_frame *)bug_frames[id];
> + end = (struct bug_frame *)bug_frames[id + 1];
> +
> + while ( start != end )
> + {
> + if ( (vaddr_t)bug_loc(start) == pc )
> + {
> + bug = start;
> + goto found;
> + }
> +
> + start++;
> + }
> + }
> +
> +found:
Please indent labels by at least one blank; see ./CODING_STYLE.
> + if ( bug == NULL )
> + return -ENOENT;
> +
> + if ( id == BUGFRAME_run_fn )
> + {
> + void (*fn)(const struct cpu_user_regs *) = (void *)regs->BUG_FN_REG;
> +
> + fn(regs);
> +
> + goto end;
> + }
> +
> + /* WARN, BUG or ASSERT: decode the filename pointer and line number. */
> + filename = bug_file(bug);
> + lineno = bug_line(bug);
> +
> + switch ( id )
> + {
> + case BUGFRAME_warn:
> + early_printk("Xen WARN at ");
> + early_printk(filename);
> + early_printk(":");
> + early_printk_hnum(lineno);
> +
> + show_execution_state(regs);
> +
> + goto end;
> +
> + case BUGFRAME_bug:
> + early_printk("Xen BUG at ");
> + early_printk(filename);
> + early_printk(":");
> + early_printk_hnum(lineno);
> +
> + show_execution_state(regs);
> + early_printk("change wait_for_interrupt to panic() when common is
> available\n");
> + wait_for_interrupt();
> +
> + case BUGFRAME_assert:
> + /* ASSERT: decode the predicate string pointer. */
> + predicate = bug_msg(bug);
> +
> + early_printk("Assertion \'");
> + early_printk(predicate);
> + early_printk("\' failed at ");
> + early_printk(filename);
> + early_printk(":");
> + early_printk_hnum(lineno);
> +
> + show_execution_state(regs);
> + early_printk("change wait_for_interrupt to panic() when common is
> available\n");
> + wait_for_interrupt();
> + }
> +
> + return -EINVAL;
> +end:
> + regs->sepc += GET_INSN_LENGTH(*(bug_insn_t *)pc);
> +
> + return 0;
> +}
> +
> +int is_valid_bugaddr(bug_insn_t insn)
> +{
> + if ((insn & __INSN_LENGTH_MASK) == __INSN_LENGTH_32)
> + return (insn == __BUG_INSN_32);
> + else
> + return ((insn & __COMPRESSED_INSN_MASK) == __BUG_INSN_16);
> +}
> +
> void __handle_exception(struct cpu_user_regs *cpu_regs)
> {
> + register_t pc = cpu_regs->sepc;
> + uint32_t instr = *(bug_insn_t *)pc;
> +
> + if (is_valid_bugaddr(instr))
> + if (!do_bug_frame(cpu_regs, pc)) return;
> +
> +// die:
Perhaps better to omit the label until it's actually needed? In any
event you shouldn't be using C++-style comments (see ./CODING_STYLE
again).
Jan
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |