Re: [PATCH 02/13] target/riscv: Extend pc for runtime pc write

[Richard Henderson <richard.henderson@linaro.org>]

On 11/1/21 9:48 PM, LIU Zhiwei wrote:
> 
> On 2021/11/1 下午6:33, Richard Henderson wrote:
>> On 11/1/21 6:01 AM, LIU Zhiwei wrote:
>>> In some cases, we must restore the guest PC to the address of the start of
>>> the TB, such as when the instruction counter hit zero. So extend pc register
>>> according to current xlen for these cases.
>>>
>>> Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
>>> ---
>>>   target/riscv/cpu.c        | 20 +++++++++++++++++---
>>>   target/riscv/cpu.h        |  2 ++
>>>   target/riscv/cpu_helper.c |  2 +-
>>>   3 files changed, 20 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>>> index 7d53125dbc..7eefd4f6a6 100644
>>> --- a/target/riscv/cpu.c
>>> +++ b/target/riscv/cpu.c
>>> @@ -319,7 +319,12 @@ static void riscv_cpu_set_pc(CPUState *cs, vaddr value)
>>>   {
>>>       RISCVCPU *cpu = RISCV_CPU(cs);
>>>       CPURISCVState *env = &cpu->env;
>>> -    env->pc = value;
>>> +
>>> +    if (cpu_get_xl(env) == MXL_RV32) {
>>> +        env->pc = (int32_t)value;
>>> +    } else {
>>> +        env->pc = value;
>>> +    }
>>>   }
>>
>> Good.
>>
>>>   static void riscv_cpu_synchronize_from_tb(CPUState *cs,
>>> @@ -327,7 +332,12 @@ static void riscv_cpu_synchronize_from_tb(CPUState *cs,
>>>   {
>>>       RISCVCPU *cpu = RISCV_CPU(cs);
>>>       CPURISCVState *env = &cpu->env;
>>> -    env->pc = tb->pc;
>>> +
>>> +    if (cpu_get_xl(env) == MXL_RV32) {
>>> +        env->pc = (int32_t)tb->pc;
>>> +    } else {
>>> +        env->pc = tb->pc;
>>> +    }
>>
>> Bad, since TB->PC should be extended properly.
>> Though this waits on a change to cpu_get_tb_cpu_state.
> 
> Should the env->pc always hold the sign-extend result? In cpu_get_tb_cpu_state, we just 
> truncate to the XLEN bits.

Oops, I mis-read patch 3; I thought that sign-extended.  Hmm.

I guess we need to zero-extend the pc for patch 3, to get the correct result in translate 
when we read from memory.  Therefore we need to sign-extend here to get the correct value 
back in env->pc.

Oh, let's not re-compute cpu_get_xl here, and restore_state_to_opc; it is in

     FIELD_EX32(tb->flags, TB_FLAGS, XL)


r~