On 3/16/2023 10:07 AM, Wu, Fei wrote: > On 3/15/2023 2:15 AM, Richard Henderson wrote: >> On 3/14/23 06:47, Wu, Fei wrote: >>> On 3/13/2023 11:00 PM, Richard Henderson wrote: >>>> On 3/13/23 07:13, Wu, Fei2 wrote: >>>>> Hi Richard, >>>>> >>>>> Sorry for disturbing you. I'm doing some perf profiling on >>>>> qemu-riscv64, >>>>> I see 10%+ faster to build stress-ng without the following patch. I >>>>> know >>>>> it's incorrect to just skip this patch, I'm wondering if we can do >>>>> something on intercepting mmap/mprotect (very rare), e.g. even >>>>> invalidating all the TBs, but keep the cross-page block chaining. >>>> >>>> It also affects breakpoints. >>>> >>>> I have no good ideas for how to keep cross-page block chaining without >>>> breaking either of these use cases. If you come up with a good idea, >>>> please post on qemu-devel for discussion. >>>> >>> Thank you for reply. I am new to qemu/tcg, lots of details and >>> backgrounds need to catch up. >>> >>> If we only want to address user-mode qemu, and assume this cross-page >>> chain, first page -> second page: >>> >>> * breakpoints. If a new bp is added to second page, the chain is hard to >>> maintain, but it looks acceptable to flush all TBs and fall back to >>> current non-cross-page implementation during debugging? I think It's >>> different from the full system situation here: >>> https://gitlab.com/qemu-project/qemu/-/issues/404 >>> >>> * mprotect. If the 2nd page remains 'X' permission after mprotect, the >>> chain is still valid, if it's changed to non-X, then the syscall >>> interceptor will change the permission of corresponding host page to >>> non-X, it will be segfault as expected? >>> >>> * mmap. I cannot figure out the situation. Is there any unit test for >>> this, or could you please shed some light? >> Also munmap, but handled via the same path through page_set_flags, see >> >> if (inval_tb) { >> tb_invalidate_phys_range(start, end); >> } >> >> There is no unit test for mmap over an existing code page. >> I believe we do have one for mprotect. >> >> You could plausibly add a global variable choosing between >> link-all-pages and link-one-page modes; it would be protected by >> mmap_lock. For link-all-pages mode, the above tb_invalidate_phys_range >> becomes tb_flush. We probably want to start in link-one-page mode if >> gdbstub is active, which is the only way to set breakpoints in user-only >> mode. >> This is a good solution for gdbstub case, clean and simple. Current code leverages tb_flush() during gdb, it looks ready to support link-all-pages mode, I tried to test gdb with link-all-pages mode, and didn't find any counter example yet.
>> I expect mprotect/mmap over existing executable pages to be extremely >> rare. I expect munmap of existing executable pages to be rare-ish, with >> dlclose() being the most common case. You might wish to change from >> link-all-pages mode to link-one-page mode after one or more instances. >> Yes, I agree these calls are rare, so performance of this path is not crucial. If I understand correctly, we need to avoid the situation when the latter page is munmap-ed or changed to non executable protection, then the jump from preceding TB to this one shouldn't happen. In tb_invalidate_phys_range() -> do_tb_phys_invalidate(), it removes all relative TBs from cache, and also unlinks/unchains these TBs from preceding TBs, so next time guest attempts to run code in this munmap-ed page, the chain doesn't exist anymore, the protection will be checked and enforced. Thanks, Fei. >> And as I said, this discussion should happen on qemu-devel. >> > My fault. I didn't notice the cc list, and initialized another thread: > https://www.mail-archive.com/qemu-devel@nongnu.org/msg949625.html > > Would you prefer commenting there, or I move the content here? > > Thanks, > Fei. > > >> >> r~ >
