Re: [PATCH -next v18 00/20] riscv: Add vector ISA support

["Björn Töpel" <bjorn@kernel.org>]

Björn Töpel <bjorn@kernel.org> writes:

> Andy Chiu <andy.chiu@sifive.com> writes:
>
>> This patchset is implemented based on vector 1.0 spec to add vector support
>> in riscv Linux kernel. There are some assumptions for this implementations.
>>
>> 1. We assume all harts has the same ISA in the system.
>> 2. We disable vector in both kernel and user space [1] by default. Only
>>    enable an user's vector after an illegal instruction trap where it
>>    actually starts executing vector (the first-use trap [2]).
>> 3. We detect "riscv,isa" to determine whether vector is support or not.
>>
>> We defined a new structure __riscv_v_ext_state in struct thread_struct to
>> save/restore the vector related registers. It is used for both kernel space
>> and user space.
>>  - In kernel space, the datap pointer in __riscv_v_ext_state will be
>>    allocated to save vector registers.
>>  - In user space,
>> 	- In signal handler of user space, the structure is placed
>> 	  right after __riscv_ctx_hdr, which is embedded in fp reserved
>> 	  aera. This is required to avoid ABI break [2]. And datap points
>> 	  to the end of __riscv_v_ext_state.
>> 	- In ptrace, the data will be put in ubuf in which we use
>> 	  riscv_vr_get()/riscv_vr_set() to get or set the
>> 	  __riscv_v_ext_state data structure from/to it, datap pointer
>> 	  would be zeroed and vector registers will be copied to the
>> 	  address right after the __riscv_v_ext_state structure in ubuf.
>>
>> This patchset is rebased to v6.3-rc1 and it is tested by running several
>> vector programs simultaneously. It delivers signals correctly in a test
>> where we can see a valid ucontext_t in a signal handler, and a correct V
>> context returing back from it. And the ptrace interface is tested by
>> PTRACE_{GET,SET}REGSET. Lastly, KVM is tested by running above tests in
>> a guest using the same kernel image. All tests are done on an rv64gcv
>> virt QEMU.
>>
>> Note: please apply the patch at [4] due to a regression introduced by
>> commit 596ff4a09b89 ("cpumask: re-introduce constant-sized cpumask
>> optimizations") before testing the series.
>>
>> Source tree:
>> https://github.com/sifive/riscv-linux/tree/riscv/for-next/vector-v18
>
> After some offlist discussions, we might have a identified a
> potential libc->application ABI break.
>
> Given an application that does custom task scheduling via a signal
> handler. The application binary is not vector aware, but libc is. Libc
> is using vector registers for memcpy. It's an "old application, new
> library, new kernel"-scenario.
>
>  | ...
>  | struct context *p1_ctx;
>  | struct context *p2_ctx;
>  | 
>  | void sighandler(int sig, siginfo_t *info, void *ucontext)
>  | {
>  |   if (p1_running)
>  |     switch_to(p1_ctx, p2_ctx);
>  |   if (p2_running)
>  |     switch_to(p2_ctx, p1_ctx);
>  | }
>  | 
>  | void p1(void)
>  | {
>  |   memcpy(foo, bar, 17);
>  | }
>  | 
>  | void p2(void)
>  | {
>  |   ...
>  | }
>  | ...
>
> The switch_to() function schedules p1() and p2(). E.g., the
> application (assumes that it) saves the complete task state from
> sigcontext (ucontext) to p1_ctx, and restores sigcontext to p2_ctx, so
> when sigreturn is called, p2() is running, and p1() has been
> interrupted.
>
> The "old application" which is not aware of vector, is now run on a
> vector enabled kernel/glibc.
>
> Assume that the sighandler is hit, and p1() is in the middle of the
> vector memcpy. The switch_to() function will not save the vector
> state, and next time p2() is scheduled to run it will have incorrect
> machine state.
>
> Now:
>
> Is this an actual or theoretical problem (i.e. are there any
> applications in the wild)? I'd be surprised if it would not be the
> latter...
>
> Regardless, a kernel knob for disabling vector (sysctl/prctl) to avoid
> these kind of breaks is needed (right?). Could this knob be a
> follow-up patch to the existing v18 series?
>
> Note that arm64 does not suffer from this with SVE, because the default
> vector length (vl==0/128b*32) fits in the "legacy" sigcontext.

Andy, to clarify from the patchwork call; In
Documentation/arm64/sve.rst:

There's a per-process prctl (section 6), and a system runtime conf
(section 9).


Björn