[PATCH v12 0/3] RISCV basic exception handling implementation

[PATCH v12 0/3] RISCV basic exception handling implementation

[Oleksii Kurochko]

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

To verify that exception handling works well it was used macros
from <asm/bug.h> such as BUG/WARN/run_in_exception/assert_failed.

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

---
Changes in V12:
 - Drop patch series depenency from "Enable build of full Xen for RISC-V" as
   this depenency has been merged to staging.
 - Change BUG_INSTR from 'ebreak' to 0x0000 ( UNIMP instruction ).
 - add new patch with refactoring of decode_trap_cause().
 - The following patches were merged to staging:
   - [PATCH v11 1/5] xen/riscv: use printk() instead of early_printk()
   - [PATCH v11 2/5] xen/riscv: introduce decode_cause() stuff
   - [PATCH v11 3/5] xen/riscv: introduce trap_init()
 - All other changes please look in patch separately.
---
Changes in V10 - V11:
 - No generic changes to describe here. Please look at patch-specific changes.
---
Changes in V9:
 - Update the cover letter message.
 - s/early_printk/printk.
 - use GENERIC_BUG_FRAME instead of arch-specific implementation of do_bug_frame().
 - Rebase on top of current staging and riscv-full-xen-build-v14.
---
Changes in V8:
 - Update the commit message of the cover letter.
 - Remove the patch with an introduction of dummy asm/bug.h and introduce
   normal asm/bug.h
 - Fix typo in return string from decode_reserved_interrupt_cause
 - Add "Acked-by: Alistair Francis <alistair.francis@wdc.com>" for the patch
   "xen/riscv: introduce decode_cause() stuff"
 - Remove Pointless initializer of id in do_bug_frame().
 - Make bug_frames[] array constant in do_bug_frame().
 - Remove cast_to_bug_frame(addr).
 - Rename is_valig_bugaddr to is_valid_bug_insn().
 - Add check that read_instr is used only on Xen code
 - Update the commit message.
---
Changes in V7:
 - Update the depenency ( mentioned in the cover letter message ) of the current
   patch series.
 - clean up comments.
 - code style fixes.
 - move definition of cast_to_bug_frame() from patch 4 to 5.
---
Changes in V6:
 - Update the cover letter message: the patch set is based on MMU patch series.
 - Introduce new patch with temporary printk functionality. ( it will be
   removed when Xen common code will be ready )
 - Change early_printk() to printk().
 - Remove usage of LINK_TO_LOAD() due to the MMU being enabled first.
 - Add additional explanatory comments.
 - Remove patch "xen/riscv: initialize boot_info structure" from the patch
   series.
---
Changes in V5:
 - Rebase on top of [1] and [2]
 - Add new patch which introduces stub for <asm/bug.h> to keep Xen compilable
   as in the patch [xen/riscv: introduce decode_cause() stuff] is used
   header <xen/lib.h> which requires <asm/bug.h>.
 - Remove <xen/error.h> from riscv/traps/c as nothing would require
   inclusion.
 - decode_reserved_interrupt_cause(), decode_interrupt_cause(),
   decode_cause, do_unexpected_trap() were made as static they are expected
   to be used only in traps.c
 - Remove "#include <xen/types.h>" from <asm/bug.h> as there is no any need in it anymore
 - Update macros GET_INSN_LENGTH: remove UL and 'unsigned int len;' from it
 - Remove " include <xen/bug.h>" from risc/setup.c. it is not needed in the current version of
   the patch
 - change an argument type from vaddr_t to uint32_t for is_valid_bugaddr and introduce 
   read_instr() to read instruction properly as the length of qinstruction can be
   either 32 or 16 bits.
 - Code style fixes
 - update the comments before do_bug_frame() in riscv/trap.c
 - [[PATCH v4 5/5] automation: modify RISC-V smoke test ] was dropped as it was provided
   more simple solution by Andrew.  CI: Simplify RISCV smoke testing
 - Refactor is_valid_bugaddr() function.
 - 2 new patches ([PATCH v5 {1-2}/7]) were introduced, the goal of which is to recalculate
   addresses used in traps.c, which can be linker time relative. It is needed as we don't
   have enabled MMU yet.
