target-ppc/spapr: Add FWNMI support in QEMU for PowerKVM guests

Extend rtas-blob to accommodate error log. Error log structure is saved in rtas space upon a machine check exception.

Signed-off-by Aravinda Prasad <[email protected]>

---
 hw/ppc/spapr.c |    4 ++++
 1 file changed, 4 insertions(+)

hw/ppc/spapr.c +4 -0

index 05926a3..b7b9e09 100644
@@ -1556,6 +1556,10 @@ static void ppc_spapr_init(MachineState *machine)
         exit(1);
     }
     spapr->rtas_size = get_image_size(filename);
+
+    /* Resize blob to accommodate error log. */
+    spapr->rtas_size = TARGET_PAGE_ALIGN(spapr->rtas_size);
+
     spapr->rtas_blob = g_malloc(spapr->rtas_size);
     if (load_image_size(filename, spapr->rtas_blob, spapr->rtas_size) < 0) {
         error_report("Could not load LPAR rtas '%s'", filename);

Extend rtas-blob to accommodate error log. Error log structure is saved in rtas space upon a machine check exception.

Signed-off-by Aravinda Prasad <[email protected]>

---
 hw/ppc/spapr.c |    4 ++++
 1 file changed, 4 insertions(+)

hw/ppc/spapr.c +0 -0

index 05926a3..b7b9e09 100644

Quoted 14 lines · Aravinda Prasad

Extend rtas-blob to accommodate error log. Error log structure is saved in rtas space upon a machine check exception.

Signed-off-by Aravinda Prasad <[email protected]>

---
 hw/ppc/spapr.c |    4 ++++
 1 file changed, 4 insertions(+)

hw/ppc/spapr.c +0 -0

index 05926a3..b7b9e09 100644

hw/ppc/spapr.c static void ppc_spapr_init(MachineState *machine) · line 1565

Quoted 10 lines

hw/ppc/spapr.c +4 -0

@@ -1556,6 +1556,10 @@ static void ppc_spapr_init(MachineState *machine)
         exit(1);
     }
     spapr->rtas_size = get_image_size(filename);
+
+    /* Resize blob to accommodate error log. */
+    spapr->rtas_size = TARGET_PAGE_ALIGN(spapr->rtas_size);
+
     spapr->rtas_blob = g_malloc(spapr->rtas_size);
     if (load_image_size(filename, spapr->rtas_blob, spapr->rtas_size) < 0) {
         error_report("Could not load LPAR rtas '%s'", filename);

Sorry to say that, but this patch is horrible!

1) If the rtas blob ever gets bigger than 512 bytes, we will get "random" corruption of the RTAS code later when an NMI occurs since the mc log is blindly copied into the RTAS area later!
==> Please add an "assert(spapr->rtas_size < RTAS_ERRLOG_OFFSET)" at the beginning of your patch.

2) Why resizing with TARGET_PAGE_ALIGN() ? In the very worst case, this would not change the size at all (if the rtas_size is already a multiple of PAGE_SIZE)
==> Please set the size to a proper value like
RTAS_ERRLOG_OFFSET + sizeof(struct rtas_mc_log)
instead!

Quoted 14 lines · Aravinda Prasad

Extend rtas-blob to accommodate error log. Error log structure is saved in rtas space upon a machine check exception.

Signed-off-by Aravinda Prasad <[email protected]>

---
 hw/ppc/spapr.c |    4 ++++
 1 file changed, 4 insertions(+)

hw/ppc/spapr.c +0 -0

index 05926a3..b7b9e09 100644

hw/ppc/spapr.c static void ppc_spapr_init(MachineState *machine) · line 1565

Quoted 10 lines

hw/ppc/spapr.c +4 -0

@@ -1556,6 +1556,10 @@ static void ppc_spapr_init(MachineState *machine)
         exit(1);
     }
     spapr->rtas_size = get_image_size(filename);
+
+    /* Resize blob to accommodate error log. */
+    spapr->rtas_size = TARGET_PAGE_ALIGN(spapr->rtas_size);
+
     spapr->rtas_blob = g_malloc(spapr->rtas_size);
     if (load_image_size(filename, spapr->rtas_blob, spapr->rtas_size) < 0) {
         error_report("Could not load LPAR rtas '%s'", filename);

Sorry to say that, but this patch is horrible!

1) If the rtas blob ever gets bigger than 512 bytes, we will get "random" corruption of the RTAS code later when an NMI occurs since the mc log is blindly copied into the RTAS area later!
==> Please add an "assert(spapr->rtas_size < RTAS_ERRLOG_OFFSET)" at the beginning of your patch.

2) Why resizing with TARGET_PAGE_ALIGN() ? In the very worst case, this would not change the size at all (if the rtas_size is already a multiple of PAGE_SIZE)
==> Please set the size to a proper value like
RTAS_ERRLOG_OFFSET + sizeof(struct rtas_mc_log)
instead!

Thomas

Receive updates from SLOF about the updated rtas-base. A separate patch for SLOF [1] adds functionality to invoke a private HCALL whenever OS issues instantiate-rtas with a new rtas-base.

This is required as QEMU needs to know the updated rtas-base as it allocates error reporting structure in RTAS space upon a machine check exception.

[1] https://lists.ozlabs.org/pipermail/linuxppc-dev/2014-August/120386.html

Signed-off-by Aravinda Prasad <[email protected]>

---
 hw/ppc/spapr_hcall.c   |    8 ++++++++
 include/hw/ppc/spapr.h |    3 ++-
 2 files changed, 10 insertions(+), 1 deletion(-)

hw/ppc/spapr_hcall.c +8 -0

index 652ddf6..9703ed2 100644
@@ -580,6 +580,13 @@ static target_ulong h_rtas(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                            nret, rtas_r3 + 12 + 4*nargs);
 }
 
