[PATCH 16/30] bsd-user/signal.c: host_to_target_siginfo_noswap

Warner Losh posted 30 patches 4 years ago
There is a newer version of this series
[PATCH 16/30] bsd-user/signal.c: host_to_target_siginfo_noswap
Posted by Warner Losh 4 years ago
Implement conversion of host to target siginfo.

Signed-off-by: Stacey Son <sson@FreeBSD.org>
Signed-off-by: Kyle Evans <kevans@freebsd.org>
Signed-off-by: Warner Losh <imp@bsdimp.com>
---
 bsd-user/signal.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/bsd-user/signal.c b/bsd-user/signal.c
index 7168d851be8..3fe8b2d9898 100644
--- a/bsd-user/signal.c
+++ b/bsd-user/signal.c
@@ -43,6 +43,43 @@ int target_to_host_signal(int sig)
     return sig;
 }
 
+/* Siginfo conversion. */
+static inline void host_to_target_siginfo_noswap(target_siginfo_t *tinfo,
+        const siginfo_t *info)
+{
+    int sig, code;
+
+    sig = host_to_target_signal(info->si_signo);
+    /* XXX should have host_to_target_si_code() */
+    code = tswap32(info->si_code);
+    tinfo->si_signo = sig;
+    tinfo->si_errno = info->si_errno;
+    tinfo->si_code = info->si_code;
+    tinfo->si_pid = info->si_pid;
+    tinfo->si_uid = info->si_uid;
+    tinfo->si_status = info->si_status;
+    tinfo->si_addr = (abi_ulong)(unsigned long)info->si_addr;
+    /* si_value is opaque to kernel */
+    tinfo->si_value.sival_ptr =
+        (abi_ulong)(unsigned long)info->si_value.sival_ptr;
+    if (SIGILL == sig || SIGFPE == sig || SIGSEGV == sig || SIGBUS == sig ||
+            SIGTRAP == sig) {
+        tinfo->_reason._fault._trapno = info->_reason._fault._trapno;
+    }
+#ifdef SIGPOLL
+    if (SIGPOLL == sig) {
+        tinfo->_reason._poll._band = info->_reason._poll._band;
+    }
+#endif
+    if (SI_TIMER == code) {
+        int timerid;
+
+        timerid = info->_reason._timer._timerid;
+        tinfo->_reason._timer._timerid = timerid;
+        tinfo->_reason._timer._overrun = info->_reason._timer._overrun;
+    }
+}
+
 /*
  * Queue a signal so that it will be send to the virtual CPU as soon as
  * possible.
-- 
2.33.1


Re: [PATCH 16/30] bsd-user/signal.c: host_to_target_siginfo_noswap
Posted by Peter Maydell 4 years ago
On Sun, 9 Jan 2022 at 16:41, Warner Losh <imp@bsdimp.com> wrote:
>
> Implement conversion of host to target siginfo.
>
> Signed-off-by: Stacey Son <sson@FreeBSD.org>
> Signed-off-by: Kyle Evans <kevans@freebsd.org>
> Signed-off-by: Warner Losh <imp@bsdimp.com>
> ---
>  bsd-user/signal.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>
> diff --git a/bsd-user/signal.c b/bsd-user/signal.c
> index 7168d851be8..3fe8b2d9898 100644
> --- a/bsd-user/signal.c
> +++ b/bsd-user/signal.c
> @@ -43,6 +43,43 @@ int target_to_host_signal(int sig)
>      return sig;
>  }
>
> +/* Siginfo conversion. */
> +static inline void host_to_target_siginfo_noswap(target_siginfo_t *tinfo,
> +        const siginfo_t *info)
> +{
> +    int sig, code;
> +
> +    sig = host_to_target_signal(info->si_signo);
> +    /* XXX should have host_to_target_si_code() */
> +    code = tswap32(info->si_code);
> +    tinfo->si_signo = sig;
> +    tinfo->si_errno = info->si_errno;
> +    tinfo->si_code = info->si_code;
> +    tinfo->si_pid = info->si_pid;
> +    tinfo->si_uid = info->si_uid;
> +    tinfo->si_status = info->si_status;
> +    tinfo->si_addr = (abi_ulong)(unsigned long)info->si_addr;
> +    /* si_value is opaque to kernel */
> +    tinfo->si_value.sival_ptr =
> +        (abi_ulong)(unsigned long)info->si_value.sival_ptr;
> +    if (SIGILL == sig || SIGFPE == sig || SIGSEGV == sig || SIGBUS == sig ||

Don't use yoda-conditions, please. sig == SIGILL, etc.

> +            SIGTRAP == sig) {
> +        tinfo->_reason._fault._trapno = info->_reason._fault._trapno;
> +    }
> +#ifdef SIGPOLL
> +    if (SIGPOLL == sig) {
> +        tinfo->_reason._poll._band = info->_reason._poll._band;
> +    }
> +#endif
> +    if (SI_TIMER == code) {
> +        int timerid;
> +
> +        timerid = info->_reason._timer._timerid;
> +        tinfo->_reason._timer._timerid = timerid;
> +        tinfo->_reason._timer._overrun = info->_reason._timer._overrun;
> +    }
> +}