---
Changes in V4:
  - Rebase the patch series on top of new version of [introduce generic
    implementation of macros from bug.h] patch series.
  - Update the cover letter message as 'Early printk' was merged and
    the current one patch series is based only on [introduce generic
    implementation of macros from bug.h] which hasn't been commited yet.
  - The following patches of the patch series were merged to staging:
      [PATCH v3 01/14] xen/riscv: change ISA to r64G
      [PATCH v3 02/14] xen/riscv: add <asm/asm.h> header
      [PATCH v3 03/14] xen/riscv: add <asm/riscv_encoding.h header
      [PATCH v3 04/14] xen/riscv: add <asm/csr.h> header
      [PATCH v3 05/14] xen/riscv: introduce empty <asm/string.h>
      [PATCH v3 06/14] xen/riscv: introduce empty <asm/cache.h>
      [PATCH v3 07/14] xen/riscv: introduce exception context
      [PATCH v3 08/14] xen/riscv: introduce exception handlers implementation
      [PATCH v3 10/14] xen/riscv: mask all interrupts
  - Fix addressed comments in xen-devel mailing list.

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

Oleksii Kurochko (3):
  xen/riscv: enable GENERIC_BUG_FRAME
  xen/riscv: test basic exception handling stuff
  xen/riscv: refactor decode_trap_cause()

 xen/arch/riscv/Kconfig           |  1 +
 xen/arch/riscv/include/asm/bug.h | 12 +++++-------
 xen/arch/riscv/setup.c           | 20 +++++++++++++++++++
 xen/arch/riscv/traps.c           | 33 ++++++++++++++++++++++++++++----
 xen/common/bug.c                 |  1 +
 5 files changed, 56 insertions(+), 11 deletions(-)

-- 
2.45.2

[PATCH v12 1/3] xen/riscv: enable GENERIC_BUG_FRAME

[Oleksii Kurochko]

Enable GENERIC_BUG_FRAME to support BUG(), WARN(), ASSERT,
and run_in_exception_handler().

The 0x0000 opcode is used for BUG_INSTR, which, when macros from
<xen/bug.h> are used, triggers an exception with the
ILLEGAL_INSTRUCTION cause.
This opcode is encoded as a 2-byte instruction and is invalid if
CONFIG_RISCV_ISA_C is enabled or not.
Using 'ebreak' as BUG_INSTR does not guarantee proper handling of macros
from <xen/bug.h>. If a debugger inserts a breakpoint (using the 'ebreak'
instruction) at a location where Xen already uses 'ebreak', it
creates ambiguity. Xen cannot distinguish whether the 'ebreak'
instruction is inserted by the debugger or is part of Xen's own code.

Remove BUG_INSN_32 and BUG_INSN_16 macros as they encode the ebreak
instruction, which is no longer used for BUG_INSN.

Update the commit above the definition of INS_LENGTH_MASK as ebreak
isn't and 2-byte instruction is used to encode BUG_INSTR so it doesn't
matter if CONFIG_RISCV_ISA_C is enabled or not.

<xen/lib.h> is included for the reason that panic() and printk() are
used in common/bug.c and RISC-V fails if it is not included.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V12:
 - Update the commit message
 - Use 0x0000 as BUG_INSTR instead of 'ebreak' to deal with cases when
   the debugger inserts 'ebreak' into the place where Xen has ebreak.
 - Remove BUG_INSN_32 and BUG_INSN_16 macros as they encode the ebreak
   instruction, which is no longer used for BUG_INSN.
 - Update the commit above the definition of INS_LENGTH_MASK.
 - Move break inside "if ( do_bug_frame(cpu_regs, pc) >= 0 )".
---
Changes in V11:
  - update the commit message
  - change "%lx" to "%#x" for PC register printing.
  - drop +1 in argument of is_kernel_text(pc) and is_kernel_inittext(pc).
  - drop return for case CAUSE_BREAKPOINT.
  - add break to default and add a blank like above it.
  - add a comment CAUSE_BREAKPOINT is handled instead of illegal instruction.
---
Changes in V10:
 - put 'select GENERIC_BUG_FRAME' in "Config RISCV".
 - rework do_trap() to not fetch an instruction in case when the cause of trap
   is BUG_insn.
 - drop read_instr() and is_valid_bug_insn().
 - update the commit message.
---
Changes in V9:
 - Rebase on the top of current staging.
 - use GENERIC_BUG_FRAME as now we have common code available.
 - add xen/lib.h to bug.c to fix a compilation error around printk.
 - update the commit message.
 - update the code of read_instr() in traps.c
 - fold two-s if into 1 in do_trap.
