Re: [RFC PATCH 5/8] RFC: accel/tcg: allow cross-page goto_tb chaining in user-only builds

On 17/8/26 21:00, Matt Turner wrote:
> translator_use_goto_tb() refuses to chain unless the destination is on the
> same page as the start of the TB. For guests whose text is much larger than
> a page this is expensive: an emulated alpha gcc compiling a 255k line
> translation unit takes the indirect dispatch path for 8.4 billion of its
> 34.2 billion TB exits, and a large share of those are ordinary direct
> branches that simply crossed an 8 KiB page boundary.
> 
> The restriction was made unconditional by d3a2a1d803 ("accel/tcg:
> Introduce translator_use_goto_tb"), whose rationale was:
> 
>      Various targets avoid the page crossing test for CONFIG_USER_ONLY,
>      but that is wrong: mmap and mprotect can change page permissions.
> 
> That is true, but in user-only builds the invalidation path already covers
> it. There are no page tables: every mmap, mprotect and munmap reaches
> page_set_flags(), which calls tb_invalidate_phys_range() whenever the flags
> actually change, and tb_phys_invalidate() calls tb_jmp_unlink() to reset
> incoming jumps. A chained cross-page jump is therefore broken whenever the
> destination page's permissions change. This is not true in system mode,
> where TBs are keyed by physical address and a page table change invalidates
> nothing, so the restriction is kept there.
> 
> Add tests/tcg/alpha/test-xpage-chain.c to cover the hazard directly. It
> places a direct branch near the end of one page targeting the next page,
> runs it 200000 times so the chain is established, then checks that
> mprotect(PROT_NONE) makes the next call fault, and that remapping the page
> with different code runs the new code rather than a stale translation.
> 
> The test detects the hazard it is meant to detect: with the
> tb_invalidate_phys_range() call in page_set_flags() commented out, it fails
> both phases, executing page B after PROT_NONE and returning the stale
> result.
> 
> Measured with qemu-alpha running an emulated alpha gcc 16.2.0 compiling the
> SQLite 3.45.1 amalgamation on an x86-64 host, LTO build, on top of the
> preceding patches:
> 
>      before: 890,713,633,237 instructions
>      after:  869,178,598,378 instructions   -2.42%
> 
>      before: 84.44s wall clock
>      after:  80.49s wall clock              -4.68%
> 
> Note that this is worth more in time than in instructions, the reverse of
> the preceding patch: a chained jump replaces a cache probe whose loads can
> miss, so the instructions it removes are more expensive than average.
> 
> Measured before the inline jump cache probe, when a missed chain cost a
> helper call rather than an inline probe, the same change was worth -7.9%.
> 
> RFC because this reverses a deliberate decision and the reasoning above
> wants review from someone who knows the invalidation paths better than I
> do.
> 
> Signed-off-by: Matt Turner <[email protected]>
> ---
>   accel/tcg/translator.c             |  12 ++++
>   tests/tcg/alpha/Makefile.target    |   2 +-
>   tests/tcg/alpha/test-xpage-chain.c | 111 +++++++++++++++++++++++++++++
>   3 files changed, 124 insertions(+), 1 deletion(-)
>   create mode 100644 tests/tcg/alpha/test-xpage-chain.c
> 
> diff --git ./accel/tcg/translator.c ./accel/tcg/translator.c
> index 29e609b2ec..4921bf978c 100644
> --- ./accel/tcg/translator.c
> +++ ./accel/tcg/translator.c
> @@ -117,8 +117,20 @@ bool translator_use_goto_tb(DisasContextBase *db, vaddr dest)
>           return false;
>       }
>   
> +#ifdef CONFIG_USER_ONLY
> +    /*
> +     * There are no page tables in user-only mode.  Every mmap, mprotect and
> +     * munmap goes through page_set_flags(), which calls
> +     * tb_invalidate_phys_range() whenever the flags actually change, and
> +     * tb_phys_invalidate() unlinks incoming jumps.  A chained cross-page
> +     * jump is therefore broken whenever the destination page's permissions
> +     * change, so the same-page restriction is not needed here.
> +     */
> +    return true;

Should this be directly changed in translator_is_same_page() in
"exec/translator.h" instead?

> +#else
>       /* Check for the dest on the same page as the start of the TB.  */
>       return translator_is_same_page(db, dest);
> +#endif
>   }