| Message ID | 1469571686-7284-24-git-send-email-benh@kernel.crashing.org |
|---|---|
| State | New |
| Headers | show |
On Wed, Jul 27, 2016 at 08:21:18AM +1000, Benjamin Herrenschmidt wrote: > The current alignment exception generation tries to load the opcode > to put in DSISR from a context where a cpu_ldl_code() is really not > a good idea. It might fault and longjmp out and that's not something > we want happening here. > > Instead, pass the releavant opcode bits via the error_code. > > There are a couple of cases of alignment interrupts that won't set > anything, the ones coming from access to direct store segments, but > that doesn't happen in practice, nobody used direct store segments > and they are gone from newer chips. Do I understand correctly that this isn't actually new? This was already wrong for direct store segments, you've just noted it? > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > --- > target-ppc/excp_helper.c | 9 +++++---- > target-ppc/translate.c | 2 +- > 2 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c > index c31bbad..9a26578 100644 > --- a/target-ppc/excp_helper.c > +++ b/target-ppc/excp_helper.c > @@ -260,11 +260,12 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) > } > break; > case POWERPC_EXCP_ALIGN: /* Alignment exception */ > - /* XXX: this is false */ > /* Get rS/rD and rA from faulting opcode */ > - /* Broken for LE mode */ > - env->spr[SPR_DSISR] |= (cpu_ldl_code(env, env->nip) > - & 0x03FF0000) >> 16; > + /* Note: the opcode fields will not be set properly for a direct > + * store load/store, but nobody cares as nobody actually uses > + * direct store segments. > + */ > + env->spr[SPR_DSISR] |= (env->error_code & 0x03FF0000) >> 16; > break; > case POWERPC_EXCP_PROGRAM: /* Program exception */ > switch (env->error_code & ~0xF) { > diff --git a/target-ppc/translate.c b/target-ppc/translate.c > index ddfec33..9af3f5f 100644 > --- a/target-ppc/translate.c > +++ b/target-ppc/translate.c > @@ -2202,7 +2202,7 @@ static inline void gen_check_align(DisasContext *ctx, TCGv EA, int mask) > tcg_gen_andi_tl(t0, EA, mask); > tcg_gen_brcondi_tl(TCG_COND_EQ, t0, 0, l1); > t1 = tcg_const_i32(POWERPC_EXCP_ALIGN); > - t2 = tcg_const_i32(0); > + t2 = tcg_const_i32(ctx->opcode & 0x03FF0000); > gen_update_nip(ctx, ctx->nip - 4); > gen_helper_raise_exception_err(cpu_env, t1, t2); > tcg_temp_free_i32(t1);
On Wed, 2016-07-27 at 12:30 +1000, David Gibson wrote: > On Wed, Jul 27, 2016 at 08:21:18AM +1000, Benjamin Herrenschmidt > wrote: > > > > The current alignment exception generation tries to load the opcode > > to put in DSISR from a context where a cpu_ldl_code() is really not > > a good idea. It might fault and longjmp out and that's not > > something > > we want happening here. > > > > Instead, pass the releavant opcode bits via the error_code. > > > > There are a couple of cases of alignment interrupts that won't set > > anything, the ones coming from access to direct store segments, but > > that doesn't happen in practice, nobody used direct store segments > > and they are gone from newer chips. > > Do I understand correctly that this isn't actually new? This was > already wrong for direct store segments, you've just noted it? No, it was *supposeldy* working by loading the opcode to set DSISR but I'm breaking it. It's not safe to try to load the opcode from the exception helper, it can cause us to longjmp into lalaland. If we really care, we could fix it differently by making the translation code load the opcode to pass it up to us like the generated code does. The translation code is run in a context that can safely exit via longjmp, so in that case, if the opcode load fails, it will just generate an ISI and try again. However I can't find anything that actually uses direct store segments so can't be bothered. Cheers, Ben. > > > > > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > --- > > target-ppc/excp_helper.c | 9 +++++---- > > target-ppc/translate.c | 2 +- > > 2 files changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c > > index c31bbad..9a26578 100644 > > --- a/target-ppc/excp_helper.c > > +++ b/target-ppc/excp_helper.c > > @@ -260,11 +260,12 @@ static inline void powerpc_excp(PowerPCCPU > > *cpu, int excp_model, int excp) > > } > > break; > > case POWERPC_EXCP_ALIGN: /* Alignment > > exception */ > > - /* XXX: this is false */ > > /* Get rS/rD and rA from faulting opcode */ > > - /* Broken for LE mode */ > > - env->spr[SPR_DSISR] |= (cpu_ldl_code(env, env->nip) > > - & 0x03FF0000) >> 16; > > + /* Note: the opcode fields will not be set properly for a > > direct > > + * store load/store, but nobody cares as nobody actually > > uses > > + * direct store segments. > > + */ > > + env->spr[SPR_DSISR] |= (env->error_code & 0x03FF0000) >> > > 16; > > break; > > case POWERPC_EXCP_PROGRAM: /* Program > > exception */ > > switch (env->error_code & ~0xF) { > > diff --git a/target-ppc/translate.c b/target-ppc/translate.c > > index ddfec33..9af3f5f 100644 > > --- a/target-ppc/translate.c > > +++ b/target-ppc/translate.c > > @@ -2202,7 +2202,7 @@ static inline void > > gen_check_align(DisasContext *ctx, TCGv EA, int mask) > > tcg_gen_andi_tl(t0, EA, mask); > > tcg_gen_brcondi_tl(TCG_COND_EQ, t0, 0, l1); > > t1 = tcg_const_i32(POWERPC_EXCP_ALIGN); > > - t2 = tcg_const_i32(0); > > + t2 = tcg_const_i32(ctx->opcode & 0x03FF0000); > > gen_update_nip(ctx, ctx->nip - 4); > > gen_helper_raise_exception_err(cpu_env, t1, t2); > > tcg_temp_free_i32(t1); >
diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c index c31bbad..9a26578 100644 --- a/target-ppc/excp_helper.c +++ b/target-ppc/excp_helper.c @@ -260,11 +260,12 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) } break; case POWERPC_EXCP_ALIGN: /* Alignment exception */ - /* XXX: this is false */ /* Get rS/rD and rA from faulting opcode */ - /* Broken for LE mode */ - env->spr[SPR_DSISR] |= (cpu_ldl_code(env, env->nip) - & 0x03FF0000) >> 16; + /* Note: the opcode fields will not be set properly for a direct + * store load/store, but nobody cares as nobody actually uses + * direct store segments. + */ + env->spr[SPR_DSISR] |= (env->error_code & 0x03FF0000) >> 16; break; case POWERPC_EXCP_PROGRAM: /* Program exception */ switch (env->error_code & ~0xF) { diff --git a/target-ppc/translate.c b/target-ppc/translate.c index ddfec33..9af3f5f 100644 --- a/target-ppc/translate.c +++ b/target-ppc/translate.c @@ -2202,7 +2202,7 @@ static inline void gen_check_align(DisasContext *ctx, TCGv EA, int mask) tcg_gen_andi_tl(t0, EA, mask); tcg_gen_brcondi_tl(TCG_COND_EQ, t0, 0, l1); t1 = tcg_const_i32(POWERPC_EXCP_ALIGN); - t2 = tcg_const_i32(0); + t2 = tcg_const_i32(ctx->opcode & 0x03FF0000); gen_update_nip(ctx, ctx->nip - 4); gen_helper_raise_exception_err(cpu_env, t1, t2); tcg_temp_free_i32(t1);
The current alignment exception generation tries to load the opcode to put in DSISR from a context where a cpu_ldl_code() is really not a good idea. It might fault and longjmp out and that's not something we want happening here. Instead, pass the releavant opcode bits via the error_code. There are a couple of cases of alignment interrupts that won't set anything, the ones coming from access to direct store segments, but that doesn't happen in practice, nobody used direct store segments and they are gone from newer chips. Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> --- target-ppc/excp_helper.c | 9 +++++---- target-ppc/translate.c | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-)