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

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

[Oleksii Kurochko]

The patch series is based on:
    enable MMU for RISC-V [1]
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.1685027257.git.oleksii.kurochko@gmail.com/T/#t

---
Changes in V6:
 - Update the cover letter message: the patch set is based on MMU patch series.
 - Introduce new patch with temporary printk functionality. ( it will be
   removed when Xen common code will be ready )
 - Change early_printk() to printk().
 - Remove usage of LINK_TO_LOAD() due to the MMU being enabled first.
 - Add additional explanatory comments.
 - Remove patch "xen/riscv: initialize boot_info structure" from the patch
   series.
---
Changes in V5:
 - Rebase on top of [1] and [2]
 - Add new patch which introduces stub for <asm/bug.h> to keep Xen compilable
   as in the patch [xen/riscv: introduce decode_cause() stuff] is used
   header <xen/lib.h> which requires <asm/bug.h>.
 - Remove <xen/error.h> from riscv/traps/c as nothing would require
   inclusion.
 - decode_reserved_interrupt_cause(), decode_interrupt_cause(),
   decode_cause, do_unexpected_trap() were made as static they are expected
   to be used only in traps.c
 - Remove "#include <xen/types.h>" from <asm/bug.h> as there is no any need in it anymore
 - Update macros GET_INSN_LENGTH: remove UL and 'unsigned int len;' from it
 - Remove " include <xen/bug.h>" from risc/setup.c. it is not needed in the current version of
   the patch
 - change an argument type from vaddr_t to uint32_t for is_valid_bugaddr and introduce 
   read_instr() to read instruction properly as the length of qinstruction can be
   either 32 or 16 bits.
 - Code style fixes
 - update the comments before do_bug_frame() in riscv/trap.c
 - [[PATCH v4 5/5] automation: modify RISC-V smoke test ] was dropped as it was provided
   more simple solution by Andrew.  CI: Simplify RISCV smoke testing
 - Refactor is_valid_bugaddr() function.
 - 2 new patches ([PATCH v5 {1-2}/7]) were introduced, the goal of which is to recalculate
   addresses used in traps.c, which can be linker time relative. It is needed as we don't
   have enabled MMU yet.
---
Changes in V4:
  - Rebase the patch series on top of new version of [introduce generic
    implementation of macros from bug.h] patch series.
  - Update the cover letter message as 'Early printk' was merged and
    the current one patch series is based only on [introduce generic
    implementation of macros from bug.h] which hasn't been commited yet.
  - The following patches of the patch series were merged to staging:
      [PATCH v3 01/14] xen/riscv: change ISA to r64G
      [PATCH v3 02/14] xen/riscv: add <asm/asm.h> header
      [PATCH v3 03/14] xen/riscv: add <asm/riscv_encoding.h header
      [PATCH v3 04/14] xen/riscv: add <asm/csr.h> header
      [PATCH v3 05/14] xen/riscv: introduce empty <asm/string.h>
      [PATCH v3 06/14] xen/riscv: introduce empty <asm/cache.h>
      [PATCH v3 07/14] xen/riscv: introduce exception context
      [PATCH v3 08/14] xen/riscv: introduce exception handlers implementation
      [PATCH v3 10/14] xen/riscv: mask all interrupts
  - Fix addressed comments in xen-devel mailing list.

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

Oleksii Kurochko (6):
  xen/riscv: introduce temporary printk stuff
  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/early_printk.c      | 168 ++++++++++++++++++++
 xen/arch/riscv/include/asm/bug.h   |  38 +++++
 xen/arch/riscv/include/asm/traps.h |   1 +
 xen/arch/riscv/setup.c             |  20 +++
 xen/arch/riscv/traps.c             | 238 ++++++++++++++++++++++++++++-
 xen/arch/riscv/xen.lds.S           |  10 ++
 6 files changed, 474 insertions(+), 1 deletion(-)
 create mode 100644 xen/arch/riscv/include/asm/bug.h

