[v2,2/6] target/i386: Use cpu_unwind_state_data for tpr access

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(-)

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>