Re: [Qemu-devel] [PATCH 4/4] target-ppc: Handle NMI guest exit

[Thomas Huth]

On 11/11/15 18:16, Aravinda Prasad wrote:
> Memory error such as bit flips that cannot be corrected
> by hardware are passed on to the kernel for handling.
> If the memory address in error belongs to guest then
> guest kernel is responsible for taking suitable action.
> Patch [1] enhances KVM to exit guest with exit reason
> set to KVM_EXIT_NMI in such cases.
> 
> This patch handles KVM_EXIT_NMI exit. If the guest OS
> has registered the machine check handling routine by
> calling "ibm,nmi-register", then the handler builds
> the error log and invokes the registered handler else
> invokes the handler at 0x200.
> 
> [1] http://marc.info/?l=kvm-ppc&m=144726114408289
> 
> Signed-off-by: Aravinda Prasad <address@hidden>
> ---
>  target-ppc/kvm.c     |   69 +++++++++++++++++++++++++++++++++++++++++++
>  target-ppc/kvm_ppc.h |   81 
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 150 insertions(+)
> 
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 110436d..e2e5170 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -1665,6 +1665,11 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run 
> *run)
>          ret = 0;
>          break;
>  
> +    case KVM_EXIT_NMI:
> +        DPRINTF("handle NMI exception\n");
> +        ret = kvm_handle_nmi(cpu);
> +        break;
> +
>      default:
>          fprintf(stderr, "KVM: unknown exit reason %d\n", run->exit_reason);
>          ret = -1;
> @@ -2484,3 +2489,67 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
>  {
>      return data & 0xffff;
>  }
> +
> +int kvm_handle_nmi(PowerPCCPU *cpu)
> +{
> +    struct rtas_mc_log mc_log;
> +    CPUPPCState *env = &cpu->env;
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> +
> +    cpu_synchronize_state(CPU(ppc_env_get_cpu(env)));
> +
> +    /* Properly set bits in MSR before we invoke the handler */
> +    env->msr = 0;
> +
> +    if (!(*pcc->interrupts_big_endian)(cpu)) {
> +        env->msr |= (1ULL << MSR_LE);
> +    }
> +
> +#ifdef TARGET_PPC64
> +    env->msr |= (1ULL << MSR_SF);
> +#endif
> +
> +    if (!spapr->guest_machine_check_addr) {
> +        /*
> +         * If OS has not registered with "ibm,nmi-register"
> +         * jump to 0x200
> +         */

Shouldn't you also check MSR_ME here first and enter checkstop when
machine checks are disabled?
Also I think you have to set up some more registers for machine check
interrupts, like SRR0 and SRR1?

> +        env->nip = 0x200;
> +        return 0;
> +    }
> +
> +    qemu_mutex_lock(&spapr->mc_in_progress);

Using a mutex here is definitely wrong. The kvm_arch_handle_exit() code
is run under the Big QEMU Lock™ (see qemu_mutex_lock_iothread() in
kvm_cpu_exec()), so if you would ever get one thread waiting for this
mutex here, it could never be unlocked again in rtas_ibm_nmi_interlock()
because the other code would wait forever to get the BQL ==> Deadlock.

I think if you want to be able to handle multiple NMIs at once, you
likely need something like an error log per CPU instead. And if an NMI
happens one CPU while there is already a NMI handler running on the very
same CPU, you could likely simply track this with an boolean variable
and put the CPU into checkstop if this happens?

> +    /* Set error log fields */
> +    mc_log.r3 = env->gpr[3];
> +    mc_log.err_log.byte0 = 0;
> +    mc_log.err_log.byte1 =
> +        (RTAS_SEVERITY_ERROR_SYNC << RTAS_ELOG_SEVERITY_SHIFT);
> +    mc_log.err_log.byte1 |=
> +        (RTAS_DISP_NOT_RECOVERED << RTAS_ELOG_DISPOSITION_SHIFT);
> +    mc_log.err_log.byte2 =
> +        (RTAS_INITIATOR_MEMORY << RTAS_ELOG_INITIATOR_SHIFT);
> +    mc_log.err_log.byte2 |= RTAS_TARGET_MEMORY;
> +
> +    if (env->spr[SPR_DSISR] & P7_DSISR_MC_UE) {
> +        mc_log.err_log.byte3 = RTAS_TYPE_ECC_UNCORR;
> +    } else {
> +        mc_log.err_log.byte3 = 0;
> +    }
> +
> +    /* Handle all Host/Guest LE/BE combinations */
> +    if (env->msr & (1ULL << MSR_LE)) {
> +        mc_log.r3 = cpu_to_le64(mc_log.r3);
> +    } else {
> +        mc_log.r3 = cpu_to_be64(mc_log.r3);
> +    }
> +
> +    cpu_physical_memory_write(spapr->rtas_addr + RTAS_ERRLOG_OFFSET,
> +                              &mc_log, sizeof(mc_log));
> +
> +    env->nip = spapr->guest_machine_check_addr;
> +    env->gpr[3] = spapr->rtas_addr + RTAS_ERRLOG_OFFSET;
> +
> +    return 0;
> +}
> diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
> index 5c1d334..1172735 100644
> --- a/target-ppc/kvm_ppc.h
> +++ b/target-ppc/kvm_ppc.h
> @@ -53,6 +53,87 @@ void kvmppc_hash64_free_pteg(uint64_t token);
>  void kvmppc_hash64_write_pte(CPUPPCState *env, target_ulong pte_index,
>                               target_ulong pte0, target_ulong pte1);
>  bool kvmppc_has_cap_fixup_hcalls(void);
> +int kvm_handle_nmi(PowerPCCPU *cpu);
> +
> +/* Offset from rtas-base where error log is placed */
> +#define RTAS_ERRLOG_OFFSET       (0x200)

Why paranthesis here?

> +#define RTAS_ELOG_SEVERITY_SHIFT         0x5
> +#define RTAS_ELOG_DISPOSITION_SHIFT      0x3
> +#define RTAS_ELOG_INITIATOR_SHIFT        0x4
> +
> +/*
> + * Only required RTAS event severity, disposition, initiator
> + * target and type are copied from arch/powerpc/include/asm/rtas.h
> + */
> +
> +/* RTAS event severity */
> +#define RTAS_SEVERITY_ERROR_SYNC    0x3
> +
> +/* RTAS event disposition */
> +#define RTAS_DISP_NOT_RECOVERED     0x2
> +
> +/* RTAS event initiator */
> +#define RTAS_INITIATOR_MEMORY       0x4
> +
> +/* RTAS event target */
> +#define RTAS_TARGET_MEMORY          0x4
> +
> +/* RTAS event type */
> +#define RTAS_TYPE_ECC_UNCORR        0x09
> +
> +/*
> + * Currently KVM only passes on the uncorrected machine
> + * check memory error to guest. Other machine check errors
> + * such as SLB multi-hit and TLB multi-hit are recovered
> + * in KVM and are not passed on to guest.
> + *
> + * DSISR Bit for uncorrected machine check error. Based
> + * on arch/powerpc/include/asm/mce.h
> + */
> +#define PPC_BIT(bit)                (0x8000000000000000ULL >> bit)
> +#define P7_DSISR_MC_UE              (PPC_BIT(48))  /* P8 too */
> +
> +/* Adopted from kernel source arch/powerpc/include/asm/rtas.h */
> +struct rtas_error_log {
> +    /* Byte 0 */
> +    uint8_t     byte0;          /* Architectural version */
> +
> +    /* Byte 1 */
> +    uint8_t     byte1;
> +    /* XXXXXXXX
> +     * XXX      3: Severity level of error
> +     *    XX    2: Degree of recovery
> +     *      X   1: Extended log present?
> +     *       XX 2: Reserved
> +     */
> +
> +    /* Byte 2 */
> +    uint8_t     byte2;
> +    /* XXXXXXXX
> +     * XXXX     4: Initiator of event
> +     *     XXXX 4: Target of failed operation
> +     */
> +    uint8_t     byte3;          /* General event or error*/
> +    __be32      extended_log_length;    /* length in bytes */
> +    unsigned char   buffer[1];      /* Start of extended log */
> +                                /* Variable length.      */
> +};
> +
> +/*
> + * Data format in RTAS-Blob
> + *
> + * This structure contains error information related to Machine
> + * Check exception. This is filled up and copied to rtas-blob
> + * upon machine check exception. The address of rtas-blob is
> + * passed on to OS registered machine check notification
> + * routines upon machine check exception
> + */
> +struct rtas_mc_log {
> +    target_ulong r3;
> +    struct rtas_error_log err_log;
> +};
> +

QEMU coding style is normally to use CamelCase for type names, see
chapter 3 in CODING_STYLE.

 Thomas