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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Mon, 2023-08-07 at 15:29 +0200, Jan Beulich wrote:
> On 03.08.2023 14:05, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/traps.c
> > +++ b/xen/arch/riscv/traps.c
> > @@ -5,6 +5,8 @@
> >   * RISC-V Trap handlers
> >   */
> >  
> > +#include <xen/bug.h>
> > +#include <xen/errno.h>
> >  #include <xen/lib.h>
> >  
> >  #include <asm/csr.h>
> > @@ -12,6 +14,8 @@
> >  #include <asm/processor.h>
> >  #include <asm/traps.h>
> >  
> > +#define cast_to_bug_frame(addr) ((const struct bug_frame *)(addr))
> > +
> >  /*
> >   * Initialize the trap handling.
> >   *
> > @@ -101,7 +105,131 @@ static void do_unexpected_trap(const struct
> > cpu_user_regs *regs)
> >      die();
> >  }
> >  
> > +void show_execution_state(const struct cpu_user_regs *regs)
> > +{
> > +    printk("implement show_execution_state(regs)\n");
> > +}
> > +
> > +/*
> > + * TODO: generic do_bug_frame() should be used instead of current
> > + * implementation 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;
> 
> Pointless initializer.
Agree. Thanks. I'll remove it.

> 
> > +    const char *filename, *predicate;
> > +    int lineno;
> > +
> > +    static const struct bug_frame *bug_frames[] = {
> 
> You likely want another const here.
Yes, I will add it.

> 
> > +        &__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 = cast_to_bug_frame(bug_frames[id]);
> > +        end   = cast_to_bug_frame(bug_frames[id + 1]);
> 
> Why these casts (and then even hidden in a macro)? The array elements
> look to already be of appropriate type.
There is no any sense for these casts. It looks like that before
bug_frames array has a different type.

> 
> > +        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:
> > +        printk("Xen WARN at %s:%d\n", filename, lineno);
> > +
> > +        show_execution_state(regs);
> > +
> > +        goto end;
> > +
> > +    case BUGFRAME_bug:
> > +        printk("Xen BUG at %s:%d\n", filename, lineno);
> > +
> > +        show_execution_state(regs);
> > +
> > +        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);
> > +
> > +        printk("Assertion %s failed at %s:%d\n", predicate,
> > filename, lineno);
> > +
> > +        show_execution_state(regs);
> > +
> > +        printk("change wait_for_interrupt to panic() when common
> > is available\n");
> > +        die();
> > +    }
> > +
> > +    return -EINVAL;
> > +
> > + end:
> > +    return 0;
> > +}
> > +
> > +static bool is_valid_bugaddr(uint32_t insn)
> > +{
> > +    return insn == BUG_INSN_32 ||
> > +           (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16;
> > +}
> 
> Why "addr" in the name when this takes an insn as argument?
In the earliest patch series it was an address. But now it should be
changed. Thanks.
> 
> > +/* Should be used only in Xen code ? */
> 
> What is this question about? With ...
I meant that it's not safe to use in guest code.

> 
> > +static uint32_t read_instr(unsigned long pc)
> > +{
> > +    uint16_t instr16 = *(uint16_t *)pc;
> > +
> > +    if ( GET_INSN_LENGTH(instr16) == 2 )
> > +        return (uint32_t)instr16;
> 
> (I don't think this cast is needed.)
> 
> > +    else
> > +        return *(uint32_t *)pc;
> > +}
> 
> ... there still being a double read here, do you perhaps mean to
> make a statement (that this code isn't safe to use on guest code)?
I wonder if it'll be safe to read 16 bytes at a time then we won't have
double read ( if you meant that first 16 bytes are read twice ):

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;

        uint16_t next_16 = *((uint16_t *)pc + 1);
        return ((uint32_t)instr16 << sizeof(instr16)) + next_16;
    }
}
> 
> >  void do_trap(struct cpu_user_regs *cpu_regs)
> >  {
> > +    register_t pc = cpu_regs->sepc;
> > +    uint32_t instr = read_instr(pc);
> > +
> > +    if ( is_valid_bugaddr(instr) )
> > +    {
> > +        if ( !do_bug_frame(cpu_regs, pc) )
> > +        {
> > +            cpu_regs->sepc += GET_INSN_LENGTH(instr);
> > +            return;
> > +        }
> > +    }
> > +
> >      do_unexpected_trap(cpu_regs);
> >  }
> 

~ Oleksii