diff mbox series

[v2,08/14] target/ppc: 405: System call exception cleanup

Message ID 20220118184448.852996-9-farosas@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series target/ppc: powerpc_excp improvements [40x] (3/n) | expand

Commit Message

Fabiano Rosas Jan. 18, 2022, 6:44 p.m. UTC
There's no sc 1.

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

Comments

David Gibson Jan. 19, 2022, 6:09 a.m. UTC | #1
On Tue, Jan 18, 2022 at 03:44:42PM -0300, Fabiano Rosas wrote:
> There's no sc 1.

No... but what exactly should and will happen if you attempt to
execute an "sc 1" on 40x.  Will it be treated as an "sc 0", or will it
cause a 0x700?  If it's a 0x700, better double check that that is
generated at translation time, if you're removing the check on level
here.

> 
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> ---
>  target/ppc/excp_helper.c | 21 ++-------------------
>  1 file changed, 2 insertions(+), 19 deletions(-)
> 
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 8fae8aa0be..9a6f8365d6 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -398,7 +398,7 @@ static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
>      CPUPPCState *env = &cpu->env;
>      int excp_model = env->excp_model;
>      target_ulong msr, new_msr, vector;
> -    int srr0, srr1, lev = -1;
> +    int srr0, srr1;
>  
>      if (excp <= POWERPC_EXCP_NONE || excp >= POWERPC_EXCP_NB) {
>          cpu_abort(cs, "Invalid PowerPC exception %d. Aborting\n", excp);
> @@ -521,30 +521,13 @@ static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
>          }
>          break;
>      case POWERPC_EXCP_SYSCALL:   /* System call exception                    */
> -        lev = env->error_code;
> -
> -        if ((lev == 1) && cpu->vhyp) {
> -            dump_hcall(env);
> -        } else {
> -            dump_syscall(env);
> -        }
> +        dump_syscall(env);
>  
>          /*
>           * We need to correct the NIP which in this case is supposed
>           * to point to the next instruction
>           */
>          env->nip += 4;
> -
> -        /* "PAPR mode" built-in hypercall emulation */
> -        if ((lev == 1) && cpu->vhyp) {
> -            PPCVirtualHypervisorClass *vhc =
> -                PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
> -            vhc->hypercall(cpu->vhyp, cpu);
> -            return;
> -        }
> -        if (lev == 1) {
> -            new_msr |= (target_ulong)MSR_HVB;
> -        }
>          break;
>      case POWERPC_EXCP_FIT:       /* Fixed-interval timer interrupt           */
>          trace_ppc_excp_print("FIT");
Cédric Le Goater Jan. 25, 2022, 8:18 a.m. UTC | #2
On 1/19/22 07:09, David Gibson wrote:
> On Tue, Jan 18, 2022 at 03:44:42PM -0300, Fabiano Rosas wrote:
>> There's no sc 1.
> 
> No... but what exactly should and will happen if you attempt to
> execute an "sc 1" on 40x.  Will it be treated as an "sc 0", or will it
> cause a 0x700?  If it's a 0x700, better double check that that is
> generated at translation time, if you're removing the check on level
> here.

A Program Interrupt with the illegal instruction error code should be
generated at translation time but it is not the case today. It never
was correctly implemented AFAICT :

   /* Top bit of opc2 corresponds with low bit of LEV, so use two handlers */
   GEN_HANDLER(sc, 0x11, 0x11, 0xFF, 0x03FFF01D, PPC_FLOW),
   GEN_HANDLER(sc, 0x11, 0x01, 0xFF, 0x03FFF01D, PPC_FLOW),

We would need a simple 'sc' instruction for the PPC405 and other
processors. Let's add that to the TODO list.

Thanks,

C.


> 
>>
>> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
>> ---
>>   target/ppc/excp_helper.c | 21 ++-------------------
>>   1 file changed, 2 insertions(+), 19 deletions(-)
>>
>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>> index 8fae8aa0be..9a6f8365d6 100644
>> --- a/target/ppc/excp_helper.c
>> +++ b/target/ppc/excp_helper.c
>> @@ -398,7 +398,7 @@ static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
>>       CPUPPCState *env = &cpu->env;
>>       int excp_model = env->excp_model;
>>       target_ulong msr, new_msr, vector;
>> -    int srr0, srr1, lev = -1;
>> +    int srr0, srr1;
>>   
>>       if (excp <= POWERPC_EXCP_NONE || excp >= POWERPC_EXCP_NB) {
>>           cpu_abort(cs, "Invalid PowerPC exception %d. Aborting\n", excp);
>> @@ -521,30 +521,13 @@ static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
>>           }
>>           break;
>>       case POWERPC_EXCP_SYSCALL:   /* System call exception                    */
>> -        lev = env->error_code;
>> -
>> -        if ((lev == 1) && cpu->vhyp) {
>> -            dump_hcall(env);
>> -        } else {
>> -            dump_syscall(env);
>> -        }
>> +        dump_syscall(env);
>>   
>>           /*
>>            * We need to correct the NIP which in this case is supposed
>>            * to point to the next instruction
>>            */
>>           env->nip += 4;
>> -
>> -        /* "PAPR mode" built-in hypercall emulation */
>> -        if ((lev == 1) && cpu->vhyp) {
>> -            PPCVirtualHypervisorClass *vhc =
>> -                PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
>> -            vhc->hypercall(cpu->vhyp, cpu);
>> -            return;
>> -        }
>> -        if (lev == 1) {
>> -            new_msr |= (target_ulong)MSR_HVB;
>> -        }
>>           break;
>>       case POWERPC_EXCP_FIT:       /* Fixed-interval timer interrupt           */
>>           trace_ppc_excp_print("FIT");
>
Cédric Le Goater Jan. 25, 2022, 11:27 a.m. UTC | #3
On 1/25/22 09:18, Cédric Le Goater wrote:
> On 1/19/22 07:09, David Gibson wrote:
>> On Tue, Jan 18, 2022 at 03:44:42PM -0300, Fabiano Rosas wrote:
>>> There's no sc 1.
>>
>> No... but what exactly should and will happen if you attempt to
>> execute an "sc 1" on 40x.  Will it be treated as an "sc 0", or will it
>> cause a 0x700?  If it's a 0x700, better double check that that is
>> generated at translation time, if you're removing the check on level
>> here.
> 
> A Program Interrupt with the illegal instruction error code should be
> generated at translation time but it is not the case today. It never
> was correctly implemented AFAICT :
> 
>    /* Top bit of opc2 corresponds with low bit of LEV, so use two handlers */
>    GEN_HANDLER(sc, 0x11, 0x11, 0xFF, 0x03FFF01D, PPC_FLOW),
>    GEN_HANDLER(sc, 0x11, 0x01, 0xFF, 0x03FFF01D, PPC_FLOW),
> 
> We would need a simple 'sc' instruction for the PPC405 and other
> processors. Let's add that to the TODO list.

