Peter Maydell <peter.mayd...@linaro.org> writes: > On 1 February 2017 at 15:05, Alex Bennée <alex.ben...@linaro.org> wrote: >> Some helpers may trigger an immediate exit of the cpu_loop. If this >> happens the PC need to be rectified to ensure the restart will begin >> on the next instruction. >> >> Signed-off-by: Alex Bennée <alex.ben...@linaro.org> >> --- >> target/arm/cpu.h | 3 ++- >> target/arm/translate-a64.c | 4 ++++ >> target/arm/translate.c | 4 ++++ >> 3 files changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/target/arm/cpu.h b/target/arm/cpu.h >> index d61793ca06..a3c4d07817 100644 >> --- a/target/arm/cpu.h >> +++ b/target/arm/cpu.h >> @@ -1465,7 +1465,8 @@ static inline uint64_t cpreg_to_kvm_id(uint32_t >> cpregid) >> #define ARM_CP_NZCV (ARM_CP_SPECIAL | (3 << 8)) >> #define ARM_CP_CURRENTEL (ARM_CP_SPECIAL | (4 << 8)) >> #define ARM_CP_DC_ZVA (ARM_CP_SPECIAL | (5 << 8)) >> -#define ARM_LAST_SPECIAL ARM_CP_DC_ZVA >> +#define ARM_CP_EXIT_PC (ARM_CP_SPECIAL | (6 << 8)) >> +#define ARM_LAST_SPECIAL ARM_CP_EXIT_PC > > This shouldn't be a "special", because those are for > "this is a special case that is handled entirely in the translate > code", not "I need some extra behaviour on the code generated > for calling the helper functions" (which is what the > plain non-special ARM_CP flags do). Notice that all the other > "special" cases completely define the behaviour of the cp that > uses them, and the code implementing them ends the case > statement with "return", not "break". > > Missing documentation comment change.
I posted this before you commented on the last version. Anyway see bellow. > > That said, I'm definitely becoming more strongly of the > opinion that longjumping out of this helper is not the > best way to implement this, so these remarks are a bit moot. Yep the tree: https://github.com/stsquad/qemu/commits/mttcg/base-patches-v10 Reverts the this change and changes the cputlb flush code to return and let the guest translation code exit the normal way. I was hoping to get some feedback from Paolo and Richard before I roll the fixes together and post v10 which will be later today. > >> /* Used only as a terminator for ARMCPRegInfo lists */ >> #define ARM_CP_SENTINEL 0xffff >> /* Mask of only the flag bits in a type field */ >> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c >> index 7e7131fe2f..98d4fac070 100644 >> --- a/target/arm/translate-a64.c >> +++ b/target/arm/translate-a64.c >> @@ -1561,6 +1561,10 @@ static void handle_sys(DisasContext *s, uint32_t >> insn, bool isread, >> tcg_rt = cpu_reg(s, rt); >> gen_helper_dc_zva(cpu_env, tcg_rt); >> return; >> + case ARM_CP_EXIT_PC: >> + /* The helper may exit the cpu_loop so ensure PC is correct */ >> + gen_a64_set_pc_im(s->pc); >> + break; >> default: >> break; >> } >> diff --git a/target/arm/translate.c b/target/arm/translate.c >> index 24faa7c60c..e1f4a48720 100644 >> --- a/target/arm/translate.c >> +++ b/target/arm/translate.c >> @@ -7510,6 +7510,10 @@ static int disas_coproc_insn(DisasContext *s, >> uint32_t insn) >> gen_set_pc_im(s, s->pc); >> s->is_jmp = DISAS_WFI; >> return 0; >> + case ARM_CP_EXIT_PC: >> + /* The helper may exit the cpu_loop so ensure PC is correct */ >> + gen_set_pc_im(s, s->pc); >> + break; >> default: >> break; >> } > > thanks > -- PMM -- Alex Bennée
