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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

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