Re: [PATCH] target/ppc: Implement new wait variants

[Nicholas Piggin]

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 <npiggin@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