Re: [PATCH v11 2/5] xen/riscv: introduce decode_cause() stuff

[oleksii.kurochko@xxxxxxxxx]

On Thu, 2024-08-01 at 13:44 +0200, Jan Beulich wrote:
> On 24.07.2024 17:31, Oleksii Kurochko wrote:
> > The patch introduces stuff needed to decode a reason of an
> > exception.
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > Acked-by: Alistair Francis <alistair.francis@xxxxxxx>
> > Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> > ---
> > Changes in V11:
> >  - Nothing changed. Only rebase.
> > ---
> > Changes in V10:
> >  - add Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> > ---
> > Changes in V9:
> >  - This patch was reverted as breaks both release and randconfig
> > builds.
> >    I don't see the failures now. ( probably it was because of
> > printk usage
> >    which was not ready at that moment ).
> >  - drop inclusion of <asm/csr.h> and <asm/early_printk.h>
> >  - add <asm/riscv_encoding.h> for CAUSE_* in decode_trap_cause().
> > ---
> > Changes in V8:
> >   - fix typo in return string from decode_reserved_interrupt_cause
> >   - add Acked-by: Alistair Francis <alistair.francis@xxxxxxx>
> > ---
> > Changes in V7:
> >  - Nothing changed. Only rebase.
> > ---
> > Changes in V6:
> >  - Remove usage of LINK_TO_LOAD() due to the MMU being enabled
> > first.
> >  - Change early_printk() to printk()
> > ---
> > 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 | 80
> > +++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 79 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
> > index 5415cf8d90..37cec40dfa 100644
> > --- a/xen/arch/riscv/traps.c
> > +++ b/xen/arch/riscv/traps.c
> > @@ -9,13 +9,91 @@
> >  #include <xen/sched.h>
> >  
> >  #include <asm/processor.h>
> > +#include <asm/riscv_encoding.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";
> > +}
> 
> While I committed this as-is, two more points: First, as soon any any
> such array access becomes potentially reachable because of guest
> activity, it wants to use array_access_nospec() or alike.
Could you please explain why ( specifically ) guest activity affects if
array access becomes potentially reachable? 

>  Second, and
> somewhat related, it may be desirable to avoid such double accesses.
> Hopefully the compiler will do that for you. But
> 
>     const char *res = cause < ARRAY_SIZE(trap_causes) ?
> trap_causes[cause]
>                                                       : NULL;
> 
>     return res ?: "UNKNOWN";
> 
> might be worthwhile anyway.

Thanks.
I will apply this suggestion in the next version of this patch series.



~ Oleksii