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

[Joel Stanley]

On Wed, 27 Jul 2022 at 13:49, Daniel Henrique Barboza
<danielhb413@gmail.com> wrote:
>
>
>
> On 7/20/22 10:33, Nicholas Piggin wrote:
> > ISA v2.06 adds new variations of wait, specified by the WC field. These
> > are not all compatible with the prior wait implementation, because they
> > add additional conditions that cause the processor to resume, which can
> > cause software to hang or run very slowly.
>
> So I suppose this is not a new feature, but a bug fix to remediate these hangs
> cause by the incompatibility of the WC field  with other ISA versions. Is
> that right?

That's the case. Nick has some kernel patches that make Linux use the
new opcode:

 https://lore.kernel.org/all/20220720132132.903462-1-npiggin@gmail.com/

With these applied the kernel hangs during boot if more than one CPU
is present. I was able to reproduce with ppc64le_defconfig and this
command line:

 qemu-system-ppc64 -M pseries,x-vof=on -cpu POWER10 -smp 2 -nographic
-kernel zImage.pseries -no-reboot

Qemu will exit (as there's no filesystem) if the test "passes", or
hang during boot if it hits the bug.

There's a kernel here with the patches applied in case someone else
wants to test:

https://ozlabs.org/~joel/zImage.pseries-v5.19-rc8-wait-v3

Tested-by: Joel Stanley <joel@jms.id.au>

Because of the hang it would be best if we merged the patch as a fix
sooner rather than later.

Cheers,

Joel

> I'm explicitly asking for it because if it's a bug fix it's ok to pick it
> during the freeze. Especially here, given that what you're doing is mostly
> adding no-ops for conditions that we're not covering.
>
> >
> > ISA v3.0 changed the wait opcode and removed the new variants (retaining
> > the WC field but making non-zero values reserved).
> >
> > 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 provides basic correctness as explained in
> > comments.
> >
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> > v3:
> > - Add EXTRACT_HELPERs
> > - Reserved fields should be ignored, not trap.
> > - v3.1 defines special case of reserved PL values being treated as
> >    a no-op when WC=2.
> > - Change code organization to (hopefully) be easier to follow each
> >    ISA / variation.
> > - Tested old wait variant with Linux e6500 boot and verify that
> >    gen_wait is called and takes the expected path.
> >
> > Thanks,
> > Nick
> >
> >   target/ppc/internal.h  |  3 ++
> >   target/ppc/translate.c | 96 ++++++++++++++++++++++++++++++++++++++----
> >   2 files changed, 91 insertions(+), 8 deletions(-)
> >
> > diff --git a/target/ppc/internal.h b/target/ppc/internal.h
> > index 2add128cd1..57c0a42a6b 100644
> > --- a/target/ppc/internal.h
> > +++ b/target/ppc/internal.h
> > @@ -168,6 +168,9 @@ EXTRACT_HELPER_SPLIT_3(DX, 10, 6, 6, 5, 16, 1, 1, 0, 0)
> >   /* darn */
> >   EXTRACT_HELPER(L, 16, 2);
> >   #endif
> > +/* wait */
> > +EXTRACT_HELPER(WC, 21, 2);
> > +EXTRACT_HELPER(PL, 16, 2);
> >
> >   /***                            Jump target decoding                      
> >  ***/
> >   /* Immediate address */
> > diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> > index 1d6daa4608..e0a835ac90 100644
> > --- a/target/ppc/translate.c
> > +++ b/target/ppc/translate.c
> > @@ -4066,12 +4066,91 @@ 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;
> > +
> > +    if (ctx->insns_flags & PPC_WAIT) {
> > +        /* v2.03-v2.07 define an older incompatible 'wait' encoding. */
> > +
> > +        if (ctx->insns_flags2 & PPC2_PM_ISA206) {
> > +            /* v2.06 introduced the WC field. WC > 0 may be treated as 
> > no-op. */
> > +            wc = WC(ctx->opcode);
> > +        } else {
> > +            wc = 0;
> > +        }
> > +
> > +    } else if (ctx->insns_flags2 & PPC2_ISA300) {
> > +        /* v3.0 defines a new 'wait' encoding. */
> > +        wc = WC(ctx->opcode);
>
>
> The ISA seems to indicate that WC=3 is always reserved in both ISA300 and
> ISA310. I believe you can check for WC=3 and gen_invalid() return right
> off the bat at this point.
>
>
> Thanks,
>
>
> Daniel
>
>
>
> > +        if (ctx->insns_flags2 & PPC2_ISA310) {
> > +            uint32_t pl = PL(ctx->opcode);
> > +
> > +            /* WC 1,2 may be treated as no-op. WC 3 is reserved. */
> > +            if (wc == 3) {
> > +                gen_invalid(ctx);
> > +                return;
> > +            }
> > +
> > +            /* PL 1-3 are reserved. If WC=2 then the insn is treated as 
> > noop. */
> > +            if (pl > 0 && wc != 2) {
> > +                gen_invalid(ctx);
> > +                return;
> > +            }
> > +
> > +        } else { /* ISA300 */
> > +            /* WC 1-3 are reserved */
> > +            if (wc > 0) {
> > +                gen_invalid(ctx);
> > +                return;
> > +            }
> > +        }
> > +
> > +    } else {
> > +        warn_report("wait instruction decoded with wrong ISA flags.");
> > +        gen_invalid(ctx);
> > +        return;
> > +    }
> > +
> > +    /*
> > +     * wait without WC field or with WC=0 waits for an exception / 
> > interrupt
> > +     * 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,3 are architected but may be implemented 
> > as
> > +     * no-ops.
> > +     *
> > +     * wc=1 and wc=3 explicitly allow the instruction to be treated as a 
> > no-op.
> > +     *
> > +     * wc=2 waits for an implementation-specific condition, such 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 (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 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 allows 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 +6931,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),
> >   GEN_HANDLER(b, 0x12, 0xFF, 0xFF, 0x00000000, PPC_FLOW),
> >   GEN_HANDLER(bc, 0x10, 0xFF, 0xFF, 0x00000000, PPC_FLOW),
> >   GEN_HANDLER(bcctr, 0x13, 0x10, 0x10, 0x00000000, PPC_FLOW),
>