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

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

[Oleksii Kurochko]

The patch series is based on:
    Enable build of full Xen for RISC-V [1]
which haven't fully been merged 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 used macros
from <asm/bug.h> such as BUG/WARN/run_in_exception/assert_failed.

It wasn't implemented show_execution_state() and stack trace discovering
as it's not necessary now.

[1] https://lore.kernel.org/xen-devel/cover.1719917348.git.oleksii.kurochko@gmail.com/T/#m8ee766297511fb6fb4a38b7753ab6b792361be04

---
Changes in V10:
 - No generic changes to describe here. Please look at patch-specific changes.
---
Changes in V9:
 - Update the cover letter message.
 - s/early_printk/printk.
 - use GENERIC_BUG_FRAME instead of arch-specific implementation of do_bug_frame().
 - Rebase on top of current staging and riscv-full-xen-build-v14.
---
Changes in V8:
 - Update the commit message of the cover letter.
 - Remove the patch with an introduction of dummy asm/bug.h and introduce
   normal asm/bug.h
 - Fix typo in return string from decode_reserved_interrupt_cause
 - Add "Acked-by: Alistair Francis <alistair.francis@wdc.com>" for the patch
   "xen/riscv: introduce decode_cause() stuff"
 - Remove Pointless initializer of id in do_bug_frame().
 - Make bug_frames[] array constant in do_bug_frame().
 - Remove cast_to_bug_frame(addr).
 - Rename is_valig_bugaddr to is_valid_bug_insn().
 - Add check that read_instr is used only on Xen code
 - Update the commit message.
---
Changes in V7:
 - Update the depenency ( mentioned in the cover letter message ) of the current
   patch series.
 - clean up comments.
 - code style fixes.
 - move definition of cast_to_bug_frame() from patch 4 to 5.
---
Changes in V6:
 - Update the cover letter message: the patch set is based on MMU patch series.
 - Introduce new patch with temporary printk functionality. ( it will be
   removed when Xen common code will be ready )
 - Change early_printk() to printk().
 - Remove usage of LINK_TO_LOAD() due to the MMU being enabled first.
 - Add additional explanatory comments.
 - Remove patch "xen/riscv: initialize boot_info structure" from the patch
   series.
---
Changes in V5:
 - Rebase on top of [1] and [2]
 - Add new patch which introduces stub for <asm/bug.h> to keep Xen compilable
   as in the patch [xen/riscv: introduce decode_cause() stuff] is used
   header <xen/lib.h> which requires <asm/bug.h>.
 - 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
 - Remove "#include <xen/types.h>" from <asm/bug.h> as there is no any need in it anymore
 - Update macros GET_INSN_LENGTH: remove UL and 'unsigned int len;' from it
 - Remove " include <xen/bug.h>" from risc/setup.c. it is not needed in the current version of
   the patch
 - change an argument type from vaddr_t to uint32_t for is_valid_bugaddr and introduce 
   read_instr() to read instruction properly as the length of qinstruction can be
   either 32 or 16 bits.
 - Code style fixes
 - update the comments before do_bug_frame() in riscv/trap.c
 - [[PATCH v4 5/5] automation: modify RISC-V smoke test ] was dropped as it was provided
   more simple solution by Andrew.  CI: Simplify RISCV smoke testing
 - Refactor is_valid_bugaddr() function.
 - 2 new patches ([PATCH v5 {1-2}/7]) were introduced, the goal of which is to recalculate
   addresses used in traps.c, which can be linker time relative. It is needed as we don't
   have enabled MMU yet.
---
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: use printk() instead of early_printk()
  xen/riscv: introduce decode_cause() stuff
  xen/riscv: introduce trap_init()
  xen/riscv: enable GENERIC_BUG_FRAME
  xen/riscv: test basic exception handling stuff

 xen/arch/riscv/Kconfig             |   1 +
 xen/arch/riscv/include/asm/traps.h |   1 +
 xen/arch/riscv/setup.c             |  25 ++++++-
 xen/arch/riscv/traps.c             | 115 ++++++++++++++++++++++++++++-
 xen/common/bug.c                   |   1 +
 5 files changed, 140 insertions(+), 3 deletions(-)

