qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

RFC: bsd-user broken a while ago, is this the right fix?


From: Warner Losh
Subject: RFC: bsd-user broken a while ago, is this the right fix?
Date: Sat, 24 Jun 2023 00:40:33 -0600

This change:

commit f00506aeca2f6d92318967693f8da8c713c163f3
Merge: d37158bb242 87e303de70f
Author: Peter Maydell <peter.maydell@linaro.org>
Date:   Wed Mar 29 11:19:19 2023 +0100

    Merge tag 'pull-tcg-20230328' of https://gitlab.com/rth7680/qemu into staging
   
    Use a local version of GTree [#285]
    Fix page_set_flags vs the last page of the address space [#1528]
    Re-enable gdbstub breakpoints under KVM
   
    # -----BEGIN PGP SIGNATURE-----
    #
    # iQFRBAABCgA7FiEEekgeeIaLTbaoWgXAZN846K9+IV8FAmQjcLIdHHJpY2hhcmQu
    # aGVuZGVyc29uQGxpbmFyby5vcmcACgkQZN846K9+IV8rkgf/ZazodovRKxfaO622
    # mGW7ywIm+hIZYmKC7ObiMKFrBoCyeXH9yOLSx42T70QstWvBMukjovLMz1+Ttbo1
    # VOvpGH2B5W76l3i+muAlKxFRbBH2kMLTaL+BXtkmkL4FJ9bS8WiPApsL3lEX/q2E
    # 3kqaT3N3C09sWO5oVAPGTUHL0EutKhOar2VZL0+PVPFzL3BNPhnQH9QcbNvDBV3n
    # cx3GSXZyL7Plyi+qwsKf/3Jo+F2wr2NVf3Dqscu9T1N1kI5hSjRpwqUEJzJZ5rei
    # ly/gBXC/J7+WN+x+w2JlN0kWXWqC0QbDfZnj96Pd3owWZ7j4sT9zR5fcNenecxlR
    # 38Bo0w==
    # =ysF7
    # -----END PGP SIGNATURE-----
    # gpg: Signature made Tue 28 Mar 2023 23:56:50 BST
    # gpg:                using RSA key 7A481E78868B4DB6A85A05C064DF38E8AF7E215F
    # gpg:                issuer "richard.henderson@linaro.org"
    # gpg: Good signature from "Richard Henderson <richard.henderson@linaro.org>" [full]
    # Primary key fingerprint: 7A48 1E78 868B 4DB6 A85A  05C0 64DF 38E8 AF7E 215F
   
    * tag 'pull-tcg-20230328' of https://gitlab.com/rth7680/qemu:
      softmmu: Restore use of CPU watchpoint for all accelerators
      softmmu/watchpoint: Add missing 'qemu/error-report.h' include
      softmmu: Restrict cpu_check_watchpoint / address_matches to TCG accel
      linux-user/arm: Take more care allocating commpage
      include/exec: Change reserved_va semantics to last byte
      linux-user: Pass last not end to probe_guest_base
      accel/tcg: Pass last not end to tb_invalidate_phys_range
      accel/tcg: Pass last not end to tb_invalidate_phys_page_range__locked
      accel/tcg: Pass last not end to page_collection_lock
      accel/tcg: Pass last not end to PAGE_FOR_EACH_TB
      accel/tcg: Pass last not end to page_reset_target_data
      accel/tcg: Pass last not end to page_set_flags
      linux-user: Diagnose misaligned -R size
      tcg: use QTree instead of GTree
      util: import GTree as QTree
   
    Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

breaks bsd-user. when I merge it to the bsd-user upstream blitz branch I get memory allocation errors on startup. At least for armv7.

specifically, if I back out the bsd-user part of both
commit 95059f9c313a7fbd7f22e4cdc1977c0393addc7b
Author: Richard Henderson <richard.henderson@linaro.org>
Date:   Mon Mar 6 01:26:29 2023 +0300

    include/exec: Change reserved_va semantics to last byte
   
    Change the semantics to be the last byte of the guest va, rather
    than the following byte.  This avoids some overflow conditions.
   
    Reviewed-by: Philippe Mathieu-Daud<C3><A9> <philmd@linaro.org>
    Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

and

commit 49840a4a098149067789255bca6894645f411036
Author: Richard Henderson <richard.henderson@linaro.org>
Date:   Mon Mar 6 01:51:09 2023 +0300

    accel/tcg: Pass last not end to page_set_flags
   
    Pass the address of the last byte to be changed, rather than
    the first address past the last byte.  This avoids overflow
    when the last page of the address space is involved.
   
    Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1528
    Reviewed-by: Philippe Mathieu-Daud<C3><A9> <philmd@linaro.org>
    Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

things work again. If I backout parts, it fails still. If I back out only one of
the two, but not both, then it fails.

What's happening is that we're picking a reserved_va that's overflowing when
we add 1 to it. this overflow goes away if I make the overflows not possible:
diff --git a/bsd-user/mmap.c b/bsd-user/mmap.c
index a88251f8705..bd86c0a8689 100644
--- a/bsd-user/mmap.c
+++ b/bsd-user/mmap.c
@@ -237,8 +237,8 @@ unsigned long last_brk;
 static abi_ulong mmap_find_vma_reserved(abi_ulong start, abi_ulong size,
                                         abi_ulong alignment)
 {
-    abi_ulong addr;
-    abi_ulong end_addr;
+    uint64_t addr;
+    uint64_t end_addr;
     int prot;
     int looped = 0;
 
My question is, is this the right fix? The old code avoided the overflow in two ways. 1 it set reserve_va to a page short (which if I fix that, it works better, but not quite right). and it never computes an address that may overflow (which the new code does without the above patch).

It seems to work, but it looks super weird.

Comments?

Warrner

reply via email to

[Prev in Thread] Current Thread [Next in Thread]