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.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).
>
> 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