From: Jan Kiszka <jan.kiszka@siemens.com>
To: Johannes Kirchmair <johannes.kirchmair@sigmatek.at>,
xenomai@lists.linux.dev, "Schaffner,
Tobias (T CED SES-DE)" <tobias.schaffner@siemens.com>
Cc: johannes.kirchmair@simgatek.at
Subject: Re: [PATCH] add dovtail signal setup function
Date: Mon, 14 Aug 2023 19:33:00 +0200 [thread overview]
Message-ID: <10100be7-f83b-4882-a147-25471784af01@siemens.com> (raw)
In-Reply-To: <20230509131151.3003178-1-johannes.kirchmair@sigmatek.at>
On 09.05.23 15:11, Johannes Kirchmair wrote:
> add an interface to dovetail for signal frame setup and restoring.
>
> This is useful for implementing rt signals within Xenomai.
>
> The interface makes it possible to use the setup use functions provided
> by Linux.
>
> This is just a proof of concept using a very naive approach using most
> of the code provided by Linux to setup signals.
> The patch should be discussed, especially considering if using the
> Linux functions is viable in a rt context.
I didn't find issues in using the Linux services so far. But the code
needs restructuring to move arch-specific bits into the respective arch
code.
There is also the issue with the return value that Tobias found. I think
the dovetail helpers do not need to return the return register, rather
only 0 on success or a negative error code.
>
> Also to discuss would be the interface itself,
> for convenience I used the kernels struct ksignal for testing. But maybe
> we should maintain our own struct, that is not that feature rich.
Before inventing something new, we would really have to understand that
this is better. Also, some of the functions you reused expect ksignal -
another reason to keep it.
> ---
> arch/x86/include/asm/sighandling.h | 3 ++
> arch/x86/kernel/signal_32.c | 2 +-
> arch/x86/kernel/signal_64.c | 2 +-
> include/linux/dovetail.h | 4 +++
> kernel/dovetail.c | 57 ++++++++++++++++++++++++++++++
> 5 files changed, 66 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/include/asm/sighandling.h b/arch/x86/include/asm/sighandling.h
> index e770c4fc47f4..ed6b7fcd7250 100644
> --- a/arch/x86/include/asm/sighandling.h
> +++ b/arch/x86/include/asm/sighandling.h
> @@ -24,4 +24,7 @@ int ia32_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs);
> int x64_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs);
> int x32_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs);
>
> +bool ia32_restore_sigcontext(struct pt_regs *regs, struct sigcontext_32 __user *usc);
> +bool restore_sigcontext(struct pt_regs *regs, struct sigcontext __user *usc, unsigned long uc_flags);
> +
> #endif /* _ASM_X86_SIGHANDLING_H */
> diff --git a/arch/x86/kernel/signal_32.c b/arch/x86/kernel/signal_32.c
> index 9027fc088f97..dc8d60378e75 100644
> --- a/arch/x86/kernel/signal_32.c
> +++ b/arch/x86/kernel/signal_32.c
> @@ -74,7 +74,7 @@ static inline void reload_segments(struct sigcontext_32 *sc)
> /*
> * Do a signal return; undo the signal stack.
> */
> -static bool ia32_restore_sigcontext(struct pt_regs *regs,
> +bool ia32_restore_sigcontext(struct pt_regs *regs,
> struct sigcontext_32 __user *usc)
> {
> struct sigcontext_32 sc;
> diff --git a/arch/x86/kernel/signal_64.c b/arch/x86/kernel/signal_64.c
> index 13a1e6083837..d24845ee6a8d 100644
> --- a/arch/x86/kernel/signal_64.c
> +++ b/arch/x86/kernel/signal_64.c
> @@ -47,7 +47,7 @@ static void force_valid_ss(struct pt_regs *regs)
> regs->ss = __USER_DS;
> }
>
> -static bool restore_sigcontext(struct pt_regs *regs,
> +bool restore_sigcontext(struct pt_regs *regs,
> struct sigcontext __user *usc,
> unsigned long uc_flags)
> {
> diff --git a/include/linux/dovetail.h b/include/linux/dovetail.h
> index 70b78fd55d5e..3384437a4c74 100644
> --- a/include/linux/dovetail.h
> +++ b/include/linux/dovetail.h
> @@ -150,6 +150,10 @@ void oob_trampoline(void);
>
> void arch_inband_task_init(struct task_struct *p);
>
> +int dovetail_setup_rt_signal_frame(struct ksignal *ksig, struct pt_regs *regs);
> +
> +int dovetail_restore_rt_signal_frame(struct pt_regs *regs);
> +
I think, this makes a reasonable interface. Its goal would be to
create/restore a Linux-compatible signal frame and hide all those
details from the RT core.
> int dovetail_start(void);
>
> void dovetail_stop(void);
> diff --git a/kernel/dovetail.c b/kernel/dovetail.c
> index 79fd75918b37..2a61bf1dbc23 100644
> --- a/kernel/dovetail.c
> +++ b/kernel/dovetail.c
> @@ -11,6 +11,9 @@
> #include <asm/syscall.h>
> #include <uapi/asm-generic/dovetail.h>
>
> +#include <asm/sighandling.h>
> +#include <asm/sigframe.h>
> +
> static bool dovetail_enabled;
>
> void __weak arch_inband_task_init(struct task_struct *p)
> @@ -447,3 +450,57 @@ void dovetail_stop(void)
> smp_wmb();
> }
> EXPORT_SYMBOL_GPL(dovetail_stop);
> +
> +int dovetail_setup_rt_signal_frame(struct ksignal *ksig, struct pt_regs *regs)
> +{
> + int ret;
> + if (regs->cs == __USER_CS) {
> + ret = x64_setup_rt_frame(ksig, regs);
> + } else if (regs->cs == __USER32_CS) {
> + ksig->ka.sa.sa_flags |= SA_IA32_ABI;
> + ret = ia32_setup_rt_frame(ksig, regs);
> + } else {
> + ret = 1;
> + }
> +
> + return ret;
> +}
> +
> +static int dovetail_restore_64_rt_signal_frame(struct pt_regs *regs)
> +{
> + struct rt_sigframe __user *frame;
> + unsigned long uc_flags;
> +
> + frame = (struct rt_sigframe __user *)(regs->sp - sizeof(long));
> + if (!access_ok(frame, sizeof(*frame)))
> + return -1;
> + if (__get_user(uc_flags, &frame->uc.uc_flags))
> + return -1;
> +
> + if (!restore_sigcontext(regs, &frame->uc.uc_mcontext, uc_flags))
> + return -1;
> +
> + return regs->ax;
> +}
> +
> +static int dovetail_restore_32_rt_signal_frame(struct pt_regs *regs)
> +{
> + struct rt_sigframe_ia32 __user *frame = (struct rt_sigframe_ia32 __user *)(regs->sp - 4);
> +
> + if (!access_ok(frame, sizeof(*frame)))
> + return -1;
> +
> + if (ia32_restore_sigcontext(regs, &frame->uc.uc_mcontext) != true)
> + return -1;
> +
> + return regs->ax;
> +}
> +
> +int dovetail_restore_rt_signal_frame(struct pt_regs *regs)
> +{
> + if (regs->cs == __USER_CS)
> + return dovetail_restore_64_rt_signal_frame(regs);
> + else
> + return dovetail_restore_32_rt_signal_frame(regs);
> +}
> +
These need to go into x86. Not sure if there would be anything generic
remaining, likely not.
And then we need code for arm and arm64.
Jan
--
Siemens AG, Technology
Linux Expert Center
next prev parent reply other threads:[~2023-08-14 17:33 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-09 13:11 [PATCH] add dovtail signal setup function Johannes Kirchmair
2023-08-14 17:33 ` Jan Kiszka [this message]
-- strict thread matches above, loose matches on Subject: below --
2023-08-16 10:18 Johannes Kirchmair
2023-08-16 10:24 ` Johannes Kirchmair
2023-08-16 10:33 ` Jan Kiszka
2023-08-16 10:39 ` Florian Bezdeka
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=10100be7-f83b-4882-a147-25471784af01@siemens.com \
--to=jan.kiszka@siemens.com \
--cc=johannes.kirchmair@sigmatek.at \
--cc=johannes.kirchmair@simgatek.at \
--cc=tobias.schaffner@siemens.com \
--cc=xenomai@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).