Excerpts from Víctor Colombo's message of July 20, 2022 12:20 am:
> Hello Nicholas,
>
> On 19/07/2022 08:38, Nicholas Piggin wrote:
>> ISA v2.06 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.
>>
>> ISA v3.0 changed the wait opcode.
>>
>> ISA v3.1 added new WC values to the new wait opcode, and added a PL
>> field.
>>
>> This implements the new wait encoding and supports WC variants with
>> no-op implementations, which is provides basic correctness as explained.
>>
>> Signed-off-by: Nicholas Piggin <npig...@gmail.com>
>> ---
>> target/ppc/translate.c | 84 ++++++++++++++++++++++++++++++++++++++----
>> 1 file changed, 76 insertions(+), 8 deletions(-)
>>
>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>> index 1d6daa4608..ce4aa84f1d 100644
>> --- a/target/ppc/translate.c
>> +++ b/target/ppc/translate.c
>> @@ -4066,12 +4066,79 @@ 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;
>> + uint32_t pl = (ctx->opcode >> 16) & 3;
>
> I think the best here would be to move this instruction to decodetree.
> However, this can be a bit of extra work and out of the scope you though
> for this patch.
Yeah you're probably right. I haven't looked into decodetree yet sorry,
if we could get this in first would be convenient.
> What do you think about adding a EXTRACT_HELPER to
> target/ppc/internal.h?
Just to avoid open coded extraction here? Probably a good idea, I'll try
it.
>> +
>> + /* v3.0 and later use the ISA flag for wait rather than a PM flag */
>> + if (!(ctx->insns_flags2 & PPC2_PM_ISA206) &&
>> + !(ctx->insns_flags2 & PPC2_ISA300)) {
>> + /* wc field was introduced in ISA v2.06 */
>> + if (wc) {
>> + gen_invalid(ctx);
>> + return;
>> + }
>> + }
>> +
>> + if (!(ctx->insns_flags2 & PPC2_ISA310)) {
>> + /* pl field was introduced in ISA v3.1 */
>> + if (pl) {
>> + gen_invalid(ctx);
>> + return;
>> + }
>
> IIUC the ISA says that "Reserved fields in instructions are ignored by
> the processor". So this check is incorrect, I guess, as we should allow
> the instruction to continue.
Hmm, I think you're right.
>> +
>> + if (ctx->insns_flags2 & PPC2_ISA300) {
>> + /* wc > 0 is reserved in v3.0 */
>> + if (wc > 0) {
>
> This however is correct
>
>> + gen_invalid(ctx);
>> + return;
>> + }
>> + }
>> + }
>> +
>> + /* wc=3 is reserved and pl=1-3 are reserved in v3.1. */
>> + if (wc == 3 || pl > 0) {
>
> This can cause a situation where the field is reserve in a previous ISA
> and should be ignored. I think the best option is to put these checks
> inside a conditional for each different ISA. Otherwise it's getting a
> bit hard to follow what should happen in each situation.
Good idea.
>
>> + gen_invalid(ctx);
>> + return;
>> + }
>> +
>> + /* 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 just wait until an exception occurs because
>> + * ignoring their other wake-up conditions could cause a hang.
>> + *
>> + * For v2.06 and 2.07, wc=1,2 are architected but may be implemented as
>> + * no-ops.
>> + *
>> + * wc=1 (waitrsv) waits for an exception or a reservation to be lost.
>> + * Reservation-loss may have implementation-specific conditions, so it
>> + * can be implemented as a no-op.
>> + *
>> + * wc=2 waits for an implementation-specific condition which could be
>> + * always true, so it can be implemented as a no-op.
>> + *
>> + * For v3.1, wc=1,2 are architected but may be implemented as no-ops.
>> + *
>> + * wc=1 similarly to v2.06 and v2.07.
>> + *
>> + * wc=2 waits for an exception or an amount of time to pass. This
>> + * amount is implementation-specific so it can 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 should be architecturally correct
>> + * (if suboptimal).
>> + */
>> }
>>
>> #if defined(TARGET_PPC64)
>> @@ -6852,8 +6919,9 @@ GEN_HANDLER2(stdcx_, "stdcx.", 0x1F, 0x16, 0x06,
>> 0x00000000, PPC_64B),
>> GEN_HANDLER_E(stqcx_, 0x1F, 0x16, 0x05, 0, PPC_NONE, PPC2_LSQ_ISA207),
>> #endif
>> GEN_HANDLER(sync, 0x1F, 0x16, 0x12, 0x039FF801, PPC_MEM_SYNC),
>> -GEN_HANDLER(wait, 0x1F, 0x1E, 0x01, 0x03FFF801, PPC_WAIT),
>> -GEN_HANDLER_E(wait, 0x1F, 0x1E, 0x00, 0x039FF801, PPC_NONE, PPC2_ISA300),
>> +/* ISA v3.0 changed the extended opcode from 62 to 30 */
>> +GEN_HANDLER(wait, 0x1F, 0x1E, 0x01, 0x039FF801, PPC_WAIT),
>> +GEN_HANDLER_E(wait, 0x1F, 0x1E, 0x00, 0x039CF801, PPC_NONE, PPC2_ISA300),
>
> Does this continue to work for the previous ISAs? I'm having a hard time
> testing this instruction for previous cpus, even without this patch
I don't think I tested that actually. I will.
Thanks for the review, I'll make updates and post a new vesion.
Thanks,
Nick
