[09/10] target/ppc: Simplify syscall exception handlers

After previous changes the hypercall handling in 7xx and 74xx
exception handlers can be folded into one if statement to simpilfy
this code.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 target/ppc/excp_helper.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

On Mon Jun 12, 2023 at 8:42 AM AEST, BALATON Zoltan wrote:
> After previous changes the hypercall handling in 7xx and 74xx
> exception handlers can be folded into one if statement to simpilfy
> this code.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  target/ppc/excp_helper.c | 26 ++++++++++----------------
>  1 file changed, 10 insertions(+), 16 deletions(-)
>
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 4f6a6dfb19..089417894e 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -738,26 +738,23 @@ static void powerpc_excp_7xx(PowerPCCPU *cpu, int excp)
>          break;
>      case POWERPC_EXCP_SYSCALL:   /* System call exception                    */
>      {
> -        int lev = env->error_code;
> +        PowerPCCPU *cpu = env_archcpu(env);

I prefer to keep lev. Makes sense to combine the tests though
I suppose. Although with powernv it's not really clear that we
want to dump_syscall. dump_hcall is probably better (powernv
could support a non-PAPR hypervisor with different hcall
semantics, but also it could support an OS with different
syscall semantics too). I guess that could be changed back
when necessary though.

Thanks,
Nick

>  
> -        if (lev == 1 && cpu->vhyp) {
> -            dump_hcall(env);
> -        } else {
> -            dump_syscall(env);
> -        }
>          /*
>           * The Virtual Open Firmware (VOF) relies on the 'sc 1'
>           * instruction to communicate with QEMU. The pegasos2 machine
>           * uses VOF and the 7xx CPUs, so although the 7xx don't have
>           * HV mode, we need to keep hypercall support.
>           */
> -        if (lev == 1 && cpu->vhyp) {
> +        if (unlikely(env->error_code == 1 && cpu->vhyp)) {
>              PPCVirtualHypervisorClass *vhc =
>                  PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
> +            dump_hcall(env);
>              vhc->hypercall(cpu->vhyp, cpu);
>              return;
> +        } else {
> +            dump_syscall(env);
>          }
> -
>          break;
>      }
>      case POWERPC_EXCP_FPU:       /* Floating-point unavailable exception     */
> @@ -882,26 +879,23 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
>          break;
>      case POWERPC_EXCP_SYSCALL:   /* System call exception                    */
>      {
> -        int lev = env->error_code;
> +        PowerPCCPU *cpu = env_archcpu(env);
>  
> -        if (lev == 1 && cpu->vhyp) {
> -            dump_hcall(env);
> -        } else {
> -            dump_syscall(env);
> -        }
>          /*
>           * The Virtual Open Firmware (VOF) relies on the 'sc 1'
>           * instruction to communicate with QEMU. The pegasos2 machine
>           * uses VOF and the 74xx CPUs, so although the 74xx don't have
>           * HV mode, we need to keep hypercall support.
>           */
> -        if (lev == 1 && cpu->vhyp) {
> +        if (unlikely(env->error_code == 1 && cpu->vhyp)) {
>              PPCVirtualHypervisorClass *vhc =
>                  PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
> +            dump_hcall(env);
>              vhc->hypercall(cpu->vhyp, cpu);
>              return;
> +        } else {
> +            dump_syscall(env);
>          }
> -
>          break;
>      }
>      case POWERPC_EXCP_FPU:       /* Floating-point unavailable exception     */
> -- 
> 2.30.9

On Wed, 14 Jun 2023, Nicholas Piggin wrote:
> On Mon Jun 12, 2023 at 8:42 AM AEST, BALATON Zoltan wrote:
>> After previous changes the hypercall handling in 7xx and 74xx
>> exception handlers can be folded into one if statement to simpilfy
>> this code.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  target/ppc/excp_helper.c | 26 ++++++++++----------------
>>  1 file changed, 10 insertions(+), 16 deletions(-)
>>
>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>> index 4f6a6dfb19..089417894e 100644
>> --- a/target/ppc/excp_helper.c
>> +++ b/target/ppc/excp_helper.c
>> @@ -738,26 +738,23 @@ static void powerpc_excp_7xx(PowerPCCPU *cpu, int excp)
>>          break;
>>      case POWERPC_EXCP_SYSCALL:   /* System call exception                    */
>>      {
>> -        int lev = env->error_code;
>> +        PowerPCCPU *cpu = env_archcpu(env);
>
> I prefer to keep lev. Makes sense to combine the tests though
> I suppose. Although with powernv it's not really clear that we
> want to dump_syscall. dump_hcall is probably better (powernv
> could support a non-PAPR hypervisor with different hcall
> semantics, but also it could support an OS with different
> syscall semantics too). I guess that could be changed back
> when necessary though.

What do you mean changed back? This is not supposed to change when 
dump_hcall and dump_syscall is called. However I've only changed the 
powerpc_excp_7xx() and powerpc_excp_74xx() functions where this is only 
present as a hack for VOF. I've left powerpc_excp_books() where hypercalls 
really exist unchanged because that takes other bits into accound so 
probably we can't combine the tests rhere.

Regards,
BALATON Zoltan

> Thanks,
> Nick
>
>>
>> -        if (lev == 1 && cpu->vhyp) {
>> -            dump_hcall(env);
>> -        } else {
>> -            dump_syscall(env);
>> -        }
>>          /*
>>           * The Virtual Open Firmware (VOF) relies on the 'sc 1'
>>           * instruction to communicate with QEMU. The pegasos2 machine
>>           * uses VOF and the 7xx CPUs, so although the 7xx don't have
>>           * HV mode, we need to keep hypercall support.
>>           */
>> -        if (lev == 1 && cpu->vhyp) {
>> +        if (unlikely(env->error_code == 1 && cpu->vhyp)) {
>>              PPCVirtualHypervisorClass *vhc =
>>                  PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
>> +            dump_hcall(env);
>>              vhc->hypercall(cpu->vhyp, cpu);
>>              return;
>> +        } else {
>> +            dump_syscall(env);
>>          }
>> -
>>          break;
>>      }
>>      case POWERPC_EXCP_FPU:       /* Floating-point unavailable exception     */
>> @@ -882,26 +879,23 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
>>          break;
>>      case POWERPC_EXCP_SYSCALL:   /* System call exception                    */
>>      {
>> -        int lev = env->error_code;
>> +        PowerPCCPU *cpu = env_archcpu(env);
>>
>> -        if (lev == 1 && cpu->vhyp) {
>> -            dump_hcall(env);
>> -        } else {
>> -            dump_syscall(env);
>> -        }
>>          /*
>>           * The Virtual Open Firmware (VOF) relies on the 'sc 1'
>>           * instruction to communicate with QEMU. The pegasos2 machine
>>           * uses VOF and the 74xx CPUs, so although the 74xx don't have
>>           * HV mode, we need to keep hypercall support.
>>           */
>> -        if (lev == 1 && cpu->vhyp) {
>> +        if (unlikely(env->error_code == 1 && cpu->vhyp)) {
>>              PPCVirtualHypervisorClass *vhc =
>>                  PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
>> +            dump_hcall(env);
>>              vhc->hypercall(cpu->vhyp, cpu);
>>              return;
>> +        } else {
>> +            dump_syscall(env);
>>          }
>> -
>>          break;
>>      }
>>      case POWERPC_EXCP_FPU:       /* Floating-point unavailable exception     */
>> --
>> 2.30.9
>
>
>

On Thu Jun 15, 2023 at 7:33 AM AEST, BALATON Zoltan wrote:
> On Wed, 14 Jun 2023, Nicholas Piggin wrote:
> > On Mon Jun 12, 2023 at 8:42 AM AEST, BALATON Zoltan wrote:
> >> After previous changes the hypercall handling in 7xx and 74xx
> >> exception handlers can be folded into one if statement to simpilfy
> >> this code.
> >>
> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> >> ---
> >>  target/ppc/excp_helper.c | 26 ++++++++++----------------
> >>  1 file changed, 10 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> >> index 4f6a6dfb19..089417894e 100644
> >> --- a/target/ppc/excp_helper.c
> >> +++ b/target/ppc/excp_helper.c
> >> @@ -738,26 +738,23 @@ static void powerpc_excp_7xx(PowerPCCPU *cpu, int excp)
> >>          break;
> >>      case POWERPC_EXCP_SYSCALL:   /* System call exception                    */
> >>      {
> >> -        int lev = env->error_code;
> >> +        PowerPCCPU *cpu = env_archcpu(env);
> >
> > I prefer to keep lev. Makes sense to combine the tests though
> > I suppose. Although with powernv it's not really clear that we
> > want to dump_syscall. dump_hcall is probably better (powernv
> > could support a non-PAPR hypervisor with different hcall
> > semantics, but also it could support an OS with different
> > syscall semantics too). I guess that could be changed back
> > when necessary though.
>
> What do you mean changed back? This is not supposed to change when 
> dump_hcall and dump_syscall is called. However I've only changed the 
> powerpc_excp_7xx() and powerpc_excp_74xx() functions where this is only 
> present as a hack for VOF. I've left powerpc_excp_books() where hypercalls 
> really exist unchanged because that takes other bits into accound so 
> probably we can't combine the tests rhere.

Oh sorry I didn't notice it wasn't books.

Thanks,
Nick