[PATCH v4 0/5] RISCV basic exception handling implementation

[PATCH v4 0/5] RISCV basic exception handling implementation

[Oleksii Kurochko]

The patch series is based on [introduce generic implementation
of macros from bug.h] which hasn't been commited yet.

The patch series provides a basic implementation of exception handling.
It can do only basic things such as decode a cause of an exception,
save/restore registers and execute "wfi" instruction if an exception
can not be handled.

To verify that exception handling works well it was implemented macros
from <asm/bug.h> such as BUG/WARN/run_in_exception/assert_failed.
The implementation of macros is used "ebreak" instruction and set up bug
frame tables for each type of macros.
Also it was implemented register save/restore to return and continue work
after WARN/run_in_exception.
Not all functionality of the macros was implemented as some of them
require hard-panic the system which is not available now. Instead of
hard-panic 'wfi' instruction is used but it should be definitely changed
in the neareset future.
It wasn't implemented show_execution_state() and stack trace discovering
as it's not necessary now.

---
Changes in V4:
  - Rebase the patch series on top of new version of [introduce generic
    implementation of macros from bug.h] patch series.
  - Update the cover letter message as 'Early printk' was merged and
    the current one patch series is based only on [introduce generic
    implementation of macros from bug.h] which hasn't been commited yet.
  - The following patches of the patch series were merged to staging:
      [PATCH v3 01/14] xen/riscv: change ISA to r64G
      [PATCH v3 02/14] xen/riscv: add <asm/asm.h> header
      [PATCH v3 03/14] xen/riscv: add <asm/riscv_encoding.h header
      [PATCH v3 04/14] xen/riscv: add <asm/csr.h> header
      [PATCH v3 05/14] xen/riscv: introduce empty <asm/string.h>
      [PATCH v3 06/14] xen/riscv: introduce empty <asm/cache.h>
      [PATCH v3 07/14] xen/riscv: introduce exception context
      [PATCH v3 08/14] xen/riscv: introduce exception handlers implementation
      [PATCH v3 10/14] xen/riscv: mask all interrupts
  - Fix addressed comments in xen-devel mailing list.

---
Changes in V3:
  - Change the name of config RISCV_ISA_RV64IMA to RISCV_ISA_RV64G
    as instructions from Zicsr and Zifencei extensions aren't part of
    I extension any more.
  - Rebase the patch "xen/riscv: introduce an implementation of macros
    from <asm/bug.h>" on top of patch series [introduce generic implementation
    of macros from bug.h]
  - Update commit messages
---
Changes in V2:
  - take the latest riscv_encoding.h from OpenSBI, update it with Xen
    related changes, and update the commit message with "Origin:"
    tag and the commit message itself.
  - add "Origin:" tag to the commit messag of the patch
    [xen/riscv: add <asm/csr.h> header].
  - Remove the patch [xen/riscv: add early_printk_hnum() function] as the
    functionality provided by the patch isn't used now.
  - Refactor prcoess.h: move structure offset defines to asm-offsets.c,
    change register_t to unsigned long.
  - Refactor entry.S to use offsets defined in asm-offsets.C
  - Rename {__,}handle_exception to handle_trap() and do_trap() to be more
    consistent with RISC-V spec.
  - Merge the pathc which introduces do_unexpected_trap() with the patch
    [xen/riscv: introduce exception handlers implementation].
  - Rename setup_trap_handler() to trap_init() and update correspondingly
    the patches in the patch series.
  - Refactor bug.h, remove bug_instr_t type from it.
  - Refactor decode_trap_cause() function to be more optimization-friendly.
  - Add two new empty headers: <cache.h> and <string.h> as they are needed to
    include <xen/lib.h> which provides ARRAY_SIZE and other macros.
  - Code style fixes.
---

Oleksii Kurochko (5):
  xen/riscv: introduce decode_cause() stuff
  xen/riscv: introduce trap_init()
  xen/riscv: introduce an implementation of macros from <asm/bug.h>
  xen/riscv: test basic handling stuff
  automation: modify RISC-V smoke test

 automation/scripts/qemu-smoke-riscv64.sh |   2 +-
 xen/arch/riscv/include/asm/bug.h         |  48 +++++
 xen/arch/riscv/include/asm/processor.h   |   2 +
 xen/arch/riscv/include/asm/traps.h       |   1 +
 xen/arch/riscv/setup.c                   |  21 +++
 xen/arch/riscv/traps.c                   | 219 ++++++++++++++++++++++-
 xen/arch/riscv/xen.lds.S                 |  10 ++
 7 files changed, 301 insertions(+), 2 deletions(-)
 create mode 100644 xen/arch/riscv/include/asm/bug.h

-- 
2.39.0

[PATCH v4 1/5] xen/riscv: introduce decode_cause() stuff

[Oleksii Kurochko]

The patch introduces stuff needed to decode a reason of an
exception.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
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..29b1a0dfae 100644
--- a/xen/arch/riscv/traps.c
+++ b/xen/arch/riscv/traps.c
@@ -4,10 +4,95 @@
  *
  * RISC-V Trap handlers
  */
+
+#include <xen/errno.h>
+#include <xen/lib.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";
+}
+
+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";
+    }
+}
+
+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);
+    }
+}
+
+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(decode_cause(cause));
+    early_printk("\n");
+
     die();
 }
+
+void do_trap(struct cpu_user_regs *cpu_regs)
+{
+    do_unexpected_trap(cpu_regs);
+}
-- 
2.39.0

[PATCH v4 2/5] xen/riscv: introduce trap_init()

[Oleksii Kurochko]

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
Changes in V4:
  - Nothing changed
---
Changes in V3:
  - Nothing changed
---
Changes in V2:
  - Rename setup_trap_handler() to trap_init().
  - Add Reviewed-by to the commit message.
---
 xen/arch/riscv/include/asm/traps.h | 1 +
 xen/arch/riscv/setup.c             | 4 ++++
 xen/arch/riscv/traps.c             | 7 +++++++
 3 files changed, 12 insertions(+)

