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
