diff mbox series

[v2,8/8] target/ppc: 74xx: Set SRRs directly in exception code

Message ID 20220127201116.1154733-9-farosas@linux.ibm.com
State New
Headers show
Series target/ppc: powerpc_excp improvements [74xx] (5/n) | expand

Commit Message

Fabiano Rosas Jan. 27, 2022, 8:11 p.m. UTC 
The 74xx does not have alternate/hypervisor Save and Restore
Registers, so we can set SRR0 and SRR1 directly.

Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
---
 target/ppc/excp_helper.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

Comments

BALATON Zoltan Jan. 27, 2022, 10:28 p.m. UTC | #1 
On Thu, 27 Jan 2022, Fabiano Rosas wrote:
> The 74xx does not have alternate/hypervisor Save and Restore
> Registers, so we can set SRR0 and SRR1 directly.
>
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> ---
> target/ppc/excp_helper.c | 13 ++-----------
> 1 file changed, 2 insertions(+), 11 deletions(-)
>
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index b7921c0956..4e6bb87b70 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -556,7 +556,6 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
>     CPUState *cs = CPU(cpu);
>     CPUPPCState *env = &cpu->env;
>     target_ulong msr, new_msr, vector;
> -    int srr0, srr1;
>
>     if (excp <= POWERPC_EXCP_NONE || excp >= POWERPC_EXCP_NB) {
>         cpu_abort(cs, "Invalid PowerPC exception %d. Aborting\n", excp);
> @@ -575,10 +574,6 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
>      */
>     new_msr = env->msr & ((target_ulong)1 << MSR_ME);
>
> -    /* target registers */
> -    srr0 = SPR_SRR0;
> -    srr1 = SPR_SRR1;
> -
>     /*
>      * Hypervisor emulation assistance interrupt only exists on server
>      * arch 2.05 server or later.
> @@ -731,10 +726,6 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
>             cpu_abort(cs, "Trying to deliver HV exception (MSR) %d with "
>                       "no HV support\n", excp);
>         }

If we have ho MSR_HVB why is this still here? Shouldn't it have been gone 
in patch 2? Or is this still reachable?

Regards,
BALATON Zoltan

> -        if (srr0 == SPR_HSRR0) {
> -            cpu_abort(cs, "Trying to deliver HV exception (HSRR) %d with "
> -                      "no HV support\n", excp);
> -        }
>     }
>
>     /*
> @@ -746,10 +737,10 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
>     }
>
>     /* Save PC */
> -    env->spr[srr0] = env->nip;
> +    env->spr[SPR_SRR0] = env->nip;
>
>     /* Save MSR */
> -    env->spr[srr1] = msr;
> +    env->spr[SPR_SRR1] = msr;
>
>     powerpc_set_excp_state(cpu, vector, new_msr);
> }
>
BALATON Zoltan Jan. 27, 2022, 10:37 p.m. UTC | #2 
On Thu, 27 Jan 2022, BALATON Zoltan wrote:
> On Thu, 27 Jan 2022, Fabiano Rosas wrote:
>> The 74xx does not have alternate/hypervisor Save and Restore
>> Registers, so we can set SRR0 and SRR1 directly.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
>> ---
>> target/ppc/excp_helper.c | 13 ++-----------
>> 1 file changed, 2 insertions(+), 11 deletions(-)
>> 
>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>> index b7921c0956..4e6bb87b70 100644
>> --- a/target/ppc/excp_helper.c
>> +++ b/target/ppc/excp_helper.c
>> @@ -556,7 +556,6 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int 
>> excp)
>>     CPUState *cs = CPU(cpu);
>>     CPUPPCState *env = &cpu->env;
>>     target_ulong msr, new_msr, vector;
>> -    int srr0, srr1;
>>
>>     if (excp <= POWERPC_EXCP_NONE || excp >= POWERPC_EXCP_NB) {
>>         cpu_abort(cs, "Invalid PowerPC exception %d. Aborting\n", excp);
>> @@ -575,10 +574,6 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int 
>> excp)
>>      */
>>     new_msr = env->msr & ((target_ulong)1 << MSR_ME);
>> 
>> -    /* target registers */
>> -    srr0 = SPR_SRR0;
>> -    srr1 = SPR_SRR1;
>> -
>>     /*
>>      * Hypervisor emulation assistance interrupt only exists on server
>>      * arch 2.05 server or later.
>> @@ -731,10 +726,6 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int 
>> excp)
>>             cpu_abort(cs, "Trying to deliver HV exception (MSR) %d with "
>>                       "no HV support\n", excp);
>>         }
>
> If we have ho MSR_HVB why is this still here? Shouldn't it have been gone in 
> patch 2? Or is this still reachable?

Additionally if it's still needed then the two ifs could be collapsed into 
one with && now that the other branch below is removed.

Regards,
BALATON Zoltan
>
>> -        if (srr0 == SPR_HSRR0) {
>> -            cpu_abort(cs, "Trying to deliver HV exception (HSRR) %d with "
>> -                      "no HV support\n", excp);
>> -        }
>>     }
>>
>>     /*
>> @@ -746,10 +737,10 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int 
>> excp)
>>     }
>>
>>     /* Save PC */
>> -    env->spr[srr0] = env->nip;
>> +    env->spr[SPR_SRR0] = env->nip;
>>
>>     /* Save MSR */
>> -    env->spr[srr1] = msr;
>> +    env->spr[SPR_SRR1] = msr;
>>
>>     powerpc_set_excp_state(cpu, vector, new_msr);
>> }
>> 
>
>
Fabiano Rosas Jan. 27, 2022, 10:52 p.m. UTC | #3 
BALATON Zoltan <balaton@eik.bme.hu> writes:

