[PATCH v5 0/7] RISCV basic exception handling implementation

[PATCH v5 0/7] RISCV basic exception handling implementation

[Oleksii Kurochko]

The patch series is based on:
* [introduce generic implementation of macros from bug.h][1]
* [Do basic initialization things][2]
* [Deal with GOT stuff for RISC-V][3]
which haven'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.

[1] https://lore.kernel.org/xen-devel/cover.1678900513.git.oleksii.kurochko@gmail.com/
[2] https://lore.kernel.org/xen-devel/cover.1677838213.git.oleksii.kurochko@gmail.com/
[3] https://lore.kernel.org/xen-devel/69e031eb-6172-1ab0-5ffa-4650f69e83a7@citrix.com/T/#t
---
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 (7):
  xen/riscv: introduce boot information structure
  xen/riscv: initialize boot_info structure
  xen/riscv: introduce dummy <asm/bug.h>
  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

 xen/arch/riscv/include/asm/boot-info.h |  15 ++
 xen/arch/riscv/include/asm/bug.h       |  38 ++++
 xen/arch/riscv/include/asm/processor.h |   2 +
 xen/arch/riscv/include/asm/traps.h     |   1 +
 xen/arch/riscv/riscv64/asm-offsets.c   |   3 +
 xen/arch/riscv/setup.c                 |  34 ++++
 xen/arch/riscv/traps.c                 | 233 ++++++++++++++++++++++++-
 xen/arch/riscv/xen.lds.S               |  10 ++
 8 files changed, 335 insertions(+), 1 deletion(-)
 create mode 100644 xen/arch/riscv/include/asm/boot-info.h
 create mode 100644 xen/arch/riscv/include/asm/bug.h

-- 
2.39.2

[PATCH v5 1/7] xen/riscv: introduce boot information structure

[Oleksii Kurochko]

The structure holds information about:
1. linker start/end address
2. load start/end address

Also the patch introduces offsets for boot information structure
members to access them in assembly code.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V5:
 * the patch was introduced in the current patch series (V5)
---
 xen/arch/riscv/include/asm/boot-info.h | 15 +++++++++++++++
 xen/arch/riscv/riscv64/asm-offsets.c   |  3 +++
 2 files changed, 18 insertions(+)
 create mode 100644 xen/arch/riscv/include/asm/boot-info.h

diff --git a/xen/arch/riscv/include/asm/boot-info.h b/xen/arch/riscv/include/asm/boot-info.h
new file mode 100644
index 0000000000..cda3d278f5
--- /dev/null
+++ b/xen/arch/riscv/include/asm/boot-info.h
@@ -0,0 +1,15 @@
+#ifndef _ASM_BOOT_INFO_H
+#define _ASM_BOOT_INFO_H
+
+extern struct boot_info {
+    unsigned long linker_start;
+    unsigned long linker_end;
+    unsigned long load_start;
+    unsigned long load_end;
+} boot_info;
+
+/* LINK_TO_LOAD() and LOAD_TO_LINK() works only when MMU isn't enabled. */
+#define LINK_TO_LOAD(addr) ((addr) - boot_info.linker_start + boot_info.load_start)
+#define LOAD_TO_LINK(addr) ((addr) - boot_info.load_start + boot_info.linker_start)
+
+#endif
\ No newline at end of file
diff --git a/xen/arch/riscv/riscv64/asm-offsets.c b/xen/arch/riscv/riscv64/asm-offsets.c
index d632b75c2a..6b89e9a91d 100644
--- a/xen/arch/riscv/riscv64/asm-offsets.c
+++ b/xen/arch/riscv/riscv64/asm-offsets.c
@@ -1,5 +1,6 @@
 #define COMPILE_OFFSETS
 
+#include <asm/boot-info.h>
 #include <asm/processor.h>
 #include <xen/types.h>
 
@@ -50,4 +51,6 @@ void asm_offsets(void)
     OFFSET(CPU_USER_REGS_SEPC, struct cpu_user_regs, sepc);
     OFFSET(CPU_USER_REGS_SSTATUS, struct cpu_user_regs, sstatus);
     OFFSET(CPU_USER_REGS_PREGS, struct cpu_user_regs, pregs);
+    OFFSET(BI_LINKER_START, struct boot_info, linker_start);
+    OFFSET(BI_LOAD_START, struct boot_info, load_start);
 }
-- 
2.39.2

[PATCH v5 2/7] xen/riscv: initialize boot_info structure

[Oleksii Kurochko]

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V5:
 * the patch was introduced in the current patch series (V5)
---
 xen/arch/riscv/setup.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index 0908bdb9f9..36556eb779 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -1,12 +1,16 @@
 #include <xen/compile.h>
 #include <xen/init.h>
+#include <xen/kernel.h>
 
+#include <asm/boot-info.h>
 #include <asm/early_printk.h>
 
 /* Xen stack for bringing up the first CPU. */
 unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
     __aligned(STACK_SIZE);
 
+struct boot_info boot_info = { (unsigned long)&_start, (unsigned long)&_end, 0UL, 0UL };
+
 /*  
  * To be sure that .bss isn't zero. It will simplify code of
  * .bss initialization.
@@ -15,11 +19,19 @@ unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
  */
 int dummy_bss __attribute__((unused));
 
