On Fri, Jun 30, 2023 at 12:24 PM Richard Henderson < richard.hender...@linaro.org> wrote:
> On 6/26/23 11:52, Richard Henderson wrote: > > On 6/26/23 10:28, Daniel P. Berrangé wrote: > >> Just CC'ing Richard to make sure it catches his attention. > >> > >> On Sat, Jun 24, 2023 at 12:40:33AM -0600, Warner Losh wrote: > >>> This change: > >>> > >>> commit f00506aeca2f6d92318967693f8da8c713c163f3 > >>> Merge: d37158bb242 87e303de70f > >>> Author: Peter Maydell <peter.mayd...@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.hender...@linaro.org" > >>> # gpg: Good signature from "Richard Henderson < > >>> richard.hender...@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.mayd...@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.hender...@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> <phi...@linaro.org> > >>> Signed-off-by: Richard Henderson <richard.hender...@linaro.org> > >>> > >>> and > >>> > >>> commit 49840a4a098149067789255bca6894645f411036 > >>> Author: Richard Henderson <richard.hender...@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> <phi...@linaro.org> > >>> Signed-off-by: Richard Henderson <richard.hender...@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). > > > > Not really correct, though it will work for the 32-bit guests. > > > > You want to change end_addr to last_addr, which would be end_addr - 1, > and do that > > basically everywhere. That's the only way to avoid overflow properly, > and is what I'm > > settling on with page_set_flags et al. > > ... and fyi there's now a follow-up that replaces this function entirely. > It is in fact much easier to do with the interval tree in hand. > thanks richard. I'll try to test the patches you posted later today... .... but most of my plans for the day have been upended by a clogged sewer drain. Warner
