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

[Jan Beulich <jbeulich@xxxxxxxx>]

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