Re: [PATCH v2 1/6] accel/tcg: Introduce cpu_unwind_state_data

[Richard Henderson]

On 10/27/22 20:40, Claudio Fontana wrote:
> On 10/27/22 12:02, 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>
> > ---
> >   accel/tcg/internal.h      |  4 +--
> >   include/exec/exec-all.h   | 21 ++++++++---
> >   accel/tcg/translate-all.c | 74 ++++++++++++++++++++++++++-------------
> >   3 files changed, 68 insertions(+), 31 deletions(-)
> >
> > diff --git a/accel/tcg/internal.h b/accel/tcg/internal.h
> > index 1227bb69bd..9c06b320b7 100644
> > --- a/accel/tcg/internal.h
> > +++ b/accel/tcg/internal.h
> > @@ -106,8 +106,8 @@ void tb_reset_jump(TranslationBlock *tb, int n);
> >   TranslationBlock *tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
> >                                  tb_page_addr_t phys_page2);
> >   bool tb_invalidate_phys_page_unwind(tb_page_addr_t addr, uintptr_t pc);
> > -int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
> > -                              uintptr_t searched_pc, bool reset_icount);
> > +void cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
> > +                               uintptr_t host_pc, bool reset_icount);
> >   
> >   /* Return the current PC from CPU, which may be cached in TB. */
> >   static inline target_ulong log_pc(CPUState *cpu, const TranslationBlock *tb)
> > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> > index e948992a80..7d851f5907 100644
> > --- a/include/exec/exec-all.h
> > +++ b/include/exec/exec-all.h
> > @@ -39,20 +39,33 @@ typedef ram_addr_t tb_page_addr_t;
> >   #define TB_PAGE_ADDR_FMT RAM_ADDR_FMT
> >   #endif
> >   
> > +/**
> > + * cpu_unwind_state_data:
> > + * @cpu: the cpu context
> > + * @host_pc: the host pc within the translation
> > + * @data: output data
> > + *
> > + * Attempt to load the the unwind state for a host pc occurring in
> > + * translated code.  If @host_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
> > - * @searched_pc: the host PC the fault occurred at
> > + * @cpu: the cpu context
> > + * @host_pc: the host pc within the translation
> >    * @will_exit: true if the TB executed will be interrupted after some
> >                  cpu adjustments. Required for maintaining the correct
> >                  icount valus
> >    * @return: true if state was restored, false otherwise
> >    *
> >    * Attempt to restore the state for a fault occurring in translated
> > - * code. If the searched_pc is not in translated code no state is
> > + * code. If @host_pc is not in translated code no state is
> >    * restored and the function returns false.
> >    */
> > -bool cpu_restore_state(CPUState *cpu, uintptr_t searched_pc, bool will_exit);
> > +bool cpu_restore_state(CPUState *cpu, uintptr_t host_pc, bool will_exit);
> >   
> >   G_NORETURN void cpu_loop_exit_noexc(CPUState *cpu);
> >   G_NORETURN void cpu_loop_exit(CPUState *cpu);
> > diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> > index f185356a36..319becb698 100644
> > --- a/accel/tcg/translate-all.c
> > +++ b/accel/tcg/translate-all.c
> > @@ -247,52 +247,66 @@ 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.
> > - */
> > -int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
> > -                              uintptr_t searched_pc, bool reset_icount)
>
>
> Maybe add a small comment about what the return value of this static function 
> means?
> It can be indirectly inferred from its point of use:
>
>   +    int insns_left = cpu_unwind_data_from_tb(tb, host_pc, data);
>
> But I find having the information about the meaning of a function and return 
> value useful to be available there.
>
> IIUC for external functions the standard way is to document in the header 
> files, but for the static functions I would think we can do it here.
>
> With that Reviewed-by: Claudio Fontana <cfontana@suse.de>


I added

+/**
+ * cpu_unwind_data_from_tb: Load unwind data for TB
+ * @tb: translation block
+ * @host_pc: the host pc within translation
+ * @data: output array
+ *
+ * Within @tb, locate the guest insn whose translation contains @host_pc,
+ * then load the unwind data created by INDEX_opc_start_insn for that
+ * guest insn.  Return the number of guest insns which remain un-executed
+ * within @tb -- these must be credited back to the cpu's icount budget.
+ *
+ * If we could not determine which guest insn to which @host_pc belongs,
+ * return -1 and do not load unwind data.
+ * FIXME: Such a failure is likely to break the guest, as we were not
+ * expecting to unwind from such a location.  This may be some sort of
+ * backend code generation problem.  Consider asserting instead.
  */

Which I think captures some of your v1 comments as well.


r~