Re: [PATCH 24/29] accel/tcg: Introduce cpu_unwind_state_data

[Claudio Fontana]

On 10/24/22 15:24, Richard Henderson wrote:
> Add a way to examine the unwind data without actually
> restoring the data back into env.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/exec/exec-all.h   | 13 ++++++++
>  accel/tcg/translate-all.c | 68 ++++++++++++++++++++++++++-------------
>  2 files changed, 58 insertions(+), 23 deletions(-)
> 
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 300832bd0b..d49cf113dd 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -39,6 +39,19 @@ typedef ram_addr_t tb_page_addr_t;
>  #define TB_PAGE_ADDR_FMT RAM_ADDR_FMT
>  #endif
>  
> +/**
> + * cpu_unwind_state_data:
> + * @cpu: the vCPU state is to be restore to
> + * @host_pc: the host PC the fault occurred at
> + * @data: output data
> + *
> + * Attempt to load the the unwind state for a host pc occurring in
> + * translated code.  If the searched_pc is not in translated code,
> + * the function returns false; otherwise @data is loaded.
> + * This is the same unwind info as given to restore_state_to_opc.
> + */
> +bool cpu_unwind_state_data(CPUState *cpu, uintptr_t host_pc, uint64_t *data);
> +
>  /**
>   * cpu_restore_state:
>   * @cpu: the vCPU state is to be restore to
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index e4386b3198..c772e3769c 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -320,29 +320,20 @@ static int encode_search(TranslationBlock *tb, uint8_t 
> *block)
>      return p - block;
>  }
>  
> -/* The cpu state corresponding to 'searched_pc' is restored.
> - * When reset_icount is true, current TB will be interrupted and
> - * icount should be recalculated.
> - */
> -static int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
> -                                     uintptr_t searched_pc, bool 
> reset_icount)
> +static int cpu_unwind_data_from_tb(TranslationBlock *tb, uintptr_t host_pc,
> +                                   uint64_t *data)
>  {
> -    uint64_t data[TARGET_INSN_START_WORDS];
> -    uintptr_t host_pc = (uintptr_t)tb->tc.ptr;
> +    uintptr_t iter_pc = (uintptr_t)tb->tc.ptr;
>      const uint8_t *p = tb->tc.ptr + tb->tc.size;
>      int i, j, num_insns = tb->icount;
> -#ifdef CONFIG_PROFILER
> -    TCGProfile *prof = &tcg_ctx->prof;
> -    int64_t ti = profile_getclock();
> -#endif
>  
> -    searched_pc -= GETPC_ADJ;
> +    host_pc -= GETPC_ADJ;
>  
> -    if (searched_pc < host_pc) {
> +    if (host_pc < iter_pc) {
>          return -1;
>      }
>  
> -    memset(data, 0, sizeof(data));
> +    memset(data, 0, sizeof(uint64_t) * TARGET_INSN_START_WORDS);
>      if (!TARGET_TB_PCREL) {
>          data[0] = tb_pc(tb);
>      }

It's not visible in this hunk, but what follows is:

    /* Reconstruct the stored insn data while looking for the point at          
                                                            
       which the end of the insn exceeds the searched_pc.  */

Should this comment be adapted, minimally with s,searched_pc,host_pc, ?


> @@ -353,19 +344,40 @@ static int cpu_restore_state_from_tb(CPUState *cpu, 
> TranslationBlock *tb,
>          for (j = 0; j < TARGET_INSN_START_WORDS; ++j) {
>              data[j] += decode_sleb128(&p);
>          }
> -        host_pc += decode_sleb128(&p);
> -        if (host_pc > searched_pc) {
> -            goto found;
> +        iter_pc += decode_sleb128(&p);
> +        if (iter_pc > host_pc) {
> +            return num_insns - i;
>          }
>      }
>      return -1;
> +}
> +
> +/*
> + * The cpu state corresponding to 'host_pc' is restored.
> + * When reset_icount is true, current TB will be interrupted and
> + * icount should be recalculated.
> + */
> +static void cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
> +                                      uintptr_t host_pc, bool reset_icount)
> +{
> +    uint64_t data[TARGET_INSN_START_WORDS];
> +#ifdef CONFIG_PROFILER
> +    TCGProfile *prof = &tcg_ctx->prof;
> +    int64_t ti = profile_getclock();
> +#endif
> +    int insns_left = cpu_unwind_data_from_tb(tb, host_pc, data);
> +
> +    if (insns_left < 0) {
> +        return;
> +    }

Is the -1 return value some error condition to do anything about, log, tcg 
assert, or ...,
under some DEBUG_* condition, or ignored as done here?

Thanks,

Claudio

>  
> - found:
>      if (reset_icount && (tb_cflags(tb) & CF_USE_ICOUNT)) {
>          assert(icount_enabled());
> -        /* Reset the cycle counter to the start of the block
> -           and shift if to the number of actually executed instructions */
> -        cpu_neg(cpu)->icount_decr.u16.low += num_insns - i;
> +        /*
> +         * Reset the cycle counter to the start of the block and
> +         * shift if to the number of actually executed instructions.
> +         */
> +        cpu_neg(cpu)->icount_decr.u16.low += insns_left;
>      }
>  
>      cpu->cc->tcg_ops->restore_state_to_opc(cpu, tb, data);
> @@ -375,7 +387,6 @@ static int cpu_restore_state_from_tb(CPUState *cpu, 
> TranslationBlock *tb,
>                  prof->restore_time + profile_getclock() - ti);
>      qatomic_set(&prof->restore_count, prof->restore_count + 1);
>  #endif
> -    return 0;
>  }
>  
>  bool cpu_restore_state(CPUState *cpu, uintptr_t host_pc, bool will_exit)
> @@ -408,6 +419,17 @@ bool cpu_restore_state(CPUState *cpu, uintptr_t host_pc, 
> bool will_exit)
>      return false;
>  }
>  
> +bool cpu_unwind_state_data(CPUState *cpu, uintptr_t host_pc, uint64_t *data)
> +{
> +    if (in_code_gen_buffer((const void *)(host_pc - tcg_splitwx_diff))) {
> +        TranslationBlock *tb = tcg_tb_lookup(host_pc);
> +        if (tb) {
> +            return cpu_unwind_data_from_tb(tb, host_pc, data) >= 0;
> +        }
> +    }
> +    return false;
> +}
> +
>  void page_init(void)
>  {
>      page_size_init();