> On Thu, 27 Jan 2022, Fabiano Rosas wrote:
>> The 74xx does not have alternate/hypervisor Save and Restore
>> Registers, so we can set SRR0 and SRR1 directly.
>>
>> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
>> ---
>> target/ppc/excp_helper.c | 13 ++-----------
>> 1 file changed, 2 insertions(+), 11 deletions(-)
>>
>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>> index b7921c0956..4e6bb87b70 100644
>> --- a/target/ppc/excp_helper.c
>> +++ b/target/ppc/excp_helper.c
>> @@ -556,7 +556,6 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
>>     CPUState *cs = CPU(cpu);
>>     CPUPPCState *env = &cpu->env;
>>     target_ulong msr, new_msr, vector;
>> -    int srr0, srr1;
>>
>>     if (excp <= POWERPC_EXCP_NONE || excp >= POWERPC_EXCP_NB) {
>>         cpu_abort(cs, "Invalid PowerPC exception %d. Aborting\n", excp);
>> @@ -575,10 +574,6 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
>>      */
>>     new_msr = env->msr & ((target_ulong)1 << MSR_ME);
>>
>> -    /* target registers */
>> -    srr0 = SPR_SRR0;
>> -    srr1 = SPR_SRR1;
>> -
>>     /*
>>      * Hypervisor emulation assistance interrupt only exists on server
>>      * arch 2.05 server or later.
>> @@ -731,10 +726,6 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
>>             cpu_abort(cs, "Trying to deliver HV exception (MSR) %d with "
>>                       "no HV support\n", excp);
>>         }
>
> If we have ho MSR_HVB why is this still here? Shouldn't it have been gone 
> in patch 2? Or is this still reachable?

It is still reachable. Any of the individual exceptions above could set
the wrong bit. I have been keeping this block for all CPUs because I
intend to extract this sanity check to an outer function after I move
all CPU families.

In the long run I think we should validate MSR against the whole
msr_mask instead of just checking this single bit. But I am not
confident that today all the bits that are set are also present in the
corresponding location in the msr_mask.

>> -        If (srr0 == SPR_HSRR0) {
>> -            cpu_abort(cs, "Trying to deliver HV exception (HSRR) %d with "
>> -                      "no HV support\n", excp);
>> -        }
>>     }
>>
>>     /*
>> @@ -746,10 +737,10 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
>>     }
>>
>>     /* Save PC */
>> -    env->spr[srr0] = env->nip;
>> +    env->spr[SPR_SRR0] = env->nip;
>>
>>     /* Save MSR */
>> -    env->spr[srr1] = msr;
>> +    env->spr[SPR_SRR1] = msr;
>>
>>     powerpc_set_excp_state(cpu, vector, new_msr);
>> }
>>
diff mbox series

Patch

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index b7921c0956..4e6bb87b70 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -556,7 +556,6 @@  static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
     CPUState *cs = CPU(cpu);
     CPUPPCState *env = &cpu->env;
     target_ulong msr, new_msr, vector;
-    int srr0, srr1;
 
     if (excp <= POWERPC_EXCP_NONE || excp >= POWERPC_EXCP_NB) {
         cpu_abort(cs, "Invalid PowerPC exception %d. Aborting\n", excp);
@@ -575,10 +574,6 @@  static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
      */
     new_msr = env->msr & ((target_ulong)1 << MSR_ME);
 
-    /* target registers */
-    srr0 = SPR_SRR0;
-    srr1 = SPR_SRR1;
-
     /*
      * Hypervisor emulation assistance interrupt only exists on server
      * arch 2.05 server or later.
@@ -731,10 +726,6 @@  static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
             cpu_abort(cs, "Trying to deliver HV exception (MSR) %d with "
                       "no HV support\n", excp);
         }
-        if (srr0 == SPR_HSRR0) {
-            cpu_abort(cs, "Trying to deliver HV exception (HSRR) %d with "
-                      "no HV support\n", excp);
-        }
     }
 
     /*
@@ -746,10 +737,10 @@  static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
     }
 
     /* Save PC */
-    env->spr[srr0] = env->nip;
+    env->spr[SPR_SRR0] = env->nip;
 
     /* Save MSR */
-    env->spr[srr1] = msr;
+    env->spr[SPR_SRR1] = msr;
 
     powerpc_set_excp_state(cpu, vector, new_msr);
 }