Re: Adding V-ext regs to signal context w/o expanding kernel struct sigcontext to avoid glibc ABI break

[Vineet Gupta <vineetg@rivosinc.com>]

Hi Vincent,

On 12/21/22 07:53, Vincent Chen wrote:
> Hi Vineet,
> Thank you for creating this discussion thread to get some consensus
> and propose a way to solve this problem. Actually, I don't object to
> your proposal. I just don't understand why my solution is
> inappropriate.

It is not inappropriate, in fact it is more natural to do it your way :-)
And if everything was rebuilt there was no issue. As some reviewers also 
pointed out the issue was with existing binaries with smaller sigcontext 
breaking with expanded sigcontext in kernel and/or glibc itself.


> IIUC, the struct sigcontext is used by the kernel to
> preserve the context of the register before entering the signal
> handler. Because the memory region used to save the register context
> is in user space, user space can obtain register context through the
> same struct sigcontext to parse the same memory region. Therefore, we
> don't want to break ABI to cause this mechanism to fail in the
> different kernel and Glibc combinations. Back to my approach, as you
> mentioned that my patch changes the size of struct sigcontext.
> However, this size difference does not seem to break the above
> mechanism. I enumerate the possible case below for discussion.
>
> 1. Kernel without RVV support + user program using original Glibc sigcontext.
> This is the current Glibc case. It has no problems.
>
> 2. Kernel with RVV support + user program using the new sigcontext definition
> The mechanism can work smoothly because the sigcontext definition in
> the kernel matches the definition in user programs.

Right but what about existing binaries. Imagine if they had

struct foo{
     struct sigcontext s;
     int bar;
}

Now with sigcontext expanded, bar is not at the expected location in memory.

> 3. Kernel without RVV support + user program using the new sigcontext definition
> Because the kernel does not store vector registers context to memory,
> the __reserved[4224] in GLIBC sigcontext is unneeded. Therefore, the
> struct sigcontext in user programs will waste a lot of memory due to
> __reserved[4224] if user programs allocate memory for it. But, the
> mechanism still can work smoothly.
>
> 4. Kernel with RVV support + user program using original Glibc sigcontext
> In this case, the kernel needs to save vector registers context to
> memory. The user program may encounter memory issues if the user space
> does not reserve enough memory size for the kernel to create the
> sigcontext. However, we can't seem to avoid this case since there is
> no flexible memory area in struct sigcontext for future expansion.
>
>  From the above enumeration, my approach in the 3rd case will be a
> problem. But, it may be solved by replacing the __reserved[4224] in
> struct sigcontext with the " C99 flexible length array". Therefore,
> the new patch will become below.
>
> --- a/sysdeps/unix/sysv/linux/riscv/bits/sigcontext.h
> +++ b/sysdeps/unix/sysv/linux/riscv/bits/sigcontext.h
>
> @@ -22,10 +22,28 @@
>   # error "Never use <bits/sigcontext.h> directly; include <signal.h> instead."
>   #endif
>
> +#define sigcontext kernel_sigcontext
> +#include <asm/sigcontext.h>
> +#undef sigcontext
>
>   struct sigcontext {
>   /* gregs[0] holds the program counter. */
> - unsigned long int gregs[32];
> - unsigned long long int fpregs[66] __attribute__ ((__aligned__ (16)));
> + __extension__ union {
> + unsigned long int gregs[32];
> + /* Kernel uses struct user_regs_struct to save x1-x31 and pc
> + to the signal context, so please use sc_regs to access these
> + these registers from the signal context. */
> + struct user_regs_struct sc_regs;
> + };
>
> + __extension__ union {
> + unsigned long long int fpregs[66] __attribute__ ((__aligned__ (16)));
> + /* Kernel uses struct __riscv_fp_state to save f0-f31 and fcsr
> + to the signal context, so please use sc_fpregs to access these
> + fpu registers from the signal context. */
> + union __riscv_fp_state sc_fpregs;
> + };
> +
> + __u8 sc_extn[] __attribute__((__aligned__(16)));
>   };
>
>   #endif
>
>
> This change can reduce memory waste size to 16 bytes in the worst
> case. The best case happens when the sc_extn locates at a 16-byte
> aligned address. The size of the struct sigcontext is still the same.