+static target_ulong h_rtas_update(PowerPCCPU *cpu, sPAPRMachineState *spapr,
+                                  target_ulong opcode, target_ulong *args)
+{
+    spapr->rtas_addr = args[0];
+    return 0;
+}
+
 static target_ulong h_logical_load(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                                    target_ulong opcode, target_ulong *args)
 {

@@ -1008,6 +1015,7 @@ static void hypercall_register_types(void)
 
     /* qemu/KVM-PPC specific hcalls */
     spapr_register_hypercall(KVMPPC_H_RTAS, h_rtas);
+    spapr_register_hypercall(KVMPPC_H_RTAS_UPDATE, h_rtas_update);
 
     spapr_register_hypercall(H_SET_MODE, h_set_mode);
 

include/hw/ppc/spapr.h +2 -1

index 91a61ab..b5cadd7 100644
@@ -347,7 +347,8 @@ struct sPAPRMachineState {
 #define KVMPPC_H_LOGICAL_MEMOP  (KVMPPC_HCALL_BASE + 0x1)
 /* Client Architecture support */
 #define KVMPPC_H_CAS            (KVMPPC_HCALL_BASE + 0x2)
-#define KVMPPC_HCALL_MAX        KVMPPC_H_CAS
+#define KVMPPC_H_RTAS_UPDATE    (KVMPPC_HCALL_BASE + 0x3)
+#define KVMPPC_HCALL_MAX        KVMPPC_H_RTAS_UPDATE
 
 typedef struct sPAPRDeviceTreeUpdateHeader {
     uint32_t version_id;

Quoted 9 lines · Aravinda Prasad

Receive updates from SLOF about the updated rtas-base. A separate patch for SLOF [1] adds functionality to invoke a private HCALL whenever OS issues instantiate-rtas with a new rtas-base.

This is required as QEMU needs to know the updated rtas-base as it allocates error reporting structure in RTAS space upon a machine check exception.

Quoted 3 lines

[1] https://lists.ozlabs.org/pipermail/linuxppc-dev/2014-August/120386.html

Signed-off-by Aravinda Prasad <[email protected]>

Reviewed-by David Gibson <[email protected]>

With the obvious proviso that the SLOF update needs to be merged (both to SLOF and to qemu) first.

Quoted 10 lines · David Gibson

Quoted 9 lines · Aravinda Prasad

Receive updates from SLOF about the updated rtas-base. A separate patch for SLOF [1] adds functionality to invoke a private HCALL whenever OS issues instantiate-rtas with a new rtas-base.

This is required as QEMU needs to know the updated rtas-base as it allocates error reporting structure in RTAS space upon a machine check exception.

Quoted 8 lines

Quoted 3 lines

[1] https://lists.ozlabs.org/pipermail/linuxppc-dev/2014-August/120386.html

Signed-off-by Aravinda Prasad <[email protected]>

Reviewed-by David Gibson <[email protected]>

With the obvious proviso that the SLOF update needs to be merged (both to SLOF and to qemu) first.

The patch is already part of SLOF tree and is there in qemu:

commit f9a60de30492863811c2cdf6f28988c9e8a2c3d9

Author: Nikunj A. Dadhania <[email protected]>
Date:   Mon Aug 25 18:30:10 2014 +0530

Add private HCALL to inform updated RTAS base and entry

This patch adds a private HCALL to inform qemu the updated rtas-base and rtas-entry address when OS invokes the call "instantiate-rtas". This is required as qemu allocates the error reporting structure in RTAS space upon a machine check exception and hence needs to know the updated RTAS.

Enhancements to qemu to handle the private HCALL, prepare error log and invoke machine check notification routine are in a separate patch.

Signed-off-by Nikunj A. Dadhania <[email protected]>

Signed-off-by Aravinda Prasad <[email protected]>

Signed-off-by Nikunj A Dadhania <[email protected]>

This patch adds support in QEMU to handle "ibm,nmi-register" and "ibm,nmi-interlock" RTAS calls.

The machine check notification address is saved when the OS issues "ibm,nmi-register" RTAS call.

This patch also handles the case when multiple processors experience machine check at or about the same time by handling "ibm,nmi-interlock" call. In such cases, as per PAPR, subsequent processors serialize waiting for the first processor to issue the "ibm,nmi-interlock" call. The second processor waits till the first processor, which also received a machine check error, is done reading the error log. The first processor issues "ibm,nmi-interlock" call when the error log is consumed. This patch implements the releasing part of the error-log while subsequent patch (which builds error log) handles the locking part.

Signed-off-by Aravinda Prasad <[email protected]>

---
 hw/ppc/spapr_rtas.c    |   29 +++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h |    8 +++++++-
 2 files changed, 36 insertions(+), 1 deletion(-)

hw/ppc/spapr_rtas.c +12 -0

index 9869bc9..fd4d2af 100644
@@ -597,6 +597,31 @@ out:
     rtas_st(rets, 0, rc);
 }
 
+static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
+                                  sPAPRMachineState *spapr,
+                                  uint32_t token, uint32_t nargs,
+                                  target_ulong args,
+                                  uint32_t nret, target_ulong rets)
+{
+    qemu_mutex_init(&spapr->mc_in_progress);
+    spapr->guest_machine_check_addr = rtas_ld(args, 1);
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+}
+
+static void rtas_ibm_nmi_interlock(PowerPCCPU *cpu,

hw/ppc/spapr_rtas.c +7 -0

+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+}
+
 static struct rtas_call {
     const char *name;
     spapr_rtas_fn fn;

@@ -747,6 +772,10 @@ static void core_rtas_register_types(void)
                         rtas_get_sensor_state);
     spapr_rtas_register(RTAS_IBM_CONFIGURE_CONNECTOR, "ibm,configure-connector",
                         rtas_ibm_configure_connector);
+    spapr_rtas_register(RTAS_IBM_NMI_REGISTER, "ibm,nmi-register",
+                        rtas_ibm_nmi_register);
+    spapr_rtas_register(RTAS_IBM_NMI_INTERLOCK, "ibm,nmi-interlock",
+                        rtas_ibm_nmi_interlock);
 }
 
 type_init(core_rtas_register_types)

include/hw/ppc/spapr.h +7 -1