-- 
2.40.1

[PATCH v6 1/6] xen/riscv: introduce temporary printk stuff

[Oleksii Kurochko]

The patch introdcuces printk related stuff which should be deleted
after Xen common code will be available.

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

diff --git a/xen/arch/riscv/early_printk.c b/xen/arch/riscv/early_printk.c
index 610c814f54..7d24932a35 100644
--- a/xen/arch/riscv/early_printk.c
+++ b/xen/arch/riscv/early_printk.c
@@ -40,3 +40,171 @@ void early_printk(const char *str)
         str++;
     }
 }
+
+/*
+ * The following #if 1 ... #endif should be removed after printk
+ * and related stuff are ready.
+ */
+#if 1
+
+#include <xen/stdarg.h>
+#include <xen/string.h>
+
+/**
+ * strlen - Find the length of a string
+ * @s: The string to be sized
+ */
+size_t (strlen)(const char * s)
+{
+    const char *sc;
+
+    for (sc = s; *sc != '\0'; ++sc)
+        /* nothing */;
+    return sc - s;
+}
+
+/**
+ * memcpy - Copy one area of memory to another
+ * @dest: Where to copy to
+ * @src: Where to copy from
+ * @count: The size of the area.
+ *
+ * You should not use this function to access IO space, use memcpy_toio()
+ * or memcpy_fromio() instead.
+ */
+void *(memcpy)(void *dest, const void *src, size_t count)
+{
+    char *tmp = (char *) dest, *s = (char *) src;
+
+    while (count--)
+        *tmp++ = *s++;
+
+    return dest;
+}
+
+int vsnprintf(char* str, size_t size, const char* format, va_list args)
+{
+    size_t i = 0; /* Current position in the output string */
+    size_t written = 0; /* Total number of characters written */
+    char* dest = str;
+
+    while ( format[i] != '\0' && written < size - 1 )
+    {
+        if ( format[i] == '%' )
+        {
+            i++;
+
+            if ( format[i] == '\0' )
+                break;
+
+            if ( format[i] == '%' )
+            {
+                if ( written < size - 1 )
+                {
+                    dest[written] = '%';
+                    written++;
+                }
+                i++;
+                continue;
+            }
+
+            /*
+             * Handle format specifiers.
+             * For simplicity, only %s and %d are implemented here.
+             */
+
+            if ( format[i] == 's' )
+            {
+                char* arg = va_arg(args, char*);
+                size_t arglen = strlen(arg);
+
+                size_t remaining = size - written - 1;
+
+                if ( arglen > remaining )
+                    arglen = remaining;
+
+                memcpy(dest + written, arg, arglen);
+
+                written += arglen;
+                i++;
+            }
+            else if ( format[i] == 'd' )
+            {
+                int arg = va_arg(args, int);
+
+                /* Convert the integer to string representation */
+                char numstr[32]; /* Assumes a maximum of 32 digits */
+                int numlen = 0;
+                int num = arg;
+                size_t remaining;
+
+                if ( arg < 0 )
+                {
+                    if ( written < size - 1 )
+                    {
+                        dest[written] = '-';
+                        written++;
+                    }
+
+                    num = -arg;
+                }
+
+                do
+                {
+                    numstr[numlen] = '0' + num % 10;
+                    num = num / 10;
+                    numlen++;
+                } while ( num > 0 );
+
+                /* Reverse the string */
+                for (int j = 0; j < numlen / 2; j++)
+                {
+                    char tmp = numstr[j];
+                    numstr[j] = numstr[numlen - 1 - j];
+                    numstr[numlen - 1 - j] = tmp;
+                }
+
+                remaining = size - written - 1;
+
+                if ( numlen > remaining )
+                    numlen = remaining;
+
+                memcpy(dest + written, numstr, numlen);
+
+                written += numlen;
+                i++;
+            }
+        }
+        else
+        {
+            if ( written < size - 1 )
+            {
+                dest[written] = format[i];
+                written++;
+            }
+            i++;
+        }
+    }
+
+    if ( size > 0 )
+        dest[written] = '\0';
+
+    return written;
+}
+
+void printk(const char *format, ...)
+{
+    static char buf[1024];
+
+    va_list args;
+    va_start(args, format);
+
+    (void)vsnprintf(buf, sizeof(buf), format, args);
+
+    early_printk(buf);
+
+    va_end(args);
+}
+
+#endif
+
-- 
2.40.1

