| Message ID | 20210517024651.2200837-1-npiggin@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/ppc: Implement ISA v3.1 wait variants | expand |
On Mon, May 17, 2021 at 12:46:51PM +1000, Nicholas Piggin wrote: > ISA v3.1 adds new variations of wait, specified by the WC field. These > are not compatible with the wait 0 implementation, because they add > additional conditions that cause the processor to resume, which can > cause software to hang or run very slowly. > > Add the new wait variants with a trivial no-op implementation, which is > allowed, as explained in comments: software must not depend on any > particular architected WC condition having caused resumption of > execution, therefore a no-op implementation is architecturally correct. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> Logic looks fine. There is no test on the CPU's features or model here, though, so this will change behaviour for pre-3.1 CPUs as well. What would invoking these wait variants (presumably reserved) on earlier CPUs do? > --- > Implementing cpu_relax() in Linux with wait 2,0 (pause_short) causes a > hang on boot without this patch. > > target/ppc/translate.c | 39 +++++++++++++++++++++++++++++++++------ > 1 file changed, 33 insertions(+), 6 deletions(-) > > diff --git a/target/ppc/translate.c b/target/ppc/translate.c > index a6381208a5..80db450cab 100644 > --- a/target/ppc/translate.c > +++ b/target/ppc/translate.c > @@ -3619,12 +3619,39 @@ static void gen_sync(DisasContext *ctx) > /* wait */ > static void gen_wait(DisasContext *ctx) > { > - TCGv_i32 t0 = tcg_const_i32(1); > - tcg_gen_st_i32(t0, cpu_env, > - -offsetof(PowerPCCPU, env) + offsetof(CPUState, halted)); > - tcg_temp_free_i32(t0); > - /* Stop translation, as the CPU is supposed to sleep from now */ > - gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next); > + uint32_t wc = (ctx->opcode >> 21) & 3; > + > + /* > + * wait 0 waits for an exception to occur. > + */ > + if (wc == 0) { > + TCGv_i32 t0 = tcg_const_i32(1); > + tcg_gen_st_i32(t0, cpu_env, > + -offsetof(PowerPCCPU, env) + offsetof(CPUState, halted)); > + tcg_temp_free_i32(t0); > + /* Stop translation, as the CPU is supposed to sleep from now */ > + gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next); > + } > + > + /* > + * Other wait types must not wait until an exception occurs because > + * ignoring their other wake-up conditions could cause a hang. > + * > + * wait 1 (waitrsv) waits for an exception or a reservation to be lost. > + * This can happen for implementation specific reasons, so it can be > + * implemented as a no-op. > + * > + * wait 2 waits for an exception or an amount of time to pass. This is > + * implementation specific so it can be implemented as a no-op. > + * > + * wait 3 is reserved, so it may be implemented as a no-op. > + * > + * ISA v3.1 does allow for execution to resume "in the rare case of > + * an implementation-dependent event", so in any case software must > + * not depend on the architected resumption condition to become > + * true, so no-op implementations are architecturally correct (if > + * suboptimal). > + */ > } > > #if defined(TARGET_PPC64)
Excerpts from David Gibson's message of May 17, 2021 3:39 pm: > On Mon, May 17, 2021 at 12:46:51PM +1000, Nicholas Piggin wrote: >> ISA v3.1 adds new variations of wait, specified by the WC field. These >> are not compatible with the wait 0 implementation, because they add >> additional conditions that cause the processor to resume, which can >> cause software to hang or run very slowly. >> >> Add the new wait variants with a trivial no-op implementation, which is >> allowed, as explained in comments: software must not depend on any >> particular architected WC condition having caused resumption of >> execution, therefore a no-op implementation is architecturally correct. >> >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > > Logic looks fine. There is no test on the CPU's features or model > here, though, so this will change behaviour for pre-3.1 CPUs as well. Huh. 2.06-2.07 has very similar WC bits as 3.1, but 3.0 removed them and made them reserved. I should have looked back but I'd assumed they weren't there either. Existing code treats WC != 0 as invalid on pre-3.0 processors AFAIKS, so that's not quite right for 2.06-7 (they should look more like 3.1). But before that it looks like it was just wait with no WC field. > What would invoking these wait variants (presumably reserved) on > earlier CPUs do? Prior to 2.06, it looks like there is no WC field, and so they should generate a program check. So that just leaves the incorrect program checks for 2.06-7, something like this should do it: -GEN_HANDLER_E(wait, 0x1F, 0x1E, 0x00, 0x039FF801, PPC_NONE, PPC2_ISA300), +GEN_HANDLER_E(wait, 0x1F, 0x1E, 0x00, 0x039FF801, PPC_NONE, PPC2_ISA206), 2.06-3.1 should all be fine with this patch, AFAIKS they all have words to the effect that WC != 0 is subject to implementation defined behaviour and may be treated as a no-op or not implemented. Thanks, Nick
On Mon, May 17, 2021 at 05:19:06PM +1000, Nicholas Piggin wrote: > Excerpts from David Gibson's message of May 17, 2021 3:39 pm: > > On Mon, May 17, 2021 at 12:46:51PM +1000, Nicholas Piggin wrote: > >> ISA v3.1 adds new variations of wait, specified by the WC field. These > >> are not compatible with the wait 0 implementation, because they add > >> additional conditions that cause the processor to resume, which can > >> cause software to hang or run very slowly. > >> > >> Add the new wait variants with a trivial no-op implementation, which is > >> allowed, as explained in comments: software must not depend on any > >> particular architected WC condition having caused resumption of > >> execution, therefore a no-op implementation is architecturally correct. > >> > >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > > > > Logic looks fine. There is no test on the CPU's features or model > > here, though, so this will change behaviour for pre-3.1 CPUs as well. > > Huh. 2.06-2.07 has very similar WC bits as 3.1, but 3.0 removed them > and made them reserved. I should have looked back but I'd assumed > they weren't there either. > > Existing code treats WC != 0 as invalid on pre-3.0 processors AFAIKS, > so that's not quite right for 2.06-7 (they should look more like 3.1). > > But before that it looks like it was just wait with no WC field. > > > What would invoking these wait variants (presumably reserved) on > > earlier CPUs do? > > Prior to 2.06, it looks like there is no WC field, and so they should > generate a program check. So that just leaves the incorrect program > checks for 2.06-7, something like this should do it: > > -GEN_HANDLER_E(wait, 0x1F, 0x1E, 0x00, 0x039FF801, PPC_NONE, PPC2_ISA300), > +GEN_HANDLER_E(wait, 0x1F, 0x1E, 0x00, 0x039FF801, PPC_NONE, PPC2_ISA206), Ok, can you update with such a change, and put some of this explanation of the history in a comment. > 2.06-3.1 should all be fine with this patch, AFAIKS they all have words > to the effect that WC != 0 is subject to implementation defined > behaviour and may be treated as a no-op or not implemented. Ok. Note that we do try to match specific CPU behaviour, not just the architecture, although the architecture is obviously more important.
diff --git a/target/ppc/translate.c b/target/ppc/translate.c index a6381208a5..80db450cab 100644 --- a/target/ppc/translate.c +++ b/target/ppc/translate.c @@ -3619,12 +3619,39 @@ static void gen_sync(DisasContext *ctx) /* wait */ static void gen_wait(DisasContext *ctx) { - TCGv_i32 t0 = tcg_const_i32(1); - tcg_gen_st_i32(t0, cpu_env, - -offsetof(PowerPCCPU, env) + offsetof(CPUState, halted)); - tcg_temp_free_i32(t0); - /* Stop translation, as the CPU is supposed to sleep from now */ - gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next); + uint32_t wc = (ctx->opcode >> 21) & 3; + + /* + * wait 0 waits for an exception to occur. + */ + if (wc == 0) { + TCGv_i32 t0 = tcg_const_i32(1); + tcg_gen_st_i32(t0, cpu_env, + -offsetof(PowerPCCPU, env) + offsetof(CPUState, halted)); + tcg_temp_free_i32(t0); + /* Stop translation, as the CPU is supposed to sleep from now */ + gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next); + } + + /* + * Other wait types must not wait until an exception occurs because + * ignoring their other wake-up conditions could cause a hang. + * + * wait 1 (waitrsv) waits for an exception or a reservation to be lost. + * This can happen for implementation specific reasons, so it can be + * implemented as a no-op. + * + * wait 2 waits for an exception or an amount of time to pass. This is + * implementation specific so it can be implemented as a no-op. + * + * wait 3 is reserved, so it may be implemented as a no-op. + * + * ISA v3.1 does allow for execution to resume "in the rare case of + * an implementation-dependent event", so in any case software must + * not depend on the architected resumption condition to become + * true, so no-op implementations are architecturally correct (if + * suboptimal). + */ } #if defined(TARGET_PPC64)
ISA v3.1 adds new variations of wait, specified by the WC field. These are not compatible with the wait 0 implementation, because they add additional conditions that cause the processor to resume, which can cause software to hang or run very slowly. Add the new wait variants with a trivial no-op implementation, which is allowed, as explained in comments: software must not depend on any particular architected WC condition having caused resumption of execution, therefore a no-op implementation is architecturally correct. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- Implementing cpu_relax() in Linux with wait 2,0 (pause_short) causes a hang on boot without this patch. target/ppc/translate.c | 39 +++++++++++++++++++++++++++++++++------ 1 file changed, 33 insertions(+), 6 deletions(-)