+static void fill_boot_info(void)
+{
+    boot_info.load_start = (unsigned long)_start;
+    boot_info.load_end   = (unsigned long)_end;
+}
+
 void __init noreturn start_xen(unsigned long bootcpu_id,
                                unsigned long dtb_paddr)
 {
     early_printk("Hello from C env\n");
 
+    fill_boot_info();
+
     early_printk("All set up\n");
     for ( ;; )
         asm volatile ("wfi");
-- 
2.39.2

[PATCH v5 3/7] xen/riscv: introduce dummy <asm/bug.h>

[Oleksii Kurochko]

<xen/lib.h> will be used in the patch "xen/riscv: introduce
decode_cause() stuff" and requires <asm/bug.h>

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V5:
 * the patch was introduced in the current patch series (V5)
---
 xen/arch/riscv/include/asm/bug.h | 10 ++++++++++
 1 file changed, 10 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..e8b1e40823
--- /dev/null
+++ b/xen/arch/riscv/include/asm/bug.h
@@ -0,0 +1,10 @@
+/* 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
+
+#endif /* _ASM_RISCV_BUG_H */
-- 
2.39.2

[PATCH v5 4/7] 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 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

[PATCH v5 5/7] 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 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             | 5 +++++
 xen/arch/riscv/traps.c             | 7 +++++++
 3 files changed, 13 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 36556eb779..b44d105b5f 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -3,7 +3,9 @@
 #include <xen/kernel.h>
 
 #include <asm/boot-info.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]
@@ -32,7 +34,10 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
 
     fill_boot_info();
 
+    trap_init();
+
     early_printk("All set up\n");
+
     for ( ;; )
         asm volatile ("wfi");
 
diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
index 8a1529e0c5..581f34efbc 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.2

[PATCH v5 6/7] xen/riscv: introduce an implementation of macros from <asm/bug.h>

[Oleksii Kurochko]

The patch introduces macros: BUG(), WARN(), run_in_exception(),
assert_failed.
To be precise, the macros from generic bug implementation (<xen/bug.h>)
will be used.

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 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/include/asm/bug.h       |  28 +++++
 xen/arch/riscv/include/asm/processor.h |   2 +
 xen/arch/riscv/traps.c                 | 139 +++++++++++++++++++++++++
 xen/arch/riscv/xen.lds.S               |  10 ++
 4 files changed, 179 insertions(+)

diff --git a/xen/arch/riscv/include/asm/bug.h b/xen/arch/riscv/include/asm/bug.h
index e8b1e40823..bf3194443f 100644
--- a/xen/arch/riscv/include/asm/bug.h
+++ b/xen/arch/riscv/include/asm/bug.h
@@ -7,4 +7,32 @@
 #ifndef _ASM_RISCV_BUG_H
 #define _ASM_RISCV_BUG_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)                               \
+    (((insn) & INSN_LENGTH_MASK) == INSN_LENGTH_32 ? 4 : 2) \
+
+#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/traps.c b/xen/arch/riscv/traps.c
index 581f34efbc..2afcabb912 100644
--- a/xen/arch/riscv/traps.c
+++ b/xen/arch/riscv/traps.c
@@ -5,6 +5,8 @@
  * RISC-V Trap handlers
  */
 
+#include <xen/bug.h>
+#include <xen/errno.h>
 #include <xen/lib.h>
 
 #include <asm/boot-info.h>
@@ -13,6 +15,10 @@
 #include <asm/processor.h>
 #include <asm/traps.h>
 
+#define cast_to_bug_frame(addr) \
+    (const struct bug_frame *)(LINK_TO_LOAD((char *)addr))
+
+
 void trap_init(void)
 {
     unsigned long addr = (unsigned long)&handle_trap;
@@ -99,7 +105,140 @@ 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()
+ * 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 = cast_to_bug_frame(bug_frames[id]);
+        end   = cast_to_bug_frame(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;
+}
+
+static bool is_valid_bugaddr(uint32_t insn)
+{
+    return insn == BUG_INSN_32 ||
+           (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16;
+}
+
+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 ( is_valid_bugaddr(instr) )
+    {
+        if ( !do_bug_frame(cpu_regs, pc) )
+        {
+            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 f299ea8422..eed457c492 100644
--- a/xen/arch/riscv/xen.lds.S
+++ b/xen/arch/riscv/xen.lds.S
@@ -40,6 +40,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.2

[PATCH v5 7/7] 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 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 | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index b44d105b5f..b56c69a3dc 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>
 #include <xen/kernel.h>
@@ -27,6 +28,20 @@ static void fill_boot_info(void)
     boot_info.load_end   = (unsigned long)_end;
 }
 
+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(unsigned long bootcpu_id,
                                unsigned long dtb_paddr)
 {
@@ -36,6 +51,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
 
     trap_init();
 
+    test_macros_from_bug_h();
+
     early_printk("All set up\n");
 
     for ( ;; )
-- 
2.39.2

Re: [PATCH v5 1/7] xen/riscv: introduce boot information structure

[Jan Beulich]

On 16.03.2023 15:39, Oleksii Kurochko wrote:
> @@ -50,4 +51,6 @@ void asm_offsets(void)
>      OFFSET(CPU_USER_REGS_SEPC, struct cpu_user_regs, sepc);
>      OFFSET(CPU_USER_REGS_SSTATUS, struct cpu_user_regs, sstatus);
>      OFFSET(CPU_USER_REGS_PREGS, struct cpu_user_regs, pregs);
> +    OFFSET(BI_LINKER_START, struct boot_info, linker_start);
> +    OFFSET(BI_LOAD_START, struct boot_info, load_start);
>  }

May I suggest that you add BLANK(); and a blank line between separate
groups of OFFSET() uses? This may not matter much right now, but it'll
help readability and findability once the file further grows.

Jan

Re: [PATCH v5 2/7] xen/riscv: initialize boot_info structure

[Jan Beulich]

On 16.03.2023 15:39, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/setup.c
> +++ b/xen/arch/riscv/setup.c
> @@ -1,12 +1,16 @@
>  #include <xen/compile.h>
>  #include <xen/init.h>
> +#include <xen/kernel.h>
>  
> +#include <asm/boot-info.h>
>  #include <asm/early_printk.h>
>  
>  /* Xen stack for bringing up the first CPU. */
>  unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
>      __aligned(STACK_SIZE);
>  
> +struct boot_info boot_info = { (unsigned long)&_start, (unsigned long)&_end, 0UL, 0UL };

You add no declaration in a header, in which case this wants to be static.
It may also want to be __initdata, depending on further planned uses. I
would also like to suggest using C99 dedicated initializers and in any
event omit the 0UL initializer values (as that's what the compiler will
use anyway). Yet even then I think the line is still too long and hence
needs wrapping.

> @@ -15,11 +19,19 @@ unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
>   */
>  int dummy_bss __attribute__((unused));
>  
> +static void fill_boot_info(void)

__init?

> +{
> +    boot_info.load_start = (unsigned long)_start;
> +    boot_info.load_end   = (unsigned long)_end;
> +}

I'd like to understand how this is intended to work: _start and _end
are known at compile time, and their value won't change (unless you
process relocations alongside switching from 1:1 mapping to some
other virtual memory layout). Therefore - why can't these be put in
the initializer as well? Guessing that the values derived here are
different because of being calculated in a PC-relative way, I think
this really needs a comment. If, of course, this can be relied upon
in the first place.

Also please be consistent about the use of unary & when taking the
address of arrays (or functions). Personally I prefer the & to be
omitted in such cases, but what I consider relevant is that there
be no mix (in new code at least).

Jan

Re: [PATCH v5 1/7] xen/riscv: introduce boot information structure

[Andrew Cooper]

On 16/03/2023 2:39 pm, Oleksii Kurochko wrote:
> The structure holds information about:
> 1. linker start/end address
> 2. load start/end address
>
> Also the patch introduces offsets for boot information structure
> members to access them in assembly code.
>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> Changes in V5:
>  * the patch was introduced in the current patch series (V5)
> ---
>  xen/arch/riscv/include/asm/boot-info.h | 15 +++++++++++++++
>  xen/arch/riscv/riscv64/asm-offsets.c   |  3 +++
>  2 files changed, 18 insertions(+)
>  create mode 100644 xen/arch/riscv/include/asm/boot-info.h
>
> diff --git a/xen/arch/riscv/include/asm/boot-info.h b/xen/arch/riscv/include/asm/boot-info.h
> new file mode 100644
> index 0000000000..cda3d278f5
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/boot-info.h
> @@ -0,0 +1,15 @@
> +#ifndef _ASM_BOOT_INFO_H
> +#define _ASM_BOOT_INFO_H
> +
> +extern struct boot_info {
> +    unsigned long linker_start;
> +    unsigned long linker_end;
> +    unsigned long load_start;
> +    unsigned long load_end;
> +} boot_info;
> +
> +/* LINK_TO_LOAD() and LOAD_TO_LINK() works only when MMU isn't enabled. */
> +#define LINK_TO_LOAD(addr) ((addr) - boot_info.linker_start + boot_info.load_start)
> +#define LOAD_TO_LINK(addr) ((addr) - boot_info.load_start + boot_info.linker_start)
> +
> +#endif
> \ No newline at end of file

As a minor point, you should have newlines at the end of each file.

However, I'm not sure boot info like this is a clever move.  You're
creating a 3rd different way of doing something which should be entirely
common.  Some details are in
https://lore.kernel.org/xen-devel/115c178b-f0a7-cf6e-3e33-e6aa49b17baf@srcf.net/
and note how many errors I already found in x86 and ARM.

Perhaps its time to dust that plan off again.  As Jan says, there's
_start and _end (or future variations therefore), and xen_phys_start
which is all that ought to exist in order to build the common functionality.

~Andrew

Re: [PATCH v5 1/7] xen/riscv: introduce boot information structure

[Oleksii]

On Tue, 2023-03-21 at 12:17 +0100, Jan Beulich wrote:
> On 16.03.2023 15:39, Oleksii Kurochko wrote:
> > @@ -50,4 +51,6 @@ void asm_offsets(void)
> >      OFFSET(CPU_USER_REGS_SEPC, struct cpu_user_regs, sepc);
> >      OFFSET(CPU_USER_REGS_SSTATUS, struct cpu_user_regs, sstatus);
> >      OFFSET(CPU_USER_REGS_PREGS, struct cpu_user_regs, pregs);
> > +    OFFSET(BI_LINKER_START, struct boot_info, linker_start);
> > +    OFFSET(BI_LOAD_START, struct boot_info, load_start);
> >  }
> 
> May I suggest that you add BLANK(); and a blank line between separate
> groups of OFFSET() uses? This may not matter much right now, but
> it'll
> help readability and findability once the file further grows.
Sure. I'll update it in the next version of the patch.

~ Oleksii

Re: [PATCH v5 2/7] xen/riscv: initialize boot_info structure

[Oleksii]

On Tue, 2023-03-21 at 12:27 +0100, Jan Beulich wrote:
> On 16.03.2023 15:39, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/setup.c
> > +++ b/xen/arch/riscv/setup.c
> > @@ -1,12 +1,16 @@
> >  #include <xen/compile.h>
> >  #include <xen/init.h>
> > +#include <xen/kernel.h>
> >  
> > +#include <asm/boot-info.h>
> >  #include <asm/early_printk.h>
> >  
> >  /* Xen stack for bringing up the first CPU. */
> >  unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
> >      __aligned(STACK_SIZE);
> >  
> > +struct boot_info boot_info = { (unsigned long)&_start, (unsigned
> > long)&_end, 0UL, 0UL };
> 
> You add no declaration in a header, in which case this wants to be
> static.
Sure.
> It may also want to be __initdata, depending on further planned uses.
I am going to use it only for initial page table initialization.
At least for now.
> I
> would also like to suggest using C99 dedicated initializers and in
> any
> event omit the 0UL initializer values (as that's what the compiler
> will
> use anyway). Yet even then I think the line is still too long and
> hence
> needs wrapping.
> 
> > @@ -15,11 +19,19 @@ unsigned char __initdata
> > cpu0_boot_stack[STACK_SIZE]
> >   */
> >  int dummy_bss __attribute__((unused));
> >  
> > +static void fill_boot_info(void)
> 
> __init?
Yes, should it be __init.
> 
> > +{
> > +    boot_info.load_start = (unsigned long)_start;
> > +    boot_info.load_end   = (unsigned long)_end;
> > +}
> 
> I'd like to understand how this is intended to work: _start and _end
> are known at compile time, and their value won't change (unless you
> process relocations alongside switching from 1:1 mapping to some
> other virtual memory layout). Therefore - why can't these be put in
> the initializer as well? Guessing that the values derived here are
> different because of being calculated in a PC-relative way, I think
> this really needs a comment. If, of course, this can be relied upon
> in the first place.
Your guessing is correct. In this case addresses of _start and _end
will be calculated in a PC-relative way.
So I'll add a comment.

> 
> Also please be consistent about the use of unary & when taking the
> address of arrays (or functions). Personally I prefer the & to be
> omitted in such cases, but what I consider relevant is that there
> be no mix (in new code at least).
Sure.

Thanks for the comments.
I'll fix that in the new version of the
patch.

~ Oleksii

Re: [PATCH v5 3/7] xen/riscv: introduce dummy <asm/bug.h>

[Julien Grall]

Hi Oleksii,

On 16/03/2023 14:39, Oleksii Kurochko wrote:
> <xen/lib.h> will be used in the patch "xen/riscv: introduce
> decode_cause() stuff" and requires <asm/bug.h>
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> Changes in V5:
>   * the patch was introduced in the current patch series (V5)
> ---
>   xen/arch/riscv/include/asm/bug.h | 10 ++++++++++
>   1 file changed, 10 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..e8b1e40823
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/bug.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2012 Regents of the University of California
> + * Copyright (C) 2021-2023 Vates

I am a bit puzzled with those copyright given the header is empty.

But is there any reason this can't be folded in #6 or part of #6 moved 
forward?

> + *

NIT: Drop the line.

> + */
> +#ifndef _ASM_RISCV_BUG_H
> +#define _ASM_RISCV_BUG_H
> +
> +#endif /* _ASM_RISCV_BUG_H */

-- 
Julien Grall

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

[Julien Grall]

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 5/7] xen/riscv: introduce trap_init()