[PATCH v6 2/6] 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 V6:
 - Nothing changed. Only rebase.
---
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.40.1

[PATCH v6 3/6] 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 V6:
 - Remove usage of LINK_TO_LOAD() due to the MMU being enabled first.
 - Change early_printk() to printk()
---
Changes in V5:
  - Remove <xen/error.h> from riscv/traps/c as nothing would require
    inclusion.
  - decode_reserved_interrupt_cause(), decode_interrupt_cause(), decode_cause, do_unexpected_trap()
    were made as static they are expected to be used only in traps.c
  - use LINK_TO_LOAD() for addresses which can be linker time relative.
---
Changes in V4:
  - fix string in decode_reserved_interrupt_cause()
---
Changes in V3:
  - Nothing changed
---
Changes in V2:
  - Make decode_trap_cause() more optimization friendly.
  - Merge the pathc which introduces do_unexpected_trap() to the current one.
---
 xen/arch/riscv/traps.c | 84 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 83 insertions(+), 1 deletion(-)

diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
index ccd3593f5a..ea1012e83e 100644
--- a/xen/arch/riscv/traps.c
+++ b/xen/arch/riscv/traps.c
@@ -4,10 +4,92 @@
  *
  * RISC-V Trap handlers
  */
+
+#include <xen/lib.h>
+
+#include <asm/csr.h>
+#include <asm/early_printk.h>
 #include <asm/processor.h>
 #include <asm/traps.h>
 
-void do_trap(struct cpu_user_regs *cpu_regs)
+static const char *decode_trap_cause(unsigned long cause)
+{
+    static const char *const trap_causes[] = {
+        [CAUSE_MISALIGNED_FETCH] = "Instruction Address Misaligned",
+        [CAUSE_FETCH_ACCESS] = "Instruction Access Fault",
+        [CAUSE_ILLEGAL_INSTRUCTION] = "Illegal Instruction",
+        [CAUSE_BREAKPOINT] = "Breakpoint",
+        [CAUSE_MISALIGNED_LOAD] = "Load Address Misaligned",
+        [CAUSE_LOAD_ACCESS] = "Load Access Fault",
+        [CAUSE_MISALIGNED_STORE] = "Store/AMO Address Misaligned",
+        [CAUSE_STORE_ACCESS] = "Store/AMO Access Fault",
+        [CAUSE_USER_ECALL] = "Environment Call from U-Mode",
+        [CAUSE_SUPERVISOR_ECALL] = "Environment Call from S-Mode",
+        [CAUSE_MACHINE_ECALL] = "Environment Call from M-Mode",
+        [CAUSE_FETCH_PAGE_FAULT] = "Instruction Page Fault",
+        [CAUSE_LOAD_PAGE_FAULT] = "Load Page Fault",
+        [CAUSE_STORE_PAGE_FAULT] = "Store/AMO Page Fault",
+        [CAUSE_FETCH_GUEST_PAGE_FAULT] = "Instruction Guest Page Fault",
+        [CAUSE_LOAD_GUEST_PAGE_FAULT] = "Load Guest Page Fault",
+        [CAUSE_VIRTUAL_INST_FAULT] = "Virtualized Instruction Fault",
+        [CAUSE_STORE_GUEST_PAGE_FAULT] = "Guest Store/AMO Page Fault",
+    };
+
+    if ( cause < ARRAY_SIZE(trap_causes) && trap_causes[cause] )
+        return trap_causes[cause];
+    return "UNKNOWN";
+}
+
+static const char *decode_reserved_interrupt_cause(unsigned long irq_cause)
+{
+    switch ( irq_cause )
+    {
+    case IRQ_M_SOFT:
+        return "M-mode Software Interrupt";
+    case IRQ_M_TIMER:
+        return "M-mode TIMER Interrupt";
+    case IRQ_M_EXT:
+        return "M-mode External Interrupt";
+    default:
+        return "UNKNOWN IRQ type";
+    }
+}
+
+static const char *decode_interrupt_cause(unsigned long cause)
+{
+    unsigned long irq_cause = cause & ~CAUSE_IRQ_FLAG;
+
+    switch ( irq_cause )
+    {
+    case IRQ_S_SOFT:
+        return "Supervisor Software Interrupt";
+    case IRQ_S_TIMER:
+        return "Supervisor Timer Interrupt";
+    case IRQ_S_EXT:
+        return "Supervisor External Interrupt";
+    default:
+        return decode_reserved_interrupt_cause(irq_cause);
+    }
+}
+
+static const char *decode_cause(unsigned long cause)
+{
+    if ( cause & CAUSE_IRQ_FLAG )
+        return decode_interrupt_cause(cause);
+
+    return decode_trap_cause(cause);
+}
+
+static void do_unexpected_trap(const struct cpu_user_regs *regs)
 {
+    unsigned long cause = csr_read(CSR_SCAUSE);
+
+    printk("Unhandled exception: %s\n", decode_cause(cause));
+
     die();
 }
