Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IBM Z (aka SystemZ / SysZ / s390x) architecture support #1446

Closed
iii-i opened this issue Sep 30, 2021 · 32 comments
Closed

IBM Z (aka SystemZ / SysZ / s390x) architecture support #1446

iii-i opened this issue Sep 30, 2021 · 32 comments
Labels

Comments

@iii-i
Copy link
Contributor

iii-i commented Sep 30, 2021

Hi,

I was wondering what would it take to support IBM Z in unicorn-engine?
QEMU 2.1.2 seems to have sufficient support for it, so I assume the main missing things are these?

  • UC_* defines
  • machine definition
  • hooks
  • build system support

Also, based on #1438, do I get it right that such contribution would have to wait until unicorn 2 is out, or would it make sense to also do this for 1.x?

Best regards,
Ilya

@wtdcode
Copy link
Member

wtdcode commented Sep 30, 2021 via email

@wtdcode
Copy link
Member

wtdcode commented Oct 1, 2021

Unfortunately, even with unicorn2, the s390 is not a supported target. You may make contributions afterward.

@wtdcode
Copy link
Member

wtdcode commented Oct 3, 2021

You could contribute now ;)

@wtdcode wtdcode closed this as completed Oct 3, 2021
@iii-i
Copy link
Contributor Author

iii-i commented Oct 7, 2021

@wtdcode Would you mind reopening this? We plan to post a BountySource bounty, and this requires having an open upstream issue.

@wtdcode
Copy link
Member

wtdcode commented Oct 7, 2021

@wtdcode Would you mind reopening this? We plan to post a BountySource bounty, and this requires having an open upstream issue.

Of course, would you add support for the architecture?

@wtdcode wtdcode reopened this Oct 7, 2021
@iii-i
Copy link
Contributor Author

iii-i commented Oct 7, 2021

@wtdcode Thanks!

Yes, even though it's not going to be me personally - we plan to offer a bounty for this feature on https://www.bountysource.com.

@wtdcode
Copy link
Member

wtdcode commented Oct 7, 2021

@wtdcode Thanks!

Yes, even though it's not going to be me personally - we plan to offer a bounty for this feature on https://www.bountysource.com.

Looks interesting. I might hunt your bounty if it looks attractive.

@edelsohn
Copy link

edelsohn commented Oct 7, 2021

Enablement seems include some or all of the following mentioned by Ilya privately:

We can break the project and issues into individual components, and we can decide which features we wish to prioritize.

@wtdcode
Copy link
Member

wtdcode commented Oct 7, 2021

Enablement seems include some or all of the following mentioned by Ilya privately:

It is the easiest part.

This part takes time and lots of tests & hacks to make every Unicorn features works properly. But I don't have both proper hardware & software to test for correctness and I lack enough spare time.

These two parts are typically equivalent.

This file is auto-generated.

We can break the project and issues into individual components, and we can decide which features we wish to prioritize.

@edelsohn
Copy link

edelsohn commented Oct 7, 2021

Access to Linux on Z VPS is available through the LinuxONE Community Cloud hosted at Marist.

https://linuxone.cloud.marist.edu/#/register?flag=VM

@aquynh
Copy link
Member

aquynh commented Oct 7, 2021 via email

@github-actions
Copy link

github-actions bot commented Dec 7, 2021

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 15 days.

@github-actions github-actions bot added the stale label Dec 7, 2021
@wtdcode wtdcode added pinned and removed stale labels Dec 7, 2021
@dmiller423
Copy link

Someone is working on this?

@dmiller423
Copy link

If not, I will.

@aquynh
Copy link
Member

aquynh commented Dec 31, 2021 via email

@wtdcode wtdcode mentioned this issue Dec 31, 2021
@wtdcode
Copy link
Member

wtdcode commented Dec 31, 2021

It is almost done. See #1521 for details.

@iii-i
Copy link
Contributor Author

iii-i commented Jan 10, 2022

