From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-vs1-xe33.google.com (mail-vs1-xe33.google.com [IPv6:2607:f8b0:4864:20::e33]) by sourceware.org (Postfix) with ESMTPS id 387DB3858D1E for ; Thu, 22 Dec 2022 01:51:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 387DB3858D1E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=sifive.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=sifive.com Received: by mail-vs1-xe33.google.com with SMTP id 3so461097vsq.7 for ; Wed, 21 Dec 2022 17:51:05 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sifive.com; s=google; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=2zCdCRDHA1sFva993ZrIStOiwPcUhBsPFFRbe2pIV1E=; b=IbeuHzPHtcP29AIWU48PV+jS8NF4mVTghA5cOMSAT1aDSmVTw6xruf4hyoh7p4R3wx o0eO6KJzn0BCByQg400AksI97NVNiKbuCUpnhMl5zoIlKvBl+PHF2I1MiezcvzRIn6zb 44RwPqiw8retcbe/jH0VQMgke20O2TwrLBAk9ASpTYQCqWD8KcnPQ+IopgQh+HZuu+Pz hl7Jn9FjZ73JlAGC/HoY6wvVeHpPz1nVTeA55b1O8aFQUU08qipTkVLkHQCq8u++bLDJ 9uJ+eX44LJWKbk8qugQEpFkXn70pOwX8D3yAx/45Z5O49vqO93WBg0sFrnXQUzwnkW84 4ERA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=2zCdCRDHA1sFva993ZrIStOiwPcUhBsPFFRbe2pIV1E=; b=qirv/DKTk4QiV74JdQ1E+PZXWNppvKsNmg85kwA9avtFjKm64yl58I27j153QFdqXT rwpLDH9WZdvJGIYQ5MYzO0NbsFnX2gVEW2Jui/L8rB7GyX1Gprr4tpnHk9M+a2uwA9uO 1llnYFF+ekrB983yiwwJry/a2B9bGmbQOSM7YMtkhRZvpW8MfP4u01OKOl8uLLw+oDyX DbXKNsH+911s4vRXRjFkaBnQx32SzOxE93bf9/1dDAvob+I9SF7x/NHfKG0KLgR1e93D yF3oMUgpHOfXqtBjjM450sqsrTqg8KGSIzgB2rHaYU2yHp31nWc2ZKxuJfk4UWa1qjgv k5Rw== X-Gm-Message-State: AFqh2kr7cMFbvo+LgzO3vlDi08SvEN9mppfiJueevSNl5cDKffSun9AI Kj6WrDE5OqASGmgibRAU887AJ0B3cwfjPtFN4TWn8w== X-Google-Smtp-Source: AMrXdXvqTtDjQvfiPLrNitf4pTfo5iVwo4U74O7DU/hmi2jex0fnspeHI3hkkSJD73M9YWuXJS8+DQ9py/Koe7t2Ugk= X-Received: by 2002:a05:6102:510e:b0:3b1:2b83:1861 with SMTP id bm14-20020a056102510e00b003b12b831861mr477443vsb.10.1671673864120; Wed, 21 Dec 2022 17:51:04 -0800 (PST) MIME-Version: 1.0 References: <1631497278-29829-1-git-send-email-vincent.chen@sifive.com> <1631497278-29829-3-git-send-email-vincent.chen@sifive.com> <871r5sd1zq.fsf@oldenburg.str.redhat.com> <20210913135247.GL13220@brightrain.aerifal.cx> <87sfy5ndid.fsf@oldenburg.str.redhat.com> <73c0124c-4794-6e40-460c-b26df407f322@rivosinc.com> <50c598a6-e3b3-3062-abe7-23a406067533@rivosinc.com> In-Reply-To: <50c598a6-e3b3-3062-abe7-23a406067533@rivosinc.com> From: Vincent Chen Date: Thu, 22 Dec 2022 09:50:53 +0800 Message-ID: Subject: Re: Adding V-ext regs to signal context w/o expanding kernel struct sigcontext to avoid glibc ABI break To: Vineet Gupta Cc: Florian Weimer , Rich Felker , Andrew Waterman , Palmer Dabbelt , Kito Cheng , =?UTF-8?Q?Christoph_M=C3=BCllner?= , davidlt@rivosinc.com, Arnd Bergmann , =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= , Philipp Tomsich , Szabolcs Nagy , Andy Chiu , Greentime Hu , Aaron Durbin , Andrew de los Reyes , linux-riscv , GNU C Library Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-3.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Thu, Dec 22, 2022 at 3:45 AM Vineet Gupta 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 sigco= ntext. > > This is the current Glibc case. It has no problems. > > > > 2. Kernel with RVV support + user program using the new sigcontext defi= nition > > 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 memo= ry. I really miss considering this case. I guess the following example is one of the cases you want to mention. 1. a.out #include ... struct foo{ struct sigcontext s; int bar; } sc; int main (void) { lala(&sc); // it defined in lala.so } 2. lala.so #include 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 d= efinition > > 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 sigconte= xt > > 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 directly; include i= nstead." > > #endif > > > > +#define sigcontext kernel_sigcontext > > +#include > > +#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 wro= te: > >> 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 suppor= t > >> in kernel/glibc. This is one of the two contentious issues (other bein= g > >> prctl enable) preventing us from getting to an RVV enabled SW ecosyste= m. > >> > >> 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=E2=80=99t s= eem 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 versi= on > >> of struct sigcontext (vs. relying on kernel exported UAPI header). > >> > >> 2. Even if we were to expand sigcontext (in both kernel and glibc, whi= ch > >> 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 struc= t. > >> > >> 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 arch= es > >> 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 leng= th > >> array to handle implementation defined VLEN wide regs > >> } > >> > >> The only downside to this is that SA_SIGINFO signal handlers don=E2=80= =99t have > >> direct access to V state (but it seems aarch64 kernel doesn=E2=80=99t = 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.ht= ml > >> [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 > >>>> wrote: > >>>>>>>> This changes the size of struct ucontext_t, which is an ABI brea= k > >>>>>>>> (getcontext callers are supposed to provide their own object). > >>>>>>>> > >>>>>> The riscv vector registers are all caller-saved registers except f= or > >>>>>> VCSR. Therefore, the struct mcontext_t needs to reserve a space fo= r > >>>>>> 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 id= eas, > >>>>>> 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 th= e > >>>> 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 becaus= e > >>>> 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 allo= w for > >>>> infinite extensibility. Since we know this will be extende= d > >>>> 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 futu= re > >>>> expansion of sigset_t. > >>>> Do you mean I could use __glibc_reserved[] to for future expansion o= f > >>>> 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 no= t > >>> 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 a= re > >>>>>>>> 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 o= nly > >>>>>> for saving caller-saved registers. However, in the signal handler,= the > >>>>>> user seems to be allowed to use uc_mcontext whose data type is str= uct > >>>>>> mcontext_t to access the content of the signal context. In this ca= se, > >>>>>> the struct mcontext_t may need to be the same as the struct sigcon= text > >>>>>> defined at kernel. However, it will have a conflict with your > >>>>>> suggestion because the struct sigcontext cannot just reserve a spa= ce > >>>>>> 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 struc= t > >>> 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 point= ers > >>>>> 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 considera= ble > >>>>> flexibility and extensibility. The main issues comes from small st= acks > >>>>> 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 shoul= d > >>> not add anything to kernel struct sigcontext i.e. do something like t= his > >>> > >>> 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 t= o > >>> 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, whic= h > >>> 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.htm= l > >>> [2] > >>> https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/risc= v-cc.adoc > >>> [3] https://sourceware.org/pipermail/libc-alpha/2022-January/135416.h= tml >