Fixes for load/store misaligned and access faults

It's wrong to override the emulator callback in sbi_trap_emulate_load/ store. The function must respect the callback function passed in the parameter. Hence, let the misaligned emulator callback decide when to use sbi_misaligned_v_ld/st_emulator. To clean up things, also make the following changes:

- Add the insn parameter to the callback. The trapping insn has been fetched by the caller already, whether transformed or directly loaded, thus saving the trouble in the callback. Note that you must not rely on the length of the insn, as it can be a transformed one from tinst

- Also the tcntx is added, providing the callback with register values to handle vector insn or other customized insns.

- Clarify that the read/write length (rlen/wlen) can be 0, in which case it could be a vector load/store or some customized instruction. The callback is responsible to handle it accordingly.

Also fixed issues in the sbi_misaligned_v_ld/st_emulator:
a. Redirect the trap when OPENSBI_CC_SUPPORT_VECTOR is not available. b. Ensure the return code is >0 when no faults are redirected.

Fixes c2acc5e5b0d8 ("lib: sbi_misaligned_ldst: Add handling of vector load/store")

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

---
 include/sbi/sbi_platform.h  |  92 +++++++++++++++++++++++----------
 include/sbi/sbi_trap_ldst.h |   4 +-
 lib/sbi/sbi_trap_ldst.c     | 100 ++++++++++++++++++++++--------------
 lib/sbi/sbi_trap_v_ldst.c   |  25 ++++-----
 4 files changed, 139 insertions(+), 82 deletions(-)

include/sbi/sbi_platform.h +64 -28

index fe382b56..525cde51 100644
@@ -136,12 +136,17 @@ struct sbi_platform_operations {
                                   struct sbi_trap_regs *regs,
                                   struct sbi_ecall_return *out);
 
-       /** platform specific handler to fixup load fault */
-       int (*emulate_load)(int rlen, unsigned long addr,
-                           union sbi_ldst_data *out_val);
-       /** platform specific handler to fixup store fault */
-       int (*emulate_store)(int wlen, unsigned long addr,
-                            union sbi_ldst_data in_val);
+       /** platform specific handler to fixup load fault
+        *  Refer to comments below at sbi_platform_emulate_load */
+       int (*emulate_load)(ulong insn, int rlen, ulong addr,
+                           union sbi_ldst_data *out_val,
+                           struct sbi_trap_context *tcntx);
+
+       /** platform specific handler to fixup store fault
+        *  Refer to comments below at sbi_platform_emulate_store */
+       int (*emulate_store)(ulong insn, int wlen, ulong addr,
+                            union sbi_ldst_data in_val,
+                            struct sbi_trap_context *tcntx);
 
        /** platform specific pmp setup on current HART */
        void (*pmp_set)(unsigned int n, unsigned long flags,

@@ -620,45 +625,76 @@ static inline int sbi_platform_vendor_ext_provider(
 }
 
 /**
- * Ask platform to emulate the trapped load
+ * Ask platform to emulate the trapped load:
  *
- * @param plat pointer to struct sbi_platform
- * @param rlen length of the load: 1/2/4/8...
- * @param addr virtual address of the load. Platform needs to page-walk and
- *        find the physical address if necessary
- * @param out_val value loaded
+ * @param insn the instruction that caused the load fault.
+ *             It could be a transformed instruction from tinst, thus do
+ *             not rely on the length of insn, and use appropriate return
+ *             code, so the caller can advance mepc properly.
+ * @param rlen read length in [0, 1, 2, 4, 8]. If 0, it's a special load.
+ *             In that case, it could be a vector load or customized insn,
+ *             which may read/gather a block of memory. The emulator should
+ *             further parse the @insn (fetch if 0), and act accordingly.
+ * @param raddr read address. If @rlen is not 0, it's the base address of
+ *              the load. It doesn't necessarily match tcntx->trap->tval,
+ *              in case of unaligned load triggering access fault.
+ *              If @rlen is 0, @raddr should be ignored.
+ * @param out_val the buffer to hold data loaded by the emulator.
+ *                If @rlen == 0, @out_val is ignored by caller.
+ * @param tcntx trap context saved on load fault entry.
  *
- * @return 0 on success and negative error code on failure
+ * @return >0 success: register will be updated by caller if @rlen != 0,
+ *            and mepc will be advanced by caller.
+ *         0  success: no register modification; no mepc advancement.
+ *         <0 failure
+ *
+ * It's expected that if @rlen != 0, and the emulator returns >0, the
+ * caller will set the corresponding registers with @out_val to simplify
+ * things. Otherwise, no register manipulation is done by the caller.
  */
 static inline int sbi_platform_emulate_load(const struct sbi_platform *plat,
-                                           int rlen, unsigned long addr,
-                                           union sbi_ldst_data *out_val)
+                                           ulong insn, int rlen, ulong raddr,
+                                           union sbi_ldst_data *out_val,
+                                           struct sbi_trap_context *tcntx)
 {
        if (plat && sbi_platform_ops(plat)->emulate_load) {
-               return sbi_platform_ops(plat)->emulate_load(rlen, addr,
-                                                           out_val);
+               return sbi_platform_ops(plat)->emulate_load(insn, rlen, raddr,
+                                                           out_val, tcntx);
        }
        return SBI_ENOTSUPP;
 }
 
 /**
- * Ask platform to emulate the trapped store
+ * Ask platform to emulate the trapped store:
  *
- * @param plat pointer to struct sbi_platform
- * @param wlen length of the store: 1/2/4/8...
- * @param addr virtual address of the store. Platform needs to page-walk and
- *        find the physical address if necessary
- * @param in_val value to store
+ * @param insn the instruction that caused the store fault.
+ *             It could be a transformed instruction from tinst, thus do
+ *             not rely on the length of insn, and use appropriate return
+ *             code, so the caller can advance mepc properly.
+ * @param wlen write length in [0, 1, 2, 4, 8]. If 0, it's a special store.
+ *             In that case, it could be a vector store or customized insn,
+ *             which may write/scatter a block of memory. The emulator should
+ *             further parse the @insn (fetch if 0), and act accordingly.
+ * @param waddr write address. If @wlen is not 0, it's the base address of
+ *              the store. It doesn't necessarily match tcntx->trap->tval,
+ *              in case of unaligned store triggering access fault.
+ *              If @wlen is 0, @waddr should be ignored.
+ * @param in_val the buffer to hold data about to be stored by the emulator.
+ *               If @wlen == 0, @in_val should be ignored.
+ * @param tcntx trap context saved on store fault entry.
  *
- * @return 0 on success and negative error code on failure
+ * @return >0 success: mepc will be advanced by caller.
+ *         0  success: no mepc advancement.
+ *         <0 failure
  */
 static inline int sbi_platform_emulate_store(const struct sbi_platform *plat,
-                                            int wlen, unsigned long addr,
-                                            union sbi_ldst_data in_val)
+                                            ulong insn, int wlen, ulong waddr,
+                                            union sbi_ldst_data in_val,
+                                            struct sbi_trap_context *tcntx)
 {
        if (plat && sbi_platform_ops(plat)->emulate_store) {
-               return sbi_platform_ops(plat)->emulate_store(wlen, addr,
-                                                            in_val);
+               return sbi_platform_ops(plat)->emulate_store(insn, wlen, waddr,
+                                                            in_val, tcntx);
        }
        return SBI_ENOTSUPP;
 }

include/sbi/sbi_trap_ldst.h +2 -2

index a6a6c75b..33c348c5 100644
@@ -31,10 +31,10 @@ int sbi_store_access_handler(struct sbi_trap_context *tcntx);
 ulong sbi_misaligned_tinst_fixup(ulong orig_tinst, ulong new_tinst,
                                 ulong addr_offset);
 
