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

[Fabiano Rosas]

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);
>> }
>>