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.hender...@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 */
