Re: [Qemu-devel] [PATCH 16/32] ppc: Rework NIP updates vs. exception generation

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Wed, 2016-07-27 at 12:19 +1000, David Gibson wrote:
> On Wed, Jul 27, 2016 at 08:21:10AM +1000, Benjamin Herrenschmidt
> wrote:
> > 
> > We make env->nip almost always point to the faulting instruction,
> > thus avoiding a mess of "store_current" vs "store_next" in the
> > exception handling. The syscall exception knows to move the PC by
> > 4 and that's really about it.
> > 
> > This actually fixes a number of cases where the translator was
> > setting env->nip to ctx->nip - 4 (ie. the *current* instruction)
> > but the program check exception handler would branch to
> > "store_current" which applies another -4 offset.
> > 
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashinng.org
> 
> I'm having a fair bit of trouble wrapping my head around this.  Still
> comments on a couple of small matters.

Because the original code was a bloody mess ? :-)

Basically, we make sure that we either don't set env->nip (because the
helper will use the TB scan mechanism based on the return address) or
we set it to the address of the instruction that caused the fault
*always*.

Then, in powerpc_excp, we just put that in SRR1 with one exception, the
syscall, where by spec, it has to be on the next instruction.

To fully understand this, you need to know that when the generator of
an instruction is called, ctx->nip has already been moved to the
next instruction (+4).

Cheers,
Ben.