---
Changes in V8:
  - remove Pointless initializer of id.
  - make bug_frames[] array constant.
  - remove cast_to_bug_frame(addr).
  - rename is_valig_bugaddr to is_valid_bug_insn().
  - add check that read_instr is used only on xen code
  - update the commit message.
---
Changes in V7:
 - move to this patch the definition of cast_to_bug_frame() from the previous patch.
 - update the comment in bug.h.
 - update the comment above do_bug_frame().
 - fix code style.
 - add comment to read_instr func.
 - add space for bug_frames in lds.S.
---
Changes in V6:
  - Avoid LINK_TO_LOAD() as bug.h functionality expected to be used
    after MMU is enabled.
  - Change early_printk() to printk()
---
Changes in V5:
  - Remove "#include <xen/types.h>" from <asm/bug.h> as there is no any need in it anymore
  - Update macros GET_INSN_LENGTH: remove UL and 'unsigned int len;' from it
  - Remove " include <xen/bug.h>" from risc/setup.c. it is not needed in the current version of
    the patch
  - change an argument type from vaddr_t to uint32_t for is_valid_bugaddr and introduce read_instr() to
    read instruction properly as the length of qinstruction can be either 32 or 16 bits.
  - Code style fixes
  - update the comments before do_bug_frame() in riscv/trap.c
  - Refactor is_valid_bugaddr() function.
  - introduce macros cast_to_bug_frame(addr) to hide casts.
  - use LINK_TO_LOAD() for addresses which are linker time relative.
---
Changes in V4:
  - Updates in RISC-V's <asm/bug.h>:
    * Add explanatory comment about why there is only defined for 32-bits length
      instructions and 16/32-bits BUG_INSN_{16,32}.
    * Change 'unsigned long' to 'unsigned int' inside GET_INSN_LENGTH().
    * Update declaration of is_valid_bugaddr(): switch return type from int to bool
      and the argument from 'unsigned int' to 'vaddr'.
  - Updates in RISC-V's traps.c:
    * replace /xen and /asm includes
    * update definition of is_valid_bugaddr():switch return type from int to bool
      and the argument from 'unsigned int' to 'vaddr'. Code style inside function
      was updated too.
    * do_bug_frame() refactoring:
      * local variables start and bug became 'const struct bug_frame'
      * bug_frames[] array became 'static const struct bug_frame[] = ...'
      * remove all casts
      * remove unneeded comments and add an explanatory comment that the do_bug_frame()
        will be switched to a generic one.
    * do_trap() refactoring:
      * read 16-bits value instead of 32-bits as compressed instruction can
        be used and it might happen than only 16-bits may be accessible.
      * code style updates
      * re-use instr variable instead of re-reading instruction.
  - Updates in setup.c:
    * add blank line between xen/ and asm/ includes.
---
Changes in V3:
  - Rebase the patch "xen/riscv: introduce an implementation of macros
    from <asm/bug.h>" on top of patch series [introduce generic implementation
    of macros from bug.h]
---
Changes in V2:
  - Remove __ in define namings
  - Update run_in_exception_handler() with
    register void *fn_ asm(__stringify(BUG_FN_REG)) = (fn);
  - Remove bug_instr_t type and change it's usage to uint32_t
---
 xen/arch/riscv/Kconfig           |  1 +
 xen/arch/riscv/include/asm/bug.h | 12 +++++-------
 xen/arch/riscv/traps.c           | 25 ++++++++++++++++++++++++-
 xen/common/bug.c                 |  1 +
 4 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/xen/arch/riscv/Kconfig b/xen/arch/riscv/Kconfig
index b4b354a778..f531e96657 100644
--- a/xen/arch/riscv/Kconfig
+++ b/xen/arch/riscv/Kconfig
@@ -1,6 +1,7 @@
 config RISCV
 	def_bool y
 	select FUNCTION_ALIGNMENT_16B
+	select GENERIC_BUG_FRAME
 
 config RISCV_64
 	def_bool y
diff --git a/xen/arch/riscv/include/asm/bug.h b/xen/arch/riscv/include/asm/bug.h
index f5ff96140f..1fffef5037 100644
--- a/xen/arch/riscv/include/asm/bug.h
+++ b/xen/arch/riscv/include/asm/bug.h
@@ -9,7 +9,11 @@
 
 #ifndef __ASSEMBLY__
 
