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

Oleksii Kurochko posted 7 patches 1 year, 2 months ago
There is a newer version of this series
[PATCH v5 4/7] xen/riscv: introduce decode_cause() stuff
Posted by Oleksii Kurochko 1 year, 2 months ago
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)));
+    early_printk("\n");
+
     die();
 }
+
+void do_trap(struct cpu_user_regs *cpu_regs)
+{
+    do_unexpected_trap(cpu_regs);
+}
-- 
2.39.2
Re: [PATCH v5 4/7] xen/riscv: introduce decode_cause() stuff
Posted by Julien Grall 1 year, 2 months ago
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
Re: [PATCH v5 4/7] xen/riscv: introduce decode_cause() stuff
Posted by Oleksii 1 year, 2 months ago
Hi Julien,

On Tue, 2023-03-21 at 17:33 +0000, Julien Grall wrote:
> 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?
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.
> 
> 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().
It might be an issue. I'll double check what will be returned from
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.
It make sense.

Thanks a lot for the comments.

~ Oleksii
Re: [PATCH v5 4/7] xen/riscv: introduce decode_cause() stuff
Posted by Jan Beulich 1 year, 2 months ago
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

Re: [PATCH v5 4/7] xen/riscv: introduce decode_cause() stuff
Posted by Oleksii 1 year, 2 months ago
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
Re: [PATCH v5 4/7] xen/riscv: introduce decode_cause() stuff
Posted by Jan Beulich 1 year, 2 months ago
On 22.03.2023 14:32, Oleksii wrote:
> On Wed, 2023-03-22 at 13:26 +0100, Jan Beulich wrote:
>> 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().

I'm not convinced. You can't stick to using earlyprintk only beyond
very short term. Yet I expect you also don't want to use

    if ( early )
        earlyprintk();
    else
        printk();

everywhere in exception handling code (and anywhere else in code
which is reachable before the MMU is turned on). This extends to
uses of BUG() and alike in such early code, which - when they
trigger - want to use printk() (only). Whether printk(), somehow,
involves an array access similar to the ones you're presently
aware of you simply shouldn't depend on (it's an implementation
detail in a separate subsystem).

> 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?

By "common code" I mean code outside of arch/riscv/. And I
sincerely hope you / we can get away without duplicated functions.

Jan
Re: [PATCH v5 4/7] xen/riscv: introduce decode_cause() stuff
Posted by Oleksii 1 year, 2 months ago
On Wed, 2023-03-22 at 14:46 +0100, Jan Beulich wrote:
> On 22.03.2023 14:32, Oleksii wrote:
> > On Wed, 2023-03-22 at 13:26 +0100, Jan Beulich wrote:
> > > 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().
> 
> I'm not convinced. You can't stick to using earlyprintk only beyond
> very short term. Yet I expect you also don't want to use
> 
>     if ( early )
>         earlyprintk();
>     else
>         printk();
> 
> everywhere in exception handling code (and anywhere else in code
> which is reachable before the MMU is turned on). This extends to
> uses of BUG() and alike in such early code, which - when they
> trigger - want to use printk() (only). Whether printk(), somehow,
> involves an array access similar to the ones you're presently
> aware of you simply shouldn't depend on (it's an implementation
> detail in a separate subsystem).
I planned to changed all earlyprintk() to printk() in traps.c after
printk will be ready.

I would like to remind that xen/common code isn't compiled now for
RISC-V.
> 
> > 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?
> 
> By "common code" I mean code outside of arch/riscv/. And I
> sincerely hope you / we can get away without duplicated functions.
Got it.

Than it might be an issue with do_bug_frame() as RISCV should be
switched to it at the end.

Then it looks like enabling of MMU should go first but in that case
this not clear what to do with BUG(), WARN() etc as for them it is
needed excpetion handling functionality and MMU related code uses the
macros.

~ Oleksii
Re: [PATCH v5 4/7] xen/riscv: introduce decode_cause() stuff
Posted by Jan Beulich 1 year, 2 months ago
On 22.03.2023 15:59, Oleksii wrote:
> Then it looks like enabling of MMU should go first but in that case
> this not clear what to do with BUG(), WARN() etc as for them it is
> needed excpetion handling functionality and MMU related code uses the
> macros.

It's still possible to reconsider and do the page table building
and MMU enabling before entering C. Or to have all that in a separate
source/object file, which is prohibited to use about all of the
infrastructure. And of course the option of (re-)applying relocations
also continues to exist.

Jan