[PATCH v2 5/8] lib: sbi: Do not override emulator callback for vector load/store
Anup Patel
apatel at ventanamicro.com
Tue Jun 16 01:17:43 PDT 2026
On Fri, Jun 5, 2026 at 5:04 PM Bo Gan <ganboing at gmail.com> wrote:
>
> 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 <ganboing at gmail.com>
LGTM.
Reviewed-by: Anup Patel <anup at brainfault.org>
Thanks,
Anup
> ---
> 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(-)
>
> diff --git a/include/sbi/sbi_platform.h b/include/sbi/sbi_platform.h
> index fe382b56..525cde51 100644
> --- a/include/sbi/sbi_platform.h
> +++ b/include/sbi/sbi_platform.h
> @@ -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;
> }
> diff --git a/include/sbi/sbi_trap_ldst.h b/include/sbi/sbi_trap_ldst.h
> index a6a6c75b..33c348c5 100644
> --- a/include/sbi/sbi_trap_ldst.h
> +++ b/include/sbi/sbi_trap_ldst.h
> @@ -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
> diff --git a/lib/sbi/sbi_trap_ldst.c b/lib/sbi/sbi_trap_ldst.c
> index 448406b1..ad19926a 100644
> --- a/lib/sbi/sbi_trap_ldst.c
> +++ b/lib/sbi/sbi_trap_ldst.c
> @@ -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)
> diff --git a/lib/sbi/sbi_trap_v_ldst.c b/lib/sbi/sbi_trap_v_ldst.c
> index f4d469dc..57f12b83 100644
> --- a/lib/sbi/sbi_trap_v_ldst.c
> +++ b/lib/sbi/sbi_trap_v_ldst.c
> @@ -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
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
More information about the opensbi
mailing list