| Message ID | 20170614194821.8754-3-rth@twiddle.net |
|---|---|
| State | New |
| Headers | show |
On Wed, Jun 14, 2017 at 12:48:18 -0700, Richard Henderson wrote: > Signed-off-by: Richard Henderson <rth@twiddle.net> (snip) > @@ -1198,7 +1205,10 @@ static ExitStatus gen_call_pal(DisasContext *ctx, int palcode) > tcg_gen_andi_i64(tmp, ctx->ir[IR_A0], PS_INT_MASK); > tcg_gen_st8_i64(tmp, cpu_env, offsetof(CPUAlphaState, ps)); > tcg_temp_free(tmp); > - break; > + > + /* Allow interrupts to be recognized right away. */ > + tcg_gen_movi_i64(cpu_pc, ctx.pc); ctx->pc though Tested-by: Emilio G. Cota <cota@braap.org> Thanks! E.
On 06/14/2017 01:37 PM, Emilio G. Cota wrote: > On Wed, Jun 14, 2017 at 12:48:18 -0700, Richard Henderson wrote: >> Signed-off-by: Richard Henderson <rth@twiddle.net> > (snip) >> @@ -1198,7 +1205,10 @@ static ExitStatus gen_call_pal(DisasContext *ctx, int palcode) >> tcg_gen_andi_i64(tmp, ctx->ir[IR_A0], PS_INT_MASK); >> tcg_gen_st8_i64(tmp, cpu_env, offsetof(CPUAlphaState, ps)); >> tcg_temp_free(tmp); >> - break; >> + >> + /* Allow interrupts to be recognized right away. */ >> + tcg_gen_movi_i64(cpu_pc, ctx.pc); > > ctx->pc though Bah. I knew I'd fixed that, but it got folded into the s390 patch. r~
Richard Henderson <rth@twiddle.net> writes: > Signed-off-by: Richard Henderson <rth@twiddle.net> > --- > target/alpha/translate.c | 27 ++++++++++++++++++++++----- > 1 file changed, 22 insertions(+), 5 deletions(-) > > diff --git a/target/alpha/translate.c b/target/alpha/translate.c > index 7c45ae3..a48e451 100644 > --- a/target/alpha/translate.c > +++ b/target/alpha/translate.c > @@ -84,6 +84,7 @@ typedef enum { > the PC (for whatever reason), so there's no need to do it again on > exiting the TB. */ > EXIT_PC_UPDATED, > + EXIT_PC_UPDATED_NOCHAIN, > > /* We are exiting the TB, but have neither emitted a goto_tb, nor > updated the PC for the next instruction to be executed. */ > @@ -458,11 +459,17 @@ static bool in_superpage(DisasContext *ctx, int64_t addr) > #endif > } > > +static bool use_exit_tb(DisasContext *ctx) > +{ > + return ((ctx->tb->cflags & CF_LAST_IO) > + || ctx->singlestep_enabled > + || singlestep); > +} minor nit: why start testing this global? At the least we should probably seed ctx->singlestep_enabled when we set up for translation. > + > static bool use_goto_tb(DisasContext *ctx, uint64_t dest) > { > /* Suppress goto_tb in the case of single-steping and IO. */ > - if ((ctx->tb->cflags & CF_LAST_IO) > - || ctx->singlestep_enabled || singlestep) { > + if (unlikely(use_exit_tb(ctx))) { > return false; > } > #ifndef CONFIG_USER_ONLY > @@ -1198,7 +1205,10 @@ static ExitStatus gen_call_pal(DisasContext *ctx, int palcode) > tcg_gen_andi_i64(tmp, ctx->ir[IR_A0], PS_INT_MASK); > tcg_gen_st8_i64(tmp, cpu_env, offsetof(CPUAlphaState, ps)); > tcg_temp_free(tmp); > - break; > + > + /* Allow interrupts to be recognized right away. */ > + tcg_gen_movi_i64(cpu_pc, ctx.pc); > + return EXIT_PC_UPDATED_NOCHAIN; > > case 0x36: > /* RDPS */ > @@ -1266,7 +1276,7 @@ static ExitStatus gen_call_pal(DisasContext *ctx, int palcode) > need the page permissions check. We'll see the existence of > the page when we create the TB, and we'll flush all TBs if > we change the PAL base register. */ > - if (!ctx->singlestep_enabled && !(ctx->tb->cflags & CF_LAST_IO)) { > + if (!use_exit_tb(ctx)) { > tcg_gen_goto_tb(0); > tcg_gen_movi_i64(cpu_pc, entry); > tcg_gen_exit_tb((uintptr_t)ctx->tb); > @@ -2686,7 +2696,8 @@ static ExitStatus translate_one(DisasContext *ctx, uint32_t insn) > tcg_gen_andi_i64(tmp, vb, 1); > tcg_gen_st8_i64(tmp, cpu_env, offsetof(CPUAlphaState, pal_mode)); > tcg_gen_andi_i64(cpu_pc, vb, ~3); > - ret = EXIT_PC_UPDATED; > + /* Allow interrupts to be recognized right away. */ > + ret = EXIT_PC_UPDATED_NOCHAIN; > break; > #else > goto invalid_opc; > @@ -3010,6 +3021,12 @@ void gen_intermediate_code(CPUAlphaState *env, struct TranslationBlock *tb) > tcg_gen_movi_i64(cpu_pc, ctx.pc); > /* FALLTHRU */ > case EXIT_PC_UPDATED: > + if (!use_exit_tb(&ctx)) { > + tcg_gen_lookup_and_goto_ptr(cpu_pc); > + break; > + } > + /* FALLTHRU */ > + case EXIT_PC_UPDATED_NOCHAIN: > if (ctx.singlestep_enabled) { > gen_excp_1(EXCP_DEBUG, 0); > } else { Aside from what Emilio already pointed at: Reviewed-by: Alex Bennée <alex.bennee@linaro.org> -- Alex Bennée
On 06/15/2017 01:48 AM, Alex Bennée wrote: > > Richard Henderson <rth@twiddle.net> writes: > >> Signed-off-by: Richard Henderson <rth@twiddle.net> >> --- >> target/alpha/translate.c | 27 ++++++++++++++++++++++----- >> 1 file changed, 22 insertions(+), 5 deletions(-) >> >> diff --git a/target/alpha/translate.c b/target/alpha/translate.c >> index 7c45ae3..a48e451 100644 >> --- a/target/alpha/translate.c >> +++ b/target/alpha/translate.c >> @@ -84,6 +84,7 @@ typedef enum { >> the PC (for whatever reason), so there's no need to do it again on >> exiting the TB. */ >> EXIT_PC_UPDATED, >> + EXIT_PC_UPDATED_NOCHAIN, >> >> /* We are exiting the TB, but have neither emitted a goto_tb, nor >> updated the PC for the next instruction to be executed. */ >> @@ -458,11 +459,17 @@ static bool in_superpage(DisasContext *ctx, int64_t addr) >> #endif >> } >> >> +static bool use_exit_tb(DisasContext *ctx) >> +{ >> + return ((ctx->tb->cflags & CF_LAST_IO) >> + || ctx->singlestep_enabled >> + || singlestep); >> +} > > minor nit: why start testing this global? At the least we should > probably seed ctx->singlestep_enabled when we set up for translation. > >> + >> static bool use_goto_tb(DisasContext *ctx, uint64_t dest) >> { >> /* Suppress goto_tb in the case of single-steping and IO. */ >> - if ((ctx->tb->cflags & CF_LAST_IO) >> - || ctx->singlestep_enabled || singlestep) { I didn't start testing this global. It's already there. Further, despite the name similarity these are very different conditions. For ctx->singlestep_enabled we emit a debug exception at the end of every instruction. For singlestep, we must execute only one insn in the TB and further we must return to the main loop after the TB. Both are required for -singlestep -d cpu to log all that's being requested. One cannot combine the two conditions. r~
Peter cleaned that and improved the doc on ARM: https://patchwork.kernel.org/patch/9690993/ (commit b636649f5a2e108413dd171edaf320f781f57942) $ git grep -- singlestep_enabled target|wc -l 113 still confusing... On Thu, Jun 15, 2017 at 5:55 PM, Richard Henderson <rth@twiddle.net> wrote: > On 06/15/2017 01:48 AM, Alex Bennée wrote: >> >> >> Richard Henderson <rth@twiddle.net> writes: >> >>> Signed-off-by: Richard Henderson <rth@twiddle.net> >>> --- >>> target/alpha/translate.c | 27 ++++++++++++++++++++++----- >>> 1 file changed, 22 insertions(+), 5 deletions(-) >>> >>> diff --git a/target/alpha/translate.c b/target/alpha/translate.c >>> index 7c45ae3..a48e451 100644 >>> --- a/target/alpha/translate.c >>> +++ b/target/alpha/translate.c >>> @@ -84,6 +84,7 @@ typedef enum { >>> the PC (for whatever reason), so there's no need to do it again >>> on >>> exiting the TB. */ >>> EXIT_PC_UPDATED, >>> + EXIT_PC_UPDATED_NOCHAIN, >>> >>> /* We are exiting the TB, but have neither emitted a goto_tb, nor >>> updated the PC for the next instruction to be executed. */ >>> @@ -458,11 +459,17 @@ static bool in_superpage(DisasContext *ctx, int64_t >>> addr) >>> #endif >>> } >>> >>> +static bool use_exit_tb(DisasContext *ctx) >>> +{ >>> + return ((ctx->tb->cflags & CF_LAST_IO) >>> + || ctx->singlestep_enabled >>> + || singlestep); >>> +} >> >> >> minor nit: why start testing this global? At the least we should >> probably seed ctx->singlestep_enabled when we set up for translation. >> >>> + >>> static bool use_goto_tb(DisasContext *ctx, uint64_t dest) >>> { >>> /* Suppress goto_tb in the case of single-steping and IO. */ >>> - if ((ctx->tb->cflags & CF_LAST_IO) >>> - || ctx->singlestep_enabled || singlestep) { > > > I didn't start testing this global. It's already there. > > Further, despite the name similarity these are very different conditions. > > For ctx->singlestep_enabled we emit a debug exception at the end of every > instruction. > > For singlestep, we must execute only one insn in the TB and further we must > return to the main loop after the TB. Both are required for -singlestep -d > cpu to log all that's being requested. > > One cannot combine the two conditions. > > > r~ >
On 06/15/2017 02:57 PM, Philippe Mathieu-Daudé wrote: > Peter cleaned that and improved the doc on ARM: > https://patchwork.kernel.org/patch/9690993/ > (commit b636649f5a2e108413dd171edaf320f781f57942) > > $ git grep -- singlestep_enabled target|wc -l > 113 > still confusing... This is two of the three single step conditions. Not including the split I describe below. r~ >>>> - || ctx->singlestep_enabled || singlestep) { >> >> >> I didn't start testing this global. It's already there. >> >> Further, despite the name similarity these are very different conditions. >> >> For ctx->singlestep_enabled we emit a debug exception at the end of every >> instruction. >> >> For singlestep, we must execute only one insn in the TB and further we must >> return to the main loop after the TB. Both are required for -singlestep -d >> cpu to log all that's being requested. >> >> One cannot combine the two conditions. >> >> >> r~ >>
diff --git a/target/alpha/translate.c b/target/alpha/translate.c index 7c45ae3..a48e451 100644 --- a/target/alpha/translate.c +++ b/target/alpha/translate.c @@ -84,6 +84,7 @@ typedef enum { the PC (for whatever reason), so there's no need to do it again on exiting the TB. */ EXIT_PC_UPDATED, + EXIT_PC_UPDATED_NOCHAIN, /* We are exiting the TB, but have neither emitted a goto_tb, nor updated the PC for the next instruction to be executed. */ @@ -458,11 +459,17 @@ static bool in_superpage(DisasContext *ctx, int64_t addr) #endif } +static bool use_exit_tb(DisasContext *ctx) +{ + return ((ctx->tb->cflags & CF_LAST_IO) + || ctx->singlestep_enabled + || singlestep); +} + static bool use_goto_tb(DisasContext *ctx, uint64_t dest) { /* Suppress goto_tb in the case of single-steping and IO. */ - if ((ctx->tb->cflags & CF_LAST_IO) - || ctx->singlestep_enabled || singlestep) { + if (unlikely(use_exit_tb(ctx))) { return false; } #ifndef CONFIG_USER_ONLY @@ -1198,7 +1205,10 @@ static ExitStatus gen_call_pal(DisasContext *ctx, int palcode) tcg_gen_andi_i64(tmp, ctx->ir[IR_A0], PS_INT_MASK); tcg_gen_st8_i64(tmp, cpu_env, offsetof(CPUAlphaState, ps)); tcg_temp_free(tmp); - break; + + /* Allow interrupts to be recognized right away. */ + tcg_gen_movi_i64(cpu_pc, ctx.pc); + return EXIT_PC_UPDATED_NOCHAIN; case 0x36: /* RDPS */ @@ -1266,7 +1276,7 @@ static ExitStatus gen_call_pal(DisasContext *ctx, int palcode) need the page permissions check. We'll see the existence of the page when we create the TB, and we'll flush all TBs if we change the PAL base register. */ - if (!ctx->singlestep_enabled && !(ctx->tb->cflags & CF_LAST_IO)) { + if (!use_exit_tb(ctx)) { tcg_gen_goto_tb(0); tcg_gen_movi_i64(cpu_pc, entry); tcg_gen_exit_tb((uintptr_t)ctx->tb); @@ -2686,7 +2696,8 @@ static ExitStatus translate_one(DisasContext *ctx, uint32_t insn) tcg_gen_andi_i64(tmp, vb, 1); tcg_gen_st8_i64(tmp, cpu_env, offsetof(CPUAlphaState, pal_mode)); tcg_gen_andi_i64(cpu_pc, vb, ~3); - ret = EXIT_PC_UPDATED; + /* Allow interrupts to be recognized right away. */ + ret = EXIT_PC_UPDATED_NOCHAIN; break; #else goto invalid_opc; @@ -3010,6 +3021,12 @@ void gen_intermediate_code(CPUAlphaState *env, struct TranslationBlock *tb) tcg_gen_movi_i64(cpu_pc, ctx.pc); /* FALLTHRU */ case EXIT_PC_UPDATED: + if (!use_exit_tb(&ctx)) { + tcg_gen_lookup_and_goto_ptr(cpu_pc); + break; + } + /* FALLTHRU */ + case EXIT_PC_UPDATED_NOCHAIN: if (ctx.singlestep_enabled) { gen_excp_1(EXCP_DEBUG, 0); } else {
Signed-off-by: Richard Henderson <rth@twiddle.net> --- target/alpha/translate.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)