Allow platform to handle load/store faults

Signed-off-by Bo Gan <[email protected]>

---
 lib/sbi/objects.mk                                 | 2 +-
 lib/sbi/{sbi_misaligned_ldst.c => sbi_trap_ldst.c} | 0
 2 files changed, 1 insertion(+), 1 deletion(-)
 rename lib/sbi/{sbi_misaligned_ldst.c => sbi_trap_ldst.c} (100%)

lib/sbi/objects.mk +1 -1

index 0a50e95..204dd6b 100644
@@ -70,7 +70,6 @@ libsbi-objs-y += sbi_illegal_insn.o
 libsbi-objs-y += sbi_init.o
 libsbi-objs-y += sbi_ipi.o
 libsbi-objs-y += sbi_irqchip.o
-libsbi-objs-y += sbi_misaligned_ldst.o
 libsbi-objs-y += sbi_platform.o
 libsbi-objs-y += sbi_pmu.o
 libsbi-objs-y += sbi_dbtr.o

@@ -80,6 +79,7 @@ libsbi-objs-y += sbi_system.o
 libsbi-objs-y += sbi_timer.o
 libsbi-objs-y += sbi_tlb.o
 libsbi-objs-y += sbi_trap.o
+libsbi-objs-y += sbi_trap_ldst.o
 libsbi-objs-y += sbi_unpriv.o
 libsbi-objs-y += sbi_expected_trap.o
 libsbi-objs-y += sbi_cppc.o

lib/sbi/sbi_trap_ldst.c renamed from lib/sbi/sbi_misaligned_ldst.c +0 -1

similarity index 100%
rename from lib/sbi/sbi_misaligned_ldst.c
rename to lib/sbi/sbi_trap_ldst.c
--

2.7.4

Signed-off-by Bo Gan <[email protected]>

---
 include/sbi/{sbi_misaligned_ldst.h => sbi_trap_ldst.h} | 4 ++--
 lib/sbi/sbi_trap.c                                     | 2 +-
 lib/sbi/sbi_trap_ldst.c                                | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)
 rename include/sbi/{sbi_misaligned_ldst.h => sbi_trap_ldst.h} (86%)

include/sbi/sbi_trap_ldst.h renamed from include/sbi/sbi_misaligned_ldst.h +2 -2

similarity index 86%
rename from include/sbi/sbi_misaligned_ldst.h
rename to include/sbi/sbi_trap_ldst.h
index ab27eb4..9bbd237 100644
@@ -7,8 +7,8 @@
  *   Anup Patel <[email protected]>
  */
 
-#ifndef __SBI_MISALIGNED_LDST_H__
-#define __SBI_MISALIGNED_LDST_H__
+#ifndef __SBI_TRAP_LDST_H__
+#define __SBI_TRAP_LDST_H__
 
 #include <sbi/sbi_types.h>
 

lib/sbi/sbi_trap.c +1 -1

index dbf307c..145db4b 100644
@@ -17,7 +17,7 @@
 #include <sbi/sbi_illegal_insn.h>
 #include <sbi/sbi_ipi.h>
 #include <sbi/sbi_irqchip.h>
-#include <sbi/sbi_misaligned_ldst.h>
+#include <sbi/sbi_trap_ldst.h>
 #include <sbi/sbi_pmu.h>
 #include <sbi/sbi_scratch.h>
 #include <sbi/sbi_timer.h>

lib/sbi/sbi_trap_ldst.c +1 -2

index aa512de..be9a394 100644
@@ -11,7 +11,7 @@
 #include <sbi/riscv_encoding.h>
 #include <sbi/riscv_fp.h>
 #include <sbi/sbi_error.h>
-#include <sbi/sbi_misaligned_ldst.h>
+#include <sbi/sbi_trap_ldst.h>
 #include <sbi/sbi_pmu.h>
 #include <sbi/sbi_trap.h>
 #include <sbi/sbi_unpriv.h>
--

2.7.4

sbi_trap_redirect now uses const pointer to trap.
This ensures the caller that we never change trap in sbi_trap_redirect.

Signed-off-by Bo Gan <[email protected]>

---
 include/sbi/sbi_trap.h | 2 +-
 lib/sbi/sbi_trap.c     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

include/sbi/sbi_trap.h +1 -1

