| Message ID | b79bcfb9031564fc8d275af66fa94c1e82e54de2.1721731887.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RISCV basic exception handling implementation | expand |
On 24.07.2024 17:31, Oleksii Kurochko wrote: > To have working BUG(), WARN(), ASSERT, run_in_exception_handler() > it is needed to enable GENERIC_BUG_FRAME. > > ebreak is used as BUG_insn so when macros from <xen/bug.h> are > used, an exception with BREAKPOINT cause will be generated. > > ebreak triggers a debug trap, which starts in debug mode and is > then filtered by every mode. A debugger will first handle the > debug trap and check if ebreak was set by it or not. If not, > CAUSE_BREAKPOINT will be generated for the mode where the ebreak > instruction was executed. Here and in the similar code comment you talk about an external debugger, requiring debug mode, which is an optional feature. In my earlier comments what I was referring to was pure software debugging. I continue to fail to see how properly distinguishing ebreak uses for breakpoints from ebreak uses for e.g. BUG() and WARN() is going to be feasible. > @@ -103,7 +104,39 @@ static void do_unexpected_trap(const struct cpu_user_regs *regs) > > void do_trap(struct cpu_user_regs *cpu_regs) > { > - do_unexpected_trap(cpu_regs); > + register_t pc = cpu_regs->sepc; > + unsigned long cause = csr_read(CSR_SCAUSE); > + > + switch ( cause ) > + { > + /* > + * ebreak is used as BUG_insn so when macros from <xen/bug.h> are > + * used, an exception with BREAKPOINT cause will be generated. > + * > + * ebreak triggers a debug trap, which starts in debug mode and is > + * then filtered by every mode. A debugger will first handle the > + * debug trap and check if ebreak was set by it or not. If not, > + * CAUSE_BREAKPOINT will be generated for the mode where the ebreak > + * instruction was executed. > + */ > + case CAUSE_BREAKPOINT: > + if ( do_bug_frame(cpu_regs, pc) >= 0 ) > + { > + if ( !(is_kernel_text(pc) || is_kernel_inittext(pc)) ) > + { > + printk("Something wrong with PC: %#lx\n", pc); > + die(); > + } > + > + cpu_regs->sepc += GET_INSN_LENGTH(*(uint16_t *)pc); > + } > + > + break; Wouldn't you better fall through here, with the "break" moved into the if()'s body? Jan > + default: > + do_unexpected_trap(cpu_regs); > + break; > + } > }
On Mon, 2024-07-29 at 15:00 +0200, Jan Beulich wrote: > On 24.07.2024 17:31, Oleksii Kurochko wrote: > > To have working BUG(), WARN(), ASSERT, run_in_exception_handler() > > it is needed to enable GENERIC_BUG_FRAME. > > > > ebreak is used as BUG_insn so when macros from <xen/bug.h> are > > used, an exception with BREAKPOINT cause will be generated. > > > > ebreak triggers a debug trap, which starts in debug mode and is > > then filtered by every mode. A debugger will first handle the > > debug trap and check if ebreak was set by it or not. If not, > > CAUSE_BREAKPOINT will be generated for the mode where the ebreak > > instruction was executed. > > Here and in the similar code comment you talk about an external > debugger, requiring debug mode, which is an optional feature. In > my earlier comments what I was referring to was pure software > debugging. I continue to fail to see how properly distinguishing > ebreak uses for breakpoints from ebreak uses for e.g. BUG() and > WARN() is going to be feasible. I am using GDB now and everything working well. > > > @@ -103,7 +104,39 @@ static void do_unexpected_trap(const struct > > cpu_user_regs *regs) > > > > void do_trap(struct cpu_user_regs *cpu_regs) > > { > > - do_unexpected_trap(cpu_regs); > > + register_t pc = cpu_regs->sepc; > > + unsigned long cause = csr_read(CSR_SCAUSE); > > + > > + switch ( cause ) > > + { > > + /* > > + * ebreak is used as BUG_insn so when macros from <xen/bug.h> > > are > > + * used, an exception with BREAKPOINT cause will be generated. > > + * > > + * ebreak triggers a debug trap, which starts in debug mode > > and is > > + * then filtered by every mode. A debugger will first handle > > the > > + * debug trap and check if ebreak was set by it or not. If > > not, > > + * CAUSE_BREAKPOINT will be generated for the mode where the > > ebreak > > + * instruction was executed. > > + */ > > + case CAUSE_BREAKPOINT: > > + if ( do_bug_frame(cpu_regs, pc) >= 0 ) > > + { > > + if ( !(is_kernel_text(pc) || is_kernel_inittext(pc)) ) > > + { > > + printk("Something wrong with PC: %#lx\n", pc); > > + die(); > > + } > > + > > + cpu_regs->sepc += GET_INSN_LENGTH(*(uint16_t *)pc); > > + } > > + > > + break; > > Wouldn't you better fall through here, with the "break" moved into > the if()'s body? It makes sense to me. Thanks. ~ Oleksii
On Mon, 2024-07-29 at 15:00 +0200, Jan Beulich wrote: > > To have working BUG(), WARN(), ASSERT, run_in_exception_handler() > > it is needed to enable GENERIC_BUG_FRAME. > > > > ebreak is used as BUG_insn so when macros from <xen/bug.h> are > > used, an exception with BREAKPOINT cause will be generated. > > > > ebreak triggers a debug trap, which starts in debug mode and is > > then filtered by every mode. A debugger will first handle the > > debug trap and check if ebreak was set by it or not. If not, > > CAUSE_BREAKPOINT will be generated for the mode where the ebreak > > instruction was executed. > > Here and in the similar code comment you talk about an external > debugger, requiring debug mode, which is an optional feature. In > my earlier comments what I was referring to was pure software > debugging. I continue to fail to see how properly distinguishing > ebreak uses for breakpoints from ebreak uses for e.g. BUG() and > WARN() is going to be feasible. GDB keeps track of what addresses it has breakpoints at. And if QEMU is being used, GDB isn't actually inserting ebreak instructions because QEMU has an infinite amount of "hardware" breakpoints. If it answers your original question I could add this to commit message/code in the next patch version. ~ Oleksii
On 02.08.2024 11:14, oleksii.kurochko@gmail.com wrote: > On Mon, 2024-07-29 at 15:00 +0200, Jan Beulich wrote: >>> To have working BUG(), WARN(), ASSERT, run_in_exception_handler() >>> it is needed to enable GENERIC_BUG_FRAME. >>> >>> ebreak is used as BUG_insn so when macros from <xen/bug.h> are >>> used, an exception with BREAKPOINT cause will be generated. >>> >>> ebreak triggers a debug trap, which starts in debug mode and is >>> then filtered by every mode. A debugger will first handle the >>> debug trap and check if ebreak was set by it or not. If not, >>> CAUSE_BREAKPOINT will be generated for the mode where the ebreak >>> instruction was executed. >> >> Here and in the similar code comment you talk about an external >> debugger, requiring debug mode, which is an optional feature. In >> my earlier comments what I was referring to was pure software >> debugging. I continue to fail to see how properly distinguishing >> ebreak uses for breakpoints from ebreak uses for e.g. BUG() and >> WARN() is going to be feasible. > GDB keeps track of what addresses it has breakpoints at. Of course it does. But in Xen how do you decide whether to trigger the debugger when you've hit an ebreak? (Just to repeat, my question is about the purely software debugging case; no hardware debugging extensions. In such a case, aiui, Xen gains control first and then decides whether to trigger the debugger, or whether to handle the exception internally. Sure, none of this infrastructure is in place right now, but imo it wants taking into consideration.) > And if QEMU is being used, GDB isn't actually inserting ebreak > instructions because QEMU has an infinite amount of "hardware" > breakpoints. > > If it answers your original question I could add this to commit > message/code in the next patch version. I'm afraid it still doesn't. Jan
On Fri, 2024-08-02 at 11:21 +0200, Jan Beulich wrote: > On 02.08.2024 11:14, oleksii.kurochko@gmail.com wrote: > > On Mon, 2024-07-29 at 15:00 +0200, Jan Beulich wrote: > > > > To have working BUG(), WARN(), ASSERT, > > > > run_in_exception_handler() > > > > it is needed to enable GENERIC_BUG_FRAME. > > > > > > > > ebreak is used as BUG_insn so when macros from <xen/bug.h> are > > > > used, an exception with BREAKPOINT cause will be generated. > > > > > > > > ebreak triggers a debug trap, which starts in debug mode and is > > > > then filtered by every mode. A debugger will first handle the > > > > debug trap and check if ebreak was set by it or not. If not, > > > > CAUSE_BREAKPOINT will be generated for the mode where the > > > > ebreak > > > > instruction was executed. > > > > > > Here and in the similar code comment you talk about an external > > > debugger, requiring debug mode, which is an optional feature. In > > > my earlier comments what I was referring to was pure software > > > debugging. I continue to fail to see how properly distinguishing > > > ebreak uses for breakpoints from ebreak uses for e.g. BUG() and > > > WARN() is going to be feasible. > > GDB keeps track of what addresses it has breakpoints at. > > Of course it does. But in Xen how do you decide whether to trigger > the debugger when you've hit an ebreak? (Just to repeat, my question > is about the purely software debugging case; no hardware debugging > extensions. In such a case, aiui, Xen gains control first and then > decides whether to trigger the debugger, or whether to handle the > exception internally. Sure, none of this infrastructure is in place > right now, but imo it wants taking into consideration.) Well, then something like KGDB is needed for Xen and mechanism to notify guests to something similar to: Right now Xen detects that 'ebreak' is inserted by using the function do_bug_frame(): ``` case CAUSE_BREAKPOINT: if ( do_bug_frame(cpu_regs, pc) >= 0 ) { if ( !(is_kernel_text(pc) || is_kernel_inittext(pc)) ) { printk("Something wrong with PC: %#lx\n", pc); die(); } cpu_regs->sepc += GET_INSN_LENGTH(*(uint16_t *)pc); } ``` So if do_bug_frame() return < 0 then it should be ebreak inserted by the debugger so need to notify GDB that he should handle that. At the moment I think we can add(): ``` if ( do_bug_frame(cpu_regs, pc) >= 0 ) { ... cpu_regs->sepc += GET_INSN_LENGTH(*(uint16_t *)pc); } else { printk("this is not Xen's ebreak\n"); die(); } ``` Probably, to have the message is enough ( "this is not Xen's ebreak\n" ): ``` if ( do_bug_frame(cpu_regs, pc) >= 0 ) { ... } else printk("this is not Xen's ebreak\n"); cpu_regs->sepc += GET_INSN_LENGTH(*(uint16_t *)pc); ``` > > > And if QEMU is being used, GDB isn't actually inserting ebreak > > instructions because QEMU has an infinite amount of "hardware" > > breakpoints. > > > > If it answers your original question I could add this to commit > > message/code in the next patch version. > > I'm afraid it still doesn't. So if we are speaking about the case QEMU + GDB then no ebreak instruction will be inserted. So what happens is: 1. QEMU notices PC hits breakpoint ( "hardware" breakpoints are used + GDB keeps track of what addresses it has breakpoints at ) 2. QEMU tells GDB that the program received SIGTRAP 3. GDB does debugger tasks. In case of real hardware it will be JTAG and OpenOCD to interface with it and GDB will use OpenOCD for debugging purpose which follows RISC-V debug spec and everything will work as I described in the commit message/code. Or it could be still without external h/w debugger ( using ebreak instruction ) but then hypervisor should have an infrastrucutre to notify GDB that a breakpoint is hited ( something like KGDB in Linux kernel ) and for this case we will print the message mentioned above ( + stop Xen if it is necessary ) ~ Oleksii
On 02.08.2024 12:22, oleksii.kurochko@gmail.com wrote: > On Fri, 2024-08-02 at 11:21 +0200, Jan Beulich wrote: >> On 02.08.2024 11:14, oleksii.kurochko@gmail.com wrote: >>> On Mon, 2024-07-29 at 15:00 +0200, Jan Beulich wrote: >>>>> To have working BUG(), WARN(), ASSERT, >>>>> run_in_exception_handler() >>>>> it is needed to enable GENERIC_BUG_FRAME. >>>>> >>>>> ebreak is used as BUG_insn so when macros from <xen/bug.h> are >>>>> used, an exception with BREAKPOINT cause will be generated. >>>>> >>>>> ebreak triggers a debug trap, which starts in debug mode and is >>>>> then filtered by every mode. A debugger will first handle the >>>>> debug trap and check if ebreak was set by it or not. If not, >>>>> CAUSE_BREAKPOINT will be generated for the mode where the >>>>> ebreak >>>>> instruction was executed. >>>> >>>> Here and in the similar code comment you talk about an external >>>> debugger, requiring debug mode, which is an optional feature. In >>>> my earlier comments what I was referring to was pure software >>>> debugging. I continue to fail to see how properly distinguishing >>>> ebreak uses for breakpoints from ebreak uses for e.g. BUG() and >>>> WARN() is going to be feasible. >>> GDB keeps track of what addresses it has breakpoints at. >> >> Of course it does. But in Xen how do you decide whether to trigger >> the debugger when you've hit an ebreak? (Just to repeat, my question >> is about the purely software debugging case; no hardware debugging >> extensions. In such a case, aiui, Xen gains control first and then >> decides whether to trigger the debugger, or whether to handle the >> exception internally. Sure, none of this infrastructure is in place >> right now, but imo it wants taking into consideration.) > Well, then something like KGDB is needed for Xen and mechanism to > notify guests to something similar to: > > Right now Xen detects that 'ebreak' is inserted by using the function > do_bug_frame(): > ``` > case CAUSE_BREAKPOINT: > if ( do_bug_frame(cpu_regs, pc) >= 0 ) > { > if ( !(is_kernel_text(pc) || is_kernel_inittext(pc)) ) > { > printk("Something wrong with PC: %#lx\n", pc); > die(); > } > > cpu_regs->sepc += GET_INSN_LENGTH(*(uint16_t *)pc); > } > ``` > > So if do_bug_frame() return < 0 then it should be ebreak inserted by > the debugger so need to notify GDB that he should handle that. > At the moment I think we can add(): > ``` > if ( do_bug_frame(cpu_regs, pc) >= 0 ) > { > ... > > cpu_regs->sepc += GET_INSN_LENGTH(*(uint16_t *)pc); > } > else > { > printk("this is not Xen's ebreak\n"); > die(); > } > ``` Except that, as previously said, this will break if the debugger inserted a breakpoint where Xen already has an ebreak. That's where we started from in this discussion. Recall that my question was how the use of ebreak can be correct / reliable here. It is for a reason that on x86 we do not use the shorter INT3 instruction, but the longer UD2 one for BUG() etc. Jan
On Fri, 2024-08-02 at 12:30 +0200, Jan Beulich wrote: > On 02.08.2024 12:22, oleksii.kurochko@gmail.com wrote: > > On Fri, 2024-08-02 at 11:21 +0200, Jan Beulich wrote: > > > On 02.08.2024 11:14, oleksii.kurochko@gmail.com wrote: > > > > On Mon, 2024-07-29 at 15:00 +0200, Jan Beulich wrote: > > > > > > To have working BUG(), WARN(), ASSERT, > > > > > > run_in_exception_handler() > > > > > > it is needed to enable GENERIC_BUG_FRAME. > > > > > > > > > > > > ebreak is used as BUG_insn so when macros from <xen/bug.h> > > > > > > are > > > > > > used, an exception with BREAKPOINT cause will be generated. > > > > > > > > > > > > ebreak triggers a debug trap, which starts in debug mode > > > > > > and is > > > > > > then filtered by every mode. A debugger will first handle > > > > > > the > > > > > > debug trap and check if ebreak was set by it or not. If > > > > > > not, > > > > > > CAUSE_BREAKPOINT will be generated for the mode where the > > > > > > ebreak > > > > > > instruction was executed. > > > > > > > > > > Here and in the similar code comment you talk about an > > > > > external > > > > > debugger, requiring debug mode, which is an optional feature. > > > > > In > > > > > my earlier comments what I was referring to was pure software > > > > > debugging. I continue to fail to see how properly > > > > > distinguishing > > > > > ebreak uses for breakpoints from ebreak uses for e.g. BUG() > > > > > and > > > > > WARN() is going to be feasible. > > > > GDB keeps track of what addresses it has breakpoints at. > > > > > > Of course it does. But in Xen how do you decide whether to > > > trigger > > > the debugger when you've hit an ebreak? (Just to repeat, my > > > question > > > is about the purely software debugging case; no hardware > > > debugging > > > extensions. In such a case, aiui, Xen gains control first and > > > then > > > decides whether to trigger the debugger, or whether to handle the > > > exception internally. Sure, none of this infrastructure is in > > > place > > > right now, but imo it wants taking into consideration.) > > Well, then something like KGDB is needed for Xen and mechanism to > > notify guests to something similar to: > > > > Right now Xen detects that 'ebreak' is inserted by using the > > function > > do_bug_frame(): > > ``` > > case CAUSE_BREAKPOINT: > > if ( do_bug_frame(cpu_regs, pc) >= 0 ) > > { > > if ( !(is_kernel_text(pc) || is_kernel_inittext(pc)) ) > > { > > printk("Something wrong with PC: %#lx\n", pc); > > die(); > > } > > > > cpu_regs->sepc += GET_INSN_LENGTH(*(uint16_t *)pc); > > } > > ``` > > > > So if do_bug_frame() return < 0 then it should be ebreak inserted > > by > > the debugger so need to notify GDB that he should handle that. > > At the moment I think we can add(): > > ``` > > if ( do_bug_frame(cpu_regs, pc) >= 0 ) > > { > > ... > > > > cpu_regs->sepc += GET_INSN_LENGTH(*(uint16_t *)pc); > > } > > else > > { > > printk("this is not Xen's ebreak\n"); > > die(); > > } > > ``` > > Except that, as previously said, this will break if the debugger > inserted > a breakpoint where Xen already has an ebreak. That's where we started > from > in this discussion. Recall that my question was how the use of ebreak > can > be correct / reliable here. It is for a reason that on x86 we do not > use > the shorter INT3 instruction, but the longer UD2 one for BUG() etc. Okay, in this case it will be an issue but only in case if the debugger really will insert ebreak instruction but not something else. I wasn't able to find a case when it is really inserted ebreak in case of RISC- V. However I agree that it would be safer to use something like UD2 but RISC-V doesn't have such instruction ( at least I don't see such in a spec). There is something what we need is mentioned here: https://github.com/riscv-non-isa/riscv-asm-manual/blob/main/riscv-asm.md#instruction-aliases But the problem is that is "de-facto" standard, but not really standard instruction. Anyway, I will update the patch to use (C0001073) instead of ebreak. ~ Oleksii
On 02.08.2024 13:02, oleksii.kurochko@gmail.com wrote: > There is something what we need is mentioned here: > https://github.com/riscv-non-isa/riscv-asm-manual/blob/main/riscv-asm.md#instruction-aliases > But the problem is that is "de-facto" standard, but not really standard > instruction. Anyway, I will update the patch to use (C0001073) instead > of ebreak. Well, the official spec, in the C extension, mentions 0x0000 as "defined illegal instruction". Wouldn't using that work? It's going to be undefined no matter whether the C extension is actually implemented. Jan
On Fri, 2024-08-02 at 13:18 +0200, Jan Beulich wrote: > On 02.08.2024 13:02, oleksii.kurochko@gmail.com wrote: > > There is something what we need is mentioned here: > > https://github.com/riscv-non-isa/riscv-asm-manual/blob/main/riscv-asm.md#instruction-aliases > > But the problem is that is "de-facto" standard, but not really > > standard > > instruction. Anyway, I will update the patch to use (C0001073) > > instead > > of ebreak. > > Well, the official spec, in the C extension, mentions 0x0000 as > "defined > illegal instruction". Wouldn't using that work? It's going to be > undefined > no matter whether the C extension is actually implemented. It should work. I will use 0x0000 then. Thanks. ~ Oleksii
diff --git a/xen/arch/riscv/Kconfig b/xen/arch/riscv/Kconfig index b4b354a778..f531e96657 100644 --- a/xen/arch/riscv/Kconfig +++ b/xen/arch/riscv/Kconfig @@ -1,6 +1,7 @@ config RISCV def_bool y select FUNCTION_ALIGNMENT_16B + select GENERIC_BUG_FRAME config RISCV_64 def_bool y diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c index cb18b30ff2..6802827eb5 100644 --- a/xen/arch/riscv/traps.c +++ b/xen/arch/riscv/traps.c @@ -5,6 +5,7 @@ * RISC-V Trap handlers */ +#include <xen/bug.h> #include <xen/lib.h> #include <xen/sched.h> @@ -103,7 +104,39 @@ static void do_unexpected_trap(const struct cpu_user_regs *regs) void do_trap(struct cpu_user_regs *cpu_regs) { - do_unexpected_trap(cpu_regs); + register_t pc = cpu_regs->sepc; + unsigned long cause = csr_read(CSR_SCAUSE); + + switch ( cause ) + { + /* + * ebreak is used as BUG_insn so when macros from <xen/bug.h> are + * used, an exception with BREAKPOINT cause will be generated. + * + * ebreak triggers a debug trap, which starts in debug mode and is + * then filtered by every mode. A debugger will first handle the + * debug trap and check if ebreak was set by it or not. If not, + * CAUSE_BREAKPOINT will be generated for the mode where the ebreak + * instruction was executed. + */ + case CAUSE_BREAKPOINT: + if ( do_bug_frame(cpu_regs, pc) >= 0 ) + { + if ( !(is_kernel_text(pc) || is_kernel_inittext(pc)) ) + { + printk("Something wrong with PC: %#lx\n", pc); + die(); + } + + cpu_regs->sepc += GET_INSN_LENGTH(*(uint16_t *)pc); + } + + break; + + default: + do_unexpected_trap(cpu_regs); + break; + } } void vcpu_show_execution_state(struct vcpu *v) diff --git a/xen/common/bug.c b/xen/common/bug.c index b7c5d8fd4d..75cb35fcfa 100644 --- a/xen/common/bug.c +++ b/xen/common/bug.c @@ -1,6 +1,7 @@ #include <xen/bug.h> #include <xen/errno.h> #include <xen/kernel.h> +#include <xen/lib.h> #include <xen/livepatch.h> #include <xen/string.h> #include <xen/types.h>
To have working BUG(), WARN(), ASSERT, run_in_exception_handler() it is needed to enable GENERIC_BUG_FRAME. ebreak is used as BUG_insn so when macros from <xen/bug.h> are used, an exception with BREAKPOINT cause will be generated. ebreak triggers a debug trap, which starts in debug mode and is then filtered by every mode. A debugger will first handle the debug trap and check if ebreak was set by it or not. If not, CAUSE_BREAKPOINT will be generated for the mode where the ebreak instruction was executed. Also, <xen/lib.h> is needed to be included for the reason that panic() and printk() are used in common/bug.c and RISC-V fails if it is not included. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in V11: - update the commit message - change "%lx" to "%#x" for PC register printing. - drop +1 in argument of is_kernel_text(pc) and is_kernel_inittext(pc). - drop return for case CAUSE_BREAKPOINT. - add break to default and add a blank like above it. - add a comment CAUSE_BREAKPOINT is handled instead of illegal instruction. --- Changes in V10: - put 'select GENERIC_BUG_FRAME' in "Config RISCV". - rework do_trap() to not fetch an instruction in case when the cause of trap is BUG_insn. - drop read_instr() and is_valid_bug_insn(). - update the commit message. --- Changes in V9: - Rebase on the top of current staging. - use GENERIC_BUG_FRAME as now we have common code available. - add xen/lib.h to bug.c to fix a compilation error around printk. - update the commit message. - update the code of read_instr() in traps.c - fold two-s if into 1 in do_trap. --- Changes in V8: - remove Pointless initializer of id. - make bug_frames[] array constant. - remove cast_to_bug_frame(addr). - rename is_valig_bugaddr to is_valid_bug_insn(). - add check that read_instr is used only on xen code - update the commit message. --- Changes in V7: - move to this patch the definition of cast_to_bug_frame() from the previous patch. - update the comment in bug.h. - update the comment above do_bug_frame(). - fix code style. - add comment to read_instr func. - add space for bug_frames in lds.S. --- Changes in V6: - Avoid LINK_TO_LOAD() as bug.h functionality expected to be used after MMU is enabled. - Change early_printk() to printk() --- Changes in V5: - Remove "#include <xen/types.h>" from <asm/bug.h> as there is no any need in it anymore - Update macros GET_INSN_LENGTH: remove UL and 'unsigned int len;' from it - Remove " include <xen/bug.h>" from risc/setup.c. it is not needed in the current version of the patch - change an argument type from vaddr_t to uint32_t for is_valid_bugaddr and introduce read_instr() to read instruction properly as the length of qinstruction can be either 32 or 16 bits. - Code style fixes - update the comments before do_bug_frame() in riscv/trap.c - Refactor is_valid_bugaddr() function. - introduce macros cast_to_bug_frame(addr) to hide casts. - use LINK_TO_LOAD() for addresses which are linker time relative. --- Changes in V4: - Updates in RISC-V's <asm/bug.h>: * Add explanatory comment about why there is only defined for 32-bits length instructions and 16/32-bits BUG_INSN_{16,32}. * Change 'unsigned long' to 'unsigned int' inside GET_INSN_LENGTH(). * Update declaration of is_valid_bugaddr(): switch return type from int to bool and the argument from 'unsigned int' to 'vaddr'. - Updates in RISC-V's traps.c: * replace /xen and /asm includes * update definition of is_valid_bugaddr():switch return type from int to bool and the argument from 'unsigned int' to 'vaddr'. Code style inside function was updated too. * do_bug_frame() refactoring: * local variables start and bug became 'const struct bug_frame' * bug_frames[] array became 'static const struct bug_frame[] = ...' * remove all casts * remove unneeded comments and add an explanatory comment that the do_bug_frame() will be switched to a generic one. * do_trap() refactoring: * read 16-bits value instead of 32-bits as compressed instruction can be used and it might happen than only 16-bits may be accessible. * code style updates * re-use instr variable instead of re-reading instruction. - Updates in setup.c: * add blank line between xen/ and asm/ includes. --- 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/Kconfig | 1 + xen/arch/riscv/traps.c | 35 ++++++++++++++++++++++++++++++++++- xen/common/bug.c | 1 + 3 files changed, 36 insertions(+), 1 deletion(-)