[Julien Grall]

Hi Oleksii,

On 16/03/2023 14:39, Oleksii Kurochko wrote:
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> 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             | 5 +++++
>   xen/arch/riscv/traps.c             | 7 +++++++
>   3 files changed, 13 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 36556eb779..b44d105b5f 100644
> --- a/xen/arch/riscv/setup.c
> +++ b/xen/arch/riscv/setup.c
> @@ -3,7 +3,9 @@
>   #include <xen/kernel.h>
>   
>   #include <asm/boot-info.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]
> @@ -32,7 +34,10 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
>   
>       fill_boot_info();
>   
> +    trap_init();
> +
>       early_printk("All set up\n");
> +
>       for ( ;; )
>           asm volatile ("wfi");
>   
> diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
> index 8a1529e0c5..581f34efbc 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;

It is not super clear to me whether this is going to store the virtual 
or physical address.

Depending on the answer, the next would be whether the value would still 
be valid after the MMU is turned on?

> +
> +    csr_write(CSR_STVEC, addr);
> +}
> +
>   static const char *decode_trap_cause(unsigned long cause)
>   {
>       static const char *const trap_causes[] = {

Cheers,

-- 
Julien Grall

Re: [PATCH v5 3/7] xen/riscv: introduce dummy <asm/bug.h>

[Oleksii]

On Tue, 2023-03-21 at 17:21 +0000, Julien Grall wrote:
> Hi Oleksii,
> 
> On 16/03/2023 14:39, Oleksii Kurochko wrote:
> > <xen/lib.h> will be used in the patch "xen/riscv: introduce
> > decode_cause() stuff" and requires <asm/bug.h>
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> > Changes in V5:
> >   * the patch was introduced in the current patch series (V5)
> > ---
> >   xen/arch/riscv/include/asm/bug.h | 10 ++++++++++
> >   1 file changed, 10 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..e8b1e40823
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/bug.h
> > @@ -0,0 +1,10 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2012 Regents of the University of California
> > + * Copyright (C) 2021-2023 Vates
> 
> I am a bit puzzled with those copyright given the header is empty.
> 
> But is there any reason this can't be folded in #6 or part of #6
> moved 
> forward?
Initially it was folded in #6 but in this case a build will be failed
after introduction of #5 as <asm/bug.h> is needed for <xen/lib.h>

Taking into account that <asm/bug.h> is fully re-written after
introduction of generic bug implementation we can remove copyrights
from header fully.

> 
> > + *
> 
> NIT: Drop the line.
Thanks. I'll do it in the next patch version.

> 
> > + */
> > +#ifndef _ASM_RISCV_BUG_H
> > +#define _ASM_RISCV_BUG_H
> > +
> > +#endif /* _ASM_RISCV_BUG_H */
> 

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

[Oleksii]

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 3/7] xen/riscv: introduce dummy <asm/bug.h>

[Jan Beulich]

On 22.03.2023 11:09, Oleksii wrote:
> On Tue, 2023-03-21 at 17:21 +0000, Julien Grall wrote:
>> On 16/03/2023 14:39, Oleksii Kurochko wrote:
>>> <xen/lib.h> will be used in the patch "xen/riscv: introduce
>>> decode_cause() stuff" and requires <asm/bug.h>
>>>
>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>>> ---
>>> Changes in V5:
>>>   * the patch was introduced in the current patch series (V5)
>>> ---
>>>   xen/arch/riscv/include/asm/bug.h | 10 ++++++++++
>>>   1 file changed, 10 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..e8b1e40823
>>> --- /dev/null
>>> +++ b/xen/arch/riscv/include/asm/bug.h
>>> @@ -0,0 +1,10 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/*
>>> + * Copyright (C) 2012 Regents of the University of California
>>> + * Copyright (C) 2021-2023 Vates
>>
>> I am a bit puzzled with those copyright given the header is empty.
>>
>> But is there any reason this can't be folded in #6 or part of #6
>> moved 
>> forward?
> Initially it was folded in #6 but in this case a build will be failed
> after introduction of #5 as <asm/bug.h> is needed for <xen/lib.h>

But what about the other option Julien mentioned, moving ahead the
later "filling" of asm/bug.h, so it wouldn't be introduced empty and
then (almost immediately) touched again to actually populate it?

Jan

Re: [PATCH v5 5/7] xen/riscv: introduce trap_init()

[Oleksii]

Hi Julien,

On Tue, 2023-03-21 at 17:42 +0000, Julien Grall wrote:
> Hi Oleksii,
> 
> On 16/03/2023 14:39, Oleksii Kurochko wrote:
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> > 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             | 5 +++++
> >   xen/arch/riscv/traps.c             | 7 +++++++
> >   3 files changed, 13 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 36556eb779..b44d105b5f 100644
> > --- a/xen/arch/riscv/setup.c
> > +++ b/xen/arch/riscv/setup.c
> > @@ -3,7 +3,9 @@
> >   #include <xen/kernel.h>
> >   
> >   #include <asm/boot-info.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]
> > @@ -32,7 +34,10 @@ void __init noreturn start_xen(unsigned long
> > bootcpu_id,
> >   
> >       fill_boot_info();
> >   
> > +    trap_init();
> > +
> >       early_printk("All set up\n");
> > +
> >       for ( ;; )
> >           asm volatile ("wfi");
> >   
> > diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
> > index 8a1529e0c5..581f34efbc 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;
> 
> It is not super clear to me whether this is going to store the
> virtual 
> or physical address.
Actually it is going to store both the virtual and physical address.
Depending on if MMU is enabled or not.
> 
> Depending on the answer, the next would be whether the value would
> still 
> be valid after the MMU is turned on?
It would still be valid because for addr will be generated PC-relative
address.
> 
> > +
> > +    csr_write(CSR_STVEC, addr);
> > +}
> > +
> >   static const char *decode_trap_cause(unsigned long cause)
> >   {
> >       static const char *const trap_causes[] = {
> 
> Cheers,
> 
> ~ Oleksii

Re: [PATCH v5 5/7] xen/riscv: introduce trap_init()

[Julien Grall]

On 22/03/2023 11:33, Oleksii wrote:
> Hi Julien,

Hi Oleksii,

> 
> On Tue, 2023-03-21 at 17:42 +0000, Julien Grall wrote:
>> Hi Oleksii,
>>
>> On 16/03/2023 14:39, Oleksii Kurochko wrote:
>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>>> ---
>>> 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             | 5 +++++
>>>    xen/arch/riscv/traps.c             | 7 +++++++
>>>    3 files changed, 13 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 36556eb779..b44d105b5f 100644
>>> --- a/xen/arch/riscv/setup.c
>>> +++ b/xen/arch/riscv/setup.c
>>> @@ -3,7 +3,9 @@
>>>    #include <xen/kernel.h>
>>>    
>>>    #include <asm/boot-info.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]
>>> @@ -32,7 +34,10 @@ void __init noreturn start_xen(unsigned long
>>> bootcpu_id,
>>>    
>>>        fill_boot_info();
>>>    
>>> +    trap_init();
>>> +
>>>        early_printk("All set up\n");
>>> +
>>>        for ( ;; )
>>>            asm volatile ("wfi");
>>>    
>>> diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
>>> index 8a1529e0c5..581f34efbc 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;
>>
>> It is not super clear to me whether this is going to store the
>> virtual
>> or physical address.
> Actually it is going to store both the virtual and physical address.
> Depending on if MMU is enabled or not.

I think some comment in the code would be really good because this is...

>>
>> Depending on the answer, the next would be whether the value would
>> still
>> be valid after the MMU is turned on?
> It would still be valid because for addr will be generated PC-relative
> address.

... not clear to me what would guarantee that Xen is compiled with 
-noPIE. Is the cmodel?

A suggestion for the top of the function:

"Initialize the trap handling. This is called twice (before and after 
the MMU)."

And for on top of 'addr', I would add:

"When the MMU is off, this will be a physical address otherwise it would 
be a virtual address. This is guarantee because [fill the blank]".

Cheers,

-- 
Julien Grall

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

[Jan Beulich]

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 1/7] xen/riscv: introduce boot information structure

[Oleksii]

On Tue, 2023-03-21 at 11:56 +0000, Andrew Cooper wrote:
> On 16/03/2023 2:39 pm, Oleksii Kurochko wrote:
> > The structure holds information about:
> > 1. linker start/end address
> > 2. load start/end address
> > 
> > Also the patch introduces offsets for boot information structure
> > members to access them in assembly code.
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> > Changes in V5:
> >  * the patch was introduced in the current patch series (V5)
> > ---
> >  xen/arch/riscv/include/asm/boot-info.h | 15 +++++++++++++++
> >  xen/arch/riscv/riscv64/asm-offsets.c   |  3 +++
> >  2 files changed, 18 insertions(+)
> >  create mode 100644 xen/arch/riscv/include/asm/boot-info.h
> > 
> > diff --git a/xen/arch/riscv/include/asm/boot-info.h
> > b/xen/arch/riscv/include/asm/boot-info.h
> > new file mode 100644
> > index 0000000000..cda3d278f5
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/boot-info.h
> > @@ -0,0 +1,15 @@
> > +#ifndef _ASM_BOOT_INFO_H
> > +#define _ASM_BOOT_INFO_H
> > +
> > +extern struct boot_info {
> > +    unsigned long linker_start;
> > +    unsigned long linker_end;
> > +    unsigned long load_start;
> > +    unsigned long load_end;
> > +} boot_info;
> > +
> > +/* LINK_TO_LOAD() and LOAD_TO_LINK() works only when MMU isn't
> > enabled. */
> > +#define LINK_TO_LOAD(addr) ((addr) - boot_info.linker_start +
> > boot_info.load_start)
> > +#define LOAD_TO_LINK(addr) ((addr) - boot_info.load_start +
> > boot_info.linker_start)
> > +
> > +#endif
> > \ No newline at end of file
> 
> As a minor point, you should have newlines at the end of each file.
> 
> However, I'm not sure boot info like this is a clever move.  You're
> creating a 3rd different way of doing something which should be
> entirely
> common.  Some details are in
> https://lore.kernel.org/xen-devel/115c178b-f0a7-cf6e-3e33-e6aa49b17baf@srcf.net/
> and note how many errors I already found in x86 and ARM.
> 
In the link above you mentioned that:
  Reviewing its usage shows that ARM is broken when trying to handle
  BUG/ASSERT in livepatches, because they don't check is_patch() on the
  message target.
