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

[Vineet Gupta <vineetg@rivosinc.com>]

[-- Attachment #1: Type: text/plain, Size: 10252 bytes --]

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

[-- Attachment #2: 0001-riscv-Add-sigcontext-save-restore-for-vector.patch --]
[-- Type: text/x-patch, Size: 10635 bytes --]

From 169eea1ef072c8403277a66313b00258080ac92c Mon Sep 17 00:00:00 2001
From: Vineet Gupta <vineetg@rivosinc.com>
Date: Wed, 21 Sep 2022 14:43:52 -0700
Subject: [PATCH] riscv: Add sigcontext save/restore for vector

V state needs to be preserved across signal handling on user stack.
To avoid glibc ABI break, this is not added to struct sigcontext (just as
for int/fp regs) but to struct rt_sigframe. Also this is all done
dynamically (vs. some static allocation) to cleanly handle implementation
defined VLEN wide V-regs.

We also borrow arm64 style of "context header" to tag the extension
state to allow for easy integration of future extensions.

Co-developed-by: Vincent Chen <vincent.chen@sifive.com>
Co-developed-by: Greentime Hu <greentime.hu@sifive.com>
Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
[vineetg: reworked to not change struct sigcontext,
          wireup init_rt_signal_env]
---
 arch/riscv/include/asm/processor.h       |   1 +
 arch/riscv/include/uapi/asm/sigcontext.h |  18 +++
 arch/riscv/kernel/asm-offsets.c          |   2 +
 arch/riscv/kernel/setup.c                |   2 +
 arch/riscv/kernel/signal.c               | 171 +++++++++++++++++++++--
 5 files changed, 186 insertions(+), 8 deletions(-)

diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
index 95917a2b24f9..854854b377b2 100644
--- a/arch/riscv/include/asm/processor.h
+++ b/arch/riscv/include/asm/processor.h
@@ -85,6 +85,7 @@ int riscv_of_parent_hartid(struct device_node *node, unsigned long *hartid);
 
 extern void riscv_fill_hwcap(void);
 extern int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src);
+void init_rt_signal_env(void);
 
 #endif /* __ASSEMBLY__ */
 
diff --git a/arch/riscv/include/uapi/asm/sigcontext.h b/arch/riscv/include/uapi/asm/sigcontext.h
index 84f2dfcfdbce..411bf6985784 100644
--- a/arch/riscv/include/uapi/asm/sigcontext.h
+++ b/arch/riscv/include/uapi/asm/sigcontext.h
@@ -8,6 +8,24 @@
 
 #include <asm/ptrace.h>
 
+/* The Magic number for signal context frame header. */
+#define RVV_MAGIC	0x53465457
+#define END_MAGIC	0x0
+
+/* The size of END signal context header. */
+#define END_HDR_SIZE	0x0
+
+/* Every optional extension state needs to have the hdr. */
+struct __riscv_ctx_hdr {
+	__u32 magic;
+	__u32 size;
+};
+
+struct __sc_riscv_v_state {
+	struct __riscv_ctx_hdr head;
+	struct __riscv_v_state v_state;
+} __attribute__((aligned(16)));
+
 /*
  * Signal context structure
  *
diff --git a/arch/riscv/kernel/asm-offsets.c b/arch/riscv/kernel/asm-offsets.c
index 37e3e6a8d877..80316ef7bb78 100644
--- a/arch/riscv/kernel/asm-offsets.c
+++ b/arch/riscv/kernel/asm-offsets.c
@@ -75,6 +75,8 @@ void asm_offsets(void)
 	OFFSET(TSK_STACK_CANARY, task_struct, stack_canary);
 #endif
 
+	OFFSET(RISCV_V_STATE_MAGIC, __riscv_ctx_hdr, magic);
+	OFFSET(RISCV_V_STATE_SIZE, __riscv_ctx_hdr, size);
 	OFFSET(RISCV_V_STATE_VSTART, __riscv_v_state, vstart);
 	OFFSET(RISCV_V_STATE_VL, __riscv_v_state, vl);
 	OFFSET(RISCV_V_STATE_VTYPE, __riscv_v_state, vtype);
diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
index 2dfc463b86bb..aa0eedd3b890 100644
--- a/arch/riscv/kernel/setup.c
+++ b/arch/riscv/kernel/setup.c
@@ -299,6 +299,8 @@ void __init setup_arch(char **cmdline_p)
 	riscv_init_cbom_blocksize();
 	riscv_fill_hwcap();
 	apply_boot_alternatives();
+	/* needs to be after riscv_fill_hwcap */
+	init_rt_signal_env();
 }
 
 static int __init topology_init(void)
