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

[Richard Henderson]

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.  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.

> +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?

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.

> 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.


r~