Re: [RFC] Don't lookup full CPU state in the indirect branch fast path on AArch64 when running in user mode.

[Owen Anderson]

Ping.

I'd like to get feedback on how/whether this could be developed into a
landable version.

Thanks,

--Owen

On Tue, Sep 29, 2020 at 2:32 PM Owen Anderson <oanderso@google.com> wrote:
>
> Hello,
>
> I would like to request feedback on the following patch, which I do
> not believe should be applied to master as-is.  The idea here is to
> avoid gathering the full CPU state in the fast path of an indirect
> branch lookup when running in user mode on a platform where the flags
> can only be changed in privileged mode.  I believe this is true on the
> AArch64 scenario that I care about, but clearly not true in general.
> I'm particularly seeking feedback on how to clean this up into a
> version that checks the correct necessary and sufficient conditions to
> allow all users that can benefit from it to do so.
>
> On the workload that I am targeting (aarch64 on x86), this patch
> reduces execution wall time by approximately 20%, and eliminates
> indirect branch lookups from the hot stack traces entirely.
>
> Thank you,
>
> --Owen
>
> From 3d96db17d3baacb92ef1bc5e70ef06b97d06a0ae Mon Sep 17 00:00:00 2001
> From: Owen Anderson <oanderso@google.com>
> Date: Tue, 29 Sep 2020 13:47:00 -0700
> Subject: [RFC] Don't lookup full CPU state in the indirect branch fast path on
>  AArch64 when running in user mode.
>
> Most of the CPU state can't be changed in user mode, so this is useless work.
>
> Signed-off-by: Owen Anderson <oanderso@google.com>
> ---
>  include/exec/tb-lookup.h | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/include/exec/tb-lookup.h b/include/exec/tb-lookup.h
> index 9cf475bb03..f4ea0eb4c0 100644
> --- a/include/exec/tb-lookup.h
> +++ b/include/exec/tb-lookup.h
> @@ -25,7 +25,15 @@ tb_lookup__cpu_state(CPUState *cpu, target_ulong
> *pc, target_ulong *cs_base,
>      TranslationBlock *tb;
>      uint32_t hash;
>
> +#if !defined(TARGET_ARM) || !defined(CONFIG_USER_ONLY)
>      cpu_get_tb_cpu_state(env, pc, cs_base, flags);
> +#else
> +    if (is_a64(env)) {
> +      *pc = env->pc;
> +    } else {
> +      *pc = env->regs[15];
> +    }
> +#endif
>      hash = tb_jmp_cache_hash_func(*pc);
>      tb = qatomic_rcu_read(&cpu->tb_jmp_cache[hash]);
>
> @@ -34,12 +42,19 @@ tb_lookup__cpu_state(CPUState *cpu, target_ulong
> *pc, target_ulong *cs_base,
>
>      if (likely(tb &&
>                 tb->pc == *pc &&
> +#if !defined(TARGET_ARM) || !defined(CONFIG_USER_ONLY)
>                 tb->cs_base == *cs_base &&
>                 tb->flags == *flags &&
> +#endif
>                 tb->trace_vcpu_dstate == *cpu->trace_dstate &&
>                 (tb_cflags(tb) & (CF_HASH_MASK | CF_INVALID)) == cf_mask)) {
>          return tb;
>      }
> +
> +#ifdef CONFIG_USER_ONLY
> +    cpu_get_tb_cpu_state(env, pc, cs_base, flags);
> +#endif
> +
>      tb = tb_htable_lookup(cpu, *pc, *cs_base, *flags, cf_mask);
>      if (tb == NULL) {
>          return NULL;
> --
> 2.28.0.709.gb0816b6eb0-goog