| Message ID | 20230626165736.65927-1-bjorn@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] riscv: Discard vector state on syscalls | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/cover_letter | success | Single patches do not need cover letters |
| conchuod/tree_selection | success | Guessed tree name to be for-next at HEAD 488833ccdcac |
| conchuod/fixes_present | success | Fixes tag not required for -next series |
| conchuod/maintainers_pattern | success | MAINTAINERS pattern errors before the patch: 6 and now 6 |
| conchuod/verify_signedoff | success | Signed-off-by tag matches author and committer |
| conchuod/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/build_rv64_clang_allmodconfig | success | Errors and warnings before: 12 this patch: 12 |
| conchuod/module_param | success | Was 0 now: 0 |
| conchuod/build_rv64_gcc_allmodconfig | success | Errors and warnings before: 120 this patch: 120 |
| conchuod/build_rv32_defconfig | success | Build OK |
| conchuod/dtb_warn_rv64 | success | Errors and warnings before: 20 this patch: 20 |
| conchuod/header_inline | success | No static functions without inline keyword in header files |
| conchuod/checkpatch | warning | CHECK: Lines should not end with a '(' |
| conchuod/build_rv64_nommu_k210_defconfig | success | Build OK |
| conchuod/verify_fixes | success | No Fixes tag |
| conchuod/build_rv64_nommu_virt_defconfig | success | Build OK |
Björn Töpel <bjorn@kernel.org> writes: > From: Björn Töpel <bjorn@rivosinc.com> > > The RISC-V vector specification states: > Executing a system call causes all caller-saved vector registers > (v0-v31, vl, vtype) and vstart to become unspecified. A bit of a corner case, but this will make sigreturn syscalls discard the vector state as well. Is that an issue? E.g. a user cannot build userspace context switching application. Does arm64 SVE handle sigreturn in a special way? Björn
Björn Töpel <bjorn@kernel.org> writes: > Björn Töpel <bjorn@kernel.org> writes: > >> From: Björn Töpel <bjorn@rivosinc.com> >> >> The RISC-V vector specification states: >> Executing a system call causes all caller-saved vector registers >> (v0-v31, vl, vtype) and vstart to become unspecified. > > A bit of a corner case, but this will make sigreturn syscalls discard > the vector state as well. > > Is that an issue? E.g. a user cannot build userspace context switching > application. Does arm64 SVE handle sigreturn in a special way? NVM; My bad. The vector state is cleared on *entry*, but then the registers passed on signal stack is restored as usual. Sorry for the noise! We're all good! Björn
[Resend with different MTA] Le 26 juin 2023 19:45:20 GMT+02:00, "Björn Töpel" <bjorn@kernel.org> a écrit : >Björn Töpel <bjorn@kernel.org> writes: > >> From: Björn Töpel <bjorn@rivosinc.com> >> >> The RISC-V vector specification states: >> Executing a system call causes all caller-saved vector registers >> (v0-v31, vl, vtype) and vstart to become unspecified. > >A bit of a corner case, but this will make sigreturn syscalls discard >the vector state as well. > >Is that an issue? E.g. a user cannot build userspace context switching >application. Does arm64 SVE handle sigreturn in a special way? Isn't sigreturn() supposed to return the status from the arch-dependent machine state within the siginfo structure, rather than whatever was saved on sigreturn() syscall entry? That being the case, I think throwing the vector register bank away on *entry* of sigreturn() is fine as with any other syscall, but the state must *not* be cleared on syscall exit. An example usecase would be emulating RVV extensions (on a CPU supporting baseline RVV 1.0) with a SIGILL handler.
On Tue, Jun 27, 2023 at 12:57 AM Björn Töpel <bjorn@kernel.org> wrote: > > From: Björn Töpel <bjorn@rivosinc.com> > > The RISC-V vector specification states: > Executing a system call causes all caller-saved vector registers > (v0-v31, vl, vtype) and vstart to become unspecified. > > The vector registers are set to all 1s, vill is set (invalid), and the > vector status is set to Initial. > > That way we can prevent userspace from accidentally relying on the > stated save. > > Rémi pointed out [1] that writing to the registers might be > superfluous, and setting vill is sufficient. > > Link: https://lore.kernel.org/linux-riscv/12784326.9UPPK3MAeB@basile.remlab.net/ # [1] > Suggested-by: Darius Rad <darius@bluespec.com> > Suggested-by: Palmer Dabbelt <palmer@rivosinc.com> > Suggested-by: Rémi Denis-Courmont <remi@remlab.net> > Signed-off-by: Björn Töpel <bjorn@rivosinc.com> > --- > v1->v2: > Proper register restore for initial state (Andy) > Set registers to 1s, and not 0s (Darius) > --- > arch/riscv/include/asm/vector.h | 42 ++++++++++++++++++++++++++++++--- > arch/riscv/kernel/traps.c | 2 ++ > 2 files changed, 41 insertions(+), 3 deletions(-) > > diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h > index 04c0b07bf6cd..93d702d9988c 100644 > --- a/arch/riscv/include/asm/vector.h > +++ b/arch/riscv/include/asm/vector.h > @@ -139,14 +139,49 @@ static inline void riscv_v_vstate_save(struct task_struct *task, > } > } > > +static inline void __riscv_v_vstate_discard(void) > +{ > + unsigned long vl, vtype_inval = 1UL << (BITS_PER_LONG - 1); > + > + riscv_v_enable(); > + asm volatile ( > + ".option push\n\t" > + ".option arch, +v\n\t" > + "vsetvli %0, x0, e8, m8, ta, ma\n\t" > + "vmv.v.i v0, -1\n\t" > + "vmv.v.i v8, -1\n\t" > + "vmv.v.i v16, -1\n\t" > + "vmv.v.i v24, -1\n\t" > + "vsetvl %0, x0, %1\n\t" > + ".option pop\n\t" > + : "=&r" (vl) : "r" (vtype_inval) : "memory"); > + riscv_v_disable(); > +} > + > +static inline void riscv_v_vstate_discard(struct pt_regs *regs) > +{ > + if (!riscv_v_vstate_query(regs)) > + return; > + > + __riscv_v_vstate_discard(); > + riscv_v_vstate_on(regs); > +} > + > static inline void riscv_v_vstate_restore(struct task_struct *task, > struct pt_regs *regs) > { > - if ((regs->status & SR_VS) != SR_VS_OFF) { > - struct __riscv_v_ext_state *vstate = &task->thread.vstate; > - > + struct __riscv_v_ext_state *vstate = &task->thread.vstate; > + unsigned long status = regs->status & SR_VS; > + > + switch (status) { > + case SR_VS_INITIAL: > + __riscv_v_vstate_discard(); > + break; > + case SR_VS_CLEAN: > + case SR_VS_DIRTY: > __riscv_v_vstate_restore(vstate, vstate->datap); > __riscv_v_vstate_clean(regs); > + break; > } > } > > @@ -178,6 +213,7 @@ static inline bool riscv_v_vstate_ctrl_user_allowed(void) { return false; } > #define __switch_to_vector(__prev, __next) do {} while (0) > #define riscv_v_vstate_off(regs) do {} while (0) > #define riscv_v_vstate_on(regs) do {} while (0) > +#define riscv_v_vstate_discard(regs) do {} while (0) > > #endif /* CONFIG_RISCV_ISA_V */ > > diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c > index 5158961ea977..5ff63a784a6d 100644 > --- a/arch/riscv/kernel/traps.c > +++ b/arch/riscv/kernel/traps.c > @@ -296,6 +296,8 @@ asmlinkage __visible __trap_section void do_trap_ecall_u(struct pt_regs *regs) > regs->epc += 4; > regs->orig_a0 = regs->a0; > > + riscv_v_vstate_discard(regs); > + > syscall = syscall_enter_from_user_mode(regs, syscall); > > if (syscall < NR_syscalls) > > base-commit: 488833ccdcac118da16701f4ee0673b20ba47fe3 > -- > 2.39.2 > Hi, the above part looks good to me. In the context of kernel-mode vector, it would also be good to just discard V-context at the syscall entry. So the kernel can freely use Vector if needed. I will rebase my work on top of yours. Another part that just came into my mind is the one for ptrace. Do we need to disallow, or immediately return all -1 if the tracee process is in the syscall path? It seems that we are likely to get stale values on datap if a tracee is being traced during a syscall. Thanks, Andy
Andy Chiu <andy.chiu@sifive.com> writes: > On Tue, Jun 27, 2023 at 12:57 AM Björn Töpel <bjorn@kernel.org> wrote: >> >> From: Björn Töpel <bjorn@rivosinc.com> >> >> The RISC-V vector specification states: >> Executing a system call causes all caller-saved vector registers >> (v0-v31, vl, vtype) and vstart to become unspecified. >> >> The vector registers are set to all 1s, vill is set (invalid), and the >> vector status is set to Initial. >> >> That way we can prevent userspace from accidentally relying on the >> stated save. >> >> Rémi pointed out [1] that writing to the registers might be >> superfluous, and setting vill is sufficient. >> >> Link: https://lore.kernel.org/linux-riscv/12784326.9UPPK3MAeB@basile.remlab.net/ # [1] >> Suggested-by: Darius Rad <darius@bluespec.com> >> Suggested-by: Palmer Dabbelt <palmer@rivosinc.com> >> Suggested-by: Rémi Denis-Courmont <remi@remlab.net> >> Signed-off-by: Björn Töpel <bjorn@rivosinc.com> >> --- >> v1->v2: >> Proper register restore for initial state (Andy) >> Set registers to 1s, and not 0s (Darius) >> --- >> arch/riscv/include/asm/vector.h | 42 ++++++++++++++++++++++++++++++--- >> arch/riscv/kernel/traps.c | 2 ++ >> 2 files changed, 41 insertions(+), 3 deletions(-) >> >> diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h >> index 04c0b07bf6cd..93d702d9988c 100644 >> --- a/arch/riscv/include/asm/vector.h >> +++ b/arch/riscv/include/asm/vector.h >> @@ -139,14 +139,49 @@ static inline void riscv_v_vstate_save(struct task_struct *task, >> } >> } >> >> +static inline void __riscv_v_vstate_discard(void) >> +{ >> + unsigned long vl, vtype_inval = 1UL << (BITS_PER_LONG - 1); >> + >> + riscv_v_enable(); >> + asm volatile ( >> + ".option push\n\t" >> + ".option arch, +v\n\t" >> + "vsetvli %0, x0, e8, m8, ta, ma\n\t" >> + "vmv.v.i v0, -1\n\t" >> + "vmv.v.i v8, -1\n\t" >> + "vmv.v.i v16, -1\n\t" >> + "vmv.v.i v24, -1\n\t" >> + "vsetvl %0, x0, %1\n\t" >> + ".option pop\n\t" >> + : "=&r" (vl) : "r" (vtype_inval) : "memory"); >> + riscv_v_disable(); >> +} >> + >> +static inline void riscv_v_vstate_discard(struct pt_regs *regs) >> +{ >> + if (!riscv_v_vstate_query(regs)) >> + return; >> + >> + __riscv_v_vstate_discard(); >> + riscv_v_vstate_on(regs); >> +} >> + >> static inline void riscv_v_vstate_restore(struct task_struct *task, >> struct pt_regs *regs) >> { >> - if ((regs->status & SR_VS) != SR_VS_OFF) { >> - struct __riscv_v_ext_state *vstate = &task->thread.vstate; >> - >> + struct __riscv_v_ext_state *vstate = &task->thread.vstate; >> + unsigned long status = regs->status & SR_VS; >> + >> + switch (status) { >> + case SR_VS_INITIAL: >> + __riscv_v_vstate_discard(); >> + break; >> + case SR_VS_CLEAN: >> + case SR_VS_DIRTY: >> __riscv_v_vstate_restore(vstate, vstate->datap); >> __riscv_v_vstate_clean(regs); >> + break; >> } >> } >> >> @@ -178,6 +213,7 @@ static inline bool riscv_v_vstate_ctrl_user_allowed(void) { return false; } >> #define __switch_to_vector(__prev, __next) do {} while (0) >> #define riscv_v_vstate_off(regs) do {} while (0) >> #define riscv_v_vstate_on(regs) do {} while (0) >> +#define riscv_v_vstate_discard(regs) do {} while (0) >> >> #endif /* CONFIG_RISCV_ISA_V */ >> >> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c >> index 5158961ea977..5ff63a784a6d 100644 >> --- a/arch/riscv/kernel/traps.c >> +++ b/arch/riscv/kernel/traps.c >> @@ -296,6 +296,8 @@ asmlinkage __visible __trap_section void do_trap_ecall_u(struct pt_regs *regs) >> regs->epc += 4; >> regs->orig_a0 = regs->a0; >> >> + riscv_v_vstate_discard(regs); >> + >> syscall = syscall_enter_from_user_mode(regs, syscall); >> >> if (syscall < NR_syscalls) >> >> base-commit: 488833ccdcac118da16701f4ee0673b20ba47fe3 >> -- >> 2.39.2 >> > > Hi, the above part looks good to me. In the context of kernel-mode > vector, it would also be good to just discard V-context at the syscall > entry. So the kernel can freely use Vector if needed. I will rebase my > work on top of yours. Ok! > Another part that just came into my mind is the one for ptrace. Do we > need to disallow, or immediately return all -1 if the tracee process > is in the syscall path? It seems that we are likely to get stale > values on datap if a tracee is being traced during a syscall. Hmm, could you elaborate a bit on when the tracer would get stale regs?
On Wed, Jun 28, 2023 at 6:35 PM Björn Töpel <bjorn@kernel.org> wrote: > > Andy Chiu <andy.chiu@sifive.com> writes: > > > On Tue, Jun 27, 2023 at 12:57 AM Björn Töpel <bjorn@kernel.org> wrote: > >> > >> From: Björn Töpel <bjorn@rivosinc.com> > >> > >> The RISC-V vector specification states: > >> Executing a system call causes all caller-saved vector registers > >> (v0-v31, vl, vtype) and vstart to become unspecified. > >> > >> The vector registers are set to all 1s, vill is set (invalid), and the > >> vector status is set to Initial. > >> > >> That way we can prevent userspace from accidentally relying on the > >> stated save. > >> > >> Rémi pointed out [1] that writing to the registers might be > >> superfluous, and setting vill is sufficient. > >> > >> Link: https://lore.kernel.org/linux-riscv/12784326.9UPPK3MAeB@basile.remlab.net/ # [1] > >> Suggested-by: Darius Rad <darius@bluespec.com> > >> Suggested-by: Palmer Dabbelt <palmer@rivosinc.com> > >> Suggested-by: Rémi Denis-Courmont <remi@remlab.net> > >> Signed-off-by: Björn Töpel <bjorn@rivosinc.com> > >> --- > >> v1->v2: > >> Proper register restore for initial state (Andy) > >> Set registers to 1s, and not 0s (Darius) > >> --- > >> arch/riscv/include/asm/vector.h | 42 ++++++++++++++++++++++++++++++--- > >> arch/riscv/kernel/traps.c | 2 ++ > >> 2 files changed, 41 insertions(+), 3 deletions(-) > >> > >> diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h > >> index 04c0b07bf6cd..93d702d9988c 100644 > >> --- a/arch/riscv/include/asm/vector.h > >> +++ b/arch/riscv/include/asm/vector.h > >> @@ -139,14 +139,49 @@ static inline void riscv_v_vstate_save(struct task_struct *task, > >> } > >> } > >> > >> +static inline void __riscv_v_vstate_discard(void) > >> +{ > >> + unsigned long vl, vtype_inval = 1UL << (BITS_PER_LONG - 1); > >> + > >> + riscv_v_enable(); > >> + asm volatile ( > >> + ".option push\n\t" > >> + ".option arch, +v\n\t" > >> + "vsetvli %0, x0, e8, m8, ta, ma\n\t" > >> + "vmv.v.i v0, -1\n\t" > >> + "vmv.v.i v8, -1\n\t" > >> + "vmv.v.i v16, -1\n\t" > >> + "vmv.v.i v24, -1\n\t" > >> + "vsetvl %0, x0, %1\n\t" > >> + ".option pop\n\t" > >> + : "=&r" (vl) : "r" (vtype_inval) : "memory"); > >> + riscv_v_disable(); > >> +} > >> + > >> +static inline void riscv_v_vstate_discard(struct pt_regs *regs) > >> +{ > >> + if (!riscv_v_vstate_query(regs)) > >> + return; > >> + > >> + __riscv_v_vstate_discard(); > >> + riscv_v_vstate_on(regs); > >> +} > >> + > >> static inline void riscv_v_vstate_restore(struct task_struct *task, > >> struct pt_regs *regs) > >> { > >> - if ((regs->status & SR_VS) != SR_VS_OFF) { > >> - struct __riscv_v_ext_state *vstate = &task->thread.vstate; > >> - > >> + struct __riscv_v_ext_state *vstate = &task->thread.vstate; > >> + unsigned long status = regs->status & SR_VS; > >> + > >> + switch (status) { > >> + case SR_VS_INITIAL: > >> + __riscv_v_vstate_discard(); > >> + break; > >> + case SR_VS_CLEAN: > >> + case SR_VS_DIRTY: > >> __riscv_v_vstate_restore(vstate, vstate->datap); > >> __riscv_v_vstate_clean(regs); > >> + break; > >> } > >> } > >> > >> @@ -178,6 +213,7 @@ static inline bool riscv_v_vstate_ctrl_user_allowed(void) { return false; } > >> #define __switch_to_vector(__prev, __next) do {} while (0) > >> #define riscv_v_vstate_off(regs) do {} while (0) > >> #define riscv_v_vstate_on(regs) do {} while (0) > >> +#define riscv_v_vstate_discard(regs) do {} while (0) > >> > >> #endif /* CONFIG_RISCV_ISA_V */ > >> > >> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c > >> index 5158961ea977..5ff63a784a6d 100644 > >> --- a/arch/riscv/kernel/traps.c > >> +++ b/arch/riscv/kernel/traps.c > >> @@ -296,6 +296,8 @@ asmlinkage __visible __trap_section void do_trap_ecall_u(struct pt_regs *regs) > >> regs->epc += 4; > >> regs->orig_a0 = regs->a0; > >> > >> + riscv_v_vstate_discard(regs); > >> + > >> syscall = syscall_enter_from_user_mode(regs, syscall); > >> > >> if (syscall < NR_syscalls) > >> > >> base-commit: 488833ccdcac118da16701f4ee0673b20ba47fe3 > >> -- > >> 2.39.2 > >> > > > > Hi, the above part looks good to me. In the context of kernel-mode > > vector, it would also be good to just discard V-context at the syscall > > entry. So the kernel can freely use Vector if needed. I will rebase my > > work on top of yours. > > Ok! > > > Another part that just came into my mind is the one for ptrace. Do we > > need to disallow, or immediately return all -1 if the tracee process > > is in the syscall path? It seems that we are likely to get stale > > values on datap if a tracee is being traced during a syscall. > > Hmm, could you elaborate a bit on when the tracer would get stale regs? Yep, consider that our tracer process attaches to a tracee with PTRACE_SYSCALL. Then, the tracee will let the tracer to inspect it whenever it makes a syscall. The tracer wants to inspect V-registers at these PTRACE_SYSCALL stops. Assume the tracee context switches out before being inspected (Sadly I didn't find this part in the code, so maybe I was wrong). Now, we set all V-regs to -1 and VS to 'On' entering a syscall. However, -1 will not be saved into datap, which the tracer copies from, because riscv_v_vstate_save() only saves whenever VS is 'Dirty'. We intentionally want this because it saves unnecessary context saves. As a result, what we will get with REGSET_V will not reflect the latest one, and what we set will get lost since VS='ON' restores V to -1. Since we are planning to discard V registers on syscall, does it make sense to also make ptrace aware of this? Or, just leave it as-it because reading/writing V register at syscall is not meaningful already. Thanks, Andy
Andy Chiu <andy.chiu@sifive.com> writes: > On Wed, Jun 28, 2023 at 6:35 PM Björn Töpel <bjorn@kernel.org> wrote: >> >> Andy Chiu <andy.chiu@sifive.com> writes: >> >> > On Tue, Jun 27, 2023 at 12:57 AM Björn Töpel <bjorn@kernel.org> wrote: >> >> >> >> From: Björn Töpel <bjorn@rivosinc.com> >> >> >> >> The RISC-V vector specification states: >> >> Executing a system call causes all caller-saved vector registers >> >> (v0-v31, vl, vtype) and vstart to become unspecified. >> >> >> >> The vector registers are set to all 1s, vill is set (invalid), and the >> >> vector status is set to Initial. >> >> >> >> That way we can prevent userspace from accidentally relying on the >> >> stated save. >> >> >> >> Rémi pointed out [1] that writing to the registers might be >> >> superfluous, and setting vill is sufficient. >> >> >> >> Link: https://lore.kernel.org/linux-riscv/12784326.9UPPK3MAeB@basile.remlab.net/ # [1] >> >> Suggested-by: Darius Rad <darius@bluespec.com> >> >> Suggested-by: Palmer Dabbelt <palmer@rivosinc.com> >> >> Suggested-by: Rémi Denis-Courmont <remi@remlab.net> >> >> Signed-off-by: Björn Töpel <bjorn@rivosinc.com> >> >> --- >> >> v1->v2: >> >> Proper register restore for initial state (Andy) >> >> Set registers to 1s, and not 0s (Darius) >> >> --- >> >> arch/riscv/include/asm/vector.h | 42 ++++++++++++++++++++++++++++++--- >> >> arch/riscv/kernel/traps.c | 2 ++ >> >> 2 files changed, 41 insertions(+), 3 deletions(-) >> >> >> >> diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h >> >> index 04c0b07bf6cd..93d702d9988c 100644 >> >> --- a/arch/riscv/include/asm/vector.h >> >> +++ b/arch/riscv/include/asm/vector.h >> >> @@ -139,14 +139,49 @@ static inline void riscv_v_vstate_save(struct task_struct *task, >> >> } >> >> } >> >> >> >> +static inline void __riscv_v_vstate_discard(void) >> >> +{ >> >> + unsigned long vl, vtype_inval = 1UL << (BITS_PER_LONG - 1); >> >> + >> >> + riscv_v_enable(); >> >> + asm volatile ( >> >> + ".option push\n\t" >> >> + ".option arch, +v\n\t" >> >> + "vsetvli %0, x0, e8, m8, ta, ma\n\t" >> >> + "vmv.v.i v0, -1\n\t" >> >> + "vmv.v.i v8, -1\n\t" >> >> + "vmv.v.i v16, -1\n\t" >> >> + "vmv.v.i v24, -1\n\t" >> >> + "vsetvl %0, x0, %1\n\t" >> >> + ".option pop\n\t" >> >> + : "=&r" (vl) : "r" (vtype_inval) : "memory"); >> >> + riscv_v_disable(); >> >> +} >> >> + >> >> +static inline void riscv_v_vstate_discard(struct pt_regs *regs) >> >> +{ >> >> + if (!riscv_v_vstate_query(regs)) >> >> + return; >> >> + >> >> + __riscv_v_vstate_discard(); >> >> + riscv_v_vstate_on(regs); >> >> +} >> >> + >> >> static inline void riscv_v_vstate_restore(struct task_struct *task, >> >> struct pt_regs *regs) >> >> { >> >> - if ((regs->status & SR_VS) != SR_VS_OFF) { >> >> - struct __riscv_v_ext_state *vstate = &task->thread.vstate; >> >> - >> >> + struct __riscv_v_ext_state *vstate = &task->thread.vstate; >> >> + unsigned long status = regs->status & SR_VS; >> >> + >> >> + switch (status) { >> >> + case SR_VS_INITIAL: >> >> + __riscv_v_vstate_discard(); >> >> + break; >> >> + case SR_VS_CLEAN: >> >> + case SR_VS_DIRTY: >> >> __riscv_v_vstate_restore(vstate, vstate->datap); >> >> __riscv_v_vstate_clean(regs); >> >> + break; >> >> } >> >> } >> >> >> >> @@ -178,6 +213,7 @@ static inline bool riscv_v_vstate_ctrl_user_allowed(void) { return false; } >> >> #define __switch_to_vector(__prev, __next) do {} while (0) >> >> #define riscv_v_vstate_off(regs) do {} while (0) >> >> #define riscv_v_vstate_on(regs) do {} while (0) >> >> +#define riscv_v_vstate_discard(regs) do {} while (0) >> >> >> >> #endif /* CONFIG_RISCV_ISA_V */ >> >> >> >> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c >> >> index 5158961ea977..5ff63a784a6d 100644 >> >> --- a/arch/riscv/kernel/traps.c >> >> +++ b/arch/riscv/kernel/traps.c >> >> @@ -296,6 +296,8 @@ asmlinkage __visible __trap_section void do_trap_ecall_u(struct pt_regs *regs) >> >> regs->epc += 4; >> >> regs->orig_a0 = regs->a0; >> >> >> >> + riscv_v_vstate_discard(regs); >> >> + >> >> syscall = syscall_enter_from_user_mode(regs, syscall); >> >> >> >> if (syscall < NR_syscalls) >> >> >> >> base-commit: 488833ccdcac118da16701f4ee0673b20ba47fe3 >> >> -- >> >> 2.39.2 >> >> >> > >> > Hi, the above part looks good to me. In the context of kernel-mode >> > vector, it would also be good to just discard V-context at the syscall >> > entry. So the kernel can freely use Vector if needed. I will rebase my >> > work on top of yours. >> >> Ok! >> >> > Another part that just came into my mind is the one for ptrace. Do we >> > need to disallow, or immediately return all -1 if the tracee process >> > is in the syscall path? It seems that we are likely to get stale >> > values on datap if a tracee is being traced during a syscall. >> >> Hmm, could you elaborate a bit on when the tracer would get stale regs? > > Yep, consider that our tracer process attaches to a tracee with > PTRACE_SYSCALL. Then, the tracee will let the tracer to inspect it > whenever it makes a syscall. The tracer wants to inspect V-registers > at these PTRACE_SYSCALL stops. Assume the tracee context switches out > before being inspected (Sadly I didn't find this part in the code, so > maybe I was wrong). Now, we set all V-regs to -1 and VS to 'On' > entering a syscall. However, -1 will not be saved into datap, which > the tracer copies from, because riscv_v_vstate_save() only saves > whenever VS is 'Dirty'. We intentionally want this because it saves > unnecessary context saves. As a result, what we will get with REGSET_V > will not reflect the latest one, and what we set will get lost since > VS='ON' restores V to -1. It's not a racy, but you're correct that setting the state to Initial, will cause issues. When get/set_regs is called, the tracee will be stopped, and a schedule() has been done. Tracee: syscall-->(datap stale; change dirty->initial)-->stopped (datap still stale). Tracer will get stale data. > Since we are planning to discard V registers on syscall, does it make > sense to also make ptrace aware of this? Or, just leave it as-it > because reading/writing V register at syscall is not meaningful > already. Special handling for ptrace is a bit overkill -- at least now. I'll spin a v3, where discard simply sets the state to dirty. Thanks for finding this! Björn
diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h index 04c0b07bf6cd..93d702d9988c 100644 --- a/arch/riscv/include/asm/vector.h +++ b/arch/riscv/include/asm/vector.h @@ -139,14 +139,49 @@ static inline void riscv_v_vstate_save(struct task_struct *task, } } +static inline void __riscv_v_vstate_discard(void) +{ + unsigned long vl, vtype_inval = 1UL << (BITS_PER_LONG - 1); + + riscv_v_enable(); + asm volatile ( + ".option push\n\t" + ".option arch, +v\n\t" + "vsetvli %0, x0, e8, m8, ta, ma\n\t" + "vmv.v.i v0, -1\n\t" + "vmv.v.i v8, -1\n\t" + "vmv.v.i v16, -1\n\t" + "vmv.v.i v24, -1\n\t" + "vsetvl %0, x0, %1\n\t" + ".option pop\n\t" + : "=&r" (vl) : "r" (vtype_inval) : "memory"); + riscv_v_disable(); +} + +static inline void riscv_v_vstate_discard(struct pt_regs *regs) +{ + if (!riscv_v_vstate_query(regs)) + return; + + __riscv_v_vstate_discard(); + riscv_v_vstate_on(regs); +} + static inline void riscv_v_vstate_restore(struct task_struct *task, struct pt_regs *regs) { - if ((regs->status & SR_VS) != SR_VS_OFF) { - struct __riscv_v_ext_state *vstate = &task->thread.vstate; - + struct __riscv_v_ext_state *vstate = &task->thread.vstate; + unsigned long status = regs->status & SR_VS; + + switch (status) { + case SR_VS_INITIAL: + __riscv_v_vstate_discard(); + break; + case SR_VS_CLEAN: + case SR_VS_DIRTY: __riscv_v_vstate_restore(vstate, vstate->datap); __riscv_v_vstate_clean(regs); + break; } } @@ -178,6 +213,7 @@ static inline bool riscv_v_vstate_ctrl_user_allowed(void) { return false; } #define __switch_to_vector(__prev, __next) do {} while (0) #define riscv_v_vstate_off(regs) do {} while (0) #define riscv_v_vstate_on(regs) do {} while (0) +#define riscv_v_vstate_discard(regs) do {} while (0) #endif /* CONFIG_RISCV_ISA_V */ diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c index 5158961ea977..5ff63a784a6d 100644 --- a/arch/riscv/kernel/traps.c +++ b/arch/riscv/kernel/traps.c @@ -296,6 +296,8 @@ asmlinkage __visible __trap_section void do_trap_ecall_u(struct pt_regs *regs) regs->epc += 4; regs->orig_a0 = regs->a0; + riscv_v_vstate_discard(regs); + syscall = syscall_enter_from_user_mode(regs, syscall); if (syscall < NR_syscalls)