From: Björn Töpel <[email protected]>
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/[email protected]/ # [1]
Suggested-by: Darius Rad <[email protected]>
Suggested-by: Palmer Dabbelt <[email protected]>
Suggested-by: Rémi Denis-Courmont <[email protected]>
Signed-off-by: Björn Töpel <[email protected]>
---
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
Björn Töpel <[email protected]> writes:
> From: Björn Töpel <[email protected]>
>
> 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 <[email protected]> writes:
> Björn Töpel <[email protected]> writes:
>
>> From: Björn Töpel <[email protected]>
>>
>> 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
On Tue, Jun 27, 2023 at 12:57 AM Björn Töpel <[email protected]> wrote:
>
> From: Björn Töpel <[email protected]>
>
> 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/[email protected]/ # [1]
> Suggested-by: Darius Rad <[email protected]>
> Suggested-by: Palmer Dabbelt <[email protected]>
> Suggested-by: Rémi Denis-Courmont <[email protected]>
> Signed-off-by: Björn Töpel <[email protected]>
> ---
> 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 <[email protected]> writes:
> On Tue, Jun 27, 2023 at 12:57 AM Björn Töpel <[email protected]> wrote:
>>
>> From: Björn Töpel <[email protected]>
>>
>> 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/[email protected]/ # [1]
>> Suggested-by: Darius Rad <[email protected]>
>> Suggested-by: Palmer Dabbelt <[email protected]>
>> Suggested-by: Rémi Denis-Courmont <[email protected]>
>> Signed-off-by: Björn Töpel <[email protected]>
>> ---
>> 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 <[email protected]> wrote:
>
> Andy Chiu <[email protected]> writes:
>
> > On Tue, Jun 27, 2023 at 12:57 AM Björn Töpel <[email protected]> wrote:
> >>
> >> From: Björn Töpel <[email protected]>
> >>
> >> 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/[email protected]/ # [1]
> >> Suggested-by: Darius Rad <[email protected]>
> >> Suggested-by: Palmer Dabbelt <[email protected]>
> >> Suggested-by: Rémi Denis-Courmont <[email protected]>
> >> Signed-off-by: Björn Töpel <[email protected]>
> >> ---
> >> 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 <[email protected]> writes:
> On Wed, Jun 28, 2023 at 6:35 PM Björn Töpel <[email protected]> wrote:
>>
>> Andy Chiu <[email protected]> writes:
>>
>> > On Tue, Jun 27, 2023 at 12:57 AM Björn Töpel <[email protected]> wrote:
>> >>
>> >> From: Björn Töpel <[email protected]>
>> >>
>> >> 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/[email protected]/ # [1]
>> >> Suggested-by: Darius Rad <[email protected]>
>> >> Suggested-by: Palmer Dabbelt <[email protected]>
>> >> Suggested-by: Rémi Denis-Courmont <[email protected]>
>> >> Signed-off-by: Björn Töpel <[email protected]>
>> >> ---
>> >> 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