| Message ID | 20220109161923.85683-24-imp@bsdimp.com |
|---|---|
| State | New |
| Headers | show |
| Series | bsd-user: upstream our signal implementation | expand |
On Sun, 9 Jan 2022 at 16:53, Warner Losh <imp@bsdimp.com> wrote: > > target_sigemptyset: resets a set to having no bits set > qemu_sigorset: computes the or of two sets > target_sigaddset: adds a signal to a set > target_sigismember: returns true when signal is a member > host_to_target_sigset_internal: convert host sigset to target > host_to_target_sigset: convert host sigset to target > target_to_host_sigset_internal: convert target sigset to host > target_to_host_sigset: convert target sigset to host > > 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/qemu.h | 3 ++ > bsd-user/signal.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 92 insertions(+) > > diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h > index e12617f5d69..e8c417c7c33 100644 > --- a/bsd-user/qemu.h > +++ b/bsd-user/qemu.h > @@ -223,7 +223,10 @@ void queue_signal(CPUArchState *env, int sig, target_siginfo_t *info); > abi_long do_sigaltstack(abi_ulong uss_addr, abi_ulong uoss_addr, abi_ulong sp); > int target_to_host_signal(int sig); > int host_to_target_signal(int sig); > +void host_to_target_sigset(target_sigset_t *d, const sigset_t *s); > +void target_to_host_sigset(sigset_t *d, const target_sigset_t *s); > void QEMU_NORETURN force_sig(int target_sig); > +int qemu_sigorset(sigset_t *dest, const sigset_t *left, const sigset_t *right); > > /* mmap.c */ > int target_mprotect(abi_ulong start, abi_ulong len, int prot); > diff --git a/bsd-user/signal.c b/bsd-user/signal.c > index 93c3b3c5033..8dadc9a39a7 100644 > --- a/bsd-user/signal.c > +++ b/bsd-user/signal.c > @@ -32,6 +32,9 @@ > > static struct target_sigaction sigact_table[TARGET_NSIG]; > static void host_signal_handler(int host_sig, siginfo_t *info, void *puc); > +static void target_to_host_sigset_internal(sigset_t *d, > + const target_sigset_t *s); > + > > int host_to_target_signal(int sig) > { > @@ -43,6 +46,44 @@ int target_to_host_signal(int sig) > return sig; > } > > +static inline void target_sigemptyset(target_sigset_t *set) > +{ > + memset(set, 0, sizeof(*set)); > +} > + > +#include <signal.h> Don't include system headers halfway through the file like this, please : put the #include at the top of the file with the others. > + > +int > +qemu_sigorset(sigset_t *dest, const sigset_t *left, const sigset_t *right) > +{ > + sigset_t work; > + int i; > + > + sigemptyset(&work); > + for (i = 1; i < NSIG; ++i) { > + if (sigismember(left, i) || sigismember(right, i)) { > + sigaddset(&work, i); > + } > + } > + > + *dest = work; > + return 0; > +} FreeBSD's manpage says it has a native sigorset() -- https://www.freebsd.org/cgi/man.cgi?query=sigemptyset&sektion=3&apropos=0&manpath=freebsd can you just use that ? > + > +static inline void target_sigaddset(target_sigset_t *set, int signum) > +{ > + signum--; > + uint32_t mask = (uint32_t)1 << (signum % TARGET_NSIG_BPW); > + set->__bits[signum / TARGET_NSIG_BPW] |= mask; > +} > + > +static inline int target_sigismember(const target_sigset_t *set, int signum) > +{ > + signum--; > + abi_ulong mask = (abi_ulong)1 << (signum % TARGET_NSIG_BPW); > + return (set->__bits[signum / TARGET_NSIG_BPW] & mask) != 0; > +} > + > /* Adjust the signal context to rewind out of safe-syscall if we're in it */ > static inline void rewind_if_in_safe_syscall(void *puc) > { > @@ -55,6 +96,54 @@ static inline void rewind_if_in_safe_syscall(void *puc) > } > } > > +static void host_to_target_sigset_internal(target_sigset_t *d, > + const sigset_t *s) > +{ > + int i; > + > + target_sigemptyset(d); > + for (i = 1; i <= TARGET_NSIG; i++) { i here is iterating through host signal numbers, not target numbers, so TARGET_NSIG isn't the right upper bound. On Linux we iterate from 1 to _NSIG-1; on BSD I think you may want (i = 0; i < NSIG; i++), but you should check that. > + if (sigismember(s, i)) { > + target_sigaddset(d, host_to_target_signal(i)); > + } > + } > +} These functions are a little odd when you compare them to their linux-user equivalents, because they're both written with a sort of abstraction between host and target signal numbers (they call host_to_target_signal() and target_to_host_signal()) but also written with baked-in assumptions that the mapping is basically 1:1 (they don't have the code that handles the possibility that the target signal isn't representable as a host signal or vice-versa). But assuming the BSDs don't change their signal numbering across architectures, this is fine. thanks -- PMM
On Fri, Jan 14, 2022 at 4:14 AM Peter Maydell <peter.maydell@linaro.org> wrote: > On Sun, 9 Jan 2022 at 16:53, Warner Losh <imp@bsdimp.com> wrote: > > > > target_sigemptyset: resets a set to having no bits set > > qemu_sigorset: computes the or of two sets > > target_sigaddset: adds a signal to a set > > target_sigismember: returns true when signal is a member > > host_to_target_sigset_internal: convert host sigset to target > > host_to_target_sigset: convert host sigset to target > > target_to_host_sigset_internal: convert target sigset to host > > target_to_host_sigset: convert target sigset to host > > > > 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/qemu.h | 3 ++ > > bsd-user/signal.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 92 insertions(+) > > > > diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h > > index e12617f5d69..e8c417c7c33 100644 > > --- a/bsd-user/qemu.h > > +++ b/bsd-user/qemu.h > > @@ -223,7 +223,10 @@ void queue_signal(CPUArchState *env, int sig, > target_siginfo_t *info); > > abi_long do_sigaltstack(abi_ulong uss_addr, abi_ulong uoss_addr, > abi_ulong sp); > > int target_to_host_signal(int sig); > > int host_to_target_signal(int sig); > > +void host_to_target_sigset(target_sigset_t *d, const sigset_t *s); > > +void target_to_host_sigset(sigset_t *d, const target_sigset_t *s); > > void QEMU_NORETURN force_sig(int target_sig); > > +int qemu_sigorset(sigset_t *dest, const sigset_t *left, const sigset_t > *right); > > > > /* mmap.c */ > > int target_mprotect(abi_ulong start, abi_ulong len, int prot); > > diff --git a/bsd-user/signal.c b/bsd-user/signal.c > > index 93c3b3c5033..8dadc9a39a7 100644 > > --- a/bsd-user/signal.c > > +++ b/bsd-user/signal.c > > @@ -32,6 +32,9 @@ > > > > static struct target_sigaction sigact_table[TARGET_NSIG]; > > static void host_signal_handler(int host_sig, siginfo_t *info, void > *puc); > > +static void target_to_host_sigset_internal(sigset_t *d, > > + const target_sigset_t *s); > > + > > > > int host_to_target_signal(int sig) > > { > > @@ -43,6 +46,44 @@ int target_to_host_signal(int sig) > > return sig; > > } > > > > +static inline void target_sigemptyset(target_sigset_t *set) > > +{ > > + memset(set, 0, sizeof(*set)); > > +} > > + > > +#include <signal.h> > > Don't include system headers halfway through the file like this, > please : put the #include at the top of the file with the others. > Yea, this isn't even needed, so I just removed it. > > + > > +int > > +qemu_sigorset(sigset_t *dest, const sigset_t *left, const sigset_t > *right) > > +{ > > + sigset_t work; > > + int i; > > + > > + sigemptyset(&work); > > + for (i = 1; i < NSIG; ++i) { > > + if (sigismember(left, i) || sigismember(right, i)) { > > + sigaddset(&work, i); > > + } > > + } > > + > > + *dest = work; > > + return 0; > > +} > > FreeBSD's manpage says it has a native sigorset() -- > > https://www.freebsd.org/cgi/man.cgi?query=sigemptyset&sektion=3&apropos=0&manpath=freebsd > can you just use that ? > Yes. > > + > > +static inline void target_sigaddset(target_sigset_t *set, int signum) > > +{ > > + signum--; > > + uint32_t mask = (uint32_t)1 << (signum % TARGET_NSIG_BPW); > > + set->__bits[signum / TARGET_NSIG_BPW] |= mask; > > +} > > + > > +static inline int target_sigismember(const target_sigset_t *set, int > signum) > > +{ > > + signum--; > > + abi_ulong mask = (abi_ulong)1 << (signum % TARGET_NSIG_BPW); > > + return (set->__bits[signum / TARGET_NSIG_BPW] & mask) != 0; > > +} > > + > > /* Adjust the signal context to rewind out of safe-syscall if we're in > it */ > > static inline void rewind_if_in_safe_syscall(void *puc) > > { > > @@ -55,6 +96,54 @@ static inline void rewind_if_in_safe_syscall(void > *puc) > > } > > } > > > > +static void host_to_target_sigset_internal(target_sigset_t *d, > > + const sigset_t *s) > > +{ > > + int i; > > + > > + target_sigemptyset(d); > > + for (i = 1; i <= TARGET_NSIG; i++) { > > i here is iterating through host signal numbers, not target > numbers, so TARGET_NSIG isn't the right upper bound. > On Linux we iterate from 1 to _NSIG-1; on BSD I think > you may want (i = 0; i < NSIG; i++), but you should check that. > You're correct. The values are the same, but logically NSIG is more correct. > > + if (sigismember(s, i)) { > > + target_sigaddset(d, host_to_target_signal(i)); > > + } > > + } > > +} > > These functions are a little odd when you compare them to their > linux-user equivalents, because they're both written > with a sort of abstraction between host and target signal > numbers (they call host_to_target_signal() and > target_to_host_signal()) but also written with baked-in > assumptions that the mapping is basically 1:1 (they don't > have the code that handles the possibility that the > target signal isn't representable as a host signal or > vice-versa). But assuming the BSDs don't change their > signal numbering across architectures, this is fine. > I can assume that, at least for now, so I've just added a comment about that. Warner
On Sat, Jan 22, 2022 at 10:44 AM Warner Losh <imp@bsdimp.com> wrote: > > > > On Fri, Jan 14, 2022 at 4:14 AM Peter Maydell <peter.maydell@linaro.org> wrote: >> >> On Sun, 9 Jan 2022 at 16:53, Warner Losh <imp@bsdimp.com> wrote: >> > >> > target_sigemptyset: resets a set to having no bits set >> > qemu_sigorset: computes the or of two sets >> > target_sigaddset: adds a signal to a set >> > target_sigismember: returns true when signal is a member >> > host_to_target_sigset_internal: convert host sigset to target >> > host_to_target_sigset: convert host sigset to target >> > target_to_host_sigset_internal: convert target sigset to host >> > target_to_host_sigset: convert target sigset to host >> > >> > 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/qemu.h | 3 ++ >> > bsd-user/signal.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++ >> > 2 files changed, 92 insertions(+) >> > >> > diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h >> > index e12617f5d69..e8c417c7c33 100644 >> > --- a/bsd-user/qemu.h >> > +++ b/bsd-user/qemu.h >> > @@ -223,7 +223,10 @@ void queue_signal(CPUArchState *env, int sig, target_siginfo_t *info); >> > abi_long do_sigaltstack(abi_ulong uss_addr, abi_ulong uoss_addr, abi_ulong sp); >> > int target_to_host_signal(int sig); >> > int host_to_target_signal(int sig); >> > +void host_to_target_sigset(target_sigset_t *d, const sigset_t *s); >> > +void target_to_host_sigset(sigset_t *d, const target_sigset_t *s); >> > void QEMU_NORETURN force_sig(int target_sig); >> > +int qemu_sigorset(sigset_t *dest, const sigset_t *left, const sigset_t *right); >> > >> > /* mmap.c */ >> > int target_mprotect(abi_ulong start, abi_ulong len, int prot); >> > diff --git a/bsd-user/signal.c b/bsd-user/signal.c >> > index 93c3b3c5033..8dadc9a39a7 100644 >> > --- a/bsd-user/signal.c >> > +++ b/bsd-user/signal.c >> > @@ -32,6 +32,9 @@ >> > >> > static struct target_sigaction sigact_table[TARGET_NSIG]; >> > static void host_signal_handler(int host_sig, siginfo_t *info, void *puc); >> > +static void target_to_host_sigset_internal(sigset_t *d, >> > + const target_sigset_t *s); >> > + >> > >> > int host_to_target_signal(int sig) >> > { >> > @@ -43,6 +46,44 @@ int target_to_host_signal(int sig) >> > return sig; >> > } >> > >> > +static inline void target_sigemptyset(target_sigset_t *set) >> > +{ >> > + memset(set, 0, sizeof(*set)); >> > +} >> > + >> > +#include <signal.h> >> >> Don't include system headers halfway through the file like this, >> please : put the #include at the top of the file with the others. > > > Yea, this isn't even needed, so I just removed it. > >> >> > + >> > +int >> > +qemu_sigorset(sigset_t *dest, const sigset_t *left, const sigset_t *right) >> > +{ >> > + sigset_t work; >> > + int i; >> > + >> > + sigemptyset(&work); >> > + for (i = 1; i < NSIG; ++i) { >> > + if (sigismember(left, i) || sigismember(right, i)) { >> > + sigaddset(&work, i); >> > + } >> > + } >> > + >> > + *dest = work; >> > + return 0; >> > +} >> >> FreeBSD's manpage says it has a native sigorset() -- >> https://www.freebsd.org/cgi/man.cgi?query=sigemptyset&sektion=3&apropos=0&manpath=freebsd >> can you just use that ? > > > Yes. > For some added context, I added sigorset() to libc after 11.3/12.1 in response to bsd-user using it, then forgot to remove the transition aide after they went EoL. Thanks, Kyle Evans
diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h index e12617f5d69..e8c417c7c33 100644 --- a/bsd-user/qemu.h +++ b/bsd-user/qemu.h @@ -223,7 +223,10 @@ void queue_signal(CPUArchState *env, int sig, target_siginfo_t *info); abi_long do_sigaltstack(abi_ulong uss_addr, abi_ulong uoss_addr, abi_ulong sp); int target_to_host_signal(int sig); int host_to_target_signal(int sig); +void host_to_target_sigset(target_sigset_t *d, const sigset_t *s); +void target_to_host_sigset(sigset_t *d, const target_sigset_t *s); void QEMU_NORETURN force_sig(int target_sig); +int qemu_sigorset(sigset_t *dest, const sigset_t *left, const sigset_t *right); /* mmap.c */ int target_mprotect(abi_ulong start, abi_ulong len, int prot); diff --git a/bsd-user/signal.c b/bsd-user/signal.c index 93c3b3c5033..8dadc9a39a7 100644 --- a/bsd-user/signal.c +++ b/bsd-user/signal.c @@ -32,6 +32,9 @@ static struct target_sigaction sigact_table[TARGET_NSIG]; static void host_signal_handler(int host_sig, siginfo_t *info, void *puc); +static void target_to_host_sigset_internal(sigset_t *d, + const target_sigset_t *s); + int host_to_target_signal(int sig) { @@ -43,6 +46,44 @@ int target_to_host_signal(int sig) return sig; } +static inline void target_sigemptyset(target_sigset_t *set) +{ + memset(set, 0, sizeof(*set)); +} + +#include <signal.h> + +int +qemu_sigorset(sigset_t *dest, const sigset_t *left, const sigset_t *right) +{ + sigset_t work; + int i; + + sigemptyset(&work); + for (i = 1; i < NSIG; ++i) { + if (sigismember(left, i) || sigismember(right, i)) { + sigaddset(&work, i); + } + } + + *dest = work; + return 0; +} + +static inline void target_sigaddset(target_sigset_t *set, int signum) +{ + signum--; + uint32_t mask = (uint32_t)1 << (signum % TARGET_NSIG_BPW); + set->__bits[signum / TARGET_NSIG_BPW] |= mask; +} + +static inline int target_sigismember(const target_sigset_t *set, int signum) +{ + signum--; + abi_ulong mask = (abi_ulong)1 << (signum % TARGET_NSIG_BPW); + return (set->__bits[signum / TARGET_NSIG_BPW] & mask) != 0; +} + /* Adjust the signal context to rewind out of safe-syscall if we're in it */ static inline void rewind_if_in_safe_syscall(void *puc) { @@ -55,6 +96,54 @@ static inline void rewind_if_in_safe_syscall(void *puc) } } +static void host_to_target_sigset_internal(target_sigset_t *d, + const sigset_t *s) +{ + int i; + + target_sigemptyset(d); + for (i = 1; i <= TARGET_NSIG; i++) { + if (sigismember(s, i)) { + target_sigaddset(d, host_to_target_signal(i)); + } + } +} + +void host_to_target_sigset(target_sigset_t *d, const sigset_t *s) +{ + target_sigset_t d1; + int i; + + host_to_target_sigset_internal(&d1, s); + for (i = 0; i < TARGET_NSIG_WORDS; i++) { + d->__bits[i] = tswap32(d1.__bits[i]); + } +} + +static void target_to_host_sigset_internal(sigset_t *d, + const target_sigset_t *s) +{ + int i; + + sigemptyset(d); + for (i = 1; i <= TARGET_NSIG; i++) { + if (target_sigismember(s, i)) { + sigaddset(d, target_to_host_signal(i)); + } + } +} + +void target_to_host_sigset(sigset_t *d, const target_sigset_t *s) +{ + target_sigset_t s1; + int i; + + for (i = 0; i < TARGET_NSIG_WORDS; i++) { + s1.__bits[i] = tswap32(s->__bits[i]); + } + target_to_host_sigset_internal(d, &s1); +} + /* Siginfo conversion. */ static inline void host_to_target_siginfo_noswap(target_siginfo_t *tinfo, const siginfo_t *info)