Check is_patch() will be added to ARM implementation after generic bug
implementation will be merged: 
https://lore.kernel.org/xen-devel/2afad972cd8da98dcb0ba509ba29ff239dc47cd0.1678900513.git.oleksii.kurochko@gmail.com/
> Perhaps its time to dust that plan off again.  As Jan says, there's
> _start and _end (or future variations therefore), and xen_phys_start
> which is all that ought to exist in order to build the common
> functionality.
I am unsure that I understand why the introduction of boot_info is a
bad idea.
Basically, it is only a wrapper/helper over _start, _end, and
xen_phys_start ( it is not introduced explicitly as taking into account
that access of _start will be PC-relative it is equal to xen_phys_start
).

~ Oleksii

Re: [PATCH v5 3/7] xen/riscv: introduce dummy <asm/bug.h>

[Oleksii]

On Wed, 2023-03-22 at 11:27 +0100, Jan Beulich wrote:
> On 22.03.2023 11:09, Oleksii wrote:
> > On Tue, 2023-03-21 at 17:21 +0000, Julien Grall wrote:
> > > On 16/03/2023 14:39, Oleksii Kurochko wrote:
> > > > <xen/lib.h> will be used in the patch "xen/riscv: introduce
> > > > decode_cause() stuff" and requires <asm/bug.h>
> > > > 
> > > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > > > ---
> > > > Changes in V5:
> > > >   * the patch was introduced in the current patch series (V5)
> > > > ---
> > > >   xen/arch/riscv/include/asm/bug.h | 10 ++++++++++
> > > >   1 file changed, 10 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..e8b1e40823
> > > > --- /dev/null
> > > > +++ b/xen/arch/riscv/include/asm/bug.h
> > > > @@ -0,0 +1,10 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > +/*
> > > > + * Copyright (C) 2012 Regents of the University of California
> > > > + * Copyright (C) 2021-2023 Vates
> > > 
> > > I am a bit puzzled with those copyright given the header is
> > > empty.
> > > 
> > > But is there any reason this can't be folded in #6 or part of #6
> > > moved 
> > > forward?
> > Initially it was folded in #6 but in this case a build will be
> > failed
> > after introduction of #5 as <asm/bug.h> is needed for <xen/lib.h>
> 
> But what about the other option Julien mentioned, moving ahead the
> later "filling" of asm/bug.h, so it wouldn't be introduced empty and
> then (almost immediately) touched again to actually populate it?
I think I can move the content of <asm/bug.h> from #6 to the current
patch.

~ Oleksii

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

[Oleksii]

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 5/7] xen/riscv: introduce trap_init()

[Oleksii]

On Wed, 2023-03-22 at 12:14 +0000, Julien Grall wrote:
> 
> 
> On 22/03/2023 11:33, Oleksii wrote:
> > Hi Julien,
> 
> Hi Oleksii,
> 
> > 
> > On Tue, 2023-03-21 at 17:42 +0000, Julien Grall wrote:
> > > Hi Oleksii,
> > > 
> > > On 16/03/2023 14:39, Oleksii Kurochko wrote:
> > > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > > > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> > > > ---
> > > > 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             | 5 +++++
> > > >    xen/arch/riscv/traps.c             | 7 +++++++
> > > >    3 files changed, 13 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 36556eb779..b44d105b5f 100644
> > > > --- a/xen/arch/riscv/setup.c
> > > > +++ b/xen/arch/riscv/setup.c
> > > > @@ -3,7 +3,9 @@
> > > >    #include <xen/kernel.h>
> > > >    
> > > >    #include <asm/boot-info.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]
> > > > @@ -32,7 +34,10 @@ void __init noreturn start_xen(unsigned long
> > > > bootcpu_id,
> > > >    
> > > >        fill_boot_info();
> > > >    
> > > > +    trap_init();
> > > > +
> > > >        early_printk("All set up\n");
> > > > +
> > > >        for ( ;; )
> > > >            asm volatile ("wfi");
> > > >    
> > > > diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
> > > > index 8a1529e0c5..581f34efbc 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;
> > > 
> > > It is not super clear to me whether this is going to store the
> > > virtual
> > > or physical address.
> > Actually it is going to store both the virtual and physical
> > address.
> > Depending on if MMU is enabled or not.
> 
> I think some comment in the code would be really good because this
> is...
> 
> > > 
> > > Depending on the answer, the next would be whether the value
> > > would
> > > still
> > > be valid after the MMU is turned on?
> > It would still be valid because for addr will be generated PC-
> > relative
> > address.
> 
> ... not clear to me what would guarantee that Xen is compiled with 
> -noPIE. Is the cmodel?
There is a patch:
https://lore.kernel.org/xen-devel/2785518800dce64fafb3096480a5ae4c4e026bcb.1678970065.git.oleksii.kurochko@gmail.com/
Which guarantees that Xen is complied with -noPIE.

The cmodel determines which software addressing mode is used, and,
therefore, what constraints are enforced on the linked program.

> 
> A suggestion for the top of the function:
> 
> "Initialize the trap handling. This is called twice (before and after
> the MMU)."
> 
> And for on top of 'addr', I would add:
> 
> "When the MMU is off, this will be a physical address otherwise it
> would 
> be a virtual address. This is guarantee because [fill the blank]".
Thanks for the recommendations.
I will take them into account.

~ Oleksii

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

[Jan Beulich]

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 5/7] xen/riscv: introduce trap_init()

[Julien Grall]

Hi Oleksii,

On 22/03/2023 13:40, Oleksii wrote:
> On Wed, 2023-03-22 at 12:14 +0000, Julien Grall wrote:
>>
>>
>> On 22/03/2023 11:33, Oleksii wrote:
>>> Hi Julien,
>>
>> Hi Oleksii,
>>
>>>
>>> On Tue, 2023-03-21 at 17:42 +0000, Julien Grall wrote:
>>>> Hi Oleksii,
>>>>
>>>> On 16/03/2023 14:39, Oleksii Kurochko wrote:
>>>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>>>>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>>>>> ---
>>>>> 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             | 5 +++++
>>>>>     xen/arch/riscv/traps.c             | 7 +++++++
>>>>>     3 files changed, 13 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 36556eb779..b44d105b5f 100644
>>>>> --- a/xen/arch/riscv/setup.c
>>>>> +++ b/xen/arch/riscv/setup.c
>>>>> @@ -3,7 +3,9 @@
>>>>>     #include <xen/kernel.h>
>>>>>     
>>>>>     #include <asm/boot-info.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]
>>>>> @@ -32,7 +34,10 @@ void __init noreturn start_xen(unsigned long
>>>>> bootcpu_id,
>>>>>     
>>>>>         fill_boot_info();
>>>>>     
>>>>> +    trap_init();
>>>>> +
>>>>>         early_printk("All set up\n");
>>>>> +
>>>>>         for ( ;; )
>>>>>             asm volatile ("wfi");
>>>>>     
>>>>> diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
>>>>> index 8a1529e0c5..581f34efbc 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;
>>>>
>>>> It is not super clear to me whether this is going to store the
>>>> virtual
>>>> or physical address.
>>> Actually it is going to store both the virtual and physical
>>> address.
>>> Depending on if MMU is enabled or not.
>>
>> I think some comment in the code would be really good because this
>> is...
>>
>>>>
>>>> Depending on the answer, the next would be whether the value
>>>> would
>>>> still
>>>> be valid after the MMU is turned on?
>>> It would still be valid because for addr will be generated PC-
>>> relative
>>> address.
>>
>> ... not clear to me what would guarantee that Xen is compiled with
>> -noPIE. Is the cmodel?

There is a missing "given" after "that".

> There is a patch:
> https://lore.kernel.org/xen-devel/2785518800dce64fafb3096480a5ae4c4e026bcb.1678970065.git.oleksii.kurochko@gmail.com/
> Which guarantees that Xen is complied with -noPIE.
I am a bit puzzled with what you wrote. From my understanding, with 
-noPIE, the compiler would be free to use absolute address rather than 
pc-relative one. Do you have any pointer to documentation that would 
back your reasoning?

I have already mentioned before, but I think it would really useful if 
you start adding more documentation (in particular of such behavior) in 
the code or docs/ (for more wide). This would help everyone to 
understand how everything is meant to work.

Cheers,

-- 
Julien Grall

Re: [PATCH v5 5/7] xen/riscv: introduce trap_init()

[Jan Beulich]

On 22.03.2023 14:51, Julien Grall wrote:
> I am a bit puzzled with what you wrote. From my understanding, with 
> -noPIE, the compiler would be free to use absolute address rather than 
> pc-relative one. Do you have any pointer to documentation that would 
> back your reasoning?

It might for RV32 (using lui), but for 64-bit the ISA simply doesn't
have any halfway efficient means to load addresses in other than a
PC-relative manner [1]. -nopie really suppresses the compiler emitting
code going through .got (which then indeed would mean using absolute
addresses). I understand that at least for now RV32 isn't really of
interest; if it were, I guess the concern would be more significant.

Jan

[1] There aren't even suitable relocations to express such in ELF,
because you'd need to have a way to get at the top 32 bits of an
address. The only alternative would be .got-like indirection without
actually using .got.

Re: [PATCH v5 5/7] xen/riscv: introduce trap_init()

[Oleksii]

On Wed, 2023-03-22 at 13:51 +0000, Julien Grall wrote:
> Hi Oleksii,
> 
> On 22/03/2023 13:40, Oleksii wrote:
> > On Wed, 2023-03-22 at 12:14 +0000, Julien Grall wrote:
> > > 
> > > 
> > > On 22/03/2023 11:33, Oleksii wrote:
> > > > Hi Julien,
> > > 
> > > Hi Oleksii,
> > > 
> > > > 
> > > > On Tue, 2023-03-21 at 17:42 +0000, Julien Grall wrote:
> > > > > Hi Oleksii,
> > > > > 
> > > > > On 16/03/2023 14:39, Oleksii Kurochko wrote:
> > > > > > Signed-off-by: Oleksii Kurochko
> > > > > > <oleksii.kurochko@gmail.com>
> > > > > > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> > > > > > ---
> > > > > > 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             | 5 +++++
> > > > > >     xen/arch/riscv/traps.c             | 7 +++++++
> > > > > >     3 files changed, 13 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 36556eb779..b44d105b5f 100644
> > > > > > --- a/xen/arch/riscv/setup.c
> > > > > > +++ b/xen/arch/riscv/setup.c
> > > > > > @@ -3,7 +3,9 @@
> > > > > >     #include <xen/kernel.h>
> > > > > >     
> > > > > >     #include <asm/boot-info.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]
> > > > > > @@ -32,7 +34,10 @@ void __init noreturn start_xen(unsigned
> > > > > > long
> > > > > > bootcpu_id,
> > > > > >     
> > > > > >         fill_boot_info();
> > > > > >     
> > > > > > +    trap_init();
> > > > > > +
> > > > > >         early_printk("All set up\n");
> > > > > > +
> > > > > >         for ( ;; )
> > > > > >             asm volatile ("wfi");
> > > > > >     
> > > > > > diff --git a/xen/arch/riscv/traps.c
> > > > > > b/xen/arch/riscv/traps.c
> > > > > > index 8a1529e0c5..581f34efbc 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;
> > > > > 
> > > > > It is not super clear to me whether this is going to store
> > > > > the
> > > > > virtual
> > > > > or physical address.
> > > > Actually it is going to store both the virtual and physical
> > > > address.
> > > > Depending on if MMU is enabled or not.
> > > 
> > > I think some comment in the code would be really good because
> > > this
> > > is...
> > > 
> > > > > 
> > > > > Depending on the answer, the next would be whether the value
> > > > > would
> > > > > still
> > > > > be valid after the MMU is turned on?
> > > > It would still be valid because for addr will be generated PC-
> > > > relative
> > > > address.
> > > 
> > > ... not clear to me what would guarantee that Xen is compiled
> > > with
> > > -noPIE. Is the cmodel?
> 
> There is a missing "given" after "that".
> 
> > There is a patch:
> > https://lore.kernel.org/xen-devel/2785518800dce64fafb3096480a5ae4c4e026bcb.1678970065.git.oleksii.kurochko@gmail.com/
> > Which guarantees that Xen is complied with -noPIE.
> I am a bit puzzled with what you wrote. From my understanding, with 
> -noPIE, the compiler would be free to use absolute address rather
> than 
> pc-relative one. Do you have any pointer to documentation that would 
> back your reasoning?

https://riscv.org/wp-content/uploads/2017/05/riscv-spec-v2.2.pdf
If look at pseudoinstructions (p110 in pdf) which work with addresses
they are always pc-relative ( they all use AUIPC instruction ). The
only question is that if .got is used or not ( which depends on
pie,pic, etc... )
> 
> I have already mentioned before, but I think it would really useful
> if 
> you start adding more documentation (in particular of such behavior)
> in 
> the code or docs/ (for more wide). This would help everyone to 
> understand how everything is meant to work.
Sure. I will add more documentation for such kind of code.

~ Oleksii

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

[Oleksii]

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

[Jan Beulich]

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