index b5cadd7..0a31d15 100644
@@ -74,6 +74,10 @@ struct sPAPRMachineState {
     /* RTAS state */
     QTAILQ_HEAD(, sPAPRConfigureConnectorState) ccs_list;
 
+    /* State related to "ibm,nmi-register" and "ibm,nmi-interlock" calls */
+    target_ulong guest_machine_check_addr;
+    QemuMutex mc_in_progress;
+
     /*< public >*/
     char *kvm_type;
 };

@@ -458,8 +462,10 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi);
 #define RTAS_IBM_SET_SLOT_RESET                 (RTAS_TOKEN_BASE + 0x23)
 #define RTAS_IBM_CONFIGURE_PE                   (RTAS_TOKEN_BASE + 0x24)
 #define RTAS_IBM_SLOT_ERROR_DETAIL              (RTAS_TOKEN_BASE + 0x25)
+#define RTAS_IBM_NMI_REGISTER                   (RTAS_TOKEN_BASE + 0x26)
+#define RTAS_IBM_NMI_INTERLOCK                  (RTAS_TOKEN_BASE + 0x27)
 
-#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x26)
+#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x28)
 
 /* RTAS ibm,get-system-parameter token values */
 #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS      20

Quoted 44 lines · Aravinda Prasad

This patch adds support in QEMU to handle "ibm,nmi-register" and "ibm,nmi-interlock" RTAS calls.

The machine check notification address is saved when the OS issues "ibm,nmi-register" RTAS call.

This patch also handles the case when multiple processors experience machine check at or about the same time by handling "ibm,nmi-interlock" call. In such cases, as per PAPR, subsequent processors serialize waiting for the first processor to issue the "ibm,nmi-interlock" call. The second processor waits till the first processor, which also received a machine check error, is done reading the error log. The first processor issues "ibm,nmi-interlock" call when the error log is consumed. This patch implements the releasing part of the error-log while subsequent patch (which builds error log) handles the locking part.

Signed-off-by Aravinda Prasad <[email protected]>

---
 hw/ppc/spapr_rtas.c    |   29 +++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h |    8 +++++++-
 2 files changed, 36 insertions(+), 1 deletion(-)

hw/ppc/spapr_rtas.c +12 -0

index 9869bc9..fd4d2af 100644
@@ -597,6 +597,31 @@ out:
     rtas_st(rets, 0, rc);
 }
 
+static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
+                                  sPAPRMachineState *spapr,
+                                  uint32_t token, uint32_t nargs,
+                                  target_ulong args,
+                                  uint32_t nret, target_ulong rets)
+{
+    qemu_mutex_init(&spapr->mc_in_progress);
+    spapr->guest_machine_check_addr = rtas_ld(args, 1);
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+}
+
+static void rtas_ibm_nmi_interlock(PowerPCCPU *cpu,

hw/ppc/spapr_rtas.c out: · line 621

Quoted 10 lines

hw/ppc/spapr_rtas.c +10 -0

+                                   sPAPRMachineState *spapr,
+                                   uint32_t token, uint32_t nargs,
+                                   target_ulong args,
+                                   uint32_t nret, target_ulong rets)
+{
+    /*
+     * VCPU issuing "ibm,nmi-interlock" is done with NMI handling,
+     * hence unlock mc_in_progress.
+     */
+    qemu_mutex_unlock(&spapr->mc_in_progress);

Hrm... allowing the guest direct control over a qemu internal mutex sets off alarm bells for me.

It could be ok, depending on exactly where the mutex is taken, and whether it has a timeout and so forth, but it makes me nervous.

If nothing else this needs some substantial comments explaining the locking scheme and why it's safe to give userspace direct control over the unlock.

Quoted 46 lines

hw/ppc/spapr_rtas.c +7 -0

+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+}
+
 static struct rtas_call {
     const char *name;
     spapr_rtas_fn fn;

@@ -747,6 +772,10 @@ static void core_rtas_register_types(void)
                         rtas_get_sensor_state);
     spapr_rtas_register(RTAS_IBM_CONFIGURE_CONNECTOR, "ibm,configure-connector",
                         rtas_ibm_configure_connector);
+    spapr_rtas_register(RTAS_IBM_NMI_REGISTER, "ibm,nmi-register",
+                        rtas_ibm_nmi_register);
+    spapr_rtas_register(RTAS_IBM_NMI_INTERLOCK, "ibm,nmi-interlock",
+                        rtas_ibm_nmi_interlock);
 }
 
 type_init(core_rtas_register_types)

include/hw/ppc/spapr.h +7 -1

index b5cadd7..0a31d15 100644
@@ -74,6 +74,10 @@ struct sPAPRMachineState {
     /* RTAS state */
     QTAILQ_HEAD(, sPAPRConfigureConnectorState) ccs_list;
 
+    /* State related to "ibm,nmi-register" and "ibm,nmi-interlock" calls */
+    target_ulong guest_machine_check_addr;
+    QemuMutex mc_in_progress;
+
     /*< public >*/
     char *kvm_type;
 };

@@ -458,8 +462,10 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi);
 #define RTAS_IBM_SET_SLOT_RESET                 (RTAS_TOKEN_BASE + 0x23)
 #define RTAS_IBM_CONFIGURE_PE                   (RTAS_TOKEN_BASE + 0x24)
 #define RTAS_IBM_SLOT_ERROR_DETAIL              (RTAS_TOKEN_BASE + 0x25)
+#define RTAS_IBM_NMI_REGISTER                   (RTAS_TOKEN_BASE + 0x26)
+#define RTAS_IBM_NMI_INTERLOCK                  (RTAS_TOKEN_BASE + 0x27)
 
-#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x26)
+#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x28)
 
 /* RTAS ibm,get-system-parameter token values */
 #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS      20

This patch adds support in QEMU to handle "ibm,nmi-register" and "ibm,nmi-interlock" RTAS calls.

The machine check notification address is saved when the OS issues "ibm,nmi-register" RTAS call.

This patch also handles the case when multiple processors experience machine check at or about the same time by handling "ibm,nmi-interlock" call. In such cases, as per PAPR, subsequent processors serialize waiting for the first processor to issue the "ibm,nmi-interlock" call. The second processor waits till the first processor, which also received a machine check error, is done reading the error log. The first processor issues "ibm,nmi-interlock" call when the error log is consumed. This patch implements the releasing part of the error-log while subsequent patch (which builds error log) handles the locking part.

