Re: [PATCH v2 01/28] accel/tcg: Introduce translator_use_goto_tb

[Wu, Fei]

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~
>