+
+void do_trap(struct cpu_user_regs *cpu_regs)
+{
+    do_unexpected_trap(cpu_regs);
+}
-- 
2.40.1

[PATCH v6 4/6] 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 V6:
 - trap_init() is now called after enabling the MMU.
 - Add additional explanatory comments.
---
Changes in V5:
  - Nothing changed
---
Changes in V4:
  - Nothing changed
---
Changes in V3:
  - Nothing changed
---
Changes in V2:
  - Rename setup_trap_handler() to trap_init().
  - Add Reviewed-by to the commit message.
---
 xen/arch/riscv/include/asm/traps.h |  1 +
 xen/arch/riscv/setup.c             |  3 +++
 xen/arch/riscv/traps.c             | 25 +++++++++++++++++++++++++
 3 files changed, 29 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 845d18d86f..1cae0e5ccc 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -3,6 +3,7 @@
 
 #include <asm/early_printk.h>
 #include <asm/mm.h>
+#include <asm/traps.h>
 
 /* Xen stack for bringing up the first CPU. */
 unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
@@ -25,6 +26,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
 
 void __init noreturn cont_after_mmu_is_enabled(void)
 {
+    trap_init();
+
     early_printk("All set up\n");
 
     for ( ;; )
diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
index ea1012e83e..48c1059954 100644
--- a/xen/arch/riscv/traps.c
+++ b/xen/arch/riscv/traps.c
@@ -12,6 +12,31 @@
 #include <asm/processor.h>
 #include <asm/traps.h>
 
+#define cast_to_bug_frame(addr) \
+    (const struct bug_frame *)(addr)
+
+/*
+ * Initialize the trap handling.
+ *
+ * The function is called after MMU is enabled.
+ */
+void trap_init(void)
+{
+    /*
+     * When the MMU is off, addr varialbe will be a physical address otherwise
+     * it would be a virtual address.
+     *
+     * It will work fine as:
+     *  - access to addr is PC-relative.
+     *  - -nopie is used. -nopie really suppresses the compiler emitting
+     *    code going through .got (which then indeed would mean using absolute
+     *    addresses).
+     */
+    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.40.1

[PATCH v6 5/6] 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 V6:
  - Avoid LINK_TO_LOAD() as bug.h functionality expected to be used
    after MMU is enabled.
  - Change early_printk() to printk()
---
Changes in V5:
  - Remove "#include <xen/types.h>" from <asm/bug.h> as there is no any need in it anymore
  - Update macros GET_INSN_LENGTH: remove UL and 'unsigned int len;' from it
  - Remove " include <xen/bug.h>" from risc/setup.c. it is not needed in the current version of
    the patch
  - change an argument type from vaddr_t to uint32_t for is_valid_bugaddr and introduce read_instr() to
    read instruction properly as the length of qinstruction can be either 32 or 16 bits.
  - Code style fixes
  - update the comments before do_bug_frame() in riscv/trap.c
  - Refactor is_valid_bugaddr() function.
  - introduce macros cast_to_bug_frame(addr) to hide casts.
  - use LINK_TO_LOAD() for addresses which are linker time relative.
---
Changes in V4:
  - Updates in RISC-V's <asm/bug.h>:
    * Add explanatory comment about why there is only defined for 32-bits length
      instructions and 16/32-bits BUG_INSN_{16,32}.
    * Change 'unsigned long' to 'unsigned int' inside GET_INSN_LENGTH().
    * Update declaration of is_valid_bugaddr(): switch return type from int to bool
      and the argument from 'unsigned int' to 'vaddr'.
  - Updates in RISC-V's traps.c:
    * replace /xen and /asm includes 
    * update definition of is_valid_bugaddr():switch return type from int to bool
      and the argument from 'unsigned int' to 'vaddr'. Code style inside function
      was updated too.
    * do_bug_frame() refactoring:
      * local variables start and bug became 'const struct bug_frame'
      * bug_frames[] array became 'static const struct bug_frame[] = ...'
      * remove all casts
      * remove unneeded comments and add an explanatory comment that the do_bug_frame()
        will be switched to a generic one.
    * do_trap() refactoring:
      * read 16-bits value instead of 32-bits as compressed instruction can
        be used and it might happen than only 16-bits may be accessible.
      * code style updates
      * re-use instr variable instead of re-reading instruction.
  - Updates in setup.c:
    * add blank line between xen/ and asm/ includes.
---
Changes in V3:
  - Rebase the patch "xen/riscv: introduce an implementation of macros
    from <asm/bug.h>" on top of patch series [introduce generic implementation
    of macros from bug.h]
---
Changes in V2:
  - Remove __ in define namings
  - Update run_in_exception_handler() with
    register void *fn_ asm(__stringify(BUG_FN_REG)) = (fn);
  - Remove bug_instr_t type and change it's usage to uint32_t
---
 xen/arch/riscv/include/asm/bug.h |  28 +++++++
 xen/arch/riscv/traps.c           | 129 +++++++++++++++++++++++++++++++
 xen/arch/riscv/xen.lds.S         |  10 +++
 3 files changed, 167 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/traps.c b/xen/arch/riscv/traps.c
index 48c1059954..535fb058e1 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/csr.h>
@@ -114,7 +116,134 @@ static void do_unexpected_trap(const struct cpu_user_regs *regs)
     die();
 }
 