Signed-off-by Aravinda Prasad <[email protected]>

---
 hw/ppc/spapr_rtas.c    |   29 +++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h |    8 +++++++-
 2 files changed, 36 insertions(+), 1 deletion(-)

hw/ppc/spapr_rtas.c +14 -0

index 9869bc9..fd4d2af 100644
@@ -597,6 +597,31 @@ out:
     rtas_st(rets, 0, rc);
 }
 
+static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
+                                  sPAPRMachineState *spapr,
+                                  uint32_t token, uint32_t nargs,
+                                  target_ulong args,
+                                  uint32_t nret, target_ulong rets)
+{
+    qemu_mutex_init(&spapr->mc_in_progress);
+    spapr->guest_machine_check_addr = rtas_ld(args, 1);
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+}
+
+static void rtas_ibm_nmi_interlock(PowerPCCPU *cpu,
+                                   sPAPRMachineState *spapr,
+                                   uint32_t token, uint32_t nargs,

Quoted 52 lines · Aravinda Prasad

This patch adds support in QEMU to handle "ibm,nmi-register" and "ibm,nmi-interlock" RTAS calls.

The machine check notification address is saved when the OS issues "ibm,nmi-register" RTAS call.

This patch also handles the case when multiple processors experience machine check at or about the same time by handling "ibm,nmi-interlock" call. In such cases, as per PAPR, subsequent processors serialize waiting for the first processor to issue the "ibm,nmi-interlock" call. The second processor waits till the first processor, which also received a machine check error, is done reading the error log. The first processor issues "ibm,nmi-interlock" call when the error log is consumed. This patch implements the releasing part of the error-log while subsequent patch (which builds error log) handles the locking part.

Signed-off-by Aravinda Prasad <[email protected]>

---
 hw/ppc/spapr_rtas.c    |   29 +++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h |    8 +++++++-
 2 files changed, 36 insertions(+), 1 deletion(-)

hw/ppc/spapr_rtas.c +20 -0

index 9869bc9..fd4d2af 100644
@@ -597,6 +597,31 @@ out:
     rtas_st(rets, 0, rc);
 }
 
+static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
+                                  sPAPRMachineState *spapr,
+                                  uint32_t token, uint32_t nargs,
+                                  target_ulong args,
+                                  uint32_t nret, target_ulong rets)
+{
+    qemu_mutex_init(&spapr->mc_in_progress);
+    spapr->guest_machine_check_addr = rtas_ld(args, 1);
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+}
+
+static void rtas_ibm_nmi_interlock(PowerPCCPU *cpu,
+                                   sPAPRMachineState *spapr,
+                                   uint32_t token, uint32_t nargs,
+                                   target_ulong args,
+                                   uint32_t nret, target_ulong rets)
+{
+    /*
+     * VCPU issuing "ibm,nmi-interlock" is done with NMI handling,
+     * hence unlock mc_in_progress.

hw/ppc/spapr_rtas.c out: · line 623

Quoted 4 lines

hw/ppc/spapr_rtas.c +4 -0

+     */
+    qemu_mutex_unlock(&spapr->mc_in_progress);
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+}

Maybe the interlock function should return an error if the nmi_register function has not been called before? OTOH, RTAS is not supposed to do excessive parameter checking, so this is maybe not worth the effort.

Thomas

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 <[email protected]>

---
 target-ppc/kvm.c     |   69 +++++++++++++++++++++++++++++++++++++++++++
 target-ppc/kvm_ppc.h |   81 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 150 insertions(+)

target-ppc/kvm.c +15 -0

index 110436d..e2e5170 100644
@@ -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)));
+

target-ppc/kvm.c +44 -0

+
+    if (!spapr->guest_machine_check_addr) {
+        /*
+         * If OS has not registered with "ibm,nmi-register"
+         * jump to 0x200
+         */
+        env->nip = 0x200;
+        return 0;
+    }
+
+    qemu_mutex_lock(&spapr->mc_in_progress);
+
+    /* 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;
+}

target-ppc/kvm_ppc.h +81 -0

index 5c1d334..1172735 100644
@@ -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)
+
+#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;
+};
+
 
 #else
 

Quoted 57 lines · Aravinda Prasad

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 <[email protected]>

---
 target-ppc/kvm.c     |   69 +++++++++++++++++++++++++++++++++++++++++++
 target-ppc/kvm_ppc.h |   81 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 150 insertions(+)

target-ppc/kvm.c +21 -0

index 110436d..e2e5170 100644
@@ -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);
+    }

target-ppc/kvm.c int kvm_arch_msi_data_to_gsi(uint32_t data) · line 2511

Quoted 4 lines

target-ppc/kvm.c +4 -0

+
+#ifdef TARGET_PPC64
+    env->msr |= (1ULL << MSR_SF);
+#endif

Surely this should depend on what the MSR actually was before, or at the very least the cpu model, rather than just whether qemu has PPC64 support compiled in.

Quoted 138 lines

target-ppc/kvm.c +44 -0

+
+    if (!spapr->guest_machine_check_addr) {
+        /*
+         * If OS has not registered with "ibm,nmi-register"
+         * jump to 0x200
+         */
+        env->nip = 0x200;
+        return 0;
+    }
+
+    qemu_mutex_lock(&spapr->mc_in_progress);
+
+    /* 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;
+}

target-ppc/kvm_ppc.h +81 -0

index 5c1d334..1172735 100644
@@ -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)
+
+#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;
+};
+
 
 #else
 

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 <[email protected]>

---
 target-ppc/kvm.c     |   69 +++++++++++++++++++++++++++++++++++++++++++
 target-ppc/kvm_ppc.h |   81 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 150 insertions(+)

target-ppc/kvm.c +21 -0

index 110436d..e2e5170 100644
@@ -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);
+    }

target-ppc/kvm.c +32 -0

+    /* 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;
+}

target-ppc/kvm_ppc.h +0 -0

index 5c1d334..1172735 100644

target-ppc/kvm_ppc.h +66 -0

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

Quoted 57 lines · Aravinda Prasad

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 <[email protected]>

---
 target-ppc/kvm.c     |   69 +++++++++++++++++++++++++++++++++++++++++++
 target-ppc/kvm_ppc.h |   81 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 150 insertions(+)

target-ppc/kvm.c +21 -0

index 110436d..e2e5170 100644
@@ -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);
+    }

target-ppc/kvm.c int kvm_arch_msi_data_to_gsi(uint32_t data) · line 2517

Quoted 10 lines

target-ppc/kvm.c +10 -0

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

target-ppc/kvm.c int kvm_arch_msi_data_to_gsi(uint32_t data) · line 2522

Quoted 5 lines

target-ppc/kvm.c +5 -0

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

Quoted 80 lines · Thomas Huth

Quoted 57 lines · Aravinda Prasad

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 <[email protected]>

---
 target-ppc/kvm.c     |   69 +++++++++++++++++++++++++++++++++++++++++++
 target-ppc/kvm_ppc.h |   81 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 150 insertions(+)

target-ppc/kvm.c +21 -0

index 110436d..e2e5170 100644
@@ -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);
+    }

target-ppc/kvm.c int kvm_arch_msi_data_to_gsi(uint32_t data) · line 2517

Quoted 10 lines

target-ppc/kvm.c +10 -0

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

Quoted 5 lines

target-ppc/kvm.c +5 -0

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

Quoted 3 lines

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()),

In case you're looking for the calls, I just noticed that the qemu_mutex_lock_iothread() have recently been pushed into kvm_arch_handle_exit() itself.

Quoted 10 lines

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?

Ok, I now had a look into the LoPAPR spec, and if I've got that right, you really have to serialize the NMIs in case they happen at multiple CPUs at the same time. So I guess the best thing you can do here is something like:

   while (spapr->mc_in_progress) {
       /*
        * There is already another NMI in progress, thus we need
        * to yield here to wait until it has been finsihed
        */
       qemu_mutex_unlock_iothread();
       usleep(10);
       qemu_mutex_lock_iothread();
   }
   spapr->mc_in_progress = true;