The ref405ep machine now boots a mainline Linux with a buildroot user space.
Let's get this series merged first.

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.
BALATON Zoltan Jan. 25, 2022, 12:49 p.m. UTC | #4
On Tue, 25 Jan 2022, Cédric Le Goater wrote:
> On 1/19/22 07:09, David Gibson wrote:
>> On Tue, Jan 18, 2022 at 03:44:42PM -0300, Fabiano Rosas wrote:
>>> There's no sc 1.
>> 
>> No... but what exactly should and will happen if you attempt to
>> execute an "sc 1" on 40x.  Will it be treated as an "sc 0", or will it
>> cause a 0x700?  If it's a 0x700, better double check that that is
>> generated at translation time, if you're removing the check on level
>> here.
>
> A Program Interrupt with the illegal instruction error code should be
> generated at translation time but it is not the case today. It never
> was correctly implemented AFAICT :
>
>  /* Top bit of opc2 corresponds with low bit of LEV, so use two handlers */
>  GEN_HANDLER(sc, 0x11, 0x11, 0xFF, 0x03FFF01D, PPC_FLOW),
>  GEN_HANDLER(sc, 0x11, 0x01, 0xFF, 0x03FFF01D, PPC_FLOW),
>
> We would need a simple 'sc' instruction for the PPC405 and other
> processors. Let's add that to the TODO list.

Not directly related to this but as a reminder: if I remember correctly 
VOF uses sc 1 for hypercalls and I use VOF on pegasos2 which has a G4 or 
G3 CPU that does not have this instruction but we emulate that anyway so 
this works now at least with TCG. AFAICT changes so far did not break this 
but please consider this when getting there. We could use a different 
method for hypercalls in VOF but that would either result in different VOF 
binary for different machines or needing more changes to spap,r neither of 
which is desirable, so we chose this solution for now to allow hypercalls 
on 32bit PPC if the vhyp is set. This was in commit 5e994fc019862.

Regards,
BALATON Zoltan
Richard Henderson Jan. 26, 2022, 10:02 p.m. UTC | #5
On 1/19/22 5:44 AM, Fabiano Rosas wrote:
> There's no sc 1.
> 
> Signed-off-by: Fabiano Rosas<farosas@linux.ibm.com>
> ---
>   target/ppc/excp_helper.c | 21 ++-------------------
>   1 file changed, 2 insertions(+), 19 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~
diff mbox series

Patch

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 8fae8aa0be..9a6f8365d6 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -398,7 +398,7 @@  static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
     CPUPPCState *env = &cpu->env;
     int excp_model = env->excp_model;
     target_ulong msr, new_msr, vector;
-    int srr0, srr1, lev = -1;
+    int srr0, srr1;
 
     if (excp <= POWERPC_EXCP_NONE || excp >= POWERPC_EXCP_NB) {
         cpu_abort(cs, "Invalid PowerPC exception %d. Aborting\n", excp);
@@ -521,30 +521,13 @@  static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
         }
         break;
     case POWERPC_EXCP_SYSCALL:   /* System call exception                    */
-        lev = env->error_code;
-
-        if ((lev == 1) && cpu->vhyp) {
-            dump_hcall(env);
-        } else {
-            dump_syscall(env);
-        }
+        dump_syscall(env);
 
         /*
          * We need to correct the NIP which in this case is supposed
          * to point to the next instruction
          */
         env->nip += 4;
-
-        /* "PAPR mode" built-in hypercall emulation */
-        if ((lev == 1) && cpu->vhyp) {
-            PPCVirtualHypervisorClass *vhc =
-                PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
-            vhc->hypercall(cpu->vhyp, cpu);
-            return;
-        }
-        if (lev == 1) {
-            new_msr |= (target_ulong)MSR_HVB;
-        }
         break;
     case POWERPC_EXCP_FIT:       /* Fixed-interval timer interrupt           */
         trace_ppc_excp_print("FIT");