index a562b95..2727bdb 100644
@@ -225,7 +225,7 @@ static inline unsigned long sbi_regs_gva(const struct sbi_trap_regs *regs)
 }
 
 int sbi_trap_redirect(struct sbi_trap_regs *regs,
-                     struct sbi_trap_info *trap);
+                     const struct sbi_trap_info *trap);
 
 struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs);
 

lib/sbi/sbi_trap.c +1 -2

index 145db4b..1024981 100644
@@ -84,7 +84,7 @@ static void __noreturn sbi_trap_error(const char *msg, int rc,
  * @return 0 on success and negative error code on failure
  */
 int sbi_trap_redirect(struct sbi_trap_regs *regs,
-                     struct sbi_trap_info *trap)
+                     const struct sbi_trap_info *trap)
 {
        ulong hstatus, vsstatus, prev_mode;
 #if __riscv_xlen == 32
--

2.7.4

This patch abstracts out the instruction decoding part of misaligned ld/st fault handlers, so it can be reused by ld/st access fault handlers. Also Added lb/lbu/sb decoding. (previously unreachable by misaligned fault)

sbi_trap_emulate_load/store is now the common handler which takes a emu parameter that is responsible for emulating the misaligned or access fault. The emu callback is expected to fixup the fault, and based on the return code of emu, sbi_trap_emulate_load/store will:

r/wlen => the fixup is successful and regs/mepc needs to be updated.
0 => the fixup is successful, but regs/mepc should be left untouched
(this is usually used if emu does sbi_trap_redirect)
-err => failed, sbi_trap_error will be called

For now, load/store access faults are blindly redirected. It will be enhanced in the following patches.

Signed-off-by Bo Gan <[email protected]>

---
 include/sbi/sbi_trap_ldst.h |  48 +++++++++++--
 lib/sbi/sbi_trap.c          |  34 ++++++----
 lib/sbi/sbi_trap_ldst.c     | 162 +++++++++++++++++++++++++++-----------------
 3 files changed, 165 insertions(+), 79 deletions(-)

include/sbi/sbi_trap_ldst.h +0 -0

index 9bbd237..646b242 100644
@@ -11,13 +11,51 @@
 #define __SBI_TRAP_LDST_H__
 

include/sbi/sbi_trap_ldst.h +14 -4

-int sbi_misaligned_load_handler(ulong addr, ulong tval2, ulong tinst,
-                               struct sbi_trap_regs *regs);
+/**
+ * Load emulator callback:
+ *
+ * @return rlen=success, 0=success w/o regs modification, or negative error
+ */
+typedef int (*sbi_trap_ld_emulator)(int rlen, union sbi_reg_data *out_val,
+                                   struct sbi_trap_regs *regs,
+                                   const struct sbi_trap_info *orig_trap);
 
-int sbi_misaligned_store_handler(ulong addr, ulong tval2, ulong tinst,
-                                struct sbi_trap_regs *regs);
+/**
+ * Store emulator callback:
+ *
+ * @return wlen=success, 0=success w/o regs modification, or negative error
+ */
+typedef int (*sbi_trap_st_emulator)(int wlen, union sbi_reg_data in_val,

include/sbi/sbi_trap_ldst.h +0 -0

 #endif

lib/sbi/sbi_trap.c +21 -13

index 1024981..eb4cc0f 100644
@@ -285,6 +285,13 @@ struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs)
                }
                return regs;
        }
+       /* Original trap_info */
+       trap.epc   = regs->mepc;
+       trap.cause = mcause;
+       trap.tval  = mtval;
+       trap.tval2 = mtval2;
+       trap.tinst = mtinst;
+       trap.gva   = sbi_regs_gva(regs);
 
        switch (mcause) {
        case CAUSE_ILLEGAL_INSTRUCTION:

@@ -292,11 +299,13 @@ struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs)
                msg = "illegal instruction handler failed";
                break;
        case CAUSE_MISALIGNED_LOAD:
-               rc = sbi_misaligned_load_handler(mtval, mtval2, mtinst, regs);
+               sbi_pmu_ctr_incr_fw(SBI_PMU_FW_MISALIGNED_LOAD);
+               rc  = sbi_misaligned_load_handler(regs, &trap);
                msg = "misaligned load handler failed";
                break;
        case CAUSE_MISALIGNED_STORE:
-               rc  = sbi_misaligned_store_handler(mtval, mtval2, mtinst, regs);
+               sbi_pmu_ctr_incr_fw(SBI_PMU_FW_MISALIGNED_STORE);
+               rc  = sbi_misaligned_store_handler(regs, &trap);
                msg = "misaligned store handler failed";
                break;
        case CAUSE_SUPERVISOR_ECALL:

