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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

>>> +#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.

>>> @@ -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).

>>> +    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.

>>> +        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?

Jan