diff --git a/xen/arch/riscv/include/asm/traps.h b/xen/arch/riscv/include/asm/traps.h
index f3fb6b25d1..f1879294ef 100644
--- a/xen/arch/riscv/include/asm/traps.h
+++ b/xen/arch/riscv/include/asm/traps.h
@@ -7,6 +7,7 @@
 
 void do_trap(struct cpu_user_regs *cpu_regs);
 void handle_trap(void);
+void trap_init(void);
 
 #endif /* __ASSEMBLY__ */
 
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index d09ffe1454..c8513ca4f8 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -1,7 +1,9 @@
 #include <xen/compile.h>
 #include <xen/init.h>
 
+#include <asm/csr.h>
 #include <asm/early_printk.h>
+#include <asm/traps.h>
 
 /* Xen stack for bringing up the first CPU. */
 unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
@@ -11,6 +13,8 @@ void __init noreturn start_xen(void)
 {
     early_printk("Hello from C env\n");
 
+    trap_init();
+
     for ( ;; )
         asm volatile ("wfi");
 
diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
index 29b1a0dfae..ad7311f269 100644
--- a/xen/arch/riscv/traps.c
+++ b/xen/arch/riscv/traps.c
@@ -13,6 +13,13 @@
 #include <asm/processor.h>
 #include <asm/traps.h>
 
+void trap_init(void)
+{
+    unsigned long addr = (unsigned long)&handle_trap;
+
+    csr_write(CSR_STVEC, addr);
+}
+
 static const char *decode_trap_cause(unsigned long cause)
 {
     static const char *const trap_causes[] = {
-- 
2.39.0

[PATCH v4 3/5] xen/riscv: introduce an implementation of macros from <asm/bug.h>

[Oleksii Kurochko]

The patch introduces macros: BUG(), WARN(), run_in_exception(),
assert_failed.

The implementation uses "ebreak" instruction in combination with
diffrent bug frame tables (for each type) which contains useful
information.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V4:
  - Updates in RISC-V's <asm/bug.h>:
    * Add explanatory comment about why there is only defined for 32-bits length
      instructions and 16/32-bits BUG_INSN_{16,32}.
    * Change 'unsigned long' to 'unsigned int' inside GET_INSN_LENGTH().
    * Update declaration of is_valid_bugaddr(): switch return type from int to bool
      and the argument from 'unsigned int' to 'vaddr'.
  - Updates in RISC-V's traps.c:
    * replace /xen and /asm includes 
    * update definition of is_valid_bugaddr():switch return type from int to bool
      and the argument from 'unsigned int' to 'vaddr'. Code style inside function
      was updated too.
    * do_bug_frame() refactoring:
      * local variables start and bug became 'const struct bug_frame'
      * bug_frames[] array became 'static const struct bug_frame[] = ...'
      * remove all casts
      * remove unneeded comments and add an explanatory comment that the do_bug_frame()
        will be switched to a generic one.
    * do_trap() refactoring:
      * read 16-bits value instead of 32-bits as compressed instruction can
        be used and it might happen than only 16-bits may be accessible.
      * code style updates
      * re-use instr variable instead of re-reading instruction.
  - Updates in setup.c:
    * add blank line between xen/ and asm/ includes.
---
Changes in V3:
  - Rebase the patch "xen/riscv: introduce an implementation of macros
    from <asm/bug.h>" on top of patch series [introduce generic implementation
    of macros from bug.h]
---
Changes in V2:
  - Remove __ in define namings
  - Update run_in_exception_handler() with
    register void *fn_ asm(__stringify(BUG_FN_REG)) = (fn);
  - Remove bug_instr_t type and change it's usage to uint32_t
---
 xen/arch/riscv/include/asm/bug.h       |  48 ++++++++++
 xen/arch/riscv/include/asm/processor.h |   2 +
 xen/arch/riscv/setup.c                 |   1 +
 xen/arch/riscv/traps.c                 | 125 +++++++++++++++++++++++++
 xen/arch/riscv/xen.lds.S               |  10 ++
 5 files changed, 186 insertions(+)
 create mode 100644 xen/arch/riscv/include/asm/bug.h

diff --git a/xen/arch/riscv/include/asm/bug.h b/xen/arch/riscv/include/asm/bug.h
new file mode 100644
index 0000000000..67ade6f895
--- /dev/null
+++ b/xen/arch/riscv/include/asm/bug.h
@@ -0,0 +1,48 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2012 Regents of the University of California
+ * Copyright (C) 2021-2023 Vates
+ *
+ */
+#ifndef _ASM_RISCV_BUG_H
+#define _ASM_RISCV_BUG_H
+
+#include <xen/types.h>
+
+#ifndef __ASSEMBLY__
+
+#define BUG_INSTR "ebreak"
+
+/*
+ * The base instruction set has a fixed length of 32-bit naturally aligned
+ * instructions.
+ *
+ * There are extensions of variable length ( where each instruction can be
+ * any number of 16-bit parcels in length ) but they aren't used in Xen
+ * and Linux kernel ( where these definitions were taken from ).
+ *
+ * Compressed ISA is used now where the instruction length is 16 bit  and
+ * 'ebreak' instruction, in this case, can be either 16 or 32 bit (
+ * depending on if compressed ISA is used or not )
+ */
+#define INSN_LENGTH_MASK        _UL(0x3)
+#define INSN_LENGTH_32          _UL(0x3)
+
+#define BUG_INSN_32             _UL(0x00100073) /* ebreak */
+#define BUG_INSN_16             _UL(0x9002)     /* c.ebreak */
+#define COMPRESSED_INSN_MASK    _UL(0xffff)
+
+#define GET_INSN_LENGTH(insn)                               \
+({                                                          \
+    unsigned int len;                                       \
+    len = ((insn & INSN_LENGTH_MASK) == INSN_LENGTH_32) ?   \
+        4UL : 2UL;                                          \
+    len;                                                    \
+})
+
+/* These are defined by the architecture */
+bool is_valid_bugaddr(vaddr_t addr);
+
+#endif /* !__ASSEMBLY__ */
+
+#endif /* _ASM_RISCV_BUG_H */
diff --git a/xen/arch/riscv/include/asm/processor.h b/xen/arch/riscv/include/asm/processor.h
index a71448e02e..ef23d9589e 100644
--- a/xen/arch/riscv/include/asm/processor.h
+++ b/xen/arch/riscv/include/asm/processor.h
@@ -69,6 +69,8 @@ static inline void die(void)
         wfi();
 }
 
+void show_execution_state(const struct cpu_user_regs *regs);
+
 #endif /* __ASSEMBLY__ */
 
 #endif /* _ASM_RISCV_PROCESSOR_H */
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index c8513ca4f8..73b9a82883 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -1,3 +1,4 @@
+#include <xen/bug.h>
 #include <xen/compile.h>
 #include <xen/init.h>
 
diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
index ad7311f269..d87a9cfd2c 100644
--- a/xen/arch/riscv/traps.c
+++ b/xen/arch/riscv/traps.c
@@ -5,6 +5,7 @@
  * RISC-V Trap handlers
  */
 
+#include <xen/bug.h>
 #include <xen/errno.h>
 #include <xen/lib.h>
 
@@ -99,7 +100,131 @@ static void do_unexpected_trap(const struct cpu_user_regs *regs)
     die();
 }
 