Also LoPAPR talks about 'subsequent processors report "fatal error previously reported"', so maybe the other processors should report that condition in this case?

And of course you've also got to check that the same CPU is not getting multiple NMIs before the interlock function has been called again.

Thomas

On Thursday 12 November 2015 03:10 PM, Thomas Huth wrote:

Quoted 3 lines

Also LoPAPR talks about 'subsequent processors report "fatal error previously reported"', so maybe the other processors should report that condition in this case?

I feel guest kernel is responsible for that or does that mean that qemu should report the same error, which first processor encountered, for subsequent processors? In that case what if the error encountered by first processor was recovered.

Quoted 2 lines

And of course you've also got to check that the same CPU is not getting multiple NMIs before the interlock function has been called again.

I think it is good to check that. However, shouldn't the guest enable ME until it calls interlock function?

Quoted 2 lines · Aravinda Prasad

On Thursday 12 November 2015 03:10 PM, Thomas Huth wrote:

...

Quoted 8 lines

Quoted 3 lines

Also LoPAPR talks about 'subsequent processors report "fatal error previously reported"', so maybe the other processors should report that condition in this case?

I feel guest kernel is responsible for that or does that mean that qemu should report the same error, which first processor encountered, for subsequent processors? In that case what if the error encountered by first processor was recovered.

I simply refered to this text in LoPAPR:

Multiple processors of the same OS image may experi- ence fatal events at, or about, the same time. The first processor to enter the machine check handling firmware reports the fatal error. Subsequent processors serialize waiting for the first processor to issue the ibm,nmi-interlock call. These subsequent processors report "fatal error previously reported".

