On 10/27/22 13:30, Richard Henderson wrote: > 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.hender...@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 <cfont...@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~ >
Very clear thanks, Reviewed-by: Claudio Fontana <cfont...@suse.de>
