| Message ID | 404a4d6621a2e5eb276d2fa61188429294d682ba.1675779308.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RISCV basic exception handling implementation | expand |
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
Hello Jan, Thanks for the comments! 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 ). Compressed ISA is used now where the instruction length is 16 bit and 'ebreak' instruction, in this case, can be either 16 or 32 bit ( depending on if compressed ISA is used or not ) > > > +#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. > > +}) > > + > > +/* 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? > It should be unsigned long. > > --- 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. > Thanks. I will bring a blank line back. > > --- 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/. > I'll fix it in the next version of the patch series. > > @@ -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. > > > + 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], ... > > > + 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. > > > + 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. Thanks. I'll fix it. > > > + 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. Since there were a lot of comments on the previous version of patch series, this comment slipped out of my head. I'll fix in new version of the patch series. > > > + 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? Inattention. I'll re-use local variable. > > Jan
Hi, On 08/02/2023 12: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 ). > > Compressed ISA is used now where the instruction length is 16 bit and > 'ebreak' instruction, in this case, can be either 16 or 32 bit ( > depending on if compressed ISA is used or not ) >> >>> +#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. > >>> +}) >>> + >>> +/* 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? >> > It should be unsigned long. In other part of the code, you are using vaddr_t/paddr_t to describe an address. It would be best to use one of those (I think vaddr_t) so it is clear what kind of address it is supposed to take. Cheers,
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
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@gmail.com/ [2] https://lore.kernel.org/xen-devel/?q=%22introduce+generic+implementation+of+macros+from+bug.h%22 > > Jan ~ Oleksii
On 09.02.2023 13:35, Oleksii wrote: > 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: >>>>> + 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. If that's to tell me that the code above to going to be replaced soon, then a well-formed commented-out piece of code is probably best for now. Jan > [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@gmail.com/ > [2] > https://lore.kernel.org/xen-devel/?q=%22introduce+generic+implementation+of+macros+from+bug.h%22 >> >> Jan > > ~ Oleksii >
diff --git a/xen/arch/riscv/include/asm/bug.h b/xen/arch/riscv/include/asm/bug.h new file mode 100644 index 0000000000..07190e9cfa --- /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) + +#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; \ +}) + +/* These are defined by the architecture */ +int is_valid_bugaddr(uint32_t addr); + +#endif /* !__ASSEMBLY__ */ + +#endif /* _ASM_RISCV_BUG_H */ diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c index c8513ca4f8..d502cf06b0 100644 --- 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> diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c index 31ed63e3c1..624847f840 100644 --- 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> @@ -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; + 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]; + + 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); + + 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) + 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; + + if (is_valid_bugaddr(instr)) { + if (!do_bug_frame(cpu_regs, cpu_regs->sepc)) { + cpu_regs->sepc += GET_INSN_LENGTH(*(uint32_t *)pc); + return; + } + } + do_unexpected_trap(cpu_regs); } diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S index ca57cce75c..139e2d04cb 100644 --- a/xen/arch/riscv/xen.lds.S +++ b/xen/arch/riscv/xen.lds.S @@ -39,6 +39,16 @@ SECTIONS . = ALIGN(PAGE_SIZE); .rodata : { _srodata = .; /* Read-only data */ + /* Bug frames table */ + __start_bug_frames = .; + *(.bug_frames.0) + __stop_bug_frames_0 = .; + *(.bug_frames.1) + __stop_bug_frames_1 = .; + *(.bug_frames.2) + __stop_bug_frames_2 = .; + *(.bug_frames.3) + __stop_bug_frames_3 = .; *(.rodata) *(.rodata.*) *(.data.rel.ro)
The patch introduces macros: BUG(), WARN(), run_in_exception(), assert_failed. The implementation uses "ebreak" instruction in combination with diffrent bug frame tables (for each type) which contains useful information. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in V3: - Rebase the patch "xen/riscv: introduce an implementation of macros from <asm/bug.h>" on top of patch series [introduce generic implementation of macros from bug.h] --- Changes in V2: - Remove __ in define namings - Update run_in_exception_handler() with register void *fn_ asm(__stringify(BUG_FN_REG)) = (fn); - Remove bug_instr_t type and change it's usage to uint32_t --- xen/arch/riscv/include/asm/bug.h | 38 +++++++++ xen/arch/riscv/setup.c | 2 +- xen/arch/riscv/traps.c | 130 +++++++++++++++++++++++++++++++ xen/arch/riscv/xen.lds.S | 10 +++ 4 files changed, 179 insertions(+), 1 deletion(-) create mode 100644 xen/arch/riscv/include/asm/bug.h