diff --git a/arch/riscv/kernel/signal.c b/arch/riscv/kernel/signal.c
index 5c591123c440..ee234c319e5b 100644
--- a/arch/riscv/kernel/signal.c
+++ b/arch/riscv/kernel/signal.c
@@ -21,15 +21,27 @@
 #include <asm/csr.h>
 
 extern u32 __user_rt_sigreturn[2];
+static size_t rvv_sc_size;
 
 #define DEBUG_SIG 0
 
 struct rt_sigframe {
 	struct siginfo info;
-	struct ucontext uc;
 #ifndef CONFIG_MMU
 	u32 sigreturn_code[2];
 #endif
+	struct ucontext uc;
+	/*
+	 * Placeholder for additional state for V ext (and others in future).
+	 *  - Not added to struct sigcontext (unlike int/fp regs) to remain
+	 *    compatible with existing glibc struct sigcontext
+	 *  - Not added here explicitly either to allow for
+	 *     - Implementation defined VLEN wide V reg
+	 *     - Ability to do this per process
+	 * The actual V state struct is defined in uapi header.
+	 * Note: The alignment of 16 is ABI mandated for stack entries.
+	 */
+	__u8 sc_extn[] __attribute__((__aligned__(16)));
 };
 
 #ifdef CONFIG_FPU
