Re: [PATCH v4 3/5] xen/riscv: introduce an implementation of macros from <asm/bug.h>

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.02.2023 12:35, Oleksii Kurochko wrote:
> The patch introduces macros: BUG(), WARN(), run_in_exception(),
> assert_failed.
> 
> The implementation uses "ebreak" instruction in combination with
> diffrent bug frame tables (for each type) which contains useful
> information.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
> Changes in V4:
>   - Updates in RISC-V's <asm/bug.h>:
>     * Add explanatory comment about why there is only defined for 32-bits 
> length
>       instructions and 16/32-bits BUG_INSN_{16,32}.
>     * Change 'unsigned long' to 'unsigned int' inside GET_INSN_LENGTH().
>     * Update declaration of is_valid_bugaddr(): switch return type from int 
> to bool
>       and the argument from 'unsigned int' to 'vaddr'.
>   - Updates in RISC-V's traps.c:
>     * replace /xen and /asm includes 
>     * update definition of is_valid_bugaddr():switch return type from int to 
> bool
>       and the argument from 'unsigned int' to 'vaddr'. Code style inside 
> function
>       was updated too.
>     * do_bug_frame() refactoring:
>       * local variables start and bug became 'const struct bug_frame'
>       * bug_frames[] array became 'static const struct bug_frame[] = ...'
>       * remove all casts
>       * remove unneeded comments and add an explanatory comment that the 
> do_bug_frame()
>         will be switched to a generic one.
>     * do_trap() refactoring:
>       * read 16-bits value instead of 32-bits as compressed instruction can
>         be used and it might happen than only 16-bits may be accessible.
>       * code style updates
>       * re-use instr variable instead of re-reading instruction.
>   - Updates in setup.c:
>     * add blank line between xen/ and asm/ includes.
> ---
> Changes in V3:
>   - Rebase the patch "xen/riscv: introduce an implementation of macros
>     from <asm/bug.h>" on top of patch series [introduce generic implementation
>     of macros from bug.h]
> ---
> Changes in V2:
>   - Remove __ in define namings
>   - Update run_in_exception_handler() with
>     register void *fn_ asm(__stringify(BUG_FN_REG)) = (fn);
>   - Remove bug_instr_t type and change it's usage to uint32_t
> ---
>  xen/arch/riscv/include/asm/bug.h       |  48 ++++++++++
>  xen/arch/riscv/include/asm/processor.h |   2 +
>  xen/arch/riscv/setup.c                 |   1 +
>  xen/arch/riscv/traps.c                 | 125 +++++++++++++++++++++++++
>  xen/arch/riscv/xen.lds.S               |  10 ++
>  5 files changed, 186 insertions(+)
>  create mode 100644 xen/arch/riscv/include/asm/bug.h
> 
> diff --git a/xen/arch/riscv/include/asm/bug.h 
> b/xen/arch/riscv/include/asm/bug.h
> new file mode 100644
> index 0000000000..67ade6f895
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/bug.h
> @@ -0,0 +1,48 @@
> +/* 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/types.h>
> +
> +#ifndef __ASSEMBLY__

This #ifndef contradicts the xen/types.h inclusion. Either the #include
moves down below here, or the #ifndef goes away as pointless. This is
because xen/types.h cannot be include in assembly files.

> +#define BUG_INSTR "ebreak"
> +
> +/*
> + * The base instruction set has a fixed length of 32-bit naturally aligned
> + * instructions.
> + *
> + * There are extensions of variable length ( where each instruction can be
> + * any number of 16-bit parcels in length ) but they aren't used in Xen
> + * and Linux kernel ( where these definitions were taken from ).
> + *
> + * Compressed ISA is used now where the instruction length is 16 bit  and
> + * 'ebreak' instruction, in this case, can be either 16 or 32 bit (
> + * depending on if compressed ISA is used or not )
> + */
> +#define INSN_LENGTH_MASK        _UL(0x3)
> +#define INSN_LENGTH_32          _UL(0x3)
> +
> +#define BUG_INSN_32             _UL(0x00100073) /* ebreak */
> +#define BUG_INSN_16             _UL(0x9002)     /* c.ebreak */
> +#define COMPRESSED_INSN_MASK    _UL(0xffff)
> +
> +#define GET_INSN_LENGTH(insn)                               \
> +({                                                          \
> +    unsigned int len;                                       \
> +    len = ((insn & INSN_LENGTH_MASK) == INSN_LENGTH_32) ?   \
> +        4UL : 2UL;                                          \

Why the UL suffixes?

> +    len;                                                    \
> +})