@@ -305,20 +314,19 @@ struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs)
                msg = "ecall handler failed";
                break;
        case CAUSE_LOAD_ACCESS:
+               sbi_pmu_ctr_incr_fw(SBI_PMU_FW_ACCESS_LOAD);
+               rc  = sbi_load_fault_handler(regs, &trap);
+               msg = "load fault handler failed";
+               break;
        case CAUSE_STORE_ACCESS:
-               sbi_pmu_ctr_incr_fw(mcause == CAUSE_LOAD_ACCESS ?
-                       SBI_PMU_FW_ACCESS_LOAD : SBI_PMU_FW_ACCESS_STORE);
-               /* fallthrough */
+               sbi_pmu_ctr_incr_fw(SBI_PMU_FW_ACCESS_STORE);
+               rc  = sbi_store_fault_handler(regs, &trap);
+               msg = "store fault handler failed";
+               break;
        default:
                /* If the trap came from S or U mode, redirect it there */
-               trap.epc = regs->mepc;
-               trap.cause = mcause;
-               trap.tval = mtval;
-               trap.tval2 = mtval2;
-               trap.tinst = mtinst;
-               trap.gva   = sbi_regs_gva(regs);
-
-               rc = sbi_trap_redirect(regs, &trap);
+               msg = "trap redirect failed";
+               rc  = sbi_trap_redirect(regs, &trap);
                break;
        }
 

lib/sbi/sbi_trap_ldst.c +101 -62

index be9a394..5516954 100644
@@ -16,12 +16,6 @@
 #include <sbi/sbi_trap.h>
 #include <sbi/sbi_unpriv.h>
 
