| Message ID | 8c7b4a5d328be8b1cd2aa99c8d9a7883e4969600.1678976127.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RISCV basic exception handling implementation | expand |
Hi Oleksii, On 16/03/2023 14:39, Oleksii Kurochko wrote: > The patch introduces stuff needed to decode a reason of an > exception. > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > --- > Changes in V5: > - Remove <xen/error.h> from riscv/traps/c as nothing would require > inclusion. > - decode_reserved_interrupt_cause(), decode_interrupt_cause(), decode_cause, do_unexpected_trap() > were made as static they are expected to be used only in traps.c > - use LINK_TO_LOAD() for addresses which can be linker time relative. > --- > Changes in V4: > - fix string in decode_reserved_interrupt_cause() > --- > Changes in V3: > - Nothing changed > --- > Changes in V2: > - Make decode_trap_cause() more optimization friendly. > - Merge the pathc which introduces do_unexpected_trap() to the current one. > --- > xen/arch/riscv/traps.c | 87 +++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 86 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c > index ccd3593f5a..8a1529e0c5 100644 > --- a/xen/arch/riscv/traps.c > +++ b/xen/arch/riscv/traps.c > @@ -4,10 +4,95 @@ > * > * RISC-V Trap handlers > */ > + > +#include <xen/lib.h> > + > +#include <asm/boot-info.h> > +#include <asm/csr.h> > +#include <asm/early_printk.h> > #include <asm/processor.h> > #include <asm/traps.h> > > -void do_trap(struct cpu_user_regs *cpu_regs) > +static const char *decode_trap_cause(unsigned long cause) > +{ > + static const char *const trap_causes[] = { > + [CAUSE_MISALIGNED_FETCH] = "Instruction Address Misaligned", > + [CAUSE_FETCH_ACCESS] = "Instruction Access Fault", > + [CAUSE_ILLEGAL_INSTRUCTION] = "Illegal Instruction", > + [CAUSE_BREAKPOINT] = "Breakpoint", > + [CAUSE_MISALIGNED_LOAD] = "Load Address Misaligned", > + [CAUSE_LOAD_ACCESS] = "Load Access Fault", > + [CAUSE_MISALIGNED_STORE] = "Store/AMO Address Misaligned", > + [CAUSE_STORE_ACCESS] = "Store/AMO Access Fault", > + [CAUSE_USER_ECALL] = "Environment Call from U-Mode", > + [CAUSE_SUPERVISOR_ECALL] = "Environment Call from S-Mode", > + [CAUSE_MACHINE_ECALL] = "Environment Call from M-Mode", > + [CAUSE_FETCH_PAGE_FAULT] = "Instruction Page Fault", > + [CAUSE_LOAD_PAGE_FAULT] = "Load Page Fault", > + [CAUSE_STORE_PAGE_FAULT] = "Store/AMO Page Fault", > + [CAUSE_FETCH_GUEST_PAGE_FAULT] = "Instruction Guest Page Fault", > + [CAUSE_LOAD_GUEST_PAGE_FAULT] = "Load Guest Page Fault", > + [CAUSE_VIRTUAL_INST_FAULT] = "Virtualized Instruction Fault", > + [CAUSE_STORE_GUEST_PAGE_FAULT] = "Guest Store/AMO Page Fault", > + }; > + > + if ( cause < ARRAY_SIZE(trap_causes) && trap_causes[cause] ) > + return trap_causes[cause]; > + return "UNKNOWN"; > +} > + > +static const char *decode_reserved_interrupt_cause(unsigned long irq_cause) > +{ > + switch ( irq_cause ) > + { > + case IRQ_M_SOFT: > + return "M-mode Software Interrupt"; > + case IRQ_M_TIMER: > + return "M-mode TIMER Interrupt"; > + case IRQ_M_EXT: > + return "M-mode External Interrupt"; > + default: > + return "UNKNOWN IRQ type"; > + } > +} > + > +static const char *decode_interrupt_cause(unsigned long cause) > +{ > + unsigned long irq_cause = cause & ~CAUSE_IRQ_FLAG; > + > + switch ( irq_cause ) > + { > + case IRQ_S_SOFT: > + return "Supervisor Software Interrupt"; > + case IRQ_S_TIMER: > + return "Supervisor Timer Interrupt"; > + case IRQ_S_EXT: > + return "Supervisor External Interrupt"; > + default: > + return decode_reserved_interrupt_cause(irq_cause); > + } > +} > + > +static const char *decode_cause(unsigned long cause) > +{ > + if ( cause & CAUSE_IRQ_FLAG ) > + return decode_interrupt_cause(cause); > + > + return decode_trap_cause(cause); > +} > + > +static void do_unexpected_trap(const struct cpu_user_regs *regs) > { > + unsigned long cause = csr_read(CSR_SCAUSE); > + > + early_printk("Unhandled exception: "); > + early_printk(LINK_TO_LOAD(decode_cause(cause))); The use of LINK_TO_LOAD is the sort of things that is worth documenting because this would raise quite a few questions. The comment on top of LINK_TO_LOAD suggests the macro can only be used while the MMU is off. But I would expect do_unexpected_trap() to be used also after the MMU is on. Isn't it going to be the case? Furthermore, AFAICT LINK_TO_LOAD() assumes that a runtime address is given. While I believe this could be true for pointer returned by decode_trap_cause() (I remember seen on Arm that an array of string would store the runtime address), I am not convinced this is the case for pointer returned by decode_interrupt_cause(). Lastly, I think you will want to document what functions can be used when the MMU is off and possibly splitting the code. So it is easier for someone to figure out in which context the function and if this is safe to use. Cheers,
Hi Julien, On Tue, 2023-03-21 at 17:33 +0000, Julien Grall wrote: > Hi Oleksii, > > On 16/03/2023 14:39, Oleksii Kurochko wrote: > > The patch introduces stuff needed to decode a reason of an > > exception. > > > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > > --- > > Changes in V5: > > - Remove <xen/error.h> from riscv/traps/c as nothing would > > require > > inclusion. > > - decode_reserved_interrupt_cause(), decode_interrupt_cause(), > > decode_cause, do_unexpected_trap() > > were made as static they are expected to be used only in > > traps.c > > - use LINK_TO_LOAD() for addresses which can be linker time > > relative. > > --- > > Changes in V4: > > - fix string in decode_reserved_interrupt_cause() > > --- > > Changes in V3: > > - Nothing changed > > --- > > Changes in V2: > > - Make decode_trap_cause() more optimization friendly. > > - Merge the pathc which introduces do_unexpected_trap() to the > > current one. > > --- > > xen/arch/riscv/traps.c | 87 > > +++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 86 insertions(+), 1 deletion(-) > > > > diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c > > index ccd3593f5a..8a1529e0c5 100644 > > --- a/xen/arch/riscv/traps.c > > +++ b/xen/arch/riscv/traps.c > > @@ -4,10 +4,95 @@ > > * > > * RISC-V Trap handlers > > */ > > + > > +#include <xen/lib.h> > > + > > +#include <asm/boot-info.h> > > +#include <asm/csr.h> > > +#include <asm/early_printk.h> > > #include <asm/processor.h> > > #include <asm/traps.h> > > > > -void do_trap(struct cpu_user_regs *cpu_regs) > > +static const char *decode_trap_cause(unsigned long cause) > > +{ > > + static const char *const trap_causes[] = { > > + [CAUSE_MISALIGNED_FETCH] = "Instruction Address > > Misaligned", > > + [CAUSE_FETCH_ACCESS] = "Instruction Access Fault", > > + [CAUSE_ILLEGAL_INSTRUCTION] = "Illegal Instruction", > > + [CAUSE_BREAKPOINT] = "Breakpoint", > > + [CAUSE_MISALIGNED_LOAD] = "Load Address Misaligned", > > + [CAUSE_LOAD_ACCESS] = "Load Access Fault", > > + [CAUSE_MISALIGNED_STORE] = "Store/AMO Address Misaligned", > > + [CAUSE_STORE_ACCESS] = "Store/AMO Access Fault", > > + [CAUSE_USER_ECALL] = "Environment Call from U-Mode", > > + [CAUSE_SUPERVISOR_ECALL] = "Environment Call from S-Mode", > > + [CAUSE_MACHINE_ECALL] = "Environment Call from M-Mode", > > + [CAUSE_FETCH_PAGE_FAULT] = "Instruction Page Fault", > > + [CAUSE_LOAD_PAGE_FAULT] = "Load Page Fault", > > + [CAUSE_STORE_PAGE_FAULT] = "Store/AMO Page Fault", > > + [CAUSE_FETCH_GUEST_PAGE_FAULT] = "Instruction Guest Page > > Fault", > > + [CAUSE_LOAD_GUEST_PAGE_FAULT] = "Load Guest Page Fault", > > + [CAUSE_VIRTUAL_INST_FAULT] = "Virtualized Instruction > > Fault", > > + [CAUSE_STORE_GUEST_PAGE_FAULT] = "Guest Store/AMO Page > > Fault", > > + }; > > + > > + if ( cause < ARRAY_SIZE(trap_causes) && trap_causes[cause] ) > > + return trap_causes[cause]; > > + return "UNKNOWN"; > > +} > > + > > +static const char *decode_reserved_interrupt_cause(unsigned long > > irq_cause) > > +{ > > + switch ( irq_cause ) > > + { > > + case IRQ_M_SOFT: > > + return "M-mode Software Interrupt"; > > + case IRQ_M_TIMER: > > + return "M-mode TIMER Interrupt"; > > + case IRQ_M_EXT: > > + return "M-mode External Interrupt"; > > + default: > > + return "UNKNOWN IRQ type"; > > + } > > +} > > + > > +static const char *decode_interrupt_cause(unsigned long cause) > > +{ > > + unsigned long irq_cause = cause & ~CAUSE_IRQ_FLAG; > > + > > + switch ( irq_cause ) > > + { > > + case IRQ_S_SOFT: > > + return "Supervisor Software Interrupt"; > > + case IRQ_S_TIMER: > > + return "Supervisor Timer Interrupt"; > > + case IRQ_S_EXT: > > + return "Supervisor External Interrupt"; > > + default: > > + return decode_reserved_interrupt_cause(irq_cause); > > + } > > +} > > + > > +static const char *decode_cause(unsigned long cause) > > +{ > > + if ( cause & CAUSE_IRQ_FLAG ) > > + return decode_interrupt_cause(cause); > > + > > + return decode_trap_cause(cause); > > +} > > + > > +static void do_unexpected_trap(const struct cpu_user_regs *regs) > > { > > + unsigned long cause = csr_read(CSR_SCAUSE); > > + > > + early_printk("Unhandled exception: "); > > + early_printk(LINK_TO_LOAD(decode_cause(cause))); > > The use of LINK_TO_LOAD is the sort of things that is worth > documenting > because this would raise quite a few questions. > > The comment on top of LINK_TO_LOAD suggests the macro can only be > used > while the MMU is off. But I would expect do_unexpected_trap() to be > used > also after the MMU is on. Isn't it going to be the case? Yes, you are right. it will be an issue now. It was not an issue before when it was used 1:1 mapping. So I have to add a check 'if ( is_mmu_enabled ) ... ' inside LINK_TO_LOAD macros. > > Furthermore, AFAICT LINK_TO_LOAD() assumes that a runtime address is > given. While I believe this could be true for pointer returned by > decode_trap_cause() (I remember seen on Arm that an array of string > would store the runtime address), I am not convinced this is the case > for pointer returned by decode_interrupt_cause(). It might be an issue. I'll double check what will be returned from decode_interrupt_cause(). > > Lastly, I think you will want to document what functions can be used > when the MMU is off and possibly splitting the code. So it is easier > for > someone to figure out in which context the function and if this is > safe > to use. It make sense. Thanks a lot for the comments. ~ Oleksii
On 22.03.2023 11:20, Oleksii wrote: > On Tue, 2023-03-21 at 17:33 +0000, Julien Grall wrote: >> On 16/03/2023 14:39, Oleksii Kurochko wrote: >>> --- a/xen/arch/riscv/traps.c >>> +++ b/xen/arch/riscv/traps.c >>> @@ -4,10 +4,95 @@ >>> * >>> * RISC-V Trap handlers >>> */ >>> + >>> +#include <xen/lib.h> >>> + >>> +#include <asm/boot-info.h> >>> +#include <asm/csr.h> >>> +#include <asm/early_printk.h> >>> #include <asm/processor.h> >>> #include <asm/traps.h> >>> >>> -void do_trap(struct cpu_user_regs *cpu_regs) >>> +static const char *decode_trap_cause(unsigned long cause) >>> +{ >>> + static const char *const trap_causes[] = { >>> + [CAUSE_MISALIGNED_FETCH] = "Instruction Address >>> Misaligned", >>> + [CAUSE_FETCH_ACCESS] = "Instruction Access Fault", >>> + [CAUSE_ILLEGAL_INSTRUCTION] = "Illegal Instruction", >>> + [CAUSE_BREAKPOINT] = "Breakpoint", >>> + [CAUSE_MISALIGNED_LOAD] = "Load Address Misaligned", >>> + [CAUSE_LOAD_ACCESS] = "Load Access Fault", >>> + [CAUSE_MISALIGNED_STORE] = "Store/AMO Address Misaligned", >>> + [CAUSE_STORE_ACCESS] = "Store/AMO Access Fault", >>> + [CAUSE_USER_ECALL] = "Environment Call from U-Mode", >>> + [CAUSE_SUPERVISOR_ECALL] = "Environment Call from S-Mode", >>> + [CAUSE_MACHINE_ECALL] = "Environment Call from M-Mode", >>> + [CAUSE_FETCH_PAGE_FAULT] = "Instruction Page Fault", >>> + [CAUSE_LOAD_PAGE_FAULT] = "Load Page Fault", >>> + [CAUSE_STORE_PAGE_FAULT] = "Store/AMO Page Fault", >>> + [CAUSE_FETCH_GUEST_PAGE_FAULT] = "Instruction Guest Page >>> Fault", >>> + [CAUSE_LOAD_GUEST_PAGE_FAULT] = "Load Guest Page Fault", >>> + [CAUSE_VIRTUAL_INST_FAULT] = "Virtualized Instruction >>> Fault", >>> + [CAUSE_STORE_GUEST_PAGE_FAULT] = "Guest Store/AMO Page >>> Fault", >>> + }; >>> + >>> + if ( cause < ARRAY_SIZE(trap_causes) && trap_causes[cause] ) >>> + return trap_causes[cause]; >>> + return "UNKNOWN"; >>> +} >>> + >>> +static const char *decode_reserved_interrupt_cause(unsigned long >>> irq_cause) >>> +{ >>> + switch ( irq_cause ) >>> + { >>> + case IRQ_M_SOFT: >>> + return "M-mode Software Interrupt"; >>> + case IRQ_M_TIMER: >>> + return "M-mode TIMER Interrupt"; >>> + case IRQ_M_EXT: >>> + return "M-mode External Interrupt"; >>> + default: >>> + return "UNKNOWN IRQ type"; >>> + } >>> +} >>> + >>> +static const char *decode_interrupt_cause(unsigned long cause) >>> +{ >>> + unsigned long irq_cause = cause & ~CAUSE_IRQ_FLAG; >>> + >>> + switch ( irq_cause ) >>> + { >>> + case IRQ_S_SOFT: >>> + return "Supervisor Software Interrupt"; >>> + case IRQ_S_TIMER: >>> + return "Supervisor Timer Interrupt"; >>> + case IRQ_S_EXT: >>> + return "Supervisor External Interrupt"; >>> + default: >>> + return decode_reserved_interrupt_cause(irq_cause); >>> + } >>> +} >>> + >>> +static const char *decode_cause(unsigned long cause) >>> +{ >>> + if ( cause & CAUSE_IRQ_FLAG ) >>> + return decode_interrupt_cause(cause); >>> + >>> + return decode_trap_cause(cause); >>> +} >>> + >>> +static void do_unexpected_trap(const struct cpu_user_regs *regs) >>> { >>> + unsigned long cause = csr_read(CSR_SCAUSE); >>> + >>> + early_printk("Unhandled exception: "); >>> + early_printk(LINK_TO_LOAD(decode_cause(cause))); >> >> The use of LINK_TO_LOAD is the sort of things that is worth >> documenting >> because this would raise quite a few questions. >> >> The comment on top of LINK_TO_LOAD suggests the macro can only be >> used >> while the MMU is off. But I would expect do_unexpected_trap() to be >> used >> also after the MMU is on. Isn't it going to be the case? > Yes, you are right. it will be an issue now. It was not an issue before > when it was used 1:1 mapping. So I have to add a check 'if ( > is_mmu_enabled ) ... ' inside LINK_TO_LOAD macros. I don't think that's going to be enough: What decode_cause() returns may be a link-time value when a result of reading from trap_causes[], but - as Julien did say already - it can also be a runtime value when coming from any of the "return <string-literal>", calculated in a PC- relative way. I guess you will need to apply LINK_TO_LOAD() to the trap_causes[] access itself. But as said before - I'm unconvinced this approach will scale, because you'll need to apply the wrapper to anything which can be reached prior to you enabling the MMU. Whether you can contain this to RISC-V-only code is unclear; I don't think it'll be acceptable to change any part of common code to meet your special needs. Jan
On Wed, 2023-03-22 at 13:26 +0100, Jan Beulich wrote: > On 22.03.2023 11:20, Oleksii wrote: > > On Tue, 2023-03-21 at 17:33 +0000, Julien Grall wrote: > > > On 16/03/2023 14:39, Oleksii Kurochko wrote: > > > > --- a/xen/arch/riscv/traps.c > > > > +++ b/xen/arch/riscv/traps.c > > > > @@ -4,10 +4,95 @@ > > > > * > > > > * RISC-V Trap handlers > > > > */ > > > > + > > > > +#include <xen/lib.h> > > > > + > > > > +#include <asm/boot-info.h> > > > > +#include <asm/csr.h> > > > > +#include <asm/early_printk.h> > > > > #include <asm/processor.h> > > > > #include <asm/traps.h> > > > > > > > > -void do_trap(struct cpu_user_regs *cpu_regs) > > > > +static const char *decode_trap_cause(unsigned long cause) > > > > +{ > > > > + static const char *const trap_causes[] = { > > > > + [CAUSE_MISALIGNED_FETCH] = "Instruction Address > > > > Misaligned", > > > > + [CAUSE_FETCH_ACCESS] = "Instruction Access Fault", > > > > + [CAUSE_ILLEGAL_INSTRUCTION] = "Illegal Instruction", > > > > + [CAUSE_BREAKPOINT] = "Breakpoint", > > > > + [CAUSE_MISALIGNED_LOAD] = "Load Address Misaligned", > > > > + [CAUSE_LOAD_ACCESS] = "Load Access Fault", > > > > + [CAUSE_MISALIGNED_STORE] = "Store/AMO Address > > > > Misaligned", > > > > + [CAUSE_STORE_ACCESS] = "Store/AMO Access Fault", > > > > + [CAUSE_USER_ECALL] = "Environment Call from U-Mode", > > > > + [CAUSE_SUPERVISOR_ECALL] = "Environment Call from S- > > > > Mode", > > > > + [CAUSE_MACHINE_ECALL] = "Environment Call from M- > > > > Mode", > > > > + [CAUSE_FETCH_PAGE_FAULT] = "Instruction Page Fault", > > > > + [CAUSE_LOAD_PAGE_FAULT] = "Load Page Fault", > > > > + [CAUSE_STORE_PAGE_FAULT] = "Store/AMO Page Fault", > > > > + [CAUSE_FETCH_GUEST_PAGE_FAULT] = "Instruction Guest > > > > Page > > > > Fault", > > > > + [CAUSE_LOAD_GUEST_PAGE_FAULT] = "Load Guest Page > > > > Fault", > > > > + [CAUSE_VIRTUAL_INST_FAULT] = "Virtualized Instruction > > > > Fault", > > > > + [CAUSE_STORE_GUEST_PAGE_FAULT] = "Guest Store/AMO Page > > > > Fault", > > > > + }; > > > > + > > > > + if ( cause < ARRAY_SIZE(trap_causes) && trap_causes[cause] > > > > ) > > > > + return trap_causes[cause]; > > > > + return "UNKNOWN"; > > > > +} > > > > + > > > > +static const char *decode_reserved_interrupt_cause(unsigned > > > > long > > > > irq_cause) > > > > +{ > > > > + switch ( irq_cause ) > > > > + { > > > > + case IRQ_M_SOFT: > > > > + return "M-mode Software Interrupt"; > > > > + case IRQ_M_TIMER: > > > > + return "M-mode TIMER Interrupt"; > > > > + case IRQ_M_EXT: > > > > + return "M-mode External Interrupt"; > > > > + default: > > > > + return "UNKNOWN IRQ type"; > > > > + } > > > > +} > > > > + > > > > +static const char *decode_interrupt_cause(unsigned long cause) > > > > +{ > > > > + unsigned long irq_cause = cause & ~CAUSE_IRQ_FLAG; > > > > + > > > > + switch ( irq_cause ) > > > > + { > > > > + case IRQ_S_SOFT: > > > > + return "Supervisor Software Interrupt"; > > > > + case IRQ_S_TIMER: > > > > + return "Supervisor Timer Interrupt"; > > > > + case IRQ_S_EXT: > > > > + return "Supervisor External Interrupt"; > > > > + default: > > > > + return decode_reserved_interrupt_cause(irq_cause); > > > > + } > > > > +} > > > > + > > > > +static const char *decode_cause(unsigned long cause) > > > > +{ > > > > + if ( cause & CAUSE_IRQ_FLAG ) > > > > + return decode_interrupt_cause(cause); > > > > + > > > > + return decode_trap_cause(cause); > > > > +} > > > > + > > > > +static void do_unexpected_trap(const struct cpu_user_regs > > > > *regs) > > > > { > > > > + unsigned long cause = csr_read(CSR_SCAUSE); > > > > + > > > > + early_printk("Unhandled exception: "); > > > > + early_printk(LINK_TO_LOAD(decode_cause(cause))); > > > > > > The use of LINK_TO_LOAD is the sort of things that is worth > > > documenting > > > because this would raise quite a few questions. > > > > > > The comment on top of LINK_TO_LOAD suggests the macro can only be > > > used > > > while the MMU is off. But I would expect do_unexpected_trap() to > > > be > > > used > > > also after the MMU is on. Isn't it going to be the case? > > Yes, you are right. it will be an issue now. It was not an issue > > before > > when it was used 1:1 mapping. So I have to add a check 'if ( > > is_mmu_enabled ) ... ' inside LINK_TO_LOAD macros. > > I don't think that's going to be enough: What decode_cause() returns > may be a link-time value when a result of reading from trap_causes[], > but - as Julien did say already - it can also be a runtime value when > coming from any of the "return <string-literal>", calculated in a PC- > relative way. I guess you will need to apply LINK_TO_LOAD() to the > trap_causes[] access itself. Probably you are right here. > > But as said before - I'm unconvinced this approach will scale, > because > you'll need to apply the wrapper to anything which can be reached > prior > to you enabling the MMU. Whether you can contain this to RISC-V-only > code is unclear; I don't think it'll be acceptable to change any part > of common code to meet your special needs. But it looks like it is only two places where it should be done: 1. As you mentioned LINK_TO_LOAD() should be applied for trap_causes. 2. And it should be applied inside do_bug_frame() for getting an start/end address of bug_frame. I want to note that do_bug_frame() will be removed after RISC-V is ready to switch to generic bug implementation. The next step after the current patch series is merged is to enable MMU, so it shouldn't be new use cases where it is needed to use LINK_TO_LOAD(). If it is not acceptable to change any part of common code ( as I understand in this case it is do_unexpected_trap() and all that is called inside it ) have I to introduce two types of function do_unexpected_trap() for when MMU is enabled and not? I have a strong feeling that I misunderstood you... ~ Oleksii
On 22.03.2023 14:32, Oleksii wrote: > On Wed, 2023-03-22 at 13:26 +0100, Jan Beulich wrote: >> But as said before - I'm unconvinced this approach will scale, >> because >> you'll need to apply the wrapper to anything which can be reached >> prior >> to you enabling the MMU. Whether you can contain this to RISC-V-only >> code is unclear; I don't think it'll be acceptable to change any part >> of common code to meet your special needs. > But it looks like it is only two places where it should be done: > 1. As you mentioned LINK_TO_LOAD() should be applied for trap_causes. > 2. And it should be applied inside do_bug_frame() for getting an > start/end address of bug_frame. I want to note that do_bug_frame() will > be removed after RISC-V is ready to switch to generic bug > implementation. > > The next step after the current patch series is merged is to enable > MMU, so it shouldn't be new use cases where it is needed to use > LINK_TO_LOAD(). I'm not convinced. You can't stick to using earlyprintk only beyond very short term. Yet I expect you also don't want to use if ( early ) earlyprintk(); else printk(); everywhere in exception handling code (and anywhere else in code which is reachable before the MMU is turned on). This extends to uses of BUG() and alike in such early code, which - when they trigger - want to use printk() (only). Whether printk(), somehow, involves an array access similar to the ones you're presently aware of you simply shouldn't depend on (it's an implementation detail in a separate subsystem). > If it is not acceptable to change any part of common code ( as I > understand in this case it is do_unexpected_trap() and all that is > called inside it ) have I to introduce two types of function > do_unexpected_trap() for when MMU is enabled and not? By "common code" I mean code outside of arch/riscv/. And I sincerely hope you / we can get away without duplicated functions. Jan
On Wed, 2023-03-22 at 14:46 +0100, Jan Beulich wrote: > On 22.03.2023 14:32, Oleksii wrote: > > On Wed, 2023-03-22 at 13:26 +0100, Jan Beulich wrote: > > > But as said before - I'm unconvinced this approach will scale, > > > because > > > you'll need to apply the wrapper to anything which can be reached > > > prior > > > to you enabling the MMU. Whether you can contain this to RISC-V- > > > only > > > code is unclear; I don't think it'll be acceptable to change any > > > part > > > of common code to meet your special needs. > > But it looks like it is only two places where it should be done: > > 1. As you mentioned LINK_TO_LOAD() should be applied for > > trap_causes. > > 2. And it should be applied inside do_bug_frame() for getting an > > start/end address of bug_frame. I want to note that do_bug_frame() > > will > > be removed after RISC-V is ready to switch to generic bug > > implementation. > > > > The next step after the current patch series is merged is to enable > > MMU, so it shouldn't be new use cases where it is needed to use > > LINK_TO_LOAD(). > > I'm not convinced. You can't stick to using earlyprintk only beyond > very short term. Yet I expect you also don't want to use > > if ( early ) > earlyprintk(); > else > printk(); > > everywhere in exception handling code (and anywhere else in code > which is reachable before the MMU is turned on). This extends to > uses of BUG() and alike in such early code, which - when they > trigger - want to use printk() (only). Whether printk(), somehow, > involves an array access similar to the ones you're presently > aware of you simply shouldn't depend on (it's an implementation > detail in a separate subsystem). I planned to changed all earlyprintk() to printk() in traps.c after printk will be ready. I would like to remind that xen/common code isn't compiled now for RISC-V. > > > If it is not acceptable to change any part of common code ( as I > > understand in this case it is do_unexpected_trap() and all that is > > called inside it ) have I to introduce two types of function > > do_unexpected_trap() for when MMU is enabled and not? > > By "common code" I mean code outside of arch/riscv/. And I > sincerely hope you / we can get away without duplicated functions. Got it. Than it might be an issue with do_bug_frame() as RISCV should be switched to it at the end. Then it looks like enabling of MMU should go first but in that case this not clear what to do with BUG(), WARN() etc as for them it is needed excpetion handling functionality and MMU related code uses the macros. ~ Oleksii
On 22.03.2023 15:59, Oleksii wrote: > Then it looks like enabling of MMU should go first but in that case > this not clear what to do with BUG(), WARN() etc as for them it is > needed excpetion handling functionality and MMU related code uses the > macros. It's still possible to reconsider and do the page table building and MMU enabling before entering C. Or to have all that in a separate source/object file, which is prohibited to use about all of the infrastructure. And of course the option of (re-)applying relocations also continues to exist. Jan
diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c index ccd3593f5a..8a1529e0c5 100644 --- a/xen/arch/riscv/traps.c +++ b/xen/arch/riscv/traps.c @@ -4,10 +4,95 @@ * * RISC-V Trap handlers */ + +#include <xen/lib.h> + +#include <asm/boot-info.h> +#include <asm/csr.h> +#include <asm/early_printk.h> #include <asm/processor.h> #include <asm/traps.h> -void do_trap(struct cpu_user_regs *cpu_regs) +static const char *decode_trap_cause(unsigned long cause) +{ + static const char *const trap_causes[] = { + [CAUSE_MISALIGNED_FETCH] = "Instruction Address Misaligned", + [CAUSE_FETCH_ACCESS] = "Instruction Access Fault", + [CAUSE_ILLEGAL_INSTRUCTION] = "Illegal Instruction", + [CAUSE_BREAKPOINT] = "Breakpoint", + [CAUSE_MISALIGNED_LOAD] = "Load Address Misaligned", + [CAUSE_LOAD_ACCESS] = "Load Access Fault", + [CAUSE_MISALIGNED_STORE] = "Store/AMO Address Misaligned", + [CAUSE_STORE_ACCESS] = "Store/AMO Access Fault", + [CAUSE_USER_ECALL] = "Environment Call from U-Mode", + [CAUSE_SUPERVISOR_ECALL] = "Environment Call from S-Mode", + [CAUSE_MACHINE_ECALL] = "Environment Call from M-Mode", + [CAUSE_FETCH_PAGE_FAULT] = "Instruction Page Fault", + [CAUSE_LOAD_PAGE_FAULT] = "Load Page Fault", + [CAUSE_STORE_PAGE_FAULT] = "Store/AMO Page Fault", + [CAUSE_FETCH_GUEST_PAGE_FAULT] = "Instruction Guest Page Fault", + [CAUSE_LOAD_GUEST_PAGE_FAULT] = "Load Guest Page Fault", + [CAUSE_VIRTUAL_INST_FAULT] = "Virtualized Instruction Fault", + [CAUSE_STORE_GUEST_PAGE_FAULT] = "Guest Store/AMO Page Fault", + }; + + if ( cause < ARRAY_SIZE(trap_causes) && trap_causes[cause] ) + return trap_causes[cause]; + return "UNKNOWN"; +} + +static const char *decode_reserved_interrupt_cause(unsigned long irq_cause) +{ + switch ( irq_cause ) + { + case IRQ_M_SOFT: + return "M-mode Software Interrupt"; + case IRQ_M_TIMER: + return "M-mode TIMER Interrupt"; + case IRQ_M_EXT: + return "M-mode External Interrupt"; + default: + return "UNKNOWN IRQ type"; + } +} + +static const char *decode_interrupt_cause(unsigned long cause) +{ + unsigned long irq_cause = cause & ~CAUSE_IRQ_FLAG; + + switch ( irq_cause ) + { + case IRQ_S_SOFT: + return "Supervisor Software Interrupt"; + case IRQ_S_TIMER: + return "Supervisor Timer Interrupt"; + case IRQ_S_EXT: + return "Supervisor External Interrupt"; + default: + return decode_reserved_interrupt_cause(irq_cause); + } +} + +static const char *decode_cause(unsigned long cause) +{ + if ( cause & CAUSE_IRQ_FLAG ) + return decode_interrupt_cause(cause); + + return decode_trap_cause(cause); +} + +static void do_unexpected_trap(const struct cpu_user_regs *regs) { + unsigned long cause = csr_read(CSR_SCAUSE); + + early_printk("Unhandled exception: "); + early_printk(LINK_TO_LOAD(decode_cause(cause))); + early_printk("\n"); + die(); } + +void do_trap(struct cpu_user_regs *cpu_regs) +{ + do_unexpected_trap(cpu_regs); +}
The patch introduces stuff needed to decode a reason of an exception. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in V5: - Remove <xen/error.h> from riscv/traps/c as nothing would require inclusion. - decode_reserved_interrupt_cause(), decode_interrupt_cause(), decode_cause, do_unexpected_trap() were made as static they are expected to be used only in traps.c - use LINK_TO_LOAD() for addresses which can be linker time relative. --- Changes in V4: - fix string in decode_reserved_interrupt_cause() --- Changes in V3: - Nothing changed --- Changes in V2: - Make decode_trap_cause() more optimization friendly. - Merge the pathc which introduces do_unexpected_trap() to the current one. --- xen/arch/riscv/traps.c | 87 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 86 insertions(+), 1 deletion(-)