@wtdcode I've given 5bb40c5 a try, looks quite promising! A few things:

  • psw.mask is not exposed (I defined UC_S390X_REG_PSWM and adjusted reg_read() and reg_write() in my local branch).

  • psw is not updated if you ask unicorn to emulate one instruction. I fixed this by calling update_psw_addr() on DISAS_UNICORN_HALT.

  • There is a use-after-free that triggers quite often. I wasn't able to find a solution quickly:

    ==23702==ERROR: AddressSanitizer: heap-use-after-free on address 0x604000002be8 at pc 0x7ffff1ec6974 bp 0x7fffffffd6f0 sp 0x7fffffffd6e0
    READ of size 8 at 0x604000002be8 thread T0
        #0 0x7ffff1ec6973 in flatview_to_dispatch unicorn/qemu/include/exec/memory-internal.h:27
        #1 0x7ffff1ec699d in address_space_to_dispatch unicorn/qemu/include/exec/memory-internal.h:32
        #2 0x7ffff1ece480 in tcg_commit unicorn/qemu/exec.c:1437
        #3 0x7ffff1edda4f in memory_region_transaction_commit_s390x unicorn/qemu/softmmu/memory.c:884
        #4 0x7ffff1edf40d in memory_region_del_subregion_s390x unicorn/qemu/softmmu/memory.c:1160
        #5 0x7ffff1ed867f in memory_free_s390x unicorn/qemu/softmmu/memory.c:181
        #6 0x7ffff1ec4183 in release_common unicorn/qemu/unicorn_common.h:63
        #7 0x7ffff1ec4772 in s390_release unicorn/qemu/target/s390x/unicorn.c:28
        #8 0x7fffefa68ec9 in uc_close unicorn/uc.c:391
        #9 0x461cd972  (<unknown module>)
    
    0x604000002be8 is located 24 bytes inside of 40-byte region [0x604000002bd0,0x604000002bf8)
    freed by thread T0 here:
        #0 0x7ffff76a47cf in __interceptor_free ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:122
        #1 0x7fffefa7cee0 in g_free unicorn/glib_compat/gmem.c:256
        #2 0x7ffff1ed9bdf in flatview_destroy unicorn/qemu/softmmu/memory.c:352
        #3 0x7ffff1ed9c46 in flatview_unref_s390x unicorn/qemu/softmmu/memory.c:359
        #4 0x7fffefa77052 in g_hash_table_remove_all_nodes unicorn/glib_compat/glib_compat.c:1046
        #5 0x7fffefa77199 in g_hash_table_remove_all unicorn/glib_compat/glib_compat.c:1075
        #6 0x7fffefa754a2 in g_hash_table_destroy unicorn/glib_compat/glib_compat.c:454
        #7 0x7ffff1edd0ec in flatviews_reset unicorn/qemu/softmmu/memory.c:810
        #8 0x7ffff1edd7e9 in memory_region_transaction_commit_s390x unicorn/qemu/softmmu/memory.c:876
        #9 0x7ffff1edf40d in memory_region_del_subregion_s390x unicorn/qemu/softmmu/memory.c:1160
        #10 0x7ffff1ed867f in memory_free_s390x unicorn/qemu/softmmu/memory.c:181
        #11 0x7ffff1ec4183 in release_common unicorn/qemu/unicorn_common.h:63
        #12 0x7ffff1ec4772 in s390_release unicorn/qemu/target/s390x/unicorn.c:28
        #13 0x7fffefa68ec9 in uc_close unicorn/uc.c:391
        #14 0x461cd972  (<unknown module>)
    
    previously allocated by thread T0 here:
        #0 0x7ffff76a4dc6 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:153
        #1 0x7fffefa7cda4 in g_malloc0 unicorn/glib_compat/gmem.c:139
        #2 0x7fffefa7cdf0 in g_malloc0_n unicorn/glib_compat/gmem.c:168
        #3 0x7ffff1ed9784 in flatview_new unicorn/qemu/softmmu/memory.c:318
        #4 0x7ffff1edbf45 in generate_memory_topology unicorn/qemu/softmmu/memory.c:690
        #5 0x7ffff1edd1f6 in flatviews_reset unicorn/qemu/softmmu/memory.c:823
        #6 0x7ffff1edd7e9 in memory_region_transaction_commit_s390x unicorn/qemu/softmmu/memory.c:876
        #7 0x7ffff1edefab in memory_region_update_container_subregions unicorn/qemu/softmmu/memory.c:1131
        #8 0x7ffff1edf10b in memory_region_add_subregion_common unicorn/qemu/softmmu/memory.c:1142
        #9 0x7ffff1edf142 in memory_region_add_subregion_s390x unicorn/qemu/softmmu/memory.c:1149
        #10 0x7ffff1ed7b96 in memory_map_s390x unicorn/qemu/softmmu/memory.c:51
        #11 0x7fffefa6b9e5 in uc_mem_map unicorn/uc.c:998

Edit: one more thing:

  • When setting PSWM to 0 (which corresponds to 24-bit mode) and emulating code from memory located above 16M, g_tree_foreach(uc->exits, uc_exit_invalidate_iter, (void*)uc); called from resume_all_vcpus() eventually triggers cpu_loop_exit() and siglongjmp(), for which the corresponding sigsetjmp() was set by cpu_exec() and is no longer on stack. I think g_tree_foreach() needs to be wrapped in sigsetjmp().