+void show_execution_state(const struct cpu_user_regs *regs)
+{
+    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:
+        printk("Xen WARN at %s:%d\n", filename, lineno);
+
+        show_execution_state(regs);
+
+        goto end;
+
+    case BUGFRAME_bug:
+        printk("Xen BUG at %s:%d\n", filename, lineno);
+
+        show_execution_state(regs);
+
+        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);
+
+        printk("Assertion %s failed at %s:%d\n", predicate, filename, lineno);
+
+        show_execution_state(regs);
+
+        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 df71d31e17..0412493911 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.40.1

[PATCH v6 6/6] 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 V6:
  - Nothing changed
---
Changes in V5:
  - Nothing changed
---
Changes in V4:
  - Add Acked-by: Alistair Francis <alistair.francis@wdc.com>
---
Changes in V3:
  - Nothing changed
---
Changes in V2:
  - Nothing changed
---
 xen/arch/riscv/setup.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index 1cae0e5ccc..481d88249d 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>
 
@@ -9,6 +10,20 @@
 unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
     __aligned(STACK_SIZE);
 
+static void test_run_in_exception(struct cpu_user_regs *regs)
+{
+    early_printk("If you see this message, ");
+    early_printk("run_in_exception_handler is most likely working\n");
+}
+
+static void test_macros_from_bug_h(void)
+{
+    run_in_exception_handler(test_run_in_exception);
+    WARN();
+    early_printk("If you see this message, ");
+    early_printk("WARN is most likely working\n");
+}
+
 void __init noreturn start_xen(unsigned long bootcpu_id,
                                paddr_t dtb_addr)
 {
@@ -28,6 +43,8 @@ void __init noreturn cont_after_mmu_is_enabled(void)
 {
     trap_init();
 
+    test_macros_from_bug_h();
+
     early_printk("All set up\n");
 
     for ( ;; )
-- 
2.40.1

Re: [PATCH v6 4/6] xen/riscv: introduce trap_init()

[Jan Beulich]

On 29.05.2023 14:13, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/traps.c
> +++ b/xen/arch/riscv/traps.c
> @@ -12,6 +12,31 @@
>  #include <asm/processor.h>
>  #include <asm/traps.h>
>  
> +#define cast_to_bug_frame(addr) \
> +    (const struct bug_frame *)(addr)

I can't find a use for this; should it be dropped or moved to some
later patch? In any event, if ti's intended to survive, it needs yet
another pair of parentheses.

> +/*
> + * Initialize the trap handling.
> + *
> + * The function is called after MMU is enabled.
> + */
> +void trap_init(void)

Is this going to be used for secondary processors as well? If not,
it will want to be __init.

> +{
> +    /*
> +     * When the MMU is off, addr varialbe will be a physical address otherwise
> +     * it would be a virtual address.
> +     *
> +     * It will work fine as:
> +     *  - access to addr is PC-relative.
> +     *  - -nopie is used. -nopie really suppresses the compiler emitting
> +     *    code going through .got (which then indeed would mean using absolute
> +     *    addresses).
> +     */

Is all of this comment still relevant not that you're running with
the MMU already enabled.

Jan

> +    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[] = {

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

[Jan Beulich]

On 29.05.2023 14:13, Oleksii Kurochko wrote:
> --- 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 ).

This, at least to some degree, looks to contradict ...

> + * 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 )

... this. Plus there already is CONFIG_RISCV_ISA_C, so compressed insns
can very well be used in Xen.

> @@ -114,7 +116,134 @@ static void do_unexpected_trap(const struct cpu_user_regs *regs)
>      die();
>  }
>  
> +void show_execution_state(const struct cpu_user_regs *regs)
> +{
> +    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.

What is this comment about? I don't think I understand what it says
needs doing.

> + * 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)

While it's going to be the maintainers to judge, I continue to be
unconvinced that introducing copies of common functions (also in
patch 1) is a good idea.

> +{
> +    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[] = {

Nit: * and blank want to swap places. I would also expect another
"const".

> +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;
> +}

As long as this function is only used on Xen code, it's kind of okay.
There you/we control whether code can change behind our backs. But as
soon as you might use this on guest code, the double read is going to
be a problem (I think; I wonder how hardware is supposed to deal with
the situation: Maybe they indeed fetch in 16-bit quantities?).

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

Nit: There looks to be an off-by-one in how you indent your addition
(except for the comment).

Jan

Re: [PATCH v6 4/6] xen/riscv: introduce trap_init()

[Oleksii]

On Tue, 2023-05-30 at 17:44 +0200, Jan Beulich wrote:
> On 29.05.2023 14:13, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/traps.c
> > +++ b/xen/arch/riscv/traps.c
> > @@ -12,6 +12,31 @@
> >  #include <asm/processor.h>
> >  #include <asm/traps.h>
> >  
> > +#define cast_to_bug_frame(addr) \
> > +    (const struct bug_frame *)(addr)
> 
> I can't find a use for this; should it be dropped or moved to some
> later patch? In any event, if ti's intended to survive, it needs yet
> another pair of parentheses.
You are right. It should be a part of the next patch.
Thanks.

> 
> > +/*
> > + * Initialize the trap handling.
> > + *
> > + * The function is called after MMU is enabled.
> > + */
> > +void trap_init(void)
> 
> Is this going to be used for secondary processors as well? If not,
> it will want to be __init.
I think I'll use it for secondary processors.

> 
> > +{
> > +    /*
> > +     * When the MMU is off, addr varialbe will be a physical
> > address otherwise
> > +     * it would be a virtual address.
> > +     *
> > +     * It will work fine as:
> > +     *  - access to addr is PC-relative.
> > +     *  - -nopie is used. -nopie really suppresses the compiler
> > emitting
> > +     *    code going through .got (which then indeed would mean
> > using absolute
> > +     *    addresses).
> > +     */
> 
> Is all of this comment still relevant not that you're running with
> the MMU already enabled.
Not really. I think comment above trap_init() function will be enough.
I'll remove this comment.

~ Oleksii

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

[Oleksii]

On Tue, 2023-05-30 at 18:00 +0200, Jan Beulich wrote:
> On 29.05.2023 14:13, Oleksii Kurochko wrote:
> > --- 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 ).
> 
> This, at least to some degree, looks to contradict ...
> 
> > + * 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 )
> 
> ... this. Plus there already is CONFIG_RISCV_ISA_C, so compressed
> insns
> can very well be used in Xen.
Thanks. You are right. The comment should be updated.

> 
> > @@ -114,7 +116,134 @@ static void do_unexpected_trap(const struct
> > cpu_user_regs *regs)
> >      die();
> >  }
> >  
> > +void show_execution_state(const struct cpu_user_regs *regs)
> > +{
> > +    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.
> 
> What is this comment about? I don't think I understand what it says
> needs doing.
I meant that it would be nice to introduce the second version of
early_printk() function which will take 'format', as printk() does.

But there is no any sense in this comment because all early_printk() in
do_bug_frame() were changed to printk().

Thereby I will update the comment.

> 
> > + * 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)
> 
> While it's going to be the maintainers to judge, I continue to be
> unconvinced that introducing copies of common functions (also in
> patch 1) is a good idea.
Generally I agree with you but as I mentioned before and in the comment
above the function do_bug_frame() the reason not to use generic
implementation of do_bug_frame() now as it will require to introduce
compilation of whole Xen's common code. ( there is no way to enable
just necessary parts for the current one function ). 

I think that after this patch series I'll introduce compilation of
Xen's common code and after it'll be merged do_bug_frame() can be
removed.

> 
> > +{
> > +    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[] = {
> 
> Nit: * and blank want to swap places. I would also expect another
> "const".
Thanks. I'll update that.

> 
> > +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;
> > +}
> 
> As long as this function is only used on Xen code, it's kind of okay.
> There you/we control whether code can change behind our backs. But as
> soon as you might use this on guest code, the double read is going to
> be a problem (I think; I wonder how hardware is supposed to deal with
> the situation: Maybe they indeed fetch in 16-bit quantities?).
I'll check how the hardware fetches instructions.

I am trying to figure out why the double-read can be a problem. It
looks pretty safe to read 16 bits ( they will be available for any
instruction length with the assumption that the minimal instruction
length is 16 ), then check the length of the instruction, and if it is
32-bit instruction, read it as uint32_t.
> 
> > --- 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)
> 
> Nit: There looks to be an off-by-one in how you indent your addition
> (except for the comment).
Thanks. One space is really absent...

~ Oleksii

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

[Jan Beulich]

On 31.05.2023 12:40, Oleksii wrote:
> On Tue, 2023-05-30 at 18:00 +0200, Jan Beulich wrote:
>> On 29.05.2023 14:13, Oleksii Kurochko wrote:
>>> +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;
>>> +}
>>
>> As long as this function is only used on Xen code, it's kind of okay.
>> There you/we control whether code can change behind our backs. But as
>> soon as you might use this on guest code, the double read is going to
>> be a problem (I think; I wonder how hardware is supposed to deal with
>> the situation: Maybe they indeed fetch in 16-bit quantities?).
> I'll check how the hardware fetches instructions.
> 
> I am trying to figure out why the double-read can be a problem. It
> looks pretty safe to read 16 bits ( they will be available for any
> instruction length with the assumption that the minimal instruction
> length is 16 ), then check the length of the instruction, and if it is
> 32-bit instruction, read it as uint32_t.

