Re: [PATCH v2 3/6] hppa: Add support for an emulated TOC/NMI button.

[Philippe Mathieu-Daudé]

On 31/1/22 22:35, Helge Deller wrote:
> Almost all PA-RISC machines have either a button that is labeled with 'TOC' or
> a BMC/GSP function to trigger a TOC.  TOC is a non-maskable interrupt that is
> sent to the processor.  This can be used for diagnostic purposes like obtaining
> a stack trace/register dump or to enter KDB/KGDB in Linux.
>
> This patch adds support for such an emulated TOC button.
>
> It wires up the qemu monitor "nmi" command to trigger a TOC.  For that it

s/qemu/QEMU/ (few others).

> provides the hppa_nmi function which is assigned to the nmi_monitor_handler
> function pointer.  When called it raises the EXCP_TOC hardware interrupt in the
> hppa_cpu_do_interrupt() function.  The interrupt function then calls the
> architecturally defined TOC function in SeaBIOS-hppa firmware (at fixed address
> 0xf0000000).
>
> According to the PA-RISC PDC specification, the SeaBIOS firmware then writes
> the CPU registers into PIM (processor internal memmory) for later analysis.  In

Typo "memory".

> order to write all registers it needs to know the contents of the CPU "shadow
> registers" and the IASQ- and IAOQ-back values. The IAOQ/IASQ values are
> provided by qemu in shadow registers when entering the SeaBIOS TOC function.
> This patch adds a new aritificial opcode "getshadowregs" (0xfffdead2) which

Typo "artificial".

> restores the original values of the shadow registers. With this opcode SeaBIOS
> can store those registers as well into PIM before calling an OS-provided TOC
> handler.
>
> To trigger a TOC, switch to the qemu monitor with Ctrl-A C, and type in the
> command "nmi".  After the TOC started the OS-debugger, exit the qemu monitor
> with Ctrl-A C.
>
> Signed-off-by: Helge Deller <deller@gmx.de>
> ---
>   hw/hppa/machine.c        | 35 ++++++++++++++++++++++++++++++++++-
>   target/hppa/cpu.c        |  2 +-
>   target/hppa/cpu.h        |  5 +++++
>   target/hppa/helper.h     |  1 +
>   target/hppa/insns.decode |  1 +
>   target/hppa/int_helper.c | 19 ++++++++++++++++++-
>   target/hppa/op_helper.c  |  7 ++++++-
>   target/hppa/translate.c  | 10 ++++++++++
>   8 files changed, 76 insertions(+), 4 deletions(-)
> +static const TypeInfo machine_hppa_machine_init_typeinfo = {
> +    .name = ("hppa" "-machine"),

       .name = MACHINE_TYPE_NAME("hppa"),

> +    .parent = "machine",
> +    .class_init = machine_hppa_machine_init_class_init,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_NMI },
> +        { }
> +    },
> +};

> diff --git a/target/hppa/helper.h b/target/hppa/helper.h
> index 0a629ffa7c..fe8a9ce493 100644
> --- a/target/hppa/helper.h
> +++ b/target/hppa/helper.h
> @@ -80,6 +80,7 @@ DEF_HELPER_FLAGS_0(read_interval_timer, TCG_CALL_NO_RWG, tr)
>   #ifndef CONFIG_USER_ONLY
>   DEF_HELPER_1(halt, noreturn, env)
>   DEF_HELPER_1(reset, noreturn, env)
> +DEF_HELPER_1(getshadowregs, void, env)
>   DEF_HELPER_1(rfi, void, env)
>   DEF_HELPER_1(rfi_r, void, env)
>   DEF_HELPER_FLAGS_2(write_interval_timer, TCG_CALL_NO_RWG, void, env, tr)
> diff --git a/target/hppa/insns.decode b/target/hppa/insns.decode
> index d4eefc0d48..c7a7e997f9 100644
> --- a/target/hppa/insns.decode
> +++ b/target/hppa/insns.decode
> @@ -111,6 +111,7 @@ rfi_r           000000 ----- ----- --- 01100101 00000
>   # They are allocated from the unassigned instruction space.
>   halt            1111 1111 1111 1101 1110 1010 1101 0000
>   reset           1111 1111 1111 1101 1110 1010 1101 0001
> +getshadowregs   1111 1111 1111 1101 1110 1010 1101 0010

> diff --git a/target/hppa/op_helper.c b/target/hppa/op_helper.c
> index 1b86557d5d..b0dec4ebf4 100644
> --- a/target/hppa/op_helper.c
> +++ b/target/hppa/op_helper.c
> @@ -694,7 +694,7 @@ void HELPER(rfi)(CPUHPPAState *env)
>       cpu_hppa_put_psw(env, env->cr[CR_IPSW]);
>   }
>
> -void HELPER(rfi_r)(CPUHPPAState *env)
> +void HELPER(getshadowregs)(CPUHPPAState *env)
>   {
>       env->gr[1] = env->shadow[0];
>       env->gr[8] = env->shadow[1];
> @@ -703,6 +703,11 @@ void HELPER(rfi_r)(CPUHPPAState *env)
>       env->gr[17] = env->shadow[4];
>       env->gr[24] = env->shadow[5];
>       env->gr[25] = env->shadow[6];
> +}
> +
> +void HELPER(rfi_r)(CPUHPPAState *env)
> +{
> +    helper_getshadowregs(env);
>       helper_rfi(env);
>   }
>   #endif
> diff --git a/target/hppa/translate.c b/target/hppa/translate.c
> index c6195590f8..5c0b1eb274 100644
> --- a/target/hppa/translate.c
> +++ b/target/hppa/translate.c
> @@ -2393,6 +2393,16 @@ static bool trans_reset(DisasContext *ctx, arg_reset *a)
>   #endif
>   }
>
> +static bool trans_getshadowregs(DisasContext *ctx, arg_getshadowregs *a)
> +{
> +    CHECK_MOST_PRIVILEGED(EXCP_PRIV_OPR);
> +#ifndef CONFIG_USER_ONLY
> +    nullify_over(ctx);
> +    gen_helper_getshadowregs(cpu_env);
> +    return nullify_end(ctx);
> +#endif
> +}

Why not add getshadowregs opcode in a preliminary patch? That would be
easier to review.

Preferably split and using MACHINE_TYPE_NAME():
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>