-int sbi_misaligned_v_ld_emulator(int rlen, union sbi_ldst_data *out_val,
+int sbi_misaligned_v_ld_emulator(ulong insn,
                                 struct sbi_trap_context *tcntx);
 
-int sbi_misaligned_v_st_emulator(int wlen, union sbi_ldst_data in_val,
+int sbi_misaligned_v_st_emulator(ulong insn,
                                 struct sbi_trap_context *tcntx);
 
 #endif

lib/sbi/sbi_trap_ldst.c +62 -38

index 448406b1..ad19926a 100644
@@ -18,18 +18,18 @@
 
 /**
  * Load emulator callback:
- *
- * @return rlen=success, 0=success w/o regs modification, or negative error
+ *   Refer to comments of `sbi_platform_emulate_load`.
  */
-typedef int (*sbi_trap_ld_emulator)(int rlen, union sbi_ldst_data *out_val,
+typedef int (*sbi_trap_ld_emulator)(ulong insn, int rlen, ulong raddr,
+                                   union sbi_ldst_data *out_val,
                                    struct sbi_trap_context *tcntx);
 
 /**
  * Store emulator callback:
- *
- * @return wlen=success, 0=success w/o regs modification, or negative error
+ *   Refer to comments of `sbi_platform_emulate_store`.
  */
-typedef int (*sbi_trap_st_emulator)(int wlen, union sbi_ldst_data in_val,
+typedef int (*sbi_trap_st_emulator)(ulong insn, int wlen, ulong waddr,
+                                   union sbi_ldst_data in_val,
                                    struct sbi_trap_context *tcntx);
 
 ulong sbi_misaligned_tinst_fixup(ulong orig_tinst, ulong new_tinst,

@@ -52,7 +52,7 @@ static int sbi_trap_emulate_load(struct sbi_trap_context *tcntx,
        ulong insn, insn_len;
        union sbi_ldst_data val = { 0 };
        struct sbi_trap_info uptrap;
-       int rc, fp = 0, shift = 0, len = 0, vector = 0;
+       int rc, fp = 0, shift = 0, len = 0;
 
        if (orig_trap->tinst & 0x1) {
                /*

@@ -144,28 +144,24 @@ static int sbi_trap_emulate_load(struct sbi_trap_context *tcntx,
                len = 2;
                shift = 8 * (sizeof(ulong) - len);
                insn = RVC_RS2S(insn) << SH_RD;
-       } else if (IS_VECTOR_LOAD_STORE(insn)) {
-               vector = 1;
-               emu = sbi_misaligned_v_ld_emulator;
-       } else {
-               return sbi_trap_redirect(regs, orig_trap);
        }
 
-       rc = emu(len, &val, tcntx);
+       rc = emu(insn, len, orig_trap->tval, &val, tcntx);
        if (rc <= 0)
                return rc;
+       if (!len)
+               goto epc_fixup;
 
-       if (!vector) {
-               if (!fp)
-                       SET_RD(insn, regs, ((long)(val.data_ulong << shift)) >> shift);
+       if (!fp)
+               SET_RD(insn, regs, ((long)(val.data_ulong << shift)) >> shift);
 #ifdef __riscv_flen
-               else if (len == 8)
-                       SET_F64_RD(insn, regs, val.data_u64);
-               else
-                       SET_F32_RD(insn, regs, val.data_ulong);
+       else if (len == 8)
+               SET_F64_RD(insn, regs, val.data_u64);
+       else
+               SET_F32_RD(insn, regs, val.data_ulong);
 #endif
-       }
 
+epc_fixup:
        regs->mepc += insn_len;
 
        return 0;

@@ -253,13 +249,9 @@ static int sbi_trap_emulate_store(struct sbi_trap_context *tcntx,
        } else if ((insn & INSN_MASK_C_SH) == INSN_MATCH_C_SH) {
                len             = 2;
                val.data_ulong = GET_RS2S(insn, regs);
-       } else if (IS_VECTOR_LOAD_STORE(insn)) {
-               emu = sbi_misaligned_v_st_emulator;
-       } else {
-               return sbi_trap_redirect(regs, orig_trap);
        }
 
-       rc = emu(len, val, tcntx);
+       rc = emu(insn, len, orig_trap->tval, val, tcntx);
        if (rc <= 0)
                return rc;
 

@@ -268,7 +260,8 @@ static int sbi_trap_emulate_store(struct sbi_trap_context *tcntx,
        return 0;
 }
 
-static int sbi_misaligned_ld_emulator(int rlen, union sbi_ldst_data *out_val,
+static int sbi_misaligned_ld_emulator(ulong insn, int rlen, ulong addr,
+                                     union sbi_ldst_data *out_val,
                                      struct sbi_trap_context *tcntx)
 {
        const struct sbi_trap_info *orig_trap = &tcntx->trap;

@@ -276,9 +269,20 @@ static int sbi_misaligned_ld_emulator(int rlen, union sbi_ldst_data *out_val,
        struct sbi_trap_info uptrap;
        int i;
 
+       if (!rlen) {
+               if (IS_VECTOR_LOAD_STORE(insn))
+                       return sbi_misaligned_v_ld_emulator(insn, tcntx);
+               else
+                       /* Unrecognized instruction. Can't emulate it. */
+                       return sbi_trap_redirect(regs, orig_trap);
+       }
+       /* For misaligned fault, addr must be the same as orig_trap->tval */
+       if (addr != orig_trap->tval)
+               return SBI_EFAIL;
+
        for (i = 0; i < rlen; i++) {
                out_val->data_bytes[i] =
-                       sbi_load_u8((void *)(orig_trap->tval + i), &uptrap);
+                       sbi_load_u8((void *)(addr + i), &uptrap);
                if (uptrap.cause) {
                        uptrap.tinst = sbi_misaligned_tinst_fixup(
                                orig_trap->tinst, uptrap.tinst, i);

@@ -293,7 +297,8 @@ int sbi_misaligned_load_handler(struct sbi_trap_context *tcntx)
        return sbi_trap_emulate_load(tcntx, sbi_misaligned_ld_emulator);
 }
 
-static int sbi_misaligned_st_emulator(int wlen, union sbi_ldst_data in_val,
+static int sbi_misaligned_st_emulator(ulong insn, int wlen, ulong addr,
+                                     union sbi_ldst_data in_val,
                                      struct sbi_trap_context *tcntx)
 {
        const struct sbi_trap_info *orig_trap = &tcntx->trap;

@@ -301,8 +306,19 @@ static int sbi_misaligned_st_emulator(int wlen, union sbi_ldst_data in_val,
        struct sbi_trap_info uptrap;
        int i;
 
+       if (!wlen) {
+               if (IS_VECTOR_LOAD_STORE(insn))
+                       return sbi_misaligned_v_st_emulator(insn, tcntx);
+               else
+                       /* Unrecognized instruction. Can't emulate it. */
+                       return sbi_trap_redirect(regs, orig_trap);
+       }
+       /* For misaligned fault, addr must be the same as orig_trap->tval */
+       if (addr != orig_trap->tval)
+               return SBI_EFAIL;
+
        for (i = 0; i < wlen; i++) {
-               sbi_store_u8((void *)(orig_trap->tval + i),
+               sbi_store_u8((void *)(addr + i),
                             in_val.data_bytes[i], &uptrap);
                if (uptrap.cause) {
                        uptrap.tinst = sbi_misaligned_tinst_fixup(

@@ -318,22 +334,26 @@ int sbi_misaligned_store_handler(struct sbi_trap_context *tcntx)
        return sbi_trap_emulate_store(tcntx, sbi_misaligned_st_emulator);
 }
 
-static int sbi_ld_access_emulator(int rlen, union sbi_ldst_data *out_val,
+static int sbi_ld_access_emulator(ulong insn, int rlen, ulong addr,
+                                 union sbi_ldst_data *out_val,
                                  struct sbi_trap_context *tcntx)
 {
        const struct sbi_trap_info *orig_trap = &tcntx->trap;
        struct sbi_trap_regs *regs = &tcntx->regs;
+       int rc;
 
        /* If fault came from M mode, just fail */
        if (sbi_mstatus_prev_mode(regs->mstatus) == PRV_M)
                return SBI_EINVAL;
 
+       rc = sbi_platform_emulate_load(sbi_platform_thishart_ptr(),
+                                      insn, rlen, addr, out_val, tcntx);
+
        /* If platform emulator failed, we redirect instead of fail */
-       if (sbi_platform_emulate_load(sbi_platform_thishart_ptr(), rlen,
-                                     orig_trap->tval, out_val))
+       if (rc < 0)
                return sbi_trap_redirect(regs, orig_trap);
 
-       return rlen;
+       return rc;
 }
 
 int sbi_load_access_handler(struct sbi_trap_context *tcntx)

@@ -341,22 +361,26 @@ int sbi_load_access_handler(struct sbi_trap_context *tcntx)
        return sbi_trap_emulate_load(tcntx, sbi_ld_access_emulator);
 }
 
-static int sbi_st_access_emulator(int wlen, union sbi_ldst_data in_val,
+static int sbi_st_access_emulator(ulong insn, int wlen, ulong addr,
+                                 union sbi_ldst_data in_val,
                                  struct sbi_trap_context *tcntx)
 {
        const struct sbi_trap_info *orig_trap = &tcntx->trap;
        struct sbi_trap_regs *regs = &tcntx->regs;
+       int rc;
 
        /* If fault came from M mode, just fail */
        if (sbi_mstatus_prev_mode(regs->mstatus) == PRV_M)
                return SBI_EINVAL;
 
+       rc = sbi_platform_emulate_store(sbi_platform_thishart_ptr(),
+                                       insn, wlen, addr, in_val, tcntx);
+
        /* If platform emulator failed, we redirect instead of fail */
-       if (sbi_platform_emulate_store(sbi_platform_thishart_ptr(), wlen,
-                                      orig_trap->tval, in_val))
+       if (rc < 0)
                return sbi_trap_redirect(regs, orig_trap);
 
-       return wlen;
+       return rc;
 }
 
 int sbi_store_access_handler(struct sbi_trap_context *tcntx)

lib/sbi/sbi_trap_v_ldst.c +11 -15

index f4d469dc..57f12b83 100644
@@ -137,13 +137,11 @@ static inline void vsetvl(ulong vl, ulong vtype)
                        :: "r" (vl), "r" (vtype));
 }
 
-int sbi_misaligned_v_ld_emulator(int rlen, union sbi_ldst_data *out_val,
-                                struct sbi_trap_context *tcntx)
+int sbi_misaligned_v_ld_emulator(ulong insn, struct sbi_trap_context *tcntx)
 {
        const struct sbi_trap_info *orig_trap = &tcntx->trap;
        struct sbi_trap_regs *regs = &tcntx->regs;
        struct sbi_trap_info uptrap;
-       ulong insn = sbi_get_insn(regs->mepc, &uptrap);
        ulong vl = csr_read(CSR_VL);
        ulong vtype = csr_read(CSR_VTYPE);
        ulong vlenb = csr_read(CSR_VLENB);

@@ -234,16 +232,15 @@ int sbi_misaligned_v_ld_emulator(int rlen, union sbi_ldst_data *out_val,
        /* restore clobbered vl/vtype */
        vsetvl(vl, vtype);
 
-       return vl;
+       /* Return a >0 value for the caller to advance mepc */
+       return 1;
 }
 
-int sbi_misaligned_v_st_emulator(int wlen, union sbi_ldst_data in_val,
-                                struct sbi_trap_context *tcntx)
+int sbi_misaligned_v_st_emulator(ulong insn, struct sbi_trap_context *tcntx)
 {
        const struct sbi_trap_info *orig_trap = &tcntx->trap;
        struct sbi_trap_regs *regs = &tcntx->regs;
        struct sbi_trap_info uptrap;
-       ulong insn = sbi_get_insn(regs->mepc, &uptrap);
        ulong vl = csr_read(CSR_VL);
        ulong vtype = csr_read(CSR_VTYPE);
        ulong vlenb = csr_read(CSR_VLENB);

@@ -328,17 +325,17 @@ int sbi_misaligned_v_st_emulator(int wlen, union sbi_ldst_data in_val,
        /* restore clobbered vl/vtype */
        vsetvl(vl, vtype);
 
-       return vl;
+       /* Return a >0 value for the caller to advance mepc */
+       return 1;
 }
 #else
-int sbi_misaligned_v_ld_emulator(int rlen, union sbi_ldst_data *out_val,
-                                struct sbi_trap_context *tcntx)
+int sbi_misaligned_v_ld_emulator(ulong insn, struct sbi_trap_context *tcntx)
 {
-       return 0;
+       return sbi_trap_redirect(&tcntx->regs, &tcntx->trap);
 }
-int sbi_misaligned_v_st_emulator(int wlen, union sbi_ldst_data in_val,
-                                struct sbi_trap_context *tcntx)
+
+int sbi_misaligned_v_st_emulator(ulong insn, struct sbi_trap_context *tcntx)
 {
-       return 0;
+       return sbi_trap_redirect(&tcntx->regs, &tcntx->trap);
 }
 #endif /* OPENSBI_CC_SUPPORT_VECTOR */
--

2.34.1

--
opensbi mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/opensbi

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

---
 Makefile | 1 +
 1 file changed, 1 insertion(+)

Makefile +1 -1

index 46541063..6ce3c11e 100644
@@ -451,6 +451,7 @@ DTSCPPFLAGS =       $(CPPFLAGS) -nostdinc -nostdlib -fno-builtin -D__DTS__ -x assemble
 
 ifneq ($(DEBUG),)
 CFLAGS         +=      -O0
+CPPFLAGS       +=      -DOPENSBI_DEBUG
 else
 CFLAGS         +=      -O2
 endif
--

2.34.1

--
opensbi mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/opensbi

Rehaul instruction decoding to fix the following issues:

- We assume the XLEN of previous mode is the same as MXLEN. However, RVC instructions decodes differently in RV32 and RV64, so shouldn't have assumed that.
- We assume it's a misaligned fault and the load/store offset is 0, i.e., base address == fault address, but access faults can have non-0 offset (on HW supporting misaligned accesses), so platform specific load/store fault handler gets the wrong base address.
- No checking of [63:32] of tinst in RV64, which is explicitly required by Privileged ISA 19.6.3. Must reject tinst with non-0 high 32 bits.

Thus, fix all the above. For misaligned load/store fault, the address offset should be 0, but we'll validate that on a DEBUG build. On an optmized build, we kill the use of base address, and use trap address instead (same as before), which lets the compiler optimize out imm parsing and other calculations.

I also analyzed the behavior of misaligned fault handler before fix. With the following conditions met, it can trigger data corruption:

- HW doesn't transform instruction into tinst.
- HW doesn't support misaligned load/store, and OS doesn't enable misaligned delegation, thus OpenSBI handler is in effect
- HW supports mixed XLEN, and M mode is running RV64, and the trapping mode (U/VS/VU) is running RV32.
- The trapping instruction is c.f{l|s}w(sp).

Due to the incorrect insn decoding, the trapping instruction would mistakenly be decoded as c.{l|s}d(sp). With this fix, c.f{l|s}w(sp) in RV32 is now emulated correctly.

Validation:
The patch is validated to have fixed the issue with test cases running on a modified version of QEMU that exposes misaligned faults [1], and a further modified version that removes tinst transformation [2]. The S-mode OS is a local build of Debian Trixie 6.12 kernel that enables COMPAT (RV32), and the U-mode test application exercises all integer and floating-point load/store (RVIFD64/32+RVC64/32) instructions with all possible imm values. The patch is also tested on real HW (Sifive P550/ESWIN EIC7700), which only supports RV64. On P550, the same test was validated both in U mode and VU mode, where the host runs a 6.12 ESWIN vendor kernel that has some ESWIN SoC device driver patches [3] applied, and the guest runs the exact same Debian Trixie 6.12 kernel mentioned above.

[1] https://github.com/ganboing/qemu/tree/ganboing-misalign
[2] https://github.com/ganboing/qemu/tree/ganboing-misalign-no-tinst
[3] https://github.com/sifiveinc/riscv-linux/tree/rel/kernel-6.12/hifive-premier-p550

Fixes 7219477f7b40 ("lib: Use MTINST CSR in misaligned load/store emulation")

Fixes b5ae8e8a650d ("lib: Add misaligned load/store trap handling")

Fixes 4c112650bbb0 ("lib: sbi: abstract out insn decoding to unify mem fault handlers")

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

---
 lib/sbi/sbi_trap_ldst.c | 436 ++++++++++++++++++++++++++++------------
 1 file changed, 308 insertions(+), 128 deletions(-)

lib/sbi/sbi_trap_ldst.c +308 -129

index ad19926a..c1392251 100644
@@ -44,28 +44,34 @@ ulong sbi_misaligned_tinst_fixup(ulong orig_tinst, ulong new_tinst,
                return orig_tinst | (addr_offset << SH_RS1);
 }
 
+static inline bool sbi_trap_tinst_valid(ulong tinst)
+{
+       /*
+        * Bit[0] == 1 implies trapped instruction value is
+        * transformed instruction or custom instruction.
+        * Also do proper checking per Privileged ISA 19.6.3,
+        * and make sure high 32 bits of tinst is 0
+        */
+       return tinst == (uint32_t)tinst && (tinst & 0x1);
+}
+
 static int sbi_trap_emulate_load(struct sbi_trap_context *tcntx,
                                 sbi_trap_ld_emulator emu)
 {
        const struct sbi_trap_info *orig_trap = &tcntx->trap;
        struct sbi_trap_regs *regs = &tcntx->regs;
-       ulong insn, insn_len;
+       ulong insn, insn_len, imm = 0, shift = 0, off = 0;
        union sbi_ldst_data val = { 0 };
        struct sbi_trap_info uptrap;
-       int rc, fp = 0, shift = 0, len = 0;
+       bool xform = false, fp = false, c_load = false, c_ldsp = false;
+       int rc, len = 0, prev_xlen = 0;
 
-       if (orig_trap->tinst & 0x1) {
-               /*
-                * Bit[0] == 1 implies trapped instruction value is
-                * transformed instruction or custom instruction.
-                */
+       if (sbi_trap_tinst_valid(orig_trap->tinst)) {
+               xform    = true;
                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
-                * zero or special value.
-                */
+               /* trapped instruction value is zero or special value */
                insn = sbi_get_insn(regs->mepc, &uptrap);
                if (uptrap.cause) {
                        return sbi_trap_redirect(regs, &uptrap);

@@ -73,93 +79,176 @@ static int sbi_trap_emulate_load(struct sbi_trap_context *tcntx,
                insn_len = INSN_LEN(insn);
        }
 
+       /**
+        * Common for RV32/RV64:
+        *    lb, lbu, lh, lhu, lw, flw, flw
+        *    c.lbu, c.lh, c.lhu, c.lw, c.lwsp, c.fld, c.fldsp
+        */
        if ((insn & INSN_MASK_LB) == INSN_MATCH_LB) {
-               len   = 1;
-               shift = 8 * (sizeof(ulong) - len);
+               len = -1;
        } 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
-       } else if ((insn & INSN_MASK_LD) == INSN_MATCH_LD) {
-               len   = 8;
-               shift = 8 * (sizeof(ulong) - len);
-       } else if ((insn & INSN_MASK_LWU) == INSN_MATCH_LWU) {
-               len = 4;
-#endif
-#ifdef __riscv_flen
-       } else if ((insn & INSN_MASK_FLD) == INSN_MATCH_FLD) {
-               fp  = 1;
-               len = 8;
-       } else if ((insn & INSN_MASK_FLW) == INSN_MATCH_FLW) {
-               fp  = 1;
-               len = 4;
-#endif
+       } else if ((insn & INSN_MASK_C_LBU) == INSN_MATCH_C_LBU) {
+               /* Zcb */
+               len = 1;
+               imm = RVC_LB_IMM(insn);
+               c_load = true;
        } else if ((insn & INSN_MASK_LH) == INSN_MATCH_LH) {
-               len   = 2;
-               shift = 8 * (sizeof(ulong) - len);
+               len = -2;
+       } else if ((insn & INSN_MASK_C_LH) == INSN_MATCH_C_LH) {
+               /* Zcb */
+               len = -2;
+               imm = RVC_LH_IMM(insn);
+               c_load = true;
        } else if ((insn & INSN_MASK_LHU) == INSN_MATCH_LHU) {
                len = 2;
-#if __riscv_xlen >= 64
-       } else if ((insn & INSN_MASK_C_LD) == INSN_MATCH_C_LD) {
-               len   = 8;
-               shift = 8 * (sizeof(ulong) - len);
-               insn  = RVC_RS2S(insn) << SH_RD;
-       } else if ((insn & INSN_MASK_C_LDSP) == INSN_MATCH_C_LDSP &&
-                  ((insn >> SH_RD) & 0x1f)) {
-               len   = 8;
-               shift = 8 * (sizeof(ulong) - len);
-#endif
+       } else if ((insn & INSN_MASK_C_LHU) == INSN_MATCH_C_LHU) {
+               /* Zcb */
+               len = 2;
+               imm = RVC_LH_IMM(insn);
+               c_load = true;
+       } else if ((insn & INSN_MASK_LW) == INSN_MATCH_LW) {
+               len = -4;
        } else if ((insn & INSN_MASK_C_LW) == INSN_MATCH_C_LW) {
-               len   = 4;
-               shift = 8 * (sizeof(ulong) - len);
-               insn  = RVC_RS2S(insn) << SH_RD;
+               /* Zca */
+               len = -4;
+               imm = RVC_LW_IMM(insn);
+               c_load = true;
        } else if ((insn & INSN_MASK_C_LWSP) == INSN_MATCH_C_LWSP &&
-                  ((insn >> SH_RD) & 0x1f)) {
-               len   = 4;
-               shift = 8 * (sizeof(ulong) - len);
+               GET_RD_NUM(insn)) {
+               /* Zca */
+               len = -4;
+               imm = RVC_LWSP_IMM(insn);
+               c_ldsp = true;
 #ifdef __riscv_flen
+       } else if ((insn & INSN_MASK_FLW) == INSN_MATCH_FLW) {
+               len = 4;
+               fp = true;
+       } else if ((insn & INSN_MASK_FLD) == INSN_MATCH_FLD) {
+               len = 8;
+               fp = true;
        } else if ((insn & INSN_MASK_C_FLD) == INSN_MATCH_C_FLD) {
-               fp   = 1;
-               len  = 8;
-               insn = RVC_RS2S(insn) << SH_RD;
+               /* Zcd */
+               len = 8;
+               imm = RVC_LD_IMM(insn);
+               c_load = true;
+               fp = true;
        } else if ((insn & INSN_MASK_C_FLDSP) == INSN_MATCH_C_FLDSP) {
-               fp  = 1;
+               /* Zcd */
                len = 8;
-#if __riscv_xlen == 32
-       } else if ((insn & INSN_MASK_C_FLW) == INSN_MATCH_C_FLW) {
-               fp   = 1;
-               len  = 4;
-               insn = RVC_RS2S(insn) << SH_RD;
-       } else if ((insn & INSN_MASK_C_FLWSP) == INSN_MATCH_C_FLWSP) {
-               fp  = 1;
-               len = 4;
+               imm = RVC_LDSP_IMM(insn);
+               c_ldsp = true;
+               fp = true;
 #endif
+       } else {
+               prev_xlen = sbi_regs_prev_xlen(regs);
+       }
+
+       /**
+        * Must distinguish between rv64 and rv32, RVC instructions have
+        * overlapping encoding:
+        *     c.ld in rv64 == c.flw in rv32
+        *     c.ldsp in rv64 == c.flwsp in rv32
+        */
+       if (prev_xlen == 64) {
+               /* RV64 Only: lwu, ld, c.ld, c.ldsp  */
+               if ((insn & INSN_MASK_LWU) == INSN_MATCH_LWU) {
+                       len = 4;
+               } else if ((insn & INSN_MASK_LD) == INSN_MATCH_LD) {
+                       len = 8;
+               } else if ((insn & INSN_MASK_C_LD) == INSN_MATCH_C_LD) {
+                       /* Zca */
+                       len = 8;
+                       imm = RVC_LD_IMM(insn);
+                       c_load = true;
+               } else if ((insn & INSN_MASK_C_LDSP) == INSN_MATCH_C_LDSP &&
+                       GET_RD_NUM(insn)) {
+                       /* Zca */
+                       len = 8;
+                       imm = RVC_LDSP_IMM(insn);
+                       c_ldsp = true;
+               }
+#ifdef __riscv_flen
+       } else if (prev_xlen == 32) {
+               /* RV32 Only: c.flw, c.flwsp */
+               if ((insn & INSN_MASK_C_FLW) == INSN_MATCH_C_FLW) {
+                       /* Zcf */
+                       len = 4;
+                       imm = RVC_LW_IMM(insn);
+                       c_load = true;
+                       fp = true;
+               } else if ((insn & INSN_MASK_C_FLWSP) == INSN_MATCH_C_FLWSP) {
+                       /* Zcf */
+                       len = 4;
+                       imm = RVC_LWSP_IMM(insn);
+                       c_ldsp = true;
+                       fp = true;
+               }
 #endif
-       } else if ((insn & INSN_MASK_C_LHU) == INSN_MATCH_C_LHU) {
-               len = 2;
-               insn = RVC_RS2S(insn) << SH_RD;
-       } else if ((insn & INSN_MASK_C_LH) == INSN_MATCH_C_LH) {
-               len = 2;
+       }
+
+       if (len < 0) {
+               len = -len;
                shift = 8 * (sizeof(ulong) - len);
-               insn = RVC_RS2S(insn) << SH_RD;
        }
 
-       rc = emu(insn, len, orig_trap->tval, &val, tcntx);
+       if (!len) // Unknown instruction
+               goto do_emu;
+
+#if !defined(OPENSBI_DEBUG)
+       /**
+        * For misaligned faults. Skip offset calculation unless DEBUG
+        * builds. It helps validating OpenSBI and HW.
+        */
+       if (orig_trap->cause == CAUSE_MISALIGNED_LOAD)
+               goto do_emu;
+#endif
+
+       if (xform)
+               /* Transformed insn */
+               off = GET_RS1_NUM(insn);
+       else if (c_load)
+               /* non SP-based compressed load */
+               off = orig_trap->tval - GET_RS1S(insn, regs) - imm;
+       else if (c_ldsp)
+               /* SP-based compressed load */
+               off = orig_trap->tval - REG_VAL(2, regs) - imm;
+       else
+               /* I-type non-compressed load */
+               off = orig_trap->tval - GET_RS1(insn, regs) - (ulong)IMM_I(insn);
+       /**
+        * Normalize offset, in case the XLEN of unpriv mode is smaller,
+        * and/or pointer masking is in effect
+        */
+       off &= (len - 1);
+
+do_emu:
+       rc = emu(insn, len, orig_trap->tval - off, &val, tcntx);
        if (rc <= 0)
                return rc;
        if (!len)
                goto epc_fixup;
 
-       if (!fp)
-               SET_RD(insn, regs, ((long)(val.data_ulong << shift)) >> shift);
+       if (!fp) {
+               ulong v = ((long)(val.data_ulong << shift)) >> shift;
+
+               if (c_load)
+                       SET_RDS(insn, regs, v);
+               else
+                       SET_RD(insn, regs, v);
 #ifdef __riscv_flen
-       else if (len == 8)
-               SET_F64_RD(insn, regs, val.data_u64);
-       else
-               SET_F32_RD(insn, regs, val.data_ulong);
+       } else if (len == 8) {
+               if (c_load)
+                       SET_F64_RDS(insn, regs, val.data_u64);
+               else
+                       SET_F64_RD(insn, regs, val.data_u64);
+       } else {
+               if (c_load)
+                       SET_F32_RDS(insn, regs, val.data_ulong);
+               else
+                       SET_F32_RD(insn, regs, val.data_ulong);
 #endif
+       }
 
 epc_fixup:
        regs->mepc += insn_len;

@@ -172,23 +261,18 @@ static int sbi_trap_emulate_store(struct sbi_trap_context *tcntx,
 {
        const struct sbi_trap_info *orig_trap = &tcntx->trap;
        struct sbi_trap_regs *regs = &tcntx->regs;
-       ulong insn, insn_len;
+       ulong insn, insn_len, imm = 0, off = 0;
        union sbi_ldst_data val;
        struct sbi_trap_info uptrap;
-       int rc, len = 0;
+       bool xform = false, fp = false, c_store = false, c_stsp = false;
+       int rc, len = 0, prev_xlen = 0;
 
-       if (orig_trap->tinst & 0x1) {
-               /*
-                * Bit[0] == 1 implies trapped instruction value is
-                * transformed instruction or custom instruction.
-                */
+       if (sbi_trap_tinst_valid(orig_trap->tinst)) {
+               xform    = true;
                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
-                * zero or special value.
-                */
+               /* trapped instruction value is zero or special value */
                insn = sbi_get_insn(regs->mepc, &uptrap);
                if (uptrap.cause) {
                        return sbi_trap_redirect(regs, &uptrap);

@@ -196,62 +280,158 @@ static int sbi_trap_emulate_store(struct sbi_trap_context *tcntx,
                insn_len = INSN_LEN(insn);
        }
 
-       val.data_ulong = GET_RS2(insn, regs);
-
+       /**
+        * Common for RV32/RV64:
+        *    sb, sh, sw, fsw, fsd
+        *    c.sb, c.sh, c.sw, c.swsp, c.fsd, c.fsdsp
+        */
        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) {
-               len = 8;
-#endif
-#ifdef __riscv_flen
-       } else if ((insn & INSN_MASK_FSD) == INSN_MATCH_FSD) {
-               len          = 8;
-               val.data_u64 = GET_F64_RS2(insn, regs);
-       } else if ((insn & INSN_MASK_FSW) == INSN_MATCH_FSW) {
-               len            = 4;
-               val.data_ulong = GET_F32_RS2(insn, regs);
-#endif
+       } else if ((insn & INSN_MASK_C_SB) == INSN_MATCH_C_SB) {
+               /* Zcb */
+               len = 1;
+               imm = RVC_SB_IMM(insn);
+               c_store = true;
        } else if ((insn & INSN_MASK_SH) == INSN_MATCH_SH) {
                len = 2;
-#if __riscv_xlen >= 64
-       } else if ((insn & INSN_MASK_C_SD) == INSN_MATCH_C_SD) {
-               len            = 8;
-               val.data_ulong = GET_RS2S(insn, regs);
-       } else if ((insn & INSN_MASK_C_SDSP) == INSN_MATCH_C_SDSP) {
-               len            = 8;
-               val.data_ulong = GET_RS2C(insn, regs);
-#endif
+       } else if ((insn & INSN_MASK_C_SH) == INSN_MATCH_C_SH) {
+               /* Zcb */
+               len = 2;
+               imm = RVC_SH_IMM(insn);
+               c_store = true;
+       } else if ((insn & INSN_MASK_SW) == INSN_MATCH_SW) {
+               len = 4;
        } else if ((insn & INSN_MASK_C_SW) == INSN_MATCH_C_SW) {
-               len            = 4;
-               val.data_ulong = GET_RS2S(insn, regs);
+               /* Zca */
+               len = 4;
+               imm = RVC_SW_IMM(insn);
+               c_store = true;
        } else if ((insn & INSN_MASK_C_SWSP) == INSN_MATCH_C_SWSP) {
-               len            = 4;
-               val.data_ulong = GET_RS2C(insn, regs);
+               /* Zca */
+               len = 4;
+               imm = RVC_SWSP_IMM(insn);
+               c_stsp = true;
 #ifdef __riscv_flen
+       } else if ((insn & INSN_MASK_FSW) == INSN_MATCH_FSW) {
+               len = 4;
+               fp = true;
+       } else if ((insn & INSN_MASK_FSD) == INSN_MATCH_FSD) {
+               len = 8;
+               fp = true;
        } else if ((insn & INSN_MASK_C_FSD) == INSN_MATCH_C_FSD) {
-               len          = 8;
-               val.data_u64 = GET_F64_RS2S(insn, regs);
+               /* Zcd */
+               len = 8;
+               imm = RVC_SD_IMM(insn);
+               c_store = true;
+               fp = true;
        } else if ((insn & INSN_MASK_C_FSDSP) == INSN_MATCH_C_FSDSP) {
-               len          = 8;
-               val.data_u64 = GET_F64_RS2C(insn, regs);
-#if __riscv_xlen == 32
-       } else if ((insn & INSN_MASK_C_FSW) == INSN_MATCH_C_FSW) {
-               len            = 4;
-               val.data_ulong = GET_F32_RS2S(insn, regs);
-       } else if ((insn & INSN_MASK_C_FSWSP) == INSN_MATCH_C_FSWSP) {
-               len            = 4;
-               val.data_ulong = GET_F32_RS2C(insn, regs);
+               /* Zcd */
+               len = 8;
+               imm = RVC_SDSP_IMM(insn);
+               c_stsp = true;
+               fp = true;
 #endif
+       } else {
+               prev_xlen = sbi_regs_prev_xlen(regs);
+       }
+
+       /**
+        * Must distinguish between rv64 and rv32, RVC instructions have
+        * overlapping encoding:
+        *     c.sd in rv64 == c.fsw in rv32
+        *     c.sdsp in rv64 == c.fswsp in rv32
+        */
+       if (prev_xlen == 64) {
+               /* RV64 Only: sd, c.sd, c.sdsp */
+               if ((insn & INSN_MASK_SD) == INSN_MATCH_SD) {
+                       len = 8;
+               } else if ((insn & INSN_MASK_C_SD) == INSN_MATCH_C_SD) {
+                       /* Zca */
+                       len = 8;
+                       imm = RVC_SD_IMM(insn);
+                       c_store = true;
+               } else if ((insn & INSN_MASK_C_SDSP) == INSN_MATCH_C_SDSP) {
+                       /* Zca */
+                       len = 8;
+                       imm = RVC_SDSP_IMM(insn);
+                       c_stsp = true;
+               }
+#ifdef __riscv_flen
+       } else if (prev_xlen == 32) {
+               /* RV32 Only: c.fsw, c.fswsp */
+               if ((insn & INSN_MASK_C_FSW) == INSN_MATCH_C_FSW) {
+                       /* Zcf */
+                       len = 4;
+                       imm = RVC_SW_IMM(insn);
+                       c_store = true;
+                       fp = true;
+               } else if ((insn & INSN_MASK_C_FSWSP) == INSN_MATCH_C_FSWSP) {
+                       /* Zcf */
+                       len = 4;
+                       imm = RVC_SWSP_IMM(insn);
+                       c_stsp = true;
+                       fp = true;
+               }
+#endif
+       }
+
+       if (!fp) {
+               if (c_store)
+                       val.data_ulong = GET_RS2S(insn, regs);
+               else if (c_stsp)
+                       val.data_ulong = GET_RS2C(insn, regs);
+               else
+                       val.data_ulong = GET_RS2(insn, regs);
+#ifdef __riscv_flen
+       } else if (len == 8) {
+               if (c_store)
+                       val.data_u64 = GET_F64_RS2S(insn, regs);
+               else if (c_stsp)
+                       val.data_u64 = GET_F64_RS2C(insn, regs);
+               else
+                       val.data_u64 = GET_F64_RS2(insn, regs);
+       } else {
+               if (c_store)
+                       val.data_ulong = GET_F32_RS2S(insn, regs);
+               else if (c_stsp)
+                       val.data_ulong = GET_F32_RS2C(insn, regs);
+               else
+                       val.data_ulong = GET_F32_RS2(insn, regs);
 #endif
-       } else if ((insn & INSN_MASK_C_SH) == INSN_MATCH_C_SH) {
-               len             = 2;
-               val.data_ulong = GET_RS2S(insn, regs);
        }
 
-       rc = emu(insn, len, orig_trap->tval, val, tcntx);
+       if (!len) // Unknown instruction
+               goto do_emu;
+
+#if !defined(OPENSBI_DEBUG)
+       /**
+        * For misaligned faults. Skip offset calculation unless DEBUG
+        * builds. It helps validating OpenSBI and HW.
+        */
+       if (orig_trap->cause == CAUSE_MISALIGNED_STORE)
+               goto do_emu;
+#endif
+
+       if (xform)
+               /* Transformed insn */
+               off = GET_RS1_NUM(insn);
+       else if (c_store)
+               /* non SP-based compressed store */
+               off = orig_trap->tval - GET_RS1S(insn, regs) - imm;
+       else if (c_stsp)
+               /* SP-based compressed store */
+               off = orig_trap->tval - REG_VAL(2, regs) - imm;
+       else
+               /* S-type non-compressed store */
+               off = orig_trap->tval - GET_RS1(insn, regs) - (ulong)IMM_S(insn);
+       /**
+        * Normalize offset, in case the XLEN of unpriv mode is smaller,
+        * and/or pointer masking is in effect
+        */
+       off &= (len - 1);
+
+do_emu:
+       rc = emu(insn, len, orig_trap->tval - off, val, tcntx);
        if (rc <= 0)
                return rc;
 
--

2.34.1

--
opensbi mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/opensbi

Build: gcc -o test-misaligned-ldst test-misaligned-ldst.c ldst.S

Failure observed before the fix on QEMU with 64-bit Linux and 32-bit test

...
loadfp 64, insn cfldsp, n=64, off=1, cmp=788796a5b4c3d2e1 loadfp 64, insn cfldsp, n=64, off=2, cmp=69788796a5b4c3d2 loadfp 64, insn cfldsp, n=64, off=3, cmp=5a69788796a5b4c3 loadfp 64, insn cfldsp, n=64, off=4, cmp=4b5a69788796a5b4 loadfp 64, insn cfldsp, n=64, off=5, cmp=3c4b5a69788796a5 loadfp 64, insn cfldsp, n=64, off=6, cmp=2d3c4b5a69788796 loadfp 64, insn cfldsp, n=64, off=7, cmp=1e2d3c4b5a697887 loadfp 32, insn cflw, n=32, off=1, cmp=ffffffffb4c3d2e1 ./test32: failed at load_f 1 4 0: 0

With the patch series:

...
loadfp 64, insn cfldsp, n=64, off=1, cmp=788796a5b4c3d2e1 loadfp 64, insn cfldsp, n=64, off=2, cmp=69788796a5b4c3d2 loadfp 64, insn cfldsp, n=64, off=3, cmp=5a69788796a5b4c3 loadfp 64, insn cfldsp, n=64, off=4, cmp=4b5a69788796a5b4 loadfp 64, insn cfldsp, n=64, off=5, cmp=3c4b5a69788796a5 loadfp 64, insn cfldsp, n=64, off=6, cmp=2d3c4b5a69788796 loadfp 64, insn cfldsp, n=64, off=7, cmp=1e2d3c4b5a697887 loadfp 32, insn cflw, n=32, off=1, cmp=ffffffffb4c3d2e1 loadfp 32, insn cflw, n=32, off=2, cmp=ffffffffa5b4c3d2 loadfp 32, insn cflw, n=32, off=3, cmp=ffffffff96a5b4c3 loadfp 32, insn cflw, n=32, off=5, cmp=ffffffff788796a5 loadfp 32, insn cflw, n=32, off=6, cmp=ffffffff69788796 loadfp 32, insn cflw, n=32, off=7, cmp=ffffffff5a697887 loadfp 32, insn cflw, n=32, off=9, cmp=ffffffff3c4b5a69 loadfp 32, insn cflw, n=32, off=10, cmp=ffffffff2d3c4b5a loadfp 32, insn cflw, n=32, off=11, cmp=ffffffff1e2d3c4b loadfp 32, insn cflwsp, n=64, off=1, cmp=ffffffffb4c3d2e1 loadfp 32, insn cflwsp, n=64, off=2, cmp=ffffffffa5b4c3d2 loadfp 32, insn cflwsp, n=64, off=3, cmp=ffffffff96a5b4c3 loadfp 32, insn cflwsp, n=64, off=5, cmp=ffffffff788796a5 loadfp 32, insn cflwsp, n=64, off=6, cmp=ffffffff69788796 loadfp 32, insn cflwsp, n=64, off=7, cmp=ffffffff5a697887 loadfp 32, insn cflwsp, n=64, off=9, cmp=ffffffff3c4b5a69 loadfp 32, insn cflwsp, n=64, off=10, cmp=ffffffff2d3c4b5a loadfp 32, insn cflwsp, n=64, off=11, cmp=ffffffff1e2d3c4b

<no failure>

---
 tests/ldst.S                 | 134 +++++++++++++++++++++++++++
 tests/ldst.h                 | 170 +++++++++++++++++++++++++++++++++++
 tests/test-misaligned-ldst.c | 154 +++++++++++++++++++++++++++++++
 3 files changed, 458 insertions(+)
 create mode 100644 tests/ldst.S
 create mode 100644 tests/ldst.h
 create mode 100644 tests/test-misaligned-ldst.c

tests/ldst.S new file +134 -0

new file mode 100644
index 00000000..66f88e42
@@ -0,0 +1,134 @@
+.altmacro
+
+.macro mem_repeat name:req, ldst:req, op:req, rx:req, ry:req, len:req, signext=0, step=1, n=4096, max=2048
+.globl mem_\name
+.type mem_\name, @function
+mem_\name\():
+.set i, 0
+.rept \n
+.set j, \max - \n * \step + i * \step
+1:
+       mem_\ldst \op \rx \ry j
+       ret
+.ifne . - mem_\name - (. - 1b) * (i + 1)
+.error "mem_\name is not aligned"
+.endif
+.set i, i + 1
+.endr
+.size mem_\name, . - mem_\name
+
+.align 3
+.globl mem_\name\()_desc
+.type mem_\name\()_desc @object
+mem_\name\()_desc:
+       .byte \len, \signext, \step, (. - mem_\name) / \n
+       .half \max - \n * \step, \n
+#if __riscv_xlen == 32
+       .word mem_\name
+#elif __riscv_xlen == 64
+       .dword mem_\name
+#endif
+.size mem_\name\()_desc, . - mem_\name\()_desc
+.endm
+
+.macro mem_load op rs rd imm
+       mv \rs, a0
+       \op \rd, \imm(\rs)
+       mv a0, \rd
+.endm
+
+.macro mem_fpld op rs rd imm
+       mv \rs, a0
+       \op \rd, \imm(\rs)
+       fmv.d fa0, \rd
+.endm
+
+.macro mem_store op rs1 rs2 imm
+       mv \rs2, a1
+       mv \rs1, a0
+       \op \rs2, \imm(\rs1)
+.endm
+
+.macro mem_fpst op rs1 rs2 imm
+       fmv.d \rs2, fa0
+       mv \rs1, a0
+       \op \rs2, \imm(\rs1)
+.endm
+
+.macro mem_ldsp op tmp rd imm
+       mv \tmp, sp
+       mv sp, a0
+       \op \rd, \imm(sp)
+       mv sp, \tmp
+       mv a0, \rd
+.endm
+
+.macro mem_fpldsp op tmp rd imm
+       mv \tmp, sp
+       mv sp, a0
+       \op \rd, \imm(sp)
+       mv sp, \tmp
+       fmv.d fa0, \rd
+.endm
+
+.macro mem_stsp op tmp rs2 imm
+       mv \tmp, sp
+       mv sp, a0
+       mv \rs2, a1
+       \op \rs2, \imm(sp)
+       mv sp, \tmp
+.endm
+
+.macro mem_fpstsp op tmp rs2 imm
+       mv \tmp, sp
+       mv sp, a0
+       fmv.d \rs2, fa0
+       \op \rs2, \imm(sp)
+       mv sp, \tmp
+.endm
+
+mem_repeat lb     load   lb    t0 a1   1 1
+mem_repeat lbu    load   lbu   t1 a2   1
+mem_repeat sb     store  sb    t2 a3   1
+mem_repeat lh     load   lh    t4 a5   2 1
+mem_repeat lhu    load   lhu   t5 a6   2
+mem_repeat sh     store  sh    t6 a7   2
+mem_repeat lw     load   lw    a7 t6   4 1
+#if __riscv_xlen == 64
+mem_repeat lwu    load   lwu   a6 t5   4
+#endif
+mem_repeat sw     store  sw    a5 t4   4
+#if __riscv_xlen == 64
+mem_repeat ld     load   ld    a4 t3   8 1
+mem_repeat sd     store  sd    a3 t2   8
+#endif
+mem_repeat flw    fpld   flw   t3 ft8  4
+mem_repeat fsw    fpst   fsw   t4 ft9  4
+mem_repeat fld    fpld   fld   t5 ft10 8
+mem_repeat fsd    fpst   fsd   t6 ft11 8
+#ifdef __riscv_zcb
+mem_repeat clbu   load   c.lbu a5 a1   1 0 1 4  4
+mem_repeat csb    store  c.sb  a4 a2   1 0 1 4  4
+mem_repeat clh    load   c.lh  a3 a3   2 1 2 2  4
+mem_repeat clhu   load   c.lhu a2 a4   2 0 2 2  4
+mem_repeat csh    store  c.sh  a1 a5   2 0 2 2  4
+#endif
+mem_repeat clw    load   c.lw  a5 a1   4 1 4 32 128
+mem_repeat csw    store  c.sw  a4 a2   4 0 4 32 128
+mem_repeat clwsp  ldsp   lw    t0 t6   4 1 4 64 256
+mem_repeat cswsp  stsp   sw    t1 t5   4 0 4 64 256
+mem_repeat cfld   fpld   c.fld a3 fa2  8 0 8 32 256
+mem_repeat cfsd   fpst   c.fsd a2 fa3  8 0 8 32 256
+mem_repeat cfldsp fpldsp fld   t2 ft10 8 0 8 64 512
+mem_repeat cfsdsp fpstsp fsd   t3 ft11 8 0 8 64 512
+#if __riscv_xlen == 32
+mem_repeat cflw   fpld   c.flw a5 fa4  4 0 4 32 128
+mem_repeat cfsw   fpst   c.fsw a4 fa5  4 0 4 32 128
+mem_repeat cflwsp fpldsp flw   t3 ft11 4 0 4 64 256
+mem_repeat cfswsp fpstsp fsw   t4 ft10 4 0 4 64 256
+#elif __riscv_xlen == 64
+mem_repeat cld    load   c.ld  a5 a4   8 1 8 32 256
+mem_repeat csd    store  c.sd  a4 a5   8 0 8 32 256
+mem_repeat cldsp  ldsp   ld    t3 t6   8 1 8 64 512
+mem_repeat csdsp  stsp   sd    t4 t5   8 0 8 64 512
+#endif

tests/ldst.h new file +170 -0

new file mode 100644
index 00000000..3d4b6062
@@ -0,0 +1,170 @@
+#include <inttypes.h>
+
+typedef struct {
+       uint8_t len;
+       uint8_t signext;
+       uint8_t imm_step;
+       uint8_t isize;
+       int16_t imm_base;
+       uint16_t n;
+       void *fp;
+} mem_op_desc;
+
+typedef union {
+       unsigned long u_long;
+       long i_long;
+       uint32_t u32[2];
+       int32_t i32[2];
+       uint64_t u64;
+       int64_t i64;
+       float f32[2];
+       double f64;
+       uint8_t bytes[8];
+} ldst_val;
+
+extern mem_op_desc mem_lb_desc;
+extern mem_op_desc mem_lbu_desc;
+extern mem_op_desc mem_sb_desc;
+extern mem_op_desc mem_lh_desc;
+extern mem_op_desc mem_lhu_desc;
+extern mem_op_desc mem_sh_desc;
+extern mem_op_desc mem_lw_desc;
+extern mem_op_desc mem_lwu_desc;
+extern mem_op_desc mem_sw_desc;
+extern mem_op_desc mem_ld_desc;
+extern mem_op_desc mem_sd_desc;
+extern mem_op_desc mem_flw_desc;
+extern mem_op_desc mem_fsw_desc;
+extern mem_op_desc mem_fld_desc;
+extern mem_op_desc mem_fsd_desc;
+extern mem_op_desc mem_clbu_desc;
+extern mem_op_desc mem_csb_desc;
+extern mem_op_desc mem_clh_desc;
+extern mem_op_desc mem_clhu_desc;
+extern mem_op_desc mem_csh_desc;
+extern mem_op_desc mem_clw_desc;
+extern mem_op_desc mem_csw_desc;
+extern mem_op_desc mem_clwsp_desc;
+extern mem_op_desc mem_cswsp_desc;
+extern mem_op_desc mem_cfld_desc;
+extern mem_op_desc mem_cfsd_desc;
+extern mem_op_desc mem_cfldsp_desc;
+extern mem_op_desc mem_cfsdsp_desc;
+extern mem_op_desc mem_cflw_desc;
+extern mem_op_desc mem_cfsw_desc;
+extern mem_op_desc mem_cflwsp_desc;
+extern mem_op_desc mem_cfswsp_desc;
+extern mem_op_desc mem_cld_desc;
+extern mem_op_desc mem_csd_desc;
+extern mem_op_desc mem_cldsp_desc;
+extern mem_op_desc mem_csdsp_desc;
+
+typedef struct ldst_func {
+       const char *name;
+       const mem_op_desc *desc;
+} ldst_func;
+
+#define DEF_LDST_FUNC(x) { #x, &mem_ ## x ## _desc }
+
+static const ldst_func load_funcs[] = {
+       DEF_LDST_FUNC(lb),
+       DEF_LDST_FUNC(lbu),
+       DEF_LDST_FUNC(lh),
+       DEF_LDST_FUNC(lhu),
+       DEF_LDST_FUNC(lw),
+#ifdef __riscv_zcb
+       DEF_LDST_FUNC(clbu),
+       DEF_LDST_FUNC(clh),
+       DEF_LDST_FUNC(clhu),
+#endif
+       DEF_LDST_FUNC(clw),
+       DEF_LDST_FUNC(clwsp),
+#if __riscv_xlen == 64
+       DEF_LDST_FUNC(lwu),
+       DEF_LDST_FUNC(ld),
+       DEF_LDST_FUNC(cld),
+       DEF_LDST_FUNC(cldsp),
+#endif
+};
+
+static const ldst_func store_funcs[] = {
+       DEF_LDST_FUNC(sb),
+       DEF_LDST_FUNC(sh),
+       DEF_LDST_FUNC(sw),
+#ifdef __riscv_zcb
+       DEF_LDST_FUNC(csb),
+       DEF_LDST_FUNC(csh),
+#endif
+       DEF_LDST_FUNC(csw),
+       DEF_LDST_FUNC(cswsp),
+#if __riscv_xlen == 64
+       DEF_LDST_FUNC(sd),
+       DEF_LDST_FUNC(csd),
+       DEF_LDST_FUNC(csdsp),
+#endif
+};
+
+static const ldst_func loadfp_funcs[] = {
+#if __riscv_xlen == 32
+       DEF_LDST_FUNC(cflw),
+       DEF_LDST_FUNC(cflwsp),
+#endif
+       DEF_LDST_FUNC(flw),
+       DEF_LDST_FUNC(fld),
+       DEF_LDST_FUNC(cfld),
+       DEF_LDST_FUNC(cfldsp),
+};
+
+static const ldst_func storefp_funcs[] = {
+#if __riscv_xlen == 32
+       DEF_LDST_FUNC(cfsw),
+       DEF_LDST_FUNC(cfswsp),
+#endif
+       DEF_LDST_FUNC(fsw),
+       DEF_LDST_FUNC(fsd),
+       DEF_LDST_FUNC(cfsd),
+       DEF_LDST_FUNC(cfsdsp),
+};
+
+static inline unsigned long load_i(const void *p, unsigned func, unsigned sel)
+{
+       typedef unsigned long (*func_i)(const void *p);
+       const mem_op_desc *desc = load_funcs[func].desc;
+
+       long imm = (long)desc->imm_base + desc->imm_step * sel;
+       func_i f = desc->fp + desc->isize * sel;
+
+       return f(p - imm);
+}
+
+static inline void store_i(void *p, unsigned long val, unsigned func, unsigned sel)
+{
+       typedef void (*func_i)(void *p, unsigned long val);
+       const mem_op_desc *desc = store_funcs[func].desc;
+
+       long imm = (long)desc->imm_base + desc->imm_step * sel;
+       func_i f = desc->fp + desc->isize * sel;
+       f(p - imm, val);
+}
+
+static inline double load_f(const void *p, unsigned func, unsigned sel)
+{
+       typedef double (*func_f)(const void *p);
+       const mem_op_desc *desc = loadfp_funcs[func].desc;
+
+       long imm = (long)desc->imm_base + desc->imm_step * sel;
+       func_f f = desc->fp + desc->isize * sel;
+
+       return f(p - imm);
+}
+
+static inline void store_f(void *p, double val, unsigned func, unsigned sel)
+{
+       typedef void (*func_f)(void *p, double val);
+       const mem_op_desc *desc = storefp_funcs[func].desc;
+
+       long imm = (long)desc->imm_base + desc->imm_step * sel;
+       func_f f = desc->fp + desc->isize * sel;
+
+       f(p - imm, val);
+}

tests/test-misaligned-ldst.c new file +154 -1

new file mode 100644
index 00000000..ccbd0d36
@@ -0,0 +1,154 @@
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <stdint.h>
+#include <error.h>
+#include "ldst.h"
+
+#define ARR_SIZE(arr) (sizeof(arr) / sizeof(arr[0]))
+
+static const uint8_t patt[16] = {
+       0xf0, 0xe1, 0xd2, 0xc3, 0xb4, 0xa5, 0x96, 0x87,
+       0x78, 0x69, 0x5a, 0x4b, 0x3c, 0x2d, 0x1e, 0x0f,
+};
+
+static const uint64_t load_val[16] = {
+       0x8796a5b4c3d2e1f0ULL,
+       0x788796a5b4c3d2e1ULL,
+       0x69788796a5b4c3d2ULL,
+       0x5a69788796a5b4c3ULL,
+       0x4b5a69788796a5b4ULL,
+       0x3c4b5a69788796a5ULL,
+       0x2d3c4b5a69788796ULL,
+       0x1e2d3c4b5a697887ULL,
+       0x0f1e2d3c4b5a6978ULL,
+         0x0f1e2d3c4b5a69ULL,
+           0x0f1e2d3c4b5aULL,
+             0x0f1e2d3c4bULL,
+               0x0f1e2d3cULL,
+                 0x0f1e2dULL,
+                   0x0f1eULL,
+                     0x0fULL,
+};
+
+int main()
+{
+       unsigned i, j, k;
+       //ldst_val val;
+
+       // Test int read
+       for (j = 0; j < ARR_SIZE(load_funcs); ++j) {
+               const mem_op_desc *desc = load_funcs[j].desc;
+               unsigned shift = 8 * (sizeof(long) - desc->len);
+
+               for (i = 0; i < ARR_SIZE(patt) - desc->len; ++i) {
+                       unsigned long expected;
+                       if (desc->len != 1 && i % desc->len == 0)
+                               continue;
+
+                       expected = load_val[i];
+                       if (desc->signext)
+                               expected = (long)(expected << shift) >> shift;
+                       else
+                               expected = (expected << shift) >> shift;
+
+                       printf("load %c%u, insn %s, n=%u, off=%u, cmp=%lx\n",
+                               desc->signext ? 'i' : 'u',
+                               desc->len * 8, load_funcs[j].name, desc->n, i, expected);
+                       fflush(stdout);
+                       for (k = 0; k < desc->n; ++k) {
+                               unsigned long read = load_i(&patt[i], j, k);
+                               if (read != expected)
+                                       error(1, 0, "failed at load_i %u %u %u: %lx",
+                                               i, j, k, read);
+                       }
+               }
+       }
+       // Test fp load
+       for (j = 0; j < ARR_SIZE(loadfp_funcs); ++j) {
+               const mem_op_desc *desc = loadfp_funcs[j].desc;
+
+               for (i = 0; i < ARR_SIZE(patt) - desc->len; ++i) {
+                       ldst_val expected, read;
+
+                       expected.u64 = load_val[i];
+                       if (desc->len == 4) // float
+                               expected.i32[1] = -1;
+
+                       if (i % desc->len == 0)
+                               continue;
+
+                       printf("loadfp %u, insn %s, n=%u, off=%u, cmp=%" PRIx64 "\n",
+                               desc->len * 8, loadfp_funcs[j].name, desc->n, i, expected.u64);
+                       fflush(stdout);
+                       for (k = 0; k < desc->n; ++k) {
+                               read.f64 = load_f(&patt[i], j, k);
+                               if (read.u64 != expected.u64)
+                                       error(1, 0, "failed at load_f %u %u %u: %" PRIx64,
+                                               i, j, k, read.u64);
+                       }
+               }
+       }
+       // Test int store
+       for (j = 0; j < ARR_SIZE(store_funcs); ++j) {
+               const mem_op_desc *desc = store_funcs[j].desc;
+
+               for (i = 0; i < ARR_SIZE(patt) - desc->len; ++i) {
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Woverflow"
+                       ldst_val val = { 0x8899aabbccddeeffULL };
+#pragma GCC diagnostic pop
+                       unsigned end = i + desc->len;
+
+                       if (desc->len != 1 && i % desc->len == 0)
+                               continue;
+
+                       memcpy(val.bytes, &patt[i], desc->len);
+                       printf("store %u, insn %s, n=%u, off=%u, src=%lx\n",
+                               desc->len * 8, store_funcs[j].name, desc->n, i, val.u_long);
+                       fflush(stdout);
+
+                       for (k = 0; k < desc->n; ++k) {
+                               uint8_t buff[ARR_SIZE(patt)] = {};
+
+                               memcpy(buff, patt, i);
+                               memcpy(&buff[end], &patt[end], ARR_SIZE(patt) - end);
+                               store_i(&buff[i], val.u_long, j, k);
+                               if (memcmp(buff, patt, sizeof(buff)))
+                                       error(1, 0, "faild at store_i %u %u %u", i, j, k);
+                       }
+
+               }
+       }
+       // Test fp store
+       for (j = 0; j < ARR_SIZE(storefp_funcs); ++j) {
+               const mem_op_desc *desc = storefp_funcs[j].desc;
+
+               for (i = 0; i < ARR_SIZE(patt) - desc->len; ++i) {
+                       ldst_val val;
+                       unsigned end = i + desc->len;
+
+                       val.u32[0] = 0xccddeeffUL;
+                       val.u32[1] = 0x8899aabbUL;
+
+                       if (i % desc->len == 0)
+                               continue;
+
+                       memcpy(val.bytes, &patt[i], desc->len);
+                       printf("storefp %u, insn %s, n=%u, off=%u, src=%" PRIx64 "\n",
+                               desc->len * 8, storefp_funcs[j].name, desc->n, i, val.u64);
+                       fflush(stdout);
+
+                       for (k = 0; k < desc->n; ++k) {
+                               uint8_t buff[ARR_SIZE(patt)] = {};
+
+                               memcpy(buff, patt, i);
+                               memcpy(&buff[end], &patt[end], ARR_SIZE(patt) - end);
+                               store_f(&buff[i], val.f64, j, k);
+                               if (memcmp(buff, patt, sizeof(buff)))
+                                       error(1, 0, "faild at store_f %u %u %u", i, j, k);
+                       }
+               }
+       }
+       return 0;
+}
--

2.34.1

--
opensbi mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/opensbi

Re-visit the load/store misaligned and access fault handlers to fix issues related to coding patterns, floating-point state, and instruction decoding:

- tinst should be zero'ed out to not confuse previous mode when redirecting faults, otherwise the vector insn can be mistaken as a regular load/store.
- VS in previous mode must be set dirty for loads.

These will be addressed in follow-up patches.

[1] https://github.com/ganboing/qemu/tree/ganboing-misalign
[2] https://github.com/ganboing/qemu/tree/ganboing-misalign-no-tinst
[3] https://github.com/ganboing/opensbi/tree/fix-ldst-v2
---
Changes in v2:
- Addressed Anup's comment for PATCH 5 in v1
- Validate load/store offset is 0 in misaligned faults w/ DEBUG build

---
Bo Gan (8):
include: sbi: Add more mstatus and instruction encoding
include: sbi: Add sbi_regs_prev_xlen
include: sbi: Add GET_RDS_NUM/SET(_FP32/_FP64)_RDS macros
include: sbi: set FS dirty in vsstatus when V=1
lib: sbi: Do not override emulator callback for vector load/store
Makefile: define OPENSBI_DEBUG if DEBUG builds
lib: sbi: Rework load/store emulator instruction decoding [NOT-FOR-UPSTREAM] Test program for misaligned load/store

 Makefile                     |   1 +
 include/sbi/riscv_encoding.h |  21 +-
 include/sbi/riscv_fp.h       |  30 ++-
 include/sbi/sbi_platform.h   |  92 +++++--
 include/sbi/sbi_trap.h       |  59 ++++
 include/sbi/sbi_trap_ldst.h  |   4 +-
 lib/sbi/sbi_trap_ldst.c      | 510 ++++++++++++++++++++++++-----------
 lib/sbi/sbi_trap_v_ldst.c    |  25 +-
 tests/ldst.S                 | 134 +++++++++
 tests/ldst.h                 | 170 ++++++++++++
 tests/test-misaligned-ldst.c | 154 +++++++++++
 11 files changed, 994 insertions(+), 206 deletions(-)
 create mode 100644 tests/ldst.S
 create mode 100644 tests/ldst.h
 create mode 100644 tests/test-misaligned-ldst.c

--
2.34.1

Hello Bo,

Quoted 4 lines · Bo Gan

Re-visit the load/store misaligned and access fault handlers to fix issues related to coding patterns, floating-point state, and instruction decoding:

I had previously reported here [1] that there were issues booting into linux after enabling misaligned trap delegation to linux on Sifive X280. In the discussion over there, we concluding that bumping up the per hart stack size in opensbi fixed the issue.

This series (without the stack size bump) also seems to fix the issues that prevented linux from booting. Particularly it was this patch "lib: sbi: Do not override emulator callback for vector load/store".

But as you say, I still think the stack size bump is needed as I was able to break the boot by adding some debug prints like this along the way. I guess this must have somehow caused the stack to overflow?

lib/sbi/sbi_trap_v_ldst.c +5 -0

index 57f12b83..5e596664 100644
@@ -16,6 +16,7 @@
  #include <sbi/sbi_trap.h>
  #include <sbi/sbi_unpriv.h>
  #include <sbi/sbi_trap.h>
+#include <sbi/sbi_console.h>
 
  #ifdef OPENSBI_CC_SUPPORT_VECTOR
 

@@ -139,6 +140,8 @@ static inline void vsetvl(ulong vl, ulong vtype)
 
  int sbi_misaligned_v_ld_emulator(ulong insn, struct sbi_trap_context *tcntx)
  {
+       sbi_printf("%s: insn=0x%lx mepc=0x%lx mtval=0x%lx\n",
+                  __func__, insn, tcntx->regs.mepc, tcntx->trap.tval);
         const struct sbi_trap_info *orig_trap = &tcntx->trap;
         struct sbi_trap_regs *regs = &tcntx->regs;
         struct sbi_trap_info uptrap;

@@ -238,6 +241,8 @@ int sbi_misaligned_v_ld_emulator(ulong insn,
 struct sbi_trap_context *tcntx)
 
  int sbi_misaligned_v_st_emulator(ulong insn, struct sbi_trap_context *tcntx)
  {
+       sbi_printf("%s: insn=0x%lx mepc=0x%lx mtval=0x%lx\n",
+                  __func__, insn, tcntx->regs.mepc, tcntx->trap.tval);
         const struct sbi_trap_info *orig_trap = &tcntx->trap;
         struct sbi_trap_regs *regs = &tcntx->regs;

         struct sbi_trap_info uptrap;

[    0.075576] clocksource: jiffies: mask: 0xffffffff max_cycles:

0xffffffff, max_idle_ns: 7645041785100000 @Z0@Υ0@1] posix`Z0@Υ0@s:
2048 T-0[0@�[0@A0@%�Z0@��0@%�Z0@Υ0@B, linea[0@Υ0@�Z0@ڤ0@T-0[0@ڤ0@T-0@([0@�[0A0@^^�[0@��0@2 KiB GF�0@�[0@[email protected]������~0@'fR0@�0@�[0@(�0@}0@�[0@dit_enab�~0@p}0@�l0@c�����/0@'�o�����a@����

                                        @�\0@`����[    0.155689] cpu1:
Ratio of byte access time to unaligned word access is 0.01, unaligned
accesses are slow
[    0.155689] cpu3: Ratio of byte access time to unaligned word
access is 0.01, unaligned accesses are slow
[    0.183733] cpu0: Ratio of byte access time to unaligned word

access is 0.01, unaligned accesses are slow sbi_misaligned_v_ld_emulator: insn=0x207d007 mepc=0xffffffff80015efc mtval=0xffff8f8000073d51
sbi_misaligned_v_ld_emulator: insn=0x207d007 mepc=0xffffffff80015efc mtval=0xffff8f800013bd51
sbi_misaligned_v_ld_emulator: insn=0x207d007 mepc=0xffffffff80015efc mtval=0xffff8f80000ebd51
sbi_misaligned_v_ld_emulator: insn=0x207d007 mepc=0xffffffff80015efc mtval=0xffff8f8000113d51
sbi_misaligned_v_ld_emulator: insn=0x205e007 mepc=0xffffffff80016fee mtval=0xffffaf8001b72003
sbi_misaligned_v_ld_emulator: insn=0x205e007 mepc=0xffffffff80016fee mtval=0xffffaf8002112003
sbi_misaligned_v_ld_emulator: insn=0x205e007 mepc=0xffffffff80016fee mtval=0xffffaf800214e003
sbi_misaligned_v_ld_emulator: insn=0x205e007 mepc=0xffffffff80016fee mtval=0xffffaf8002026003
sbi_misaligned_v_st_emulator: insn=0x2056027 mepc=0xffffffff80016ff2 mtval=0xffffaf8001b70001
sbi_misaligned_v_st_emulator: insn=0x2056027 mepc=0xffffffff80016ff2 mtval=0xffffaf8002110001
sbi_misaligned_v_st_emulator: insn=0x2056027 mepc=0xffffffff80016ff2 mtval=0xffffaf800214c001
sbi_misaligned_v_st_emulator: insn=0x2056027 mepc=0xffffffff80016ff2 mtval=0xffffaf8002024001
sbi_misaligned_v_ld_emulator: insn=0x205e007 mepc=0xffffffff80016fee mtval=0xffffaf8001b72023
sbi_misaligned_v_ld_emulator: insn=0x205e007 mepc=0xffffffff80016fee mtval=0xffffaf8002112023
sbi_misaligned_v_ld_emulator: insn=0x205e007 mepc=0xffffffff80016fee mtval=0xffffaf800214e023
sbi_misaligned_v_ld_emulator: insn=0x205e007 mepc=0xffffffff80016fee mtval=0xffffaf8002026023
sbi_misaligned_v_st_emulator: insn=0x2056027 mepc=0xffffffff80016ff2 mtval=0xffffaf8001b70021
sbi_misaligned_v_st_emulator: insn=0x2056027 mepc=0xffffffff80016ff2 mtval=0xffffaf8002110021
sbi_misaligned_v_st_emulator: insn=0x2056027 mepc=0xffffffff80016ff2 mtval=0xffffaf800214c021
sbi_misaligned_v_st_emulator: insn=0x2056027 mepc=0xffffffff80016ff2 mtval=0xffffaf8002024021
sbi_misaligned_v_ld_emulator: insn=0x205e007 mepc=0xffffffff80016fee mtval=0xffffaf8001b72043
sbi_misaligned_v_ld_emulator: insn=0x205e007 mepc=0xffffffff80016fee mtval=0xffffaf8002112043
sbi_misaligned_v_ld_emulator: insn=0x205e007 mepc=0xffffffff80016fee mtval=0xffffaf800214e043
sbi_misaligned_v_ld_emulator: insn=0x205e007 mepc=0xffffffff80016fee mtval=0xffffaf8002026043
sbi_misaligned_v_st_emulator: insn=0x2056027 mepc=0xffffffff80016ff2 mtval=0xffffaf8001b70041
sbi_misaligned_v_st_emulator: insn=0x2056027 mepc=0xffffffff80016ff2 mtval=0xffffaf8002110041
sbi_misaligned_v_st_emulator: insn=0x2056027 mepc=0xffffffff80016ff2 mtval=0xffffaf800214c041
sbi_misaligned_v_st_emulator: insn=0x2056027 mepc=0xffffffff80016ff2 mtval=0xffffaf8002024041 sbi_misaligned_v_l�X0@lS0@�X0@�0@f8001b72Y0@Y0@M0@

�0@��������80021120�0@��������emulatoB0@^b�Z0@

                          �0@b�0@`Z0@h�M������

                                              ������0@�6�Z0@bb^

                                                               HL0@B0@

�([0@�Z0@ a��0@"

                 �b�~0@��0@ator: insn=0x205[0@fA0@


                                                 B0@^


                                                     A0@[0@sb

Bumping the stack size seems to fix this. If you want me to test anything else, let me know. Not sure if the tests in PATCH 8 are appropriate to test on this hw (I think they are for KVM guests only?).

[1] - https://lore.kernel.org/linux-riscv/nrvt74qnojaubiwjo37ums4lnclu466hovwrhmtbag6f5uhrql@q6msoe2oto4b/

Quoted 45 lines

- tinst should be zero'ed out to not confuse previous mode when redirecting faults, otherwise the vector insn can be mistaken as a regular load/store.
- VS in previous mode must be set dirty for loads.

These will be addressed in follow-up patches.

[1] https://github.com/ganboing/qemu/tree/ganboing-misalign
[2] https://github.com/ganboing/qemu/tree/ganboing-misalign-no-tinst
[3] https://github.com/ganboing/opensbi/tree/fix-ldst-v2
---
Changes in v2:
- Addressed Anup's comment for PATCH 5 in v1
- Validate load/store offset is 0 in misaligned faults w/ DEBUG build

---
Bo Gan (8):
include: sbi: Add more mstatus and instruction encoding
include: sbi: Add sbi_regs_prev_xlen
include: sbi: Add GET_RDS_NUM/SET(_FP32/_FP64)_RDS macros
include: sbi: set FS dirty in vsstatus when V=1
lib: sbi: Do not override emulator callback for vector load/store
Makefile: define OPENSBI_DEBUG if DEBUG builds
lib: sbi: Rework load/store emulator instruction decoding [NOT-FOR-UPSTREAM] Test program for misaligned load/store

  Makefile                     |   1 +
  include/sbi/riscv_encoding.h |  21 +-
  include/sbi/riscv_fp.h       |  30 ++-
  include/sbi/sbi_platform.h   |  92 +++++--
  include/sbi/sbi_trap.h       |  59 ++++
  include/sbi/sbi_trap_ldst.h  |   4 +-
  lib/sbi/sbi_trap_ldst.c      | 510 ++++++++++++++++++++++++-----------
  lib/sbi/sbi_trap_v_ldst.c    |  25 +-
  tests/ldst.S                 | 134 +++++++++
  tests/ldst.h                 | 170 ++++++++++++
  tests/test-misaligned-ldst.c | 154 +++++++++++
  11 files changed, 994 insertions(+), 206 deletions(-)

create mode 100644 tests/ldst.S
create mode 100644 tests/ldst.h
create mode 100644 tests/test-misaligned-ldst.c

--
2.34.1

Hi Anirudh,

Glad you asked. The stack overflow issue you saw is exactly the same as Vivian reported. I just sent another series to fix the issue among others:

https://lore.kernel.org/opensbi/[email protected]/T/#t

It optimizes stack usage, so we'll never overflow the stack again, also being vlen-agnostic. If you could, please help review and validate, as I don't have any real HW that supports rvv 1.0, except perhaps the quite old K230, which in itself is a pain to test with. Thanks. I'm hesitant to go the default stack-size bump route, as it may cause memory space issues on machines with huge number of cores or tiny embedded ones with limited ram.

BTW, PATCH 8 can be run anywhere, but it's for scalar misaligned ld/st only. It doesn't test vector load/store.

Bo

Quoted 207 lines · Anirudh Srinivasan

Hello Bo,

Quoted 4 lines · Bo Gan

Re-visit the load/store misaligned and access fault handlers to fix issues related to coding patterns, floating-point state, and instruction decoding:

I had previously reported here [1] that there were issues booting into linux after enabling misaligned trap delegation to linux on Sifive X280. In the discussion over there, we concluding that bumping up the per hart stack size in opensbi fixed the issue.

This series (without the stack size bump) also seems to fix the issues that prevented linux from booting. Particularly it was this patch "lib: sbi: Do not override emulator callback for vector load/store".

But as you say, I still think the stack size bump is needed as I was able to break the boot by adding some debug prints like this along the way. I guess this must have somehow caused the stack to overflow?

lib/sbi/sbi_trap_v_ldst.c +5 -0

index 57f12b83..5e596664 100644
@@ -16,6 +16,7 @@
  #include <sbi/sbi_trap.h>
  #include <sbi/sbi_unpriv.h>
  #include <sbi/sbi_trap.h>
+#include <sbi/sbi_console.h>
 
  #ifdef OPENSBI_CC_SUPPORT_VECTOR
 

@@ -139,6 +140,8 @@ static inline void vsetvl(ulong vl, ulong vtype)
 
  int sbi_misaligned_v_ld_emulator(ulong insn, struct sbi_trap_context *tcntx)
  {
+       sbi_printf("%s: insn=0x%lx mepc=0x%lx mtval=0x%lx\n",
+                  __func__, insn, tcntx->regs.mepc, tcntx->trap.tval);
         const struct sbi_trap_info *orig_trap = &tcntx->trap;
         struct sbi_trap_regs *regs = &tcntx->regs;
         struct sbi_trap_info uptrap;

@@ -238,6 +241,8 @@ int sbi_misaligned_v_ld_emulator(ulong insn,
 struct sbi_trap_context *tcntx)
 
  int sbi_misaligned_v_st_emulator(ulong insn, struct sbi_trap_context *tcntx)
  {
+       sbi_printf("%s: insn=0x%lx mepc=0x%lx mtval=0x%lx\n",
+                  __func__, insn, tcntx->regs.mepc, tcntx->trap.tval);
         const struct sbi_trap_info *orig_trap = &tcntx->trap;
         struct sbi_trap_regs *regs = &tcntx->regs;

         struct sbi_trap_info uptrap;

[    0.075576] clocksource: jiffies: mask: 0xffffffff max_cycles:

0xffffffff, max_idle_ns: 7645041785100000 @Z0@Υ0@1] posix`Z0@Υ0@s:
2048 T-0[0@�[0@A0@%�Z0@��0@%�Z0@Υ0@B, linea[0@Υ0@�Z0@ڤ0@T-0[0@ڤ0@T-0@([0@�[0A0@^^�[0@��0@2 KiB GF�0@�[0@[email protected]������~0@'fR0@�0@�[0@(�0@}0@�[0@dit_enab�~0@p}0@�l0@c�����/0@'�o�����a@����

                                        @�\0@`����[    0.155689] cpu1:
Ratio of byte access time to unaligned word access is 0.01, unaligned
accesses are slow
[    0.155689] cpu3: Ratio of byte access time to unaligned word
access is 0.01, unaligned accesses are slow
[    0.183733] cpu0: Ratio of byte access time to unaligned word

access is 0.01, unaligned accesses are slow sbi_misaligned_v_ld_emulator: insn=0x207d007 mepc=0xffffffff80015efc mtval=0xffff8f8000073d51
sbi_misaligned_v_ld_emulator: insn=0x207d007 mepc=0xffffffff80015efc mtval=0xffff8f800013bd51
sbi_misaligned_v_ld_emulator: insn=0x207d007 mepc=0xffffffff80015efc mtval=0xffff8f80000ebd51
sbi_misaligned_v_ld_emulator: insn=0x207d007 mepc=0xffffffff80015efc mtval=0xffff8f8000113d51
sbi_misaligned_v_ld_emulator: insn=0x205e007 mepc=0xffffffff80016fee mtval=0xffffaf8001b72003
sbi_misaligned_v_ld_emulator: insn=0x205e007 mepc=0xffffffff80016fee mtval=0xffffaf8002112003
sbi_misaligned_v_ld_emulator: insn=0x205e007 mepc=0xffffffff80016fee mtval=0xffffaf800214e003
sbi_misaligned_v_ld_emulator: insn=0x205e007 mepc=0xffffffff80016fee mtval=0xffffaf8002026003
sbi_misaligned_v_st_emulator: insn=0x2056027 mepc=0xffffffff80016ff2 mtval=0xffffaf8001b70001
sbi_misaligned_v_st_emulator: insn=0x2056027 mepc=0xffffffff80016ff2 mtval=0xffffaf8002110001
sbi_misaligned_v_st_emulator: insn=0x2056027 mepc=0xffffffff80016ff2 mtval=0xffffaf800214c001
sbi_misaligned_v_st_emulator: insn=0x2056027 mepc=0xffffffff80016ff2 mtval=0xffffaf8002024001
sbi_misaligned_v_ld_emulator: insn=0x205e007 mepc=0xffffffff80016fee mtval=0xffffaf8001b72023
sbi_misaligned_v_ld_emulator: insn=0x205e007 mepc=0xffffffff80016fee mtval=0xffffaf8002112023
sbi_misaligned_v_ld_emulator: insn=0x205e007 mepc=0xffffffff80016fee mtval=0xffffaf800214e023
sbi_misaligned_v_ld_emulator: insn=0x205e007 mepc=0xffffffff80016fee mtval=0xffffaf8002026023
sbi_misaligned_v_st_emulator: insn=0x2056027 mepc=0xffffffff80016ff2 mtval=0xffffaf8001b70021
sbi_misaligned_v_st_emulator: insn=0x2056027 mepc=0xffffffff80016ff2 mtval=0xffffaf8002110021
sbi_misaligned_v_st_emulator: insn=0x2056027 mepc=0xffffffff80016ff2 mtval=0xffffaf800214c021
sbi_misaligned_v_st_emulator: insn=0x2056027 mepc=0xffffffff80016ff2 mtval=0xffffaf8002024021
sbi_misaligned_v_ld_emulator: insn=0x205e007 mepc=0xffffffff80016fee mtval=0xffffaf8001b72043
sbi_misaligned_v_ld_emulator: insn=0x205e007 mepc=0xffffffff80016fee mtval=0xffffaf8002112043
sbi_misaligned_v_ld_emulator: insn=0x205e007 mepc=0xffffffff80016fee mtval=0xffffaf800214e043
sbi_misaligned_v_ld_emulator: insn=0x205e007 mepc=0xffffffff80016fee mtval=0xffffaf8002026043
sbi_misaligned_v_st_emulator: insn=0x2056027 mepc=0xffffffff80016ff2 mtval=0xffffaf8001b70041
sbi_misaligned_v_st_emulator: insn=0x2056027 mepc=0xffffffff80016ff2 mtval=0xffffaf8002110041
sbi_misaligned_v_st_emulator: insn=0x2056027 mepc=0xffffffff80016ff2 mtval=0xffffaf800214c041
sbi_misaligned_v_st_emulator: insn=0x2056027 mepc=0xffffffff80016ff2 mtval=0xffffaf8002024041 sbi_misaligned_v_l�X0@lS0@�X0@�0@f8001b72Y0@Y0@M0@

�0@��������80021120�0@��������emulatoB0@^b�Z0@

                          �0@b�0@`Z0@h�M������

                                              ������0@�6�Z0@bb^

                                                               HL0@B0@

�([0@�Z0@ a��0@"

                 �b�~0@��0@ator: insn=0x205[0@fA0@


                                                 B0@^


                                                     A0@[0@sb

Bumping the stack size seems to fix this. If you want me to test anything else, let me know. Not sure if the tests in PATCH 8 are appropriate to test on this hw (I think they are for KVM guests only?).

[1] - https://lore.kernel.org/linux-riscv/nrvt74qnojaubiwjo37ums4lnclu466hovwrhmtbag6f5uhrql@q6msoe2oto4b/

Quoted 45 lines

- tinst should be zero'ed out to not confuse previous mode when redirecting faults, otherwise the vector insn can be mistaken as a regular load/store.
- VS in previous mode must be set dirty for loads.

These will be addressed in follow-up patches.

[1] https://github.com/ganboing/qemu/tree/ganboing-misalign
[2] https://github.com/ganboing/qemu/tree/ganboing-misalign-no-tinst
[3] https://github.com/ganboing/opensbi/tree/fix-ldst-v2
---
Changes in v2:
- Addressed Anup's comment for PATCH 5 in v1
- Validate load/store offset is 0 in misaligned faults w/ DEBUG build

---
Bo Gan (8):
include: sbi: Add more mstatus and instruction encoding
include: sbi: Add sbi_regs_prev_xlen
include: sbi: Add GET_RDS_NUM/SET(_FP32/_FP64)_RDS macros
include: sbi: set FS dirty in vsstatus when V=1
lib: sbi: Do not override emulator callback for vector load/store
Makefile: define OPENSBI_DEBUG if DEBUG builds
lib: sbi: Rework load/store emulator instruction decoding [NOT-FOR-UPSTREAM] Test program for misaligned load/store

  Makefile                     |   1 +
  include/sbi/riscv_encoding.h |  21 +-
  include/sbi/riscv_fp.h       |  30 ++-
  include/sbi/sbi_platform.h   |  92 +++++--
  include/sbi/sbi_trap.h       |  59 ++++
  include/sbi/sbi_trap_ldst.h  |   4 +-
  lib/sbi/sbi_trap_ldst.c      | 510 ++++++++++++++++++++++++-----------
  lib/sbi/sbi_trap_v_ldst.c    |  25 +-
  tests/ldst.S                 | 134 +++++++++
  tests/ldst.h                 | 170 ++++++++++++
  tests/test-misaligned-ldst.c | 154 +++++++++++
  11 files changed, 994 insertions(+), 206 deletions(-)

create mode 100644 tests/ldst.S
create mode 100644 tests/ldst.h
create mode 100644 tests/test-misaligned-ldst.c

--
2.34.1

Re-visit the load/store misaligned and access fault handlers to fix issues related to coding patterns, floating-point state, and instruction decoding:

a. Vector misaligned load/store emulator is invoked improperly b. vsstatus.FS is not set dirty when V=1
c. No checking of previous XLEN, resulting in wrong insn decoding d. Load/Store base address is wrongly assumed to be trap address e. High 32-bits of tinst needs to be checked against 0

The pathset is validated on a modified QEMU[1] that exposed misaligned faults, and QEMU[2], which further disabled the insn transformation.
[2] covers more insn decoding branches, including the particular ones which can trigger the wrong decoding (c)

The patchset is also validated on Sifive P550 core (ESWIN EIC7700), which is RV64-only in M/(V)S/(V)U, no HW misaligned support, no vector, and mtinst always 0. Refer to [3] for the git repo/branch that has all 6 patches applied along with the test case that exercises all integer and floating-point load/store instructions.

Test case used is available in PATCH 8.

There's no change to the behavior of the vector misaligned load/store handler. However, I've found additional issues with them:

- uint8_t mask[VLEN_MAX / 8] in sbi_trap_v_ldst.c is 8KB, which can overflow the default 4KB stack.
- tinst should be zero'ed out to not confuse previous mode when redirecting faults, otherwise the vector insn can be mistaken as a regular load/store.
- VS in previous mode must be set dirty for loads.

These will be addressed in follow-up patches.

[1] https://github.com/ganboing/qemu/tree/ganboing-misalign
[2] https://github.com/ganboing/qemu/tree/ganboing-misalign-no-tinst
[3] https://github.com/ganboing/opensbi/tree/fix-ldst-v2
---
Changes in v2:
- Addressed Anup's comment for PATCH 5 in v1
- Validate load/store offset is 0 in misaligned faults w/ DEBUG build

---
Bo Gan (8):
include: sbi: Add more mstatus and instruction encoding
include: sbi: Add sbi_regs_prev_xlen
include: sbi: Add GET_RDS_NUM/SET(_FP32/_FP64)_RDS macros
include: sbi: set FS dirty in vsstatus when V=1
lib: sbi: Do not override emulator callback for vector load/store
Makefile: define OPENSBI_DEBUG if DEBUG builds
lib: sbi: Rework load/store emulator instruction decoding [NOT-FOR-UPSTREAM] Test program for misaligned load/store

 Makefile                     |   1 +
 include/sbi/riscv_encoding.h |  21 +-
 include/sbi/riscv_fp.h       |  30 ++-
 include/sbi/sbi_platform.h   |  92 +++++--
 include/sbi/sbi_trap.h       |  59 ++++
 include/sbi/sbi_trap_ldst.h  |   4 +-
 lib/sbi/sbi_trap_ldst.c      | 510 ++++++++++++++++++++++++-----------
 lib/sbi/sbi_trap_v_ldst.c    |  25 +-
 tests/ldst.S                 | 134 +++++++++
 tests/ldst.h                 | 170 ++++++++++++
 tests/test-misaligned-ldst.c | 154 +++++++++++
 11 files changed, 994 insertions(+), 206 deletions(-)
 create mode 100644 tests/ldst.S
 create mode 100644 tests/ldst.h
 create mode 100644 tests/test-misaligned-ldst.c

--
2.34.1

--
opensbi mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/opensbi