-#define BUG_INSTR "ebreak"
+#include <xen/stringify.h>
+
+#define BUG_OPCODE  0x0000
+
+#define BUG_INSTR ".hword " __stringify(BUG_OPCODE)
 
 /*
  * The base instruction set has a fixed length of 32-bit naturally aligned
@@ -17,16 +21,10 @@
  *
  * There are extensions of variable length ( where each instruction can be
  * any number of 16-bit parcels in length ).
- *
- * 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)                               \
diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
index cb18b30ff2..72ffdcd79e 100644
--- a/xen/arch/riscv/traps.c
+++ b/xen/arch/riscv/traps.c
@@ -5,6 +5,7 @@
  * RISC-V Trap handlers
  */
 
+#include <xen/bug.h>
 #include <xen/lib.h>
 #include <xen/sched.h>
 
@@ -103,7 +104,29 @@ static void do_unexpected_trap(const struct cpu_user_regs *regs)
 
 void do_trap(struct cpu_user_regs *cpu_regs)
 {
-    do_unexpected_trap(cpu_regs);
+    register_t pc = cpu_regs->sepc;
+    unsigned long cause = csr_read(CSR_SCAUSE);
+
+    switch ( cause )
+    {
+    case CAUSE_ILLEGAL_INSTRUCTION:
+        if ( do_bug_frame(cpu_regs, pc) >= 0 )
+        {
+            if ( !(is_kernel_text(pc) || is_kernel_inittext(pc)) )
+            {
+                printk("Something wrong with PC: %#lx\n", pc);
+                die();
+            }
+
+            cpu_regs->sepc += GET_INSN_LENGTH(*(uint16_t *)pc);
+
+            break;
+        }
+
+    default:
+        do_unexpected_trap(cpu_regs);
+        break;
+    }
 }
 
 void vcpu_show_execution_state(struct vcpu *v)
diff --git a/xen/common/bug.c b/xen/common/bug.c
index b7c5d8fd4d..75cb35fcfa 100644
--- a/xen/common/bug.c
+++ b/xen/common/bug.c
@@ -1,6 +1,7 @@
 #include <xen/bug.h>
 #include <xen/errno.h>
 #include <xen/kernel.h>
+#include <xen/lib.h>
 #include <xen/livepatch.h>
 #include <xen/string.h>
 #include <xen/types.h>
-- 
2.45.2

[PATCH v12 2/3] xen/riscv: test basic exception handling stuff

[Oleksii Kurochko]

Introduces testing of some macros from <xen/bug.h>.

Also wraps this testing into SELF_TESTS config to not produce
a noise in the log related to functionality testing ( in the
current case, it is macros from xen/bug.h ) when CONFIG_SELF_TESTS
is disabled.

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

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

[PATCH v12 3/3] xen/riscv: refactor decode_trap_cause()

[Oleksii Kurochko]

Use array_access_nospec() to prevent guest speculation.

Avoid double access of trap_causes[cause].

Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V12:
 - New patch.
---
 xen/arch/riscv/traps.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
index 72ffdcd79e..51f6e45ccc 100644
--- a/xen/arch/riscv/traps.c
+++ b/xen/arch/riscv/traps.c
@@ -7,6 +7,7 @@
 
 #include <xen/bug.h>
 #include <xen/lib.h>
+#include <xen/nospec.h>
 #include <xen/sched.h>
 
 #include <asm/processor.h>
@@ -48,9 +49,10 @@ static const char *decode_trap_cause(unsigned long cause)
         [CAUSE_STORE_GUEST_PAGE_FAULT] = "Guest Store/AMO Page Fault",
     };
 
-    if ( cause < ARRAY_SIZE(trap_causes) && trap_causes[cause] )
-        return trap_causes[cause];
-    return "UNKNOWN";
+    const char *res = cause < ARRAY_SIZE(trap_causes) ? array_access_nospec(trap_causes, cause)
+                                                      : NULL;
+
+    return res ?: "UNKNOWN";
 }
 
 static const char *decode_reserved_interrupt_cause(unsigned long irq_cause)
-- 
2.45.2

Re: [PATCH v12 3/3] xen/riscv: refactor decode_trap_cause()

[Jan Beulich]

On 02.08.2024 15:54, Oleksii Kurochko wrote:
> Use array_access_nospec() to prevent guest speculation.
> 
> Avoid double access of trap_causes[cause].
> 
> Suggested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

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

> @@ -48,9 +49,10 @@ static const char *decode_trap_cause(unsigned long cause)
>          [CAUSE_STORE_GUEST_PAGE_FAULT] = "Guest Store/AMO Page Fault",
>      };
>  
> -    if ( cause < ARRAY_SIZE(trap_causes) && trap_causes[cause] )
> -        return trap_causes[cause];
> -    return "UNKNOWN";
> +    const char *res = cause < ARRAY_SIZE(trap_causes) ? array_access_nospec(trap_causes, cause)

... the overly long line here suitably wrapped; commonly we'd do this
as ...

> +                                                      : NULL;

    const char *res = cause < ARRAY_SIZE(trap_causes)
                      ? array_access_nospec(trap_causes, cause)
                      : NULL;

I guess I'll adjust this while committing.

Jan

> +    return res ?: "UNKNOWN";
>  }
>  
>  static const char *decode_reserved_interrupt_cause(unsigned long irq_cause)

Re: [PATCH v12 3/3] xen/riscv: refactor decode_trap_cause()

[oleksii.kurochko]

On Mon, 2024-08-05 at 08:20 +0200, Jan Beulich wrote:
> On 02.08.2024 15:54, Oleksii Kurochko wrote:
> > Use array_access_nospec() to prevent guest speculation.
> > 
> > Avoid double access of trap_causes[cause].
> > 
> > Suggested-by: Jan Beulich <jbeulich@suse.com>
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with ...
> 
> > @@ -48,9 +49,10 @@ static const char *decode_trap_cause(unsigned
> > long cause)
> >          [CAUSE_STORE_GUEST_PAGE_FAULT] = "Guest Store/AMO Page
> > Fault",
> >      };
> >  
> > -    if ( cause < ARRAY_SIZE(trap_causes) && trap_causes[cause] )
> > -        return trap_causes[cause];
> > -    return "UNKNOWN";
> > +    const char *res = cause < ARRAY_SIZE(trap_causes) ?
> > array_access_nospec(trap_causes, cause)
> 
> ... the overly long line here suitably wrapped; commonly we'd do this
> as ...
> 
> > +                                                      : NULL;
> 
>     const char *res = cause < ARRAY_SIZE(trap_causes)
>                       ? array_access_nospec(trap_causes, cause)
>                       : NULL;
> 
> I guess I'll adjust this while committing.
I will be happy with that. Thanks!

~ Oleksii
> 
> Jan
> 
> > +    return res ?: "UNKNOWN";
> >  }
> >  
> >  static const char *decode_reserved_interrupt_cause(unsigned long
> > irq_cause)
> 

Re: [PATCH v12 1/3] xen/riscv: enable GENERIC_BUG_FRAME

[Jan Beulich]

On 02.08.2024 15:54, Oleksii Kurochko wrote:
> Enable GENERIC_BUG_FRAME to support BUG(), WARN(), ASSERT,
> and run_in_exception_handler().
> 
> The 0x0000 opcode is used for BUG_INSTR, which, when macros from
> <xen/bug.h> are used, triggers an exception with the
> ILLEGAL_INSTRUCTION cause.
> This opcode is encoded as a 2-byte instruction and is invalid if
> CONFIG_RISCV_ISA_C is enabled or not.

Yes, but there's a caveat: Without the C extension instructions have
to be aligned on 32-bit boundaries. You can't just go and insert a
16-bit item there. When RISCV_ISA_C is not set, I think you want to
insert two such 16-bit zeroes. Beware of an alignment handling bug
in the assembler - don't think of using an alignment directive here.

> Update the commit above the definition of INS_LENGTH_MASK as ebreak

s/commit/comment/?

> --- a/xen/arch/riscv/include/asm/bug.h
> +++ b/xen/arch/riscv/include/asm/bug.h
> @@ -9,7 +9,11 @@
>  
>  #ifndef __ASSEMBLY__
>  
> -#define BUG_INSTR "ebreak"
> +#include <xen/stringify.h>
> +
> +#define BUG_OPCODE  0x0000

You don't really use this other than ...

> +#define BUG_INSTR ".hword " __stringify(BUG_OPCODE)

... here - does this really warrant a separate #define _and_ inclusion of
stringify.h?

