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

Commit Message

Oleksii Kurochko May 29, 2023, 12:13 p.m. UTC

Signed-off-by: Oleksii Kurochko <[e-mail]>
Reviewed-by: Alistair Francis <[e-mail]>
---
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(+)

Comments

Jan Beulich May 30, 2023, 3:44 p.m. UTC | #1

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

Oleksii Kurochko May 31, 2023, 10:15 a.m. UTC | #2

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

Patch

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