Re: [PATCH v5 4/7] xen/riscv: introduce decode_cause() stuff

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

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@xxxxxxxxx>
> > ---
> > 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