@wtdcode
Copy link
Member

wtdcode commented Jan 10, 2022

@wtdcode I've given 5bb40c5 a try, looks quite promising! A few things:

  • psw.mask is not exposed (I defined UC_S390X_REG_PSWM and adjusted reg_read() and reg_write() in my local branch).

Fixed in the latest commit.

  • psw is not updated if you ask unicorn to emulate one instruction. I fixed this by calling update_psw_addr() on DISAS_UNICORN_HALT.

If you mean the PC register, it's not updated by design. See https://github.com/unicorn-engine/unicorn/wiki/FAQ#why-do-i-get-a-wrong-pc-after-emulation-stops.

Sorry I misunderstood. It is fixed already.

  • There is a use-after-free that triggers quite often. I wasn't able to find a solution quickly:
    ==23702==ERROR: AddressSanitizer: heap-use-after-free on address 0x604000002be8 at pc 0x7ffff1ec6974 bp 0x7fffffffd6f0 sp 0x7fffffffd6e0
    READ of size 8 at 0x604000002be8 thread T0
        #0 0x7ffff1ec6973 in flatview_to_dispatch unicorn/qemu/include/exec/memory-internal.h:27
        #1 0x7ffff1ec699d in address_space_to_dispatch unicorn/qemu/include/exec/memory-internal.h:32
        #2 0x7ffff1ece480 in tcg_commit unicorn/qemu/exec.c:1437
        #3 0x7ffff1edda4f in memory_region_transaction_commit_s390x unicorn/qemu/softmmu/memory.c:884
        #4 0x7ffff1edf40d in memory_region_del_subregion_s390x unicorn/qemu/softmmu/memory.c:1160
        #5 0x7ffff1ed867f in memory_free_s390x unicorn/qemu/softmmu/memory.c:181
        #6 0x7ffff1ec4183 in release_common unicorn/qemu/unicorn_common.h:63
        #7 0x7ffff1ec4772 in s390_release unicorn/qemu/target/s390x/unicorn.c:28
        #8 0x7fffefa68ec9 in uc_close unicorn/uc.c:391
        #9 0x461cd972  (<unknown module>)
    
    0x604000002be8 is located 24 bytes inside of 40-byte region [0x604000002bd0,0x604000002bf8)
    freed by thread T0 here:
        #0 0x7ffff76a47cf in __interceptor_free ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:122
        #1 0x7fffefa7cee0 in g_free unicorn/glib_compat/gmem.c:256
        #2 0x7ffff1ed9bdf in flatview_destroy unicorn/qemu/softmmu/memory.c:352
        #3 0x7ffff1ed9c46 in flatview_unref_s390x unicorn/qemu/softmmu/memory.c:359
        #4 0x7fffefa77052 in g_hash_table_remove_all_nodes unicorn/glib_compat/glib_compat.c:1046
        #5 0x7fffefa77199 in g_hash_table_remove_all unicorn/glib_compat/glib_compat.c:1075
        #6 0x7fffefa754a2 in g_hash_table_destroy unicorn/glib_compat/glib_compat.c:454
        #7 0x7ffff1edd0ec in flatviews_reset unicorn/qemu/softmmu/memory.c:810
        #8 0x7ffff1edd7e9 in memory_region_transaction_commit_s390x unicorn/qemu/softmmu/memory.c:876
        #9 0x7ffff1edf40d in memory_region_del_subregion_s390x unicorn/qemu/softmmu/memory.c:1160
        #10 0x7ffff1ed867f in memory_free_s390x unicorn/qemu/softmmu/memory.c:181
        #11 0x7ffff1ec4183 in release_common unicorn/qemu/unicorn_common.h:63
        #12 0x7ffff1ec4772 in s390_release unicorn/qemu/target/s390x/unicorn.c:28
        #13 0x7fffefa68ec9 in uc_close unicorn/uc.c:391
        #14 0x461cd972  (<unknown module>)
    
    previously allocated by thread T0 here:
        #0 0x7ffff76a4dc6 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:153
        #1 0x7fffefa7cda4 in g_malloc0 unicorn/glib_compat/gmem.c:139
        #2 0x7fffefa7cdf0 in g_malloc0_n unicorn/glib_compat/gmem.c:168
        #3 0x7ffff1ed9784 in flatview_new unicorn/qemu/softmmu/memory.c:318
        #4 0x7ffff1edbf45 in generate_memory_topology unicorn/qemu/softmmu/memory.c:690
        #5 0x7ffff1edd1f6 in flatviews_reset unicorn/qemu/softmmu/memory.c:823
        #6 0x7ffff1edd7e9 in memory_region_transaction_commit_s390x unicorn/qemu/softmmu/memory.c:876
        #7 0x7ffff1edefab in memory_region_update_container_subregions unicorn/qemu/softmmu/memory.c:1131
        #8 0x7ffff1edf10b in memory_region_add_subregion_common unicorn/qemu/softmmu/memory.c:1142
        #9 0x7ffff1edf142 in memory_region_add_subregion_s390x unicorn/qemu/softmmu/memory.c:1149
        #10 0x7ffff1ed7b96 in memory_map_s390x unicorn/qemu/softmmu/memory.c:51
        #11 0x7fffefa6b9e5 in uc_mem_map unicorn/uc.c:998