-- 
2.45.2

[PATCH v10 1/5] xen/riscv: use printk() instead of early_printk()

[Oleksii Kurochko]

As common code is available it is better to use printk() instead
of early_printk().

Also the printing of "Hello from RISC-V world" is dropped as
it is useless and "All set up is enough".

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Changes in V10:
 - add Acked-by: Jan Beulich <jbeulich@suse.com>
---
Changes in V9:
 - new patch
---
 xen/arch/riscv/setup.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index 8bb5bdb2ae..e3cb0866d5 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -23,9 +23,7 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
 {
     remove_identity_mapping();
 
-    early_printk("Hello from C env\n");
-
-    early_printk("All set up\n");
+    printk("All set up\n");
 
     for ( ;; )
         asm volatile ("wfi");
-- 
2.45.2

[PATCH v10 2/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>
Acked-by: Alistair Francis <alistair.francis@wdc.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Changes in V10:
 - add Acked-by: Jan Beulich <jbeulich@suse.com>
---
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@wdc.com>
---
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";
+}
+
+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);
+
+    printk("Unhandled exception: %s\n", decode_cause(cause));
+
     die();
 }
 
+void do_trap(struct cpu_user_regs *cpu_regs)
+{
+    do_unexpected_trap(cpu_regs);
+}
+
 void vcpu_show_execution_state(struct vcpu *v)
 {
     BUG_ON("unimplemented");
-- 
2.45.2

[PATCH v10 3/5] xen/riscv: introduce trap_init()

[Oleksii Kurochko]

trap_init() wasn't declared with the __init attribute to avoid removing
__init when multi-CPU support for Xen is added.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
Changes in V10:
  - update the commit message
---
Changes in V9:
 - drop inclusion of <asm/mm.h> in setup.c
 - drop #include <public/version.h>
 - ... <xen/bug.h>
 - introduce stub for
   void arch_get_xen_caps(xen_capabilities_info_t *info);

 - resolve rebase conflicts.
---
Changes in V8:
 - nothing changed. only rebase was done.
---
Changes in V7:
 - #define cast_to_bug_frame(addr) ((const struct bug_frame *)(addr)).
 - remove unnecessary comments in trap_init() function.
---
Changes in V6:
 - trap_init() is now called after enabling the MMU.
 - Add additional explanatory comments.
---
Changes in V5:
  - Nothing changed
---
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             |  3 +++
 xen/arch/riscv/traps.c             | 12 ++++++++++++
 3 files changed, 16 insertions(+)

diff --git a/xen/arch/riscv/include/asm/traps.h b/xen/arch/riscv/include/asm/traps.h
index 3fef318478..c30118e095 100644
--- a/xen/arch/riscv/include/asm/traps.h
+++ b/xen/arch/riscv/include/asm/traps.h
@@ -9,6 +9,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 e3cb0866d5..a6a29a1508 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -8,6 +8,7 @@
 #include <public/version.h>
 
 #include <asm/early_printk.h>
+#include <asm/traps.h>
 
 void arch_get_xen_caps(xen_capabilities_info_t *info)
 {
@@ -23,6 +24,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
 {
     remove_identity_mapping();
 
+    trap_init();
+
     printk("All set up\n");
 
     for ( ;; )
diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
index 37cec40dfa..cb18b30ff2 100644
--- a/xen/arch/riscv/traps.c
+++ b/xen/arch/riscv/traps.c
@@ -12,6 +12,18 @@
 #include <asm/riscv_encoding.h>
 #include <asm/traps.h>
 
+/*
+ * Initialize the trap handling.
+ *
+ * The function is called after MMU is enabled.
+ */
+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.45.2

[PATCH v10 4/5] xen/riscv: enable GENERIC_BUG_FRAME

[Oleksii Kurochko]

To have working BUG(), WARN(), ASSERT, run_in_exception_handler()
it is needed to enable GENERIC_BUG_FRAME.

Also, <xen/lib.h> is needed to be included for the reason that panic() and
printk() are used in common/bug.c and RISC-V fails if it is not included
with the following errors:
   common/bug.c:69:9: error: implicit declaration of function 'printk'
   [-Werror=implicit-function-declaration]
      69 |         printk("Xen WARN at %s%s:%d\n", prefix, filename,
   lineno);
         |         ^~~~~~
   common/bug.c:77:9: error: implicit declaration of function 'panic'
   [-Werror=implicit-function-declaration]
      77 |         panic("Xen BUG at %s%s:%d\n", prefix, filename,
   lineno);

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V10:
 - put 'select GENERIC_BUG_FRAME' in "Config RISCV".
 - rework do_trap() to not fetch an instruction in case when the cause of trap
   is BUG_insn.
 - drop read_instr() and is_valid_bug_insn().
 - update the commit message.
---
Changes in V9:
 - Rebase on the top of current staging.
 - use GENERIC_BUG_FRAME as now we have common code available.
 - add xen/lib.h to bug.c to fix a compilation error around printk.
 - update the commit message.
 - update the code of read_instr() in traps.c
 - fold two-s if into 1 in do_trap.
---
Changes in V8:
  - remove Pointless initializer of id.
  - make bug_frames[] array constant.
  - remove cast_to_bug_frame(addr).
  - rename is_valig_bugaddr to is_valid_bug_insn().
  - add check that read_instr is used only on xen code
  - update the commit message.
---
Changes in V7:
 - move to this patch the definition of cast_to_bug_frame() from the previous patch.
 - update the comment in bug.h.
 - update the comment above do_bug_frame().
 - fix code style.
 - add comment to read_instr func.
 - add space for bug_frames in lds.S.
---
Changes in V6:
  - Avoid LINK_TO_LOAD() as bug.h functionality expected to be used
    after MMU is enabled.
  - Change early_printk() to printk()
---
Changes in V5:
  - Remove "#include <xen/types.h>" from <asm/bug.h> as there is no any need in it anymore
  - Update macros GET_INSN_LENGTH: remove UL and 'unsigned int len;' from it
  - Remove " include <xen/bug.h>" from risc/setup.c. it is not needed in the current version of
    the patch
  - change an argument type from vaddr_t to uint32_t for is_valid_bugaddr and introduce read_instr() to
    read instruction properly as the length of qinstruction can be either 32 or 16 bits.
  - Code style fixes
  - update the comments before do_bug_frame() in riscv/trap.c
  - Refactor is_valid_bugaddr() function.
  - introduce macros cast_to_bug_frame(addr) to hide casts.
  - use LINK_TO_LOAD() for addresses which are linker time relative.
---
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/Kconfig |  1 +
 xen/arch/riscv/traps.c | 25 ++++++++++++++++++++++++-
 xen/common/bug.c       |  1 +
 3 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/xen/arch/riscv/Kconfig b/xen/arch/riscv/Kconfig
index b4b354a778..f531e96657 100644
--- a/xen/arch/riscv/Kconfig
+++ b/xen/arch/riscv/Kconfig
@@ -1,6 +1,7 @@
 config RISCV
 	def_bool y
 	select FUNCTION_ALIGNMENT_16B
+	select GENERIC_BUG_FRAME
 
 config RISCV_64
 	def_bool y
diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
index cb18b30ff2..e39603dc95 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/lib.h>
 #include <xen/sched.h>
 
@@ -103,7 +104,29 @@ static void do_unexpected_trap(const struct cpu_user_regs *regs)
 
 void do_trap(struct cpu_user_regs *cpu_regs)
 {
-    do_unexpected_trap(cpu_regs);
+    register_t pc = cpu_regs->sepc;
+    unsigned long cause = csr_read(CSR_SCAUSE);
+
+    switch ( cause )
+    {
+    case CAUSE_BREAKPOINT:
+        if ( do_bug_frame(cpu_regs, pc) >= 0 )
+        {
+            if ( !pc ||
+                 !(is_kernel_text(pc + 1) || is_kernel_inittext(pc + 1)) )
+            {
+                printk("Something wrong with PC: 0x%lx\n", pc);
+                die();
+            }
+
+            cpu_regs->sepc += GET_INSN_LENGTH(*(uint16_t *)pc);
+            return;
+        }
+
+        break;
+    default:
+        do_unexpected_trap(cpu_regs);
+    }
 }
 
 void vcpu_show_execution_state(struct vcpu *v)
diff --git a/xen/common/bug.c b/xen/common/bug.c
index b7c5d8fd4d..75cb35fcfa 100644
--- a/xen/common/bug.c
+++ b/xen/common/bug.c
@@ -1,6 +1,7 @@
 #include <xen/bug.h>
 #include <xen/errno.h>
 #include <xen/kernel.h>
+#include <xen/lib.h>
 #include <xen/livepatch.h>
 #include <xen/string.h>
 #include <xen/types.h>
-- 
2.45.2

[PATCH v10 5/5] xen/riscv: test basic exception handling stuff

[Oleksii Kurochko]

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Acked-by: Alistair Francis <alistair.francis@wdc.com>
---
Changes in V10:
 - wrap test_macros_from_bug_h() under "#ifdef CONFIG_SELF_TESTS"
 - update the commit title to: "xen/riscv: test basic exception handling stuff"
---
Changes in V9:
  - s/early_printk/printk as common code is now available
---
Changes in V8:
  - Nothing changed. Only rebase.
---
Changes in V6:
  - Nothing changed. Only rebase.
---
Changes in V6:
  - Nothing changed
---
Changes in V5:
  - Nothing changed
---
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 | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index a6a29a1508..4defad68f4 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -19,6 +19,22 @@ void arch_get_xen_caps(xen_capabilities_info_t *info)
 unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
     __aligned(STACK_SIZE);
 
+#ifdef CONFIG_SELF_TESTS
+static void test_run_in_exception(const struct cpu_user_regs *regs)
+{
+    printk("If you see this message, ");
+    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();
+    printk("If you see this message, ");
+    printk("WARN is most likely working\n");
+}
+#endif
+
 void __init noreturn start_xen(unsigned long bootcpu_id,
                                paddr_t dtb_addr)
 {
@@ -26,6 +42,10 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
 
     trap_init();
 
+#ifdef CONFIG_SELF_TESTS
+    test_macros_from_bug_h();
+#endif
+
     printk("All set up\n");
 
     for ( ;; )
-- 
2.45.2

Re: [PATCH v10 3/5] xen/riscv: introduce trap_init()

[Jan Beulich]

On 12.07.2024 18:18, Oleksii Kurochko wrote:
> trap_init() wasn't declared with the __init attribute to avoid removing
> __init when multi-CPU support for Xen is added.

When you say "wasn't" one could easily take this as something you're
correcting in the patch being described. Therefore perhaps better
"isn't". Plus, as this isn't really describing the change, but just
a perhaps unexpected aspect thereof, maybe also better start with
something like "Note that ...".

> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v10 4/5] xen/riscv: enable GENERIC_BUG_FRAME

[Jan Beulich]

On 12.07.2024 18:18, Oleksii Kurochko wrote:
> To have working BUG(), WARN(), ASSERT, run_in_exception_handler()
> it is needed to enable GENERIC_BUG_FRAME.
> 
> Also, <xen/lib.h> is needed to be included for the reason that panic() and
> printk() are used in common/bug.c and RISC-V fails if it is not included
> with the following errors:
>    common/bug.c:69:9: error: implicit declaration of function 'printk'
>    [-Werror=implicit-function-declaration]
>       69 |         printk("Xen WARN at %s%s:%d\n", prefix, filename,
>    lineno);
>          |         ^~~~~~
>    common/bug.c:77:9: error: implicit declaration of function 'panic'
>    [-Werror=implicit-function-declaration]
>       77 |         panic("Xen BUG at %s%s:%d\n", prefix, filename,
>    lineno);

I don't think the diagnostics themselves are needed here.

> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> Changes in V10:
>  - put 'select GENERIC_BUG_FRAME' in "Config RISCV".
>  - rework do_trap() to not fetch an instruction in case when the cause of trap
>    is BUG_insn.

It's BUG_insn here, but then ...

> @@ -103,7 +104,29 @@ static void do_unexpected_trap(const struct cpu_user_regs *regs)
>  
>  void do_trap(struct cpu_user_regs *cpu_regs)
>  {
> -    do_unexpected_trap(cpu_regs);
> +    register_t pc = cpu_regs->sepc;
> +    unsigned long cause = csr_read(CSR_SCAUSE);
> +
> +    switch ( cause )
> +    {
> +    case CAUSE_BREAKPOINT:

... BREAKPOINT here? Generally I'd deem something named "breakpoint" as
debugging related (and hence continuable). I'd have expected
CAUSE_ILLEGAL_INSTRUCTION here, but likely I'm missing something.

> +        if ( do_bug_frame(cpu_regs, pc) >= 0 )
> +        {
> +            if ( !pc ||

In how far does this really need special casing? Isn't that case covered by

> +                 !(is_kernel_text(pc + 1) || is_kernel_inittext(pc + 1)) )

... these checks anyway? And btw, why the "+ 1" in both function arguments?

> +            {
> +                printk("Something wrong with PC: 0x%lx\n", pc);

Nit: %#lx please in situations like this.

> +                die();
> +            }
> +
> +            cpu_regs->sepc += GET_INSN_LENGTH(*(uint16_t *)pc);
> +            return;

This isn't needed, is it? You'd return anyway by ...

> +        }
> +
> +        break;

.... going through here to ...

> +    default:
> +        do_unexpected_trap(cpu_regs);
> +    }
>  }

... here.

Two further nits for the default case: Please have a break statement
there as well, and please have a blank line immediately up from it.

Jan

Re: [PATCH v10 5/5] xen/riscv: test basic exception handling stuff

[Jan Beulich]

On 12.07.2024 18:18, Oleksii Kurochko wrote:
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> Acked-by: Alistair Francis <alistair.francis@wdc.com>

As to the once again empty description and as to earlier comments of mine:

> --- a/xen/arch/riscv/setup.c
> +++ b/xen/arch/riscv/setup.c
> @@ -19,6 +19,22 @@ void arch_get_xen_caps(xen_capabilities_info_t *info)
>  unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
>      __aligned(STACK_SIZE);
>  
> +#ifdef CONFIG_SELF_TESTS
> +static void test_run_in_exception(const struct cpu_user_regs *regs)
> +{
> +    printk("If you see this message, ");
> +    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();
> +    printk("If you see this message, ");
> +    printk("WARN is most likely working\n");
> +}
> +#endif
> +
>  void __init noreturn start_xen(unsigned long bootcpu_id,
>                                 paddr_t dtb_addr)
>  {
> @@ -26,6 +42,10 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
>  
>      trap_init();
>  
> +#ifdef CONFIG_SELF_TESTS
> +    test_macros_from_bug_h();
> +#endif
> +
>      printk("All set up\n");
>  
>      for ( ;; )

While for the moment having this may be okay, I'd like the commit itself to
clarify what the future plans here are. The latest once Xen comes up properly
on RISC-V, this is likely going to get in the way because of the noise it
causes in the log.

Jan

Re: [PATCH v10 4/5] xen/riscv: enable GENERIC_BUG_FRAME

[Oleksii]

On Mon, 2024-07-22 at 13:02 +0200, Jan Beulich wrote:
> On 12.07.2024 18:18, Oleksii Kurochko wrote:
> > To have working BUG(), WARN(), ASSERT, run_in_exception_handler()
> > it is needed to enable GENERIC_BUG_FRAME.
> > 
> > Also, <xen/lib.h> is needed to be included for the reason that
> > panic() and
> > printk() are used in common/bug.c and RISC-V fails if it is not
> > included
> > with the following errors:
> >    common/bug.c:69:9: error: implicit declaration of function
> > 'printk'
> >    [-Werror=implicit-function-declaration]
> >       69 |         printk("Xen WARN at %s%s:%d\n", prefix,
> > filename,
> >    lineno);
> >          |         ^~~~~~
> >    common/bug.c:77:9: error: implicit declaration of function
> > 'panic'
> >    [-Werror=implicit-function-declaration]
> >       77 |         panic("Xen BUG at %s%s:%d\n", prefix, filename,
> >    lineno);
> 
> I don't think the diagnostics themselves are needed here.
> 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> > Changes in V10:
> >  - put 'select GENERIC_BUG_FRAME' in "Config RISCV".
> >  - rework do_trap() to not fetch an instruction in case when the
> > cause of trap
> >    is BUG_insn.
> 
> It's BUG_insn here, but then ...
> 
> > @@ -103,7 +104,29 @@ static void do_unexpected_trap(const struct
> > cpu_user_regs *regs)
> >  
> >  void do_trap(struct cpu_user_regs *cpu_regs)
> >  {
> > -    do_unexpected_trap(cpu_regs);
> > +    register_t pc = cpu_regs->sepc;
> > +    unsigned long cause = csr_read(CSR_SCAUSE);
> > +
> > +    switch ( cause )
> > +    {
> > +    case CAUSE_BREAKPOINT:
> 
> ... BREAKPOINT here? Generally I'd deem something named "breakpoint"
> as
> debugging related (and hence continuable). I'd have expected
> CAUSE_ILLEGAL_INSTRUCTION here, but likely I'm missing something.
Agree, that is is confusing, but BUG_insn is defined as ebreak
instruction ( Linux kernel uses also ebreak ) and it generates
CAUSE_BREAKPOINT.

> 
> > +        if ( do_bug_frame(cpu_regs, pc) >= 0 )
> > +        {
> > +            if ( !pc ||
> 
> In how far does this really need special casing? Isn't that case
> covered by
> 
> > +                 !(is_kernel_text(pc + 1) || is_kernel_inittext(pc
> > + 1)) )
> 
> ... these checks anyway?
Good point. We could drop it.

> And btw, why the "+ 1" in both function arguments?
There is no need for them anymore, just missed to drop +1.

~ Oleksii

> 
> > +            {
> > +                printk("Something wrong with PC: 0x%lx\n", pc);
> 
> Nit: %#lx please in situations like this.
> 
> > +                die();
> > +            }
> > +
> > +            cpu_regs->sepc += GET_INSN_LENGTH(*(uint16_t *)pc);
> > +            return;
> 
> This isn't needed, is it? You'd return anyway by ...
> 
> > +        }
> > +
> > +        break;
> 
> .... going through here to ...
> 
> > +    default:
> > +        do_unexpected_trap(cpu_regs);
> > +    }
> >  }
> 
> ... here.
> 
> Two further nits for the default case: Please have a break statement
> there as well, and please have a blank line immediately up from it.
> 
> Jan

Re: [PATCH v10 4/5] xen/riscv: enable GENERIC_BUG_FRAME

[Jan Beulich]

On 22.07.2024 16:09, Oleksii wrote:
> On Mon, 2024-07-22 at 13:02 +0200, Jan Beulich wrote:
>> On 12.07.2024 18:18, Oleksii Kurochko wrote:
>>> ---
>>> Changes in V10:
>>>  - put 'select GENERIC_BUG_FRAME' in "Config RISCV".
>>>  - rework do_trap() to not fetch an instruction in case when the
>>> cause of trap
>>>    is BUG_insn.
>>
>> It's BUG_insn here, but then ...
>>
>>> @@ -103,7 +104,29 @@ static void do_unexpected_trap(const struct
>>> cpu_user_regs *regs)
>>>  
>>>  void do_trap(struct cpu_user_regs *cpu_regs)
>>>  {
>>> -    do_unexpected_trap(cpu_regs);
>>> +    register_t pc = cpu_regs->sepc;
>>> +    unsigned long cause = csr_read(CSR_SCAUSE);
>>> +
>>> +    switch ( cause )
>>> +    {
>>> +    case CAUSE_BREAKPOINT:
>>
>> ... BREAKPOINT here? Generally I'd deem something named "breakpoint"
>> as
>> debugging related (and hence continuable). I'd have expected
>> CAUSE_ILLEGAL_INSTRUCTION here, but likely I'm missing something.
> Agree, that is is confusing, but BUG_insn is defined as ebreak
> instruction ( Linux kernel uses also ebreak ) and it generates
> CAUSE_BREAKPOINT.

I'm curious: How do you / does a debugger tell a breakpoint set on
such an EBREAK insn (e.g. as a result of a use of WARN_ON()) from
the original, unmodified insn? If there's a breakpoint, you want
to forward to the debugger. Whereas if there's no breakpoint, you
want to process the WARN_ON() normally.

Jan

Re: [PATCH v10 4/5] xen/riscv: enable GENERIC_BUG_FRAME

[oleksii.kurochko]

On Mon, 2024-07-22 at 17:32 +0200, Jan Beulich wrote:
> On 22.07.2024 16:09, Oleksii wrote:
> > On Mon, 2024-07-22 at 13:02 +0200, Jan Beulich wrote:
> > > On 12.07.2024 18:18, Oleksii Kurochko wrote:
> > > > ---
> > > > Changes in V10:
> > > >  - put 'select GENERIC_BUG_FRAME' in "Config RISCV".
> > > >  - rework do_trap() to not fetch an instruction in case when
> > > > the
> > > > cause of trap
> > > >    is BUG_insn.
> > > 
> > > It's BUG_insn here, but then ...
> > > 
> > > > @@ -103,7 +104,29 @@ static void do_unexpected_trap(const
> > > > struct
> > > > cpu_user_regs *regs)
> > > >  
> > > >  void do_trap(struct cpu_user_regs *cpu_regs)
> > > >  {
> > > > -    do_unexpected_trap(cpu_regs);
> > > > +    register_t pc = cpu_regs->sepc;
> > > > +    unsigned long cause = csr_read(CSR_SCAUSE);
> > > > +
> > > > +    switch ( cause )
> > > > +    {
> > > > +    case CAUSE_BREAKPOINT:
> > > 
> > > ... BREAKPOINT here? Generally I'd deem something named
> > > "breakpoint"
> > > as
> > > debugging related (and hence continuable). I'd have expected
> > > CAUSE_ILLEGAL_INSTRUCTION here, but likely I'm missing something.
> > Agree, that is is confusing, but BUG_insn is defined as ebreak
> > instruction ( Linux kernel uses also ebreak ) and it generates
> > CAUSE_BREAKPOINT.
> 
> I'm curious: How do you / does a debugger tell a breakpoint set on
> such an EBREAK insn (e.g. as a result of a use of WARN_ON()) from
> the original, unmodified insn? If there's a breakpoint, you want
> to forward to the debugger. Whereas if there's no breakpoint, you
> want to process the WARN_ON() normally.
I don't know details of debug spec but AFAIU ebreak triggers a debug
trap, which starts in debug mode and is then filtered by every mode as
it goes towards user-mode. So first GDB will handle this debug trap and
will check if this ebreak was set by him or not.

~ Oleksii