[PULL 0/3] SIGSEGV fixes

[PULL 0/3] SIGSEGV fixes

[Taylor Simpson]

The following changes since commit 7457b407edd6e8555e4b46488aab2f13959fccf8:

  Merge remote-tracking branch 'remotes/thuth-gitlab/tags/pull-request-2021-07-19' into staging (2021-07-19 11:34:08 +0100)

are available in the git repository at:

  https://github.com/quic/qemu tags/pull-hex-20210721

for you to fetch changes up to 953ea3e4b426ee0c8349343c53e3358cfec720f2:

  linux-test (tests/tcg/multiarch/linux-test.c) add check (2021-07-21 15:54:28 -0500)

----------------------------------------------------------------
The Hexagon target was silently failing the SIGSEGV test because
the signal handler was not called.

Patch 1/3 fixes the Hexagon target
Patch 2/3 drops include qemu.h from target/hexagon/op_helper.c
Patch 3/3 adds a check that the signal handler is called

----------------------------------------------------------------
Peter Maydell (1):
      target/hexagon: Drop include of qemu.h

Taylor Simpson (2):
      Hexagon (target/hexagon) remove put_user_*/get_user_*
      linux-test (tests/tcg/multiarch/linux-test.c) add check

 target/hexagon/op_helper.c       | 42 +++++++++++++++++++---------------------
 tests/tcg/multiarch/linux-test.c |  8 ++++++++
 2 files changed, 28 insertions(+), 22 deletions(-)

[PULL 1/3] Hexagon (target/hexagon) remove put_user_*/get_user_*

[Taylor Simpson]

Replace put_user_* with cpu_st*_data_ra
Replace get_user_* with cpu_ld*_data_ra

Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
Message-Id: <1626384156-6248-2-git-send-email-tsimpson@quicinc.com>
---
 target/hexagon/op_helper.c | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
index 4595559..a959dba 100644
--- a/target/hexagon/op_helper.c
+++ b/target/hexagon/op_helper.c
@@ -17,6 +17,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu.h"
+#include "exec/cpu_ldst.h"
 #include "exec/helper-proto.h"
 #include "fpu/softfloat.h"
 #include "cpu.h"
