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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Wed, 2023-02-08 at 15:01 +0100, Jan Beulich wrote:
> On 08.02.2023 13:00, Oleksii wrote:
> > On Tue, 2023-02-07 at 16:07 +0100, Jan Beulich wrote:
> > > 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)?
> > 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 ).
> 
> Can there then please be a comment here about this (current)
> assumption?
> 
Sure, I'll do it.

> > > > +#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?
> > > 
> > There is no specific reason (at least I don't see it now). It looks
> > like it can be used here even smaller type than 'unsigned int' as
> > len,
> > in current case, can be either 4 or 2.
> 
> Often working with more narrow types isn't as efficient, so using
> (signed/unsigned) int is generally best unless there are specific
> reasons to use a wider or more narrow type.
> 
Thanks for explaining.

> > > > @@ -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?
> > regs aren't changed in the function so I decided to put it as
> > const.
> 
> Hmm, a misunderstanding? I was asking whether there is a reason not
> to have the three local variables be "pointer to const". As a rule
> of thumb, const should be added to pointed-to types whenever
> possible.
> That way it's very obvious even when looking only in passing that the
> value/array pointed to isn't supposed to be modified through the
> variable (or function parameter).
Oh, got it. I though that you wrote about const in "const struct
cpu_user_regs *regs".
> 
> > > > +    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.
> > Do you mean that it is better to re-write bug_frame[] to:
> >     struct bug_frane bug_frames[] = {
> >         &__start_bug_frame[0],
> >         ...
> 
> Yes - the fewer casts the better. Also as per above, as much const as
> possible. And I expect the array can actually also be static rather
> than living on the stack.
Thanks. I'll update with the proposed changes.
> 
> > > > +        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.
> > It's an old code that should be removed.
> 
> Removed? I.e. the line number will never be logged?
It will, but:
1. I decided not to provide early_printk_hnum() and focus on getting
printk() working.
2. I introduced generic do_bug_frame() [1] (at least, for ARM and RISC-
V) so the current implementation will be switched to generic one when
panic, printk and find_text_region() (virtual memory) will be
ready/merged. It is what I am going to do next.

[2] - is a link to patch series with introduction of generic
implementation of macros from bug.h. Probably you can look at it too
when you will have free time. Thank you so much for your attention and
participation.

[1]
https://lore.kernel.org/xen-devel/8adf4aeff96750982e3d670cb3aed11553d546d5.1675441720.git.oleksii.kurochko@xxxxxxxxx/
[2]
https://lore.kernel.org/xen-devel/?q=%22introduce+generic+implementation+of+macros+from+bug.h%22
> 
> Jan

~ Oleksii