Its a neat trick. But the additional stack alignment means we could 
still potentially changing the size of sigcontext - even if by 16 bytes 
- again for existing binaries.

I agree that struct sigcontext is not something people commonly use in 
their code. And also not sure if the concern of breaking existing 
binaries with struct sigcontext is a real problem or a theoretical 
exercise. Hence I wanted some of the maintainers to weigh-in. I don't 
have issues with your approach, just that in the prior 2 reviews it 
seemed it was a no go.


> If the above inference is acceptable, I want to mention some
> advantages of my patch. This approach allows user programs to directly
> access the vector register context.

Correct, that is very true.

> Besides, new user programs can use
> kernel-defined struct sigcontext to access the context of the
> register. Actually, the memory layout of the FPU register in
> kernel-defined struct sigcontext is different from the Glibc-defined
> struct sigcontext. It probably causes the user programs to get the
> wrong value of FPU registers from the context. Therefore, my approach
> can help user programs get the correct FPU registers because the user
> program is able to use kernel-defined struct sigcontext to access the
> FPU register context. It will help RISC-V users get rid of the
> historical burden in Glibc sigcontext.h.

Indeed.

Thx,
-Vineet



>
>
> Thanks,
> Vincent Chen
>
> On Wed, Dec 21, 2022 at 4:05 AM Vineet Gupta <vineetg@rivosinc.com> wrote:
>> Hi folks,
>>
>> Apologies for the extraneous CC (and the top post), but I would really
>> appreciate some feedback on this to close on the V-ext plumbing support
>> in kernel/glibc. This is one of the two contentious issues (other being
>> prctl enable) preventing us from getting to an RVV enabled SW ecosystem.
>>
>> The premise is : for preserving V-ext registers across signal handling,
>> the natural way is to add V reg storage to kernel struct sigcontext
>> where scalar / fp regs are currently saved. But this doesn’t seem to be
>> the right way to go:
>>
>> 1. Breaks the userspace ABI (even if user programs were recompiled)
>> because RV glibc port for historical reasons has defined its own version
>> of struct sigcontext (vs. relying on kernel exported UAPI header).
>>
>> 2. Even if we were to expand sigcontext (in both kernel and glibc, which
>> is always hard to time) there's still a (different) ABI breakage for
>> existing binaries despite earlier proposed __extension__ union trick [2]
>> since it still breaks old binaries w.r.t. size of the sigcontext struct.
>>
>> 3. glibc {set,get,*}context() routines use struct mcontext_t which is
>> analogous to kernel struct sigcontext (in respective ucontext structs
>> [1]). Thus ideally mcontext_t needs to be expanded too but need not be,
>> given its semantics to save callee-saved regs only : per current psABI
>> RVVV regs are caller-saved/call-clobbered [3]. Apparently this
>> connection of sigcontext to mcontext_t is also historical as some arches
>> used/still-use sigreturn to restore regs in setcontext [4]
>>
>> Does anyone disagree that 1-3 are not valid reasons.
>>
>> So the proposal here is to *not* add V-ext state to kernel sigcontext
>> but instead dynamically to struct rt_sigframe, similar to aarch64
>> kernel. This avoids touching glibc sigcontext as well.
>>
>> struct rt_sigframe {
>>     struct siginfo info;
>>     struct ucontext uc;
>> +__u8 sc_extn[] __attribute__((__aligned__(16))); // C99 flexible length
>> array to handle implementation defined VLEN wide regs
>> }
>>
>> The only downside to this is that SA_SIGINFO signal handlers don’t have
>> direct access to V state (but it seems aarch64 kernel doesn’t either).
>>
>> Does anyone really disagree with this proposal.
>>
>> Attached is a proof-of-concept kernel patch which implements this
>> proposal with no need for any corresponding glibc change.
>>
>> Thx,
>> -Vineet
>>
>>
>> [1] ucontex in kernel and glibc respectively.
>>
>> kernel: arch/riscv/include/uapi/asm/ucontext.h
>>
>> struct ucontext {
>>    unsigned long uc_flags;
>>    struct ucontext *uc_link;
>>    stack_t uc_stack;
>>    sigset_t uc_sigmask;
>>    __u8 __unused[1024 / 8 - sizeof(sigset_t)];
>>    struct sigcontext uc_mcontext;
>> }
>>
>> glibc: sysdeps/unix/sysv/linux/riscv/sys/ucontext.h
>>
>> typedef struct ucontext_t
>>     {
>>       unsigned long int  __uc_flags;
>>       struct ucontext_t *uc_link;
>>       stack_t            uc_stack;
>>       sigset_t           uc_sigmask;
>>       /* padding to allow future sigset_t expansion */
>>       char   __glibc_reserved[1024 / 8 - sizeof (sigset_t)];
>>        mcontext_t uc_mcontext;
>> } ucontext_t;
>>
>> [2] https://sourceware.org/pipermail/libc-alpha/2022-January/135610.html
>> [3]
>> https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-cc.adoc
>> [4] https://sourceware.org/legacy-ml/libc-alpha/2014-04/msg00006.html
>>
>>
>>
>>
>> On 12/8/22 19:39, Vineet Gupta wrote:
>>> Hi Florian,
>>>
>>> P.S. Since I'm revisiting a year old thread with some new CC
>>> recipients, here's the link to original patch/thread [1]
>>>
>>> On 9/17/21 20:04, Vincent Chen wrote:
>>>> On Thu, Sep 16, 2021 at 4:14 PM Florian Weimer <fweimer@redhat.com>
>>>> wrote:
>>>>>>>> This changes the size of struct ucontext_t, which is an ABI break
>>>>>>>> (getcontext callers are supposed to provide their own object).
>>>>>>>>
>>>>>> The riscv vector registers are all caller-saved registers except for
>>>>>> VCSR. Therefore, the struct mcontext_t needs to reserve a space for
>>>>>> it. In addition, RISCV ISA is growing, so I also hope the struct
>>>>>> mcontext_t has a space for future expansion. Based on the above ideas,
>>>>>> I reserved a 5K space here.
>>>>> You have reserved space in ucontext_t that you could use for this.
>>>>>
>>>> Sorry, I cannot really understand what you mean. The following is the
>>>> contents of ucontext_t
>>>> typedef struct ucontext_t
>>>>     {
>>>>       unsigned long int  __uc_flags;
>>>>       struct ucontext_t *uc_link;
>>>>       stack_t            uc_stack;
>>>>       sigset_t           uc_sigmask;
>>>>       /* 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. */
>>>>       char               __glibc_reserved[1024 / 8 - sizeof (sigset_t)];
>>>>       /* 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.  */
>>>>       mcontext_t uc_mcontext;
>>>>     } ucontext_t;
>>>>
>>>> Currently, we only reserve a space, __glibc_reserved[], for the future
>>>> expansion of sigset_t.
>>>> Do you mean I could use __glibc_reserved[] to for future expansion of
>>>> ISA as well?
>>> Given unlikely sigset expansion, we could in theory use some of those
>>> reserved fields to store pointers (offsets) to actual V state, but not
>>> for actual V state which is way too large for non-embedded machines
>>> with typical 128 or even wider V regs.
>>>
>>>
>>>>>>>> This shouldn't be necessary if the additional vector registers are
>>>>>>>> caller-saved.
>>>>>> Here I am a little confused about the usage of struct mcontext_t. As
>>>>>> far as I know, the struct mcontext_t is used to save the
>>>>>> machine-specific information in user context operation. Therefore, in
>>>>>> this case, the struct mcontext_t is allowed to reserve the space only
>>>>>> for saving caller-saved registers. However, in the signal handler, the
>>>>>> user seems to be allowed to use uc_mcontext whose data type is struct
>>>>>> mcontext_t to access the content of the signal context. In this case,
>>>>>> the struct mcontext_t may need to be the same as the struct sigcontext
>>>>>> defined at kernel. However, it will have a conflict with your
>>>>>> suggestion because the struct sigcontext cannot just reserve a space
>>>>>> for saving caller-saved registers. Could you help me point out my
>>>>>> misunderstanding? Thank you.
>>> I think the confusion comes from apparent equivalence of kernel struct
>>> sigcontext and glibc mcontext_t as they appear in respective struct
>>> ucontext definitions.
>>> I've enumerated the actual RV structs below to keep them handy in one
>>> place for discussion.
>>>
>>>>> struct sigcontext is allocated by the kernel, so you can have pointers
>>>>> in reserved fields to out-of-line start, or after struct sigcontext.
>>> In this scheme, would the actual V regfile contents (at the
>>> out-of-line location w.r.t kernel sigcontext) be anonymous for glibc
>>> i.e. do we not need to expose them to glibc userspace ABI ?
>>>
>>>
>>>>> I don't know how the kernel implements this, but there is considerable
>>>>> flexibility and extensibility.  The main issues comes from small stacks
>>>>> which are incompatible with large register files.
>>> Simplistically, Linux kernel needs to preserve the V regfile across
>>> task switch. The necessary evil that follows is preserving V across
>>> signal-handling (sigaction/sigreturn).
>>>
>>> In RV kernel we have following:
>>>
>>> struct rt_sigframe {
>>>    struct siginfo info;
>>>    struct ucontext uc;
>>> };
>>>
>>> struct ucontext {
>>>     unsigned long uc_flags;
>>>     struct ucontext *uc_link;
>>>     stack_t uc_stack;
>>>     sigset_t uc_sigmask;
>>>     __u8 __unused[1024 / 8 - sizeof(sigset_t)];     // this is for
>>> sigset_t expansion
>>>     struct sigcontext uc_mcontext;
>>> };
>>>
>>> struct sigcontext {
>>>     struct user_regs_struct sc_regs;
>>>     union __riscv_fp_state sc_fpregs;
>>> +  __u8 sc_extn[4096+128] __attribute__((__aligned__(16)));   //
>>> handle 128B V regs
>>> };
>>>
>>> The sc_extn[] would have V state (regfile + control state) in kernel
>>> defined format.
>>>
>>> As I understand it, you are suggesting to prevent ABI break, we should
>>> not add anything to kernel struct sigcontext i.e. do something like this
>>>
>>> struct rt_sigframe {
>>>    struct siginfo info;
>>>    struct ucontext uc;
>>> +__u8 sc_extn[4096+128] __attribute__((__aligned__(16)));
>>> }
>>>
>>> So kernel sig handling can continue to save/restore the V regfile on
>>> user stack, w/o making it part of actual struct sigcontext.
>>> So they are not explicitly visible to userspace at all - is that
>>> feasible ? I know that SA_SIGINFO handlers can access the scalar/fp
>>> regs, they won't do it V.
>>> Is there a POSIX req for SA_SIGINFO handlers being able to access all
>>> machine regs saved by signal handling.
>>>
>>> An alternate approach is what Vincent did originally, to add sc_exn to
>>> struct sigcontext. Here to prevent ABI breakage, we can choose to not
>>> reflect this in the glibc sigcontext. But the question remains, is
>>> that OK ?
>>>
>>> The other topic is changing glibc mcontext_t to add V-regs. It would
>>> seem one has to as mcontext is "visually equivalent" to struct
>>> sigcontext in the respective ucontext structs. But in unserspace
>>> *context routine semantics only require callee-regs to be saved, which
>>> V regs are not per psABI [2]. So looks like this can be avoided which
>>> is what Vincent did in v2 series [3]
>>>
>>>
>>> [1]
>>> https://sourceware.org/pipermail/libc-alpha/2021-September/130899.html
>>> [2]
>>> https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-cc.adoc
>>> [3] https://sourceware.org/pipermail/libc-alpha/2022-January/135416.html