+void show_execution_state(const struct cpu_user_regs *regs)
+{
+    early_printk("implement show_execution_state(regs)\n");
+}
+
+/*
+ * TODO: change early_printk's function to early_printk with format
+ *       when s(n)printf() will be added.
+ *
+ * Probably the TODO won't be needed as generic do_bug_frame() (at
+ * least, for ARM and RISC-V) has been introduced and current
+ * implementation will be replaced with generic one when panic(),
+ * printk() and find_text_region() (virtual memory?) will be
+ * ready/merged
+ */
+int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
+{
+    const struct bug_frame *start, *end;
+    const struct bug_frame *bug = NULL;
+    unsigned int id = 0;
+    const char *filename, *predicate;
+    int lineno;
+
+    static const struct bug_frame* bug_frames[] = {
+        &__start_bug_frames[0],
+        &__stop_bug_frames_0[0],
+        &__stop_bug_frames_1[0],
+        &__stop_bug_frames_2[0],
+        &__stop_bug_frames_3[0],
+    };
+
+    for ( id = 0; id < BUGFRAME_NR; id++ )
+    {
+        start = bug_frames[id];
+        end   = bug_frames[id + 1];
+
+        while ( start != end )
+        {
+            if ( (vaddr_t)bug_loc(start) == pc )
+            {
+                bug = start;
+                goto found;
+            }
+
+            start++;
+        }
+    }
+
+ found:
+    if ( bug == NULL )
+        return -ENOENT;
+
+    if ( id == BUGFRAME_run_fn )
+    {
+        void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug);
+
+        fn(regs);
+
+        goto end;
+    }
+
+    /* WARN, BUG or ASSERT: decode the filename pointer and line number. */
+    filename = bug_ptr(bug);
+    lineno = bug_line(bug);
+
+    switch ( id )
+    {
+    case BUGFRAME_warn:
+        early_printk("Xen WARN at ");
+        early_printk(filename);
+        early_printk("\n");
+
+        show_execution_state(regs);
+
+        goto end;
+
+    case BUGFRAME_bug:
+        early_printk("Xen BUG at ");
+        early_printk(filename);
+        early_printk("\n");
+
+        show_execution_state(regs);
+        early_printk("change wait_for_interrupt to panic() when common is available\n");
+        die();
+
+    case BUGFRAME_assert:
+        /* ASSERT: decode the predicate string pointer. */
+        predicate = bug_msg(bug);
+
+        early_printk("Assertion \'");
+        early_printk(predicate);
+        early_printk("\' failed at ");
+        early_printk(filename);
+        early_printk("\n");
+
+        show_execution_state(regs);
+        early_printk("change wait_for_interrupt to panic() when common is available\n");
+        die();
+    }
+
+    return -EINVAL;
+
+ end:
+    return 0;
+}
+
+bool is_valid_bugaddr(vaddr_t insn)
+{
+    if ( (insn & INSN_LENGTH_MASK) == INSN_LENGTH_32 )
+        return ( insn == BUG_INSN_32 );
+    else
+        return ( (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16 );
+}
+
 void do_trap(struct cpu_user_regs *cpu_regs)
 {
+    register_t pc = cpu_regs->sepc;
+    uint16_t instr = *(uint16_t *)pc;
+
+    if ( is_valid_bugaddr(instr) ) {
+        if ( !do_bug_frame(cpu_regs, cpu_regs->sepc) ) {
+            cpu_regs->sepc += GET_INSN_LENGTH(instr);
+            return;
+        }
+    }
+
     do_unexpected_trap(cpu_regs);
 }
diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S
index ca57cce75c..139e2d04cb 100644
--- a/xen/arch/riscv/xen.lds.S
+++ b/xen/arch/riscv/xen.lds.S
@@ -39,6 +39,16 @@ SECTIONS
     . = ALIGN(PAGE_SIZE);
     .rodata : {
         _srodata = .;          /* Read-only data */
+        /* Bug frames table */
+       __start_bug_frames = .;
+       *(.bug_frames.0)
+       __stop_bug_frames_0 = .;
+       *(.bug_frames.1)
+       __stop_bug_frames_1 = .;
+       *(.bug_frames.2)
+       __stop_bug_frames_2 = .;
+       *(.bug_frames.3)
+       __stop_bug_frames_3 = .;
         *(.rodata)
         *(.rodata.*)
         *(.data.rel.ro)
-- 
2.39.0

[PATCH v4 4/5] xen/riscv: test basic handling stuff

[Oleksii Kurochko]

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Acked-by: Alistair Francis <alistair.francis@wdc.com>
---
Changes in V4:
  - Add Acked-by: Alistair Francis <alistair.francis@wdc.com>
---
Changes in V3:
  - Nothing changed
---
Changes in V2:
  - Nothing changed
---
 xen/arch/riscv/setup.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index 73b9a82883..b3f8b10f71 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -10,12 +10,28 @@
 unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
     __aligned(STACK_SIZE);
 
+static void test_run_in_exception(struct cpu_user_regs *regs)
+{
+    early_printk("If you see this message, ");
+    early_printk("run_in_exception_handler is most likely working\n");
+}
+
+static void test_macros_from_bug_h(void)
+{
+    run_in_exception_handler(test_run_in_exception);
+    WARN();
+    early_printk("If you see this message, ");
+    early_printk("WARN is most likely working\n");
+}
+
 void __init noreturn start_xen(void)
 {
     early_printk("Hello from C env\n");
 
     trap_init();
 
+    test_macros_from_bug_h();
+
     for ( ;; )
         asm volatile ("wfi");
 
-- 
2.39.0

[PATCH v4 5/5] automation: modify RISC-V smoke test

[Oleksii Kurochko]

The patch modifies the grep pattern to reflect the usage of WARN.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
Changes in V4:
 - Add Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
   and Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
Changes in V3:
 - Update commit message
---
Changes in V2:
 - Leave only the latest "grep ..."
---
 automation/scripts/qemu-smoke-riscv64.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/automation/scripts/qemu-smoke-riscv64.sh b/automation/scripts/qemu-smoke-riscv64.sh
index e0f06360bc..02fc66be03 100755
--- a/automation/scripts/qemu-smoke-riscv64.sh
+++ b/automation/scripts/qemu-smoke-riscv64.sh
@@ -16,5 +16,5 @@ qemu-system-riscv64 \
     |& tee smoke.serial
 
 set -e
-(grep -q "Hello from C env" smoke.serial) || exit 1
+(grep -q "WARN is most likely working" smoke.serial) || exit 1
 exit 0
-- 
2.39.0

Re: [PATCH v4 1/5] xen/riscv: introduce decode_cause() stuff

[Jan Beulich]

On 24.02.2023 12:35, 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/errno.h>

I couldn't spot anything in the file which would require this inclusion.

> +#include <xen/lib.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";
> +}
> +
> +const char *decode_reserved_interrupt_cause(unsigned long irq_cause)

For any non-static function that you add you will need a declaration
in a header, which the defining C file then includes. I understand
that during initial bringup functions without (external) callers may
want to (temporarily) exist, but briefly clarifying what the future
expectation regarding external uses might help. Any function that's
not expected to gain external callers should be static.

Jan

Re: [PATCH v4 2/5] xen/riscv: introduce trap_init()

[Jan Beulich]

On 24.02.2023 12:35, Oleksii Kurochko wrote:
> @@ -11,6 +13,8 @@ void __init noreturn start_xen(void)
>  {
>      early_printk("Hello from C env\n");
>  
> +    trap_init();
> +
>      for ( ;; )
>          asm volatile ("wfi");

Along the lines of what Andrew has said there - it's hard to see
how this can come (both code flow and patch ordering wise) ahead
of clearing .bss.

Jan

Re: [PATCH v4 3/5] xen/riscv: introduce an implementation of macros from <asm/bug.h>

[Jan Beulich]

On 24.02.2023 12:35, Oleksii Kurochko wrote:
> The patch introduces macros: BUG(), WARN(), run_in_exception(),
> assert_failed.
> 
> The implementation uses "ebreak" instruction in combination with
> diffrent bug frame tables (for each type) which contains useful
> information.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> Changes in V4:
>   - Updates in RISC-V's <asm/bug.h>:
>     * Add explanatory comment about why there is only defined for 32-bits length
>       instructions and 16/32-bits BUG_INSN_{16,32}.
>     * Change 'unsigned long' to 'unsigned int' inside GET_INSN_LENGTH().
>     * Update declaration of is_valid_bugaddr(): switch return type from int to bool
>       and the argument from 'unsigned int' to 'vaddr'.
>   - Updates in RISC-V's traps.c:
>     * replace /xen and /asm includes 
>     * update definition of is_valid_bugaddr():switch return type from int to bool
>       and the argument from 'unsigned int' to 'vaddr'. Code style inside function
>       was updated too.
>     * do_bug_frame() refactoring:
>       * local variables start and bug became 'const struct bug_frame'
>       * bug_frames[] array became 'static const struct bug_frame[] = ...'
>       * remove all casts
>       * remove unneeded comments and add an explanatory comment that the do_bug_frame()
>         will be switched to a generic one.
>     * do_trap() refactoring:
>       * read 16-bits value instead of 32-bits as compressed instruction can
>         be used and it might happen than only 16-bits may be accessible.
>       * code style updates
>       * re-use instr variable instead of re-reading instruction.
>   - Updates in setup.c:
>     * add blank line between xen/ and asm/ includes.
> ---
> Changes in V3:
>   - Rebase the patch "xen/riscv: introduce an implementation of macros
>     from <asm/bug.h>" on top of patch series [introduce generic implementation
>     of macros from bug.h]
> ---
> Changes in V2:
>   - Remove __ in define namings
>   - Update run_in_exception_handler() with
>     register void *fn_ asm(__stringify(BUG_FN_REG)) = (fn);
>   - Remove bug_instr_t type and change it's usage to uint32_t
> ---
>  xen/arch/riscv/include/asm/bug.h       |  48 ++++++++++
>  xen/arch/riscv/include/asm/processor.h |   2 +
>  xen/arch/riscv/setup.c                 |   1 +
>  xen/arch/riscv/traps.c                 | 125 +++++++++++++++++++++++++
>  xen/arch/riscv/xen.lds.S               |  10 ++
>  5 files changed, 186 insertions(+)
>  create mode 100644 xen/arch/riscv/include/asm/bug.h
> 
> diff --git a/xen/arch/riscv/include/asm/bug.h b/xen/arch/riscv/include/asm/bug.h
> new file mode 100644
> index 0000000000..67ade6f895
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/bug.h
> @@ -0,0 +1,48 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2012 Regents of the University of California
> + * Copyright (C) 2021-2023 Vates
> + *
> + */
> +#ifndef _ASM_RISCV_BUG_H
> +#define _ASM_RISCV_BUG_H
> +
> +#include <xen/types.h>
> +
> +#ifndef __ASSEMBLY__

This #ifndef contradicts the xen/types.h inclusion. Either the #include
moves down below here, or the #ifndef goes away as pointless. This is
because xen/types.h cannot be include in assembly files.

> +#define BUG_INSTR "ebreak"
> +
> +/*
> + * The base instruction set has a fixed length of 32-bit naturally aligned
> + * instructions.
> + *
> + * There are extensions of variable length ( where each instruction can be
> + * any number of 16-bit parcels in length ) but they aren't used in Xen
> + * and Linux kernel ( where these definitions were taken from ).
> + *
> + * Compressed ISA is used now where the instruction length is 16 bit  and
> + * 'ebreak' instruction, in this case, can be either 16 or 32 bit (
> + * depending on if compressed ISA is used or not )
> + */
> +#define INSN_LENGTH_MASK        _UL(0x3)
> +#define INSN_LENGTH_32          _UL(0x3)
> +
> +#define BUG_INSN_32             _UL(0x00100073) /* ebreak */
> +#define BUG_INSN_16             _UL(0x9002)     /* c.ebreak */
> +#define COMPRESSED_INSN_MASK    _UL(0xffff)
> +
> +#define GET_INSN_LENGTH(insn)                               \
> +({                                                          \
> +    unsigned int len;                                       \
> +    len = ((insn & INSN_LENGTH_MASK) == INSN_LENGTH_32) ?   \
> +        4UL : 2UL;                                          \

Why the UL suffixes?

> +    len;                                                    \
> +})

And anyway - can't the whole macro be written without using any
extension:

#define GET_INSN_LENGTH(insn)                               \
    (((insn) & INSN_LENGTH_MASK) == INSN_LENGTH_32 ? 4 : 2) \

? (Note also that uses of macro parameters should be properly
parenthesized.)

> --- a/xen/arch/riscv/setup.c
> +++ b/xen/arch/riscv/setup.c
> @@ -1,3 +1,4 @@
> +#include <xen/bug.h>
>  #include <xen/compile.h>
>  #include <xen/init.h>

Why? Eventually this will be needed, I agree, but right now?

> @@ -99,7 +100,131 @@ static void do_unexpected_trap(const struct cpu_user_regs *regs)
>      die();
>  }
>  
> +void show_execution_state(const struct cpu_user_regs *regs)
> +{
> +    early_printk("implement show_execution_state(regs)\n");
> +}
> +
> +/*
> + * TODO: change early_printk's function to early_printk with format
> + *       when s(n)printf() will be added.
> + *
> + * Probably the TODO won't be needed as generic do_bug_frame() (at
> + * least, for ARM and RISC-V) has been introduced and current
> + * implementation will be replaced with generic one when panic(),
> + * printk() and find_text_region() (virtual memory?) will be
> + * ready/merged
> + */
> +int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
> +{
> +    const struct bug_frame *start, *end;
> +    const struct bug_frame *bug = NULL;
> +    unsigned int id = 0;
> +    const char *filename, *predicate;
> +    int lineno;
> +
> +    static const struct bug_frame* bug_frames[] = {
> +        &__start_bug_frames[0],
> +        &__stop_bug_frames_0[0],
> +        &__stop_bug_frames_1[0],
> +        &__stop_bug_frames_2[0],
> +        &__stop_bug_frames_3[0],
> +    };
> +
> +    for ( id = 0; id < BUGFRAME_NR; id++ )
> +    {
> +        start = bug_frames[id];
> +        end   = bug_frames[id + 1];
> +
> +        while ( start != end )
> +        {
> +            if ( (vaddr_t)bug_loc(start) == pc )
> +            {
> +                bug = start;
> +                goto found;
> +            }
> +
> +            start++;
> +        }
> +    }
> +
> + found:
> +    if ( bug == NULL )
> +        return -ENOENT;
> +
> +    if ( id == BUGFRAME_run_fn )
> +    {
> +        void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug);
> +
> +        fn(regs);
> +
> +        goto end;
> +    }
> +
> +    /* WARN, BUG or ASSERT: decode the filename pointer and line number. */
> +    filename = bug_ptr(bug);
> +    lineno = bug_line(bug);
> +
> +    switch ( id )
> +    {
> +    case BUGFRAME_warn:
> +        early_printk("Xen WARN at ");
> +        early_printk(filename);
> +        early_printk("\n");
> +
> +        show_execution_state(regs);
> +
> +        goto end;
> +
> +    case BUGFRAME_bug:
> +        early_printk("Xen BUG at ");
> +        early_printk(filename);
> +        early_printk("\n");
> +
> +        show_execution_state(regs);
> +        early_printk("change wait_for_interrupt to panic() when common is available\n");
> +        die();
> +
> +    case BUGFRAME_assert:
> +        /* ASSERT: decode the predicate string pointer. */
> +        predicate = bug_msg(bug);
> +
> +        early_printk("Assertion \'");
> +        early_printk(predicate);
> +        early_printk("\' failed at ");
> +        early_printk(filename);
> +        early_printk("\n");
> +
> +        show_execution_state(regs);
> +        early_printk("change wait_for_interrupt to panic() when common is available\n");
> +        die();
> +    }
> +
> +    return -EINVAL;
> +
> + end:
> +    return 0;
> +}