And anyway - can't the whole macro be written without using any
extension:

#define GET_INSN_LENGTH(insn)                               \
    (((insn) & INSN_LENGTH_MASK) == INSN_LENGTH_32 ? 4 : 2) \

? (Note also that uses of macro parameters should be properly
parenthesized.)

> --- a/xen/arch/riscv/setup.c
> +++ b/xen/arch/riscv/setup.c
> @@ -1,3 +1,4 @@
> +#include <xen/bug.h>
>  #include <xen/compile.h>
>  #include <xen/init.h>

Why? Eventually this will be needed, I agree, but right now?

> @@ -99,7 +100,131 @@ static void do_unexpected_trap(const struct 
> cpu_user_regs *regs)
>      die();
>  }
>  
> +void show_execution_state(const struct cpu_user_regs *regs)
> +{
> +    early_printk("implement show_execution_state(regs)\n");
> +}
> +
> +/*
> + * TODO: change early_printk's function to early_printk with format
> + *       when s(n)printf() will be added.
> + *
> + * Probably the TODO won't be needed as generic do_bug_frame() (at
> + * least, for ARM and RISC-V) has been introduced and current
> + * implementation will be replaced with generic one when panic(),
> + * printk() and find_text_region() (virtual memory?) will be
> + * ready/merged
> + */
> +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 = 0;
> +    const char *filename, *predicate;
> +    int lineno;
> +
> +    static const struct bug_frame* 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;
> +    }
> +
> +    /* WARN, BUG or ASSERT: decode the filename pointer and line number. */
> +    filename = bug_ptr(bug);
> +    lineno = bug_line(bug);
> +
> +    switch ( id )
> +    {
> +    case BUGFRAME_warn:
> +        early_printk("Xen WARN at ");
> +        early_printk(filename);
> +        early_printk("\n");
> +
> +        show_execution_state(regs);
> +
> +        goto end;
> +
> +    case BUGFRAME_bug:
> +        early_printk("Xen BUG at ");
> +        early_printk(filename);
> +        early_printk("\n");
> +
> +        show_execution_state(regs);
> +        early_printk("change wait_for_interrupt to panic() when common is 
> available\n");
> +        die();
> +
> +    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("\n");
> +
> +        show_execution_state(regs);
> +        early_printk("change wait_for_interrupt to panic() when common is 
> available\n");
> +        die();
> +    }
> +
> +    return -EINVAL;
> +
> + end:
> +    return 0;
> +}

Wasn't the goal to use the generic do_bug_frame()?

> +bool is_valid_bugaddr(vaddr_t insn)

Why is insn of type vaddr_t?

> +{
> +    if ( (insn & INSN_LENGTH_MASK) == INSN_LENGTH_32 )
> +        return ( insn == BUG_INSN_32 );
> +    else
> +        return ( (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16 );

Nit (style): The kind-of-operand to return is an expression. Hence
you have stray blanks there immediately inside the parentheses.
(This is unlike e.g. if(), where you've formatted things correctly.)

> +}
> +
>  void do_trap(struct cpu_user_regs *cpu_regs)
>  {
> +    register_t pc = cpu_regs->sepc;
> +    uint16_t instr = *(uint16_t *)pc;
> +
> +    if ( is_valid_bugaddr(instr) ) {
> +        if ( !do_bug_frame(cpu_regs, cpu_regs->sepc) ) {

Nit: Brace placement (twice).

Jan