diff mbox series

[v3,09/14] target/ppc: Move patching nip from exception handler to helper_scv

Message ID b7317331ebccb0209fd0b12687945af6f626b0eb.1686868895.git.balaton@eik.bme.hu
State New
Headers show
Series Misc clean ups to target/ppc exception handling | expand

Commit Message

BALATON Zoltan June 15, 2023, 11:03 p.m. UTC 
From: Nicholas Piggin <npiggin@gmail.com>

Unlike sc, for scv a facility unavailable interrupt must be generated
if FSCR[SCV]=0 so we can't raise the exception with nip set to next
instruction but we can move advancing nip if the FSCR check passes to
helper_scv so the exception handler does not need to change it.

[balaton: added commit message]
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
This needs SoB from Nick

 target/ppc/excp_helper.c | 2 +-
 target/ppc/translate.c   | 6 +++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

Comments

Nicholas Piggin June 20, 2023, 4:09 a.m. UTC | #1 
On Fri Jun 16, 2023 at 9:03 AM AEST, BALATON Zoltan wrote:
> From: Nicholas Piggin <npiggin@gmail.com>
>
> Unlike sc, for scv a facility unavailable interrupt must be generated
> if FSCR[SCV]=0 so we can't raise the exception with nip set to next
> instruction but we can move advancing nip if the FSCR check passes to
> helper_scv so the exception handler does not need to change it.
>
> [balaton: added commit message]
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

Ah you sent it, fine, thank you. But actually now I look again,
now we're off by one in the other direction for the dumps.

So... probably your way is still better because it matches the
interrupt semantics of the ISA when executing the instruction,
but it needs this patch:

For my patch you can add

Signed-off-by: Nicholas Piggin <npiggin@gmail.com

Thanks,
Nick


diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 0e21cb4451..d7f42639c8 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -117,7 +117,7 @@ static void dump_syscall(CPUPPCState *env)
                   ppc_dump_gpr(env, 0), ppc_dump_gpr(env, 3),
                   ppc_dump_gpr(env, 4), ppc_dump_gpr(env, 5),
                   ppc_dump_gpr(env, 6), ppc_dump_gpr(env, 7),
-                  ppc_dump_gpr(env, 8), env->nip);
+                  ppc_dump_gpr(env, 8), env->nip - 4);
 }

 static void dump_hcall(CPUPPCState *env)
@@ -132,7 +132,7 @@ static void dump_hcall(CPUPPCState *env)
                   ppc_dump_gpr(env, 7), ppc_dump_gpr(env, 8),
                   ppc_dump_gpr(env, 9), ppc_dump_gpr(env, 10),
                   ppc_dump_gpr(env, 11), ppc_dump_gpr(env, 12),
-                  env->nip);
+                  env->nip - 4);
 }

 #ifdef CONFIG_TCG