@@ -86,16 +98,142 @@ static long save_fp_state(struct pt_regs *regs,
 #define restore_fp_state(task, regs) (0)
 #endif
 
-static long restore_sigcontext(struct pt_regs *regs,
-	struct sigcontext __user *sc)
+#ifdef CONFIG_RISCV_ISA_V
+
+static long save_v_state(struct pt_regs *regs, void **sc_vec)
+{
+	/*
+	 * Put __sc_riscv_v_state to the user's signal context space pointed
+	 * by sc_vec and the datap point the address right
+	 * after __sc_riscv_v_state.
+	 */
+	struct __sc_riscv_v_state __user *state = (struct __sc_riscv_v_state *) (*sc_vec);
+	void __user *datap = state + 1;
+	long err;
+
+	err = __put_user(RVV_MAGIC, &state->head.magic);
+	err = __put_user(rvv_sc_size, &state->head.size);
+
+	vstate_save(current, regs);
+	/* Copy additional vstate (except V regfile). */
+	err = __copy_to_user(&state->v_state, &current->thread.vstate,
+			     RISCV_V_STATE_DATAP);
+	if (unlikely(err))
+		return err;
+
+	/* Copy the pointer datap itself. */
+	err = __put_user(datap, &state->v_state.datap);
+	if (unlikely(err))
+		return err;
+
+	/* Copy the V regfile to user space datap. */
+	err = __copy_to_user(datap, current->thread.vstate.datap, riscv_vsize);
+
+	*sc_vec += rvv_sc_size;
+
+	return err;
+}
+
+static long restore_v_state(struct pt_regs *regs, void **sc_vec)
+{
+	long err;
+	struct __sc_riscv_v_state __user *state = (struct __sc_riscv_v_state *)(*sc_vec);
+	void __user *datap;
+
+	/* ctx_hdr check for RVV_MAGIC already done in caller. */
+
+	/* Copy everything of __sc_riscv_v_state except datap. */
+	err = __copy_from_user(&current->thread.vstate, &state->v_state,
+			       RISCV_V_STATE_DATAP);
+	if (unlikely(err))
+		return err;
+
+	/* Copy the pointer datap itself. */
+	err = __get_user(datap, &state->v_state.datap);
+	if (unlikely(err))
+		return err;
+
+	/* Copy the whole vector content from user space datap. */
+	err = __copy_from_user(current->thread.vstate.datap, datap, riscv_vsize);
+	if (unlikely(err))
+		return err;
+
+	vstate_restore(current, regs);
+
+	*sc_vec += rvv_sc_size;
+
+	return err;
+}
+
+#else
+#define save_v_state(task, regs) (0)
+#define restore_v_state(task, regs) (0)
+#endif
+
+static long restore_sigcontext(struct rt_sigframe __user *frame,
+			       struct pt_regs *regs)
 {
+	struct sigcontext __user *sc = &frame->uc.uc_mcontext;
+	void *sc_extn = &frame->sc_extn;
 	long err;
+
 	/* sc_regs is structured the same as the start of pt_regs */
 	err = __copy_from_user(regs, &sc->sc_regs, sizeof(sc->sc_regs));
 	/* Restore the floating-point state. */
 	if (has_fpu())
 		err |= restore_fp_state(regs, &sc->sc_fpregs);
+
+	while (1 && !err) {
+		struct __riscv_ctx_hdr *head = (struct __riscv_ctx_hdr *)sc_extn;
+		__u32 magic, size;
+
+		err |= __get_user(magic, &head->magic);
+		err |= __get_user(size, &head->size);
+		if (err)
+			goto done;
+
+		switch (magic) {
+		case END_MAGIC:
+			if (size != END_HDR_SIZE)
+				goto invalid;
+			goto done;
+		case RVV_MAGIC:
+			if (!has_vector() || (size != rvv_sc_size))
+				goto invalid;
+			err |= restore_v_state(regs, &sc_extn);
+			break;
+		default:
+			goto invalid;
+		}
+	}
+done:
 	return err;
+
+invalid:
+	return -EINVAL;
+}
+
+static size_t cal_rt_frame_size(void)
+{
+	struct rt_sigframe __user *frame;
+	static size_t frame_size;
+	size_t total_context_size = 0;
+
+	if (frame_size)
+		goto done;
+
+	total_context_size = sizeof(*frame);
+
+	if (has_vector())
+		total_context_size += rvv_sc_size;
+
+	/* Add a __riscv_ctx_hdr for END signal context header. */
+	total_context_size += sizeof(struct __riscv_ctx_hdr);
+
+	frame_size = round_up(total_context_size, 16);
+done:
+	return frame_size;
+
 }
 
 SYSCALL_DEFINE0(rt_sigreturn)
@@ -104,13 +242,14 @@ SYSCALL_DEFINE0(rt_sigreturn)
 	struct rt_sigframe __user *frame;
 	struct task_struct *task;
 	sigset_t set;
+	size_t frame_size = cal_rt_frame_size();
 
 	/* Always make any pending restarted system calls return -EINTR */
 	current->restart_block.fn = do_no_restart_syscall;
 
 	frame = (struct rt_sigframe __user *)regs->sp;
 
-	if (!access_ok(frame, sizeof(*frame)))
+	if (!access_ok(frame, frame_size))
 		goto badframe;
 
 	if (__copy_from_user(&set, &frame->uc.uc_sigmask, sizeof(set)))
@@ -118,7 +257,7 @@ SYSCALL_DEFINE0(rt_sigreturn)
 
 	set_current_blocked(&set);
 
-	if (restore_sigcontext(regs, &frame->uc.uc_mcontext))
+	if (restore_sigcontext(frame, regs))
 		goto badframe;
 
 	if (restore_altstack(&frame->uc.uc_stack))
@@ -141,15 +280,24 @@ SYSCALL_DEFINE0(rt_sigreturn)
 }
 
 static long setup_sigcontext(struct rt_sigframe __user *frame,
-	struct pt_regs *regs)
+			     struct pt_regs *regs)
 {
 	struct sigcontext __user *sc = &frame->uc.uc_mcontext;
+	void *sc_extn = &frame->sc_extn;
 	long err;
+
 	/* sc_regs is structured the same as the start of pt_regs */
 	err = __copy_to_user(&sc->sc_regs, regs, sizeof(sc->sc_regs));
 	/* Save the floating-point state. */
 	if (has_fpu())
 		err |= save_fp_state(regs, &sc->sc_fpregs);
+	/* Save the vector state. */
+	if (has_vector())
+		err |= save_v_state(regs, &sc_extn);
+
+	/* Put END __riscv_ctx_hdr at the end. */
+	err = __put_user(END_MAGIC, &((struct __riscv_ctx_hdr *)sc_extn)->magic);
+	err = __put_user(END_HDR_SIZE, &((struct __riscv_ctx_hdr *)sc_extn)->size);
 	return err;
 }
 
@@ -180,10 +328,11 @@ static int setup_rt_frame(struct ksignal *ksig, sigset_t *set,
 	struct pt_regs *regs)
 {
 	struct rt_sigframe __user *frame;
+	size_t frame_size = cal_rt_frame_size();
 	long err = 0;
 
-	frame = get_sigframe(ksig, regs, sizeof(*frame));
-	if (!access_ok(frame, sizeof(*frame)))
+	frame = get_sigframe(ksig, regs, frame_size);
+	if (!access_ok(frame, frame_size))
 		return -EFAULT;
 
 	err |= copy_siginfo_to_user(&frame->info, &ksig->info);
@@ -329,3 +478,9 @@ asmlinkage __visible void do_notify_resume(struct pt_regs *regs,
 	if (thread_info_flags & _TIF_NOTIFY_RESUME)
 		resume_user_mode_work(regs);
 }
+
+void __init init_rt_signal_env(void)
+{
+	/* Vector regfile + control regs. */
+	rvv_sc_size = sizeof(struct __sc_riscv_v_state) + riscv_vsize;
+}
-- 
2.34.1