Furthermore you want to avoid using .hword (or any data generating
directive), to avoid disturbing disassembly. Please use .insn if at all
possible. I understand though that in certain cases you won't be able to
use .insn. Yet for the common case (more recent binutils) you'd still
better avoid .hword or alike, imo.

> @@ -103,7 +104,29 @@ static void do_unexpected_trap(const struct cpu_user_regs *regs)
>  
>  void do_trap(struct cpu_user_regs *cpu_regs)
>  {
> -    do_unexpected_trap(cpu_regs);
> +    register_t pc = cpu_regs->sepc;
> +    unsigned long cause = csr_read(CSR_SCAUSE);
> +
> +    switch ( cause )
> +    {
> +    case CAUSE_ILLEGAL_INSTRUCTION:
> +        if ( do_bug_frame(cpu_regs, pc) >= 0 )
> +        {
> +            if ( !(is_kernel_text(pc) || is_kernel_inittext(pc)) )
> +            {
> +                printk("Something wrong with PC: %#lx\n", pc);
> +                die();
> +            }
> +
> +            cpu_regs->sepc += GET_INSN_LENGTH(*(uint16_t *)pc);
> +
> +            break;
> +        }
> +
> +    default:

The falling-through here wants annotating, preferably with the pseudo-
keyword.

Jan

> +        do_unexpected_trap(cpu_regs);
> +        break;
> +    }
>  }

Re: [PATCH v12 1/3] xen/riscv: enable GENERIC_BUG_FRAME

[oleksii.kurochko]

On Mon, 2024-08-05 at 17:41 +0200, Jan Beulich wrote:
> On 02.08.2024 15:54, Oleksii Kurochko wrote:
> > Enable GENERIC_BUG_FRAME to support BUG(), WARN(), ASSERT,
> > and run_in_exception_handler().
> > 
> > The 0x0000 opcode is used for BUG_INSTR, which, when macros from
> > <xen/bug.h> are used, triggers an exception with the
> > ILLEGAL_INSTRUCTION cause.
> > This opcode is encoded as a 2-byte instruction and is invalid if
> > CONFIG_RISCV_ISA_C is enabled or not.
> 
> Yes, but there's a caveat: Without the C extension instructions have
> to be aligned on 32-bit boundaries. You can't just go and insert a
> 16-bit item there. When RISCV_ISA_C is not set, I think you want to
> insert two such 16-bit zeroes. Beware of an alignment handling bug
> in the assembler - don't think of using an alignment directive here.
Then probably it will be better to define BUG_INSTR as:
 #define BUG_INSTR "UNIMP"
and let compiler to provide proper opcode.

Or define BUG_INSTRT always as 0x00000000 will be better?
> 
> 
> > --- a/xen/arch/riscv/include/asm/bug.h
> > +++ b/xen/arch/riscv/include/asm/bug.h
> > @@ -9,7 +9,11 @@
> >  
> >  #ifndef __ASSEMBLY__
> >  
> > -#define BUG_INSTR "ebreak"
> > +#include <xen/stringify.h>
> > +
> > +#define BUG_OPCODE  0x0000
> 
> You don't really use this other than ...
> 
> > +#define BUG_INSTR ".hword " __stringify(BUG_OPCODE)
> 
> ... here - does this really warrant a separate #define _and_
> inclusion of
> stringify.h?
> 
> Furthermore you want to avoid using .hword (or any data generating
> directive), to avoid disturbing disassembly. Please use .insn if at
> all
> possible. I understand though that in certain cases you won't be able
> to
> use .insn. Yet for the common case (more recent binutils) you'd still
> better avoid .hword or alike, imo.
> 
> > @@ -103,7 +104,29 @@ static void do_unexpected_trap(const struct
> > cpu_user_regs *regs)
> >  
> >  void do_trap(struct cpu_user_regs *cpu_regs)
> >  {
> > -    do_unexpected_trap(cpu_regs);
> > +    register_t pc = cpu_regs->sepc;
> > +    unsigned long cause = csr_read(CSR_SCAUSE);
> > +
> > +    switch ( cause )
> > +    {
> > +    case CAUSE_ILLEGAL_INSTRUCTION:
> > +        if ( do_bug_frame(cpu_regs, pc) >= 0 )
> > +        {
> > +            if ( !(is_kernel_text(pc) || is_kernel_inittext(pc)) )
> > +            {
> > +                printk("Something wrong with PC: %#lx\n", pc);
> > +                die();
> > +            }
> > +
> > +            cpu_regs->sepc += GET_INSN_LENGTH(*(uint16_t *)pc);
> > +
> > +            break;
> > +        }
> > +
> > +    default:
> 
> The falling-through here wants annotating, preferably with the
> pseudo-
> keyword.
What kind of pseudo-keyword? I though about /* goto default */ to
underline that CAUSE_ILLEGAL_INSTRUCTION should be close to "default:".

~ Oleksii
> > +        do_unexpected_trap(cpu_regs);
> > +        break;
> > +    }
> >  }
> 

