Re: [PATCH v1 12/14] xen/riscv: introduce an implementation of macros from <asm/bug.h>

[Jan Beulich <jbeulich@suse.com>]

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