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

[Richard Henderson]

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.

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.

And as I said, this discussion should happen on qemu-devel.


r~