| Message ID | 20221024132459.3229709-26-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | tcg: Fix x86 TARGET_TB_PCREL (#1269) | expand |
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();
On 10/25/22 11:23, Claudio Fontana wrote: > 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 "the vCPU state to be restored 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(); >
On 10/25/22 19:23, Claudio Fontana wrote: >> +/* >> + * 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? Interesting question. By presenting this tb, have we asserted that host_pc is within (otherwise, why select this tb). But if we didn't find host_pc within the unwind data... that suggests that the tcg backend code generation may be wrong, generating an exception at an unexpected point. But for the purposes of this patch, it is no change in behaviour. Previously we returned from the function without goto found. r~
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); } @@ -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; + } - 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();
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(-)