Adding V-ext regs to signal context w/o expanding kernel struct sigcontext to avoid glibc ABI break
Vincent Chen
vincent.chen at sifive.com
Wed Dec 21 17:50:53 PST 2022
On Thu, Dec 22, 2022 at 3:45 AM Vineet Gupta <vineetg at rivosinc.com> wrote:
>
> 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.
Thank you for your detailed explanations :-) I still have some
questions and hope you can help me clarify them.
>
>
> > 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.
I really miss considering this case. I guess the following example is
one of the cases you want to mention.
1. a.out
#include <bits/sigcontext.h>
...
struct foo{
struct sigcontext s;
int bar;
} sc;
int main (void) {
lala(&sc); // it defined in lala.so
}
2. lala.so
#include <bits/sigcontext.h>
struct foo{
struct sigcontext s;
int bar;
} sc;
void lala(struct foo *ptr) {
}
If the lala.so and a.out are compiled with different sizes of the
struct sigcontext, it will have an issue apparently. But, as you
mentioned, I am also curious if this example is a real problem or just
a theoretical exercise.
>
> > 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.
I agree with you that we need more maintainers to weigh-in to find an
appropriate solution. In my opinion, if the prior example is not
extensively used, maybe it is a good time to get rid of the historical
burden.
Thanks,
Vincent
>
>
> > 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 at 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 at 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
>
More information about the linux-riscv
mailing list