I think this will only compile on FreeBSD (the other BSDs having
notably different target_siginfo_t structs); I guess we're OK
with that ?

I also commented on the general setup linux-user has for this
function back in patch 2; I'll let you figure out whether what
you have here is the right thing for BSD.

-- PMM

Re: [PATCH 16/30] bsd-user/signal.c: host_to_target_siginfo_noswap
Posted by Warner Losh 4 years ago
On Thu, Jan 13, 2022 at 12:43 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Sun, 9 Jan 2022 at 16:41, Warner Losh <imp@bsdimp.com> wrote:
> >
> > Implement conversion of host to target siginfo.
> >
> > Signed-off-by: Stacey Son <sson@FreeBSD.org>
> > Signed-off-by: Kyle Evans <kevans@freebsd.org>
> > Signed-off-by: Warner Losh <imp@bsdimp.com>
> > ---
> >  bsd-user/signal.c | 37 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 37 insertions(+)
> >
> > diff --git a/bsd-user/signal.c b/bsd-user/signal.c
> > index 7168d851be8..3fe8b2d9898 100644
> > --- a/bsd-user/signal.c
> > +++ b/bsd-user/signal.c
> > @@ -43,6 +43,43 @@ int target_to_host_signal(int sig)
> >      return sig;
> >  }
> >
> > +/* Siginfo conversion. */
> > +static inline void host_to_target_siginfo_noswap(target_siginfo_t
> *tinfo,
> > +        const siginfo_t *info)
> > +{
> > +    int sig, code;
> > +
> > +    sig = host_to_target_signal(info->si_signo);
> > +    /* XXX should have host_to_target_si_code() */
> > +    code = tswap32(info->si_code);
> > +    tinfo->si_signo = sig;
> > +    tinfo->si_errno = info->si_errno;
> > +    tinfo->si_code = info->si_code;
> > +    tinfo->si_pid = info->si_pid;
> > +    tinfo->si_uid = info->si_uid;
> > +    tinfo->si_status = info->si_status;
> > +    tinfo->si_addr = (abi_ulong)(unsigned long)info->si_addr;
> > +    /* si_value is opaque to kernel */
> > +    tinfo->si_value.sival_ptr =
> > +        (abi_ulong)(unsigned long)info->si_value.sival_ptr;
> > +    if (SIGILL == sig || SIGFPE == sig || SIGSEGV == sig || SIGBUS ==
> sig ||
>
> Don't use yoda-conditions, please. sig == SIGILL, etc.
>
> > +            SIGTRAP == sig) {
> > +        tinfo->_reason._fault._trapno = info->_reason._fault._trapno;
> > +    }
> > +#ifdef SIGPOLL
> > +    if (SIGPOLL == sig) {
> > +        tinfo->_reason._poll._band = info->_reason._poll._band;
> > +    }
> > +#endif
> > +    if (SI_TIMER == code) {
> > +        int timerid;
> > +
> > +        timerid = info->_reason._timer._timerid;
> > +        tinfo->_reason._timer._timerid = timerid;
> > +        tinfo->_reason._timer._overrun = info->_reason._timer._overrun;
> > +    }
> > +}
>
> I think this will only compile on FreeBSD (the other BSDs having
> notably different target_siginfo_t structs); I guess we're OK
> with that ?
>

Yes. bsd-user fork does not compile on the other BSDs. There's too many
things missing, and too few places where specific code is in place. I'm
thinking
that it won't be possible to implement running NetBSD binaries on FreeBSD
or vice versa since they are so different. OpenBSD and NetBSD are a lot
closer to each other, with fewer critical differences in areas like
threads, so
it may be possible there. There's a lot of rework that's needed in this area
to take what's even in bsd-user fork and make it work on NetBSD or
OpenBSD, exactly for reasons like this. I've been ignoring the elephant
in the room for a while now, ever since I realized this fundamental
shift.


> I also commented on the general setup linux-user has for this
> function back in patch 2; I'll let you figure out whether what
> you have here is the right thing for BSD.
>

Yea. I'm still thinking through what you said there (and elsewhere). These
issues may be the root cause of some regressions in arm binaries between
6.1 and 6.2 as I tried to adopt the sigsegv/sigbus changes.

I need to work through those things in our development branch before trying
to fold them into this series. And I'm not yet sure the right way to do
that because
many of the things are likely to be largish changes that may be tough to
manage
keeping this patch series in sync. So I'm going to do all the trivial style
and
tiny bug things first, then tackle this more fundamental issue. I've thought
about it enough to understand that the code in this patch series has some
conceptual mistakes that must be addressed. Having this very detailed
feedback
is quite helpful in laying out the path for me to fix these issues (even if
I don't
ultimately do everything like linux-user, I'll know why it's different
rather than
the current situation where there's much inherited code and the best answer
I could give is 'well linux-user was like that 5 years ago and we needed to
make
these hacks to make things work' which is completely unsatisfying to give
and
to hear.

Warner
Re: [PATCH 16/30] bsd-user/signal.c: host_to_target_siginfo_noswap
Posted by Peter Maydell 4 years ago
On Sat, 15 Jan 2022 at 06:19, Warner Losh <imp@bsdimp.com> wrote:
> I need to work through those things in our development branch before trying
> to fold them into this series. And I'm not yet sure the right way to do that because
> many of the things are likely to be largish changes that may be tough to manage
> keeping this patch series in sync. So I'm going to do all the trivial style and
> tiny bug things first, then tackle this more fundamental issue. I've thought
> about it enough to understand that the code in this patch series has some
> conceptual mistakes that must be addressed. Having this very detailed feedback
> is quite helpful in laying out the path for me to fix these issues (even if I don't
> ultimately do everything like linux-user, I'll know why it's different rather than
> the current situation where there's much inherited code and the best answer
> I could give is 'well linux-user was like that 5 years ago and we needed to make
> these hacks to make things work' which is completely unsatisfying to give and
> to hear.

Mmm. To the extent that the signal handling code you have in your out-of-tree
branch is "this is what FreeBSD is shipping to users and it works more-or-less",
maybe we should just accept that upstream with (with comments noting that
it's got issues/is based on an older linux-user) and then update it to
match today's
linux-user as a second round of patching? If we have a definite path to
eventually getting to the right place, I don't want to insist that you update
all this stuff in your branch first before we let it land upstream if that's
going to burden you with massively more work.

-- PMM

Re: [PATCH 16/30] bsd-user/signal.c: host_to_target_siginfo_noswap
Posted by Richard Henderson 4 years ago
On 1/10/22 3:19 AM, Warner Losh wrote:
> +static inline void host_to_target_siginfo_noswap(target_siginfo_t *tinfo,
> +        const siginfo_t *info)
> +{
> +    int sig, code;
> +
> +    sig = host_to_target_signal(info->si_signo);

You now have a target signo, so...

> +    if (SIGILL == sig || SIGFPE == sig || SIGSEGV == sig || SIGBUS == sig ||
> +            SIGTRAP == sig) {

... you need TARGET_SIGFOO in the comparision.

Though, really, I think the categorization that Peter suggested is a better way to 
structure this.


r~

Re: [PATCH 16/30] bsd-user/signal.c: host_to_target_siginfo_noswap
Posted by Warner Losh 4 years ago
On Sun, Jan 23, 2022 at 7:05 PM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 1/10/22 3:19 AM, Warner Losh wrote:
> > +static inline void host_to_target_siginfo_noswap(target_siginfo_t
> *tinfo,
> > +        const siginfo_t *info)
> > +{
> > +    int sig, code;
> > +
> > +    sig = host_to_target_signal(info->si_signo);
>
> You now have a target signo, so...
>
> > +    if (SIGILL == sig || SIGFPE == sig || SIGSEGV == sig || SIGBUS ==
> sig ||
> > +            SIGTRAP == sig) {
>
> ... you need TARGET_SIGFOO in the comparision.
>
> Though, really, I think the categorization that Peter suggested is a
> better way to
> structure this.
>

How about both? Both is good? I've reworked based on Peter's suggestion, but
still have a need to be careful about target vs host signal numbers.
Thanks! I'd overlooked it.

Warner