| Message ID | 20230224170118.16766-16-andy.chiu@sifive.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | riscv: Add vector ISA support | expand |
On Fri, Feb 24, 2023 at 05:01:14PM +0000, Andy Chiu wrote: > MINSIGSTKSZ alone have become less informative by the time an user calls > sigaltstack(), as the kernel starts to support extensions that > dynamically introduce footprint on a signal frame. This sentence is a bit difficult to understand. I find re-hashing the wording often helps me understand, so does the following mean the same thing: "Some extensions, such as vector, dynamically change the footprint of a signal frame, so MINSIGSTKSZ is no longer accurate" The wording "less informative" doesn't really mean anything, what could happen here is that the sigaltstack could be larger the MINSIGSTKSZ and would therefore be outright wrong? > For example, an RV64V > implementation with vlen = 512 may occupy 2K + 40 + 12 Bytes of a signal > frame with the upcoming Vector support. > And there is no need for > reserving the extra sigframe for some processes that do not execute any > V-instructions. Can you reword this sentence like the following, substituting for "so xyz" please? "Processes that do not execute any vector instructions do not need to reserve the extra sigframe, so xyz". > Thus, provide the function sigaltstack_size_valid() to validate its size > based on current allocation status of supported extensions. > > Signed-off-by: Andy Chiu <andy.chiu@sifive.com> > --- > arch/riscv/kernel/signal.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/arch/riscv/kernel/signal.c b/arch/riscv/kernel/signal.c > index aa8ee95dee2d..aff441e83a98 100644 > --- a/arch/riscv/kernel/signal.c > +++ b/arch/riscv/kernel/signal.c > @@ -494,3 +494,11 @@ void __init init_rt_signal_env(void) > */ > signal_minsigstksz = cal_rt_frame_size(true); > } > + > +#ifdef CONFIG_DYNAMIC_SIGFRAME > +bool sigaltstack_size_valid(size_t ss_size) > +{ > + return ss_size > cal_rt_frame_size(false); Seeing it here made me wonder, what does "cal" mean. I assume it is meant to be "calculate", but "cal" in my head is usually "calibrate". s/cal/get in the patch adding that function IMO. > +} > +#endif /* CONFIG_DYNAMIC_SIGFRAME */ The change itself, if my understanding is correct, looks fine... Thanks, Conor.
diff --git a/arch/riscv/kernel/signal.c b/arch/riscv/kernel/signal.c index aa8ee95dee2d..aff441e83a98 100644 --- a/arch/riscv/kernel/signal.c +++ b/arch/riscv/kernel/signal.c @@ -494,3 +494,11 @@ void __init init_rt_signal_env(void) */ signal_minsigstksz = cal_rt_frame_size(true); } + +#ifdef CONFIG_DYNAMIC_SIGFRAME +bool sigaltstack_size_valid(size_t ss_size) +{ + return ss_size > cal_rt_frame_size(false); +} +#endif /* CONFIG_DYNAMIC_SIGFRAME */ +
MINSIGSTKSZ alone have become less informative by the time an user calls sigaltstack(), as the kernel starts to support extensions that dynamically introduce footprint on a signal frame. For example, an RV64V implementation with vlen = 512 may occupy 2K + 40 + 12 Bytes of a signal frame with the upcoming Vector support. And there is no need for reserving the extra sigframe for some processes that do not execute any V-instructions. Thus, provide the function sigaltstack_size_valid() to validate its size based on current allocation status of supported extensions. Signed-off-by: Andy Chiu <andy.chiu@sifive.com> --- arch/riscv/kernel/signal.c | 8 ++++++++ 1 file changed, 8 insertions(+)