@@ -140,22 +141,22 @@ void HELPER(debug_check_store_width)(CPUHexagonState *env, int slot, int check)
 
 void HELPER(commit_store)(CPUHexagonState *env, int slot_num)
 {
-    switch (env->mem_log_stores[slot_num].width) {
+    uintptr_t ra = GETPC();
+    uint8_t width = env->mem_log_stores[slot_num].width;
+    target_ulong va = env->mem_log_stores[slot_num].va;
+
+    switch (width) {
     case 1:
-        put_user_u8(env->mem_log_stores[slot_num].data32,
-                    env->mem_log_stores[slot_num].va);
+        cpu_stb_data_ra(env, va, env->mem_log_stores[slot_num].data32, ra);
         break;
     case 2:
-        put_user_u16(env->mem_log_stores[slot_num].data32,
-                     env->mem_log_stores[slot_num].va);
+        cpu_stw_data_ra(env, va, env->mem_log_stores[slot_num].data32, ra);
         break;
     case 4:
-        put_user_u32(env->mem_log_stores[slot_num].data32,
-                     env->mem_log_stores[slot_num].va);
+        cpu_stl_data_ra(env, va, env->mem_log_stores[slot_num].data32, ra);
         break;
     case 8:
-        put_user_u64(env->mem_log_stores[slot_num].data64,
-                     env->mem_log_stores[slot_num].va);
+        cpu_stq_data_ra(env, va, env->mem_log_stores[slot_num].data64, ra);
         break;
     default:
         g_assert_not_reached();
@@ -393,37 +394,33 @@ static void check_noshuf(CPUHexagonState *env, uint32_t slot)
 static uint8_t mem_load1(CPUHexagonState *env, uint32_t slot,
                          target_ulong vaddr)
 {
-    uint8_t retval;
+    uintptr_t ra = GETPC();
     check_noshuf(env, slot);
-    get_user_u8(retval, vaddr);
-    return retval;
+    return cpu_ldub_data_ra(env, vaddr, ra);
 }
 
 static uint16_t mem_load2(CPUHexagonState *env, uint32_t slot,
                           target_ulong vaddr)
 {
-    uint16_t retval;
+    uintptr_t ra = GETPC();
     check_noshuf(env, slot);
-    get_user_u16(retval, vaddr);
-    return retval;
+    return cpu_lduw_data_ra(env, vaddr, ra);
 }
 
 static uint32_t mem_load4(CPUHexagonState *env, uint32_t slot,
                           target_ulong vaddr)
 {
-    uint32_t retval;
+    uintptr_t ra = GETPC();
     check_noshuf(env, slot);
-    get_user_u32(retval, vaddr);
-    return retval;
+    return cpu_ldl_data_ra(env, vaddr, ra);
 }
 
 static uint64_t mem_load8(CPUHexagonState *env, uint32_t slot,
                           target_ulong vaddr)
 {
-    uint64_t retval;
+    uintptr_t ra = GETPC();
     check_noshuf(env, slot);
-    get_user_u64(retval, vaddr);
-    return retval;
+    return cpu_ldq_data_ra(env, vaddr, ra);
 }
 
 /* Floating point */
-- 
2.7.4

[PULL 2/3] target/hexagon: Drop include of qemu.h

[Taylor Simpson]

From: Peter Maydell <peter.maydell@linaro.org>

The qemu.h file is a CONFIG_USER_ONLY header; it doesn't appear on
the include path for softmmu builds.  Currently we include it
unconditionally in target/hexagon/op_helper.c.  We used to need it
for the put_user_*() and get_user_*() functions, but now that we have
removed the uses of those from op_helper.c, the only reason it's
still there is that we're implicitly relying on it pulling in some
other headers.

Explicitly include the headers we need for other functions, and drop
the include of qemu.h.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Message-Id: <20210717103017.20491-1-peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Taylor Simpson <tsimpson@quicinc.com>
Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
---
 target/hexagon/op_helper.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
index a959dba..61d5cde 100644
--- a/target/hexagon/op_helper.c
+++ b/target/hexagon/op_helper.c
@@ -16,7 +16,8 @@
  */
 
 #include "qemu/osdep.h"
-#include "qemu.h"
+#include "qemu/log.h"
+#include "exec/exec-all.h"
 #include "exec/cpu_ldst.h"
 #include "exec/helper-proto.h"
 #include "fpu/softfloat.h"
-- 
2.7.4

[PULL 3/3] linux-test (tests/tcg/multiarch/linux-test.c) add check

[Taylor Simpson]

Add a check that the SIGSEGV handler is called

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
Message-Id: <1626384156-6248-3-git-send-email-tsimpson@quicinc.com>
---
 tests/tcg/multiarch/linux-test.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tests/tcg/multiarch/linux-test.c b/tests/tcg/multiarch/linux-test.c
index c8c6aed..e2d88f8 100644
--- a/tests/tcg/multiarch/linux-test.c
+++ b/tests/tcg/multiarch/linux-test.c
@@ -439,10 +439,14 @@ static void sig_alarm(int sig)
     alarm_count++;
 }
 
+/* Count the number of times handler is called */
+static int sig_segv_called;
+
 static void sig_segv(int sig, siginfo_t *info, void *puc)
 {
     if (sig != SIGSEGV)
         error("signal");
+    sig_segv_called++;
     longjmp(jmp_env, 1);
 }
 
@@ -492,6 +496,10 @@ static void test_signal(void)
         *(volatile uint8_t *)0 = 0;
     }
 
+    if (sig_segv_called == 0) {
+        error("SIGSEGV handler not called");
+    }
+
     act.sa_handler = SIG_DFL;
     sigemptyset(&act.sa_mask);
     act.sa_flags = 0;
-- 
2.7.4

Re: [PULL 0/3] SIGSEGV fixes

[Peter Maydell]

On Wed, 21 Jul 2021 at 22:19, Taylor Simpson <tsimpson@quicinc.com> wrote:
>
> The following changes since commit 7457b407edd6e8555e4b46488aab2f13959fccf8:
>
>   Merge remote-tracking branch 'remotes/thuth-gitlab/tags/pull-request-2021-07-19' into staging (2021-07-19 11:34:08 +0100)
>
> are available in the git repository at:
>
>   https://github.com/quic/qemu tags/pull-hex-20210721
>
> for you to fetch changes up to 953ea3e4b426ee0c8349343c53e3358cfec720f2:
>
>   linux-test (tests/tcg/multiarch/linux-test.c) add check (2021-07-21 15:54:28 -0500)
>
> ----------------------------------------------------------------
> The Hexagon target was silently failing the SIGSEGV test because
> the signal handler was not called.
>
> Patch 1/3 fixes the Hexagon target
> Patch 2/3 drops include qemu.h from target/hexagon/op_helper.c
> Patch 3/3 adds a check that the signal handler is called
>
> ----------------------------------------------------------------

Hi; the check added in patch 2 seems to fire about 50% of the
time for qemu-riscv64, causing 'make check-tcg' to fail.

$ ./qemu-riscv64 ./tests/tcg/riscv64-linux-user/linux-test
$ ./qemu-riscv64 ./tests/tcg/riscv64-linux-user/linux-test
$ ./qemu-riscv64 ./tests/tcg/riscv64-linux-user/linux-test
$ ./qemu-riscv64 ./tests/tcg/riscv64-linux-user/linux-test
/mnt/nvmedisk/linaro/qemu-for-merges/tests/tcg/multiarch/linux-test.c:500:
SIGSEGV handler not called
$ ./qemu-riscv64 ./tests/tcg/riscv64-linux-user/linux-test
$ ./qemu-riscv64 ./tests/tcg/riscv64-linux-user/linux-test
/mnt/nvmedisk/linaro/qemu-for-merges/tests/tcg/multiarch/linux-test.c:500:
SIGSEGV handler not called
$ ./qemu-riscv64 ./tests/tcg/riscv64-linux-user/linux-test
/mnt/nvmedisk/linaro/qemu-for-merges/tests/tcg/multiarch/linux-test.c:500:
SIGSEGV handler not called


thanks
-- PMM

RE: [PULL 0/3] SIGSEGV fixes

[Taylor Simpson]

I've added the riscv maintainers and Laurent and Alex to the CC list.

Please advise on how to proceed.  Is this a known issue with riscv?  Should I try to debug the riscv target or remove the change to linux-test.c from the pull request?


Thanks,
Taylor

> -----Original Message-----
> From: Peter Maydell <peter.maydell@linaro.org>
> Sent: Thursday, July 22, 2021 12:31 PM
> To: Taylor Simpson <tsimpson@quicinc.com>
> Cc: QEMU Developers <qemu-devel@nongnu.org>; Richard Henderson
> <richard.henderson@linaro.org>; Philippe Mathieu-Daudé
> <philmd@redhat.com>; Alessandro Di Federico <ale@rev.ng>; Brian Cain
> <bcain@quicinc.com>
> Subject: Re: [PULL 0/3] SIGSEGV fixes
> 
> On Wed, 21 Jul 2021 at 22:19, Taylor Simpson <tsimpson@quicinc.com>
> wrote:
> >
> > The following changes since commit
> 7457b407edd6e8555e4b46488aab2f13959fccf8:
> >
> >   Merge remote-tracking branch
> > 'remotes/thuth-gitlab/tags/pull-request-2021-07-19' into staging
> > (2021-07-19 11:34:08 +0100)
> >
> > are available in the git repository at:
> >
> >   https://github.com/quic/qemu tags/pull-hex-20210721
> >
> > for you to fetch changes up to
> 953ea3e4b426ee0c8349343c53e3358cfec720f2:
> >
> >   linux-test (tests/tcg/multiarch/linux-test.c) add check (2021-07-21
> > 15:54:28 -0500)
> >
> > ----------------------------------------------------------------
> > The Hexagon target was silently failing the SIGSEGV test because the
> > signal handler was not called.
> >
> > Patch 1/3 fixes the Hexagon target
> > Patch 2/3 drops include qemu.h from target/hexagon/op_helper.c Patch
> > 3/3 adds a check that the signal handler is called
> >
> > ----------------------------------------------------------------
> 
> Hi; the check added in patch 2 seems to fire about 50% of the time for qemu-
> riscv64, causing 'make check-tcg' to fail.
> 
> $ ./qemu-riscv64 ./tests/tcg/riscv64-linux-user/linux-test
> $ ./qemu-riscv64 ./tests/tcg/riscv64-linux-user/linux-test
> $ ./qemu-riscv64 ./tests/tcg/riscv64-linux-user/linux-test
> $ ./qemu-riscv64 ./tests/tcg/riscv64-linux-user/linux-test
> /mnt/nvmedisk/linaro/qemu-for-merges/tests/tcg/multiarch/linux-
> test.c:500:
> SIGSEGV handler not called
> $ ./qemu-riscv64 ./tests/tcg/riscv64-linux-user/linux-test
> $ ./qemu-riscv64 ./tests/tcg/riscv64-linux-user/linux-test
> /mnt/nvmedisk/linaro/qemu-for-merges/tests/tcg/multiarch/linux-
> test.c:500:
> SIGSEGV handler not called
> $ ./qemu-riscv64 ./tests/tcg/riscv64-linux-user/linux-test
> /mnt/nvmedisk/linaro/qemu-for-merges/tests/tcg/multiarch/linux-
> test.c:500:
> SIGSEGV handler not called

Re: [PULL 0/3] SIGSEGV fixes

[Richard Henderson]

On 7/23/21 8:48 AM, Taylor Simpson wrote:
> I've added the riscv maintainers and Laurent and Alex to the CC list.
> 
> Please advise on how to proceed.  Is this a known issue with riscv?  Should I try to debug the riscv target or remove the change to linux-test.c from the pull request?

Remove the linux-test.c from the pull request.

Someone will need to debug riscv, but I don't think you want that to block the bug fix for 
hexagon.


r~


PS: Please use some subject-line indicator for what component you're patching.  "SIGSEGV 
fixes" is impossibly generic.

Re: [PULL 0/3] SIGSEGV fixes

[Philippe Mathieu-Daudé]

On 7/23/21 9:10 PM, Richard Henderson wrote:
> On 7/23/21 8:48 AM, Taylor Simpson wrote:
>> I've added the riscv maintainers and Laurent and Alex to the CC list.
>>
>> Please advise on how to proceed.  Is this a known issue with riscv? 
>> Should I try to debug the riscv target or remove the change to
>> linux-test.c from the pull request?
> 
> Remove the linux-test.c from the pull request.
> 
> Someone will need to debug riscv, but I don't think you want that to
> block the bug fix for hexagon.
> 
> 
> r~
> 
> 
> PS: Please use some subject-line indicator for what component you're
> patching.  "SIGSEGV fixes" is impossibly generic.

Although more generic is possible: "fixes"