| Message ID | 20170707161822.29659-1-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
Alex Bennée <alex.bennee@linaro.org> writes: > Previously DISAS_JUMP did ensure this but with the optimisation of > 8a6b28c7 (optimize indirect branches) we might not leave the loop. > This means if any pending interrupts are cleared by changing IRQ flags > we might never get around to servicing them. You usually notice this > by seeing the lookup_tb_ptr() helper gainfully chaining TBs together > while cpu->interrupt_request remains high and the exit_request has not > been set. > > This breaks amongst other things the OPTEE test suite which executes > an eret from the secure world after a non-secure world IRQ has gone > pending which then never gets serviced. > > An alternate approach might be for the exception helpers to ensure the > exit request flag is set if an IRQ is now unmasked. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > CC: Etienne Carriere <etienne.carriere@linaro.org> > CC: Joakim Bech <joakim.bech@linaro.org> > CC: Peter Maydell <peter.maydell@linaro.org> > CC: Emilio G. Cota <cota@braap.org> > CC: Richard Henderson <rth@twiddle.net> > --- > target/arm/translate-a64.c | 3 ++- > target/arm/translate.c | 6 ++++-- > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c > index e55547d95d..3ee88b2590 100644 > --- a/target/arm/translate-a64.c > +++ b/target/arm/translate-a64.c > @@ -1788,7 +1788,8 @@ static void disas_uncond_b_reg(DisasContext *s, uint32_t insn) > return; > } > gen_helper_exception_return(cpu_env); > - s->is_jmp = DISAS_JUMP; > + /* Must exit loop to check un-masked IRQs */ > + s->is_jmp = DISAS_EXIT; Given the wording of: /* is_jmp field values */ #define DISAS_NEXT 0 /* next instruction can be analyzed */ #define DISAS_JUMP 1 /* only pc was modified dynamically */ #define DISAS_UPDATE 2 /* cpu state was modified dynamically */ #define DISAS_TB_JUMP 3 /* only pc was modified statically */ I'm thinking that really these DISAS_JUMP's should be DISAS_UPDATEs and we need to disable the TB chaining optimisations for this. I'll prepare a more comprehensive series next week. However testing this patch for known failure modes will be useful. > return; > case 5: /* DRPS */ > if (rn != 0x1f) { > diff --git a/target/arm/translate.c b/target/arm/translate.c > index 0862f9e4aa..920fb41395 100644 > --- a/target/arm/translate.c > +++ b/target/arm/translate.c > @@ -4475,7 +4475,8 @@ static void gen_rfe(DisasContext *s, TCGv_i32 pc, TCGv_i32 cpsr) > */ > gen_helper_cpsr_write_eret(cpu_env, cpsr); > tcg_temp_free_i32(cpsr); > - s->is_jmp = DISAS_JUMP; > + /* Must exit loop to check un-masked IRQs */ > + s->is_jmp = DISAS_EXIT; > } > > /* Generate an old-style exception return. Marks pc as dead. */ > @@ -9519,7 +9520,8 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn) > tmp = load_cpu_field(spsr); > gen_helper_cpsr_write_eret(cpu_env, tmp); > tcg_temp_free_i32(tmp); > - s->is_jmp = DISAS_JUMP; > + /* Must exit loop to check un-masked IRQs */ > + s->is_jmp = DISAS_EXIT; > } > } > break; -- Alex Bennée
On 7 July 2017 at 18:32, Alex Bennée <alex.bennee@linaro.org> wrote: > Given the wording of: > > /* is_jmp field values */ > #define DISAS_NEXT 0 /* next instruction can be analyzed */ > #define DISAS_JUMP 1 /* only pc was modified dynamically */ > #define DISAS_UPDATE 2 /* cpu state was modified dynamically */ > #define DISAS_TB_JUMP 3 /* only pc was modified statically */ > > I'm thinking that really these DISAS_JUMP's should be DISAS_UPDATEs and > we need to disable the TB chaining optimisations for this. I'll prepare > a more comprehensive series next week. However testing this patch for > known failure modes will be useful. The problem I think is that a lot of our current code assumes that DISAS_UPDATE means "go back to the top loop". In any case I think you are right that DISAS_UPDATE should not allow chaining to the next TB, because it implies that perhaps a TB flags bit got changed, so you need to look in the hash table again. thanks -- PMM
On 07/07/2017 07:36 AM, Peter Maydell wrote: > On 7 July 2017 at 18:32, Alex Bennée <alex.bennee@linaro.org> wrote: >> Given the wording of: >> >> /* is_jmp field values */ >> #define DISAS_NEXT 0 /* next instruction can be analyzed */ >> #define DISAS_JUMP 1 /* only pc was modified dynamically */ >> #define DISAS_UPDATE 2 /* cpu state was modified dynamically */ >> #define DISAS_TB_JUMP 3 /* only pc was modified statically */ >> >> I'm thinking that really these DISAS_JUMP's should be DISAS_UPDATEs and >> we need to disable the TB chaining optimisations for this. I'll prepare >> a more comprehensive series next week. However testing this patch for >> known failure modes will be useful. > > The problem I think is that a lot of our current code assumes > that DISAS_UPDATE means "go back to the top loop". > > In any case I think you are right that DISAS_UPDATE should not > allow chaining to the next TB, because it implies that perhaps > a TB flags bit got changed, so you need to look in the hash > table again. Changes to TB flags are not a problem; helper_lookup_tb does examine them, and indeed go back to the hash table. The only thing it doesn't do is re-evaluate the interrupt state. Naming all of these different exit conditions is certainly non-trivial. r~
Richard Henderson <rth@twiddle.net> writes: > On 07/07/2017 07:36 AM, Peter Maydell wrote: >> On 7 July 2017 at 18:32, Alex Bennée <alex.bennee@linaro.org> wrote: >>> Given the wording of: >>> >>> /* is_jmp field values */ >>> #define DISAS_NEXT 0 /* next instruction can be analyzed */ >>> #define DISAS_JUMP 1 /* only pc was modified dynamically */ >>> #define DISAS_UPDATE 2 /* cpu state was modified dynamically */ >>> #define DISAS_TB_JUMP 3 /* only pc was modified statically */ >>> >>> I'm thinking that really these DISAS_JUMP's should be DISAS_UPDATEs and >>> we need to disable the TB chaining optimisations for this. I'll prepare >>> a more comprehensive series next week. However testing this patch for >>> known failure modes will be useful. >> >> The problem I think is that a lot of our current code assumes >> that DISAS_UPDATE means "go back to the top loop". >> >> In any case I think you are right that DISAS_UPDATE should not >> allow chaining to the next TB, because it implies that perhaps >> a TB flags bit got changed, so you need to look in the hash >> table again. > > Changes to TB flags are not a problem; helper_lookup_tb does examine > them, and indeed go back to the hash table. The only thing it doesn't > do is re-evaluate the interrupt state. Last time around we I did have a patch that checked for cpu->interrupt_request in the lookup helper. Of course this is a bit blunt because really its a case of is the cpu->interrupt_request live and the masking has changed to enable things to be run. Unfortunately the current API of cc->cpu_exec_interrupt() isn't amenable to querying the state so maybe we should just exit back to the prologue when the request is up. > Naming all of these different exit conditions is certainly > non-trivial. Given the variation of usage this is something that should probably be done after Lluís common run loop goes in and we can beef up the semantics of the various exit conditions. One thing I have noticed in the ARM translator is DISAS_UPDATE does a: gen_a64_set_pc_im(dc->pc); I think this is to deal with handling exceptions either side of various instructions. Am I right in thinking this is superfluous now as we can derive the PC from the translated code address? > > > r~ -- Alex Bennée
On 07/07/2017 08:29 AM, Alex Bennée wrote: >> Naming all of these different exit conditions is certainly >> non-trivial. > > Given the variation of usage this is something that should probably be > done after Lluís common run loop goes in and we can beef up the > semantics of the various exit conditions. Definitely. > One thing I have noticed in the ARM translator is DISAS_UPDATE does a: > > gen_a64_set_pc_im(dc->pc); > > I think this is to deal with handling exceptions either side of various > instructions. Am I right in thinking this is superfluous now as we can > derive the PC from the translated code address? Yes and no. We have typically distinguished between two kinds of exceptions: those that are dynamic (fp state, page permissions) and those that are static (illegal opcodes). For the dynamic, we used to pessimistically save state, but now use the unwinder to restore it. The unwinding is expensive but is used infrequently (especially compared to the number of load/store insns executed). For the static, we know the exception must be raised, and we know the state that must be saved. By doing that, we save the expense of the unwinding. So, for the static case you're talking about, we could get the PC (and other state) back, and remove the explicit stores, but we shouldn't. r~
Hi Alex, On 07/07/2017 03:52 PM, Richard Henderson wrote: > On 07/07/2017 08:29 AM, Alex Bennée wrote: >>> Naming all of these different exit conditions is certainly >>> non-trivial. >> >> Given the variation of usage this is something that should probably be >> done after Lluís common run loop goes in and we can beef up the >> semantics of the various exit conditions. > > Definitely. > >> One thing I have noticed in the ARM translator is DISAS_UPDATE does a: >> >> gen_a64_set_pc_im(dc->pc); >> >> I think this is to deal with handling exceptions either side of various >> instructions. Am I right in thinking this is superfluous now as we can >> derive the PC from the translated code address? > > Yes and no. > > We have typically distinguished between two kinds of exceptions: those > that are dynamic (fp state, page permissions) and those that are static > (illegal opcodes). > > For the dynamic, we used to pessimistically save state, but now use the > unwinder to restore it. The unwinding is expensive but is used > infrequently (especially compared to the number of load/store insns > executed). > > For the static, we know the exception must be raised, and we know the > state that must be saved. By doing that, we save the expense of the > unwinding. > > So, for the static case you're talking about, we could get the PC (and > other state) back, and remove the explicit stores, but we shouldn't. While doing changes in this file, can you add a comment around DISAS_UPDATE about Richard explanation? > > > r~ >
Richard Henderson <rth@twiddle.net> writes: > On 07/07/2017 08:29 AM, Alex Bennée wrote: >>> Naming all of these different exit conditions is certainly >>> non-trivial. >> >> Given the variation of usage this is something that should probably be >> done after Lluís common run loop goes in and we can beef up the >> semantics of the various exit conditions. > > Definitely. > >> One thing I have noticed in the ARM translator is DISAS_UPDATE does a: >> >> gen_a64_set_pc_im(dc->pc); >> >> I think this is to deal with handling exceptions either side of various >> instructions. Am I right in thinking this is superfluous now as we can >> derive the PC from the translated code address? > > Yes and no. > > We have typically distinguished between two kinds of exceptions: those > that are dynamic (fp state, page permissions) and those that are > static (illegal opcodes). > > For the dynamic, we used to pessimistically save state, but now use > the unwinder to restore it. The unwinding is expensive but is used > infrequently (especially compared to the number of load/store insns > executed). > > For the static, we know the exception must be raised, and we know the > state that must be saved. By doing that, we save the expense of the > unwinding. > > So, for the static case you're talking about, we could get the PC (and > other state) back, and remove the explicit stores, but we shouldn't. I'm thinking of how to bring this into line with the other translators. Pretty much everyone else generates an exit block although m68k does do a update_cc_op(dc); In the ARM translator we have the backend specific DISAS_EXIT which behaves like DISAS_UPDATE everywhere else. For consistency DISAS_UPDATE should work the same across all arches but doing: gen_set_pc_im(dc, dc->pc); if we are going to use DISAS_UPDATE instead of DISAS_EXIT is going to break things like eret as dc->pc will certainly not be the correct PC. Looking at translate.c (32 bit arm), we have: gen_srs gen_mrs_banked gen_msr_banked These all manually set: gen_set_pc_im(s, s->pc - 4); before their respective helpers. I think setting the PC after the helper is superfluous given we are will at that point be exiting the block. The situation is the same for translate-a64.c. So I think the correct changes are: - strengthen commentary in exec-all.h - don't set pc on our way out of a DISAS_UPDATE - convert eret to DISAS_UPDATE - get rid of DISAS_EXIT and use DISAS_UPDATE instead My only worry is inadvertently breaking something because something does need the post-foo set_pc. But I can't think what does. Peter? -- Alex Bennée
On 10 July 2017 at 13:15, Alex Bennée <alex.bennee@linaro.org> wrote: > Looking at translate.c (32 bit arm), we have: > > gen_srs > gen_mrs_banked > gen_msr_banked > > These all manually set: > > gen_set_pc_im(s, s->pc - 4); > > before their respective helpers. I think setting the PC after the helper > is superfluous given we are will at that point be exiting the block. No, you need both. We do gen_set_pc_im(s, s->pc - 4); before calling the helper because the helper might throw an exception, in which case the PC needs to point to that insn. We then call gen_set_pc_im(dc, dc->pc); before exiting the block because if we leave execution by falling off the end of the block then the PC should point to the insn that comes next (ie just after the last one in the block) thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On 10 July 2017 at 13:15, Alex Bennée <alex.bennee@linaro.org> wrote: >> Looking at translate.c (32 bit arm), we have: >> >> gen_srs >> gen_mrs_banked >> gen_msr_banked >> >> These all manually set: >> >> gen_set_pc_im(s, s->pc - 4); >> >> before their respective helpers. I think setting the PC after the helper >> is superfluous given we are will at that point be exiting the block. > > No, you need both. We do > gen_set_pc_im(s, s->pc - 4); > before calling the helper because the helper might throw an > exception, in which case the PC needs to point to that insn. > We then call > gen_set_pc_im(dc, dc->pc); > before exiting the block because if we leave execution by > falling off the end of the block then the PC should point to > the insn that comes next (ie just after the last one in the block) OK I see now. I think I have a plan going forward. > > thanks > -- PMM -- Alex Bennée
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c index e55547d95d..3ee88b2590 100644 --- a/target/arm/translate-a64.c +++ b/target/arm/translate-a64.c @@ -1788,7 +1788,8 @@ static void disas_uncond_b_reg(DisasContext *s, uint32_t insn) return; } gen_helper_exception_return(cpu_env); - s->is_jmp = DISAS_JUMP; + /* Must exit loop to check un-masked IRQs */ + s->is_jmp = DISAS_EXIT; return; case 5: /* DRPS */ if (rn != 0x1f) { diff --git a/target/arm/translate.c b/target/arm/translate.c index 0862f9e4aa..920fb41395 100644 --- a/target/arm/translate.c +++ b/target/arm/translate.c @@ -4475,7 +4475,8 @@ static void gen_rfe(DisasContext *s, TCGv_i32 pc, TCGv_i32 cpsr) */ gen_helper_cpsr_write_eret(cpu_env, cpsr); tcg_temp_free_i32(cpsr); - s->is_jmp = DISAS_JUMP; + /* Must exit loop to check un-masked IRQs */ + s->is_jmp = DISAS_EXIT; } /* Generate an old-style exception return. Marks pc as dead. */ @@ -9519,7 +9520,8 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn) tmp = load_cpu_field(spsr); gen_helper_cpsr_write_eret(cpu_env, tmp); tcg_temp_free_i32(tmp); - s->is_jmp = DISAS_JUMP; + /* Must exit loop to check un-masked IRQs */ + s->is_jmp = DISAS_EXIT; } } break;
Previously DISAS_JUMP did ensure this but with the optimisation of 8a6b28c7 (optimize indirect branches) we might not leave the loop. This means if any pending interrupts are cleared by changing IRQ flags we might never get around to servicing them. You usually notice this by seeing the lookup_tb_ptr() helper gainfully chaining TBs together while cpu->interrupt_request remains high and the exit_request has not been set. This breaks amongst other things the OPTEE test suite which executes an eret from the secure world after a non-secure world IRQ has gone pending which then never gets serviced. An alternate approach might be for the exception helpers to ensure the exit request flag is set if an IRQ is now unmasked. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> CC: Etienne Carriere <etienne.carriere@linaro.org> CC: Joakim Bech <joakim.bech@linaro.org> CC: Peter Maydell <peter.maydell@linaro.org> CC: Emilio G. Cota <cota@braap.org> CC: Richard Henderson <rth@twiddle.net> --- target/arm/translate-a64.c | 3 ++- target/arm/translate.c | 6 ++++-- 2 files changed, 6 insertions(+), 3 deletions(-)