> ---
> This needs SoB from Nick
>
>  target/ppc/excp_helper.c | 2 +-
>  target/ppc/translate.c   | 6 +++++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 903216c2a6..ef363b0285 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -1304,7 +1304,6 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
>      case POWERPC_EXCP_SYSCALL_VECTORED: /* scv exception                     */
>          lev = env->error_code;
>          dump_syscall(env);
> -        env->nip += 4;
>          new_msr |= env->msr & ((target_ulong)1 << MSR_EE);
>          new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
>  
> @@ -2410,6 +2409,7 @@ void helper_ppc_maybe_interrupt(CPUPPCState *env)
>  void helper_scv(CPUPPCState *env, uint32_t lev)
>  {
>      if (env->spr[SPR_FSCR] & (1ull << FSCR_SCV)) {
> +        env->nip += 4;
>          raise_exception_err(env, POWERPC_EXCP_SYSCALL_VECTORED, lev);
>      } else {
>          raise_exception_err(env, POWERPC_EXCP_FU, FSCR_IC_SCV);
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 4260d3d66f..0360a17fb3 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -4433,7 +4433,11 @@ static void gen_scv(DisasContext *ctx)
>  {
>      uint32_t lev = (ctx->opcode >> 5) & 0x7F;
>  
> -    /* Set the PC back to the faulting instruction. */
> +    /*
> +     * Set the PC back to the scv instruction (unlike sc), because a facility
> +     * unavailable interrupt must be generated if FSCR[SCV]=0. The helper
> +     * advances nip if the FSCR check passes.
> +     */
>      gen_update_nip(ctx, ctx->cia);
>      gen_helper_scv(cpu_env, tcg_constant_i32(lev));
>  
> -- 
> 2.30.9
BALATON Zoltan June 20, 2023, 10:47 a.m. UTC | #2 
On Tue, 20 Jun 2023, Nicholas Piggin wrote:
> On Fri Jun 16, 2023 at 9:03 AM AEST, BALATON Zoltan wrote:
>> From: Nicholas Piggin <npiggin@gmail.com>
>>
>> Unlike sc, for scv a facility unavailable interrupt must be generated
>> if FSCR[SCV]=0 so we can't raise the exception with nip set to next
>> instruction but we can move advancing nip if the FSCR check passes to
>> helper_scv so the exception handler does not need to change it.
>>
>> [balaton: added commit message]
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>
> Ah you sent it, fine, thank you. But actually now I look again,
> now we're off by one in the other direction for the dumps.

This is mentioned in the commit message for the patch changing sc. I think 
we should not patch nip in the dump so we actually dump what the CPU 
should have and match the ISA docs.

> So... probably your way is still better because it matches the
> interrupt semantics of the ISA when executing the instruction,
> but it needs this patch:

OK so then I'm confused why we need nip - 4 in dumps?

> For my patch you can add
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com

Please reply to that patch with this if you're OK with that or I'll just 
drop it in v4 and let you send a follow up to avoid confusion.

Regards,
BALATON Zoltan

> Thanks,
> Nick
>
>
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 0e21cb4451..d7f42639c8 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -117,7 +117,7 @@ static void dump_syscall(CPUPPCState *env)
>                   ppc_dump_gpr(env, 0), ppc_dump_gpr(env, 3),
>                   ppc_dump_gpr(env, 4), ppc_dump_gpr(env, 5),
>                   ppc_dump_gpr(env, 6), ppc_dump_gpr(env, 7),
> -                  ppc_dump_gpr(env, 8), env->nip);
> +                  ppc_dump_gpr(env, 8), env->nip - 4);
> }
>
> static void dump_hcall(CPUPPCState *env)
> @@ -132,7 +132,7 @@ static void dump_hcall(CPUPPCState *env)
>                   ppc_dump_gpr(env, 7), ppc_dump_gpr(env, 8),
>                   ppc_dump_gpr(env, 9), ppc_dump_gpr(env, 10),
>                   ppc_dump_gpr(env, 11), ppc_dump_gpr(env, 12),
> -                  env->nip);
> +                  env->nip - 4);
> }
>
> #ifdef CONFIG_TCG
>
>
>
>> ---
>> This needs SoB from Nick
>>
>>  target/ppc/excp_helper.c | 2 +-
>>  target/ppc/translate.c   | 6 +++++-
>>  2 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>> index 903216c2a6..ef363b0285 100644
>> --- a/target/ppc/excp_helper.c
>> +++ b/target/ppc/excp_helper.c
>> @@ -1304,7 +1304,6 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
>>      case POWERPC_EXCP_SYSCALL_VECTORED: /* scv exception                     */
>>          lev = env->error_code;
>>          dump_syscall(env);
>> -        env->nip += 4;
>>          new_msr |= env->msr & ((target_ulong)1 << MSR_EE);
>>          new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
>>
>> @@ -2410,6 +2409,7 @@ void helper_ppc_maybe_interrupt(CPUPPCState *env)
>>  void helper_scv(CPUPPCState *env, uint32_t lev)
>>  {
>>      if (env->spr[SPR_FSCR] & (1ull << FSCR_SCV)) {
>> +        env->nip += 4;
>>          raise_exception_err(env, POWERPC_EXCP_SYSCALL_VECTORED, lev);
>>      } else {
>>          raise_exception_err(env, POWERPC_EXCP_FU, FSCR_IC_SCV);
>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>> index 4260d3d66f..0360a17fb3 100644
>> --- a/target/ppc/translate.c
>> +++ b/target/ppc/translate.c
>> @@ -4433,7 +4433,11 @@ static void gen_scv(DisasContext *ctx)
>>  {
>>      uint32_t lev = (ctx->opcode >> 5) & 0x7F;
>>
>> -    /* Set the PC back to the faulting instruction. */
>> +    /*
>> +     * Set the PC back to the scv instruction (unlike sc), because a facility
>> +     * unavailable interrupt must be generated if FSCR[SCV]=0. The helper
>> +     * advances nip if the FSCR check passes.
>> +     */
>>      gen_update_nip(ctx, ctx->cia);
>>      gen_helper_scv(cpu_env, tcg_constant_i32(lev));
>>
>> --
>> 2.30.9
>
>
>
Nicholas Piggin June 26, 2023, 11:25 a.m. UTC | #3 
On Tue Jun 20, 2023 at 8:47 PM AEST, BALATON Zoltan wrote:
> On Tue, 20 Jun 2023, Nicholas Piggin wrote:
> > On Fri Jun 16, 2023 at 9:03 AM AEST, BALATON Zoltan wrote:
> >> From: Nicholas Piggin <npiggin@gmail.com>
> >>
> >> Unlike sc, for scv a facility unavailable interrupt must be generated
> >> if FSCR[SCV]=0 so we can't raise the exception with nip set to next
> >> instruction but we can move advancing nip if the FSCR check passes to
> >> helper_scv so the exception handler does not need to change it.
> >>
> >> [balaton: added commit message]
> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> >
> > Ah you sent it, fine, thank you. But actually now I look again,
> > now we're off by one in the other direction for the dumps.
>
> This is mentioned in the commit message for the patch changing sc. I think 
> we should not patch nip in the dump so we actually dump what the CPU 
> should have and match the ISA docs.
>
> > So... probably your way is still better because it matches the
> > interrupt semantics of the ISA when executing the instruction,
> > but it needs this patch:
>
> OK so then I'm confused why we need nip - 4 in dumps?

Sorry I missed your reply here. We want nip - 4 in dumps so the
address of the syscall is the sc instruction itself, not the
random one after it.

Thanks,
Nick
Nicholas Piggin June 26, 2023, 11:28 a.m. UTC | #4 
On Fri Jun 16, 2023 at 9:03 AM AEST, BALATON Zoltan wrote:
> From: Nicholas Piggin <npiggin@gmail.com>
>
> Unlike sc, for scv a facility unavailable interrupt must be generated
> if FSCR[SCV]=0 so we can't raise the exception with nip set to next
> instruction but we can move advancing nip if the FSCR check passes to
> helper_scv so the exception handler does not need to change it.
>
> [balaton: added commit message]

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Thanks, sorry for the delay :( Would you be able to resend the series?
You could drop the machine check one for now perhaps until we sort out
what to do with it.

Thanks,
Nick

> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> This needs SoB from Nick
>
>  target/ppc/excp_helper.c | 2 +-
>  target/ppc/translate.c   | 6 +++++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 903216c2a6..ef363b0285 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -1304,7 +1304,6 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
>      case POWERPC_EXCP_SYSCALL_VECTORED: /* scv exception                     */
>          lev = env->error_code;
>          dump_syscall(env);
> -        env->nip += 4;
>          new_msr |= env->msr & ((target_ulong)1 << MSR_EE);
>          new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
>  
> @@ -2410,6 +2409,7 @@ void helper_ppc_maybe_interrupt(CPUPPCState *env)
>  void helper_scv(CPUPPCState *env, uint32_t lev)
>  {
>      if (env->spr[SPR_FSCR] & (1ull << FSCR_SCV)) {
> +        env->nip += 4;
>          raise_exception_err(env, POWERPC_EXCP_SYSCALL_VECTORED, lev);
>      } else {
>          raise_exception_err(env, POWERPC_EXCP_FU, FSCR_IC_SCV);
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 4260d3d66f..0360a17fb3 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -4433,7 +4433,11 @@ static void gen_scv(DisasContext *ctx)
>  {
>      uint32_t lev = (ctx->opcode >> 5) & 0x7F;
>  
> -    /* Set the PC back to the faulting instruction. */
> +    /*
> +     * Set the PC back to the scv instruction (unlike sc), because a facility
> +     * unavailable interrupt must be generated if FSCR[SCV]=0. The helper
> +     * advances nip if the FSCR check passes.
> +     */
>      gen_update_nip(ctx, ctx->cia);
>      gen_helper_scv(cpu_env, tcg_constant_i32(lev));
>
BALATON Zoltan June 27, 2023, 5:40 p.m. UTC | #5 
On Mon, 26 Jun 2023, Nicholas Piggin wrote:
> On Tue Jun 20, 2023 at 8:47 PM AEST, BALATON Zoltan wrote:
>> On Tue, 20 Jun 2023, Nicholas Piggin wrote:
>>> On Fri Jun 16, 2023 at 9:03 AM AEST, BALATON Zoltan wrote:
>>>> From: Nicholas Piggin <npiggin@gmail.com>
>>>>
>>>> Unlike sc, for scv a facility unavailable interrupt must be generated
>>>> if FSCR[SCV]=0 so we can't raise the exception with nip set to next
>>>> instruction but we can move advancing nip if the FSCR check passes to
>>>> helper_scv so the exception handler does not need to change it.
>>>>
>>>> [balaton: added commit message]
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>
>>> Ah you sent it, fine, thank you. But actually now I look again,
>>> now we're off by one in the other direction for the dumps.
>>
>> This is mentioned in the commit message for the patch changing sc. I think
>> we should not patch nip in the dump so we actually dump what the CPU
>> should have and match the ISA docs.
>>
>>> So... probably your way is still better because it matches the
>>> interrupt semantics of the ISA when executing the instruction,
>>> but it needs this patch:
>>
>> OK so then I'm confused why we need nip - 4 in dumps?
>
> Sorry I missed your reply here. We want nip - 4 in dumps so the
> address of the syscall is the sc instruction itself, not the
> random one after it.

Although that's how it was in QEMU before that's not how it is on real 
hardware so I don't think we should keep this and just log what a real CPU 
would have and people should know how to interpret that after consulting 
the ISA docs.

Regards,
BALATON Zoltan
Nicholas Piggin June 28, 2023, 1:03 a.m. UTC | #6 
On Wed Jun 28, 2023 at 3:40 AM AEST, BALATON Zoltan wrote:
> On Mon, 26 Jun 2023, Nicholas Piggin wrote:
> > On Tue Jun 20, 2023 at 8:47 PM AEST, BALATON Zoltan wrote:
> >> On Tue, 20 Jun 2023, Nicholas Piggin wrote:
> >>> On Fri Jun 16, 2023 at 9:03 AM AEST, BALATON Zoltan wrote:
> >>>> From: Nicholas Piggin <npiggin@gmail.com>
> >>>>
> >>>> Unlike sc, for scv a facility unavailable interrupt must be generated
> >>>> if FSCR[SCV]=0 so we can't raise the exception with nip set to next
> >>>> instruction but we can move advancing nip if the FSCR check passes to
> >>>> helper_scv so the exception handler does not need to change it.
> >>>>
> >>>> [balaton: added commit message]
> >>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> >>>
> >>> Ah you sent it, fine, thank you. But actually now I look again,
> >>> now we're off by one in the other direction for the dumps.
> >>
> >> This is mentioned in the commit message for the patch changing sc. I think
> >> we should not patch nip in the dump so we actually dump what the CPU
> >> should have and match the ISA docs.
> >>
> >>> So... probably your way is still better because it matches the
> >>> interrupt semantics of the ISA when executing the instruction,
> >>> but it needs this patch:
> >>
> >> OK so then I'm confused why we need nip - 4 in dumps?
> >
> > Sorry I missed your reply here. We want nip - 4 in dumps so the
> > address of the syscall is the sc instruction itself, not the
> > random one after it.
>
> Although that's how it was in QEMU before

Current upstream QEMU dumps syscall address of sc instruction. After
patch 8 and 9, it will dump the address of the instruction after it.

> that's not how it is on real 
> hardware so I don't think we should keep this and just log what a real CPU 
> would have and people should know how to interpret that after consulting 
> the ISA docs.

I did get the feeling it was nicer your way, OTOH there really is not
anything in the ISA that requires a particular implementation. QEMU is
a real implementation of the ISA anyway. You could argue it's more
consistent for QEMU to keep env->nip as the address of instruction that
caused the interrupt, and then the sc fixup is restricted to setting
SRR0. I'm on the fence about it now.

Thanks,
Nick
diff mbox series

Patch

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 903216c2a6..ef363b0285 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -1304,7 +1304,6 @@  static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
     case POWERPC_EXCP_SYSCALL_VECTORED: /* scv exception                     */
         lev = env->error_code;
         dump_syscall(env);
-        env->nip += 4;
         new_msr |= env->msr & ((target_ulong)1 << MSR_EE);
         new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
 
@@ -2410,6 +2409,7 @@  void helper_ppc_maybe_interrupt(CPUPPCState *env)
 void helper_scv(CPUPPCState *env, uint32_t lev)
 {
     if (env->spr[SPR_FSCR] & (1ull << FSCR_SCV)) {
+        env->nip += 4;
         raise_exception_err(env, POWERPC_EXCP_SYSCALL_VECTORED, lev);
     } else {
         raise_exception_err(env, POWERPC_EXCP_FU, FSCR_IC_SCV);
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 4260d3d66f..0360a17fb3 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -4433,7 +4433,11 @@  static void gen_scv(DisasContext *ctx)
 {
     uint32_t lev = (ctx->opcode >> 5) & 0x7F;
 
-    /* Set the PC back to the faulting instruction. */
+    /*
+     * Set the PC back to the scv instruction (unlike sc), because a facility
+     * unavailable interrupt must be generated if FSCR[SCV]=0. The helper
+     * advances nip if the FSCR check passes.
+     */
     gen_update_nip(ctx, ctx->cia);
     gen_helper_scv(cpu_env, tcg_constant_i32(lev));