| Message ID | 20221027100254.215253-3-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tcg: Fix x86 TARGET_TB_PCREL (#1269) | expand |
On 10/27/22 12:02, Richard Henderson wrote: > Avoid cpu_restore_state, and modifying env->eip out from > underneath the translator with TARGET_TB_PCREL. There is > some slight duplication from x86_restore_state_to_opc, > but it's just a few lines. > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1269 > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/i386/helper.c | 21 +++++++++++++++++++-- > 1 file changed, 19 insertions(+), 2 deletions(-) > > diff --git a/target/i386/helper.c b/target/i386/helper.c > index b62a1e48e2..2cd1756f1a 100644 > --- a/target/i386/helper.c > +++ b/target/i386/helper.c > @@ -509,6 +509,23 @@ void cpu_x86_inject_mce(Monitor *mon, X86CPU *cpu, int bank, > } > } > > +static target_ulong get_memio_eip(CPUX86State *env) > +{ > + uint64_t data[TARGET_INSN_START_WORDS]; > + CPUState *cs = env_cpu(env); > + > + if (!cpu_unwind_state_data(cs, cs->mem_io_pc, data)) { > + return env->eip; > + } > + > + /* Per x86_restore_state_to_opc. */ > + if (TARGET_TB_PCREL) { > + return (env->eip & TARGET_PAGE_MASK) | data[0]; > + } else { > + return data[0] - env->segs[R_CS].base; here we switch from taking cs_base from the TranslationBlock to taking it from env-> . I traced the tb->cs_base use back to cpu_exec() and cpu_exec_step_atomic() and from there it seems ok, as the sequence is cpu_get_tb_cpu_state(cpu->env_ptr, &pc, &cs_base, &flags), which gets it from env, followed by tb_gen_code(...cs_base) which sets the TranslationBlock cs_base, and tb->cs_base does not seem to change again. I mention this in the case there can be some weird situation in which env and tb can end up not being consistent, does a TranslationBlock that is initialized with a certain cs_base from the env that contains user code to load / change the CS segment base potentially constitute a problem? Ciao, Claudio > + } > +} > + > void cpu_report_tpr_access(CPUX86State *env, TPRAccess access) > { > X86CPU *cpu = env_archcpu(env); > @@ -519,9 +536,9 @@ void cpu_report_tpr_access(CPUX86State *env, TPRAccess access) > > cpu_interrupt(cs, CPU_INTERRUPT_TPR); > } else if (tcg_enabled()) { > - cpu_restore_state(cs, cs->mem_io_pc, false); > + target_ulong eip = get_memio_eip(env); > > - apic_handle_tpr_access_report(cpu->apic_state, env->eip, access); > + apic_handle_tpr_access_report(cpu->apic_state, eip, access); > } > } > #endif /* !CONFIG_USER_ONLY */
On 10/27/22 22:22, Claudio Fontana wrote: > On 10/27/22 12:02, Richard Henderson wrote: >> + /* Per x86_restore_state_to_opc. */ >> + if (TARGET_TB_PCREL) { >> + return (env->eip & TARGET_PAGE_MASK) | data[0]; >> + } else { >> + return data[0] - env->segs[R_CS].base; > > here we switch from taking cs_base from the TranslationBlock to taking it from env-> . > > I traced the tb->cs_base use back to > > cpu_exec() and cpu_exec_step_atomic() > > and from there it seems ok, as the sequence is > > cpu_get_tb_cpu_state(cpu->env_ptr, &pc, &cs_base, &flags), which gets it from env, > > followed by > > tb_gen_code(...cs_base) which sets the TranslationBlock cs_base, and tb->cs_base does > not seem to change again. Correct. I wondered if I'd made a mistake by not returning the TB located during the search, but it doesn't seem to have mattered for the two users. > I mention this in the case there can be some weird situation in which env and tb can > end up not being consistent, does a TranslationBlock that is initialized with a certain > cs_base from the env that contains user code to load / change the CS segment base > potentially constitute a problem? The only way to load/change a CS segment base is a branch instruction, which will of course end the TB. There should be no way to change CS that continues the TB. r~
On 10/27/22 22:13, Richard Henderson wrote: > On 10/27/22 22:22, Claudio Fontana wrote: >> On 10/27/22 12:02, Richard Henderson wrote: >>> + /* Per x86_restore_state_to_opc. */ >>> + if (TARGET_TB_PCREL) { >>> + return (env->eip & TARGET_PAGE_MASK) | data[0]; >>> + } else { >>> + return data[0] - env->segs[R_CS].base; >> >> here we switch from taking cs_base from the TranslationBlock to taking it from env-> . >> >> I traced the tb->cs_base use back to >> >> cpu_exec() and cpu_exec_step_atomic() >> >> and from there it seems ok, as the sequence is >> >> cpu_get_tb_cpu_state(cpu->env_ptr, &pc, &cs_base, &flags), which gets it from env, >> >> followed by >> >> tb_gen_code(...cs_base) which sets the TranslationBlock cs_base, and tb->cs_base does >> not seem to change again. > Correct. I wondered if I'd made a mistake by not returning the TB located during the > search, but it doesn't seem to have mattered for the two users. > >> I mention this in the case there can be some weird situation in which env and tb can >> end up not being consistent, does a TranslationBlock that is initialized with a certain >> cs_base from the env that contains user code to load / change the CS segment base >> potentially constitute a problem? > The only way to load/change a CS segment base is a branch instruction, which will of > course end the TB. There should be no way to change CS that continues the TB. > > > r~ Reviewed-by: Claudio Fontana <cfontana@suse.de>
diff --git a/target/i386/helper.c b/target/i386/helper.c index b62a1e48e2..2cd1756f1a 100644 --- a/target/i386/helper.c +++ b/target/i386/helper.c @@ -509,6 +509,23 @@ void cpu_x86_inject_mce(Monitor *mon, X86CPU *cpu, int bank, } } +static target_ulong get_memio_eip(CPUX86State *env) +{ + uint64_t data[TARGET_INSN_START_WORDS]; + CPUState *cs = env_cpu(env); + + if (!cpu_unwind_state_data(cs, cs->mem_io_pc, data)) { + return env->eip; + } + + /* Per x86_restore_state_to_opc. */ + if (TARGET_TB_PCREL) { + return (env->eip & TARGET_PAGE_MASK) | data[0]; + } else { + return data[0] - env->segs[R_CS].base; + } +} + void cpu_report_tpr_access(CPUX86State *env, TPRAccess access) { X86CPU *cpu = env_archcpu(env); @@ -519,9 +536,9 @@ void cpu_report_tpr_access(CPUX86State *env, TPRAccess access) cpu_interrupt(cs, CPU_INTERRUPT_TPR); } else if (tcg_enabled()) { - cpu_restore_state(cs, cs->mem_io_pc, false); + target_ulong eip = get_memio_eip(env); - apic_handle_tpr_access_report(cpu->apic_state, env->eip, access); + apic_handle_tpr_access_report(cpu->apic_state, eip, access); } } #endif /* !CONFIG_USER_ONLY */
Avoid cpu_restore_state, and modifying env->eip out from underneath the translator with TARGET_TB_PCREL. There is some slight duplication from x86_restore_state_to_opc, but it's just a few lines. Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1269 Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/i386/helper.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)