Re: [PATCH v1 08/14] xen/riscv: introduce decode_cause() stuff

[Andrew Cooper <Andrew.Cooper3@citrix.com>]

On 20/01/2023 2:59 pm, Oleksii Kurochko wrote:
> diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
> index 3201b851ef..dd64f053a5 100644
> --- a/xen/arch/riscv/traps.c
> +++ b/xen/arch/riscv/traps.c
> @@ -4,8 +4,96 @@
>   *
>   * RISC-V Trap handlers
>   */
> +#include <asm/csr.h>
> +#include <asm/early_printk.h>
>  #include <asm/processor.h>
>  #include <asm/traps.h>
> +#include <xen/errno.h>
> +
> +const char *decode_trap_cause(unsigned long cause)

These should be static as you've not put a declaration in a header
file.  But as it stands, you'll then get a compiler warning on
decode_cause() as it's not used.

I would merge this patch with the following patch, as the following
patch is very related to this, and then you can get everything nicely
static without unused warnings.

> +{
> +    switch ( cause )
> +    {
> +    case CAUSE_MISALIGNED_FETCH:
> +        return "Instruction Address Misaligned";
> +    case CAUSE_FETCH_ACCESS:
> +        return "Instruction Access Fault";
> +    case CAUSE_ILLEGAL_INSTRUCTION:
> +        return "Illegal Instruction";
> +    case CAUSE_BREAKPOINT:
> +        return "Breakpoint";
> +    case CAUSE_MISALIGNED_LOAD:
> +        return "Load Address Misaligned";
> +    case CAUSE_LOAD_ACCESS:
> +        return "Load Access Fault";
> +    case CAUSE_MISALIGNED_STORE:
> +        return "Store/AMO Address Misaligned";
> +    case CAUSE_STORE_ACCESS:
> +        return "Store/AMO Access Fault";
> +    case CAUSE_USER_ECALL:
> +        return "Environment Call from U-Mode";
> +    case CAUSE_SUPERVISOR_ECALL:
> +        return "Environment Call from S-Mode";
> +    case CAUSE_MACHINE_ECALL:
> +        return "Environment Call from M-Mode";
> +    case CAUSE_FETCH_PAGE_FAULT:
> +        return "Instruction Page Fault";
> +    case CAUSE_LOAD_PAGE_FAULT:
> +        return "Load Page Fault";
> +    case CAUSE_STORE_PAGE_FAULT:
> +        return "Store/AMO Page Fault";
> +    case CAUSE_FETCH_GUEST_PAGE_FAULT:
> +        return "Instruction Guest Page Fault";
> +    case CAUSE_LOAD_GUEST_PAGE_FAULT:
> +        return "Load Guest Page Fault";
> +    case CAUSE_VIRTUAL_INST_FAULT:
> +        return "Virtualized Instruction Fault";
> +    case CAUSE_STORE_GUEST_PAGE_FAULT:
> +        return "Guest Store/AMO Page Fault";
> +    default:
> +        return "UNKNOWN";

This style tends to lead to poor code generation.  You probably want:

const char *decode_trap_cause(unsigned long cause)
{
    static const char *const trap_causes[] = {
        [CAUSE_MISALIGNED_FETCH] = "Instruction Address Misaligned",
        ...
        [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";
}

(note the trailing comma on the final entry, which is there to simply
future diffs)

However, given the hope to get snprintf() wired up, you actually want to
to adjust this to:

    if ( cause < ARRAY_SIZE(trap_causes) )
        return trap_causes[cause];
    return NULL;

And render the raw cause number for the unknown case, because that is
far more useful for whomever is debugging.

~Andrew