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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.02.2023 15:46, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/bug.h
> @@ -0,0 +1,38 @@
> +/* 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_FN_REG t0
> +
> +#define BUG_INSTR "ebreak"
> +
> +#define INSN_LENGTH_MASK        _UL(0x3)
> +#define INSN_LENGTH_32          _UL(0x3)

I assume these are deliberately over-simplified (not accounting for
wider than 32-bit insns in any way)?

> +#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 long len;                                      \
> +    len = ((insn & INSN_LENGTH_MASK) == INSN_LENGTH_32) ?   \
> +        4UL : 2UL;                                          \
> +    len;                                                    \

Any reason for the use of "unsigned long" (not "unsigned int") here?

> +})
> +
> +/* These are defined by the architecture */
> +int is_valid_bugaddr(uint32_t addr);

A function by this name very likely wants to return bool. Also -
uint32_t (not "unsigned long") for an address?

> --- a/xen/arch/riscv/setup.c
> +++ b/xen/arch/riscv/setup.c
> @@ -1,6 +1,6 @@
> +#include <xen/bug.h>
>  #include <xen/compile.h>
>  #include <xen/init.h>
> -
>  #include <asm/csr.h>
>  #include <asm/early_printk.h>
>  #include <asm/traps.h>

I think it is good practice to have a blank line between xen/ and asm/
includes.

> --- a/xen/arch/riscv/traps.c
> +++ b/xen/arch/riscv/traps.c
> @@ -8,6 +8,7 @@
>  #include <asm/early_printk.h>
>  #include <asm/processor.h>
>  #include <asm/traps.h>
> +#include <xen/bug.h>
>  #include <xen/errno.h>
>  #include <xen/lib.h>

Perhaps wants adjusting earlier in the series: Preferably all xen/
ahead of all asm/.

> @@ -97,7 +98,136 @@ 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");
> +}
> +
> +int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
> +{
> +    struct bug_frame *start, *end;
> +    struct bug_frame *bug = NULL;

const?

> +    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];

Nit: Stray double blanks. But I'd like to suggest that you get away
without casts here in the first place. Such casts are always a certain
risk going forward.

> +        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 *) = (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:
> +        /*
> +         * TODO: change early_printk's function to early_printk with format
> +         *       when s(n)printf() will be added.
> +         */
> +        early_printk("Xen WARN at ");
> +        early_printk(filename);
> +        early_printk(":");
> +        // early_printk_hnum(lineno);

What's this? At the very least the comment is malformed.

> +        show_execution_state(regs);
> +
> +        goto end;
> +
> +    case BUGFRAME_bug:
> +         /*
> +          * TODO: change early_printk's function to early_printk with format
> +          *       when s(n)printf() will be added.
> +          */
> +        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");
> +        die();
> +
> +    case BUGFRAME_assert:
> +        /* ASSERT: decode the predicate string pointer. */
> +        predicate = bug_msg(bug);
> +
> +        /*
> +         * TODO: change early_printk's function to early_printk with format
> +         *       when s(n)printf() will be added.
> +         */
> +        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");
> +        die();
> +    }
> +
> +    return -EINVAL;
> +
> + end:
> +    return 0;
> +}
> +
> +int is_valid_bugaddr(uint32_t insn)
> +{
> +    if ((insn & INSN_LENGTH_MASK) == INSN_LENGTH_32)

Nit: Style.

> +        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;
> +    uint32_t instr = *(uint32_t *)pc;

You still read a 32-bit value when only 16 bits may be accessible.

> +    if (is_valid_bugaddr(instr)) {
> +        if (!do_bug_frame(cpu_regs, cpu_regs->sepc)) {

I'm pretty sure I did point out the style issues here.

> +            cpu_regs->sepc += GET_INSN_LENGTH(*(uint32_t *)pc);

Why would you re-read the insn here, when you have it in a local variable
already?

Jan