> > 
> > ---
> >  linux-user/main.c        |  12 ++--
> >  target-ppc/excp_helper.c | 148 ++++++++++++++++++-----------------
> > ------------
> >  target-ppc/translate.c   |  59 +++++++++++--------
> >  3 files changed, 96 insertions(+), 123 deletions(-)
> > 
> > diff --git a/linux-user/main.c b/linux-user/main.c
> > index 462e820..1d149dc 100644
> > --- a/linux-user/main.c
> > +++ b/linux-user/main.c
> > @@ -1814,7 +1814,7 @@ void cpu_loop(CPUPPCState *env)
> >                            env->error_code);
> >                  break;
> >              }
> > -            info._sifields._sigfault._addr = env->nip - 4;
> > +            info._sifields._sigfault._addr = env->nip;
> >              queue_signal(env, info.si_signo, &info);
> >              break;
> >          case POWERPC_EXCP_FPU:      /* Floating-point unavailable
> > exception  */
> > @@ -1822,7 +1822,7 @@ void cpu_loop(CPUPPCState *env)
> >              info.si_signo = TARGET_SIGILL;
> >              info.si_errno = 0;
> >              info.si_code = TARGET_ILL_COPROC;
> > -            info._sifields._sigfault._addr = env->nip - 4;
> > +            info._sifields._sigfault._addr = env->nip;
> >              queue_signal(env, info.si_signo, &info);
> >              break;
> >          case POWERPC_EXCP_SYSCALL:  /* System call
> > exception                 */
> > @@ -1834,7 +1834,7 @@ void cpu_loop(CPUPPCState *env)
> >              info.si_signo = TARGET_SIGILL;
> >              info.si_errno = 0;
> >              info.si_code = TARGET_ILL_COPROC;
> > -            info._sifields._sigfault._addr = env->nip - 4;
> > +            info._sifields._sigfault._addr = env->nip;
> >              queue_signal(env, info.si_signo, &info);
> >              break;
> >          case POWERPC_EXCP_DECR:     /* Decrementer
> > exception                 */
> > @@ -1862,7 +1862,7 @@ void cpu_loop(CPUPPCState *env)
> >              info.si_signo = TARGET_SIGILL;
> >              info.si_errno = 0;
> >              info.si_code = TARGET_ILL_COPROC;
> > -            info._sifields._sigfault._addr = env->nip - 4;
> > +            info._sifields._sigfault._addr = env->nip;
> >              queue_signal(env, info.si_signo, &info);
> >              break;
> >          case POWERPC_EXCP_EFPDI:    /* Embedded floating-point
> > data IRQ      */
> > @@ -1926,7 +1926,7 @@ void cpu_loop(CPUPPCState *env)
> >              info.si_signo = TARGET_SIGILL;
> >              info.si_errno = 0;
> >              info.si_code = TARGET_ILL_COPROC;
> > -            info._sifields._sigfault._addr = env->nip - 4;
> > +            info._sifields._sigfault._addr = env->nip;
> >              queue_signal(env, info.si_signo, &info);
> >              break;
> >          case POWERPC_EXCP_PIT:      /* Programmable interval timer
> > IRQ       */
> > @@ -2001,9 +2001,9 @@ void cpu_loop(CPUPPCState *env)
> >                               env->gpr[5], env->gpr[6], env-
> > >gpr[7],
> >                               env->gpr[8], 0, 0);
> >              if (ret == -TARGET_ERESTARTSYS) {
> > -                env->nip -= 4;
> >                  break;
> >              }
> > +            env->nip += 4;
> >              if (ret == (target_ulong)(-TARGET_QEMU_ESIGRETURN)) {
> >                  /* Returning from a successful sigreturn syscall.
> >                     Avoid corrupting register state.  */
> > diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
> > index 563c7bc..570d188 100644
> > --- a/target-ppc/excp_helper.c
> > +++ b/target-ppc/excp_helper.c
> > @@ -198,7 +198,7 @@ static inline void powerpc_excp(PowerPCCPU
> > *cpu, int excp_model, int excp)
> >          default:
> >              goto excp_invalid;
> >          }
> > -        goto store_next;
> > +        break;
> >      case POWERPC_EXCP_MCHECK:    /* Machine check
> > exception                  */
> >          if (msr_me == 0) {
> >              /* Machine check exception is not enabled.
> > @@ -209,7 +209,7 @@ static inline void powerpc_excp(PowerPCCPU
> > *cpu, int excp_model, int excp)
> >              if (qemu_log_separate()) {
> >                  qemu_log("Machine check while not allowed. "
> >                          "Entering checkstop state\n");
> > -            }
> > +             }
> 
> Looks like an accidental whitespace change.
> 
> > 
> >              cs->halted = 1;
> >              cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
> >          }
> > @@ -235,16 +235,16 @@ static inline void powerpc_excp(PowerPCCPU
> > *cpu, int excp_model, int excp)
> >          default:
> >              break;
> >          }
> > -        goto store_next;
> > +        break;
> >      case POWERPC_EXCP_DSI:       /* Data storage
> > exception                   */
> >          LOG_EXCP("DSI exception: DSISR=" TARGET_FMT_lx" DAR="
> > TARGET_FMT_lx
> >                   "\n", env->spr[SPR_DSISR], env->spr[SPR_DAR]);
> > -        goto store_next;
> > +        break;
> >      case POWERPC_EXCP_ISI:       /* Instruction storage
> > exception            */
> >          LOG_EXCP("ISI exception: msr=" TARGET_FMT_lx ", nip="
> > TARGET_FMT_lx
> >                   "\n", msr, env->nip);
> >          msr |= env->error_code;
> > -        goto store_next;
> > +        break;
> >      case POWERPC_EXCP_EXTERNAL:  /* External
> > input                           */
> >          cs = CPU(cpu);
> >  
> > @@ -258,13 +258,14 @@ static inline void powerpc_excp(PowerPCCPU
> > *cpu, int excp_model, int excp)
> >              /* IACK the IRQ on delivery */
> >              env->spr[SPR_BOOKE_EPR] = ldl_phys(cs->as, env-
> > >mpic_iack);
> >          }
> > -        goto store_next;
> > +        break;
> >      case POWERPC_EXCP_ALIGN:     /* Alignment
> > exception                      */
> >          /* XXX: this is false */
> >          /* Get rS/rD and rA from faulting opcode */
> > -        env->spr[SPR_DSISR] |= (cpu_ldl_code(env, (env->nip - 4))
> > +        /* Broken for LE mode */
> > +        env->spr[SPR_DSISR] |= (cpu_ldl_code(env, env->nip)
> >                                  & 0x03FF0000) >> 16;
> > -        goto store_next;
> > +        break;
> >      case POWERPC_EXCP_PROGRAM:   /* Program
> > exception                        */
> >          switch (env->error_code & ~0xF) {
> >          case POWERPC_EXCP_FP:
> > @@ -280,15 +281,11 @@ static inline void powerpc_excp(PowerPCCPU
> > *cpu, int excp_model, int excp)
> >               * precise in the MSR.
> >               */
> >              msr |= 0x00100000;
> > -            goto store_next;
> > +            break;
> >          case POWERPC_EXCP_INVAL:
> >              LOG_EXCP("Invalid instruction at " TARGET_FMT_lx "\n",
> > env->nip);
> >              msr |= 0x00080000;
> >              env->spr[SPR_BOOKE_ESR] = ESR_PIL;
> > -            /* Some invalids will have the PC in the right place
> > already */
> > -            if (env->error_code & POWERPC_EXCP_INVAL_LSWX) {
> > -                goto store_next;
> > -            }
> >              break;
> >          case POWERPC_EXCP_PRIV:
> >              msr |= 0x00040000;
> > @@ -304,23 +301,16 @@ static inline void powerpc_excp(PowerPCCPU
> > *cpu, int excp_model, int excp)
> >                        env->error_code);
> >              break;
> >          }
> > -        goto store_current;
> > -    case POWERPC_EXCP_HV_EMU:
> > -        srr0 = SPR_HSRR0;
> > -        srr1 = SPR_HSRR1;
> > -        new_msr |= (target_ulong)MSR_HVB;
> > -        new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
> > -        /* Some invalids will have the PC in the right place
> > already */
> > -        if (env->error_code ==
> > (POWERPC_EXCP_INVAL|POWERPC_EXCP_INVAL_LSWX)) {
> > -                goto store_next;
> > -        }
> > -        goto store_current;
> > -    case POWERPC_EXCP_FPU:       /* Floating-point unavailable
> > exception     */
> > -        goto store_current;
> > +        break;
> >      case POWERPC_EXCP_SYSCALL:   /* System call
> > exception                    */
> >          dump_syscall(env);
> >          lev = env->error_code;
> >  
> > +        /* 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_ppc_hypercall) {
> >              cpu_ppc_hypercall(cpu);
> > @@ -329,15 +319,15 @@ static inline void powerpc_excp(PowerPCCPU
> > *cpu, int excp_model, int excp)
> >          if (lev == 1) {
> >              new_msr |= (target_ulong)MSR_HVB;
> >          }
> > -        goto store_next;
> > +        break;
> > +    case POWERPC_EXCP_FPU:       /* Floating-point unavailable
> > exception     */
> >      case POWERPC_EXCP_APU:       /* Auxiliary processor
> > unavailable          */
> > -        goto store_current;
> >      case POWERPC_EXCP_DECR:      /* Decrementer
> > exception                    */
> > -        goto store_next;
> > +        break;
> >      case POWERPC_EXCP_FIT:       /* Fixed-interval timer
> > interrupt           */
> >          /* FIT on 4xx */
> >          LOG_EXCP("FIT exception\n");
> > -        goto store_next;
> > +        break;
> >      case POWERPC_EXCP_WDT:       /* Watchdog timer
> > interrupt                 */
> >          LOG_EXCP("WDT exception\n");
> >          switch (excp_model) {
> > @@ -348,11 +338,10 @@ static inline void powerpc_excp(PowerPCCPU
> > *cpu, int excp_model, int excp)
> >          default:
> >              break;
> >          }
> > -        goto store_next;
> > +        break;
> >      case POWERPC_EXCP_DTLB:      /* Data TLB
> > error                           */
> > -        goto store_next;
> >      case POWERPC_EXCP_ITLB:      /* Instruction TLB
> > error                    */
> > -        goto store_next;
> > +        break;
> >      case POWERPC_EXCP_DEBUG:     /* Debug
> > interrupt                          */
> >          switch (excp_model) {
> >          case POWERPC_EXCP_BOOKE:
> > @@ -367,33 +356,33 @@ static inline void powerpc_excp(PowerPCCPU
> > *cpu, int excp_model, int excp)
> >          }
> >          /* XXX: TODO */
> >          cpu_abort(cs, "Debug exception is not implemented yet
> > !\n");
> > -        goto store_next;
> > +        break;
> >      case POWERPC_EXCP_SPEU:      /* SPE/embedded floating-point
> > unavailable  */
> >          env->spr[SPR_BOOKE_ESR] = ESR_SPV;
> > -        goto store_current;
> > +        break;
> >      case POWERPC_EXCP_EFPDI:     /* Embedded floating-point data
> > interrupt   */
> >          /* XXX: TODO */
> >          cpu_abort(cs, "Embedded floating point data exception "
> >                    "is not implemented yet !\n");
> >          env->spr[SPR_BOOKE_ESR] = ESR_SPV;
> > -        goto store_next;
> > +        break;
> >      case POWERPC_EXCP_EFPRI:     /* Embedded floating-point round
> > interrupt  */
> >          /* XXX: TODO */
> >          cpu_abort(cs, "Embedded floating point round exception "
> >                    "is not implemented yet !\n");
> >          env->spr[SPR_BOOKE_ESR] = ESR_SPV;
> > -        goto store_next;
> > +        break;
> >      case POWERPC_EXCP_EPERFM:    /* Embedded performance monitor
> > interrupt   */
> >          /* XXX: TODO */
> >          cpu_abort(cs,
> >                    "Performance counter exception is not
> > implemented yet !\n");
> > -        goto store_next;
> > +        break;
> >      case POWERPC_EXCP_DOORI:     /* Embedded doorbell
> > interrupt              */
> > -        goto store_next;
> > +        break;
> >      case POWERPC_EXCP_DOORCI:    /* Embedded doorbell critical
> > interrupt     */
> >          srr0 = SPR_BOOKE_CSRR0;
> >          srr1 = SPR_BOOKE_CSRR1;
> > -        goto store_next;
> > +        break;
> >      case POWERPC_EXCP_RESET:     /* System reset
> > exception                   */
> >          if (msr_pow) {
> >              /* indicate that we resumed from power save mode */
> > @@ -404,65 +393,42 @@ static inline void powerpc_excp(PowerPCCPU
> > *cpu, int excp_model, int excp)
> >  
> >          new_msr |= (target_ulong)MSR_HVB;
> >          ail = 0;
> > -        goto store_next;
> > +        break;
> >      case POWERPC_EXCP_DSEG:      /* Data segment
> > exception                   */
> > -        goto store_next;
> >      case POWERPC_EXCP_ISEG:      /* Instruction segment
> > exception            */
> > -        goto store_next;
> > -    case POWERPC_EXCP_HDECR:     /* Hypervisor decrementer
> > exception         */
> > -        srr0 = SPR_HSRR0;
> > -        srr1 = SPR_HSRR1;
> > -        new_msr |= (target_ulong)MSR_HVB;
> > -        new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
> > -        goto store_next;
> >      case POWERPC_EXCP_TRACE:     /* Trace
> > exception                          */
> > -        goto store_next;
> > +        break;
> > +    case POWERPC_EXCP_HDECR:     /* Hypervisor decrementer
> > exception         */
> >      case POWERPC_EXCP_HDSI:      /* Hypervisor data storage
> > exception        */
> > -        srr0 = SPR_HSRR0;
> > -        srr1 = SPR_HSRR1;
> > -        new_msr |= (target_ulong)MSR_HVB;
> > -        new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
> > -        goto store_next;
> >      case POWERPC_EXCP_HISI:      /* Hypervisor instruction storage
> > exception */
> > -        srr0 = SPR_HSRR0;
> > -        srr1 = SPR_HSRR1;
> > -        new_msr |= (target_ulong)MSR_HVB;
> > -        new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
> > -        goto store_next;
> >      case POWERPC_EXCP_HDSEG:     /* Hypervisor data segment
> > exception        */
> > -        srr0 = SPR_HSRR0;
> > -        srr1 = SPR_HSRR1;
> > -        new_msr |= (target_ulong)MSR_HVB;
> > -        new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
> > -        goto store_next;
> >      case POWERPC_EXCP_HISEG:     /* Hypervisor instruction segment
> > exception */
> > +    case POWERPC_EXCP_HV_EMU:
> >          srr0 = SPR_HSRR0;
> >          srr1 = SPR_HSRR1;
> >          new_msr |= (target_ulong)MSR_HVB;
> >          new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
> > -        goto store_next;
> > +        break;
> >      case POWERPC_EXCP_VPU:       /* Vector unavailable
> > exception             */
> > -        goto store_current;
> >      case POWERPC_EXCP_VSXU:       /* VSX unavailable
> > exception               */
> > -        goto store_current;
> >      case POWERPC_EXCP_FU:         /* Facility unavailable
> > exception          */
> > -        goto store_current;
> > +        break;
> >      case POWERPC_EXCP_PIT:       /* Programmable interval timer
> > interrupt    */
> >          LOG_EXCP("PIT exception\n");
> > -        goto store_next;
> > +        break;
> >      case POWERPC_EXCP_IO:        /* IO error
> > exception                       */
> >          /* XXX: TODO */
> >          cpu_abort(cs, "601 IO error exception is not implemented
> > yet !\n");
> > -        goto store_next;
> > +        break;
> >      case POWERPC_EXCP_RUNM:      /* Run mode
> > exception                       */
> >          /* XXX: TODO */
> >          cpu_abort(cs, "601 run mode exception is not implemented
> > yet !\n");
> > -        goto store_next;
> > +        break;
> >      case POWERPC_EXCP_EMUL:      /* Emulation trap
> > exception                 */
> >          /* XXX: TODO */
> >          cpu_abort(cs, "602 emulation trap exception "
> >                    "is not implemented yet !\n");
> > -        goto store_next;
> > +        break;
> >      case POWERPC_EXCP_IFTLB:     /* Instruction fetch TLB
> > error              */
> >          switch (excp_model) {
> >          case POWERPC_EXCP_602:
> > @@ -577,71 +543,67 @@ static inline void powerpc_excp(PowerPCCPU
> > *cpu, int excp_model, int excp)
> >              cpu_abort(cs, "Invalid data store TLB miss
> > exception\n");
> >              break;
> >          }
> > -        goto store_next;
> > +        break;
> >      case POWERPC_EXCP_FPA:       /* Floating-point assist
> > exception          */
> >          /* XXX: TODO */
> >          cpu_abort(cs, "Floating point assist exception "
> >                    "is not implemented yet !\n");
> > -        goto store_next;
> > +        break;
> >      case POWERPC_EXCP_DABR:      /* Data address
> > breakpoint                  */
> >          /* XXX: TODO */
> >          cpu_abort(cs, "DABR exception is not implemented yet
> > !\n");
> > -        goto store_next;
> > +        break;
> >      case POWERPC_EXCP_IABR:      /* Instruction address
> > breakpoint           */
> >          /* XXX: TODO */
> >          cpu_abort(cs, "IABR exception is not implemented yet
> > !\n");
> > -        goto store_next;
> > +        break;
> >      case POWERPC_EXCP_SMI:       /* System management
> > interrupt              */
> >          /* XXX: TODO */
> >          cpu_abort(cs, "SMI exception is not implemented yet !\n");
> > -        goto store_next;
> > +        break;
> >      case POWERPC_EXCP_THERM:     /* Thermal
> > interrupt                        */
> >          /* XXX: TODO */
> >          cpu_abort(cs, "Thermal management exception "
> >                    "is not implemented yet !\n");
> > -        goto store_next;
> > +        break;
> >      case POWERPC_EXCP_PERFM:     /* Embedded performance monitor
> > interrupt   */
> >          /* XXX: TODO */
> >          cpu_abort(cs,
> >                    "Performance counter exception is not
> > implemented yet !\n");
> > -        goto store_next;
> > +        break;
> >      case POWERPC_EXCP_VPUA:      /* Vector assist
> > exception                  */
> >          /* XXX: TODO */
> >          cpu_abort(cs, "VPU assist exception is not implemented yet
> > !\n");
> > -        goto store_next;
> > +        break;
> >      case POWERPC_EXCP_SOFTP:     /* Soft patch
> > exception                     */
> >          /* XXX: TODO */
> >          cpu_abort(cs,
> >                    "970 soft-patch exception is not implemented yet
> > !\n");
> > -        goto store_next;
> > +        break;
> >      case POWERPC_EXCP_MAINT:     /* Maintenance
> > exception                    */
> >          /* XXX: TODO */
> >          cpu_abort(cs,
> >                    "970 maintenance exception is not implemented
> > yet !\n");
> > -        goto store_next;
> > +        break;
> >      case POWERPC_EXCP_MEXTBR:    /* Maskable external
> > breakpoint             */
> >          /* XXX: TODO */
> >          cpu_abort(cs, "Maskable external exception "
> >                    "is not implemented yet !\n");
> > -        goto store_next;
> > +        break;
> >      case POWERPC_EXCP_NMEXTBR:   /* Non maskable external
> > breakpoint         */
> >          /* XXX: TODO */
> >          cpu_abort(cs, "Non maskable external exception "
> >                    "is not implemented yet !\n");
> > -        goto store_next;
> > +        break;
> >      default:
> >      excp_invalid:
> >          cpu_abort(cs, "Invalid PowerPC exception %d. Aborting\n",
> > excp);
> >          break;
> > -    store_current:
> > -        /* save current instruction location */
> > -        env->spr[srr0] = env->nip - 4;
> > -        break;
> > -    store_next:
> > -        /* save next instruction location */
> > -        env->spr[srr0] = env->nip;
> > -        break;
> >      }
> > +
> > +    /* Save PC */
> > +    env->spr[srr0] = env->nip;
> > +
> >      /* Save MSR */
> >      env->spr[srr1] = msr;
> >  
> > diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> > index c4f8916..84bcb09 100644
> > --- a/target-ppc/translate.c
> > +++ b/target-ppc/translate.c
> > @@ -276,8 +276,12 @@ void gen_update_current_nip(void *opaque)
> >  static void gen_exception_err(DisasContext *ctx, uint32_t excp,
> > uint32_t error)
> >  {
> >      TCGv_i32 t0, t1;
> > +
> > +    /* These are all synchronous exceptions, we set the PC back to
> > +     * the faulting instruction
> > +     */
> >      if (ctx->exception == POWERPC_EXCP_NONE) {
> > -        gen_update_nip(ctx, ctx->nip);
> > +        gen_update_nip(ctx, ctx->nip - 4);
> >      }
> >      t0 = tcg_const_i32(excp);
> >      t1 = tcg_const_i32(error);
> > @@ -290,8 +294,12 @@ static void gen_exception_err(DisasContext
> > *ctx, uint32_t excp, uint32_t error)
> >  static void gen_exception(DisasContext *ctx, uint32_t excp)
> >  {
> >      TCGv_i32 t0;
> > +
> > +    /* These are all synchronous exceptions, we set the PC back to
> > +     * the faulting instruction
> > +     */
> >      if (ctx->exception == POWERPC_EXCP_NONE) {
> > -        gen_update_nip(ctx, ctx->nip);
> > +        gen_update_nip(ctx, ctx->nip - 4);
> >      }
> >      t0 = tcg_const_i32(excp);
> >      gen_helper_raise_exception(cpu_env, t0);
> > @@ -299,13 +307,28 @@ static void gen_exception(DisasContext *ctx,
> > uint32_t excp)
> >      ctx->exception = (excp);
> >  }
> >  
> > +static void gen_exception_nip(DisasContext *ctx, uint32_t excp,
> > +                              target_ulong nip)
> > +{
> > +    TCGv_i32 t0;
> > +
> > +    gen_update_nip(ctx, nip);
> > +    t0 = tcg_const_i32(excp);
> > +    gen_helper_raise_exception(cpu_env, t0);
> > +    tcg_temp_free_i32(t0);
> > +    ctx->exception = (excp);
> > +}
> > +
> >  static void gen_debug_exception(DisasContext *ctx)
> >  {
> >      TCGv_i32 t0;
> >  
> > +    /* These are all synchronous exceptions, we set the PC back to
> > +     * the faulting instruction
> > +     */
> >      if ((ctx->exception != POWERPC_EXCP_BRANCH) &&
> >          (ctx->exception != POWERPC_EXCP_SYNC)) {
> > -        gen_update_nip(ctx, ctx->nip);
> > +        gen_update_nip(ctx, ctx->nip - 4);
> >      }
> >      t0 = tcg_const_i32(EXCP_DEBUG);
> >      gen_helper_raise_exception(cpu_env, t0);
> > @@ -1437,7 +1460,7 @@ static void gen_pause(DisasContext *ctx)
> >      tcg_temp_free_i32(t0);
> >  
> >      /* Stop translation, this gives other CPUs a chance to run */
> > -    gen_exception_err(ctx, EXCP_HLT, 1);
> > +    gen_exception_nip(ctx, EXCP_HLT, ctx->nip);
> >  }
> >  #endif /* defined(TARGET_PPC64) */
> >  
> > @@ -2697,12 +2720,6 @@ static void gen_lswi(DisasContext *ctx)
> >          nb = 32;
> >      nr = (nb + 3) / 4;
> >      if (unlikely(lsw_reg_in_range(start, nr, ra))) {
> > -        /* The handler expects the PC to point to *this*
> > instruction,
> > -         * so setting ctx->exception here prevents it from being
> > -         * improperly updated again by gen_inval_exception
> > -         */
> > -        gen_update_nip(ctx, ctx->nip - 4);
> > -        ctx->exception = POWERPC_EXCP_HV_EMU;
> >          gen_inval_exception(ctx, POWERPC_EXCP_INVAL_LSWX);
> >          return;
> >      }
> > @@ -2845,10 +2862,7 @@ static void
> > gen_conditional_store(DisasContext *ctx, TCGv EA,
> >      tcg_gen_movi_tl(t0, (size << 5) | reg);
> >      tcg_gen_st_tl(t0, cpu_env, offsetof(CPUPPCState,
> > reserve_info));
> >      tcg_temp_free(t0);
> > -    gen_update_nip(ctx, ctx->nip-4);
> > -    ctx->exception = POWERPC_EXCP_BRANCH;
> > -    gen_exception(ctx, POWERPC_EXCP_STCX);
> > -    ctx->exception = save_exception;
> > +    gen_exception_err(ctx, POWERPC_EXCP_STCX, 0);
> >  }
> >  #else
> >  static void gen_conditional_store(DisasContext *ctx, TCGv EA,
> > @@ -2987,7 +3001,7 @@ static void gen_wait(DisasContext *ctx)
> >                     -offsetof(PowerPCCPU, env) + offsetof(CPUState,
> > halted));
> >      tcg_temp_free_i32(t0);
> >      /* Stop translation, as the CPU is supposed to sleep from now
> > */
> > -    gen_exception_err(ctx, EXCP_HLT, 1);
> > +    gen_exception_nip(ctx, EXCP_HLT, ctx->nip);
> >  }
> >  
> >  #if defined(TARGET_PPC64)
> > @@ -3090,10 +3104,7 @@ static inline void gen_goto_tb(DisasContext
> > *ctx, int n, target_ulong dest)
> >                  (CPU_BRANCH_STEP | CPU_SINGLE_STEP)) &&
> >                  (ctx->exception == POWERPC_EXCP_BRANCH ||
> >                   ctx->exception == POWERPC_EXCP_TRACE)) {
> > -                target_ulong tmp = ctx->nip;
> > -                ctx->nip = dest;
> > -                gen_exception(ctx, POWERPC_EXCP_TRACE);
> > -                ctx->nip = tmp;
> > +                gen_exception_nip(ctx, POWERPC_EXCP_TRACE, dest);
> >              }
> >              if (ctx->singlestep_enabled & GDBSTUB_SINGLE_STEP) {
> >                  gen_debug_exception(ctx);
> > @@ -3362,7 +3373,7 @@ static void gen_tw(DisasContext *ctx)
> >  {
> >      TCGv_i32 t0 = tcg_const_i32(TO(ctx->opcode));
> >      /* Update the nip since this might generate a trap exception
> > */
> > -    gen_update_nip(ctx, ctx->nip);
> > +    gen_update_nip(ctx, ctx->nip - 4);
> 
> twi etc will generally resume from the next instruction if they trap,
> yes?  In which case I'm a bit confused by the nip - 4.  But possibly
> I
> just haven't correctly followed all the nip update logic changed by
> this patch.
> 
> > 
> >      gen_helper_tw(cpu_env, cpu_gpr[rA(ctx->opcode)],
> > cpu_gpr[rB(ctx->opcode)],
> >                    t0);
> >      tcg_temp_free_i32(t0);
> > @@ -3374,7 +3385,7 @@ static void gen_twi(DisasContext *ctx)
> >      TCGv t0 = tcg_const_tl(SIMM(ctx->opcode));
> >      TCGv_i32 t1 = tcg_const_i32(TO(ctx->opcode));
> >      /* Update the nip since this might generate a trap exception
> > */
> > -    gen_update_nip(ctx, ctx->nip);
> > +    gen_update_nip(ctx, ctx->nip - 4);
> >      gen_helper_tw(cpu_env, cpu_gpr[rA(ctx->opcode)], t0, t1);
> >      tcg_temp_free(t0);
> >      tcg_temp_free_i32(t1);
> > @@ -3386,7 +3397,7 @@ static void gen_td(DisasContext *ctx)
> >  {
> >      TCGv_i32 t0 = tcg_const_i32(TO(ctx->opcode));
> >      /* Update the nip since this might generate a trap exception
> > */
> > -    gen_update_nip(ctx, ctx->nip);
> > +    gen_update_nip(ctx, ctx->nip - 4);
> >      gen_helper_td(cpu_env, cpu_gpr[rA(ctx->opcode)],
> > cpu_gpr[rB(ctx->opcode)],
> >                    t0);
> >      tcg_temp_free_i32(t0);
> > @@ -3398,7 +3409,7 @@ static void gen_tdi(DisasContext *ctx)
> >      TCGv t0 = tcg_const_tl(SIMM(ctx->opcode));
> >      TCGv_i32 t1 = tcg_const_i32(TO(ctx->opcode));
> >      /* Update the nip since this might generate a trap exception
> > */
> > -    gen_update_nip(ctx, ctx->nip);
> > +    gen_update_nip(ctx, ctx->nip - 4);
> >      gen_helper_td(cpu_env, cpu_gpr[rA(ctx->opcode)], t0, t1);
> >      tcg_temp_free(t0);
> >      tcg_temp_free_i32(t1);
> > @@ -6757,7 +6768,7 @@ void gen_intermediate_code(CPUPPCState *env,
> > struct TranslationBlock *tb)
> >                       ctx.exception != POWERPC_SYSCALL &&
> >                       ctx.exception != POWERPC_EXCP_TRAP &&
> >                       ctx.exception != POWERPC_EXCP_BRANCH)) {
> > -            gen_exception(ctxp, POWERPC_EXCP_TRACE);
> > +            gen_exception_nip(ctxp, POWERPC_EXCP_TRACE, ctx.nip);
> >          } else if (unlikely(((ctx.nip & (TARGET_PAGE_SIZE - 1)) ==
> > 0) ||
> >                              (cs->singlestep_enabled) ||
> >                              singlestep ||
>