Could you post a reproduction script?

I believe it has been fixed here 6fabf30 and has been merged into s390x branch.

Edit: one more thing:

  • When setting PSWM to 0 (which corresponds to 24-bit mode) and emulating code from memory located above 16M, g_tree_foreach(uc->exits, uc_exit_invalidate_iter, (void*)uc); called from resume_all_vcpus() eventually triggers cpu_loop_exit() and siglongjmp(), for which the corresponding sigsetjmp() was set by cpu_exec() and is no longer on stack. I think g_tree_foreach() needs to be wrapped in sigsetjmp().

It has been fixed in a recent commit. I have sync-ed s390x branch with dev just now.

@iii-i
Copy link
Contributor Author

iii-i commented Jan 10, 2022

Thanks, all of these issues are gone in 980eae7 (I had to regenerate bindings, but that's very minor).

I found a new one though:

==35351==ERROR: AddressSanitizer: heap-use-after-free on address 0x62d00001d2a8 at pc 0x7ffb22598f3d bp 0x7ffc420f96d0 sp 0x7ffc420f96c0
READ of size 8 at 0x62d00001d2a8 thread T0
    #0 0x7ffb22598f3c in mmu_handle_skey qemu/target/s390x/mmu_helper.c:322
    #1 0x7ffb2259a016 in mmu_translate_real qemu/target/s390x/mmu_helper.c:552
    #2 0x7ffb22573d03 in s390_cpu_tlb_fill qemu/target/s390x/excp_helper.c:131
    #3 0x7ffb2253fffc in tlb_fill qemu/accel/tcg/cputlb.c:965
    #4 0x7ffb2253f6b1 in get_page_addr_code_hostp_s390x qemu/accel/tcg/cputlb.c:1110
    #5 0x7ffb225400c0 in get_page_addr_code_s390x qemu/accel/tcg/cputlb.c:1142
    #6 0x7ffb225361fa in tb_htable_lookup_s390x qemu/accel/tcg/cpu-exec.c:187
    #7 0x7ffb22539f7d in tb_lookup__cpu_state qemu/include/exec/tb-lookup.h:43
    #8 0x7ffb22538557 in tb_find qemu/accel/tcg/cpu-exec.c:253
    #9 0x7ffb22536f0d in cpu_exec_s390x qemu/accel/tcg/cpu-exec.c:593
    #10 0x7ffb22463aec in tcg_cpu_exec qemu/softmmu/cpus.c:96
    #11 0x7ffb22463937 in resume_all_vcpus_s390x qemu/softmmu/cpus.c:206
    #12 0x7ffb22463ea4 in vm_start_s390x qemu/softmmu/cpus.c:222
    #13 0x7ffb1f2f6872 in uc_emu_start unicorn/uc.c:824
    #14 0x4cb40b96  (<unknown module>)

0x62d00001d2a8 is located 36520 bytes inside of 36576-byte region [0x62d000014400,0x62d00001d2e0)
freed by thread T0 here:
    #0 0x7ffb274697cf in __interceptor_free ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:122
    #1 0x7ffb1f2f3c2a in uc_close unicorn/uc.c:400
    #2 0x44a1ee12  (<unknown module>)

previously allocated by thread T0 here:
    #0 0x7ffb27469dc6 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:153
    #1 0x7ffb2256ef61 in cpu_s390_init qemu/target/s390x/cpu.c:248
    #2 0x7ffb2243dd7c in s390_cpus_init qemu/target/s390x/unicorn.c:171
    #3 0x7ffb1f302133 in machine_initialize qemu/softmmu/vl.c:56
    #4 0x7ffb1f2f43fd in uc_init unicorn/uc.c:177
    #5 0x7ffb1f2f893e in uc_mem_map unicorn/uc.c:987
    #6 0x4caa550e  (<unknown module>)

The following helped (not sure why we need to cache all that, s390_get_skeys_device() doesn't look too performance-intensive to me):

--- a/qemu/target/s390x/mmu_helper.c
+++ b/qemu/target/s390x/mmu_helper.c
@@ -284,8 +284,8 @@ static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
 
 static void mmu_handle_skey(uc_engine *uc, target_ulong addr, int rw, int *flags)
 {
-    static S390SKeysClass *skeyclass;
-    static S390SKeysState *ss;
+    S390SKeysClass *skeyclass;
+    S390SKeysState *ss;
     uint8_t key;
     int rc;
 
@@ -295,10 +295,8 @@ static void mmu_handle_skey(uc_engine *uc, target_ulong addr, int rw, int *flags
     }
 #endif
 
-    if (unlikely(!ss)) {
-        ss = s390_get_skeys_device(uc);
-        skeyclass = S390_SKEYS_GET_CLASS(ss);
-    }
+    ss = s390_get_skeys_device(uc);
+    skeyclass = S390_SKEYS_GET_CLASS(ss);
 
     /*
      * Whenever we create a new TLB entry, we set the storage key reference

Edit: apparently in upstream qemu it is performance-intensive. Maybe a better solution would be to have a couple fields in uc_engine.

@wtdcode
Copy link
Member

wtdcode commented Jan 10, 2022

Thanks, all of these issues are gone in 980eae7 (I had to regenerate bindings, but that's very minor).

I found a new one though:

==35351==ERROR: AddressSanitizer: heap-use-after-free on address 0x62d00001d2a8 at pc 0x7ffb22598f3d bp 0x7ffc420f96d0 sp 0x7ffc420f96c0
READ of size 8 at 0x62d00001d2a8 thread T0
    #0 0x7ffb22598f3c in mmu_handle_skey qemu/target/s390x/mmu_helper.c:322
    #1 0x7ffb2259a016 in mmu_translate_real qemu/target/s390x/mmu_helper.c:552
    #2 0x7ffb22573d03 in s390_cpu_tlb_fill qemu/target/s390x/excp_helper.c:131
    #3 0x7ffb2253fffc in tlb_fill qemu/accel/tcg/cputlb.c:965
    #4 0x7ffb2253f6b1 in get_page_addr_code_hostp_s390x qemu/accel/tcg/cputlb.c:1110
    #5 0x7ffb225400c0 in get_page_addr_code_s390x qemu/accel/tcg/cputlb.c:1142
    #6 0x7ffb225361fa in tb_htable_lookup_s390x qemu/accel/tcg/cpu-exec.c:187
    #7 0x7ffb22539f7d in tb_lookup__cpu_state qemu/include/exec/tb-lookup.h:43
    #8 0x7ffb22538557 in tb_find qemu/accel/tcg/cpu-exec.c:253
    #9 0x7ffb22536f0d in cpu_exec_s390x qemu/accel/tcg/cpu-exec.c:593
    #10 0x7ffb22463aec in tcg_cpu_exec qemu/softmmu/cpus.c:96
    #11 0x7ffb22463937 in resume_all_vcpus_s390x qemu/softmmu/cpus.c:206
    #12 0x7ffb22463ea4 in vm_start_s390x qemu/softmmu/cpus.c:222
    #13 0x7ffb1f2f6872 in uc_emu_start unicorn/uc.c:824
    #14 0x4cb40b96  (<unknown module>)

0x62d00001d2a8 is located 36520 bytes inside of 36576-byte region [0x62d000014400,0x62d00001d2e0)
freed by thread T0 here:
    #0 0x7ffb274697cf in __interceptor_free ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:122
    #1 0x7ffb1f2f3c2a in uc_close unicorn/uc.c:400
    #2 0x44a1ee12  (<unknown module>)

previously allocated by thread T0 here:
    #0 0x7ffb27469dc6 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:153
    #1 0x7ffb2256ef61 in cpu_s390_init qemu/target/s390x/cpu.c:248
    #2 0x7ffb2243dd7c in s390_cpus_init qemu/target/s390x/unicorn.c:171
    #3 0x7ffb1f302133 in machine_initialize qemu/softmmu/vl.c:56
    #4 0x7ffb1f2f43fd in uc_init unicorn/uc.c:177
    #5 0x7ffb1f2f893e in uc_mem_map unicorn/uc.c:987
    #6 0x4caa550e  (<unknown module>)

The following helped (not sure why we need to cache all that, s390_get_skeys_device() doesn't look too performance-intensive to me):

--- a/qemu/target/s390x/mmu_helper.c
+++ b/qemu/target/s390x/mmu_helper.c
@@ -284,8 +284,8 @@ static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
 
 static void mmu_handle_skey(uc_engine *uc, target_ulong addr, int rw, int *flags)
 {
-    static S390SKeysClass *skeyclass;
-    static S390SKeysState *ss;
+    S390SKeysClass *skeyclass;
+    S390SKeysState *ss;
     uint8_t key;
     int rc;
 
@@ -295,10 +295,8 @@ static void mmu_handle_skey(uc_engine *uc, target_ulong addr, int rw, int *flags
     }
 #endif
 
-    if (unlikely(!ss)) {
-        ss = s390_get_skeys_device(uc);
-        skeyclass = S390_SKEYS_GET_CLASS(ss);
-    }
+    ss = s390_get_skeys_device(uc);
+    skeyclass = S390_SKEYS_GET_CLASS(ss);
 
     /*
      * Whenever we create a new TLB entry, we set the storage key reference

Edit: apparently in upstream qemu it is performance-intensive. Maybe a better solution would be to have a couple fields in uc_engine.

Fixed. The ss and skeyclass should be never cached since we may have multiple uc instances.

@iii-i
Copy link
Contributor Author

iii-i commented Jan 10, 2022

Thanks! I found yet another corner case: we need to do something along the lines of

--- a/qemu/target/s390x/translate.c
+++ b/qemu/target/s390x/translate.c
@@ -6891,6 +6891,7 @@ static void s390x_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
     case DISAS_UNICORN_HALT:
         tcg_gen_insn_start(tcg_ctx, dc->base.pc_next, 0, 0);
         update_psw_addr(dc);
+        update_cc_op(dc);
         gen_helper_uc_s390x_exit(tcg_ctx, tcg_ctx->cpu_env);
         break;
     case DISAS_GOTO_TB:

otherwise in some cases (e.g. alfi) at the end env->op_cc will not be set and reading UC_S390X_REG_PSWM will produce an incorrect value.

@wtdcode
Copy link
Member

wtdcode commented Jan 10, 2022

Thanks! I found yet another corner case: we need to do something along the lines of

--- a/qemu/target/s390x/translate.c
+++ b/qemu/target/s390x/translate.c
@@ -6891,6 +6891,7 @@ static void s390x_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
     case DISAS_UNICORN_HALT:
         tcg_gen_insn_start(tcg_ctx, dc->base.pc_next, 0, 0);
         update_psw_addr(dc);
+        update_cc_op(dc);
         gen_helper_uc_s390x_exit(tcg_ctx, tcg_ctx->cpu_env);
         break;
     case DISAS_GOTO_TB:

otherwise in some cases (e.g. alfi) at the end env->op_cc will not be set and reading UC_S390X_REG_PSWM will produce an incorrect value.

Yes, you are right. cc might be lost if we don't save them at the end of emulation.

@iii-i
Copy link
Contributor Author

iii-i commented Jan 12, 2022

Just a quick update from my side - things are looking quite good, I haven't seen any further Unicorn-specific bugs, but I bumped into a few QEMU problems: https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg02488.html. I think these can be backported to Unicorn2 separately once the fixes are accepted upstream?

@aquynh
Copy link
Member

aquynh commented Jan 12, 2022 via email

@wtdcode
Copy link
Member

wtdcode commented Jan 12, 2022

Just a quick update from my side - things are looking quite good, I haven't seen any further Unicorn-specific bugs, but I bumped into a few QEMU problems: https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg02488.html. I think these can be backported to Unicorn2 separately once the fixes are accepted upstream?

Yes, we can backport that patch once it gets reviewed & merged.

@aquynh
Copy link
Member

aquynh commented Jan 17, 2022

@edelsohn @iii-i is there any issues that you want to fix, or it is OK to merge #1521 now?

@iii-i
Copy link
Contributor Author

iii-i commented Jan 17, 2022

I haven't bumped into any further issues when doing constructive work with the s390x branch, so I'm ok with merging it.

However, note that fuzzing (iii-i@2b11031) finds a segfault quite quickly:

unicorn$ cmake -DCMAKE_C_FLAGS="-fsanitize=address,fuzzer-no-link -O2"
unicorn$ make -j$(nproc)
unicorn/tests/fuzz$ clang -I ../../include -L ../.. -fsanitize=address,fuzzer -O2 -g fuzz_emu_s390x.c -o fuzz_emu_s390x -lunicorn
unicorn/tests/fuzz$ ASAN_OPTIONS=detect_leaks=0 LD_LIBRARY_PATH=../.. ./fuzz_emu_s390x
AddressSanitizer:DEADLYSIGNAL
=================================================================
==24239==ERROR: AddressSanitizer: SEGV on unknown address (pc 0x7f40e130e5fc bp 0x7fff1a8f0890 sp 0x7fff1a8f0680 T0)
==24239==The signal is caused by a READ memory access.
==24239==Hint: this fault was caused by a dereference of a high value address (see register values below).  Disassemble the provided pc to learn which register was used.
    #0 0x7f40e130e5fc in test_bit unicorn/qemu/include/qemu/bitops.h:122:19
    #1 0x7f40e130e5fc in init_ts_info unicorn/qemu/tcg/optimize.c:96:10
    #2 0x7f40e130e5fc in init_arg_info unicorn/qemu/tcg/optimize.c:111:5
    #3 0x7f40e130e5fc in tcg_optimize_s390x unicorn/qemu/tcg/optimize.c:641:17
    #4 0x7f40e113950e in tcg_gen_code_s390x unicorn/qemu/tcg/tcg.c:3747:5
    #5 0x7f40e11fd2f3 in tb_gen_code_s390x unicorn/qemu/accel/tcg/translate-all.c:1637:21
    #6 0x7f40e11a1eb3 in tb_find unicorn/qemu/accel/tcg/cpu-exec.c:256:14
    #7 0x7f40e11a1eb3 in cpu_exec_s390x unicorn/qemu/accel/tcg/cpu-exec.c:593:18
    #8 0x7f40e10e1fb0 in tcg_cpu_exec unicorn/qemu/softmmu/cpus.c:96:17
    #9 0x7f40e10e1fb0 in resume_all_vcpus_s390x unicorn/qemu/softmmu/cpus.c:206:13
    #10 0x7f40ddc09617 in uc_emu_start unicorn/uc.c:824:5
    #11 0x511a65 in LLVMFuzzerTestOneInput unicorn/tests/fuzz/fuzz_emu_s390x.c:48:9
    #12 0x43aea3 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (unicorn/tests/fuzz/fuzz_emu_s390x+0x43aea3) (BuildId: e26cd58bd7f50b8982923753679bbc956f6c903e)
    #13 0x42525f in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) (unicorn/tests/fuzz/fuzz_emu_s390x+0x42525f) (BuildId: e26cd58bd7f50b8982923753679bbc956f6c903e)
    #14 0x42af76 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (unicorn/tests/fuzz/fuzz_emu_s390x+0x42af76) (BuildId: e26cd58bd7f50b8982923753679bbc956f6c903e)
    #15 0x454592 in main (unicorn/tests/fuzz/fuzz_emu_s390x+0x454592) (BuildId: e26cd58bd7f50b8982923753679bbc956f6c903e)
    #16 0x7f40dc5480b2 in __libc_start_main /build/glibc-YbNSs7/glibc-2.31/csu/../csu/libc-start.c:308:16
    #17 0x41f9ed in _start (unicorn/tests/fuzz/fuzz_emu_s390x+0x41f9ed) (BuildId: e26cd58bd7f50b8982923753679bbc956f6c903e)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV unicorn/qemu/include/qemu/bitops.h:122:19 in test_bit
==24239==ABORTING

The problematic input is:

0000000000000000 <.data>:
   0:	b2 04 66 0a       	sck	1546(%r6)

@edelsohn
Copy link

If @iii-i is satisfied that the implementation is complete, it's fine with me.

@wtdcode
Copy link
Member

wtdcode commented Jan 18, 2022

I haven't bumped into any further issues when doing constructive work with the s390x branch, so I'm ok with merging it.

However, note that fuzzing (iii-i@2b11031) finds a segfault quite quickly:

unicorn$ cmake -DCMAKE_C_FLAGS="-fsanitize=address,fuzzer-no-link -O2"
unicorn$ make -j$(nproc)
unicorn/tests/fuzz$ clang -I ../../include -L ../.. -fsanitize=address,fuzzer -O2 -g fuzz_emu_s390x.c -o fuzz_emu_s390x -lunicorn
unicorn/tests/fuzz$ ASAN_OPTIONS=detect_leaks=0 LD_LIBRARY_PATH=../.. ./fuzz_emu_s390x
AddressSanitizer:DEADLYSIGNAL
=================================================================
==24239==ERROR: AddressSanitizer: SEGV on unknown address (pc 0x7f40e130e5fc bp 0x7fff1a8f0890 sp 0x7fff1a8f0680 T0)
==24239==The signal is caused by a READ memory access.
==24239==Hint: this fault was caused by a dereference of a high value address (see register values below).  Disassemble the provided pc to learn which register was used.
    #0 0x7f40e130e5fc in test_bit unicorn/qemu/include/qemu/bitops.h:122:19
    #1 0x7f40e130e5fc in init_ts_info unicorn/qemu/tcg/optimize.c:96:10
    #2 0x7f40e130e5fc in init_arg_info unicorn/qemu/tcg/optimize.c:111:5
    #3 0x7f40e130e5fc in tcg_optimize_s390x unicorn/qemu/tcg/optimize.c:641:17
    #4 0x7f40e113950e in tcg_gen_code_s390x unicorn/qemu/tcg/tcg.c:3747:5
    #5 0x7f40e11fd2f3 in tb_gen_code_s390x unicorn/qemu/accel/tcg/translate-all.c:1637:21
    #6 0x7f40e11a1eb3 in tb_find unicorn/qemu/accel/tcg/cpu-exec.c:256:14
    #7 0x7f40e11a1eb3 in cpu_exec_s390x unicorn/qemu/accel/tcg/cpu-exec.c:593:18
    #8 0x7f40e10e1fb0 in tcg_cpu_exec unicorn/qemu/softmmu/cpus.c:96:17
    #9 0x7f40e10e1fb0 in resume_all_vcpus_s390x unicorn/qemu/softmmu/cpus.c:206:13
    #10 0x7f40ddc09617 in uc_emu_start unicorn/uc.c:824:5
    #11 0x511a65 in LLVMFuzzerTestOneInput unicorn/tests/fuzz/fuzz_emu_s390x.c:48:9
    #12 0x43aea3 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (unicorn/tests/fuzz/fuzz_emu_s390x+0x43aea3) (BuildId: e26cd58bd7f50b8982923753679bbc956f6c903e)
    #13 0x42525f in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) (unicorn/tests/fuzz/fuzz_emu_s390x+0x42525f) (BuildId: e26cd58bd7f50b8982923753679bbc956f6c903e)
    #14 0x42af76 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (unicorn/tests/fuzz/fuzz_emu_s390x+0x42af76) (BuildId: e26cd58bd7f50b8982923753679bbc956f6c903e)
    #15 0x454592 in main (unicorn/tests/fuzz/fuzz_emu_s390x+0x454592) (BuildId: e26cd58bd7f50b8982923753679bbc956f6c903e)
    #16 0x7f40dc5480b2 in __libc_start_main /build/glibc-YbNSs7/glibc-2.31/csu/../csu/libc-start.c:308:16
    #17 0x41f9ed in _start (unicorn/tests/fuzz/fuzz_emu_s390x+0x41f9ed) (BuildId: e26cd58bd7f50b8982923753679bbc956f6c903e)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV unicorn/qemu/include/qemu/bitops.h:122:19 in test_bit
==24239==ABORTING

The problematic input is:

0000000000000000 <.data>:
   0:	b2 04 66 0a       	sck	1546(%r6)

@iii-i I'm integrating Unicorn2 into Google oss-fuzz here: google/oss-fuzz#6562 and I added support for s390x just now. Once that PR and #1521 get merged, we will receive regular fuzzing reports from Google. Therefore, maybe we can merge #1521 firstly?

@iii-i
Copy link
Contributor Author

iii-i commented Jan 18, 2022

Absolutely, please go ahead.

@wtdcode
Copy link
Member

wtdcode commented Jan 19, 2022

Close due to #1521 merged.

@wtdcode wtdcode closed this as completed Jan 19, 2022
iii-i added a commit to iii-i/serval that referenced this issue Feb 17, 2022
s390 support was implemented in Unicorn2:
unicorn-engine/unicorn#1446

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
iii-i added a commit to iii-i/serval that referenced this issue Feb 23, 2022
s390 support was implemented in Unicorn2:
unicorn-engine/unicorn#1446

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
iii-i added a commit to iii-i/serval that referenced this issue Mar 14, 2022
s390 support was implemented in Unicorn2:
unicorn-engine/unicorn#1446

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
iii-i added a commit to iii-i/serval that referenced this issue Mar 21, 2022
s390 support was implemented in Unicorn2:
unicorn-engine/unicorn#1446

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
iii-i added a commit to iii-i/serval that referenced this issue Mar 21, 2022
s390 support was implemented in Unicorn2:
unicorn-engine/unicorn#1446

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
iii-i added a commit to iii-i/serval that referenced this issue Mar 30, 2022
s390 support was implemented in Unicorn2:
unicorn-engine/unicorn#1446

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
iii-i added a commit to iii-i/serval that referenced this issue Apr 6, 2022
s390 support was implemented in Unicorn2:
unicorn-engine/unicorn#1446

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants