diff mbox series

[v2,4/6] target/openrisc: Use cpu_unwind_state_data for mfspr

Message ID 20221027100254.215253-5-richard.henderson@linaro.org
State Superseded
Headers show
Series tcg: Fix x86 TARGET_TB_PCREL (#1269) | expand

Commit Message

Richard Henderson Oct. 27, 2022, 10:02 a.m. UTC
Since we do not plan to exit, use cpu_unwind_state_data
and extract exactly the data requested.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/openrisc/sys_helper.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Claudio Fontana Oct. 28, 2022, 9:16 a.m. UTC | #1
On 10/27/22 12:02, Richard Henderson wrote:
> Since we do not plan to exit, use cpu_unwind_state_data
> and extract exactly the data requested.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/openrisc/sys_helper.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/target/openrisc/sys_helper.c b/target/openrisc/sys_helper.c
> index a3508e421d..dde2fa1623 100644
> --- a/target/openrisc/sys_helper.c
> +++ b/target/openrisc/sys_helper.c
> @@ -199,6 +199,7 @@ target_ulong HELPER(mfspr)(CPUOpenRISCState *env, target_ulong rd,
>                             target_ulong spr)
>  {
>  #ifndef CONFIG_USER_ONLY
> +    uint64_t data[TARGET_INSN_START_WORDS];
>      MachineState *ms = MACHINE(qdev_get_machine());
>      OpenRISCCPU *cpu = env_archcpu(env);
>      CPUState *cs = env_cpu(env);
> @@ -232,14 +233,20 @@ target_ulong HELPER(mfspr)(CPUOpenRISCState *env, target_ulong rd,
>          return env->evbar;
>  
>      case TO_SPR(0, 16): /* NPC (equals PC) */
> -        cpu_restore_state(cs, GETPC(), false);
> +        if (cpu_unwind_state_data(cs, GETPC(), data)) {
> +            return data[0];
> +        }
>          return env->pc;
>  
>      case TO_SPR(0, 17): /* SR */
>          return cpu_get_sr(env);
>  
>      case TO_SPR(0, 18): /* PPC */
> -        cpu_restore_state(cs, GETPC(), false);
> +        if (cpu_unwind_state_data(cs, GETPC(), data)) {
> +            if (data[1] & 2) {
> +                return data[0] - 4;
> +            }
> +        }
>          return env->ppc;
>  
>      case TO_SPR(0, 32): /* EPCR */

I am struggling to understand if the fact that we are not setting cpu->env.dflag anymore in the mfspr helper is fine;

here I am unfamiliar with the arch, also Ccing Philippe in case he wants to step in to review this bit.

Thanks,

CLaudio
Richard Henderson Oct. 28, 2022, 6:46 p.m. UTC | #2
On 10/28/22 20:16, Claudio Fontana wrote:
> On 10/27/22 12:02, Richard Henderson wrote:
>> Since we do not plan to exit, use cpu_unwind_state_data
>> and extract exactly the data requested.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   target/openrisc/sys_helper.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/openrisc/sys_helper.c b/target/openrisc/sys_helper.c
>> index a3508e421d..dde2fa1623 100644
>> --- a/target/openrisc/sys_helper.c
>> +++ b/target/openrisc/sys_helper.c
>> @@ -199,6 +199,7 @@ target_ulong HELPER(mfspr)(CPUOpenRISCState *env, target_ulong rd,
>>                              target_ulong spr)
>>   {
>>   #ifndef CONFIG_USER_ONLY
>> +    uint64_t data[TARGET_INSN_START_WORDS];
>>       MachineState *ms = MACHINE(qdev_get_machine());
>>       OpenRISCCPU *cpu = env_archcpu(env);
>>       CPUState *cs = env_cpu(env);
>> @@ -232,14 +233,20 @@ target_ulong HELPER(mfspr)(CPUOpenRISCState *env, target_ulong rd,
>>           return env->evbar;
>>   
>>       case TO_SPR(0, 16): /* NPC (equals PC) */
>> -        cpu_restore_state(cs, GETPC(), false);
>> +        if (cpu_unwind_state_data(cs, GETPC(), data)) {
>> +            return data[0];
>> +        }
>>           return env->pc;
>>   
>>       case TO_SPR(0, 17): /* SR */
>>           return cpu_get_sr(env);
>>   
>>       case TO_SPR(0, 18): /* PPC */
>> -        cpu_restore_state(cs, GETPC(), false);
>> +        if (cpu_unwind_state_data(cs, GETPC(), data)) {
>> +            if (data[1] & 2) {
>> +                return data[0] - 4;
>> +            }
>> +        }
>>           return env->ppc;
>>   
>>       case TO_SPR(0, 32): /* EPCR */
> 
> I am struggling to understand if the fact that we are not setting cpu->env.dflag anymore in the mfspr helper is fine;

That we might do so before was buggy.  We manage dflag in openrisc_tr_tb_stop expecting a 
particular value.  I should expand the patch comment on this.

Consider:

	l.j	L2	// branch
	l.mfspr r1, ppc	// delay

L1:	boom
L2:	l.lwa	r3, (r4)

With  "l.mfspr reg, ppc" executed in a delay slot, dflag would be set, but it would not be 
cleared by openrisc_tr_tb_stop on exiting the TB, because tb_stop thinks the value is 
already zero.

The next TB begins at L2 with dflag set when it should not.  If the load has a tlb miss, 
then the interrupt will be delivered with DSX set in the status register, and the pc 
decremented (openrisc implements restart after delay slot by re-executing the branch). 
This will cause the return from interrupt to go to L1, and boom!


r~
diff mbox series

Patch

diff --git a/target/openrisc/sys_helper.c b/target/openrisc/sys_helper.c
index a3508e421d..dde2fa1623 100644
--- a/target/openrisc/sys_helper.c
+++ b/target/openrisc/sys_helper.c
@@ -199,6 +199,7 @@  target_ulong HELPER(mfspr)(CPUOpenRISCState *env, target_ulong rd,
                            target_ulong spr)
 {
 #ifndef CONFIG_USER_ONLY
+    uint64_t data[TARGET_INSN_START_WORDS];
     MachineState *ms = MACHINE(qdev_get_machine());
     OpenRISCCPU *cpu = env_archcpu(env);
     CPUState *cs = env_cpu(env);
@@ -232,14 +233,20 @@  target_ulong HELPER(mfspr)(CPUOpenRISCState *env, target_ulong rd,
         return env->evbar;
 
     case TO_SPR(0, 16): /* NPC (equals PC) */
-        cpu_restore_state(cs, GETPC(), false);
+        if (cpu_unwind_state_data(cs, GETPC(), data)) {
+            return data[0];
+        }
         return env->pc;
 
     case TO_SPR(0, 17): /* SR */
         return cpu_get_sr(env);
 
     case TO_SPR(0, 18): /* PPC */
-        cpu_restore_state(cs, GETPC(), false);
+        if (cpu_unwind_state_data(cs, GETPC(), data)) {
+            if (data[1] & 2) {
+                return data[0] - 4;
+            }
+        }
         return env->ppc;
 
     case TO_SPR(0, 32): /* EPCR */