* [PATCH] target/ppc: Implement ISA v3.1 wait variants
@ 2021-05-17 2:46 Nicholas Piggin
2021-05-17 5:39 ` David Gibson
0 siblings, 1 reply; 4+ messages in thread
From: Nicholas Piggin @ 2021-05-17 2:46 UTC (permalink / raw)
To: qemu-ppc; +Cc: qemu-devel, Nicholas Piggin, David Gibson
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(-)
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)
--
2.23.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] target/ppc: Implement ISA v3.1 wait variants
2021-05-17 2:46 [PATCH] target/ppc: Implement ISA v3.1 wait variants Nicholas Piggin
@ 2021-05-17 5:39 ` David Gibson
2021-05-17 7:19 ` Nicholas Piggin
0 siblings, 1 reply; 4+ messages in thread
From: David Gibson @ 2021-05-17 5:39 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: richard.henderson, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3471 bytes --]
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)
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] target/ppc: Implement ISA v3.1 wait variants
2021-05-17 5:39 ` David Gibson
@ 2021-05-17 7:19 ` Nicholas Piggin
2021-05-24 4:49 ` David Gibson
0 siblings, 1 reply; 4+ messages in thread
From: Nicholas Piggin @ 2021-05-17 7:19 UTC (permalink / raw)
To: David Gibson; +Cc: richard.henderson, qemu-ppc, qemu-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] target/ppc: Implement ISA v3.1 wait variants
2021-05-17 7:19 ` Nicholas Piggin
@ 2021-05-24 4:49 ` David Gibson
0 siblings, 0 replies; 4+ messages in thread
From: David Gibson @ 2021-05-24 4:49 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: richard.henderson, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2511 bytes --]
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.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-05-24 4:52 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-17 2:46 [PATCH] target/ppc: Implement ISA v3.1 wait variants Nicholas Piggin
2021-05-17 5:39 ` David Gibson
2021-05-17 7:19 ` Nicholas Piggin
2021-05-24 4:49 ` David Gibson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.