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

Commit Message

Oleksii Kurochko Feb. 24, 2023, 11:35 a.m. UTC

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 <[e-mail]>
---
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

Comments

Jan Beulich Feb. 27, 2023, 12:59 p.m. UTC | #1

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 <[e-mail]>
> ---
> 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

Oleksii Kurochko March 1, 2023, 11:51 a.m. UTC | #2

On Mon, 2023-02-27 at 13:59 +0100, Jan Beulich wrote:
> 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 <[e-mail]>
> > ---
> > 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.
I will remove it. It looks like it leave when as generic implementation
was taken ARM.
> 
> > +#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.)
It can. I'll update the macros.
> 
> > --- 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?
Agree. <xen/bug.h> should be removed fro the current patch.
Probably merge error during rebase.
> 
> > @@ -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()?
It's the goal but at the current stage we can't switch to
do_bug_frame() because it is based on function from xen/common which we
don't have now for RISC-V. So it will be switched when xen/common will
be available.
> 
> > +bool is_valid_bugaddr(vaddr_t insn)
> 
> Why is insn of type vaddr_t?
Initially address of insn was passed here. But now we should change
vaddr_t to 'uint32_t' as the instruction length can be either 16 or 32
and update instr variable inside do_trap():

  static uint32_t read_instr(unsigned long pc)
  {
      uint16_t instr16 = *(uint16_t *)pc;
 
      if ( GET_INSN_LENGTH(instr16) == 2 )
          return (uint32_t)instr16;
      else
          return *(uint32_t *)pc;
  }

  void do_trap(struct cpu_user_regs *cpu_regs)
  {
      register_t pc = cpu_regs->sepc;
      uint32_t instr = read_instr(pc);
  ...

> 
> > +{
> > +    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.)
To be 100% sure, should it be: 
  return ( ( insn & COMPRESSED_INSN_MASK ) == BUG_INSN_16 );
> > +}
> > +
> >  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).
Thanks. I'll update then.
> 
> Jan

Jan Beulich March 1, 2023, 12:42 p.m. UTC | #3

On 01.03.2023 12:51, Oleksii wrote:
> On Mon, 2023-02-27 at 13:59 +0100, Jan Beulich wrote:
>> On 24.02.2023 12:35, Oleksii Kurochko wrote:
>>> +{
>>> +    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.)
> To be 100% sure, should it be: 
>   return ( ( insn & COMPRESSED_INSN_MASK ) == BUG_INSN_16 );

No, that's yet more spaces instead of fewer ones.

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

or, if you really want the extra parentheses:

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

(Personally I'd also omit the "else":

    if ( (insn & INSN_LENGTH_MASK) == INSN_LENGTH_32 )
        return insn == BUG_INSN_32;

    return (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16;

. Plus I don't think you really need to mask as much, i.e.

    return insn == BUG_INSN_32 ||
           (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16;

would do as well afaict.)

Jan

Patch

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__
+
+#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;                                          \
+    len;                                                    \
+})
+
+/* These are defined by the architecture */
+bool is_valid_bugaddr(vaddr_t addr);
+
+#endif /* !__ASSEMBLY__ */
+
+#endif /* _ASM_RISCV_BUG_H */
diff --git a/xen/arch/riscv/include/asm/processor.h b/xen/arch/riscv/include/asm/processor.h
index a71448e02e..ef23d9589e 100644
--- a/xen/arch/riscv/include/asm/processor.h
+++ b/xen/arch/riscv/include/asm/processor.h
@@ -69,6 +69,8 @@  static inline void die(void)
         wfi();
 }
 
+void show_execution_state(const struct cpu_user_regs *regs);
+
 #endif /* __ASSEMBLY__ */
 
 #endif /* _ASM_RISCV_PROCESSOR_H */
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index c8513ca4f8..73b9a82883 100644
--- 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>
 
diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
index ad7311f269..d87a9cfd2c 100644
--- a/xen/arch/riscv/traps.c
+++ b/xen/arch/riscv/traps.c
@@ -5,6 +5,7 @@ 
  * RISC-V Trap handlers
  */
 
+#include <xen/bug.h>
 #include <xen/errno.h>
 #include <xen/lib.h>
 
@@ -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;
+}
+
+bool is_valid_bugaddr(vaddr_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 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) ) {
+            cpu_regs->sepc += GET_INSN_LENGTH(instr);
+            return;
+        }
+    }
+
     do_unexpected_trap(cpu_regs);
 }
diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S
index ca57cce75c..139e2d04cb 100644
--- a/xen/arch/riscv/xen.lds.S
+++ b/xen/arch/riscv/xen.lds.S
@@ -39,6 +39,16 @@  SECTIONS
     . = ALIGN(PAGE_SIZE);
     .rodata : {
         _srodata = .;          /* Read-only data */
+        /* Bug frames table */
+       __start_bug_frames = .;
+       *(.bug_frames.0)
+       __stop_bug_frames_0 = .;
+       *(.bug_frames.1)
+       __stop_bug_frames_1 = .;
+       *(.bug_frames.2)
+       __stop_bug_frames_2 = .;
+       *(.bug_frames.3)
+       __stop_bug_frames_3 = .;
         *(.rodata)
         *(.rodata.*)
         *(.data.rel.ro)