| Message ID | c51c485bac43b7589961aabec8af1b82d4673b94.1719918148.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RISCV basic exception handling implementation | expand |
On 02.07.2024 13:23, Oleksii Kurochko wrote: > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > --- > xen/arch/riscv/Kconfig | 1 + > xen/arch/riscv/traps.c | 31 +++++++++++++++++++++++++++++++ > xen/common/bug.c | 1 + > 3 files changed, 33 insertions(+) > > diff --git a/xen/arch/riscv/Kconfig b/xen/arch/riscv/Kconfig > index b4b354a778..74ad019fe7 100644 > --- a/xen/arch/riscv/Kconfig > +++ b/xen/arch/riscv/Kconfig > @@ -5,6 +5,7 @@ config RISCV > config RISCV_64 > def_bool y > select 64BIT > + select GENERIC_BUG_FRAME Any particular reason to put this here, and not higher up with RISCV? > @@ -101,8 +102,38 @@ static void do_unexpected_trap(const struct cpu_user_regs *regs) > die(); > } > > +static bool is_valid_bug_insn(uint32_t insn) > +{ > + return insn == BUG_INSN_32 || > + (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16; > +} > + > +/* Should be used only on Xen code */ > +static uint32_t read_instr(unsigned long pc) > +{ > + uint16_t instr16 = *(uint16_t *)pc; > + > + ASSERT(is_kernel_text(pc + 1) || is_kernel_inittext(pc + 1)); > + > + if ( GET_INSN_LENGTH(instr16) == 2 ) > + return instr16; > + > + ASSERT(is_kernel_text(pc + 3) || is_kernel_inittext(pc + 3)); > + > + return *(uint32_t *)pc; > +} Related to the point made further down: If either of these assertions fails, won't we come back again right here? If either of the is_kernel_*text() wasn't working quite right, wouldn't we be at risk of entering an infinite loop (presumably not quite infinite because of the stack overflowing at some point)? > void do_trap(struct cpu_user_regs *cpu_regs) > { > + register_t pc = cpu_regs->sepc; > + uint32_t instr = read_instr(pc); > + > + if ( ( is_valid_bug_insn(instr) ) && ( do_bug_frame(cpu_regs, pc) >= 0 ) ) No consideration of the kind of exception? I'd expect it is one very specific one which the BUG insn would raise, and then there's no point fetching the insn when it's a different kind of exception. Further, nit: Certainly no need for the parentheses on the lhs of the &&. Having them on the rhs is a matter of taste, so okay, but then the blanks immediately inside will want dropping. > --- 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> Unrelated change? Or did you simply forget to mention in the description why it's needed? Jan
On Wed, 2024-07-10 at 12:01 +0200, Jan Beulich wrote: > On 02.07.2024 13:23, Oleksii Kurochko wrote: > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > > --- > > xen/arch/riscv/Kconfig | 1 + > > xen/arch/riscv/traps.c | 31 +++++++++++++++++++++++++++++++ > > xen/common/bug.c | 1 + > > 3 files changed, 33 insertions(+) > > > > diff --git a/xen/arch/riscv/Kconfig b/xen/arch/riscv/Kconfig > > index b4b354a778..74ad019fe7 100644 > > --- a/xen/arch/riscv/Kconfig > > +++ b/xen/arch/riscv/Kconfig > > @@ -5,6 +5,7 @@ config RISCV > > config RISCV_64 > > def_bool y > > select 64BIT > > + select GENERIC_BUG_FRAME > > Any particular reason to put this here, and not higher up with RISCV? Yes, you are right it would be better to put it inside "config RISCV". > > > @@ -101,8 +102,38 @@ static void do_unexpected_trap(const struct > > cpu_user_regs *regs) > > die(); > > } > > > > +static bool is_valid_bug_insn(uint32_t insn) > > +{ > > + return insn == BUG_INSN_32 || > > + (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16; > > +} > > + > > +/* Should be used only on Xen code */ > > +static uint32_t read_instr(unsigned long pc) > > +{ > > + uint16_t instr16 = *(uint16_t *)pc; > > + > > + ASSERT(is_kernel_text(pc + 1) || is_kernel_inittext(pc + 1)); > > + > > + if ( GET_INSN_LENGTH(instr16) == 2 ) > > + return instr16; > > + > > + ASSERT(is_kernel_text(pc + 3) || is_kernel_inittext(pc + 3)); > > + > > + return *(uint32_t *)pc; > > +} > > Related to the point made further down: If either of these assertions > fails, > won't we come back again right here? If either of the > is_kernel_*text() > wasn't working quite right, wouldn't we be at risk of entering an > infinite > loop (presumably not quite infinite because of the stack overflowing > at some > point)? It is really possible to have infinite loop here so it should be better to use 'if' with die() or panic(). > > > void do_trap(struct cpu_user_regs *cpu_regs) > > { > > + register_t pc = cpu_regs->sepc; > > + uint32_t instr = read_instr(pc); > > + > > + if ( ( is_valid_bug_insn(instr) ) && ( do_bug_frame(cpu_regs, > > pc) >= 0 ) ) > > No consideration of the kind of exception? I'd expect it is one very > specific one which the BUG insn would raise, and then there's no > point > fetching the insn when it's a different kind of exception. Good point. We should have 0x3 ( breakpoint exception ) in scause register. We can just check that without reading instruction and then also is_valid_bug_insn could be dropped too. > > Further, nit: Certainly no need for the parentheses on the lhs of the > &&. > Having them on the rhs is a matter of taste, so okay, but then the > blanks immediately inside will want dropping. > > > > --- 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> > > Unrelated change? Or did you simply forget to mention in the > description > why it's needed? I added it to "Changes in ..." which I forgot to add, but I will add an explanation to the description. It is better place for it. <xen/lib.h> is needed to be included for the reason that panic() and printk() is used in common/bug.c and RISC-V fails if it is not included with the following errors: common/bug.c:69:9: error: implicit declaration of function 'printk' [-Werror=implicit-function-declaration] 69 | printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno); | ^~~~~~ common/bug.c:77:9: error: implicit declaration of function 'panic' [-Werror=implicit-function-declaration] 77 | panic("Xen BUG at %s%s:%d\n", prefix, filename, lineno); > > ~ Oleksii
On 11.07.2024 10:50, Oleksii wrote: > On Wed, 2024-07-10 at 12:01 +0200, Jan Beulich wrote: >> On 02.07.2024 13:23, Oleksii Kurochko wrote: >>> @@ -101,8 +102,38 @@ static void do_unexpected_trap(const struct >>> cpu_user_regs *regs) >>> die(); >>> } >>> >>> +static bool is_valid_bug_insn(uint32_t insn) >>> +{ >>> + return insn == BUG_INSN_32 || >>> + (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16; >>> +} >>> + >>> +/* Should be used only on Xen code */ >>> +static uint32_t read_instr(unsigned long pc) >>> +{ >>> + uint16_t instr16 = *(uint16_t *)pc; >>> + >>> + ASSERT(is_kernel_text(pc + 1) || is_kernel_inittext(pc + 1)); >>> + >>> + if ( GET_INSN_LENGTH(instr16) == 2 ) >>> + return instr16; >>> + >>> + ASSERT(is_kernel_text(pc + 3) || is_kernel_inittext(pc + 3)); >>> + >>> + return *(uint32_t *)pc; >>> +} >> >> Related to the point made further down: If either of these assertions >> fails, >> won't we come back again right here? If either of the >> is_kernel_*text() >> wasn't working quite right, wouldn't we be at risk of entering an >> infinite >> loop (presumably not quite infinite because of the stack overflowing >> at some >> point)? > It is really possible to have infinite loop here so it should be better > to use 'if' with die() or panic(). > >> >>> void do_trap(struct cpu_user_regs *cpu_regs) >>> { >>> + register_t pc = cpu_regs->sepc; >>> + uint32_t instr = read_instr(pc); >>> + >>> + if ( ( is_valid_bug_insn(instr) ) && ( do_bug_frame(cpu_regs, >>> pc) >= 0 ) ) >> >> No consideration of the kind of exception? I'd expect it is one very >> specific one which the BUG insn would raise, and then there's no >> point >> fetching the insn when it's a different kind of exception. > Good point. > > We should have 0x3 ( breakpoint exception ) in scause register. We can > just check that without reading instruction and then also > is_valid_bug_insn could be dropped too. Just that then you'll also lose the is_kernel_*text() checking, which I understand is there to remind you/us that one this becomes reachable from non-Xen code, adjustments are going to be needed. Jan
On Thu, 2024-07-11 at 11:25 +0200, Jan Beulich wrote: > On 11.07.2024 10:50, Oleksii wrote: > > On Wed, 2024-07-10 at 12:01 +0200, Jan Beulich wrote: > > > On 02.07.2024 13:23, Oleksii Kurochko wrote: > > > > @@ -101,8 +102,38 @@ static void do_unexpected_trap(const > > > > struct > > > > cpu_user_regs *regs) > > > > die(); > > > > } > > > > > > > > +static bool is_valid_bug_insn(uint32_t insn) > > > > +{ > > > > + return insn == BUG_INSN_32 || > > > > + (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16; > > > > +} > > > > + > > > > +/* Should be used only on Xen code */ > > > > +static uint32_t read_instr(unsigned long pc) > > > > +{ > > > > + uint16_t instr16 = *(uint16_t *)pc; > > > > + > > > > + ASSERT(is_kernel_text(pc + 1) || is_kernel_inittext(pc + > > > > 1)); > > > > + > > > > + if ( GET_INSN_LENGTH(instr16) == 2 ) > > > > + return instr16; > > > > + > > > > + ASSERT(is_kernel_text(pc + 3) || is_kernel_inittext(pc + > > > > 3)); > > > > + > > > > + return *(uint32_t *)pc; > > > > +} > > > > > > Related to the point made further down: If either of these > > > assertions > > > fails, > > > won't we come back again right here? If either of the > > > is_kernel_*text() > > > wasn't working quite right, wouldn't we be at risk of entering an > > > infinite > > > loop (presumably not quite infinite because of the stack > > > overflowing > > > at some > > > point)? > > It is really possible to have infinite loop here so it should be > > better > > to use 'if' with die() or panic(). > > > > > > > > > void do_trap(struct cpu_user_regs *cpu_regs) > > > > { > > > > + register_t pc = cpu_regs->sepc; > > > > + uint32_t instr = read_instr(pc); > > > > + > > > > + if ( ( is_valid_bug_insn(instr) ) && ( > > > > do_bug_frame(cpu_regs, > > > > pc) >= 0 ) ) > > > > > > No consideration of the kind of exception? I'd expect it is one > > > very > > > specific one which the BUG insn would raise, and then there's no > > > point > > > fetching the insn when it's a different kind of exception. > > Good point. > > > > We should have 0x3 ( breakpoint exception ) in scause register. We > > can > > just check that without reading instruction and then also > > is_valid_bug_insn could be dropped too. > > Just that then you'll also lose the is_kernel_*text() checking, which > I > understand is there to remind you/us that one this becomes reachable > from non-Xen code, adjustments are going to be needed. One thing I wrote incorrectly is that we still need fetch instruction or at least 16 bits to identify the length of instruction to set proper sepc: cpu_regs->sepc += GET_INSN_LENGTH(instr); We could write that in the following way: cpu_regs->sepc += GET_INSN_LENGTH(*(uint16_t *)pc); Would it be okay? ~ Oleksii
On 11.07.2024 14:14, Oleksii wrote: > On Thu, 2024-07-11 at 11:25 +0200, Jan Beulich wrote: >> On 11.07.2024 10:50, Oleksii wrote: >>> On Wed, 2024-07-10 at 12:01 +0200, Jan Beulich wrote: >>>> On 02.07.2024 13:23, Oleksii Kurochko wrote: >>>>> @@ -101,8 +102,38 @@ static void do_unexpected_trap(const >>>>> struct >>>>> cpu_user_regs *regs) >>>>> die(); >>>>> } >>>>> >>>>> +static bool is_valid_bug_insn(uint32_t insn) >>>>> +{ >>>>> + return insn == BUG_INSN_32 || >>>>> + (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16; >>>>> +} >>>>> + >>>>> +/* Should be used only on Xen code */ >>>>> +static uint32_t read_instr(unsigned long pc) >>>>> +{ >>>>> + uint16_t instr16 = *(uint16_t *)pc; >>>>> + >>>>> + ASSERT(is_kernel_text(pc + 1) || is_kernel_inittext(pc + >>>>> 1)); >>>>> + >>>>> + if ( GET_INSN_LENGTH(instr16) == 2 ) >>>>> + return instr16; >>>>> + >>>>> + ASSERT(is_kernel_text(pc + 3) || is_kernel_inittext(pc + >>>>> 3)); >>>>> + >>>>> + return *(uint32_t *)pc; >>>>> +} >>>> >>>> Related to the point made further down: If either of these >>>> assertions >>>> fails, >>>> won't we come back again right here? If either of the >>>> is_kernel_*text() >>>> wasn't working quite right, wouldn't we be at risk of entering an >>>> infinite >>>> loop (presumably not quite infinite because of the stack >>>> overflowing >>>> at some >>>> point)? >>> It is really possible to have infinite loop here so it should be >>> better >>> to use 'if' with die() or panic(). >>> >>>> >>>>> void do_trap(struct cpu_user_regs *cpu_regs) >>>>> { >>>>> + register_t pc = cpu_regs->sepc; >>>>> + uint32_t instr = read_instr(pc); >>>>> + >>>>> + if ( ( is_valid_bug_insn(instr) ) && ( >>>>> do_bug_frame(cpu_regs, >>>>> pc) >= 0 ) ) >>>> >>>> No consideration of the kind of exception? I'd expect it is one >>>> very >>>> specific one which the BUG insn would raise, and then there's no >>>> point >>>> fetching the insn when it's a different kind of exception. >>> Good point. >>> >>> We should have 0x3 ( breakpoint exception ) in scause register. We >>> can >>> just check that without reading instruction and then also >>> is_valid_bug_insn could be dropped too. >> >> Just that then you'll also lose the is_kernel_*text() checking, which >> I >> understand is there to remind you/us that one this becomes reachable >> from non-Xen code, adjustments are going to be needed. > One thing I wrote incorrectly is that we still need fetch instruction > or at least 16 bits to identify the length of instruction to set proper > sepc: > cpu_regs->sepc += GET_INSN_LENGTH(instr); > > We could write that in the following way: > cpu_regs->sepc += GET_INSN_LENGTH(*(uint16_t *)pc); > Would it be okay? I think so, as long as you retain the assertion in some way, ahead of the deref of pc. Jan
diff --git a/xen/arch/riscv/Kconfig b/xen/arch/riscv/Kconfig index b4b354a778..74ad019fe7 100644 --- a/xen/arch/riscv/Kconfig +++ b/xen/arch/riscv/Kconfig @@ -5,6 +5,7 @@ config RISCV config RISCV_64 def_bool y select 64BIT + select GENERIC_BUG_FRAME config ARCH_DEFCONFIG string diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c index cb18b30ff2..7ba16252fc 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> @@ -101,8 +102,38 @@ static void do_unexpected_trap(const struct cpu_user_regs *regs) die(); } +static bool is_valid_bug_insn(uint32_t insn) +{ + return insn == BUG_INSN_32 || + (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16; +} + +/* Should be used only on Xen code */ +static uint32_t read_instr(unsigned long pc) +{ + uint16_t instr16 = *(uint16_t *)pc; + + ASSERT(is_kernel_text(pc + 1) || is_kernel_inittext(pc + 1)); + + if ( GET_INSN_LENGTH(instr16) == 2 ) + return instr16; + + ASSERT(is_kernel_text(pc + 3) || is_kernel_inittext(pc + 3)); + + return *(uint32_t *)pc; +} + void do_trap(struct cpu_user_regs *cpu_regs) { + register_t pc = cpu_regs->sepc; + uint32_t instr = read_instr(pc); + + if ( ( is_valid_bug_insn(instr) ) && ( do_bug_frame(cpu_regs, pc) >= 0 ) ) + { + cpu_regs->sepc += GET_INSN_LENGTH(instr); + return; + } + do_unexpected_trap(cpu_regs); } 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>
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- xen/arch/riscv/Kconfig | 1 + xen/arch/riscv/traps.c | 31 +++++++++++++++++++++++++++++++ xen/common/bug.c | 1 + 3 files changed, 33 insertions(+)