* [PATCH 1/2] target/s390x: Fix SIGILL psw.addr reporting
2021-05-21 3:01 [PATCH 0/2] Fix SIGILL psw.addr reporting Ilya Leoshkevich
@ 2021-05-21 3:01 ` Ilya Leoshkevich
2021-05-21 7:49 ` David Hildenbrand
2021-05-21 3:01 ` [PATCH 2/2] tests/tcg/s390x: Test SIGILL handling Ilya Leoshkevich
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Ilya Leoshkevich @ 2021-05-21 3:01 UTC (permalink / raw)
To: Richard Henderson, David Hildenbrand, Laurent Vivier
Cc: Christian Borntraeger, qemu-s390x, qemu-devel, Ilya Leoshkevich,
Andreas Krebbel
When a s390x CPU attempts to execute an illegal instruction, an
operation exception is recognized. This is a suppressing exception,
which means that the PSW is advanced by the length of the illegal
instruction.
On the real hardware or in qemu-system-s390x the kernel then raises
SIGILL with si_addr pointing to the suppressed instruction and
psw.addr containing the updated PSW.
Unfortunately qemu-s390x sets both to the address of the suppressed
instruction at the moment. Fix by sharing the PSW advancement logic
with qemu-system-s390x and setting si_addr to the address of the
instruction that raised the exception.
Buglink: https://bugs.launchpad.net/qemu/+bug/1920913
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
linux-user/s390x/cpu_loop.c | 6 +++-
target/s390x/excp_helper.c | 69 ++++++++++++++++++++-----------------
target/s390x/internal.h | 1 +
3 files changed, 43 insertions(+), 33 deletions(-)
diff --git a/linux-user/s390x/cpu_loop.c b/linux-user/s390x/cpu_loop.c
index f2d1215fb1..6f5462d4f8 100644
--- a/linux-user/s390x/cpu_loop.c
+++ b/linux-user/s390x/cpu_loop.c
@@ -21,6 +21,7 @@
#include "qemu-common.h"
#include "qemu.h"
#include "cpu_loop-common.h"
+#include "internal.h"
/* s390x masks the fault address it reports in si_addr for SIGSEGV and SIGBUS */
#define S390X_FAIL_ADDR_MASK -4096LL
@@ -29,6 +30,7 @@ void cpu_loop(CPUS390XState *env)
{
CPUState *cs = env_cpu(env);
int trapnr, n, sig;
+ target_ulong excp_psw_addr;
target_siginfo_t info;
target_ulong addr;
abi_long ret;
@@ -38,6 +40,7 @@ void cpu_loop(CPUS390XState *env)
trapnr = cpu_exec(cs);
cpu_exec_end(cs);
process_queued_cpu_work(cs);
+ excp_psw_addr = env->psw.addr;
switch (trapnr) {
case EXCP_INTERRUPT:
@@ -66,6 +69,7 @@ void cpu_loop(CPUS390XState *env)
n = TARGET_TRAP_BRKPT;
goto do_signal_pc;
case EXCP_PGM:
+ s390_cpu_program_interrupt_advance_psw(env);
n = env->int_pgm_code;
switch (n) {
case PGM_OPERATION:
@@ -131,7 +135,7 @@ void cpu_loop(CPUS390XState *env)
break;
do_signal_pc:
- addr = env->psw.addr;
+ addr = excp_psw_addr;
do_signal:
info.si_signo = sig;
info.si_errno = 0;
diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
index 20625c2c8f..0a323967ae 100644
--- a/target/s390x/excp_helper.c
+++ b/target/s390x/excp_helper.c
@@ -82,6 +82,42 @@ void HELPER(data_exception)(CPUS390XState *env, uint32_t dxc)
tcg_s390_data_exception(env, dxc, GETPC());
}
+void s390_cpu_program_interrupt_advance_psw(CPUS390XState *env)
+{
+ switch (env->int_pgm_code) {
+ case PGM_PER:
+ if (env->per_perc_atmid & PER_CODE_EVENT_NULLIFICATION) {
+ break;
+ }
+ /* FALL THROUGH */
+ case PGM_OPERATION:
+ case PGM_PRIVILEGED:
+ case PGM_EXECUTE:
+ case PGM_PROTECTION:
+ case PGM_ADDRESSING:
+ case PGM_SPECIFICATION:
+ case PGM_DATA:
+ case PGM_FIXPT_OVERFLOW:
+ case PGM_FIXPT_DIVIDE:
+ case PGM_DEC_OVERFLOW:
+ case PGM_DEC_DIVIDE:
+ case PGM_HFP_EXP_OVERFLOW:
+ case PGM_HFP_EXP_UNDERFLOW:
+ case PGM_HFP_SIGNIFICANCE:
+ case PGM_HFP_DIVIDE:
+ case PGM_TRANS_SPEC:
+ case PGM_SPECIAL_OP:
+ case PGM_OPERAND:
+ case PGM_HFP_SQRT:
+ case PGM_PC_TRANS_SPEC:
+ case PGM_ALET_SPEC:
+ case PGM_MONITOR:
+ /* advance the PSW if our exception is not nullifying */
+ env->psw.addr += env->int_pgm_ilen;
+ break;
+ }
+}
+
#if defined(CONFIG_USER_ONLY)
void s390_cpu_do_interrupt(CPUState *cs)
@@ -202,38 +238,7 @@ static void do_program_interrupt(CPUS390XState *env)
assert(ilen == 2 || ilen == 4 || ilen == 6);
- switch (env->int_pgm_code) {
- case PGM_PER:
- if (env->per_perc_atmid & PER_CODE_EVENT_NULLIFICATION) {
- break;
- }
- /* FALL THROUGH */
- case PGM_OPERATION:
- case PGM_PRIVILEGED:
- case PGM_EXECUTE:
- case PGM_PROTECTION:
- case PGM_ADDRESSING:
- case PGM_SPECIFICATION:
- case PGM_DATA:
- case PGM_FIXPT_OVERFLOW:
- case PGM_FIXPT_DIVIDE:
- case PGM_DEC_OVERFLOW:
- case PGM_DEC_DIVIDE:
- case PGM_HFP_EXP_OVERFLOW:
- case PGM_HFP_EXP_UNDERFLOW:
- case PGM_HFP_SIGNIFICANCE:
- case PGM_HFP_DIVIDE:
- case PGM_TRANS_SPEC:
- case PGM_SPECIAL_OP:
- case PGM_OPERAND:
- case PGM_HFP_SQRT:
- case PGM_PC_TRANS_SPEC:
- case PGM_ALET_SPEC:
- case PGM_MONITOR:
- /* advance the PSW if our exception is not nullifying */
- env->psw.addr += ilen;
- break;
- }
+ s390_cpu_program_interrupt_advance_psw(env);
qemu_log_mask(CPU_LOG_INT,
"%s: code=0x%x ilen=%d psw: %" PRIx64 " %" PRIx64 "\n",
diff --git a/target/s390x/internal.h b/target/s390x/internal.h
index 11515bb617..9f1665ccbf 100644
--- a/target/s390x/internal.h
+++ b/target/s390x/internal.h
@@ -272,6 +272,7 @@ bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
void s390x_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
MMUAccessType access_type,
int mmu_idx, uintptr_t retaddr);
+void s390_cpu_program_interrupt_advance_psw(CPUS390XState *cpu);
/* fpu_helper.c */
--
2.31.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 1/2] target/s390x: Fix SIGILL psw.addr reporting
2021-05-21 3:01 ` [PATCH 1/2] target/s390x: " Ilya Leoshkevich
@ 2021-05-21 7:49 ` David Hildenbrand
0 siblings, 0 replies; 10+ messages in thread
From: David Hildenbrand @ 2021-05-21 7:49 UTC (permalink / raw)
To: Ilya Leoshkevich, Richard Henderson, Laurent Vivier
Cc: Christian Borntraeger, qemu-s390x, qemu-devel, Andreas Krebbel
On 21.05.21 05:01, Ilya Leoshkevich wrote:
> When a s390x CPU attempts to execute an illegal instruction, an
> operation exception is recognized. This is a suppressing exception,
> which means that the PSW is advanced by the length of the illegal
> instruction.
>
> On the real hardware or in qemu-system-s390x the kernel then raises
> SIGILL with si_addr pointing to the suppressed instruction and
> psw.addr containing the updated PSW.
>
> Unfortunately qemu-s390x sets both to the address of the suppressed
> instruction at the moment. Fix by sharing the PSW advancement logic
> with qemu-system-s390x and setting si_addr to the address of the
> instruction that raised the exception.
>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1920913
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
> linux-user/s390x/cpu_loop.c | 6 +++-
> target/s390x/excp_helper.c | 69 ++++++++++++++++++++-----------------
> target/s390x/internal.h | 1 +
> 3 files changed, 43 insertions(+), 33 deletions(-)
>
> diff --git a/linux-user/s390x/cpu_loop.c b/linux-user/s390x/cpu_loop.c
> index f2d1215fb1..6f5462d4f8 100644
> --- a/linux-user/s390x/cpu_loop.c
> +++ b/linux-user/s390x/cpu_loop.c
> @@ -21,6 +21,7 @@
> #include "qemu-common.h"
> #include "qemu.h"
> #include "cpu_loop-common.h"
> +#include "internal.h"
>
> /* s390x masks the fault address it reports in si_addr for SIGSEGV and SIGBUS */
> #define S390X_FAIL_ADDR_MASK -4096LL
> @@ -29,6 +30,7 @@ void cpu_loop(CPUS390XState *env)
> {
> CPUState *cs = env_cpu(env);
> int trapnr, n, sig;
> + target_ulong excp_psw_addr;
> target_siginfo_t info;
> target_ulong addr;
> abi_long ret;
> @@ -38,6 +40,7 @@ void cpu_loop(CPUS390XState *env)
> trapnr = cpu_exec(cs);
> cpu_exec_end(cs);
> process_queued_cpu_work(cs);
> + excp_psw_addr = env->psw.addr;
>
> switch (trapnr) {
> case EXCP_INTERRUPT:
> @@ -66,6 +69,7 @@ void cpu_loop(CPUS390XState *env)
> n = TARGET_TRAP_BRKPT;
> goto do_signal_pc;
> case EXCP_PGM:
> + s390_cpu_program_interrupt_advance_psw(env);
> n = env->int_pgm_code;
> switch (n) {
> case PGM_OPERATION:
> @@ -131,7 +135,7 @@ void cpu_loop(CPUS390XState *env)
> break;
>
> do_signal_pc:
> - addr = env->psw.addr;
> + addr = excp_psw_addr;
> do_signal:
> info.si_signo = sig;
> info.si_errno = 0;
> diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
> index 20625c2c8f..0a323967ae 100644
> --- a/target/s390x/excp_helper.c
> +++ b/target/s390x/excp_helper.c
> @@ -82,6 +82,42 @@ void HELPER(data_exception)(CPUS390XState *env, uint32_t dxc)
> tcg_s390_data_exception(env, dxc, GETPC());
> }
>
> +void s390_cpu_program_interrupt_advance_psw(CPUS390XState *env)
> +{
> + switch (env->int_pgm_code) {
> + case PGM_PER:
> + if (env->per_perc_atmid & PER_CODE_EVENT_NULLIFICATION) {
> + break;
> + }
> + /* FALL THROUGH */
> + case PGM_OPERATION:
> + case PGM_PRIVILEGED:
> + case PGM_EXECUTE:
> + case PGM_PROTECTION:
> + case PGM_ADDRESSING:
> + case PGM_SPECIFICATION:
> + case PGM_DATA:
> + case PGM_FIXPT_OVERFLOW:
> + case PGM_FIXPT_DIVIDE:
> + case PGM_DEC_OVERFLOW:
> + case PGM_DEC_DIVIDE:
> + case PGM_HFP_EXP_OVERFLOW:
> + case PGM_HFP_EXP_UNDERFLOW:
> + case PGM_HFP_SIGNIFICANCE:
> + case PGM_HFP_DIVIDE:
> + case PGM_TRANS_SPEC:
> + case PGM_SPECIAL_OP:
> + case PGM_OPERAND:
> + case PGM_HFP_SQRT:
> + case PGM_PC_TRANS_SPEC:
> + case PGM_ALET_SPEC:
> + case PGM_MONITOR:
> + /* advance the PSW if our exception is not nullifying */
> + env->psw.addr += env->int_pgm_ilen;
> + break;
> + }
> +}
> +
> #if defined(CONFIG_USER_ONLY)
>
> void s390_cpu_do_interrupt(CPUState *cs)
> @@ -202,38 +238,7 @@ static void do_program_interrupt(CPUS390XState *env)
>
> assert(ilen == 2 || ilen == 4 || ilen == 6);
>
> - switch (env->int_pgm_code) {
> - case PGM_PER:
> - if (env->per_perc_atmid & PER_CODE_EVENT_NULLIFICATION) {
> - break;
> - }
> - /* FALL THROUGH */
> - case PGM_OPERATION:
> - case PGM_PRIVILEGED:
> - case PGM_EXECUTE:
> - case PGM_PROTECTION:
> - case PGM_ADDRESSING:
> - case PGM_SPECIFICATION:
> - case PGM_DATA:
> - case PGM_FIXPT_OVERFLOW:
> - case PGM_FIXPT_DIVIDE:
> - case PGM_DEC_OVERFLOW:
> - case PGM_DEC_DIVIDE:
> - case PGM_HFP_EXP_OVERFLOW:
> - case PGM_HFP_EXP_UNDERFLOW:
> - case PGM_HFP_SIGNIFICANCE:
> - case PGM_HFP_DIVIDE:
> - case PGM_TRANS_SPEC:
> - case PGM_SPECIAL_OP:
> - case PGM_OPERAND:
> - case PGM_HFP_SQRT:
> - case PGM_PC_TRANS_SPEC:
> - case PGM_ALET_SPEC:
> - case PGM_MONITOR:
> - /* advance the PSW if our exception is not nullifying */
> - env->psw.addr += ilen;
> - break;
> - }
> + s390_cpu_program_interrupt_advance_psw(env);
>
> qemu_log_mask(CPU_LOG_INT,
> "%s: code=0x%x ilen=%d psw: %" PRIx64 " %" PRIx64 "\n",
> diff --git a/target/s390x/internal.h b/target/s390x/internal.h
> index 11515bb617..9f1665ccbf 100644
> --- a/target/s390x/internal.h
> +++ b/target/s390x/internal.h
> @@ -272,6 +272,7 @@ bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> void s390x_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
> MMUAccessType access_type,
> int mmu_idx, uintptr_t retaddr);
> +void s390_cpu_program_interrupt_advance_psw(CPUS390XState *cpu);
>
>
> /* fpu_helper.c */
>
LGTM, thanks
Reviewed-by: David Hildenbrand <david@redhat.com>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] tests/tcg/s390x: Test SIGILL handling
2021-05-21 3:01 [PATCH 0/2] Fix SIGILL psw.addr reporting Ilya Leoshkevich
2021-05-21 3:01 ` [PATCH 1/2] target/s390x: " Ilya Leoshkevich
@ 2021-05-21 3:01 ` Ilya Leoshkevich
2021-05-21 7:54 ` David Hildenbrand
2021-05-21 3:09 ` [PATCH 0/2] Fix SIGILL psw.addr reporting no-reply
2021-05-21 7:42 ` David Hildenbrand
3 siblings, 1 reply; 10+ messages in thread
From: Ilya Leoshkevich @ 2021-05-21 3:01 UTC (permalink / raw)
To: Richard Henderson, David Hildenbrand, Laurent Vivier
Cc: Christian Borntraeger, qemu-s390x, qemu-devel, Ilya Leoshkevich,
Andreas Krebbel
Verify that s390x-specific uc_mcontext.psw.addr is reported correctly.
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
tests/tcg/s390x/Makefile.target | 1 +
tests/tcg/s390x/sigill.c | 41 +++++++++++++++++++++++++++++++++
2 files changed, 42 insertions(+)
create mode 100644 tests/tcg/s390x/sigill.c
diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
index 241ef28f61..8699d829a5 100644
--- a/tests/tcg/s390x/Makefile.target
+++ b/tests/tcg/s390x/Makefile.target
@@ -8,3 +8,4 @@ TESTS+=exrl-trtr
TESTS+=pack
TESTS+=mvo
TESTS+=mvc
+TESTS+=sigill
diff --git a/tests/tcg/s390x/sigill.c b/tests/tcg/s390x/sigill.c
new file mode 100644
index 0000000000..f8021dc6af
--- /dev/null
+++ b/tests/tcg/s390x/sigill.c
@@ -0,0 +1,41 @@
+#include <assert.h>
+#include <signal.h>
+#include <string.h>
+#include <ucontext.h>
+#include <unistd.h>
+
+extern char expected_si_addr[];
+extern char expected_psw_addr[];
+
+static void handle_signal(int sig, siginfo_t *info, void *ucontext)
+{
+ if (sig != SIGILL) {
+ _exit(1);
+ }
+
+ if (info->si_addr != expected_si_addr) {
+ _exit(2);
+ }
+
+ if (((ucontext_t *)ucontext)->uc_mcontext.psw.addr !=
+ (unsigned long)expected_psw_addr) {
+ _exit(3);
+ }
+}
+
+int main(void)
+{
+ struct sigaction act;
+
+ memset(&act, 0, sizeof(act));
+ act.sa_sigaction = handle_signal;
+ act.sa_flags = SA_SIGINFO;
+
+ int ret = sigaction(SIGILL, &act, NULL);
+ assert(ret == 0);
+
+ asm volatile("expected_si_addr:\t.byte\t0x00,0x00\n"
+ "expected_psw_addr:");
+
+ return 0;
+}
--
2.31.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 2/2] tests/tcg/s390x: Test SIGILL handling
2021-05-21 3:01 ` [PATCH 2/2] tests/tcg/s390x: Test SIGILL handling Ilya Leoshkevich
@ 2021-05-21 7:54 ` David Hildenbrand
2021-05-21 10:42 ` Ilya Leoshkevich
0 siblings, 1 reply; 10+ messages in thread
From: David Hildenbrand @ 2021-05-21 7:54 UTC (permalink / raw)
To: Ilya Leoshkevich, Richard Henderson, Laurent Vivier
Cc: Christian Borntraeger, qemu-s390x, qemu-devel, Andreas Krebbel
On 21.05.21 05:01, Ilya Leoshkevich wrote:
> Verify that s390x-specific uc_mcontext.psw.addr is reported correctly.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
> tests/tcg/s390x/Makefile.target | 1 +
> tests/tcg/s390x/sigill.c | 41 +++++++++++++++++++++++++++++++++
> 2 files changed, 42 insertions(+)
> create mode 100644 tests/tcg/s390x/sigill.c
>
> diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
> index 241ef28f61..8699d829a5 100644
> --- a/tests/tcg/s390x/Makefile.target
> +++ b/tests/tcg/s390x/Makefile.target
> @@ -8,3 +8,4 @@ TESTS+=exrl-trtr
> TESTS+=pack
> TESTS+=mvo
> TESTS+=mvc
> +TESTS+=sigill
> diff --git a/tests/tcg/s390x/sigill.c b/tests/tcg/s390x/sigill.c
> new file mode 100644
> index 0000000000..f8021dc6af
> --- /dev/null
> +++ b/tests/tcg/s390x/sigill.c
> @@ -0,0 +1,41 @@
> +#include <assert.h>
> +#include <signal.h>
> +#include <string.h>
> +#include <ucontext.h>
> +#include <unistd.h>
> +
> +extern char expected_si_addr[];
> +extern char expected_psw_addr[];
Why "extern" ? For the magic inline asm below to work?
> +
> +static void handle_signal(int sig, siginfo_t *info, void *ucontext)
> +{
> + if (sig != SIGILL) {
> + _exit(1);
> + }
> +
> + if (info->si_addr != expected_si_addr) {
> + _exit(2);
> + }
> +
> + if (((ucontext_t *)ucontext)->uc_mcontext.psw.addr !=
> + (unsigned long)expected_psw_addr) {
> + _exit(3);
> + }
> +}
> +
> +int main(void)
> +{
> + struct sigaction act;
> +
> + memset(&act, 0, sizeof(act));
> + act.sa_sigaction = handle_signal;
> + act.sa_flags = SA_SIGINFO;
> +
> + int ret = sigaction(SIGILL, &act, NULL);
Mixing code and declaration.
> + assert(ret == 0);
> +
> + asm volatile("expected_si_addr:\t.byte\t0x00,0x00\n"
> + "expected_psw_addr:");
At least I am confused how the right values actually end up in
expected_si_addr and expected_psw_addr.
Can we maybe add a comment? This looks quite hacky ;)
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 2/2] tests/tcg/s390x: Test SIGILL handling
2021-05-21 7:54 ` David Hildenbrand
@ 2021-05-21 10:42 ` Ilya Leoshkevich
0 siblings, 0 replies; 10+ messages in thread
From: Ilya Leoshkevich @ 2021-05-21 10:42 UTC (permalink / raw)
To: David Hildenbrand, Richard Henderson, Laurent Vivier
Cc: Christian Borntraeger, qemu-s390x, qemu-devel, Andreas Krebbel
On Fri, 2021-05-21 at 09:54 +0200, David Hildenbrand wrote:
> On 21.05.21 05:01, Ilya Leoshkevich wrote:
> > Verify that s390x-specific uc_mcontext.psw.addr is reported
> > correctly.
> >
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> > tests/tcg/s390x/Makefile.target | 1 +
> > tests/tcg/s390x/sigill.c | 41
> > +++++++++++++++++++++++++++++++++
> > 2 files changed, 42 insertions(+)
> > create mode 100644 tests/tcg/s390x/sigill.c
> >
> > diff --git a/tests/tcg/s390x/Makefile.target
> > b/tests/tcg/s390x/Makefile.target
> > index 241ef28f61..8699d829a5 100644
> > --- a/tests/tcg/s390x/Makefile.target
> > +++ b/tests/tcg/s390x/Makefile.target
> > @@ -8,3 +8,4 @@ TESTS+=exrl-trtr
> > TESTS+=pack
> > TESTS+=mvo
> > TESTS+=mvc
> > +TESTS+=sigill
> > diff --git a/tests/tcg/s390x/sigill.c b/tests/tcg/s390x/sigill.c
> > new file mode 100644
> > index 0000000000..f8021dc6af
> > --- /dev/null
> > +++ b/tests/tcg/s390x/sigill.c
> > @@ -0,0 +1,41 @@
> > +#include <assert.h>
> > +#include <signal.h>
> > +#include <string.h>
> > +#include <ucontext.h>
> > +#include <unistd.h>
> > +
> > +extern char expected_si_addr[];
> > +extern char expected_psw_addr[];
>
> Why "extern" ? For the magic inline asm below to work?
Yes - it cannot be static, because AFAIK there is no such thing as
static variable declaration (one can only define static variables).
> > +
> > +static void handle_signal(int sig, siginfo_t *info, void
> > *ucontext)
> > +{
> > + if (sig != SIGILL) {
> > + _exit(1);
> > + }
> > +
> > + if (info->si_addr != expected_si_addr) {
> > + _exit(2);
> > + }
> > +
> > + if (((ucontext_t *)ucontext)->uc_mcontext.psw.addr !=
> > + (unsigned long)expected_psw_addr) {
> > + _exit(3);
> > + }
> > +}
> > +
> > +int main(void)
> > +{
> > + struct sigaction act;
> > +
> > + memset(&act, 0, sizeof(act));
> > + act.sa_sigaction = handle_signal;
> > + act.sa_flags = SA_SIGINFO;
> > +
> > + int ret = sigaction(SIGILL, &act, NULL);
>
> Mixing code and declaration.
Ouch, will fix.
> > + assert(ret == 0);
> > +
> > + asm volatile("expected_si_addr:\t.byte\t0x00,0x00\n"
> > + "expected_psw_addr:");
>
> At least I am confused how the right values actually end up in
> expected_si_addr and expected_psw_addr.
>
> Can we maybe add a comment? This looks quite hacky ;)
This whole construction is roughly the same as having sigill.s file
with:
.globl expected_si_addr
expected_si_addr: .byte 0,0
.globl expected_psw_addr
expected_psw_addr: br 14
and sigill.h file with:
void expected_si_addr(void);
extern char expected_psw_addr[];
Doing it this way would complicate the build, so I thought it would be
better to just put everything into a single file.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] Fix SIGILL psw.addr reporting
2021-05-21 3:01 [PATCH 0/2] Fix SIGILL psw.addr reporting Ilya Leoshkevich
2021-05-21 3:01 ` [PATCH 1/2] target/s390x: " Ilya Leoshkevich
2021-05-21 3:01 ` [PATCH 2/2] tests/tcg/s390x: Test SIGILL handling Ilya Leoshkevich
@ 2021-05-21 3:09 ` no-reply
2021-05-21 7:42 ` David Hildenbrand
3 siblings, 0 replies; 10+ messages in thread
From: no-reply @ 2021-05-21 3:09 UTC (permalink / raw)
To: iii
Cc: iii, david, richard.henderson, qemu-devel, laurent, borntraeger,
qemu-s390x, krebbel
Patchew URL: https://patchew.org/QEMU/20210521030146.2831663-1-iii@linux.ibm.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20210521030146.2831663-1-iii@linux.ibm.com
Subject: [PATCH 0/2] Fix SIGILL psw.addr reporting
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
* [new tag] patchew/20210521030146.2831663-1-iii@linux.ibm.com -> patchew/20210521030146.2831663-1-iii@linux.ibm.com
Switched to a new branch 'test'
069d02b tests/tcg/s390x: Test SIGILL handling
a869643 target/s390x: Fix SIGILL psw.addr reporting
=== OUTPUT BEGIN ===
1/2 Checking commit a86964364d4d (target/s390x: Fix SIGILL psw.addr reporting)
2/2 Checking commit 069d02b0edf2 (tests/tcg/s390x: Test SIGILL handling)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#26:
new file mode 100644
ERROR: externs should be avoided in .c files
#37: FILE: tests/tcg/s390x/sigill.c:7:
+extern char expected_si_addr[];
ERROR: externs should be avoided in .c files
#38: FILE: tests/tcg/s390x/sigill.c:8:
+extern char expected_psw_addr[];
total: 2 errors, 1 warnings, 45 lines checked
Patch 2/2 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20210521030146.2831663-1-iii@linux.ibm.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] Fix SIGILL psw.addr reporting
2021-05-21 3:01 [PATCH 0/2] Fix SIGILL psw.addr reporting Ilya Leoshkevich
` (2 preceding siblings ...)
2021-05-21 3:09 ` [PATCH 0/2] Fix SIGILL psw.addr reporting no-reply
@ 2021-05-21 7:42 ` David Hildenbrand
2021-05-21 10:45 ` Ilya Leoshkevich
3 siblings, 1 reply; 10+ messages in thread
From: David Hildenbrand @ 2021-05-21 7:42 UTC (permalink / raw)
To: Ilya Leoshkevich, Richard Henderson, Laurent Vivier
Cc: Christian Borntraeger, qemu-s390x, qemu-devel, Andreas Krebbel
On 21.05.21 05:01, Ilya Leoshkevich wrote:
> qemu-s390x puts a wrong value into SIGILL's siginfo_t's psw.addr: it
> should be a pointer to the instruction following the illegal
> instruction, but at the moment it is a pointer to the illegal
> instruction itself. This breaks OpenJDK, which relies on this value.
>
> Patch 1 fixes the issue, patch 2 adds a test.
I assume that should fix
https://bugs.launchpad.net/qemu/+bug/1920913
right?
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 0/2] Fix SIGILL psw.addr reporting
2021-05-21 7:42 ` David Hildenbrand
@ 2021-05-21 10:45 ` Ilya Leoshkevich
2021-05-21 10:59 ` Cornelia Huck
0 siblings, 1 reply; 10+ messages in thread
From: Ilya Leoshkevich @ 2021-05-21 10:45 UTC (permalink / raw)
To: David Hildenbrand, Richard Henderson, Laurent Vivier
Cc: Christian Borntraeger, qemu-s390x, qemu-devel, Andreas Krebbel
On Fri, 2021-05-21 at 09:42 +0200, David Hildenbrand wrote:
> On 21.05.21 05:01, Ilya Leoshkevich wrote:
> > qemu-s390x puts a wrong value into SIGILL's siginfo_t's psw.addr:
> > it
> > should be a pointer to the instruction following the illegal
> > instruction, but at the moment it is a pointer to the illegal
> > instruction itself. This breaks OpenJDK, which relies on this
> > value.
> >
> > Patch 1 fixes the issue, patch 2 adds a test.
>
> I assume that should fix
>
> https://bugs.launchpad.net/qemu/+bug/1920913
>
> right?
Yes, I have this Buglink in the fix commit message.
I have to admit I did not test Java more extensively - there might be
more things going on - but at least with this the SIGILL on startup is
gone, and very simple programs work.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] Fix SIGILL psw.addr reporting
2021-05-21 10:45 ` Ilya Leoshkevich
@ 2021-05-21 10:59 ` Cornelia Huck
0 siblings, 0 replies; 10+ messages in thread
From: Cornelia Huck @ 2021-05-21 10:59 UTC (permalink / raw)
To: Ilya Leoshkevich
Cc: David Hildenbrand, Richard Henderson, qemu-devel, Laurent Vivier,
Christian Borntraeger, qemu-s390x, Andreas Krebbel
On Fri, 21 May 2021 12:45:30 +0200
Ilya Leoshkevich <iii@linux.ibm.com> wrote:
> On Fri, 2021-05-21 at 09:42 +0200, David Hildenbrand wrote:
> > On 21.05.21 05:01, Ilya Leoshkevich wrote:
> > > qemu-s390x puts a wrong value into SIGILL's siginfo_t's psw.addr:
> > > it
> > > should be a pointer to the instruction following the illegal
> > > instruction, but at the moment it is a pointer to the illegal
> > > instruction itself. This breaks OpenJDK, which relies on this
> > > value.
> > >
> > > Patch 1 fixes the issue, patch 2 adds a test.
> >
> > I assume that should fix
> >
> > https://bugs.launchpad.net/qemu/+bug/1920913
> >
> > right?
>
> Yes, I have this Buglink in the fix commit message.
Better add https://gitlab.com/qemu-project/qemu/-/issues/319 as the
Buglink :)
> I have to admit I did not test Java more extensively - there might be
> more things going on - but at least with this the SIGILL on startup is
> gone, and very simple programs work.
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread