Re: [PATCH 1/3] ppc: Add QOM interface for machine check injection

["Cédric Le Goater" <clg@kaod.org>]

On 10/13/21 23:40, Cédric Le Goater wrote:
> From: Nicholas Piggin <npiggin@gmail.com>
> 
> This implements a machine check injection framework and defines a
> 'mce' monitor command for ppc.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> [ clg: - moved definition under "hw/ppc/mce.h"
>         - renamed to PPCMceInjection
>         - simplified injection call in hmp_mce
>         - QMP support ]
> Message-Id: <20200325144147.221875-4-npiggin@gmail.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>


I did not initially copy the QAPI and HMP maintainers :/

One important question is about the stability of the API. MCE is
implementation specific and may change with CPUs. How much of it
can we change once it is merged ? May be this is not the right
approach.


Thanks,

C.

> ---
>   qapi/misc-target.json | 26 ++++++++++++++++++++
>   include/hw/ppc/mce.h  | 31 ++++++++++++++++++++++++
>   target/ppc/monitor.c  | 56 +++++++++++++++++++++++++++++++++++++++++++
>   hmp-commands.hx       | 20 +++++++++++++++-
>   4 files changed, 132 insertions(+), 1 deletion(-)
>   create mode 100644 include/hw/ppc/mce.h
> 
> diff --git a/qapi/misc-target.json b/qapi/misc-target.json
> index 594fbd1577fa..b1456901893c 100644
> --- a/qapi/misc-target.json
> +++ b/qapi/misc-target.json
> @@ -394,3 +394,29 @@
>   #
>   ##
>   { 'command': 'query-sgx-capabilities', 'returns': 'SGXInfo', 'if': 'TARGET_I386' }
> +
> +##
> +# @mce:
> +#
> +# This command injects a machine check exception
> +#
> +# @cpu-index: CPU number on which to inject the machine check exception
> +#
> +# @srr1-mask : possible reasons for the exception
> +#
> +# @dsisr : more reasons
> +#
> +# @dar : effective address of next instruction
> +#
> +# @recovered : recoverable exception. Set MSR[RI]
> +#
> +# Since: 6.2
> +#
> +##
> +{ 'command': 'mce',
> +  'data': { 'cpu-index': 'uint32',
> +            'srr1-mask': 'uint64',
> +            'dsisr': 'uint32',
> +            'dar': 'uint64',
> +            'recovered': 'bool' },
> +  'if': 'TARGET_PPC' }
> diff --git a/include/hw/ppc/mce.h b/include/hw/ppc/mce.h
> new file mode 100644
> index 000000000000..b2b7dfa3342c
> --- /dev/null
> +++ b/include/hw/ppc/mce.h
> @@ -0,0 +1,31 @@
> +/*
> + * Copyright (c) 2021, IBM Corporation.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef HW_PPC_MCE_H
> +#define HW_PPC_MCE_H
> +
> +typedef struct PPCMceInjectionParams {
> +    uint64_t srr1_mask;
> +    uint32_t dsisr;
> +    uint64_t dar;
> +    bool recovered;
> +} PPCMceInjectionParams;
> +
> +typedef struct PPCMceInjection PPCMceInjection;
> +
> +#define TYPE_PPC_MCE_INJECTION "ppc-mce-injection"
> +#define PPC_MCE_INJECTION(obj) \
> +    INTERFACE_CHECK(PPCMceInjection, (obj), TYPE_PPC_MCE_INJECTION)
> +typedef struct PPCMceInjectionClass PPCMceInjectionClass;
> +DECLARE_CLASS_CHECKERS(PPCMceInjectionClass, PPC_MCE_INJECTION,
> +                       TYPE_PPC_MCE_INJECTION)
> +
> +struct PPCMceInjectionClass {
> +    InterfaceClass parent_class;
> +    void (*inject_mce)(CPUState *cs, PPCMceInjectionParams *p);
> +};
> +
> +#endif
> diff --git a/target/ppc/monitor.c b/target/ppc/monitor.c
> index a475108b2dbc..ae1a047e86de 100644
> --- a/target/ppc/monitor.c
> +++ b/target/ppc/monitor.c
> @@ -23,11 +23,15 @@
>    */
>   
>   #include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qapi/qapi-commands-misc-target.h"
>   #include "cpu.h"
>   #include "monitor/monitor.h"
>   #include "qemu/ctype.h"
>   #include "monitor/hmp-target.h"
>   #include "monitor/hmp.h"
> +#include "qapi/qmp/qdict.h"
> +#include "hw/ppc/mce.h"
>   
>   static target_long monitor_get_ccr(Monitor *mon, const struct MonitorDef *md,
>                                      int val)
> @@ -76,6 +80,48 @@ void hmp_info_tlb(Monitor *mon, const QDict *qdict)
>       dump_mmu(env1);
>   }
>   
> +void qmp_mce(uint32_t cpu_index, uint64_t srr1_mask, uint32_t dsisr,
> +             uint64_t dar, bool recovered, Error **errp)
> +{
> +    PPCMceInjection *mce = (PPCMceInjection *)
> +        object_dynamic_cast(qdev_get_machine(), TYPE_PPC_MCE_INJECTION);
> +    CPUState *cs;
> +
> +    if (!mce) {
> +        error_setg(errp, "MCE injection not supported on this machine");
> +        return;
> +    }
> +
> +    cs = qemu_get_cpu(cpu_index);
> +
> +    if (cs != NULL) {
> +        PPCMceInjectionClass *mcec = PPC_MCE_INJECTION_GET_CLASS(mce);
> +        PPCMceInjectionParams p = {
> +            .srr1_mask = srr1_mask,
> +            .dsisr = dsisr,
> +            .dar = dar,
> +            .recovered = recovered,
> +        };
> +        mcec->inject_mce(cs, &p);
> +    }
> +}
> +
> +void hmp_mce(Monitor *mon, const QDict *qdict)
> +{
> +    uint32_t cpu_index = qdict_get_int(qdict, "cpu_index");
> +    uint64_t srr1_mask = qdict_get_int(qdict, "srr1_mask");
> +    uint32_t dsisr = qdict_get_int(qdict, "dsisr");
> +    uint64_t dar = qdict_get_int(qdict, "dar");
> +    bool recovered = qdict_get_int(qdict, "recovered");
> +    Error *err = NULL;
> +
> +    qmp_mce(cpu_index, srr1_mask, dsisr, dar, recovered, &err);
> +    if (err) {
> +        hmp_handle_error(mon, err);
> +        return;
> +    }
> +}
> +
>   const MonitorDef monitor_defs[] = {
>       { "fpscr", offsetof(CPUPPCState, fpscr) },
>       /* Next instruction pointer */
> @@ -156,3 +202,13 @@ int target_get_monitor_def(CPUState *cs, const char *name, uint64_t *pval)
>   
>       return -EINVAL;
>   }
> +
> +static const TypeInfo type_infos[] = {
> +    {
> +        .name = TYPE_PPC_MCE_INJECTION,
> +        .parent = TYPE_INTERFACE,
> +        .class_size = sizeof(PPCMceInjectionClass),
> +    },
> +};
> +
> +DEFINE_TYPES(type_infos);
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index cf723c69acb7..15d939ae096e 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1461,12 +1461,30 @@ ERST
>           .cmd        = hmp_mce,
>       },
>   
> -#endif
>   SRST
>   ``mce`` *cpu* *bank* *status* *mcgstatus* *addr* *misc*
>     Inject an MCE on the given CPU (x86 only).
>   ERST
>   
> +#endif
> +
> +#if defined(TARGET_PPC)
> +
> +    {
> +        .name       = "mce",
> +        .args_type  = "cpu_index:i,srr1_mask:l,dsisr:i,dar:l,recovered:i",
> +        .params     = "cpu srr1_mask dsisr dar recovered",
> +        .help       = "inject a MCE on the given CPU",
> +        .cmd        = hmp_mce,
> +    },
> +
> +SRST
> +``mce`` *cpu* *srr1_mask* *dsisr* *dar* *recovered*
> +  Inject an MCE on the given CPU (PPC only).
> +ERST
> +
> +#endif
> +
>       {
>           .name       = "getfd",
>           .args_type  = "fdname:s",
>