Simply consider what happens if a buggy or malicious entity changes the
code between the two reads. And not just with the detection of "break"
in mind that you use it for here.

Jan

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

[Jan Beulich]

On 31.05.2023 22:06, Oleksii wrote:
> On Tue, 2023-05-30 at 18:00 +0200, Jan Beulich wrote:
>>> +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;
>>> +}
>>
>> As long as this function is only used on Xen code, it's kind of okay.
>> There you/we control whether code can change behind our backs. But as
>> soon as you might use this on guest code, the double read is going to
>> be a problem
> Will it be enough to add a comment that read_instr() should be used
> only on Xen code? Or it is needed to introduce some lock?

A comment will do for now. A lock would be problematic: It won't help
when the function is used on non-Xen code, and since you use this in
exception handling you may deadlock unless you carefully use a
recursive lock.

>> (I think; I wonder how hardware is supposed to deal with
>> the situation: Maybe they indeed fetch in 16-bit quantities?).
> I thought that it reads amount of bytes corresponded to i-cache size
> and then the pipeline tracks whether an instruction is 16  or 32 bit.

And what if an insn spans a cacheline boundary?

Jan

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

[Oleksii]

On Thu, 2023-06-01 at 09:59 +0200, Jan Beulich wrote:
> On 31.05.2023 22:06, Oleksii wrote:
> > On Tue, 2023-05-30 at 18:00 +0200, Jan Beulich wrote:
> > > > +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;
> > > > +}
> > > 
> > > As long as this function is only used on Xen code, it's kind of
> > > okay.
> > > There you/we control whether code can change behind our backs.
> > > But as
> > > soon as you might use this on guest code, the double read is
> > > going to
> > > be a problem
> > Will it be enough to add a comment that read_instr() should be used
> > only on Xen code? Or it is needed to introduce some lock?
> 
> A comment will do for now. A lock would be problematic: It won't help
> when the function is used on non-Xen code, and since you use this in
> exception handling you may deadlock unless you carefully use a
> recursive lock.
Then I'll add a comment.

