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

[Julien Grall <julien@xen.org>]

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,

-- 
Julien Grall