Is there code in the host kernel already that takes care of this (I haven't checked)? If so, how does the host kernel know that the event happened "at or about the same time" since you're checking at the QEMU side for the mutex condition?

Quoted 4 lines

Quoted 3 lines

And of course you've also got to check that the same CPU is not getting multiple NMIs before the interlock function has been called again.

Quoted 2 lines

I think it is good to check that. However, shouldn't the guest enable ME until it calls interlock function?

First, the hypervisor should never trust the guest to do the right things. Second, LoPAPR says "the OS permanently relinquishes to firmware the Machine State Register's Machine Check Enable bit", and Paul also said something similar in another mail to this thread, so I think you really have to check this in QEMU instead.

Thomas

Quoted 13 lines · Thomas Huth

Quoted 2 lines · Aravinda Prasad

On Thursday 12 November 2015 03:10 PM, Thomas Huth wrote:

...

Quoted 8 lines

Quoted 3 lines

Also LoPAPR talks about 'subsequent processors report "fatal error previously reported"', so maybe the other processors should report that condition in this case?

I feel guest kernel is responsible for that or does that mean that qemu should report the same error, which first processor encountered, for subsequent processors? In that case what if the error encountered by first processor was recovered.

Quoted 8 lines

I simply refered to this text in LoPAPR:

Multiple processors of the same OS image may experi- ence fatal events at, or about, the same time. The first processor to enter the machine check handling firmware reports the fatal error. Subsequent processors serialize waiting for the first processor to issue the ibm,nmi-interlock call. These subsequent processors report "fatal error previously reported".

Yes, I asked this because I am not clear what "fatal error previously reported" means as described in PAPR.

Quoted 4 lines

Is there code in the host kernel already that takes care of this (I haven't checked)? If so, how does the host kernel know that the event happened "at or about the same time" since you're checking at the QEMU side for the mutex condition?

I don't think the host kernel takes care of this; it simply forwards such errors to QEMU via NMI exit. I feel the time referred by "at or about the same time" is the duration between the registered machine check handler is invoked and the corresponding interlock call is issued by guest, which QEMU knows and is protected by a mutex.

Quoted 11 lines

Quoted 5 lines

Quoted 2 lines

And of course you've also got to check that the same CPU is not getting multiple NMIs before the interlock function has been called again.

I think it is good to check that. However, shouldn't the guest enable ME until it calls interlock function?

First, the hypervisor should never trust the guest to do the right things. Second, LoPAPR says "the OS permanently relinquishes to firmware the Machine State Register's Machine Check Enable bit", and Paul also said something similar in another mail to this thread, so I think you really have to check this in QEMU instead.

Hmm. ok. Since ME is always set when running in guest (assuming guest is not disabling it), we cannot check ME bit to figure out whether the same CPU is getting UEs before interlock is called. One way is to record the CPU ID upon such error and check before invoking registered machine check handler whether that CPU has a pending interlock call. Terminate the guest if there is a pending interlock call for that CPU rather than causing the guest to trigger recursive machine check errors.

Quoted 25 lines · Aravinda Prasad

Quoted 21 lines · Thomas Huth

Quoted 2 lines · Aravinda Prasad

On Thursday 12 November 2015 03:10 PM, Thomas Huth wrote:

...

Quoted 8 lines

Quoted 3 lines

Also LoPAPR talks about 'subsequent processors report "fatal error previously reported"', so maybe the other processors should report that condition in this case?

I feel guest kernel is responsible for that or does that mean that qemu should report the same error, which first processor encountered, for subsequent processors? In that case what if the error encountered by first processor was recovered.

I simply refered to this text in LoPAPR:

Multiple processors of the same OS image may experi- ence fatal events at, or about, the same time. The first processor to enter the machine check handling firmware reports the fatal error. Subsequent processors serialize waiting for the first processor to issue the ibm,nmi-interlock call. These subsequent processors report "fatal error previously reported".

Quoted 2 lines

Yes, I asked this because I am not clear what "fatal error previously reported" means as described in PAPR.

Looking at table "Table 137. RTAS Event Return Format (Fixed Part)" in LoPAPR, there is a "ALREADY_REPORTED" severity - I assume this is what is meant by the cited paragraph?

Quoted 11 lines

Quoted 5 lines

Is there code in the host kernel already that takes care of this (I haven't checked)? If so, how does the host kernel know that the event happened "at or about the same time" since you're checking at the QEMU side for the mutex condition?

I don't think the host kernel takes care of this; it simply forwards such errors to QEMU via NMI exit. I feel the time referred by "at or about the same time" is the duration between the registered machine check handler is invoked and the corresponding interlock call is issued by guest, which QEMU knows and is protected by a mutex.

I agree, that makes sense.

Quoted 12 lines

Quoted 11 lines

Quoted 5 lines

Quoted 2 lines

And of course you've also got to check that the same CPU is not getting multiple NMIs before the interlock function has been called again.

I think it is good to check that. However, shouldn't the guest enable ME until it calls interlock function?

First, the hypervisor should never trust the guest to do the right things. Second, LoPAPR says "the OS permanently relinquishes to firmware the Machine State Register's Machine Check Enable bit", and Paul also said something similar in another mail to this thread, so I think you really have to check this in QEMU instead.

Quoted 7 lines

Hmm. ok. Since ME is always set when running in guest (assuming guest is not disabling it), we cannot check ME bit to figure out whether the same CPU is getting UEs before interlock is called. One way is to record the CPU ID upon such error and check before invoking registered machine check handler whether that CPU has a pending interlock call. Terminate the guest if there is a pending interlock call for that CPU rather than causing the guest to trigger recursive machine check errors.

Do we have some kind of checkstop state emulation in QEMU (sorry, I haven't checked yet)? If yes, it might be nicer to use that and set the guest state to PANIC instead of exiting QEMU directly - i.e. to do something similar like the guest_panicked() function in target-s390x/kvm.c. That way the management layer (libvirt) can decide on its own whether to terminate the guest, reboot or keep it in the crashed state for further analysis.

Thomas

Quoted 17 lines · Thomas Huth

Quoted 16 lines · Aravinda Prasad

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 <[email protected]>

Quoted 9 lines

target-ppc/kvm.c int kvm_arch_msi_data_to_gsi(uint32_t data) · line 2522

Quoted 5 lines

target-ppc/kvm.c +5 -0

+        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()),

In case you're looking for the calls, I just noticed that the qemu_mutex_lock_iothread() have recently been pushed into kvm_arch_handle_exit() itself.

Quoted 9 lines

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?

Ok, I now had a look into the LoPAPR spec, and if I've got that right, you really have to serialize the NMIs in case they happen at multiple CPUs at the same time. So I guess the best thing you can do here is something like:

   while (spapr->mc_in_progress) {
       /*
        * There is already another NMI in progress, thus we need
        * to yield here to wait until it has been finsihed
        */
       qemu_mutex_unlock_iothread();
       usleep(10);
       qemu_mutex_lock_iothread();
   }
   spapr->mc_in_progress = true;

Also LoPAPR talks about 'subsequent processors report "fatal error previously reported"', so maybe the other processors should report that condition in this case?
And of course you've also got to check that the same CPU is not getting multiple NMIs before the interlock function has been called again.

Quoted 2 lines · Thomas Huth

Quoted 1 line · Thomas Huth

On 11/11/15 18:16, Aravinda Prasad wrote:

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

+ 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()),

Quoted 15 lines

Ok, I now had a look into the LoPAPR spec, and if I've got that right, you really have to serialize the NMIs in case they happen at multiple CPUs at the same time. So I guess the best thing you can do here is something like:

   while (spapr->mc_in_progress) {
       /*
        * There is already another NMI in progress, thus we need
        * to yield here to wait until it has been finsihed
        */
       qemu_mutex_unlock_iothread();
       usleep(10);
       qemu_mutex_lock_iothread();
   }
   spapr->mc_in_progress = true;

You should be able to avoid the nasty usleep by using a pthread condition variable. So here you'd have

    while (spapr->mc_in_progress) {
        pthread_cond_wait(&mc_delivery_cond, &qemu_iothread_lock);
    }
    spapr->mc_in_progress = true;

Or.. there may be qemu wrappers around the pthread functions you should be using. Once delivery of a single MC is complete, you'd use pthread_cond_signal() to wake up any additional ones.

pthread_cond_wait automatically drops the specified mutex internally, so access to mc_in_progress itself is still protected by the iothread mutex.

Quoted 3 lines · David Gibson

Quoted 2 lines · Thomas Huth

Quoted 1 line · Thomas Huth

On 11/11/15 18:16, Aravinda Prasad wrote:

[...]

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

+ 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()),

[...]

Quoted 15 lines

Quoted 15 lines

Ok, I now had a look into the LoPAPR spec, and if I've got that right, you really have to serialize the NMIs in case they happen at multiple CPUs at the same time. So I guess the best thing you can do here is something like:

   while (spapr->mc_in_progress) {
       /*
        * There is already another NMI in progress, thus we need
        * to yield here to wait until it has been finsihed
        */
       qemu_mutex_unlock_iothread();
       usleep(10);
       qemu_mutex_lock_iothread();
   }
   spapr->mc_in_progress = true;

[...]

Quoted 12 lines

You should be able to avoid the nasty usleep by using a pthread condition variable. So here you'd have

    while (spapr->mc_in_progress) {
        pthread_cond_wait(&mc_delivery_cond, &qemu_iothread_lock);
    }
    spapr->mc_in_progress = true;

Or.. there may be qemu wrappers around the pthread functions you should be using. Once delivery of a single MC is complete, you'd use pthread_cond_signal() to wake up any additional ones.

Quoted 3 lines

pthread_cond_wait automatically drops the specified mutex internally, so access to mc_in_progress itself is still protected by the iothread mutex.

That's a nice one, didn't know that function yet! And actually, there is already a QEMU wrapper function: qemu_cond_wait() - so this should be used instead since threads on Windows are working differently in QEMU as far as I know.

Quoted 62 lines · Aravinda Prasad

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 <[email protected]>

---
 target-ppc/kvm.c     |   69 +++++++++++++++++++++++++++++++++++++++++++
 target-ppc/kvm_ppc.h |   81 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 150 insertions(+)

target-ppc/kvm.c +26 -0

index 110436d..e2e5170 100644
@@ -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
+

target-ppc/kvm.c int kvm_arch_msi_data_to_gsi(uint32_t data) · line 2517

Quoted 5 lines

target-ppc/kvm.c +5 -0

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

target-ppc/kvm.c int kvm_arch_msi_data_to_gsi(uint32_t data) · line 2522

Quoted 6 lines

target-ppc/kvm.c +5 -0

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

Quoted 38 lines

target-ppc/kvm.c +32 -0

+    /* 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;
+}

target-ppc/kvm_ppc.h +0 -0

index 5c1d334..1172735 100644
@@ -53,6 +53,87 @@ void kvmppc_hash64_free_pteg(uint64_t token);
 void kvmppc_hash64_write_pte(CPUPPCState *env, target_ulong pte_index,

target-ppc/kvm_ppc.h void kvmppc_hash64_free_pteg(uint64_t token); · line 59

Quoted 6 lines

target-ppc/kvm_ppc.h +4 -0

                              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?

Quoted 72 lines

target-ppc/kvm_ppc.h +71 -0

+#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
+ */

target-ppc/kvm_ppc.h void kvmppc_hash64_free_pteg(uint64_t token); · line 136

Quoted 5 lines

target-ppc/kvm_ppc.h +5 -0

+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

Quoted 63 lines · Thomas Huth

Quoted 62 lines · Aravinda Prasad

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 <[email protected]>

---
 target-ppc/kvm.c     |   69 +++++++++++++++++++++++++++++++++++++++++++
 target-ppc/kvm_ppc.h |   81 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 150 insertions(+)

target-ppc/kvm.c +26 -0

index 110436d..e2e5170 100644
@@ -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
+

Quoted 8 lines

target-ppc/kvm.c int kvm_arch_msi_data_to_gsi(uint32_t data) · line 2517

Quoted 5 lines

target-ppc/kvm.c +5 -0

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

Yes, MSR_ME should be checked first.

Quoted 2 lines

Also I think you have to set up some more registers for machine check interrupts, like SRR0 and SRR1?

SRRO and SRR1 of vcpu are properly set in KVM in kvmppc_interrupt_hv. I am not sure if any other registers need to be set.

Quoted 77 lines · Aravinda Prasad

Quoted 63 lines · Thomas Huth

Quoted 62 lines · Aravinda Prasad

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 <[email protected]>

---
 target-ppc/kvm.c     |   69 +++++++++++++++++++++++++++++++++++++++++++
 target-ppc/kvm_ppc.h |   81 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 150 insertions(+)

target-ppc/kvm.c +26 -0

index 110436d..e2e5170 100644
@@ -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
+

Quoted 8 lines

target-ppc/kvm.c int kvm_arch_msi_data_to_gsi(uint32_t data) · line 2517

Quoted 5 lines

target-ppc/kvm.c +5 -0

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

Yes, MSR_ME should be checked first.

Quoted 5 lines

Quoted 2 lines

Also I think you have to set up some more registers for machine check interrupts, like SRR0 and SRR1?

SRRO and SRR1 of vcpu are properly set in KVM in kvmppc_interrupt_hv. I am not sure if any other registers need to be set.

DAR and DSISR are the obvious ones you need to consider, although I suspect they're already set up correctly by the kernel, too.

Quoted 81 lines · David Gibson

Quoted 80 lines · Aravinda Prasad

Quoted 63 lines · Thomas Huth

Quoted 62 lines · Aravinda Prasad

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 <[email protected]>

---
 target-ppc/kvm.c     |   69 +++++++++++++++++++++++++++++++++++++++++++
 target-ppc/kvm_ppc.h |   81 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 150 insertions(+)

target-ppc/kvm.c +26 -0

index 110436d..e2e5170 100644
@@ -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
+

Quoted 8 lines

target-ppc/kvm.c int kvm_arch_msi_data_to_gsi(uint32_t data) · line 2517

Quoted 5 lines

target-ppc/kvm.c +5 -0

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

Yes, MSR_ME should be checked first.

Quoted 2 lines

Also I think you have to set up some more registers for machine check interrupts, like SRR0 and SRR1?

Quoted 5 lines

Quoted 2 lines

SRRO and SRR1 of vcpu are properly set in KVM in kvmppc_interrupt_hv. I am not sure if any other registers need to be set.

DAR and DSISR are the obvious ones you need to consider, although I suspect they're already set up correctly by the kernel, too.

Yes, they are also setup properly by KVM.

Quoted 87 lines · Aravinda Prasad

Quoted 84 lines · David Gibson

Quoted 82 lines · Aravinda Prasad

Quoted 63 lines · Thomas Huth

Quoted 62 lines · Aravinda Prasad

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 <[email protected]>

---
 target-ppc/kvm.c     |   69 +++++++++++++++++++++++++++++++++++++++++++
 target-ppc/kvm_ppc.h |   81 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 150 insertions(+)

target-ppc/kvm.c +26 -0

index 110436d..e2e5170 100644
@@ -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
+

Quoted 8 lines

target-ppc/kvm.c int kvm_arch_msi_data_to_gsi(uint32_t data) · line 2517

Quoted 5 lines

target-ppc/kvm.c +5 -0

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

Yes, MSR_ME should be checked first.

Quoted 2 lines

Also I think you have to set up some more registers for machine check interrupts, like SRR0 and SRR1?

SRRO and SRR1 of vcpu are properly set in KVM in kvmppc_interrupt_hv. I am not sure if any other registers need to be set.

Quoted 4 lines

Quoted 2 lines

DAR and DSISR are the obvious ones you need to consider, although I suspect they're already set up correctly by the kernel, too.

Yes, they are also setup properly by KVM.

Ok, good. Might be worth throwing a comment in the qemu code noting that the kernel has already set up that state properly.

Quoted 48 lines

Quoted 48 lines

target-ppc/kvm_ppc.h +48 -0

+
+/*
+ * 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
+ */

Quoted 8 lines

target-ppc/kvm_ppc.h void kvmppc_hash64_free_pteg(uint64_t token); · line 136

Quoted 5 lines

target-ppc/kvm_ppc.h +5 -0

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

Will take care in the next revision.

Quoted 57 lines · Aravinda Prasad

Quoted 56 lines

Quoted 43 lines

target-ppc/kvm_ppc.h +43 -0

+
+/*
+ * 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

target-ppc/kvm_ppc.h void kvmppc_hash64_free_pteg(uint64_t token); · line 136

Quoted 10 lines

target-ppc/kvm_ppc.h +10 -0

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

Quoted 1 line

Will take care in the next revision.

Just to make it clear - the CamelCase comment is valid for the new rtas_mc_log type but not for rtas_error_log which is a copy of kernel's arch/powerpc/include/asm/rtas.h

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?

Quoted 3 lines · Thomas Huth

Shouldn't you also check MSR_ME here first and enter checkstop when machine checks are disabled?

MSR_ME is a hypervisor resource and is not able to be controlled by HV KVM guests, or in fact by the OS running on the pseries machine target regardless of how it's accelerated or emulated.

What you say would only apply if we had a powernv machine target and we were emulating the whole system, and in that case we wouldn't be using any hcalls, and we wouldn't be doing FWNMI (or at least not at this level).

So the answer is no, MSR_ME will always be set when running in a guest, and we don't ever need to checkstop the virtual machine.

Quoted 14 lines · Paul Mackerras

Quoted 3 lines · Thomas Huth

Shouldn't you also check MSR_ME here first and enter checkstop when machine checks are disabled?

MSR_ME is a hypervisor resource and is not able to be controlled by HV KVM guests, or in fact by the OS running on the pseries machine target regardless of how it's accelerated or emulated.

What you say would only apply if we had a powernv machine target and we were emulating the whole system, and in that case we wouldn't be using any hcalls, and we wouldn't be doing FWNMI (or at least not at this level).

Quoted 2 lines

So the answer is no, MSR_ME will always be set when running in a guest, and we don't ever need to checkstop the virtual machine.

Good point, I missed that sentence about the hypervisor resource in the PowerISA. So QEMU does not have to check this bit here.

But out of curiosity: What happens if a guest disables the ME bit? Is this checked somewhere or simply ignored?

Thomas

Quoted 14 lines · Paul Mackerras

Quoted 3 lines · Thomas Huth

Shouldn't you also check MSR_ME here first and enter checkstop when machine checks are disabled?

MSR_ME is a hypervisor resource and is not able to be controlled by HV KVM guests, or in fact by the OS running on the pseries machine target regardless of how it's accelerated or emulated.

What you say would only apply if we had a powernv machine target and we were emulating the whole system, and in that case we wouldn't be using any hcalls, and we wouldn't be doing FWNMI (or at least not at this level).

Quoted 2 lines

So the answer is no, MSR_ME will always be set when running in a guest, and we don't ever need to checkstop the virtual machine.

Good point, I missed that sentence about the hypervisor resource in the PowerISA. So QEMU does not have to check this bit here.

But out of curiosity: What happens if a guest disables the ME bit? Is this checked somewhere or simply ignored?

This series of patches add support for FWNMI in PowerKVM guests.

Memory error such as bit flips that cannot be corrected by hardware is passed on to the kernel for handling by raising machine check exception (an NMI). Upon such machine check exception, if the address in error belongs to guest then KVM causes a guest exit with KVM_EXIT_NMI exit reason [1].

This patch series adds functionality to pass on such machine check exception to the guest kernel by suitably handling KVM_EXIT_NMI exit and building the error log. This patch series explores the alternate design discussed in the QEMU mailing list [2], [3].

In earlier design [2] KVM invoked guest's NMI interrupt vector 0x200 upon machine check exception, while QEMU patched 0x200 vector to issue hcall to facilitate building and copying of error log into RTAS space.

The new design simplifies couple of things. Thanks to David Gibson for the suggestion:

- Eliminates patching of 0x200 interrupt vector from QEMU, hence, trampoline not required
- Avoids introducing a new private hcall to be called from guest's 0x200 vector to enable QEMU to build error log
- Simplifies re-trying by other processors when one of the processor is consuming/processing the error log.

[1] http://marc.info/?l=kvm-ppc&m=144726114408289
[2] https://lists.nongnu.org/archive/html/qemu-devel/2014-11/msg00813.html
[3]
https://lists.nongnu.org/archive/html/qemu-devel/2015-08/msg00898.html