Re: [PATCH v12 1/3] xen/riscv: enable GENERIC_BUG_FRAME

[Jan Beulich]

On 06.08.2024 12:11, oleksii.kurochko@gmail.com wrote:
> On Mon, 2024-08-05 at 17:41 +0200, Jan Beulich wrote:
>> On 02.08.2024 15:54, Oleksii Kurochko wrote:
>>> Enable GENERIC_BUG_FRAME to support BUG(), WARN(), ASSERT,
>>> and run_in_exception_handler().
>>>
>>> The 0x0000 opcode is used for BUG_INSTR, which, when macros from
>>> <xen/bug.h> are used, triggers an exception with the
>>> ILLEGAL_INSTRUCTION cause.
>>> This opcode is encoded as a 2-byte instruction and is invalid if
>>> CONFIG_RISCV_ISA_C is enabled or not.
>>
>> Yes, but there's a caveat: Without the C extension instructions have
>> to be aligned on 32-bit boundaries. You can't just go and insert a
>> 16-bit item there. When RISCV_ISA_C is not set, I think you want to
>> insert two such 16-bit zeroes. Beware of an alignment handling bug
>> in the assembler - don't think of using an alignment directive here.
> Then probably it will be better to define BUG_INSTR as:
>  #define BUG_INSTR "UNIMP"
> and let compiler to provide proper opcode.
> 
> Or define BUG_INSTRT always as 0x00000000 will be better?

I don't know.

>>> --- a/xen/arch/riscv/include/asm/bug.h
>>> +++ b/xen/arch/riscv/include/asm/bug.h
>>> @@ -9,7 +9,11 @@
>>>  
>>>  #ifndef __ASSEMBLY__
>>>  
>>> -#define BUG_INSTR "ebreak"
>>> +#include <xen/stringify.h>
>>> +
>>> +#define BUG_OPCODE  0x0000
>>
>> You don't really use this other than ...
>>
>>> +#define BUG_INSTR ".hword " __stringify(BUG_OPCODE)
>>
>> ... here - does this really warrant a separate #define _and_
>> inclusion of
>> stringify.h?
>>
>> Furthermore you want to avoid using .hword (or any data generating
>> directive), to avoid disturbing disassembly. Please use .insn if at
>> all
>> possible. I understand though that in certain cases you won't be able
>> to
>> use .insn. Yet for the common case (more recent binutils) you'd still
>> better avoid .hword or alike, imo.
>>
>>> @@ -103,7 +104,29 @@ static void do_unexpected_trap(const struct
>>> cpu_user_regs *regs)
>>>  
>>>  void do_trap(struct cpu_user_regs *cpu_regs)
>>>  {
>>> -    do_unexpected_trap(cpu_regs);
>>> +    register_t pc = cpu_regs->sepc;
>>> +    unsigned long cause = csr_read(CSR_SCAUSE);
>>> +
>>> +    switch ( cause )
>>> +    {
>>> +    case CAUSE_ILLEGAL_INSTRUCTION:
>>> +        if ( do_bug_frame(cpu_regs, pc) >= 0 )
>>> +        {
>>> +            if ( !(is_kernel_text(pc) || is_kernel_inittext(pc)) )
>>> +            {
>>> +                printk("Something wrong with PC: %#lx\n", pc);
>>> +                die();
>>> +            }
>>> +
>>> +            cpu_regs->sepc += GET_INSN_LENGTH(*(uint16_t *)pc);
>>> +
>>> +            break;
>>> +        }
>>> +
>>> +    default:
>>
>> The falling-through here wants annotating, preferably with the
>> pseudo-
>> keyword.
> What kind of pseudo-keyword? I though about /* goto default */ to
> underline that CAUSE_ILLEGAL_INSTRUCTION should be close to "default:".

In compiler.h we define "fallthrough" specifically for purposes like
this.

Jan