Fixes for vector misaligned load/store handlers
cover Fixes for vector misaligned load/store handlers
Re-visit the vector misaligned load/store handlers and fix:
a. Avoid stack overflow by using a small sliding mask[] buffer,
thus optimizes stack usage *IMPORTANT* (correctness). There's no- longer a need to have a pre-defined vlen maximum, and worry about whether the stack can hold the mask[] variable.
b. Maintain the value of vstart when redirecting uptrap. (optmization)
Avoids doing duplicate work when the instruction gets restarted.
c. Explicitly set VS dirty in (V)SSTATUS. (correctness), VS in
VSSTATUS must be set dirty if coming from V=1.
d. Zero out tinst in uptrap if not guest-page fault (correctness).
This is a follow up patch to [1] and should be applied on top.
[1] https://lore.kernel.org/opensbi/CAEev2e_Vg1mMP4mOKanFX_EGeUzpczRcWW++vBBuN1CfyM0BJw@mail.gmail.com/T/#t
Bo Gan (4):
lib: sbi: cosmetic changes to reduce indentation
lib: sbi: Rework and split sbi_misaligned(_v)_tinst_fixup
lib: sbi: Add variable-length unprivilege access functions
lib: sbi: Rework misaligned vector load/store
include/sbi/sbi_trap_ldst.h | 3 - include/sbi/sbi_unpriv.h | 16 +++ include/sbi/sbi_vector.h | 6 ++ lib/sbi/sbi_trap_ldst.c | 66 +++++++----- lib/sbi/sbi_trap_v_ldst.c | 198 +++++++++++++++++++++++------------- lib/sbi/sbi_unpriv.c | 88 ++++++++++++++-- 6 files changed, 269 insertions(+), 108 deletions(-)
Signature · 7 lines
2.34.1 -- opensbi mailing list [email protected] http://lists.infradead.org/mailman/listinfo/opensbi
PATCH 1/4 lib: sbi: cosmetic changes to reduce indentation
In preparation for subsequent patches
--- lib/sbi/sbi_trap_v_ldst.c | 117 +++++++++++++++++++------------------- 1 file changed, 60 insertions(+), 57 deletions(-)
lib/sbi/sbi_trap_v_ldst.c +60 -57
@@ -11,6 +11,7 @@#include <sbi/riscv_asm.h> #include <sbi/riscv_encoding.h> +#include <sbi/sbi_bitops.h> #include <sbi/sbi_error.h> #include <sbi/sbi_trap_ldst.h> #include <sbi/sbi_trap.h>
@@ -189,44 +190,45 @@ int sbi_misaligned_v_ld_emulator(ulong insn, struct sbi_trap_context *tcntx)get_vreg (vlenb,0 ,0 , vlenb, mask);do { -if (!masked || ((mask[vstart /8 ] >> (vstart %8 )) &1 )) { -/* compute element address */ -ulong addr = base + vstart * stride; +if (masked && (~mask[vstart /8 ] &BIT (vstart %8 ))) +continue ; -if (IS_INDEXED_LOAD (insn)) { -ulong offset =0 ; +/* compute element address */ +ulong addr = base + vstart * stride; -get_vreg (vlenb, vs2, vstart << view,1 << view, (uint8_t *)&offset); - addr = base + offset; - } +if (IS_INDEXED_LOAD (insn)) { +ulong offset =0 ; + +get_vreg (vlenb, vs2, vstart << view,1 << view, (uint8_t *)&offset); + addr = base + offset; + } -csr_write (CSR_VSTART , vstart); - -/* obtain load data from memory */ -for (ulong seg =0 ; seg < nf; seg++) { -for (ulong i =0 ; i < len; i++) { - bytes[seg * len + i] = -sbi_load_u8 ((void *)(addr + seg * len + i), - &uptrap); - -if (uptrap.cause) { -if (IS_FAULT_ONLY_FIRST_LOAD (insn) && vstart !=0 ) { - vl = vstart; -break ; - } -vsetvl (vl, vtype); - uptrap.tinst =sbi_misaligned_tinst_fixup ( - orig_trap->tinst, uptrap.tinst, i); -return sbi_trap_redirect (regs, &uptrap); +csr_write (CSR_VSTART , vstart); + +/* obtain load data from memory */ +for (ulong seg =0 ; seg < nf; seg++) { +for (ulong i =0 ; i < len; i++) { + bytes[seg * len + i] = +sbi_load_u8 ((void *)(addr + seg * len + i), + &uptrap); + +if (uptrap.cause) { +if (IS_FAULT_ONLY_FIRST_LOAD (insn) && vstart !=0 ) { + vl = vstart; +break ; } +vsetvl (vl, vtype); + uptrap.tinst =sbi_misaligned_tinst_fixup ( + orig_trap->tinst, uptrap.tinst, i); +return sbi_trap_redirect (regs, &uptrap); } } - -/* write load data to regfile */ -for (ulong seg =0 ; seg < nf; seg++) -set_vreg (vlenb, vd + seg * emul, vstart * len, - len, &bytes[seg * len]); } + +/* write load data to regfile */ +for (ulong seg =0 ; seg < nf; seg++) +set_vreg (vlenb, vd + seg * emul, vstart * len, + len, &bytes[seg * len]); }while (++vstart < vl);/* restore clobbered vl/vtype */
@@ -288,35 +290,36 @@ int sbi_misaligned_v_st_emulator(ulong insn, struct sbi_trap_context *tcntx)get_vreg (vlenb,0 ,0 , vlenb, mask); do { - if (!masked || ((mask[vstart / 8] >> (vstart % 8)) & 1)) { - /* compute element address */ - ulong addr = base + vstart * stride; + if (masked && (~mask[vstart / 8] & BIT(vstart % 8))) + continue; - if (IS_INDEXED_STORE(insn)) { - ulong offset = 0; + /* compute element address */ + ulong addr = base + vstart * stride; - get_vreg(vlenb, vs2, vstart << view, 1 << view, (uint8_t *)&offset); - addr = base + offset; - } + if (IS_INDEXED_STORE(insn)) { + ulong offset = 0; - /* obtain store data from regfile */ - for (ulong seg = 0; seg < nf; seg++) - get_vreg(vlenb, vd + seg * emul, vstart * len, - len, &bytes[seg * len]); - - csr_write(CSR_VSTART, vstart); - - /* write store data to memory */ - for (ulong seg = 0; seg < nf; seg++) { - for (ulong i = 0; i < len; i++) { - sbi_store_u8((void *)(addr + seg * len + i), - bytes[seg * len + i], &uptrap); - if (uptrap.cause) { - vsetvl(vl, vtype); - uptrap.tinst = sbi_misaligned_tinst_fixup( - orig_trap->tinst, uptrap.tinst, i); - return sbi_trap_redirect(regs, &uptrap); - } + get_vreg(vlenb, vs2, vstart << view, 1 << view, (uint8_t *)&offset); + addr = base + offset; + } + + /* obtain store data from regfile */ + for (ulong seg = 0; seg < nf; seg++) + get_vreg(vlenb, vd + seg * emul, vstart * len, + len, &bytes[seg * len]); + + csr_write(CSR_VSTART, vstart); + + /* write store data to memory */ + for (ulong seg = 0; seg < nf; seg++) { + for (ulong i = 0; i < len; i++) { + sbi_store_u8((void *)(addr + seg * len + i), + bytes[seg * len + i], &uptrap); + if (uptrap.cause) { + vsetvl(vl, vtype); + uptrap.tinst = sbi_misaligned_tinst_fixup( + orig_trap->tinst, uptrap.tinst, i); + return sbi_trap_redirect(regs, &uptrap); } } }
Signature · 7 lines
2.34.1 -- opensbi mailing list [email protected] http://lists.infradead.org/mailman/listinfo/opensbi
PATCH 2/4 lib: sbi: Rework and split sbi_misaligned(_v)_tinst_fixup
In reply to Bo GanThe load/store address offset between the uptrap and the orig_trap can be derived by orig_trap->tval - uptrap->tval, thus refactor the function prototype for simplicity.
For vector load, sbi_misaligned_v_tinst_fixup is introduced. There's no transformed instruction for vector load/store, so null out tinst if the fault is not a guest-page fault.
--- include/sbi/sbi_trap_ldst.h | 3 --- lib/sbi/sbi_trap_ldst.c | 46 +++++++++++++++++++++++++++---------- lib/sbi/sbi_trap_v_ldst.c | 31 ++++++++++++++++++++----- 3 files changed, 59 insertions(+), 21 deletions(-)
include/sbi/sbi_trap_ldst.h +0 -3
@@ -28,9 +28,6 @@ int sbi_load_access_handler(struct sbi_trap_context *tcntx);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 (ulong insn,struct sbi_trap_context *tcntx);
lib/sbi/sbi_trap_ldst.c +34 -12
66 lines not on this page · raw message
lib/sbi/sbi_trap_v_ldst.c +25 -6
@@ -138,9 +138,31 @@ static inline void vsetvl(ulong vl, ulong vtype) :: "r" (vl), "r" (vtype)); } +/** +* Handling of misaligned fault is done by a collection of smaller, but +* aligned load/store(s). Another fault (load/store, page fault...) can +* arise from any of them, then the handling gets aborted. We must fixup +* the tinst to pretend the fault was rised from the original insn. For +* vector insn, simply null out tinst if it's not a guest-page fault, as +* there's no transformed insn for vector load/store +*/ +static inline void sbi_misaligned_v_tinst_fixup (struct sbi_trap_info *uptrap) +{ +/* +* The function is called in code path for handling a vector +* load/store misaligned fault, thus the new uptrap can't have +* custom value of tinst +*/ +if (uptrap->tinst ==INSN_PSEUDO_VS_LOAD || + uptrap->tinst ==INSN_PSEUDO_VS_STORE ) +/* Use uptrap as-is for guest-page faults */ +return ; + + uptrap->tinst =0 ; +} +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 vl =csr_read (CSR_VL );
@@ -218,8 +240,7 @@ int sbi_misaligned_v_ld_emulator(ulong insn, struct sbi_trap_context *tcntx)break ; }vsetvl (vl, vtype); - uptrap.tinst =sbi_misaligned_tinst_fixup ( - orig_trap->tinst, uptrap.tinst, i); +sbi_misaligned_v_tinst_fixup (&uptrap);return sbi_trap_redirect (regs, &uptrap); } }
@@ -240,7 +261,6 @@ 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) { -const struct sbi_trap_info *orig_trap = &tcntx->trap;struct sbi_trap_regs *regs = &tcntx->regs;struct sbi_trap_info uptrap;ulong vl =csr_read (CSR_VL );
@@ -317,8 +337,7 @@ int sbi_misaligned_v_st_emulator(ulong insn, struct sbi_trap_context *tcntx) bytes[seg * len + i], &uptrap);if (uptrap.cause) {vsetvl (vl, vtype); - uptrap.tinst =sbi_misaligned_tinst_fixup ( - orig_trap->tinst, uptrap.tinst, i); +sbi_misaligned_v_tinst_fixup (&uptrap);return sbi_trap_redirect (regs, &uptrap); } }
Signature · 7 lines
2.34.1 -- opensbi mailing list [email protected] http://lists.infradead.org/mailman/listinfo/opensbi
PATCH 3/4 lib: sbi: Add variable-length unprivilege access functions
In reply to Bo Gansbi_load/store_loop read/write variable-length buffer unprivileged. Both function use the widest aligned 8/4/2/1 byte load/stores in each loop to reduce the total number of iterations.
Also switch the scalar/vector misaligned handlers to make use of such functions to simplify code.
Miscellaneous: remove the unnecessary [taddr] in inline assembly
--- include/sbi/sbi_unpriv.h | 16 +++++++ lib/sbi/sbi_trap_ldst.c | 24 ++++------- lib/sbi/sbi_trap_v_ldst.c | 38 ++++++++--------- lib/sbi/sbi_unpriv.c | 88 ++++++++++++++++++++++++++++++++++++--- 4 files changed, 123 insertions(+), 43 deletions(-)
include/sbi/sbi_unpriv.h +16 -0
@@ -15,6 +15,16 @@struct sbi_scratch ;struct sbi_trap_info ; +union sbi_unpriv_data { +u8 b; +u16 h; +u32 w; +#if __riscv_xlen ==64 +u64 d; +#endif +u8 bytes[__riscv_xlen /8 ]; +}; +#define DECLARE_UNPRIVILEGED_LOAD_FUNCTION (type) \ type sbi_load_##type(const type *addr, \ struct sbi_trap_info *trap);
@@ -36,6 +46,12 @@ DECLARE_UNPRIVILEGED_LOAD_FUNCTION(u64) DECLARE_UNPRIVILEGED_STORE_FUNCTION(u64) DECLARE_UNPRIVILEGED_LOAD_FUNCTION(ulong) +void sbi_load_loop(u8 *buffer, ulong addr, ulong len, + struct sbi_trap_info *trap); + +void sbi_store_loop(u8 *buffer, ulong addr, ulong len, + struct sbi_trap_info *trap); + ulong sbi_get_insn(ulong mepc, struct sbi_trap_info *trap);#endif
lib/sbi/sbi_trap_ldst.c +8 -16
@@ -471,7 +471,6 @@ static int sbi_misaligned_ld_emulator(ulong insn, int rlen, ulong addr,const struct sbi_trap_info *orig_trap = &tcntx->trap;struct sbi_trap_regs *regs = &tcntx->regs;struct sbi_trap_info uptrap; -int i;if (!rlen) {if (IS_VECTOR_LOAD_STORE (insn))
@@ -484,13 +483,10 @@ static int sbi_misaligned_ld_emulator(ulong insn, int rlen, ulong addr,if (addr != orig_trap->tval)return SBI_EFAIL ; -for (i =0 ; i < rlen; i++) { - out_val->data_bytes[i] = -sbi_load_u8 ((void *)(addr + i), &uptrap); -if (uptrap.cause) { -sbi_misaligned_tinst_fixup (orig_trap, &uptrap); -return sbi_trap_redirect (regs, &uptrap); - } +sbi_load_loop (out_val->data_bytes, addr, rlen, &uptrap); +if (uptrap.cause) { +sbi_misaligned_tinst_fixup (orig_trap, &uptrap); +return sbi_trap_redirect (regs, &uptrap); }return rlen; }
@@ -507,7 +503,6 @@ static int sbi_misaligned_st_emulator(ulong insn, int wlen, ulong addr,const struct sbi_trap_info *orig_trap = &tcntx->trap;struct sbi_trap_regs *regs = &tcntx->regs;struct sbi_trap_info uptrap; -int i;if (!wlen) {if (IS_VECTOR_LOAD_STORE (insn))
@@ -520,13 +515,10 @@ static int sbi_misaligned_st_emulator(ulong insn, int wlen, ulong addr,if (addr != orig_trap->tval)return SBI_EFAIL ; -for (i =0 ; i < wlen; i++) { -sbi_store_u8 ((void *)(addr + i), - in_val.data_bytes[i], &uptrap); -if (uptrap.cause) { -sbi_misaligned_tinst_fixup (orig_trap, &uptrap); -return sbi_trap_redirect (regs, &uptrap); - } +sbi_store_loop (in_val.data_bytes, addr, wlen, &uptrap); +if (uptrap.cause) { +sbi_misaligned_tinst_fixup (orig_trap, &uptrap); +return sbi_trap_redirect (regs, &uptrap); }return wlen; }
lib/sbi/sbi_trap_v_ldst.c +17 -21
@@ -229,20 +229,17 @@ int sbi_misaligned_v_ld_emulator(ulong insn, struct sbi_trap_context *tcntx)/* obtain load data from memory */ for (ulong seg =0 ; seg < nf; seg++) { -for (ulong i =0 ; i < len; i++) { - bytes[seg * len + i] = -sbi_load_u8 ((void *)(addr + seg * len + i), - &uptrap); - -if (uptrap.cause) { -if (IS_FAULT_ONLY_FIRST_LOAD (insn) && vstart !=0 ) { - vl = vstart; -break ; - } -vsetvl (vl, vtype); -sbi_misaligned_v_tinst_fixup (&uptrap); -return sbi_trap_redirect (regs, &uptrap); +sbi_load_loop (bytes + seg * len, + addr + seg * len, len, &uptrap); + +if (uptrap.cause) { +if (IS_FAULT_ONLY_FIRST_LOAD (insn) && vstart !=0 ) { + vl = vstart; +break ; } +vsetvl (vl, vtype); +sbi_misaligned_v_tinst_fixup (&uptrap); +return sbi_trap_redirect (regs, &uptrap); } }
@@ -332,14 +329,13 @@ int sbi_misaligned_v_st_emulator(ulong insn, struct sbi_trap_context *tcntx)/* write store data to memory */ for (ulong seg =0 ; seg < nf; seg++) { -for (ulong i =0 ; i < len; i++) { -sbi_store_u8 ((void *)(addr + seg * len + i), - bytes[seg * len + i], &uptrap); -if (uptrap.cause) { -vsetvl (vl, vtype); -sbi_misaligned_v_tinst_fixup (&uptrap); -return sbi_trap_redirect (regs, &uptrap); - } +sbi_store_loop (bytes + seg * len, + addr + seg * len, len, &uptrap); + +if (uptrap.cause) { +vsetvl (vl, vtype); +sbi_misaligned_v_tinst_fixup (&uptrap); +return sbi_trap_redirect (regs, &uptrap); } } }while (++vstart < vl);
lib/sbi/sbi_unpriv.c +82 -6
129 lines not on this page · raw message
Signature · 7 lines
2.34.1 -- opensbi mailing list [email protected] http://lists.infradead.org/mailman/listinfo/opensbi
PATCH 4/4 lib: sbi: Rework misaligned vector load/store
In reply to Bo GanFix the following issues with misaligned vector load/store:
a. Stack overflow: the mask[VLEN_MAX / 8] variable consumes 8K stack space, given VLEN_MAX=65536, overflowing the default-sized stack. There's no need to fetch the whole mask in one go, instead, make it on-demand. Use a 128-byte mask as local buffer to hold the sliding window of mask. For rvv load, this is allowed -- from the spec:
"The destination vector register group for a masked vector
instruction cannot overlap the source mask register (v0), unless the destination vector register is being written with a mask value (e.g., compares) or the scalar result of a reduction"
We don't need to worry about the mask getting overwritten.
b. Maintain the value of vstart upon abort (uptrap) to avoid duplicate work. After fault resolution, the instruction can restart from the faulting vstart. For Fault-Only-First loads, reset vstart to 0, as previously done so, to conform to spec.
c. Explicitly set VS dirty in VSSTATUS with SET_VS_DIRTY() if faulting from V=1, and if any vector register, including vstart/vl/vtype, gets changed in the handler. It can add 1 unnecessary op to set VS dirty in M/SSTATUS (not VSSTATUS), where the HW already did, but for code simplicity, do it anyway. The overhead should be negligible.
--- include/sbi/sbi_vector.h | 6 +++ lib/sbi/sbi_trap_v_ldst.c | 100 +++++++++++++++++++++++++------------- 2 files changed, 73 insertions(+), 33 deletions(-)
include/sbi/sbi_vector.h +6 -0
@@ -20,6 +20,12 @@ struct sbi_vector_context {uint8_t vregs[]; }; +#define SET_VS_DIRTY (regs) do { \ + if (sbi_regs_from_virt(regs)) \ + csr_set(CSR_VSSTATUS, MSTATUS_VS); \ + regs->mstatus |= MSTATUS_VS; \ +} while(0) +#ifdef OPENSBI_CC_SUPPORT_VECTOR void sbi_vector_save (struct sbi_vector_context *dst);void sbi_vector_restore (const struct sbi_vector_context *src);
lib/sbi/sbi_trap_v_ldst.c +67 -33
183 lines not on this page · raw message
Signature · 7 lines
2.34.1 -- opensbi mailing list [email protected] http://lists.infradead.org/mailman/listinfo/opensbi
Please disregard this series and use v2 instead:
https://lore.kernel.org/opensbi/[email protected]/
Made some mistake during merge, and the PATCH 4/4 is messed up. sorry.
Bo
Quoted 32 lines · Bo Gan
Re-visit the vector misaligned load/store handlers and fix:
a. Avoid stack overflow by using a small sliding mask[] buffer,
thus optimizes stack usage *IMPORTANT* (correctness). There's no- longer a need to have a pre-defined vlen maximum, and worry about whether the stack can hold the mask[] variable.b. Maintain the value of vstart when redirecting uptrap. (optmization)
Avoids doing duplicate work when the instruction gets restarted.c. Explicitly set VS dirty in (V)SSTATUS. (correctness), VS in
VSSTATUS must be set dirty if coming from V=1.d. Zero out tinst in uptrap if not guest-page fault (correctness).
This is a follow up patch to [1] and should be applied on top.
[1] https://lore.kernel.org/opensbi/CAEev2e_Vg1mMP4mOKanFX_EGeUzpczRcWW++vBBuN1CfyM0BJw@mail.gmail.com/T/#tBo Gan (4):
lib: sbi: cosmetic changes to reduce indentation
lib: sbi: Rework and split sbi_misaligned(_v)_tinst_fixup
lib: sbi: Add variable-length unprivilege access functions
lib: sbi: Rework misaligned vector load/storeinclude/sbi/sbi_trap_ldst.h | 3 - include/sbi/sbi_unpriv.h | 16 +++ include/sbi/sbi_vector.h | 6 ++ lib/sbi/sbi_trap_ldst.c | 66 +++++++----- lib/sbi/sbi_trap_v_ldst.c | 198 +++++++++++++++++++++++------------- lib/sbi/sbi_unpriv.c | 88 ++++++++++++++-- 6 files changed, 269 insertions(+), 108 deletions(-)
Signature · 3 lines
opensbi mailing list [email protected] http://lists.infradead.org/mailman/listinfo/opensbi