Re: [PATCH v8 17/18] RISC-V: User-facing API

[Palmer Dabbelt]

On Wed, 13 Sep 2017 08:56:54 PDT (-0700), Arnd Bergmann wrote:
> On Tue, Sep 12, 2017 at 11:57 PM, Palmer Dabbelt <palmer@xxxxxxxxxxx> wrote:
>> This patch contains code that is in some way visible to the user:
>> including via system calls, the VDSO, module loading and signal
>> handling.  It also contains some generic code that is ABI visible.
>
> I took a good look at this and found nothing that is really wrong, but
> I noticed two smaller issues that I'd like to bring up for discussion:
>
>> diff --git a/arch/riscv/include/uapi/asm/ucontext.h b/arch/riscv/include/uapi/asm/ucontext.h
>> new file mode 100644
>> index 000000000000..52eff9febcfd
>> --- /dev/null
>> +++ b/arch/riscv/include/uapi/asm/ucontext.h
>> @@ -0,0 +1,35 @@
>> + * This file was copied from arch/arm64/include/uapi/asm/ucontext.h
>> + */
>> +#ifndef _UAPI__ASM_UCONTEXT_H
>> +#define _UAPI__ASM_UCONTEXT_H
>> +
>> +#include <linux/types.h>
>> +
>> +struct ucontext {
>> +       unsigned long     uc_flags;
>> +       struct ucontext  *uc_link;
>> +       stack_t           uc_stack;
>> +       sigset_t          uc_sigmask;
>> +       /* glibc uses a 1024-bit sigset_t */
>> +       __u8              __unused[1024 / 8 - sizeof(sigset_t)];
>> +       /* last for future expansion */
>> +       struct sigcontext uc_mcontext;
>> +};
>
> This seems odd, the arm64 file was added with this comment
>
> commit 33b36543df336d9158e1a763fe97251885f52c5c
> Author: Will Deacon <will.deacon@xxxxxxx>
> Date:   Fri Jan 16 13:52:14 2015 +0000
>
>     arm64: uapi: expose our struct ucontext to the uapi headers
>
>     arm64 defines its own ucontext structure which is incompatible with the
>     struct defined (and exposed to userspace by) the asm-generic headers.
>
>     glibc carries its own struct definition that is compatible with the
>     arm64 definition, but we should expose our format in the uapi headers in
>     case other libraries want to make use of the ucontext pushed as part of
>     an arm64 sigframe.
>
>     This patch moves the arm64 asm/ucontext.h to the uapi headers, along
>     with the necessary #include of linux/types.h.
>
> which doesn't really explain _why_ they are different from asm-generic.
>
> Can you explain this? Does the ARM64 layout have a significant
> advantage over the asm-generic one, or is it just what you happened
> to use because you copied from ARM64?

I copied those comments from ARM64, and they're pretty useless.  How does this
look?

diff --git a/arch/riscv/include/uapi/asm/ucontext.h b/arch/riscv/include/uapi/asm/ucontext.h
index 52eff9febcfd..1fae8b1697e0 100644
--- a/arch/riscv/include/uapi/asm/ucontext.h
+++ b/arch/riscv/include/uapi/asm/ucontext.h
@@ -26,9 +26,19 @@ struct ucontext {
        struct ucontext  *uc_link;
        stack_t           uc_stack;
        sigset_t          uc_sigmask;
-       /* glibc uses a 1024-bit sigset_t */
+       /* There's some padding here to allow sigset_t to be expanded in the
+        * future.  Though this is unlikely, other architectures put uc_sigmask
+        * at the end of this structure and explicitly state it can be
+        * expanded, so we didn't want to box ourselves in here. */
        __u8              __unused[1024 / 8 - sizeof(sigset_t)];
-       /* last for future expansion */
+       /* We can't put uc_sigmask at the end of this structure because we need
+        * to be able to expand sigcontext in the future.  For example, the
+        * vector ISA extension will almost certainly add ISA state.  We want
+        * to ensure all user-visible ISA state can be saved and restored via a
+        * ucontext, so we're putting this at the end in order to allow for
+        * infinite extensibility.  Since we know this will be extended and we
+        * assume sigset_t won't be extended an extreme amount, we're
+        * prioritizing this. */
        struct sigcontext uc_mcontext;
 };

> If that layout is indeed better, maybe we should change asm-generic
> to use that, and fall back to the old layout for the architectures that
> already use it.

That cropped up during glibc as well, and I think it might be the right answer.
Should I submit another patch that fixes up the other ISAs?

> If one is as good as the other, could you change kernel and glibc
> to use the normal one instead?
>
>> + */
>> +VERSION
>> +{
>> +       LINUX_2.6 {
>> +       global:
>> +               __vdso_rt_sigreturn;
>> +               __vdso_cmpxchg32;
>> +               __vdso_cmpxchg64;
>> +       local: *;
>> +       };
>
> The last vdso that got added was for arm64, and it was still
> during linux-2.6 times.
>
> Should this instead use the version that you are targetting for
> the merge, i.e. 4.15?

That sounds right to me

diff --git a/arch/riscv/kernel/vdso/vdso.lds.S b/arch/riscv/kernel/vdso/vdso.lds.S
index 7142e1aafc30..8c9dce95c11d 100644
--- a/arch/riscv/kernel/vdso/vdso.lds.S
+++ b/arch/riscv/kernel/vdso/vdso.lds.S
@@ -67,7 +67,7 @@ PHDRS
  */
 VERSION
 {
-       LINUX_2.6 {
+       LINUX_4.15 {
        global:
                __vdso_rt_sigreturn;
                __vdso_cmpxchg32;