Re: [RFC PATCH 1/6] target/ppc: Add support for the Processor Attention instruction

[Cédric Le Goater]

On 3/26/22 14:04, Richard Henderson wrote:
> On 3/24/22 13:08, Leandro Lupori wrote:
> > +    /* Processor Attention                                                   */
> > +    POWERPC_EXCP_ATTN          = 0x100,
> > +    /*
> > +     * NOTE: POWERPC_EXCP_ATTN uses values from 0x100 to 0x1ff to return
> > +     *       error codes.
> > +     */
>
> As used below, this is not an exception -- the exception is POWERPC_EXCP_MCHECK.  This is something else, for env->error_code.  

Yes. I hacked my way through.

> You could probably come up with a better name, but see below.
>
> > +            if ((env->error_code & ~0xff) == POWERPC_EXCP_ATTN) {
> > +                exit(env->error_code & 0xff);
> > +            }
>
> This will want gdb_exit(value) as well; see e.g. semihosting/arm-compat-semi.c.
>
> In this and the next patch, I do not see anything that makes support for attn 
> conditional, and importantly, off by default.  Otherwise this seems to have the 
> potential for denial of service.

Indeed.

> > +void helper_attn(CPUPPCState *env, target_ulong r3)
> > +{
> > +    bool attn = false;
> > +
> > +    if (env->excp_model == POWERPC_EXCP_POWER8) {
> > +        attn = !!(env->spr[SPR_HID0] & HID0_ATTN);
> > +    } else if (env->excp_model == POWERPC_EXCP_POWER9 ||
> > +               env->excp_model == POWERPC_EXCP_POWER10) {
> > +        attn = !!(env->spr[SPR_HID0] & HID0_POWER9_ATTN);
> > +    }
> > +
> > +    if (attn) {
> > +        raise_exception_err(env, POWERPC_EXCP_MCHECK,
> > +                            POWERPC_EXCP_ATTN | (r3 & 0xff));
> > +    } else {
> > +        raise_exception_err_ra(env, POWERPC_EXCP_PROGRAM,
> > +                               POWERPC_EXCP_INVAL |
> > +                               POWERPC_EXCP_INVAL_INVAL, GETPC());
> > +    }
> > +}
>
> Why did you decide to raise an exception instead of exiting right here?

attn quiesce the thread/core but it can generate an 'host attn'
interrupt event for the service processor, it behaves like a
checkstop.

I think my idea was to raise an interrupt in the instruction model,
and then from the exception model, reach the machine through some
QOM Interface to take action. pSeries would do nothing or exit
depending on some machine option, PowerNV could raise a PSI IRQ
line to signal the embedded BMC simulator to do poweroff

I took a shortcut and introduced an exit() call when I saw the
complexity increase.

But really, the need in this series is to be able to exit from
QEMU after some test has run.
  
> I suggest syncing env state before calling the helper, so that you don't need 
> to unwind here, and so that state is up-to-date for the debugger before exiting.

ok.

> > diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> > index 408ae26173..5ace6f3a29 100644
> > --- a/target/ppc/translate.c
> > +++ b/target/ppc/translate.c
> > @@ -4123,6 +4123,19 @@ static void gen_rvwinkle(DisasContext *ctx)
> >       gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next);
> >   #endif /* defined(CONFIG_USER_ONLY) */
> >   }
> > +
> > +static void gen_attn(DisasContext *ctx)
> > +{
> > + #if defined(CONFIG_USER_ONLY)
> > +    GEN_PRIV;
> > +#else
> > +    CHK_SV;
> > +
> > +    gen_helper_attn(cpu_env, cpu_gpr[3]);
> > +    ctx->base.is_jmp = DISAS_NORETURN;
> > +#endif
> > +}
>
> You want gen_update_nip(ctx, ctx->cia) in there, like gen_exception_err.
>
>
> > +GEN_HANDLER(attn, 0x0, 0x00, 0x8, 0xfffffdff, PPC_FLOW),
>
> New insns into insns32.decode, I would expect.

Thanks,

C.