Wasn't the goal to use the generic do_bug_frame()?

> +bool is_valid_bugaddr(vaddr_t insn)

Why is insn of type vaddr_t?

> +{
> +    if ( (insn & INSN_LENGTH_MASK) == INSN_LENGTH_32 )
> +        return ( insn == BUG_INSN_32 );
> +    else
> +        return ( (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16 );

Nit (style): The kind-of-operand to return is an expression. Hence
you have stray blanks there immediately inside the parentheses.
(This is unlike e.g. if(), where you've formatted things correctly.)

> +}
> +
>  void do_trap(struct cpu_user_regs *cpu_regs)
>  {
> +    register_t pc = cpu_regs->sepc;
> +    uint16_t instr = *(uint16_t *)pc;
> +
> +    if ( is_valid_bugaddr(instr) ) {
> +        if ( !do_bug_frame(cpu_regs, cpu_regs->sepc) ) {

Nit: Brace placement (twice).

Jan

Re: [PATCH v4 1/5] xen/riscv: introduce decode_cause() stuff

[Oleksii]

On Mon, 2023-02-27 at 13:48 +0100, Jan Beulich wrote:
> On 24.02.2023 12:35, 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/errno.h>
> 
> I couldn't spot anything in the file which would require this
> inclusion.
Sure. It can be removed.
> 
> > +#include <xen/lib.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";
> > +}
> > +
> > +const char *decode_reserved_interrupt_cause(unsigned long
> > irq_cause)
> 
> For any non-static function that you add you will need a declaration
> in a header, which the defining C file then includes. I understand
> that during initial bringup functions without (external) callers may
> want to (temporarily) exist, but briefly clarifying what the future
> expectation regarding external uses might help. Any function that's
> not expected to gain external callers should be static.
Thanks. For explanation. I'll make them static.

~ Oleksii

Re: [PATCH v4 1/5] xen/riscv: introduce decode_cause() stuff

[Oleksii]

On Mon, 2023-02-27 at 13:48 +0100, Jan Beulich wrote:
> On 24.02.2023 12:35, 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/errno.h>
> 
> I couldn't spot anything in the file which would require this
> inclusion.
-EINVAL, -ENOENT are used in do_bug_frame() I missed that when wrote
previous answer on e-mail.

Re: [PATCH v4 1/5] xen/riscv: introduce decode_cause() stuff

[Oleksii]

On Wed, 2023-03-01 at 11:41 +0200, Oleksii wrote:
> On Mon, 2023-02-27 at 13:48 +0100, Jan Beulich wrote:
> > On 24.02.2023 12:35, 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/errno.h>
> > 
> > I couldn't spot anything in the file which would require this
> > inclusion.
> -EINVAL, -ENOENT are used in do_bug_frame() I missed that when wrote
> previous answer on e-mail.
Sorry for confusion. I resolved merge conflicts after rebase so
<xen/errno.h> should be remove as anything is used in traps.c from it

~ Oleksii

Re: [PATCH v4 2/5] xen/riscv: introduce trap_init()

[Oleksii]

On Mon, 2023-02-27 at 13:50 +0100, Jan Beulich wrote:
> On 24.02.2023 12:35, Oleksii Kurochko wrote:
> > @@ -11,6 +13,8 @@ void __init noreturn start_xen(void)
> >  {
> >      early_printk("Hello from C env\n");
> >  
> > +    trap_init();
> > +
> >      for ( ;; )
> >          asm volatile ("wfi");
> 
> Along the lines of what Andrew has said there - it's hard to see
> how this can come (both code flow and patch ordering wise) ahead
> of clearing .bss.
So should I add the patch with initializatin of bss as a part of this
patch series?

~ Oleksii

Re: [PATCH v4 2/5] xen/riscv: introduce trap_init()

[Jan Beulich]

On 01.03.2023 11:34, Oleksii wrote:
> On Mon, 2023-02-27 at 13:50 +0100, Jan Beulich wrote:
>> On 24.02.2023 12:35, Oleksii Kurochko wrote:
>>> @@ -11,6 +13,8 @@ void __init noreturn start_xen(void)
>>>  {
>>>      early_printk("Hello from C env\n");
>>>  
>>> +    trap_init();
>>> +
>>>      for ( ;; )
>>>          asm volatile ("wfi");
>>
>> Along the lines of what Andrew has said there - it's hard to see
>> how this can come (both code flow and patch ordering wise) ahead
>> of clearing .bss.
> So should I add the patch with initializatin of bss as a part of this
> patch series?

That or make clear in the cover letter that there is a dependency.

Jan

Re: [PATCH v4 3/5] xen/riscv: introduce an implementation of macros from <asm/bug.h>

[Oleksii]

On Mon, 2023-02-27 at 13:59 +0100, Jan Beulich wrote:
> On 24.02.2023 12:35, Oleksii Kurochko wrote:
> > The patch introduces macros: BUG(), WARN(), run_in_exception(),
> > assert_failed.
> > 
> > The implementation uses "ebreak" instruction in combination with
> > diffrent bug frame tables (for each type) which contains useful
> > information.
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> > Changes in V4:
> >   - Updates in RISC-V's <asm/bug.h>:
> >     * Add explanatory comment about why there is only defined for
> > 32-bits length
> >       instructions and 16/32-bits BUG_INSN_{16,32}.
> >     * Change 'unsigned long' to 'unsigned int' inside
> > GET_INSN_LENGTH().
> >     * Update declaration of is_valid_bugaddr(): switch return type
> > from int to bool
> >       and the argument from 'unsigned int' to 'vaddr'.
> >   - Updates in RISC-V's traps.c:
> >     * replace /xen and /asm includes 
> >     * update definition of is_valid_bugaddr():switch return type
> > from int to bool
> >       and the argument from 'unsigned int' to 'vaddr'. Code style
> > inside function
> >       was updated too.
> >     * do_bug_frame() refactoring:
> >       * local variables start and bug became 'const struct
> > bug_frame'
> >       * bug_frames[] array became 'static const struct bug_frame[]
> > = ...'
> >       * remove all casts
> >       * remove unneeded comments and add an explanatory comment
> > that the do_bug_frame()
> >         will be switched to a generic one.
> >     * do_trap() refactoring:
> >       * read 16-bits value instead of 32-bits as compressed
> > instruction can
> >         be used and it might happen than only 16-bits may be
> > accessible.
> >       * code style updates
> >       * re-use instr variable instead of re-reading instruction.
> >   - Updates in setup.c:
> >     * add blank line between xen/ and asm/ includes.
> > ---
> > Changes in V3:
> >   - Rebase the patch "xen/riscv: introduce an implementation of
> > macros
> >     from <asm/bug.h>" on top of patch series [introduce generic
> > implementation
> >     of macros from bug.h]
> > ---
> > Changes in V2:
> >   - Remove __ in define namings
> >   - Update run_in_exception_handler() with
> >     register void *fn_ asm(__stringify(BUG_FN_REG)) = (fn);
> >   - Remove bug_instr_t type and change it's usage to uint32_t
> > ---
> >  xen/arch/riscv/include/asm/bug.h       |  48 ++++++++++
> >  xen/arch/riscv/include/asm/processor.h |   2 +
> >  xen/arch/riscv/setup.c                 |   1 +
> >  xen/arch/riscv/traps.c                 | 125
> > +++++++++++++++++++++++++
> >  xen/arch/riscv/xen.lds.S               |  10 ++
> >  5 files changed, 186 insertions(+)
> >  create mode 100644 xen/arch/riscv/include/asm/bug.h
> > 
> > diff --git a/xen/arch/riscv/include/asm/bug.h
> > b/xen/arch/riscv/include/asm/bug.h
> > new file mode 100644
> > index 0000000000..67ade6f895
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/bug.h
> > @@ -0,0 +1,48 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2012 Regents of the University of California
> > + * Copyright (C) 2021-2023 Vates
> > + *
> > + */
> > +#ifndef _ASM_RISCV_BUG_H
> > +#define _ASM_RISCV_BUG_H
> > +
> > +#include <xen/types.h>
> > +
> > +#ifndef __ASSEMBLY__
> 
> This #ifndef contradicts the xen/types.h inclusion. Either the
> #include
> moves down below here, or the #ifndef goes away as pointless. This is
> because xen/types.h cannot be include in assembly files.
I will remove it. It looks like it leave when as generic implementation
was taken ARM.
> 
> > +#define BUG_INSTR "ebreak"
> > +
> > +/*
> > + * The base instruction set has a fixed length of 32-bit naturally
> > aligned
> > + * instructions.
> > + *
> > + * There are extensions of variable length ( where each
> > instruction can be
> > + * any number of 16-bit parcels in length ) but they aren't used
> > in Xen
> > + * and Linux kernel ( where these definitions were taken from ).
> > + *
> > + * Compressed ISA is used now where the instruction length is 16
> > bit  and
> > + * 'ebreak' instruction, in this case, can be either 16 or 32 bit
> > (
> > + * depending on if compressed ISA is used or not )
> > + */
> > +#define INSN_LENGTH_MASK        _UL(0x3)
> > +#define INSN_LENGTH_32          _UL(0x3)
> > +
> > +#define BUG_INSN_32             _UL(0x00100073) /* ebreak */
> > +#define BUG_INSN_16             _UL(0x9002)     /* c.ebreak */
> > +#define COMPRESSED_INSN_MASK    _UL(0xffff)
> > +
> > +#define GET_INSN_LENGTH(insn)                               \
> > +({                                                          \
> > +    unsigned int len;                                       \
> > +    len = ((insn & INSN_LENGTH_MASK) == INSN_LENGTH_32) ?   \
> > +        4UL : 2UL;                                          \
> 
> Why the UL suffixes?
> 
> > +    len;                                                    \
> > +})
> 
> And anyway - can't the whole macro be written without using any
> extension:
> 
> #define GET_INSN_LENGTH(insn)                               \
>     (((insn) & INSN_LENGTH_MASK) == INSN_LENGTH_32 ? 4 : 2) \
> 
> ? (Note also that uses of macro parameters should be properly
> parenthesized.)
It can. I'll update the macros.
> 
> > --- a/xen/arch/riscv/setup.c
> > +++ b/xen/arch/riscv/setup.c
> > @@ -1,3 +1,4 @@
> > +#include <xen/bug.h>
> >  #include <xen/compile.h>
> >  #include <xen/init.h>
> 
> Why? Eventually this will be needed, I agree, but right now?
Agree. <xen/bug.h> should be removed fro the current patch.
Probably merge error during rebase.
> 
> > @@ -99,7 +100,131 @@ static void do_unexpected_trap(const struct
> > cpu_user_regs *regs)
> >      die();
> >  }
> >  
> > +void show_execution_state(const struct cpu_user_regs *regs)
> > +{
> > +    early_printk("implement show_execution_state(regs)\n");
> > +}
> > +
> > +/*
> > + * TODO: change early_printk's function to early_printk with
> > format
> > + *       when s(n)printf() will be added.
> > + *
> > + * Probably the TODO won't be needed as generic do_bug_frame() (at
> > + * least, for ARM and RISC-V) has been introduced and current
> > + * implementation will be replaced with generic one when panic(),
> > + * printk() and find_text_region() (virtual memory?) will be
> > + * ready/merged
> > + */
> > +int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
> > +{
> > +    const struct bug_frame *start, *end;
> > +    const struct bug_frame *bug = NULL;
> > +    unsigned int id = 0;
> > +    const char *filename, *predicate;
> > +    int lineno;
> > +
> > +    static const struct bug_frame* bug_frames[] = {
> > +        &__start_bug_frames[0],
> > +        &__stop_bug_frames_0[0],
> > +        &__stop_bug_frames_1[0],
> > +        &__stop_bug_frames_2[0],
> > +        &__stop_bug_frames_3[0],
> > +    };
> > +
> > +    for ( id = 0; id < BUGFRAME_NR; id++ )
> > +    {
> > +        start = bug_frames[id];
> > +        end   = bug_frames[id + 1];
> > +
> > +        while ( start != end )
> > +        {
> > +            if ( (vaddr_t)bug_loc(start) == pc )
> > +            {
> > +                bug = start;
> > +                goto found;
> > +            }
> > +
> > +            start++;
> > +        }
> > +    }
> > +
> > + found:
> > +    if ( bug == NULL )
> > +        return -ENOENT;
> > +
> > +    if ( id == BUGFRAME_run_fn )
> > +    {
> > +        void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug);
> > +
> > +        fn(regs);
> > +
> > +        goto end;
> > +    }
> > +
> > +    /* WARN, BUG or ASSERT: decode the filename pointer and line
> > number. */
> > +    filename = bug_ptr(bug);
> > +    lineno = bug_line(bug);
> > +
> > +    switch ( id )
> > +    {
> > +    case BUGFRAME_warn:
> > +        early_printk("Xen WARN at ");
> > +        early_printk(filename);
> > +        early_printk("\n");
> > +
> > +        show_execution_state(regs);
> > +
> > +        goto end;
> > +
> > +    case BUGFRAME_bug:
> > +        early_printk("Xen BUG at ");
> > +        early_printk(filename);
> > +        early_printk("\n");
> > +
> > +        show_execution_state(regs);
> > +        early_printk("change wait_for_interrupt to panic() when
> > common is available\n");
> > +        die();
> > +
> > +    case BUGFRAME_assert:
> > +        /* ASSERT: decode the predicate string pointer. */
> > +        predicate = bug_msg(bug);
> > +
> > +        early_printk("Assertion \'");
> > +        early_printk(predicate);
> > +        early_printk("\' failed at ");
> > +        early_printk(filename);
> > +        early_printk("\n");
> > +
> > +        show_execution_state(regs);
> > +        early_printk("change wait_for_interrupt to panic() when
> > common is available\n");
> > +        die();
> > +    }
> > +
> > +    return -EINVAL;
> > +
> > + end:
> > +    return 0;
> > +}
> 
> Wasn't the goal to use the generic do_bug_frame()?
It's the goal but at the current stage we can't switch to
do_bug_frame() because it is based on function from xen/common which we
don't have now for RISC-V. So it will be switched when xen/common will
be available.
> 
> > +bool is_valid_bugaddr(vaddr_t insn)
> 
> Why is insn of type vaddr_t?
Initially address of insn was passed here. But now we should change
vaddr_t to 'uint32_t' as the instruction length can be either 16 or 32
and update instr variable inside do_trap():

  static uint32_t read_instr(unsigned long pc)
  {
      uint16_t instr16 = *(uint16_t *)pc;
 
      if ( GET_INSN_LENGTH(instr16) == 2 )
          return (uint32_t)instr16;
      else
          return *(uint32_t *)pc;
  }

  void do_trap(struct cpu_user_regs *cpu_regs)
  {
      register_t pc = cpu_regs->sepc;
      uint32_t instr = read_instr(pc);
  ...

> 
> > +{
> > +    if ( (insn & INSN_LENGTH_MASK) == INSN_LENGTH_32 )
> > +        return ( insn == BUG_INSN_32 );
> > +    else
> > +        return ( (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16 );
> 
> Nit (style): The kind-of-operand to return is an expression. Hence
> you have stray blanks there immediately inside the parentheses.
> (This is unlike e.g. if(), where you've formatted things correctly.)
To be 100% sure, should it be: 
  return ( ( insn & COMPRESSED_INSN_MASK ) == BUG_INSN_16 );
> > +}
> > +
> >  void do_trap(struct cpu_user_regs *cpu_regs)
> >  {
> > +    register_t pc = cpu_regs->sepc;
> > +    uint16_t instr = *(uint16_t *)pc;
> > +
> > +    if ( is_valid_bugaddr(instr) ) {
> > +        if ( !do_bug_frame(cpu_regs, cpu_regs->sepc) ) {
> 
> Nit: Brace placement (twice).
Thanks. I'll update then.
> 
> Jan

Re: [PATCH v4 3/5] xen/riscv: introduce an implementation of macros from <asm/bug.h>

[Jan Beulich]

On 01.03.2023 12:51, Oleksii wrote:
> On Mon, 2023-02-27 at 13:59 +0100, Jan Beulich wrote:
>> On 24.02.2023 12:35, Oleksii Kurochko wrote:
>>> +{
>>> +    if ( (insn & INSN_LENGTH_MASK) == INSN_LENGTH_32 )
>>> +        return ( insn == BUG_INSN_32 );
>>> +    else
>>> +        return ( (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16 );
>>
>> Nit (style): The kind-of-operand to return is an expression. Hence
>> you have stray blanks there immediately inside the parentheses.
>> (This is unlike e.g. if(), where you've formatted things correctly.)
> To be 100% sure, should it be: 
>   return ( ( insn & COMPRESSED_INSN_MASK ) == BUG_INSN_16 );

No, that's yet more spaces instead of fewer ones.

    if ( (insn & INSN_LENGTH_MASK) == INSN_LENGTH_32 )
        return insn == BUG_INSN_32;
    else
        return (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16;

or, if you really want the extra parentheses:

    if ( (insn & INSN_LENGTH_MASK) == INSN_LENGTH_32 )
        return (insn == BUG_INSN_32);
    else
        return ((insn & COMPRESSED_INSN_MASK) == BUG_INSN_16);

(Personally I'd also omit the "else":

    if ( (insn & INSN_LENGTH_MASK) == INSN_LENGTH_32 )
        return insn == BUG_INSN_32;

    return (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16;

. Plus I don't think you really need to mask as much, i.e.

    return insn == BUG_INSN_32 ||
           (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16;

would do as well afaict.)

Jan