> 
> > > (I think; I wonder how hardware is supposed to deal with
> > > the situation: Maybe they indeed fetch in 16-bit quantities?).
> > I thought that it reads amount of bytes corresponded to i-cache
> > size
> > and then the pipeline tracks whether an instruction is 16  or 32
> > bit.
> 
> And what if an insn spans a cacheline boundary?
I think it is CPU specific, but your original assumption ( about 16-bit
fetching ) was probably right.

In RISC-V ISA doc [1] I found the following in chapter 1.2:
 The base RISC-V ISA has fixed-length 32-bit instructions that must be
naturally aligned on 32-bit boundaries. However, the standard RISC-V 
encoding scheme is designed to support ISA extensions with variable-
length instructions, where each instruction can be any number of 16-bit
instruction parcels in length, and parcels are naturally aligned on 16-
bit boundaries. The standard compressed ISA extension described in 
Chapter 12 reduces code size by providing compressed 16-bit
instructions and relaxes the alignment constraints to allow all
instructions (16 bit and 32 bit) to be aligned on any 16-bit boundary
to improve code density.

It sounds like h/w reads 16-bit and then based on the first bits
decides if it is needed to read more 16-bit parcels.

[1] https://riscv.org/wp-content/uploads/2017/05/riscv-spec-v2.2.pdf