-union reg_data {
-       u8 data_bytes[8];
-       ulong data_ulong;
-       u64 data_u64;
-};
-
 static ulong sbi_misaligned_tinst_fixup(ulong orig_tinst, ulong new_tinst,
                                        ulong addr_offset)
 {

@@ -34,23 +28,22 @@ static ulong sbi_misaligned_tinst_fixup(ulong orig_tinst, ulong new_tinst,
                return orig_tinst | (addr_offset << SH_RS1);
 }
 
-int sbi_misaligned_load_handler(ulong addr, ulong tval2, ulong tinst,
-                               struct sbi_trap_regs *regs)
+int sbi_trap_emulate_load(struct sbi_trap_regs *regs,
+                         const struct sbi_trap_info *orig_trap,
+                         sbi_trap_ld_emulator emu)
 {
        ulong insn, insn_len;
-       union reg_data val;
+       union sbi_reg_data val = { 0 };
        struct sbi_trap_info uptrap;
-       int i, fp = 0, shift = 0, len = 0;
+       int rc, fp = 0, shift = 0, len = 0;
 
-       sbi_pmu_ctr_incr_fw(SBI_PMU_FW_MISALIGNED_LOAD);
-
-       if (tinst & 0x1) {
+       if (orig_trap->tinst & 0x1) {
                /*
                 * Bit[0] == 1 implies trapped instruction value is
                 * transformed instruction or custom instruction.
                 */
-               insn = tinst | INSN_16BIT_MASK;
-               insn_len = (tinst & 0x2) ? INSN_LEN(insn) : 2;
+               insn     = orig_trap->tinst | INSN_16BIT_MASK;
+               insn_len = (orig_trap->tinst & 0x2) ? INSN_LEN(insn) : 2;
        } else {
                /*
                 * Bit[0] == 0 implies trapped instruction value is

@@ -64,7 +57,12 @@ int sbi_misaligned_load_handler(ulong addr, ulong tval2, ulong tinst,
                insn_len = INSN_LEN(insn);
        }
 
-       if ((insn & INSN_MASK_LW) == INSN_MATCH_LW) {
+       if ((insn & INSN_MASK_LB) == INSN_MATCH_LB) {
+               len   = 1;
+               shift = 8 * (sizeof(ulong) - len);
+       } else if ((insn & INSN_MASK_LBU) == INSN_MATCH_LBU) {
+               len = 1;
+       } else if ((insn & INSN_MASK_LW) == INSN_MATCH_LW) {
                len   = 4;
                shift = 8 * (sizeof(ulong) - len);
 #if __riscv_xlen == 64

@@ -124,26 +122,13 @@ int sbi_misaligned_load_handler(ulong addr, ulong tval2, ulong tinst,
 #endif
 #endif
        } else {
-               uptrap.epc = regs->mepc;
-               uptrap.cause = CAUSE_MISALIGNED_LOAD;
-               uptrap.tval = addr;
-               uptrap.tval2 = tval2;
-               uptrap.tinst = tinst;
-               uptrap.gva   = sbi_regs_gva(regs);
-               return sbi_trap_redirect(regs, &uptrap);
+               return sbi_trap_redirect(regs, orig_trap);
        }
 
-       val.data_u64 = 0;
-       for (i = 0; i < len; i++) {
-               val.data_bytes[i] = sbi_load_u8((void *)(addr + i),
-                                               &uptrap);
-               if (uptrap.cause) {
-                       uptrap.epc = regs->mepc;
-                       uptrap.tinst = sbi_misaligned_tinst_fixup(
-                               tinst, uptrap.tinst, i);
-                       return sbi_trap_redirect(regs, &uptrap);
-               }
-       }
+       rc = emu(len, &val, regs, orig_trap);
+
+       if (rc <= 0)
+               return rc;
 
        if (!fp)
                SET_RD(insn, regs, ((long)(val.data_ulong << shift)) >> shift);

@@ -159,23 +144,22 @@ int sbi_misaligned_load_handler(ulong addr, ulong tval2, ulong tinst,
        return 0;
 }
 
-int sbi_misaligned_store_handler(ulong addr, ulong tval2, ulong tinst,
-                                struct sbi_trap_regs *regs)
+int sbi_trap_emulate_store(struct sbi_trap_regs *regs,
+                          const struct sbi_trap_info *orig_trap,
+                          sbi_trap_st_emulator emu)
 {
        ulong insn, insn_len;
-       union reg_data val;
+       union sbi_reg_data val;
        struct sbi_trap_info uptrap;
-       int i, len = 0;
+       int rc, len = 0;
 
-       sbi_pmu_ctr_incr_fw(SBI_PMU_FW_MISALIGNED_STORE);
-
-       if (tinst & 0x1) {
+       if (orig_trap->tinst & 0x1) {
                /*
                 * Bit[0] == 1 implies trapped instruction value is
                 * transformed instruction or custom instruction.
                 */
-               insn = tinst | INSN_16BIT_MASK;
-               insn_len = (tinst & 0x2) ? INSN_LEN(insn) : 2;
+               insn     = orig_trap->tinst | INSN_16BIT_MASK;
+               insn_len = (orig_trap->tinst & 0x2) ? INSN_LEN(insn) : 2;
        } else {
                /*
                 * Bit[0] == 0 implies trapped instruction value is

@@ -191,7 +175,9 @@ int sbi_misaligned_store_handler(ulong addr, ulong tval2, ulong tinst,
 
        val.data_ulong = GET_RS2(insn, regs);
 
-       if ((insn & INSN_MASK_SW) == INSN_MATCH_SW) {
+       if ((insn & INSN_MASK_SB) == INSN_MATCH_SB) {
+               len = 1;
+       } else if ((insn & INSN_MASK_SW) == INSN_MATCH_SW) {
                len = 4;
 #if __riscv_xlen == 64
        } else if ((insn & INSN_MASK_SD) == INSN_MATCH_SD) {

@@ -238,27 +224,81 @@ int sbi_misaligned_store_handler(ulong addr, ulong tval2, ulong tinst,
 #endif
 #endif
        } else {
-               uptrap.epc = regs->mepc;
-               uptrap.cause = CAUSE_MISALIGNED_STORE;
-               uptrap.tval = addr;
-               uptrap.tval2 = tval2;
-               uptrap.tinst = tinst;
-               uptrap.gva   = sbi_regs_gva(regs);
-               return sbi_trap_redirect(regs, &uptrap);
+               return sbi_trap_redirect(regs, orig_trap);
        }
 
-       for (i = 0; i < len; i++) {
-               sbi_store_u8((void *)(addr + i), val.data_bytes[i],
-                            &uptrap);
-               if (uptrap.cause) {
-                       uptrap.epc = regs->mepc;
-                       uptrap.tinst = sbi_misaligned_tinst_fixup(
-                               tinst, uptrap.tinst, i);
-                       return sbi_trap_redirect(regs, &uptrap);
-               }
-       }
+       rc = emu(len, val, regs, orig_trap);
+
+       if (rc <= 0)
+               return rc;
 
        regs->mepc += insn_len;
 
        return 0;
 }
+
+static int sbi_misaligned_ld_emulator(int rlen, union sbi_reg_data *out_val,
+                                     struct sbi_trap_regs *regs,
+                                     const struct sbi_trap_info *orig_trap)
+{
+       struct sbi_trap_info uptrap;
+       int i;
+
+       for (i = 0; i < rlen; i++) {
+               out_val->data_bytes[i] =
+                       sbi_load_u8((void *)(orig_trap->tval + i), &uptrap);
+               if (uptrap.cause) {
+                       uptrap.epc   = regs->mepc;
+                       uptrap.tinst = sbi_misaligned_tinst_fixup(
+                               orig_trap->tinst, uptrap.tinst, i);
+                       return sbi_trap_redirect(regs, &uptrap);
+               }
+       }
+       return rlen;
+}
+
+int sbi_misaligned_load_handler(struct sbi_trap_regs *regs,
+                               const struct sbi_trap_info *orig_trap)
+{
+       return sbi_trap_emulate_load(regs, orig_trap,
+                                    sbi_misaligned_ld_emulator);
+}
+
+static int sbi_misaligned_st_emulator(int wlen, union sbi_reg_data in_val,
+                                     struct sbi_trap_regs *regs,
+                                     const struct sbi_trap_info *orig_trap)
+{
+       struct sbi_trap_info uptrap;
+       int i;
+
+       for (i = 0; i < wlen; i++) {
+               sbi_store_u8((void *)(orig_trap->tval + i),
+                            in_val.data_bytes[i], &uptrap);
+               if (uptrap.cause) {
+                       uptrap.epc   = regs->mepc;
+                       uptrap.tinst = sbi_misaligned_tinst_fixup(
+                               orig_trap->tinst, uptrap.tinst, i);
+                       return sbi_trap_redirect(regs, &uptrap);
+               }
+       }
+       return wlen;
+}
+
+int sbi_misaligned_store_handler(struct sbi_trap_regs *regs,
+                                const struct sbi_trap_info *orig_trap)
+{
+       return sbi_trap_emulate_store(regs, orig_trap,
+                                     sbi_misaligned_st_emulator);
+}
+
+int sbi_load_fault_handler(struct sbi_trap_regs *regs,
+                          const struct sbi_trap_info *orig_trap)
+{
+       return sbi_trap_redirect(regs, orig_trap);
+}
+
+int sbi_store_fault_handler(struct sbi_trap_regs *regs,
+                           const struct sbi_trap_info *orig_trap)
+{
+       return sbi_trap_redirect(regs, orig_trap);
+}
--

2.7.4

This patch allows the platform to define load/store emulators. This enables a platform to trap-and-emulate special devices or filter access to existing physical devices.

Signed-off-by Bo Gan <[email protected]>

---
 include/sbi/sbi_platform.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

include/sbi/sbi_platform.h +2 -0

index 2fb33e1..06247dc 100644
@@ -53,6 +53,8 @@ struct sbi_domain_memregion;
 struct sbi_ecall_return;
 struct sbi_trap_regs;
 struct sbi_hart_features;
+struct sbi_trap_info;
+union sbi_reg_data;
 
 /** Possible feature flags of a platform */
 enum sbi_platform_features {

@@ -139,6 +141,15 @@ struct sbi_platform_operations {
        int (*vendor_ext_provider)(long funcid,
                                   struct sbi_trap_regs *regs,

include/sbi/sbi_platform.h +0 -1

 };
 
 /** Platform default per-HART stack size for exception/interrupt handling */
--

2.7.4

Quoted 26 lines · On Sat, Mar 2, 2024 at 4:55?PM Bo Gan <[email protected]> wrote:

This patch allows the platform to define load/store emulators. This enables a platform to trap-and-emulate special devices or filter access to existing physical devices.

Signed-off-by Bo Gan <[email protected]>

---
  include/sbi/sbi_platform.h | 11 +++++++++++
  1 file changed, 11 insertions(+)

include/sbi/sbi_platform.h +2 -0

index 2fb33e1..06247dc 100644
@@ -53,6 +53,8 @@ struct sbi_domain_memregion;
  struct sbi_ecall_return;
  struct sbi_trap_regs;
  struct sbi_hart_features;
+struct sbi_trap_info;
+union sbi_reg_data;
 
  /** Possible feature flags of a platform */
  enum sbi_platform_features {

@@ -139,6 +141,15 @@ struct sbi_platform_operations {
         int (*vendor_ext_provider)(long funcid,
                                    struct sbi_trap_regs *regs,

include/sbi/sbi_platform.h struct sbi_platform_operations { · line 152

Quoted 10 lines

include/sbi/sbi_platform.h +9 -0

                                    struct sbi_ecall_return *out);
+
+       /** platform specific handler to fixup load fault */
+       int (*emulate_load)(int rlen, union sbi_reg_data *out_val,
+                           struct sbi_trap_regs *regs,
+                           const struct sbi_trap_info *orig_trap);
+       /** platform specific handler to fixup store fault */
+       int (*emulate_store)(int wlen, union sbi_reg_data in_val,
+                            struct sbi_trap_regs *regs,
+                            const struct sbi_trap_info *orig_trap);

This callback prototype is totally wrong. The platform code need not be aware of sbi_trap_regs and sbi_trap_info. The generic sbi_ldst_trap.c should not expose how load/store is emulated to other parts of OpenSBI.

These callback prototypes should be:

        /** platform specific handler to fixup load fault */
        int (*emulate_load)(int rlen, union sbi_reg_data *out_val);
        /** platform specific handler to fixup store fault */
        int (*emulate_store)(int wlen, union sbi_reg_data in_val);

Quoted 6 lines

include/sbi/sbi_platform.h +0 -1

  };
 
  /** Platform default per-HART stack size for exception/interrupt handling */
--

2.7.4

Regards,
Anup

Quoted 43 lines · Anup Patel

Quoted 26 lines · On Sat, Mar 2, 2024 at 4:55?PM Bo Gan <[email protected]> wrote:

This patch allows the platform to define load/store emulators. This enables a platform to trap-and-emulate special devices or filter access to existing physical devices.

Signed-off-by Bo Gan <[email protected]>

---
  include/sbi/sbi_platform.h | 11 +++++++++++
  1 file changed, 11 insertions(+)

include/sbi/sbi_platform.h +2 -0

index 2fb33e1..06247dc 100644
@@ -53,6 +53,8 @@ struct sbi_domain_memregion;
  struct sbi_ecall_return;
  struct sbi_trap_regs;
  struct sbi_hart_features;
+struct sbi_trap_info;
+union sbi_reg_data;
 
  /** Possible feature flags of a platform */
  enum sbi_platform_features {

@@ -139,6 +141,15 @@ struct sbi_platform_operations {
         int (*vendor_ext_provider)(long funcid,
                                    struct sbi_trap_regs *regs,

include/sbi/sbi_platform.h struct sbi_platform_operations { · line 152

Quoted 10 lines

include/sbi/sbi_platform.h +9 -0

                                    struct sbi_ecall_return *out);
+
+       /** platform specific handler to fixup load fault */
+       int (*emulate_load)(int rlen, union sbi_reg_data *out_val,
+                           struct sbi_trap_regs *regs,
+                           const struct sbi_trap_info *orig_trap);
+       /** platform specific handler to fixup store fault */
+       int (*emulate_store)(int wlen, union sbi_reg_data in_val,
+                            struct sbi_trap_regs *regs,
+                            const struct sbi_trap_info *orig_trap);

This callback prototype is totally wrong. The platform code need not be aware of sbi_trap_regs and sbi_trap_info. The generic sbi_ldst_trap.c should not expose how load/store is emulated to other parts of OpenSBI.

Quoted 6 lines

These callback prototypes should be:

        /** platform specific handler to fixup load fault */
        int (*emulate_load)(int rlen, union sbi_reg_data *out_val);
        /** platform specific handler to fixup store fault */
        int (*emulate_store)(int wlen, union sbi_reg_data in_val);

Look like the major issue with this patchset is this.
I understand your concern. The only reason I need the platform code to receive these two pointers is for it to call sbi_trap_redirect for *real* faults. Platform code will treat these two as opaque pointers and never access struct members.

I can do without these two, then it will basically change the semantic of the return code of the platform handler like this:

plat->ops->emulate_load/store returns:

r/wlen => successful, do mepc/rd update
0 => unhandled, do sbi_trap_redirect
-errno => failed, do sbi_trap_err

= 0 ==> successful, do mepc/rd update

With this, whenever platform code decide to redirect trap, it must redirect the original trap (by returning 0). Also platform handler will never see any register state.

I'm good with this. If this is acceptable behavior to you, please let me know.

Quoted 10 lines

Quoted 6 lines

include/sbi/sbi_platform.h +0 -1

  };
 
  /** Platform default per-HART stack size for exception/interrupt handling */
--

2.7.4

Regards,
Anup

--
opensbi mailing list
opensbi at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

sbi_load/store_fault_handler now tries to call platform emulators if defined. Otherwise, redirects the fault.

We let the handler fail if the trap was originated from M mode. In this case, something must be very wrong and we should not attempt to access sbi_platform_ptr(sbi_scratch_thishart_ptr()).

Signed-off-by Bo Gan <[email protected]>

---
 include/sbi/sbi_platform.h | 25 +++++++++++++++++++++++++
 lib/sbi/sbi_trap_ldst.c    | 17 +++++++++++++++--
 2 files changed, 40 insertions(+), 2 deletions(-)

include/sbi/sbi_platform.h +15 -0

index 06247dc..c556925 100644
@@ -48,6 +48,7 @@
 #include <sbi/sbi_error.h>
 #include <sbi/sbi_scratch.h>
 #include <sbi/sbi_version.h>
+#include <sbi/sbi_trap_ldst.h>
 
 struct sbi_domain_memregion;
 struct sbi_ecall_return;

@@ -686,6 +687,30 @@ static inline int sbi_platform_vendor_ext_provider(
        return SBI_ENOTSUPP;
 }
 
+static inline int
+sbi_platform_emulate_load(const struct sbi_platform *plat,
+                         struct sbi_trap_regs *regs,
+                         const struct sbi_trap_info *orig_trap)
+{
+       if (plat && sbi_platform_ops(plat)->emulate_load) {
+               return sbi_trap_emulate_load(
+                       regs, orig_trap, sbi_platform_ops(plat)->emulate_load);
+       }
+       return sbi_trap_redirect(regs, orig_trap);
+}
+
+static inline int
+sbi_platform_emulate_store(const struct sbi_platform *plat,

include/sbi/sbi_platform.h +0 -0

 #endif
 
 #endif

lib/sbi/sbi_trap_ldst.c +15 -3

index 5516954..606830e 100644
@@ -15,6 +15,7 @@
 #include <sbi/sbi_pmu.h>
 #include <sbi/sbi_trap.h>
 #include <sbi/sbi_unpriv.h>
+#include <sbi/sbi_platform.h>
 
 static ulong sbi_misaligned_tinst_fixup(ulong orig_tinst, ulong new_tinst,
                                        ulong addr_offset)

@@ -294,11 +295,23 @@ int sbi_misaligned_store_handler(struct sbi_trap_regs *regs,
 int sbi_load_fault_handler(struct sbi_trap_regs *regs,
                           const struct sbi_trap_info *orig_trap)
 {
-       return sbi_trap_redirect(regs, orig_trap);
+       /* If fault came from M mode, just fail */
+       if (((regs->mstatus & MSTATUS_MPP) >> MSTATUS_MPP_SHIFT) == PRV_M) {
+               return SBI_EINVAL;
+       }
+
+       return sbi_platform_emulate_load(sbi_platform_thishart_ptr(), regs,
+                                        orig_trap);
 }
 
 int sbi_store_fault_handler(struct sbi_trap_regs *regs,
                            const struct sbi_trap_info *orig_trap)
 {
-       return sbi_trap_redirect(regs, orig_trap);
+       /* If fault came from M mode, just fail */
+       if (((regs->mstatus & MSTATUS_MPP) >> MSTATUS_MPP_SHIFT) == PRV_M) {
+               return SBI_EINVAL;
+       }
+
+       return sbi_platform_emulate_store(sbi_platform_thishart_ptr(), regs,
+                                         orig_trap);
 }
--

2.7.4

Quoted 44 lines · On Sat, Mar 2, 2024 at 4:55?PM Bo Gan <[email protected]> wrote:

sbi_load/store_fault_handler now tries to call platform emulators if defined. Otherwise, redirects the fault.

We let the handler fail if the trap was originated from M mode. In this case, something must be very wrong and we should not attempt to access sbi_platform_ptr(sbi_scratch_thishart_ptr()).

Signed-off-by Bo Gan <[email protected]>

---
 include/sbi/sbi_platform.h | 25 +++++++++++++++++++++++++
 lib/sbi/sbi_trap_ldst.c    | 17 +++++++++++++++--
 2 files changed, 40 insertions(+), 2 deletions(-)

include/sbi/sbi_platform.h +15 -0

index 06247dc..c556925 100644
@@ -48,6 +48,7 @@
 #include <sbi/sbi_error.h>
 #include <sbi/sbi_scratch.h>
 #include <sbi/sbi_version.h>
+#include <sbi/sbi_trap_ldst.h>
 
 struct sbi_domain_memregion;
 struct sbi_ecall_return;

@@ -686,6 +687,30 @@ static inline int sbi_platform_vendor_ext_provider(
        return SBI_ENOTSUPP;
 }
 
+static inline int
+sbi_platform_emulate_load(const struct sbi_platform *plat,
+                         struct sbi_trap_regs *regs,
+                         const struct sbi_trap_info *orig_trap)
+{
+       if (plat && sbi_platform_ops(plat)->emulate_load) {
+               return sbi_trap_emulate_load(
+                       regs, orig_trap, sbi_platform_ops(plat)->emulate_load);
+       }
+       return sbi_trap_redirect(regs, orig_trap);
+}
+
+static inline int
+sbi_platform_emulate_store(const struct sbi_platform *plat,

include/sbi/sbi_platform.h static inline int sbi_platform_vendor_ext_provider( · line 713

Quoted 10 lines

include/sbi/sbi_platform.h +10 -0

+                          struct sbi_trap_regs *regs,
+                          const struct sbi_trap_info *orig_trap)
+{
+       if (plat && sbi_platform_ops(plat)->emulate_store) {
+               return sbi_trap_emulate_store(
+                       regs, orig_trap, sbi_platform_ops(plat)->emulate_store);
+       }
+       return sbi_trap_redirect(regs, orig_trap);
+}
+

These should be implemented as follows:

static inline int
sbi_platform_emulate_load(const struct sbi_platform *plat,
int rlen, union sbi_reg_data *out_val)

{
       if (plat && sbi_platform_ops(plat)->emulate_load) {
               return sbi_platform_ops(plat)->emulate_load(rlen, out_val);
       }
       return SBI_ENOTSUPP;
}

static inline int
sbi_platform_emulate_store(const struct sbi_platform *plat,
int wlen, union sbi_reg_data in_val)

{
       if (plat && sbi_platform_ops(plat)->emulate_store) {
               return sbi_platform_ops(plat)->emulate_store(wlen, in_val);
       }
       return SBI_ENOTSUPP;
}

Quoted 43 lines

include/sbi/sbi_platform.h +0 -0

 #endif
 
 #endif

lib/sbi/sbi_trap_ldst.c +15 -3

index 5516954..606830e 100644
@@ -15,6 +15,7 @@
 #include <sbi/sbi_pmu.h>
 #include <sbi/sbi_trap.h>
 #include <sbi/sbi_unpriv.h>
+#include <sbi/sbi_platform.h>
 
 static ulong sbi_misaligned_tinst_fixup(ulong orig_tinst, ulong new_tinst,
                                        ulong addr_offset)

@@ -294,11 +295,23 @@ int sbi_misaligned_store_handler(struct sbi_trap_regs *regs,
 int sbi_load_fault_handler(struct sbi_trap_regs *regs,
                           const struct sbi_trap_info *orig_trap)
 {
-       return sbi_trap_redirect(regs, orig_trap);
+       /* If fault came from M mode, just fail */
+       if (((regs->mstatus & MSTATUS_MPP) >> MSTATUS_MPP_SHIFT) == PRV_M) {
+               return SBI_EINVAL;
+       }
+
+       return sbi_platform_emulate_load(sbi_platform_thishart_ptr(), regs,
+                                        orig_trap);
 }
 
 int sbi_store_fault_handler(struct sbi_trap_regs *regs,
                            const struct sbi_trap_info *orig_trap)
 {
-       return sbi_trap_redirect(regs, orig_trap);
+       /* If fault came from M mode, just fail */
+       if (((regs->mstatus & MSTATUS_MPP) >> MSTATUS_MPP_SHIFT) == PRV_M) {
+               return SBI_EINVAL;
+       }
+
+       return sbi_platform_emulate_store(sbi_platform_thishart_ptr(), regs,
+                                         orig_trap);
 }
--

Quoted 2 lines

2.7.4

Regards,
Anup

--
opensbi mailing list
opensbi at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi