* [PATCH for-6.2 1/7] linux-user/aarch64: Set siginfo_t addr field for SIGTRAP signals
2021-08-13 13:18 [PATCH for-6.2 0/7] linux-user: Clean up siginfo_t handling for arm, aarch64 Peter Maydell
@ 2021-08-13 13:18 ` Peter Maydell
2021-08-15 19:51 ` Richard Henderson
2021-08-13 13:18 ` [PATCH for-6.2 2/7] linux-user/arm: " Peter Maydell
` (6 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Peter Maydell @ 2021-08-13 13:18 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Laurent Vivier
When generating a TRAP_BRKPT SIGTRAP, set the siginfo_t addr field
to the PC where the breakpoint/singlestep trap occurred; this is
what the kernel does for this signal for this architecture.
Fixes: Coverity 1459154
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
linux-user/aarch64/cpu_loop.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/linux-user/aarch64/cpu_loop.c b/linux-user/aarch64/cpu_loop.c
index ee72a1c20f0..5d8675944d9 100644
--- a/linux-user/aarch64/cpu_loop.c
+++ b/linux-user/aarch64/cpu_loop.c
@@ -148,6 +148,7 @@ void cpu_loop(CPUARMState *env)
info.si_signo = TARGET_SIGTRAP;
info.si_errno = 0;
info.si_code = TARGET_TRAP_BRKPT;
+ info._sifields._sigfault._addr = env->pc;
queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
break;
case EXCP_SEMIHOST:
--
2.20.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH for-6.2 2/7] linux-user/arm: Set siginfo_t addr field for SIGTRAP signals
2021-08-13 13:18 [PATCH for-6.2 0/7] linux-user: Clean up siginfo_t handling for arm, aarch64 Peter Maydell
2021-08-13 13:18 ` [PATCH for-6.2 1/7] linux-user/aarch64: Set siginfo_t addr field for SIGTRAP signals Peter Maydell
@ 2021-08-13 13:18 ` Peter Maydell
2021-08-15 19:53 ` Richard Henderson
2021-08-13 13:18 ` [PATCH for-6.2 3/7] linux-user/arm: Use force_sig() to deliver fpa11 emulation SIGFPE Peter Maydell
` (5 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Peter Maydell @ 2021-08-13 13:18 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Laurent Vivier
When generating a TRAP_BRKPT SIGTRAP, set the siginfo_t addr field
to the PC where the breakpoint/singlestep trap occurred; this is
what the kernel does for this signal for this architecture.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
linux-user/arm/cpu_loop.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
index 69632d15be1..007752f5b74 100644
--- a/linux-user/arm/cpu_loop.c
+++ b/linux-user/arm/cpu_loop.c
@@ -453,6 +453,7 @@ void cpu_loop(CPUARMState *env)
info.si_signo = TARGET_SIGTRAP;
info.si_errno = 0;
info.si_code = TARGET_TRAP_BRKPT;
+ info._sifields._sigfault._addr = env->regs[15];
queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
break;
case EXCP_KERNEL_TRAP:
--
2.20.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH for-6.2 2/7] linux-user/arm: Set siginfo_t addr field for SIGTRAP signals
2021-08-13 13:18 ` [PATCH for-6.2 2/7] linux-user/arm: " Peter Maydell
@ 2021-08-15 19:53 ` Richard Henderson
0 siblings, 0 replies; 18+ messages in thread
From: Richard Henderson @ 2021-08-15 19:53 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel; +Cc: Laurent Vivier
On 8/13/21 3:18 AM, Peter Maydell wrote:
> When generating a TRAP_BRKPT SIGTRAP, set the siginfo_t addr field
> to the PC where the breakpoint/singlestep trap occurred; this is
> what the kernel does for this signal for this architecture.
>
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> linux-user/arm/cpu_loop.c | 1 +
> 1 file changed, 1 insertion(+)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH for-6.2 3/7] linux-user/arm: Use force_sig() to deliver fpa11 emulation SIGFPE
2021-08-13 13:18 [PATCH for-6.2 0/7] linux-user: Clean up siginfo_t handling for arm, aarch64 Peter Maydell
2021-08-13 13:18 ` [PATCH for-6.2 1/7] linux-user/aarch64: Set siginfo_t addr field for SIGTRAP signals Peter Maydell
2021-08-13 13:18 ` [PATCH for-6.2 2/7] linux-user/arm: " Peter Maydell
@ 2021-08-13 13:18 ` Peter Maydell
2021-08-15 20:00 ` Richard Henderson
2021-08-13 13:18 ` [PATCH for-6.2 4/7] linux-user: Zero out target_siginfo_t in force_sig() Peter Maydell
` (4 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Peter Maydell @ 2021-08-13 13:18 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Laurent Vivier
In the Arm target code, when the fpa11 emulation code tells us we
need to send the guest a SIGFPE, we do this with queue_signal(), but
we are using the wrong si_type, and we aren't setting the _sifields
union members corresponding to either the si_type we are using or the
si_type we should be using.
As the existing comment notes, the kernel code for this calls the old
send_sig() function to deliver the signal. This eventually results
in the kernel's signal handling code fabricating a siginfo_t with a
SI_KERNEL code and a zero pid and uid. For QEMU this means we need
to use QEMU_SI_KILL. We already have a function for that:
force_sig() sets up the whole target_siginfo_t the way we need it.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
linux-user/arm/cpu_loop.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
index 007752f5b74..44324976196 100644
--- a/linux-user/arm/cpu_loop.c
+++ b/linux-user/arm/cpu_loop.c
@@ -266,16 +266,13 @@ static bool emulate_arm_fpa11(CPUARMState *env, uint32_t opcode)
ts->fpa.fpsr |= raise & ~enabled;
if (raise & enabled) {
- target_siginfo_t info = { };
-
/*
* The kernel's nwfpe emulator does not pass a real si_code.
- * It merely uses send_sig(SIGFPE, current, 1).
+ * It merely uses send_sig(SIGFPE, current, 1), which results in
+ * __send_signal() filling out SI_KERNEL with pid and uid 0 (under
+ * the "SEND_SIG_PRIV" case). That's what our force_sig() does.
*/
- info.si_signo = TARGET_SIGFPE;
- info.si_code = TARGET_SI_KERNEL;
-
- queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
+ force_sig(TARGET_SIGFPE);
} else {
env->regs[15] += 4;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH for-6.2 3/7] linux-user/arm: Use force_sig() to deliver fpa11 emulation SIGFPE
2021-08-13 13:18 ` [PATCH for-6.2 3/7] linux-user/arm: Use force_sig() to deliver fpa11 emulation SIGFPE Peter Maydell
@ 2021-08-15 20:00 ` Richard Henderson
0 siblings, 0 replies; 18+ messages in thread
From: Richard Henderson @ 2021-08-15 20:00 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel; +Cc: Laurent Vivier
On 8/13/21 3:18 AM, Peter Maydell wrote:
> In the Arm target code, when the fpa11 emulation code tells us we
> need to send the guest a SIGFPE, we do this with queue_signal(), but
> we are using the wrong si_type, and we aren't setting the _sifields
> union members corresponding to either the si_type we are using or the
> si_type we should be using.
>
> As the existing comment notes, the kernel code for this calls the old
> send_sig() function to deliver the signal. This eventually results
> in the kernel's signal handling code fabricating a siginfo_t with a
> SI_KERNEL code and a zero pid and uid. For QEMU this means we need
> to use QEMU_SI_KILL. We already have a function for that:
> force_sig() sets up the whole target_siginfo_t the way we need it.
>
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> linux-user/arm/cpu_loop.c | 11 ++++-------
> 1 file changed, 4 insertions(+), 7 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH for-6.2 4/7] linux-user: Zero out target_siginfo_t in force_sig()
2021-08-13 13:18 [PATCH for-6.2 0/7] linux-user: Clean up siginfo_t handling for arm, aarch64 Peter Maydell
` (2 preceding siblings ...)
2021-08-13 13:18 ` [PATCH for-6.2 3/7] linux-user/arm: Use force_sig() to deliver fpa11 emulation SIGFPE Peter Maydell
@ 2021-08-13 13:18 ` Peter Maydell
2021-08-15 20:00 ` Richard Henderson
2021-08-13 13:18 ` [PATCH for-6.2 5/7] linux-user: Provide new force_sig_fault() function Peter Maydell
` (3 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Peter Maydell @ 2021-08-13 13:18 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Laurent Vivier
The target_siginfo_t we populate in force_sig() will eventually
get copied onto the target's stack. Zero it out so that any extra
padding in the sifields union is consistently zero when the guest
sees it.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
linux-user/signal.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/linux-user/signal.c b/linux-user/signal.c
index a8faea6f090..fd3c6a3e60d 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -636,7 +636,7 @@ void force_sig(int sig)
{
CPUState *cpu = thread_cpu;
CPUArchState *env = cpu->env_ptr;
- target_siginfo_t info;
+ target_siginfo_t info = {};
info.si_signo = sig;
info.si_errno = 0;
--
2.20.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH for-6.2 4/7] linux-user: Zero out target_siginfo_t in force_sig()
2021-08-13 13:18 ` [PATCH for-6.2 4/7] linux-user: Zero out target_siginfo_t in force_sig() Peter Maydell
@ 2021-08-15 20:00 ` Richard Henderson
0 siblings, 0 replies; 18+ messages in thread
From: Richard Henderson @ 2021-08-15 20:00 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel; +Cc: Laurent Vivier
On 8/13/21 3:18 AM, Peter Maydell wrote:
> The target_siginfo_t we populate in force_sig() will eventually
> get copied onto the target's stack. Zero it out so that any extra
> padding in the sifields union is consistently zero when the guest
> sees it.
>
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> linux-user/signal.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH for-6.2 5/7] linux-user: Provide new force_sig_fault() function
2021-08-13 13:18 [PATCH for-6.2 0/7] linux-user: Clean up siginfo_t handling for arm, aarch64 Peter Maydell
` (3 preceding siblings ...)
2021-08-13 13:18 ` [PATCH for-6.2 4/7] linux-user: Zero out target_siginfo_t in force_sig() Peter Maydell
@ 2021-08-13 13:18 ` Peter Maydell
2021-08-15 20:10 ` Richard Henderson
2021-08-13 13:18 ` [PATCH for-6.2 6/7] linux-user/arm: Use force_sig_fault() Peter Maydell
` (2 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Peter Maydell @ 2021-08-13 13:18 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Laurent Vivier
In many places in the linux-user code we need to queue a signal for
the guest using the QEMU_SI_FAULT si_type. This requires that the
caller sets up and passes us a target_siginfo, including setting the
appropriate part of the _sifields union for the si_type. In a number
of places the code forgets to set the _sifields union field.
Provide a new force_sig_fault() function, which does the same thing
as the Linux kernel function of that name -- it takes the signal
number, the si_code value and the address to use in
_sifields._sigfault, and assembles the target_siginfo itself. This
makes the callsites simpler and means it's harder to forget to pass
in an address value.
We follow force_sig() and the kernel's force_sig_fault() in not
requiring the caller to pass in the CPU pointer but always acting
on the CPU of the current thread.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
linux-user/signal-common.h | 1 +
linux-user/signal.c | 17 +++++++++++++++++
2 files changed, 18 insertions(+)
diff --git a/linux-user/signal-common.h b/linux-user/signal-common.h
index ea86328b289..536c7ac2c20 100644
--- a/linux-user/signal-common.h
+++ b/linux-user/signal-common.h
@@ -40,6 +40,7 @@ void tswap_siginfo(target_siginfo_t *tinfo,
void set_sigmask(const sigset_t *set);
void force_sig(int sig);
void force_sigsegv(int oldsig);
+void force_sig_fault(int sig, int code, abi_ulong addr);
#if defined(TARGET_ARCH_HAS_SETUP_FRAME)
void setup_frame(int sig, struct target_sigaction *ka,
target_sigset_t *set, CPUArchState *env);
diff --git a/linux-user/signal.c b/linux-user/signal.c
index fd3c6a3e60d..5ea8e4584a7 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -646,6 +646,23 @@ void force_sig(int sig)
queue_signal(env, info.si_signo, QEMU_SI_KILL, &info);
}
+/*
+ * Force a synchronously taken QEMU_SI_FAULT signal. For QEMU the
+ * 'force' part is handled in process_pending_signals().
+ */
+void force_sig_fault(int sig, int code, abi_ulong addr)
+{
+ CPUState *cpu = thread_cpu;
+ CPUArchState *env = cpu->env_ptr;
+ target_siginfo_t info = {};
+
+ info.si_signo = sig;
+ info.si_errno = 0;
+ info.si_code = code;
+ info._sifields._sigfault._addr = addr;
+ queue_signal(env, sig, QEMU_SI_FAULT, &info);
+}
+
/* Force a SIGSEGV if we couldn't write to memory trying to set
* up the signal frame. oldsig is the signal we were trying to handle
* at the point of failure.
--
2.20.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH for-6.2 5/7] linux-user: Provide new force_sig_fault() function
2021-08-13 13:18 ` [PATCH for-6.2 5/7] linux-user: Provide new force_sig_fault() function Peter Maydell
@ 2021-08-15 20:10 ` Richard Henderson
2021-08-16 9:03 ` Peter Maydell
0 siblings, 1 reply; 18+ messages in thread
From: Richard Henderson @ 2021-08-15 20:10 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel; +Cc: Laurent Vivier
On 8/13/21 3:18 AM, Peter Maydell wrote:
> +void force_sig_fault(int sig, int code, abi_ulong addr)
Better as abi_ptr?
Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH for-6.2 5/7] linux-user: Provide new force_sig_fault() function
2021-08-15 20:10 ` Richard Henderson
@ 2021-08-16 9:03 ` Peter Maydell
2021-08-16 17:27 ` Richard Henderson
0 siblings, 1 reply; 18+ messages in thread
From: Peter Maydell @ 2021-08-16 9:03 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-arm, QEMU Developers, Laurent Vivier
On Sun, 15 Aug 2021 at 21:10, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 8/13/21 3:18 AM, Peter Maydell wrote:
> > +void force_sig_fault(int sig, int code, abi_ulong addr)
>
> Better as abi_ptr?
I followed the same type used for 'addr' in the target_siginfo_t
struct definition.
-- PMM
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH for-6.2 5/7] linux-user: Provide new force_sig_fault() function
2021-08-16 9:03 ` Peter Maydell
@ 2021-08-16 17:27 ` Richard Henderson
0 siblings, 0 replies; 18+ messages in thread
From: Richard Henderson @ 2021-08-16 17:27 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-arm, QEMU Developers, Laurent Vivier
On 8/15/21 11:03 PM, Peter Maydell wrote:
> On Sun, 15 Aug 2021 at 21:10, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 8/13/21 3:18 AM, Peter Maydell wrote:
>>> +void force_sig_fault(int sig, int code, abi_ulong addr)
>>
>> Better as abi_ptr?
>
> I followed the same type used for 'addr' in the target_siginfo_t
> struct definition.
Fair enough.
r~
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH for-6.2 6/7] linux-user/arm: Use force_sig_fault()
2021-08-13 13:18 [PATCH for-6.2 0/7] linux-user: Clean up siginfo_t handling for arm, aarch64 Peter Maydell
` (4 preceding siblings ...)
2021-08-13 13:18 ` [PATCH for-6.2 5/7] linux-user: Provide new force_sig_fault() function Peter Maydell
@ 2021-08-13 13:18 ` Peter Maydell
2021-08-15 20:25 ` Richard Henderson
2021-08-13 13:18 ` [PATCH for-6.2 7/7] linux-user/aarch64: " Peter Maydell
2021-09-23 13:12 ` [PATCH for-6.2 0/7] linux-user: Clean up siginfo_t handling for arm, aarch64 Laurent Vivier
7 siblings, 1 reply; 18+ messages in thread
From: Peter Maydell @ 2021-08-13 13:18 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Laurent Vivier
Use the new force_sig_fault() function instead of setting up
a target_siginfo_t and calling queue_signal().
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I threw in a comment confirming that the si_addr value for the "bad
SWI immediate" SIGILL really is different from the PC value reported
in the ucontext_t and resumed from if the handler returns, because it
looked like a bug to me when I was reading the code...
---
linux-user/arm/cpu_loop.c | 54 ++++++++++++---------------------------
1 file changed, 16 insertions(+), 38 deletions(-)
diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
index 44324976196..d4b4f0c71fc 100644
--- a/linux-user/arm/cpu_loop.c
+++ b/linux-user/arm/cpu_loop.c
@@ -22,6 +22,7 @@
#include "qemu.h"
#include "elf.h"
#include "cpu_loop-common.h"
+#include "signal-common.h"
#include "semihosting/common-semi.h"
#define get_user_code_u32(x, gaddr, env) \
@@ -92,7 +93,6 @@ static void arm_kernel_cmpxchg64_helper(CPUARMState *env)
{
uint64_t oldval, newval, val;
uint32_t addr, cpsr;
- target_siginfo_t info;
/* Based on the 32 bit code in do_kernel_trap */
@@ -141,12 +141,9 @@ segv:
end_exclusive();
/* We get the PC of the entry address - which is as good as anything,
on a real kernel what you get depends on which mode it uses. */
- info.si_signo = TARGET_SIGSEGV;
- info.si_errno = 0;
/* XXX: check env->error_code */
- info.si_code = TARGET_SEGV_MAPERR;
- info._sifields._sigfault._addr = env->exception.vaddress;
- queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
+ force_sig_fault(TARGET_SIGSEGV, TARGET_SEGV_MAPERR,
+ env->exception.vaddress);
}
/* Handle a jump to the kernel code page. */
@@ -284,8 +281,6 @@ void cpu_loop(CPUARMState *env)
CPUState *cs = env_cpu(env);
int trapnr;
unsigned int n, insn;
- target_siginfo_t info;
- uint32_t addr;
abi_ulong ret;
for(;;) {
@@ -320,11 +315,8 @@ void cpu_loop(CPUARMState *env)
break;
}
- info.si_signo = TARGET_SIGILL;
- info.si_errno = 0;
- info.si_code = TARGET_ILL_ILLOPN;
- info._sifields._sigfault._addr = env->regs[15];
- queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
+ force_sig_fault(TARGET_SIGILL, TARGET_ILL_ILLOPN,
+ env->regs[15]);
}
break;
case EXCP_SWI:
@@ -392,18 +384,14 @@ void cpu_loop(CPUARMState *env)
* Otherwise SIGILL. This includes any SWI with
* immediate not originally 0x9fxxxx, because
* of the earlier XOR.
+ * Like the real kernel, we report the addr of the
+ * SWI in the siginfo si_addr but leave the PC
+ * pointing at the insn after the SWI.
*/
- info.si_signo = TARGET_SIGILL;
- info.si_errno = 0;
- info.si_code = TARGET_ILL_ILLTRP;
- info._sifields._sigfault._addr = env->regs[15];
- if (env->thumb) {
- info._sifields._sigfault._addr -= 2;
- } else {
- info._sifields._sigfault._addr -= 4;
- }
- queue_signal(env, info.si_signo,
- QEMU_SI_FAULT, &info);
+ abi_ulong faultaddr = env->regs[15];
+ faultaddr -= env->thumb ? 2 : 4;
+ force_sig_fault(TARGET_SIGILL, TARGET_ILL_ILLTRP,
+ faultaddr);
}
break;
}
@@ -434,24 +422,14 @@ void cpu_loop(CPUARMState *env)
break;
case EXCP_PREFETCH_ABORT:
case EXCP_DATA_ABORT:
- addr = env->exception.vaddress;
- {
- info.si_signo = TARGET_SIGSEGV;
- info.si_errno = 0;
- /* XXX: check env->error_code */
- info.si_code = TARGET_SEGV_MAPERR;
- info._sifields._sigfault._addr = addr;
- queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
- }
+ /* XXX: check env->error_code */
+ force_sig_fault(TARGET_SIGSEGV, TARGET_SEGV_MAPERR,
+ env->exception.vaddress);
break;
case EXCP_DEBUG:
case EXCP_BKPT:
excp_debug:
- info.si_signo = TARGET_SIGTRAP;
- info.si_errno = 0;
- info.si_code = TARGET_TRAP_BRKPT;
- info._sifields._sigfault._addr = env->regs[15];
- queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
+ force_sig_fault(TARGET_SIGTRAP, TARGET_TRAP_BRKPT, env->regs[15]);
break;
case EXCP_KERNEL_TRAP:
if (do_kernel_trap(env))
--
2.20.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH for-6.2 6/7] linux-user/arm: Use force_sig_fault()
2021-08-13 13:18 ` [PATCH for-6.2 6/7] linux-user/arm: Use force_sig_fault() Peter Maydell
@ 2021-08-15 20:25 ` Richard Henderson
0 siblings, 0 replies; 18+ messages in thread
From: Richard Henderson @ 2021-08-15 20:25 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel; +Cc: Laurent Vivier
On 8/13/21 3:18 AM, Peter Maydell wrote:
> Use the new force_sig_fault() function instead of setting up
> a target_siginfo_t and calling queue_signal().
>
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> I threw in a comment confirming that the si_addr value for the "bad
> SWI immediate" SIGILL really is different from the PC value reported
> in the ucontext_t and resumed from if the handler returns, because it
> looked like a bug to me when I was reading the code...
> ---
> linux-user/arm/cpu_loop.c | 54 ++++++++++++---------------------------
> 1 file changed, 16 insertions(+), 38 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH for-6.2 7/7] linux-user/aarch64: Use force_sig_fault()
2021-08-13 13:18 [PATCH for-6.2 0/7] linux-user: Clean up siginfo_t handling for arm, aarch64 Peter Maydell
` (5 preceding siblings ...)
2021-08-13 13:18 ` [PATCH for-6.2 6/7] linux-user/arm: Use force_sig_fault() Peter Maydell
@ 2021-08-13 13:18 ` Peter Maydell
2021-08-15 20:29 ` Richard Henderson
2021-09-23 13:12 ` [PATCH for-6.2 0/7] linux-user: Clean up siginfo_t handling for arm, aarch64 Laurent Vivier
7 siblings, 1 reply; 18+ messages in thread
From: Peter Maydell @ 2021-08-13 13:18 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Laurent Vivier
Use the new force_sig_fault() function instead of setting up
a target_siginfo_t and calling queue_signal().
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
linux-user/aarch64/cpu_loop.c | 34 +++++++++-------------------------
1 file changed, 9 insertions(+), 25 deletions(-)
diff --git a/linux-user/aarch64/cpu_loop.c b/linux-user/aarch64/cpu_loop.c
index 5d8675944d9..11e34cb1007 100644
--- a/linux-user/aarch64/cpu_loop.c
+++ b/linux-user/aarch64/cpu_loop.c
@@ -21,6 +21,7 @@
#include "qemu-common.h"
#include "qemu.h"
#include "cpu_loop-common.h"
+#include "signal-common.h"
#include "qemu/guest-random.h"
#include "semihosting/common-semi.h"
#include "target/arm/syndrome.h"
@@ -77,9 +78,8 @@
void cpu_loop(CPUARMState *env)
{
CPUState *cs = env_cpu(env);
- int trapnr, ec, fsc;
+ int trapnr, ec, fsc, si_code;
abi_long ret;
- target_siginfo_t info;
for (;;) {
cpu_exec_start(cs);
@@ -108,18 +108,10 @@ void cpu_loop(CPUARMState *env)
/* just indicate that signals should be handled asap */
break;
case EXCP_UDEF:
- info.si_signo = TARGET_SIGILL;
- info.si_errno = 0;
- info.si_code = TARGET_ILL_ILLOPN;
- info._sifields._sigfault._addr = env->pc;
- queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
+ force_sig_fault(TARGET_SIGILL, TARGET_ILL_ILLOPN, env->pc);
break;
case EXCP_PREFETCH_ABORT:
case EXCP_DATA_ABORT:
- info.si_signo = TARGET_SIGSEGV;
- info.si_errno = 0;
- info._sifields._sigfault._addr = env->exception.vaddress;
-
/* We should only arrive here with EC in {DATAABORT, INSNABORT}. */
ec = syn_get_ec(env->exception.syndrome);
assert(ec == EC_DATAABORT || ec == EC_INSNABORT);
@@ -128,28 +120,24 @@ void cpu_loop(CPUARMState *env)
fsc = extract32(env->exception.syndrome, 0, 6);
switch (fsc) {
case 0x04 ... 0x07: /* Translation fault, level {0-3} */
- info.si_code = TARGET_SEGV_MAPERR;
+ si_code = TARGET_SEGV_MAPERR;
break;
case 0x09 ... 0x0b: /* Access flag fault, level {1-3} */
case 0x0d ... 0x0f: /* Permission fault, level {1-3} */
- info.si_code = TARGET_SEGV_ACCERR;
+ si_code = TARGET_SEGV_ACCERR;
break;
case 0x11: /* Synchronous Tag Check Fault */
- info.si_code = TARGET_SEGV_MTESERR;
+ si_code = TARGET_SEGV_MTESERR;
break;
default:
g_assert_not_reached();
}
- queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
+ force_sig_fault(TARGET_SIGSEGV, si_code, env->exception.vaddress);
break;
case EXCP_DEBUG:
case EXCP_BKPT:
- info.si_signo = TARGET_SIGTRAP;
- info.si_errno = 0;
- info.si_code = TARGET_TRAP_BRKPT;
- info._sifields._sigfault._addr = env->pc;
- queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
+ force_sig_fault(TARGET_SIGTRAP, TARGET_TRAP_BRKPT, env->pc);
break;
case EXCP_SEMIHOST:
env->xregs[0] = do_common_semihosting(cs);
@@ -169,11 +157,7 @@ void cpu_loop(CPUARMState *env)
/* Check for MTE asynchronous faults */
if (unlikely(env->cp15.tfsr_el[0])) {
env->cp15.tfsr_el[0] = 0;
- info.si_signo = TARGET_SIGSEGV;
- info.si_errno = 0;
- info._sifields._sigfault._addr = 0;
- info.si_code = TARGET_SEGV_MTEAERR;
- queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
+ force_sig_fault(TARGET_SIGSEGV, TARGET_SEGV_MTEAERR, 0);
}
process_pending_signals(env);
--
2.20.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH for-6.2 7/7] linux-user/aarch64: Use force_sig_fault()
2021-08-13 13:18 ` [PATCH for-6.2 7/7] linux-user/aarch64: " Peter Maydell
@ 2021-08-15 20:29 ` Richard Henderson
0 siblings, 0 replies; 18+ messages in thread
From: Richard Henderson @ 2021-08-15 20:29 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel; +Cc: Laurent Vivier
On 8/13/21 3:18 AM, Peter Maydell wrote:
> Use the new force_sig_fault() function instead of setting up
> a target_siginfo_t and calling queue_signal().
>
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> linux-user/aarch64/cpu_loop.c | 34 +++++++++-------------------------
> 1 file changed, 9 insertions(+), 25 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH for-6.2 0/7] linux-user: Clean up siginfo_t handling for arm, aarch64
2021-08-13 13:18 [PATCH for-6.2 0/7] linux-user: Clean up siginfo_t handling for arm, aarch64 Peter Maydell
` (6 preceding siblings ...)
2021-08-13 13:18 ` [PATCH for-6.2 7/7] linux-user/aarch64: " Peter Maydell
@ 2021-09-23 13:12 ` Laurent Vivier
7 siblings, 0 replies; 18+ messages in thread
From: Laurent Vivier @ 2021-09-23 13:12 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel
Le 13/08/2021 à 15:18, Peter Maydell a écrit :
> Coverity reported that we don't set the _sifields union when queuing
> the SIGTRAP for EXCP_DEBUG events on aarch64. This series fixes that
> bug and a few others, and cleans up the way we queue fault signals
> to be less error-prone.
>
> The underlying cause of the bug is that when queueing a signal
> the code must set the right field in the _sifields union depending
> on the si_type that it passes to queue_signal(), and there's nothing
> guarding against forgetting to do this. The "fill in a whole struct
> and then call queue_signal()" code is also a bit longwinded. In the
> real kernel, there is a function force_sig_fault() which does this
> for the SI_FAULT signal type. The series provides a QEMU implementation
> of this (which goes alongside the existing force_sig() that does this
> for SI_KILL signals), and uses it in the arm and aarch64 code.
> Because force_sig_fault() takes the address as an argument, it's
> not possible for the caller to forget to fill it in.
>
> Obviously we should also convert the other targets where they are
> directly calling queue_signal() (there are other places that forget
> to fill in the sifields union fields; I don't know why Coverity
> hasn't spotted those). But I thought it better to send this out
> for review of the new API and approach before spending time on
> converting other targets. (In particular fixing places where
> EXCP_DEBUG handling forgets to set the sifields address is a
> pain, because in the real kernel different architectures might
> either report the PC or else report a zero address here, so it
> requires looking into the kernel sources to check which...)
> Once all the archs are cleaned up we might be able to make
> queue_signal() static so it's local to signal.c.
>
> PS: there's probably a trivial conflict with my include-file-split
> patchseries.
>
> thanks
> -- PMM
>
> Peter Maydell (7):
> linux-user/aarch64: Set siginfo_t addr field for SIGTRAP signals
> linux-user/arm: Set siginfo_t addr field for SIGTRAP signals
> linux-user/arm: Use force_sig() to deliver fpa11 emulation SIGFPE
> linux-user: Zero out target_siginfo_t in force_sig()
> linux-user: Provide new force_sig_fault() function
> linux-user/arm: Use force_sig_fault()
> linux-user/aarch64: Use force_sig_fault()
>
> linux-user/signal-common.h | 1 +
> linux-user/aarch64/cpu_loop.c | 33 +++++-------------
> linux-user/arm/cpu_loop.c | 64 +++++++++++------------------------
> linux-user/signal.c | 19 ++++++++++-
> 4 files changed, 48 insertions(+), 69 deletions(-)
>
Applied to my linux-user-for-6.2 branch.
Thanks,
Laurent
^ permalink raw reply [flat|nested] 18+ messages in thread