target-ppc: 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         |    7 +++++++
 include/hw/ppc/spapr.h |    5 +++++
 2 files changed, 12 insertions(+)

hw/ppc/spapr.c +0 -0

index 30de25d..38e26af 100644

hw/ppc/spapr.c +1 -0

+
     spapr->rtas_blob = g_malloc(spapr->rtas_size);
     if (load_image_size(filename, spapr->rtas_blob, spapr->rtas_size) < 0) {
         hw_error("qemu: could not load LPAR rtas '%s'\n", filename);

include/hw/ppc/spapr.h +0 -0

index 749daf4..d08fcc2 100644

Quoted 16 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         |    7 +++++++
 include/hw/ppc/spapr.h |    5 +++++
 2 files changed, 12 insertions(+)

hw/ppc/spapr.c +0 -0

index 30de25d..38e26af 100644
@@ -1431,6 +1431,13 @@ static void ppc_spapr_init(MachineState *machine)
 

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

Quoted 8 lines

hw/ppc/spapr.c +6 -0

     filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, "spapr-rtas.bin");
     spapr->rtas_size = get_image_size(filename);
+
+    /*
+     * Resize blob to accommodate error log. The layout of the rtas
+     * blob is defined in include/hw/ppc/spapr.h
+     */
+    spapr->rtas_size = TARGET_PAGE_ALIGN(spapr->rtas_size);

How big is the error log? You could just extend the RTAS blob to include space for it if it's not too big.

Quoted 13 lines

hw/ppc/spapr.c +1 -0

+
     spapr->rtas_blob = g_malloc(spapr->rtas_size);
     if (load_image_size(filename, spapr->rtas_blob, spapr->rtas_size) < 0) {
         hw_error("qemu: could not load LPAR rtas '%s'\n", filename);

include/hw/ppc/spapr.h +0 -0

index 749daf4..d08fcc2 100644
@@ -480,4 +480,9 @@ int spapr_dma_dt(void *fdt, int node_off, const char *propname,
 int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname,
                       sPAPRTCETable *tcet);
 

include/hw/ppc/spapr.h int spapr_dma_dt(void *fdt, int node_off, const char *propname, · line 486

Quoted 4 lines

include/hw/ppc/spapr.h +4 -0

+/* RTAS Blob layout in memory */
+#define RTAS_ENTRY_OFFSET        0
+#define RTAS_TRAMPOLINE_OFFSET   0x200
+#define RTAS_ERRLOG_OFFSET       0x800

I thought we agreed that these offsets should've been defined by the blob itself?

Alex

Quoted 19 lines · Alexander Graf

Quoted 16 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         |    7 +++++++
 include/hw/ppc/spapr.h |    5 +++++
 2 files changed, 12 insertions(+)

hw/ppc/spapr.c +0 -0

index 30de25d..38e26af 100644
@@ -1431,6 +1431,13 @@ static void ppc_spapr_init(MachineState *machine)
 

Quoted 11 lines

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

Quoted 8 lines

hw/ppc/spapr.c +6 -0

     filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, "spapr-rtas.bin");
     spapr->rtas_size = get_image_size(filename);
+
+    /*
+     * Resize blob to accommodate error log. The layout of the rtas
+     * blob is defined in include/hw/ppc/spapr.h
+     */
+    spapr->rtas_size = TARGET_PAGE_ALIGN(spapr->rtas_size);

How big is the error log? You could just extend the RTAS blob to include space for it if it's not too big.

Error log is around 10 bytes and requires additional 24 bytes to store saved sro/srr1.

Hmm.. yes it can be included in RTAS blob itself.

Quoted 13 lines

Quoted 13 lines

hw/ppc/spapr.c +1 -0

+
     spapr->rtas_blob = g_malloc(spapr->rtas_size);
     if (load_image_size(filename, spapr->rtas_blob, spapr->rtas_size) < 0) {
         hw_error("qemu: could not load LPAR rtas '%s'\n", filename);

include/hw/ppc/spapr.h +0 -0

index 749daf4..d08fcc2 100644
@@ -480,4 +480,9 @@ int spapr_dma_dt(void *fdt, int node_off, const char *propname,
 int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname,
                       sPAPRTCETable *tcet);
 

Quoted 8 lines

include/hw/ppc/spapr.h int spapr_dma_dt(void *fdt, int node_off, const char *propname, · line 486

Quoted 4 lines

include/hw/ppc/spapr.h +4 -0

+/* RTAS Blob layout in memory */
+#define RTAS_ENTRY_OFFSET        0
+#define RTAS_TRAMPOLINE_OFFSET   0x200
+#define RTAS_ERRLOG_OFFSET       0x800

I thought we agreed that these offsets should've been defined by the blob itself?

I think I got it wrong.

I will include these indexes at the entry of RTAS blob. With that we will have something like this:

RTAS_ENTRY_OFFSET  =      *(spapr->rtas_addr)
RTAS_TRAMPOLINE_OFFSET =  *(spapr->rtas_addr+8)
RTAS_ERRLOG_OFFSET =      *(spapr->rtas_addr+16)

I will fix this.

Quoted 71 lines · Aravinda Prasad

Quoted 19 lines · Alexander Graf

Quoted 16 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         |    7 +++++++
 include/hw/ppc/spapr.h |    5 +++++
 2 files changed, 12 insertions(+)

hw/ppc/spapr.c +0 -0

index 30de25d..38e26af 100644
@@ -1431,6 +1431,13 @@ static void ppc_spapr_init(MachineState *machine)
 

Quoted 11 lines

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

Quoted 8 lines

hw/ppc/spapr.c +6 -0

     filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, "spapr-rtas.bin");
     spapr->rtas_size = get_image_size(filename);
+
+    /*
+     * Resize blob to accommodate error log. The layout of the rtas
+     * blob is defined in include/hw/ppc/spapr.h
+     */
+    spapr->rtas_size = TARGET_PAGE_ALIGN(spapr->rtas_size);

How big is the error log? You could just extend the RTAS blob to include space for it if it's not too big.

Error log is around 10 bytes and requires additional 24 bytes to store saved sro/srr1.

Hmm.. yes it can be included in RTAS blob itself.

Quoted 13 lines

Quoted 13 lines

hw/ppc/spapr.c +1 -0

+
     spapr->rtas_blob = g_malloc(spapr->rtas_size);
     if (load_image_size(filename, spapr->rtas_blob, spapr->rtas_size) < 0) {
         hw_error("qemu: could not load LPAR rtas '%s'\n", filename);

include/hw/ppc/spapr.h +0 -0

index 749daf4..d08fcc2 100644
@@ -480,4 +480,9 @@ int spapr_dma_dt(void *fdt, int node_off, const char *propname,
 int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname,
                       sPAPRTCETable *tcet);
 

Quoted 8 lines

include/hw/ppc/spapr.h int spapr_dma_dt(void *fdt, int node_off, const char *propname, · line 486

Quoted 4 lines

include/hw/ppc/spapr.h +4 -0

+/* RTAS Blob layout in memory */
+#define RTAS_ENTRY_OFFSET        0
+#define RTAS_TRAMPOLINE_OFFSET   0x200
+#define RTAS_ERRLOG_OFFSET       0x800

I thought we agreed that these offsets should've been defined by the blob itself?

I think I got it wrong.

I will include these indexes at the entry of RTAS blob. With that we will have something like this:

RTAS_ENTRY_OFFSET  =      *(spapr->rtas_addr)
RTAS_TRAMPOLINE_OFFSET =  *(spapr->rtas_addr+8)
RTAS_ERRLOG_OFFSET =      *(spapr->rtas_addr+16)

Quoted 1 line

I will fix this.

Cool :). Just store the offsets inside of a helper struct that you for example store in the spapr struct, then we don't need to read volatile guest memory for the offsets.

Quoted 74 lines · Alexander Graf

Quoted 71 lines · Aravinda Prasad

Quoted 19 lines · Alexander Graf

Quoted 16 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         |    7 +++++++
 include/hw/ppc/spapr.h |    5 +++++
 2 files changed, 12 insertions(+)

hw/ppc/spapr.c +0 -0

index 30de25d..38e26af 100644
@@ -1431,6 +1431,13 @@ static void ppc_spapr_init(MachineState *machine)
 

Quoted 11 lines

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

Quoted 8 lines

hw/ppc/spapr.c +6 -0

     filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, "spapr-rtas.bin");
     spapr->rtas_size = get_image_size(filename);
+
+    /*
+     * Resize blob to accommodate error log. The layout of the rtas
+     * blob is defined in include/hw/ppc/spapr.h
+     */
+    spapr->rtas_size = TARGET_PAGE_ALIGN(spapr->rtas_size);

How big is the error log? You could just extend the RTAS blob to include space for it if it's not too big.

Error log is around 10 bytes and requires additional 24 bytes to store saved sro/srr1.

Hmm.. yes it can be included in RTAS blob itself.

Quoted 13 lines

Quoted 13 lines

hw/ppc/spapr.c +1 -0

+
     spapr->rtas_blob = g_malloc(spapr->rtas_size);
     if (load_image_size(filename, spapr->rtas_blob, spapr->rtas_size) < 0) {
         hw_error("qemu: could not load LPAR rtas '%s'\n", filename);

include/hw/ppc/spapr.h +0 -0

index 749daf4..d08fcc2 100644
@@ -480,4 +480,9 @@ int spapr_dma_dt(void *fdt, int node_off, const char *propname,
 int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname,
                       sPAPRTCETable *tcet);
 

Quoted 8 lines

include/hw/ppc/spapr.h int spapr_dma_dt(void *fdt, int node_off, const char *propname, · line 486

Quoted 4 lines

include/hw/ppc/spapr.h +4 -0

+/* RTAS Blob layout in memory */
+#define RTAS_ENTRY_OFFSET        0
+#define RTAS_TRAMPOLINE_OFFSET   0x200
+#define RTAS_ERRLOG_OFFSET       0x800

I thought we agreed that these offsets should've been defined by the blob itself?

I think I got it wrong.

I will include these indexes at the entry of RTAS blob. With that we will have something like this:

RTAS_ENTRY_OFFSET  =      *(spapr->rtas_addr)
RTAS_TRAMPOLINE_OFFSET =  *(spapr->rtas_addr+8)
RTAS_ERRLOG_OFFSET =      *(spapr->rtas_addr+16)

Quoted 5 lines

Quoted 1 line

I will fix this.

Cool :). Just store the offsets inside of a helper struct that you for example store in the spapr struct, then we don't need to read volatile guest memory for the offsets.

I just reread what I wrote and figured it's not exactly verbose. What I meant was that you read them on load into a struct. Then when working with the offsets, you only use the cached ones from the struct.

That way when the guest for whatever reason modifies the RTAS blob in memory, we would still use the old offsets and ensure that we don't end up overwriting memory that we never intended to overwrite ;).

Alex

This patch adds FWNMI support in qemu for powerKVM guests by handling the ibm,nmi-register rtas call. Whenever OS issues ibm,nmi-register RTAS call, the machine check notification address is saved and the machine check interrupt vector 0x200 is patched to issue a private hcall.

This patch also handles the cases when multi-processors experience machine check at or about the same time. As per PAPR, subsequent processors serialize waiting for the first processor to issue the ibm,nmi-interlock call. The second processor retries if the first processor which received a machine check is still reading the error log and is yet to issue ibm,nmi-interlock call.

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

---
 hw/ppc/spapr_hcall.c            |   16 +++++++
 hw/ppc/spapr_rtas.c             |   93 +++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h          |   17 +++++++
 pc-bios/spapr-rtas/spapr-rtas.S |   38 ++++++++++++++++
 4 files changed, 163 insertions(+), 1 deletion(-)

hw/ppc/spapr_hcall.c +3 -0

index 8f16160..eceb5e5 100644
@@ -97,6 +97,9 @@ struct rtas_mc_log {
     struct rtas_error_log err_log;
 };
 
+/* Whether machine check handling is in progress by any CPU */
+bool mc_in_progress;
+
 static void do_spr_sync(void *arg)
 {
     struct SPRSyncState *s = arg;

hw/ppc/spapr_hcall.c +6 -0

+        return 0;
+    }
+
+    mc_in_progress = 1;
+
+    /*
      * We save the original r3 register in SPRG2 in 0x200 vector,
      * which is patched during call to ibm.nmi-register. Original
      * r3 is required to be included in error log

hw/ppc/spapr_rtas.c +0 -0

index 2ec2a8e..71c7662 100644

hw/ppc/spapr_rtas.c +3 -0

+
 static void rtas_display_character(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                    uint32_t token, uint32_t nargs,
                                    target_ulong args,

@@ -290,6 +293,90 @@ static void rtas_ibm_os_term(PowerPCCPU *cpu,
     rtas_st(rets, 0, ret);
 }
 
+static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
+                                  sPAPREnvironment *spapr,

pc-bios/spapr-rtas/spapr-rtas.S +38 -0

@@ -35,3 +35,41 @@ _start:
 	ori	3,3,KVMPPC_H_RTAS@l
 	sc	1
 	blr
+	. = 0x200
+	/*
+	 * Trampoline saves r3 in sprg2 and issues private hcall
+	 * to request qemu to build error log. QEMU builds the
+	 * error log, copies to rtas-blob and returns the address.
+	 * The initial 16 bytes in return adress consist of saved
+	 * srr0 and srr1 which we restore and pass on the actual error
+	 * log address to OS handled mcachine check notification
+	 * routine
+	 *
+	 * All the below instructions are copied to interrupt vector
+	 * 0x200 at the time of handling ibm,nmi-register rtas call.
+	 */
+	mtsprg  2,3
+	li      3,0
+	/*
+	 * ori r3,r3,KVMPPC_H_REPORT_MC_ERR. The KVMPPC_H_REPORT_MC_ERR
+	 * value is patched below
+	 */
+1:	ori     3,3,0
+	sc      1               /* Issue H_CALL */
+	cmpdi   cr0,3,0
+	beq     cr0,1b          /* retry KVMPPC_H_REPORT_MC_ERR */
+	mtsprg  2,4
+	ld      4,0(3)
+	mtsrr0  4               /* Restore srr0 */
+	ld      4,8(3)
+	mtsrr1  4               /* Restore srr1 */
+	ld      4,16(3)
+	mtcrf   0,4             /* Restore cr */
+	addi    3,3,24
+	mfsprg  4,2
+	/*
+	 * Branch to address registered by OS. The branch address is
+	 * patched in the ibm,nmi-register rtas call.
+	 */
+	ba      0x0
+	b       .

Quoted 40 lines · Aravinda Prasad

This patch adds FWNMI support in qemu for powerKVM guests by handling the ibm,nmi-register rtas call. Whenever OS issues ibm,nmi-register RTAS call, the machine check notification address is saved and the machine check interrupt vector 0x200 is patched to issue a private hcall.

This patch also handles the cases when multi-processors experience machine check at or about the same time. As per PAPR, subsequent processors serialize waiting for the first processor to issue the ibm,nmi-interlock call. The second processor retries if the first processor which received a machine check is still reading the error log and is yet to issue ibm,nmi-interlock call.

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

---
 hw/ppc/spapr_hcall.c            |   16 +++++++
 hw/ppc/spapr_rtas.c             |   93 +++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h          |   17 +++++++
 pc-bios/spapr-rtas/spapr-rtas.S |   38 ++++++++++++++++
 4 files changed, 163 insertions(+), 1 deletion(-)

hw/ppc/spapr_hcall.c +3 -0

index 8f16160..eceb5e5 100644
@@ -97,6 +97,9 @@ struct rtas_mc_log {
     struct rtas_error_log err_log;
 };
 
+/* Whether machine check handling is in progress by any CPU */
+bool mc_in_progress;
+
 static void do_spr_sync(void *arg)
 {
     struct SPRSyncState *s = arg;

@@ -678,6 +681,19 @@ static target_ulong h_report_mc_err(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     cpu_synchronize_state(CPU(ppc_env_get_cpu(env)));
 

hw/ppc/spapr_hcall.c static target_ulong h_report_mc_err(PowerPCCPU *cpu, sPAPREnvironment *spapr, · line 690

Quoted 8 lines

hw/ppc/spapr_hcall.c +7 -0

     /*
+     * Only one VCPU can process machine check NMI at a time. Hence
+     * set the lock mc_in_progress. Once the VCPU finishes processing
+     * NMI, it executes ibm,nmi-interlock and mc_in_progress is unset
+     * in ibm,nmi-interlock handler. Meanwhile if other VCPUs encounter
+     * NMI we return 0 asking the VCPU to retry h_report_mc_err
+     */
+    if (mc_in_progress == 1) {

Please don't depend on bools being numbers. Use true / false. For if()s, just don't use == at all - it makes it more readable.

Quoted 16 lines

hw/ppc/spapr_hcall.c +6 -0

+        return 0;
+    }
+
+    mc_in_progress = 1;
+
+    /*
      * We save the original r3 register in SPRG2 in 0x200 vector,
      * which is patched during call to ibm.nmi-register. Original
      * r3 is required to be included in error log

hw/ppc/spapr_rtas.c +0 -0

index 2ec2a8e..71c7662 100644
@@ -36,6 +36,9 @@
 

hw/ppc/spapr_rtas.c line 40

Quoted 4 lines

hw/ppc/spapr_rtas.c +2 -0

 #include <libfdt.h>
 
+#define BRANCH_INST_MASK  0xFC000000
+extern bool mc_in_progress;

Please put this into the spapr struct.

Quoted 11 lines

hw/ppc/spapr_rtas.c +3 -0

+
 static void rtas_display_character(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                    uint32_t token, uint32_t nargs,
                                    target_ulong args,

@@ -290,6 +293,90 @@ static void rtas_ibm_os_term(PowerPCCPU *cpu,
     rtas_st(rets, 0, ret);
 }
 
+static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
+                                  sPAPREnvironment *spapr,

hw/ppc/spapr_rtas.c static void rtas_ibm_os_term(PowerPCCPU *cpu, · line 307

Quoted 10 lines

hw/ppc/spapr_rtas.c +10 -0

+                                  uint32_t token, uint32_t nargs,
+                                  target_ulong args,
+                                  uint32_t nret, target_ulong rets)
+{
+    int i;
+    uint32_t ori_inst = 0x60630000;
+    uint32_t branch_inst = 0x48000002;
+    target_ulong guest_machine_check_addr;
+    uint32_t trampoline[TRAMPOLINE_INSTS];
+    int total_inst = sizeof(trampoline) / sizeof(uint32_t);

ARRAY_SIZE(trampoline), though I don't quite understand why you need a variable that contains the same value as a constant (TRAMPOLINE_INSTS).

But since you're moving all of those bits into variable fields on the rtas blob itself as we discussed in the last version, I guess this code will go away anyways ;).

hw/ppc/spapr_rtas.c static void rtas_ibm_os_term(PowerPCCPU *cpu, · line 311

Quoted 5 lines

hw/ppc/spapr_rtas.c +4 -0

+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+
+    /* Store the system reset and machine check address */
+    guest_machine_check_addr = rtas_ld(args, 1);

Load or Store? I don't find the comment particularly useful either ;).

hw/ppc/spapr_rtas.c static void rtas_ibm_os_term(PowerPCCPU *cpu, · line 321

Quoted 11 lines

hw/ppc/spapr_rtas.c +10 -0

+
+    /*
+     * Read the trampoline instructions from RTAS Blob and patch
+     * the KVMPPC_H_REPORT_MC_ERR hcall number and the guest
+     * machine check address before copying to 0x200 vector
+     */
+    cpu_physical_memory_read(spapr->rtas_addr + RTAS_TRAMPOLINE_OFFSET,
+                             trampoline, sizeof(trampoline));
+
+    /* Safety Check */

Same for this comment.

hw/ppc/spapr_rtas.c static void rtas_ibm_os_term(PowerPCCPU *cpu, · line 327

Quoted 7 lines

hw/ppc/spapr_rtas.c +6 -0

+    QEMU_BUILD_BUG_ON(sizeof(trampoline) > MC_INTERRUPT_VECTOR_SIZE);
+
+    /* Update the KVMPPC_H_REPORT_MC_ERR value in trampoline */
+    ori_inst |= KVMPPC_H_REPORT_MC_ERR;
+    memcpy(&trampoline[TRAMPOLINE_ORI_INST_INDEX], &ori_inst,
+            sizeof(ori_inst));

Why memcpy a u32 into a u32 array?

hw/ppc/spapr_rtas.c static void rtas_ibm_os_term(PowerPCCPU *cpu, · line 335

Quoted 9 lines

hw/ppc/spapr_rtas.c +8 -0

+
+    /*
+     * Sanity check guest_machine_check_addr to prevent clobbering
+     * operator value in branch instruction
+     */
+    if (guest_machine_check_addr & BRANCH_INST_MASK) {
+        fprintf(stderr, "Unable to register ibm,nmi_register: "
+                "Invalid machine check handler address\n");

In general, printf's in guest triggerable code aren't a great idea, since the guest could flood our host logs with this. I can't say we're doing a great job at it already though, so it probably doesn't matter much.

pc-bios/spapr-rtas/spapr-rtas.S

Quoted 99 lines

hw/ppc/spapr_rtas.c +50 -0

+        rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);
+        return;
+    }
+
+    /*
+     * Update the branch instruction in trampoline
+     * with the absolute machine check address requested by OS.
+     */
+    branch_inst |= guest_machine_check_addr;
+    memcpy(&trampoline[TRAMPOLINE_BR_INST_INDEX], &branch_inst,
+            sizeof(branch_inst));
+
+    /* Handle all Host/Guest LE/BE combinations */
+    if ((*pcc->interrupts_big_endian)(cpu)) {
+        for (i = 0; i < total_inst; i++) {
+            trampoline[i] = cpu_to_be32(trampoline[i]);
+        }
+    } else {
+        for (i = 0; i < total_inst; i++) {
+            trampoline[i] = cpu_to_le32(trampoline[i]);
+        }
+    }
+
+    /* Patch 0x200 NMI interrupt vector memory area of guest */
+    cpu_physical_memory_write(MC_INTERRUPT_VECTOR, trampoline,
+                              sizeof(trampoline));
+
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+}
+
+static void rtas_ibm_nmi_interlock(PowerPCCPU *cpu,
+                                   sPAPREnvironment *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 unset mc_in_progress.
+     */
+    mc_in_progress = 0;
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+}
+
 static struct rtas_call {
     const char *name;
     spapr_rtas_fn fn;

@@ -419,6 +506,12 @@ static void core_rtas_register_types(void)
                         rtas_ibm_set_system_parameter);
     spapr_rtas_register(RTAS_IBM_OS_TERM, "ibm,os-term",
                         rtas_ibm_os_term);
+    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 +16 -1

index a2d67e9..98d0a6c 100644
@@ -384,8 +384,10 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi);
 #define RTAS_GET_SENSOR_STATE                   (RTAS_TOKEN_BASE + 0x1D)
 #define RTAS_IBM_CONFIGURE_CONNECTOR            (RTAS_TOKEN_BASE + 0x1E)
 #define RTAS_IBM_OS_TERM                        (RTAS_TOKEN_BASE + 0x1F)
+#define RTAS_IBM_NMI_REGISTER                   (RTAS_TOKEN_BASE + 0x20)
+#define RTAS_IBM_NMI_INTERLOCK                  (RTAS_TOKEN_BASE + 0x21)
 
-#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x20)
+#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x22)
 
 /* RTAS ibm,get-system-parameter token values */
 #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS      20

@@ -488,4 +490,17 @@ int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname,
 #define RTAS_TRAMPOLINE_OFFSET   0x200
 #define RTAS_ERRLOG_OFFSET       0x800
 
+/* Machine Check Trampoline related macros
+ *
+ * These macros should co-relate to the code we
+ * have in pc-bios/spapr-rtas/spapr-rtas.S
+ */
+#define TRAMPOLINE_INSTS           17
+#define TRAMPOLINE_ORI_INST_INDEX  2
+#define TRAMPOLINE_BR_INST_INDEX   15
+
+/* Machine Check Interrupt related macros */
+#define MC_INTERRUPT_VECTOR           0x200
+#define MC_INTERRUPT_VECTOR_SIZE      0x100
+
 #endif /* !defined (__HW_SPAPR_H__) */

pc-bios/spapr-rtas/spapr-rtas.S +0 -0

index 903bec2..c315332 100644

Please add #defines at the top of the file for the register names:

  #define r0 0
  #define r1 1
  ...

That way the code below will get much more readable :)

Also, you want a jump table here as we discussed in the last review round. That table would tell you

a) Entry address for RTAS
b) Offset of the NMI code
c) To-be-patched offsets of the instructions inside the NMI code

Then we have all offsets automatically generated inside a single file and don't have to maintain fragile relationships between random headers with offset defines and the .S file.

Alex

Quoted 44 lines

pc-bios/spapr-rtas/spapr-rtas.S +38 -0

@@ -35,3 +35,41 @@ _start:
 	ori	3,3,KVMPPC_H_RTAS@l
 	sc	1
 	blr
+	. = 0x200
+	/*
+	 * Trampoline saves r3 in sprg2 and issues private hcall
+	 * to request qemu to build error log. QEMU builds the
+	 * error log, copies to rtas-blob and returns the address.
+	 * The initial 16 bytes in return adress consist of saved
+	 * srr0 and srr1 which we restore and pass on the actual error
+	 * log address to OS handled mcachine check notification
+	 * routine
+	 *
+	 * All the below instructions are copied to interrupt vector
+	 * 0x200 at the time of handling ibm,nmi-register rtas call.
+	 */
+	mtsprg  2,3
+	li      3,0
+	/*
+	 * ori r3,r3,KVMPPC_H_REPORT_MC_ERR. The KVMPPC_H_REPORT_MC_ERR
+	 * value is patched below
+	 */
+1:	ori     3,3,0
+	sc      1               /* Issue H_CALL */
+	cmpdi   cr0,3,0
+	beq     cr0,1b          /* retry KVMPPC_H_REPORT_MC_ERR */
+	mtsprg  2,4
+	ld      4,0(3)
+	mtsrr0  4               /* Restore srr0 */
+	ld      4,8(3)
+	mtsrr1  4               /* Restore srr1 */
+	ld      4,16(3)
+	mtcrf   0,4             /* Restore cr */
+	addi    3,3,24
+	mfsprg  4,2
+	/*
+	 * Branch to address registered by OS. The branch address is
+	 * patched in the ibm,nmi-register rtas call.
+	 */
+	ba      0x0
+	b       .

Quoted 43 lines · Alexander Graf

Quoted 40 lines · Aravinda Prasad

This patch adds FWNMI support in qemu for powerKVM guests by handling the ibm,nmi-register rtas call. Whenever OS issues ibm,nmi-register RTAS call, the machine check notification address is saved and the machine check interrupt vector 0x200 is patched to issue a private hcall.

This patch also handles the cases when multi-processors experience machine check at or about the same time. As per PAPR, subsequent processors serialize waiting for the first processor to issue the ibm,nmi-interlock call. The second processor retries if the first processor which received a machine check is still reading the error log and is yet to issue ibm,nmi-interlock call.

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

---
 hw/ppc/spapr_hcall.c            |   16 +++++++
 hw/ppc/spapr_rtas.c             |   93 +++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h          |   17 +++++++
 pc-bios/spapr-rtas/spapr-rtas.S |   38 ++++++++++++++++
 4 files changed, 163 insertions(+), 1 deletion(-)

hw/ppc/spapr_hcall.c +3 -0

index 8f16160..eceb5e5 100644
@@ -97,6 +97,9 @@ struct rtas_mc_log {
     struct rtas_error_log err_log;
 };
 
+/* Whether machine check handling is in progress by any CPU */
+bool mc_in_progress;
+
 static void do_spr_sync(void *arg)
 {
     struct SPRSyncState *s = arg;

@@ -678,6 +681,19 @@ static target_ulong h_report_mc_err(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     cpu_synchronize_state(CPU(ppc_env_get_cpu(env)));
 

Quoted 11 lines

hw/ppc/spapr_hcall.c static target_ulong h_report_mc_err(PowerPCCPU *cpu, sPAPREnvironment *spapr, · line 690

Quoted 8 lines

hw/ppc/spapr_hcall.c +7 -0

     /*
+     * Only one VCPU can process machine check NMI at a time. Hence
+     * set the lock mc_in_progress. Once the VCPU finishes processing
+     * NMI, it executes ibm,nmi-interlock and mc_in_progress is unset
+     * in ibm,nmi-interlock handler. Meanwhile if other VCPUs encounter
+     * NMI we return 0 asking the VCPU to retry h_report_mc_err
+     */
+    if (mc_in_progress == 1) {

Please don't depend on bools being numbers. Use true / false. For if()s, just don't use == at all - it makes it more readable.

ok

Quoted 16 lines

Quoted 16 lines

hw/ppc/spapr_hcall.c +6 -0

+        return 0;
+    }
+
+    mc_in_progress = 1;
+
+    /*
      * We save the original r3 register in SPRG2 in 0x200 vector,
      * which is patched during call to ibm.nmi-register. Original
      * r3 is required to be included in error log

hw/ppc/spapr_rtas.c +0 -0

index 2ec2a8e..71c7662 100644
@@ -36,6 +36,9 @@
 

Quoted 6 lines

hw/ppc/spapr_rtas.c line 40

Quoted 4 lines

hw/ppc/spapr_rtas.c +2 -0

 #include <libfdt.h>
 
+#define BRANCH_INST_MASK  0xFC000000
+extern bool mc_in_progress;

Please put this into the spapr struct.

ok

Quoted 25 lines

Quoted 11 lines

hw/ppc/spapr_rtas.c +3 -0

+
 static void rtas_display_character(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                    uint32_t token, uint32_t nargs,
                                    target_ulong args,

@@ -290,6 +293,90 @@ static void rtas_ibm_os_term(PowerPCCPU *cpu,
     rtas_st(rets, 0, ret);
 }
 
+static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
+                                  sPAPREnvironment *spapr,

hw/ppc/spapr_rtas.c static void rtas_ibm_os_term(PowerPCCPU *cpu, · line 307

Quoted 10 lines

hw/ppc/spapr_rtas.c +10 -0

+                                  uint32_t token, uint32_t nargs,
+                                  target_ulong args,
+                                  uint32_t nret, target_ulong rets)
+{
+    int i;
+    uint32_t ori_inst = 0x60630000;
+    uint32_t branch_inst = 0x48000002;
+    target_ulong guest_machine_check_addr;
+    uint32_t trampoline[TRAMPOLINE_INSTS];
+    int total_inst = sizeof(trampoline) / sizeof(uint32_t);

ARRAY_SIZE(trampoline), though I don't quite understand why you need a variable that contains the same value as a constant (TRAMPOLINE_INSTS).

Quoted 3 lines

But since you're moving all of those bits into variable fields on the rtas blob itself as we discussed in the last version, I guess this code will go away anyways ;).

I think we still need this. We need to patch the KVMPPC_H_REPORT_MC_ERR number and branch address in the trampoline and also, depending on whether the guest running in LE/BE we may need to flip the bits in the trampoline before copying it to 0x200 machine check vector.

As rtas-blob is part of the guest memory I do not want to patch these in rtas-blob, hence I copy the trampoline from the rtas-blob to an array, modify accordingly and then move it to 0x200 machine check vector.

Quoted 8 lines

hw/ppc/spapr_rtas.c static void rtas_ibm_os_term(PowerPCCPU *cpu, · line 311

Quoted 6 lines

hw/ppc/spapr_rtas.c +4 -0

+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+
+    /* Store the system reset and machine check address */
+    guest_machine_check_addr = rtas_ld(args, 1);

Load or Store? I don't find the comment particularly useful either ;).

will reword it or may delete it.

Quoted 13 lines

hw/ppc/spapr_rtas.c static void rtas_ibm_os_term(PowerPCCPU *cpu, · line 321

Quoted 11 lines

hw/ppc/spapr_rtas.c +10 -0

+
+    /*
+     * Read the trampoline instructions from RTAS Blob and patch
+     * the KVMPPC_H_REPORT_MC_ERR hcall number and the guest
+     * machine check address before copying to 0x200 vector
+     */
+    cpu_physical_memory_read(spapr->rtas_addr + RTAS_TRAMPOLINE_OFFSET,
+                             trampoline, sizeof(trampoline));
+
+    /* Safety Check */

Same for this comment.

we have only 0x100 bytes that can be copied at 0x200. If the trampoline size exceeds then the next interrupt vector code is overwritten. Hence a safety check during compile time to make sure trampoline is within 0x100 bytes.

Quoted 114 lines · Aravinda Prasad

Quoted 43 lines · Alexander Graf

Quoted 40 lines · Aravinda Prasad

This patch adds FWNMI support in qemu for powerKVM guests by handling the ibm,nmi-register rtas call. Whenever OS issues ibm,nmi-register RTAS call, the machine check notification address is saved and the machine check interrupt vector 0x200 is patched to issue a private hcall.

This patch also handles the cases when multi-processors experience machine check at or about the same time. As per PAPR, subsequent processors serialize waiting for the first processor to issue the ibm,nmi-interlock call. The second processor retries if the first processor which received a machine check is still reading the error log and is yet to issue ibm,nmi-interlock call.

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

---
 hw/ppc/spapr_hcall.c            |   16 +++++++
 hw/ppc/spapr_rtas.c             |   93 +++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h          |   17 +++++++
 pc-bios/spapr-rtas/spapr-rtas.S |   38 ++++++++++++++++
 4 files changed, 163 insertions(+), 1 deletion(-)

hw/ppc/spapr_hcall.c +3 -0

index 8f16160..eceb5e5 100644
@@ -97,6 +97,9 @@ struct rtas_mc_log {
     struct rtas_error_log err_log;
 };
 
+/* Whether machine check handling is in progress by any CPU */
+bool mc_in_progress;
+
 static void do_spr_sync(void *arg)
 {
     struct SPRSyncState *s = arg;

@@ -678,6 +681,19 @@ static target_ulong h_report_mc_err(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     cpu_synchronize_state(CPU(ppc_env_get_cpu(env)));
 

Quoted 11 lines

hw/ppc/spapr_hcall.c static target_ulong h_report_mc_err(PowerPCCPU *cpu, sPAPREnvironment *spapr, · line 690

Quoted 8 lines

hw/ppc/spapr_hcall.c +7 -0

     /*
+     * Only one VCPU can process machine check NMI at a time. Hence
+     * set the lock mc_in_progress. Once the VCPU finishes processing
+     * NMI, it executes ibm,nmi-interlock and mc_in_progress is unset
+     * in ibm,nmi-interlock handler. Meanwhile if other VCPUs encounter
+     * NMI we return 0 asking the VCPU to retry h_report_mc_err
+     */
+    if (mc_in_progress == 1) {

Please don't depend on bools being numbers. Use true / false. For if()s, just don't use == at all - it makes it more readable.

ok

Quoted 16 lines

Quoted 16 lines

hw/ppc/spapr_hcall.c +6 -0

+        return 0;
+    }
+
+    mc_in_progress = 1;
+
+    /*
      * We save the original r3 register in SPRG2 in 0x200 vector,
      * which is patched during call to ibm.nmi-register. Original
      * r3 is required to be included in error log

hw/ppc/spapr_rtas.c +0 -0

index 2ec2a8e..71c7662 100644
@@ -36,6 +36,9 @@
 

Quoted 6 lines

hw/ppc/spapr_rtas.c line 40

Quoted 4 lines

hw/ppc/spapr_rtas.c +2 -0

 #include <libfdt.h>
 
+#define BRANCH_INST_MASK  0xFC000000
+extern bool mc_in_progress;

Please put this into the spapr struct.

ok

Quoted 25 lines

Quoted 11 lines

hw/ppc/spapr_rtas.c +3 -0

+
 static void rtas_display_character(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                    uint32_t token, uint32_t nargs,
                                    target_ulong args,

@@ -290,6 +293,90 @@ static void rtas_ibm_os_term(PowerPCCPU *cpu,
     rtas_st(rets, 0, ret);
 }
 
+static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
+                                  sPAPREnvironment *spapr,

hw/ppc/spapr_rtas.c static void rtas_ibm_os_term(PowerPCCPU *cpu, · line 307

Quoted 10 lines

hw/ppc/spapr_rtas.c +10 -0

+                                  uint32_t token, uint32_t nargs,
+                                  target_ulong args,
+                                  uint32_t nret, target_ulong rets)
+{
+    int i;
+    uint32_t ori_inst = 0x60630000;
+    uint32_t branch_inst = 0x48000002;
+    target_ulong guest_machine_check_addr;
+    uint32_t trampoline[TRAMPOLINE_INSTS];
+    int total_inst = sizeof(trampoline) / sizeof(uint32_t);

ARRAY_SIZE(trampoline), though I don't quite understand why you need a variable that contains the same value as a constant (TRAMPOLINE_INSTS).

Quoted 3 lines

But since you're moving all of those bits into variable fields on the rtas blob itself as we discussed in the last version, I guess this code will go away anyways ;).

Quoted 8 lines

I think we still need this. We need to patch the KVMPPC_H_REPORT_MC_ERR number and branch address in the trampoline and also, depending on whether the guest running in LE/BE we may need to flip the bits in the trampoline before copying it to 0x200 machine check vector.

As rtas-blob is part of the guest memory I do not want to patch these in rtas-blob, hence I copy the trampoline from the rtas-blob to an array, modify accordingly and then move it to 0x200 machine check vector.

Yes, you will still need the array. But the array should be dynamically sized based on spapr->rtas_info->fwnmi_size which you extract from the blob on load.

That way you wouldn't need the "total_inst" variable anymore ;).

Quoted 26 lines

Quoted 9 lines

hw/ppc/spapr_rtas.c static void rtas_ibm_os_term(PowerPCCPU *cpu, · line 311

Quoted 7 lines

hw/ppc/spapr_rtas.c +4 -0

+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+
+    /* Store the system reset and machine check address */
+    guest_machine_check_addr = rtas_ld(args, 1);

Load or Store? I don't find the comment particularly useful either ;).

will reword it or may delete it.

Quoted 13 lines

hw/ppc/spapr_rtas.c static void rtas_ibm_os_term(PowerPCCPU *cpu, · line 321

Quoted 11 lines

hw/ppc/spapr_rtas.c +10 -0

+
+    /*
+     * Read the trampoline instructions from RTAS Blob and patch
+     * the KVMPPC_H_REPORT_MC_ERR hcall number and the guest
+     * machine check address before copying to 0x200 vector
+     */
+    cpu_physical_memory_read(spapr->rtas_addr + RTAS_TRAMPOLINE_OFFSET,
+                             trampoline, sizeof(trampoline));
+
+    /* Safety Check */

Same for this comment.

Quoted 4 lines

we have only 0x100 bytes that can be copied at 0x200. If the trampoline size exceeds then the next interrupt vector code is overwritten. Hence a safety check during compile time to make sure trampoline is within 0x100 bytes.

I think the check is fine, the comment is just redundant with QEMU_BUILD_BUG_ON. Either be more verbose in the comment or remove it

;). But something a la

  /* check for failure */
  BUG_ON(foo);

is useless redundancy, because everyone already knows that BUG_ON checks for failure.

The interesting bit is the why. Also, as a general rule of thumb, if you need a comment explaining the "what" of what you're doing, your function and/or variable names are probably not well chosen ;). But I don't think this is a problem here.

Thanks for the patches btw :)

Alex

Quoted 122 lines · Alexander Graf

Quoted 119 lines · Aravinda Prasad

Quoted 43 lines · Alexander Graf

Quoted 40 lines · Aravinda Prasad

This patch adds FWNMI support in qemu for powerKVM guests by handling the ibm,nmi-register rtas call. Whenever OS issues ibm,nmi-register RTAS call, the machine check notification address is saved and the machine check interrupt vector 0x200 is patched to issue a private hcall.

This patch also handles the cases when multi-processors experience machine check at or about the same time. As per PAPR, subsequent processors serialize waiting for the first processor to issue the ibm,nmi-interlock call. The second processor retries if the first processor which received a machine check is still reading the error log and is yet to issue ibm,nmi-interlock call.

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

---
 hw/ppc/spapr_hcall.c            |   16 +++++++
 hw/ppc/spapr_rtas.c             |   93 +++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h          |   17 +++++++
 pc-bios/spapr-rtas/spapr-rtas.S |   38 ++++++++++++++++
 4 files changed, 163 insertions(+), 1 deletion(-)

hw/ppc/spapr_hcall.c +3 -0

index 8f16160..eceb5e5 100644
@@ -97,6 +97,9 @@ struct rtas_mc_log {
     struct rtas_error_log err_log;
 };
 
+/* Whether machine check handling is in progress by any CPU */
+bool mc_in_progress;
+
 static void do_spr_sync(void *arg)
 {
     struct SPRSyncState *s = arg;

@@ -678,6 +681,19 @@ static target_ulong h_report_mc_err(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     cpu_synchronize_state(CPU(ppc_env_get_cpu(env)));
 

Quoted 11 lines

hw/ppc/spapr_hcall.c static target_ulong h_report_mc_err(PowerPCCPU *cpu, sPAPREnvironment *spapr, · line 690

Quoted 8 lines

hw/ppc/spapr_hcall.c +7 -0

     /*
+     * Only one VCPU can process machine check NMI at a time. Hence
+     * set the lock mc_in_progress. Once the VCPU finishes processing
+     * NMI, it executes ibm,nmi-interlock and mc_in_progress is unset
+     * in ibm,nmi-interlock handler. Meanwhile if other VCPUs encounter
+     * NMI we return 0 asking the VCPU to retry h_report_mc_err
+     */
+    if (mc_in_progress == 1) {

Please don't depend on bools being numbers. Use true / false. For if()s, just don't use == at all - it makes it more readable.

ok

Quoted 16 lines

Quoted 16 lines

hw/ppc/spapr_hcall.c +6 -0

+        return 0;
+    }
+
+    mc_in_progress = 1;
+
+    /*
      * We save the original r3 register in SPRG2 in 0x200 vector,
      * which is patched during call to ibm.nmi-register. Original
      * r3 is required to be included in error log

hw/ppc/spapr_rtas.c +0 -0

index 2ec2a8e..71c7662 100644
@@ -36,6 +36,9 @@
 

Quoted 6 lines

hw/ppc/spapr_rtas.c line 40

Quoted 4 lines

hw/ppc/spapr_rtas.c +2 -0

 #include <libfdt.h>
 
+#define BRANCH_INST_MASK  0xFC000000
+extern bool mc_in_progress;

Please put this into the spapr struct.

ok

Quoted 25 lines

Quoted 11 lines

hw/ppc/spapr_rtas.c +3 -0

+
 static void rtas_display_character(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                    uint32_t token, uint32_t nargs,
                                    target_ulong args,

@@ -290,6 +293,90 @@ static void rtas_ibm_os_term(PowerPCCPU *cpu,
     rtas_st(rets, 0, ret);
 }
 
+static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
+                                  sPAPREnvironment *spapr,

hw/ppc/spapr_rtas.c static void rtas_ibm_os_term(PowerPCCPU *cpu, · line 307

Quoted 10 lines

hw/ppc/spapr_rtas.c +10 -0

+                                  uint32_t token, uint32_t nargs,
+                                  target_ulong args,
+                                  uint32_t nret, target_ulong rets)
+{
+    int i;
+    uint32_t ori_inst = 0x60630000;
+    uint32_t branch_inst = 0x48000002;
+    target_ulong guest_machine_check_addr;
+    uint32_t trampoline[TRAMPOLINE_INSTS];
+    int total_inst = sizeof(trampoline) / sizeof(uint32_t);

ARRAY_SIZE(trampoline), though I don't quite understand why you need a variable that contains the same value as a constant (TRAMPOLINE_INSTS).

Quoted 3 lines

But since you're moving all of those bits into variable fields on the rtas blob itself as we discussed in the last version, I guess this code will go away anyways ;).

I think we still need this. We need to patch the KVMPPC_H_REPORT_MC_ERR number and branch address in the trampoline and also, depending on whether the guest running in LE/BE we may need to flip the bits in the trampoline before copying it to 0x200 machine check vector.

Quoted 6 lines

Quoted 3 lines

As rtas-blob is part of the guest memory I do not want to patch these in rtas-blob, hence I copy the trampoline from the rtas-blob to an array, modify accordingly and then move it to 0x200 machine check vector.

Yes, you will still need the array. But the array should be dynamically sized based on spapr->rtas_info->fwnmi_size which you extract from the

Quoted 3 lines

blob on load.

That way you wouldn't need the "total_inst" variable anymore ;).

Yes, I will fix it that way.

Quoted 31 lines

Quoted 30 lines

Quoted 9 lines

hw/ppc/spapr_rtas.c static void rtas_ibm_os_term(PowerPCCPU *cpu, · line 311

Quoted 7 lines

hw/ppc/spapr_rtas.c +4 -0

+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+
+    /* Store the system reset and machine check address */
+    guest_machine_check_addr = rtas_ld(args, 1);

Load or Store? I don't find the comment particularly useful either ;).

will reword it or may delete it.

Quoted 13 lines

hw/ppc/spapr_rtas.c static void rtas_ibm_os_term(PowerPCCPU *cpu, · line 321

Quoted 11 lines

hw/ppc/spapr_rtas.c +10 -0

+
+    /*
+     * Read the trampoline instructions from RTAS Blob and patch
+     * the KVMPPC_H_REPORT_MC_ERR hcall number and the guest
+     * machine check address before copying to 0x200 vector
+     */
+    cpu_physical_memory_read(spapr->rtas_addr + RTAS_TRAMPOLINE_OFFSET,
+                             trampoline, sizeof(trampoline));
+
+    /* Safety Check */

Same for this comment.

we have only 0x100 bytes that can be copied at 0x200. If the trampoline size exceeds then the next interrupt vector code is overwritten. Hence a safety check during compile time to make sure trampoline is within 0x100 bytes.

Quoted 2 lines

I think the check is fine, the comment is just redundant with QEMU_BUILD_BUG_ON. Either be more verbose in the comment or remove it

I will add above lines as comment.

Quoted 38 lines · Aravinda Prasad

This patch adds FWNMI support in qemu for powerKVM guests by handling the ibm,nmi-register rtas call. Whenever OS issues ibm,nmi-register RTAS call, the machine check notification address is saved and the machine check interrupt vector 0x200 is patched to issue a private hcall.

This patch also handles the cases when multi-processors experience machine check at or about the same time. As per PAPR, subsequent processors serialize waiting for the first processor to issue the ibm,nmi-interlock call. The second processor retries if the first processor which received a machine check is still reading the error log and is yet to issue ibm,nmi-interlock call.

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

---
 hw/ppc/spapr_hcall.c            |   16 +++++++
 hw/ppc/spapr_rtas.c             |   93 +++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h          |   17 +++++++
 pc-bios/spapr-rtas/spapr-rtas.S |   38 ++++++++++++++++
 4 files changed, 163 insertions(+), 1 deletion(-)

hw/ppc/spapr_hcall.c +3 -0

index 8f16160..eceb5e5 100644
@@ -97,6 +97,9 @@ struct rtas_mc_log {
     struct rtas_error_log err_log;
 };
 
+/* Whether machine check handling is in progress by any CPU */
+bool mc_in_progress;
+
 static void do_spr_sync(void *arg)
 {
     struct SPRSyncState *s = arg;

hw/ppc/spapr_hcall.c static target_ulong h_report_mc_err(PowerPCCPU *cpu, sPAPREnvironment *spapr, · line 690

Quoted 10 lines

hw/ppc/spapr_hcall.c +7 -0

@@ -678,6 +681,19 @@ static target_ulong h_report_mc_err(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     cpu_synchronize_state(CPU(ppc_env_get_cpu(env)));
 
     /*
+     * Only one VCPU can process machine check NMI at a time. Hence
+     * set the lock mc_in_progress. Once the VCPU finishes processing
+     * NMI, it executes ibm,nmi-interlock and mc_in_progress is unset
+     * in ibm,nmi-interlock handler. Meanwhile if other VCPUs encounter
+     * NMI we return 0 asking the VCPU to retry h_report_mc_err
+     */
+    if (mc_in_progress == 1) {

Please don't depend on bools being numbers. Use true / false. For if()s, just don't use == at all - it makes it more readable.

Quoted 14 lines

hw/ppc/spapr_hcall.c +6 -0

+        return 0;
+    }
+
+    mc_in_progress = 1;
+
+    /*
      * We save the original r3 register in SPRG2 in 0x200 vector,
      * which is patched during call to ibm.nmi-register. Original
      * r3 is required to be included in error log

hw/ppc/spapr_rtas.c +0 -0

index 2ec2a8e..71c7662 100644

hw/ppc/spapr_rtas.c line 40

Quoted 5 lines

hw/ppc/spapr_rtas.c +2 -0

@@ -36,6 +36,9 @@
 
 #include <libfdt.h>
 
+#define BRANCH_INST_MASK  0xFC000000
+extern bool mc_in_progress;

Please put this into the spapr struct.

Quoted 10 lines

hw/ppc/spapr_rtas.c +3 -0

+
 static void rtas_display_character(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                    uint32_t token, uint32_t nargs,
                                    target_ulong args,

@@ -290,6 +293,90 @@ static void rtas_ibm_os_term(PowerPCCPU *cpu,
     rtas_st(rets, 0, ret);
 }
 
+static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
+                                  sPAPREnvironment *spapr,

hw/ppc/spapr_rtas.c static void rtas_ibm_os_term(PowerPCCPU *cpu, · line 307

Quoted 10 lines

hw/ppc/spapr_rtas.c +10 -0

+                                  uint32_t token, uint32_t nargs,
+                                  target_ulong args,
+                                  uint32_t nret, target_ulong rets)
+{
+    int i;
+    uint32_t ori_inst = 0x60630000;
+    uint32_t branch_inst = 0x48000002;
+    target_ulong guest_machine_check_addr;
+    uint32_t trampoline[TRAMPOLINE_INSTS];
+    int total_inst = sizeof(trampoline) / sizeof(uint32_t);

ARRAY_SIZE(trampoline), though I don't quite understand why you need a variable that contains the same value as a constant (TRAMPOLINE_INSTS).

But since you're moving all of those bits into variable fields on the rtas blob itself as we discussed in the last version, I guess this code will go away anyways ;).

hw/ppc/spapr_rtas.c static void rtas_ibm_os_term(PowerPCCPU *cpu, · line 311

Quoted 4 lines

hw/ppc/spapr_rtas.c +4 -0

+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+
+    /* Store the system reset and machine check address */
+    guest_machine_check_addr = rtas_ld(args, 1);

Load or Store? I don't find the comment particularly useful either ;).

hw/ppc/spapr_rtas.c static void rtas_ibm_os_term(PowerPCCPU *cpu, · line 321

Quoted 10 lines

hw/ppc/spapr_rtas.c +10 -0

+
+    /*
+     * Read the trampoline instructions from RTAS Blob and patch
+     * the KVMPPC_H_REPORT_MC_ERR hcall number and the guest
+     * machine check address before copying to 0x200 vector
+     */
+    cpu_physical_memory_read(spapr->rtas_addr + RTAS_TRAMPOLINE_OFFSET,
+                             trampoline, sizeof(trampoline));
+
+    /* Safety Check */

Same for this comment.

hw/ppc/spapr_rtas.c static void rtas_ibm_os_term(PowerPCCPU *cpu, · line 327

Quoted 6 lines

hw/ppc/spapr_rtas.c +6 -0

+    QEMU_BUILD_BUG_ON(sizeof(trampoline) > MC_INTERRUPT_VECTOR_SIZE);
+
+    /* Update the KVMPPC_H_REPORT_MC_ERR value in trampoline */
+    ori_inst |= KVMPPC_H_REPORT_MC_ERR;
+    memcpy(&trampoline[TRAMPOLINE_ORI_INST_INDEX], &ori_inst,
+            sizeof(ori_inst));

Why memcpy a u32 into a u32 array?

hw/ppc/spapr_rtas.c static void rtas_ibm_os_term(PowerPCCPU *cpu, · line 335

Quoted 9 lines

hw/ppc/spapr_rtas.c +8 -0

+
+    /*
+     * Sanity check guest_machine_check_addr to prevent clobbering
+     * operator value in branch instruction
+     */
+    if (guest_machine_check_addr & BRANCH_INST_MASK) {
+        fprintf(stderr, "Unable to register ibm,nmi_register: "
+                "Invalid machine check handler address\n");

In general, printf's in guest triggerable code aren't a great idea, since the guest could flood our host logs with this. I can't say we're doing a great job at it already though, so it probably doesn't matter much.

Quoted 1 line

hw/ppc/spapr_rtas.c +1 -0

+        rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);

pc-bios/spapr-rtas/spapr-rtas.S

Quoted 97 lines

hw/ppc/spapr_rtas.c +49 -0

+        return;
+    }
+
+    /*
+     * Update the branch instruction in trampoline
+     * with the absolute machine check address requested by OS.
+     */
+    branch_inst |= guest_machine_check_addr;
+    memcpy(&trampoline[TRAMPOLINE_BR_INST_INDEX], &branch_inst,
+            sizeof(branch_inst));
+
+    /* Handle all Host/Guest LE/BE combinations */
+    if ((*pcc->interrupts_big_endian)(cpu)) {
+        for (i = 0; i < total_inst; i++) {
+            trampoline[i] = cpu_to_be32(trampoline[i]);
+        }
+    } else {
+        for (i = 0; i < total_inst; i++) {
+            trampoline[i] = cpu_to_le32(trampoline[i]);
+        }
+    }
+
+    /* Patch 0x200 NMI interrupt vector memory area of guest */
+    cpu_physical_memory_write(MC_INTERRUPT_VECTOR, trampoline,
+                              sizeof(trampoline));
+
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+}
+
+static void rtas_ibm_nmi_interlock(PowerPCCPU *cpu,
+                                   sPAPREnvironment *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 unset mc_in_progress.
+     */
+    mc_in_progress = 0;
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+}
+
 static struct rtas_call {
     const char *name;
     spapr_rtas_fn fn;

@@ -419,6 +506,12 @@ static void core_rtas_register_types(void)
                         rtas_ibm_set_system_parameter);
     spapr_rtas_register(RTAS_IBM_OS_TERM, "ibm,os-term",
                         rtas_ibm_os_term);
+    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 +16 -1

index a2d67e9..98d0a6c 100644
@@ -384,8 +384,10 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi);
 #define RTAS_GET_SENSOR_STATE                   (RTAS_TOKEN_BASE + 0x1D)
 #define RTAS_IBM_CONFIGURE_CONNECTOR            (RTAS_TOKEN_BASE + 0x1E)
 #define RTAS_IBM_OS_TERM                        (RTAS_TOKEN_BASE + 0x1F)
+#define RTAS_IBM_NMI_REGISTER                   (RTAS_TOKEN_BASE + 0x20)
+#define RTAS_IBM_NMI_INTERLOCK                  (RTAS_TOKEN_BASE + 0x21)
 
-#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x20)
+#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x22)
 
 /* RTAS ibm,get-system-parameter token values */
 #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS      20

@@ -488,4 +490,17 @@ int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname,
 #define RTAS_TRAMPOLINE_OFFSET   0x200
 #define RTAS_ERRLOG_OFFSET       0x800
 
+/* Machine Check Trampoline related macros
+ *
+ * These macros should co-relate to the code we
+ * have in pc-bios/spapr-rtas/spapr-rtas.S
+ */
+#define TRAMPOLINE_INSTS           17
+#define TRAMPOLINE_ORI_INST_INDEX  2
+#define TRAMPOLINE_BR_INST_INDEX   15
+
+/* Machine Check Interrupt related macros */
+#define MC_INTERRUPT_VECTOR           0x200
+#define MC_INTERRUPT_VECTOR_SIZE      0x100
+
 #endif /* !defined (__HW_SPAPR_H__) */

pc-bios/spapr-rtas/spapr-rtas.S +0 -0

index 903bec2..c315332 100644

Please add #defines at the top of the file for the register names:

  #define r0 0
  #define r1 1
  ...

That way the code below will get much more readable :)

Also, you want a jump table here as we discussed in the last review round. That table would tell you

a) Entry address for RTAS
b) Offset of the NMI code
c) To-be-patched offsets of the instructions inside the NMI code

Then we have all offsets automatically generated inside a single file and don't have to maintain fragile relationships between random headers with offset defines and the .S file.

Alex

Quoted 16 lines

pc-bios/spapr-rtas/spapr-rtas.S +12 -0

@@ -35,3 +35,41 @@ _start:
 	ori	3,3,KVMPPC_H_RTAS@l
 	sc	1
 	blr
+	. = 0x200
+	/*
+	 * Trampoline saves r3 in sprg2 and issues private hcall
+	 * to request qemu to build error log. QEMU builds the
+	 * error log, copies to rtas-blob and returns the address.
+	 * The initial 16 bytes in return adress consist of saved
+	 * srr0 and srr1 which we restore and pass on the actual error
+	 * log address to OS handled mcachine check notification
+	 * routine
+	 *
+	 * All the below instructions are copied to interrupt vector
+	 * 0x200 at the time of handling ibm,nmi-register rtas call.

pc-bios/spapr-rtas/spapr-rtas.S _start: · line 57

Quoted 8 lines

pc-bios/spapr-rtas/spapr-rtas.S +8 -0

+	 */
+	mtsprg  2,3
+	li      3,0
+	/*
+	 * ori r3,r3,KVMPPC_H_REPORT_MC_ERR. The KVMPPC_H_REPORT_MC_ERR
+	 * value is patched below
+	 */
+1:	ori     3,3,0

Quoted 20 lines

pc-bios/spapr-rtas/spapr-rtas.S +18 -0

+	sc      1               /* Issue H_CALL */
+	cmpdi   cr0,3,0
+	beq     cr0,1b          /* retry KVMPPC_H_REPORT_MC_ERR */
+	mtsprg  2,4
+	ld      4,0(3)
+	mtsrr0  4               /* Restore srr0 */
+	ld      4,8(3)
+	mtsrr1  4               /* Restore srr1 */
+	ld      4,16(3)
+	mtcrf   0,4             /* Restore cr */
+	addi    3,3,24
+	mfsprg  4,2
+	/*
+	 * Branch to address registered by OS. The branch address is
+	 * patched in the ibm,nmi-register rtas call.
+	 */
+	ba      0x0
+	b       .

Quoted 132 lines · Alexander Graf

Quoted 38 lines · Aravinda Prasad

This patch adds FWNMI support in qemu for powerKVM guests by handling the ibm,nmi-register rtas call. Whenever OS issues ibm,nmi-register RTAS call, the machine check notification address is saved and the machine check interrupt vector 0x200 is patched to issue a private hcall.

This patch also handles the cases when multi-processors experience machine check at or about the same time. As per PAPR, subsequent processors serialize waiting for the first processor to issue the ibm,nmi-interlock call. The second processor retries if the first processor which received a machine check is still reading the error log and is yet to issue ibm,nmi-interlock call.

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

---
 hw/ppc/spapr_hcall.c            |   16 +++++++
 hw/ppc/spapr_rtas.c             |   93 +++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h          |   17 +++++++
 pc-bios/spapr-rtas/spapr-rtas.S |   38 ++++++++++++++++
 4 files changed, 163 insertions(+), 1 deletion(-)

hw/ppc/spapr_hcall.c +3 -0

index 8f16160..eceb5e5 100644
@@ -97,6 +97,9 @@ struct rtas_mc_log {
     struct rtas_error_log err_log;
 };
 
+/* Whether machine check handling is in progress by any CPU */
+bool mc_in_progress;
+
 static void do_spr_sync(void *arg)
 {
     struct SPRSyncState *s = arg;

hw/ppc/spapr_hcall.c static target_ulong h_report_mc_err(PowerPCCPU *cpu, sPAPREnvironment *spapr, · line 690

Quoted 10 lines

hw/ppc/spapr_hcall.c +7 -0

@@ -678,6 +681,19 @@ static target_ulong h_report_mc_err(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     cpu_synchronize_state(CPU(ppc_env_get_cpu(env)));
 
     /*
+     * Only one VCPU can process machine check NMI at a time. Hence
+     * set the lock mc_in_progress. Once the VCPU finishes processing
+     * NMI, it executes ibm,nmi-interlock and mc_in_progress is unset
+     * in ibm,nmi-interlock handler. Meanwhile if other VCPUs encounter
+     * NMI we return 0 asking the VCPU to retry h_report_mc_err
+     */
+    if (mc_in_progress == 1) {

Please don't depend on bools being numbers. Use true / false. For if()s, just don't use == at all - it makes it more readable.

Quoted 14 lines

hw/ppc/spapr_hcall.c +6 -0

+        return 0;
+    }
+
+    mc_in_progress = 1;
+
+    /*
      * We save the original r3 register in SPRG2 in 0x200 vector,
      * which is patched during call to ibm.nmi-register. Original
      * r3 is required to be included in error log

hw/ppc/spapr_rtas.c +0 -0

index 2ec2a8e..71c7662 100644

hw/ppc/spapr_rtas.c line 40

Quoted 5 lines

hw/ppc/spapr_rtas.c +2 -0

@@ -36,6 +36,9 @@
 
 #include <libfdt.h>
 
+#define BRANCH_INST_MASK  0xFC000000
+extern bool mc_in_progress;

Please put this into the spapr struct.

Quoted 10 lines

hw/ppc/spapr_rtas.c +3 -0

+
 static void rtas_display_character(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                    uint32_t token, uint32_t nargs,
                                    target_ulong args,

@@ -290,6 +293,90 @@ static void rtas_ibm_os_term(PowerPCCPU *cpu,
     rtas_st(rets, 0, ret);
 }
 
+static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
+                                  sPAPREnvironment *spapr,

hw/ppc/spapr_rtas.c static void rtas_ibm_os_term(PowerPCCPU *cpu, · line 307

Quoted 10 lines

hw/ppc/spapr_rtas.c +10 -0

+                                  uint32_t token, uint32_t nargs,
+                                  target_ulong args,
+                                  uint32_t nret, target_ulong rets)
+{
+    int i;
+    uint32_t ori_inst = 0x60630000;
+    uint32_t branch_inst = 0x48000002;
+    target_ulong guest_machine_check_addr;
+    uint32_t trampoline[TRAMPOLINE_INSTS];
+    int total_inst = sizeof(trampoline) / sizeof(uint32_t);

ARRAY_SIZE(trampoline), though I don't quite understand why you need a variable that contains the same value as a constant (TRAMPOLINE_INSTS).

But since you're moving all of those bits into variable fields on the rtas blob itself as we discussed in the last version, I guess this code will go away anyways ;).

hw/ppc/spapr_rtas.c static void rtas_ibm_os_term(PowerPCCPU *cpu, · line 311

Quoted 4 lines

hw/ppc/spapr_rtas.c +4 -0

+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+
+    /* Store the system reset and machine check address */
+    guest_machine_check_addr = rtas_ld(args, 1);

Load or Store? I don't find the comment particularly useful either ;).

hw/ppc/spapr_rtas.c static void rtas_ibm_os_term(PowerPCCPU *cpu, · line 321

Quoted 10 lines

hw/ppc/spapr_rtas.c +10 -0

+
+    /*
+     * Read the trampoline instructions from RTAS Blob and patch
+     * the KVMPPC_H_REPORT_MC_ERR hcall number and the guest
+     * machine check address before copying to 0x200 vector
+     */
+    cpu_physical_memory_read(spapr->rtas_addr + RTAS_TRAMPOLINE_OFFSET,
+                             trampoline, sizeof(trampoline));
+
+    /* Safety Check */

Same for this comment.

Quoted 6 lines

hw/ppc/spapr_rtas.c +6 -0

+    QEMU_BUILD_BUG_ON(sizeof(trampoline) > MC_INTERRUPT_VECTOR_SIZE);
+
+    /* Update the KVMPPC_H_REPORT_MC_ERR value in trampoline */
+    ori_inst |= KVMPPC_H_REPORT_MC_ERR;
+    memcpy(&trampoline[TRAMPOLINE_ORI_INST_INDEX], &ori_inst,
+            sizeof(ori_inst));

Quoted 1 line

Why memcpy a u32 into a u32 array?

Additionally, I don't see the need for the ori_inst *variable* .... the instruction is known at compile time. So why not just do

  trampoline[TRAMPOLINE_ORI_INST_INDEX] = 0x60630000 | KVMPPC_H_REPORT_MC_ERR;

Likewise for the branch_inst variable.

Also see my comment in the trampoline code below.

Quoted 10 lines

Quoted 9 lines

hw/ppc/spapr_rtas.c +8 -0

+
+    /*
+     * Sanity check guest_machine_check_addr to prevent clobbering
+     * operator value in branch instruction
+     */
+    if (guest_machine_check_addr & BRANCH_INST_MASK) {
+        fprintf(stderr, "Unable to register ibm,nmi_register: "
+                "Invalid machine check handler address\n");

Quoted 5 lines

In general, printf's in guest triggerable code aren't a great idea, since the guest could flood our host logs with this. I can't say we're doing a great job at it already though, so it probably doesn't matter much.

Quoted 1 line

hw/ppc/spapr_rtas.c +1 -0

+        rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);

NIT: Shouldn't this be RTAS_OUT_PARAM_ERR? That is what SPAPR says (both are implemented to be -3).

Quoted 139 lines

pc-bios/spapr-rtas/spapr-rtas.S

Quoted 98 lines

hw/ppc/spapr_rtas.c +49 -0

+        return;
+    }
+
+    /*
+     * Update the branch instruction in trampoline
+     * with the absolute machine check address requested by OS.
+     */
+    branch_inst |= guest_machine_check_addr;
+    memcpy(&trampoline[TRAMPOLINE_BR_INST_INDEX], &branch_inst,
+            sizeof(branch_inst));
+
+    /* Handle all Host/Guest LE/BE combinations */
+    if ((*pcc->interrupts_big_endian)(cpu)) {
+        for (i = 0; i < total_inst; i++) {
+            trampoline[i] = cpu_to_be32(trampoline[i]);
+        }
+    } else {
+        for (i = 0; i < total_inst; i++) {
+            trampoline[i] = cpu_to_le32(trampoline[i]);
+        }
+    }
+
+    /* Patch 0x200 NMI interrupt vector memory area of guest */
+    cpu_physical_memory_write(MC_INTERRUPT_VECTOR, trampoline,
+                              sizeof(trampoline));
+
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+}
+
+static void rtas_ibm_nmi_interlock(PowerPCCPU *cpu,
+                                   sPAPREnvironment *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 unset mc_in_progress.
+     */
+    mc_in_progress = 0;
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+}
+
 static struct rtas_call {
     const char *name;
     spapr_rtas_fn fn;

@@ -419,6 +506,12 @@ static void core_rtas_register_types(void)
                         rtas_ibm_set_system_parameter);
     spapr_rtas_register(RTAS_IBM_OS_TERM, "ibm,os-term",
                         rtas_ibm_os_term);
+    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 +16 -1

index a2d67e9..98d0a6c 100644
@@ -384,8 +384,10 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi);
 #define RTAS_GET_SENSOR_STATE                   (RTAS_TOKEN_BASE + 0x1D)
 #define RTAS_IBM_CONFIGURE_CONNECTOR            (RTAS_TOKEN_BASE + 0x1E)
 #define RTAS_IBM_OS_TERM                        (RTAS_TOKEN_BASE + 0x1F)
+#define RTAS_IBM_NMI_REGISTER                   (RTAS_TOKEN_BASE + 0x20)
+#define RTAS_IBM_NMI_INTERLOCK                  (RTAS_TOKEN_BASE + 0x21)
 
-#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x20)
+#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x22)
 
 /* RTAS ibm,get-system-parameter token values */
 #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS      20

@@ -488,4 +490,17 @@ int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname,
 #define RTAS_TRAMPOLINE_OFFSET   0x200
 #define RTAS_ERRLOG_OFFSET       0x800
 
+/* Machine Check Trampoline related macros
+ *
+ * These macros should co-relate to the code we
+ * have in pc-bios/spapr-rtas/spapr-rtas.S
+ */
+#define TRAMPOLINE_INSTS           17
+#define TRAMPOLINE_ORI_INST_INDEX  2
+#define TRAMPOLINE_BR_INST_INDEX   15
+
+/* Machine Check Interrupt related macros */
+#define MC_INTERRUPT_VECTOR           0x200
+#define MC_INTERRUPT_VECTOR_SIZE      0x100
+
 #endif /* !defined (__HW_SPAPR_H__) */

pc-bios/spapr-rtas/spapr-rtas.S +0 -0

index 903bec2..c315332 100644

Please add #defines at the top of the file for the register names:

  #define r0 0
  #define r1 1
  ...

That way the code below will get much more readable :)

Also, you want a jump table here as we discussed in the last review round. That table would tell you

a) Entry address for RTAS
b) Offset of the NMI code
c) To-be-patched offsets of the instructions inside the NMI code

Then we have all offsets automatically generated inside a single file and don't have to maintain fragile relationships between random headers with offset defines and the .S file.

Alex

Quoted 18 lines

pc-bios/spapr-rtas/spapr-rtas.S +14 -0

@@ -35,3 +35,41 @@ _start:
 	ori	3,3,KVMPPC_H_RTAS@l
 	sc	1
 	blr
+	. = 0x200
+	/*
+	 * Trampoline saves r3 in sprg2 and issues private hcall
+	 * to request qemu to build error log. QEMU builds the
+	 * error log, copies to rtas-blob and returns the address.
+	 * The initial 16 bytes in return adress consist of saved
+	 * srr0 and srr1 which we restore and pass on the actual error
+	 * log address to OS handled mcachine check notification
+	 * routine
+	 *
+	 * All the below instructions are copied to interrupt vector
+	 * 0x200 at the time of handling ibm,nmi-register rtas call.
+	 */
+	mtsprg  2,3

Quoted 6 lines

pc-bios/spapr-rtas/spapr-rtas.S _start: · line 57

Quoted 6 lines

pc-bios/spapr-rtas/spapr-rtas.S +6 -0

+	li      3,0
+	/*
+	 * ori r3,r3,KVMPPC_H_REPORT_MC_ERR. The KVMPPC_H_REPORT_MC_ERR
+	 * value is patched below
+	 */
+1:	ori     3,3,0

Why do "li 3,0" followed by "ori 3,3,X"? Isn't this just "li 3,X" ? (aka "addi 3,0,X")

And, perhaps this was discussed in an earlier patch, but couldn't you just do:

li 3,KVMPPC_H_REPORT_MC_ERR

here and avoid the patching altogether?

Quoted 24 lines

Quoted 23 lines

pc-bios/spapr-rtas/spapr-rtas.S +18 -0

+	sc      1               /* Issue H_CALL */
+	cmpdi   cr0,3,0
+	beq     cr0,1b          /* retry KVMPPC_H_REPORT_MC_ERR */
+	mtsprg  2,4
+	ld      4,0(3)
+	mtsrr0  4               /* Restore srr0 */
+	ld      4,8(3)
+	mtsrr1  4               /* Restore srr1 */
+	ld      4,16(3)
+	mtcrf   0,4             /* Restore cr */
+	addi    3,3,24
+	mfsprg  4,2
+	/*
+	 * Branch to address registered by OS. The branch address is
+	 * patched in the ibm,nmi-register rtas call.
+	 */
+	ba      0x0
+	b       .

Am 06.11.2014 um 11:00 schrieb Aravinda Prasad <[email protected]>:

Quoted 134 lines · Tom Musta

Quoted 133 lines · Alexander Graf

Quoted 41 lines · Aravinda Prasad

This patch adds FWNMI support in qemu for powerKVM guests by handling the ibm,nmi-register rtas call. Whenever OS issues ibm,nmi-register RTAS call, the machine check notification address is saved and the machine check interrupt vector 0x200 is patched to issue a private hcall.

This patch also handles the cases when multi-processors experience machine check at or about the same time. As per PAPR, subsequent processors serialize waiting for the first processor to issue the ibm,nmi-interlock call. The second processor retries if the first processor which received a machine check is still reading the error log and is yet to issue ibm,nmi-interlock call.

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

---

hw/ppc/spapr_hcall.c            |   16 +++++++
hw/ppc/spapr_rtas.c             |   93 +++++++++++++++++++++++++++++++++++++++
include/hw/ppc/spapr.h          |   17 +++++++
pc-bios/spapr-rtas/spapr-rtas.S |   38 ++++++++++++++++
4 files changed, 163 insertions(+), 1 deletion(-)

hw/ppc/spapr_hcall.c +3 -0

index 8f16160..eceb5e5 100644
@@ -97,6 +97,9 @@ struct rtas_mc_log {
    struct rtas_error_log err_log;
 };
 
+/* Whether machine check handling is in progress by any CPU */
+bool mc_in_progress;
+
 static void do_spr_sync(void *arg)
 {
    struct SPRSyncState *s = arg;

hw/ppc/spapr_hcall.c static target_ulong h_report_mc_err(PowerPCCPU *cpu, sPAPREnvironment *spapr, · line 690

Quoted 10 lines

hw/ppc/spapr_hcall.c +7 -0

@@ -678,6 +681,19 @@ static target_ulong h_report_mc_err(PowerPCCPU *cpu, sPAPREnvironment *spapr,
    cpu_synchronize_state(CPU(ppc_env_get_cpu(env)));
 
    /*
+     * Only one VCPU can process machine check NMI at a time. Hence
+     * set the lock mc_in_progress. Once the VCPU finishes processing
+     * NMI, it executes ibm,nmi-interlock and mc_in_progress is unset
+     * in ibm,nmi-interlock handler. Meanwhile if other VCPUs encounter
+     * NMI we return 0 asking the VCPU to retry h_report_mc_err
+     */
+    if (mc_in_progress == 1) {

Please don't depend on bools being numbers. Use true / false. For if()s, just don't use == at all - it makes it more readable.

Quoted 14 lines

hw/ppc/spapr_hcall.c +6 -0

+        return 0;
+    }
+
+    mc_in_progress = 1;
+
+    /*
     * We save the original r3 register in SPRG2 in 0x200 vector,
     * which is patched during call to ibm.nmi-register. Original
     * r3 is required to be included in error log

hw/ppc/spapr_rtas.c +0 -0

index 2ec2a8e..71c7662 100644

hw/ppc/spapr_rtas.c line 40

Quoted 5 lines

hw/ppc/spapr_rtas.c +2 -0

@@ -36,6 +36,9 @@
 
 #include <libfdt.h>
 
+#define BRANCH_INST_MASK  0xFC000000
+extern bool mc_in_progress;

Please put this into the spapr struct.

Quoted 10 lines

hw/ppc/spapr_rtas.c +3 -0

+
 static void rtas_display_character(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                   uint32_t token, uint32_t nargs,
                                   target_ulong args,

@@ -290,6 +293,90 @@ static void rtas_ibm_os_term(PowerPCCPU *cpu,
    rtas_st(rets, 0, ret);
 }
 
+static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
+                                  sPAPREnvironment *spapr,

hw/ppc/spapr_rtas.c static void rtas_ibm_os_term(PowerPCCPU *cpu, · line 307

Quoted 10 lines

hw/ppc/spapr_rtas.c +10 -0

+                                  uint32_t token, uint32_t nargs,
+                                  target_ulong args,
+                                  uint32_t nret, target_ulong rets)
+{
+    int i;
+    uint32_t ori_inst = 0x60630000;
+    uint32_t branch_inst = 0x48000002;
+    target_ulong guest_machine_check_addr;
+    uint32_t trampoline[TRAMPOLINE_INSTS];
+    int total_inst = sizeof(trampoline) / sizeof(uint32_t);

ARRAY_SIZE(trampoline), though I don't quite understand why you need a variable that contains the same value as a constant (TRAMPOLINE_INSTS).

But since you're moving all of those bits into variable fields on the rtas blob itself as we discussed in the last version, I guess this code will go away anyways ;).

hw/ppc/spapr_rtas.c static void rtas_ibm_os_term(PowerPCCPU *cpu, · line 311

Quoted 4 lines

hw/ppc/spapr_rtas.c +4 -0

+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+
+    /* Store the system reset and machine check address */
+    guest_machine_check_addr = rtas_ld(args, 1);

Load or Store? I don't find the comment particularly useful either ;).

hw/ppc/spapr_rtas.c static void rtas_ibm_os_term(PowerPCCPU *cpu, · line 321

Quoted 10 lines

hw/ppc/spapr_rtas.c +10 -0

+
+    /*
+     * Read the trampoline instructions from RTAS Blob and patch
+     * the KVMPPC_H_REPORT_MC_ERR hcall number and the guest
+     * machine check address before copying to 0x200 vector
+     */
+    cpu_physical_memory_read(spapr->rtas_addr + RTAS_TRAMPOLINE_OFFSET,
+                             trampoline, sizeof(trampoline));
+
+    /* Safety Check */

Same for this comment.

Quoted 6 lines

hw/ppc/spapr_rtas.c +6 -0

+    QEMU_BUILD_BUG_ON(sizeof(trampoline) > MC_INTERRUPT_VECTOR_SIZE);
+
+    /* Update the KVMPPC_H_REPORT_MC_ERR value in trampoline */
+    ori_inst |= KVMPPC_H_REPORT_MC_ERR;
+    memcpy(&trampoline[TRAMPOLINE_ORI_INST_INDEX], &ori_inst,
+            sizeof(ori_inst));

Quoted 6 lines

Quoted 1 line

Why memcpy a u32 into a u32 array?

Additionally, I don't see the need for the ori_inst *variable* .... the instruction is known at compile time. So why not just do

trampoline[TRAMPOLINE_ORI_INST_INDEX] = 0x60630000 | KVMPPC_H_REPORT_MC_ERR;

I can directly do trampoline[TRAMPOLINE_ORI_INST_INDEX] |= KVMPPC_H_REPORT_MC_ERR;

as trampoline[TRAMPOLINE_ORI_INST_INDEX] already contains 0x60630000

Quoted 14 lines

Likewise for the branch_inst variable.

Also see my comment in the trampoline code below.

Quoted 10 lines

Quoted 9 lines

hw/ppc/spapr_rtas.c +8 -0

+
+    /*
+     * Sanity check guest_machine_check_addr to prevent clobbering
+     * operator value in branch instruction
+     */
+    if (guest_machine_check_addr & BRANCH_INST_MASK) {
+        fprintf(stderr, "Unable to register ibm,nmi_register: "
+                "Invalid machine check handler address\n");

Quoted 7 lines

Quoted 5 lines

In general, printf's in guest triggerable code aren't a great idea, since the guest could flood our host logs with this. I can't say we're doing a great job at it already though, so it probably doesn't matter much.

Quoted 1 line

hw/ppc/spapr_rtas.c +1 -0

+        rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);

NIT: Shouldn't this be RTAS_OUT_PARAM_ERR? That is what SPAPR says (both are implemented to be -3).

Yes, SPAPR says -3 Parameter Error. I think RTAS_OUT_PARAM_ERR is better to be in consistent with SPAPR.

Quoted 139 lines

Quoted 139 lines

pc-bios/spapr-rtas/spapr-rtas.S

Quoted 98 lines

hw/ppc/spapr_rtas.c +49 -0

+        return;
+    }
+
+    /*
+     * Update the branch instruction in trampoline
+     * with the absolute machine check address requested by OS.
+     */
+    branch_inst |= guest_machine_check_addr;
+    memcpy(&trampoline[TRAMPOLINE_BR_INST_INDEX], &branch_inst,
+            sizeof(branch_inst));
+
+    /* Handle all Host/Guest LE/BE combinations */
+    if ((*pcc->interrupts_big_endian)(cpu)) {
+        for (i = 0; i < total_inst; i++) {
+            trampoline[i] = cpu_to_be32(trampoline[i]);
+        }
+    } else {
+        for (i = 0; i < total_inst; i++) {
+            trampoline[i] = cpu_to_le32(trampoline[i]);
+        }
+    }
+
+    /* Patch 0x200 NMI interrupt vector memory area of guest */
+    cpu_physical_memory_write(MC_INTERRUPT_VECTOR, trampoline,
+                              sizeof(trampoline));
+
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+}
+
+static void rtas_ibm_nmi_interlock(PowerPCCPU *cpu,
+                                   sPAPREnvironment *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 unset mc_in_progress.
+     */
+    mc_in_progress = 0;
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+}
+
 static struct rtas_call {
    const char *name;
    spapr_rtas_fn fn;

@@ -419,6 +506,12 @@ static void core_rtas_register_types(void)
                        rtas_ibm_set_system_parameter);
    spapr_rtas_register(RTAS_IBM_OS_TERM, "ibm,os-term",
                        rtas_ibm_os_term);
+    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 +16 -1

index a2d67e9..98d0a6c 100644
@@ -384,8 +384,10 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi);
 #define RTAS_GET_SENSOR_STATE                   (RTAS_TOKEN_BASE + 0x1D)
 #define RTAS_IBM_CONFIGURE_CONNECTOR            (RTAS_TOKEN_BASE + 0x1E)
 #define RTAS_IBM_OS_TERM                        (RTAS_TOKEN_BASE + 0x1F)
+#define RTAS_IBM_NMI_REGISTER                   (RTAS_TOKEN_BASE + 0x20)
+#define RTAS_IBM_NMI_INTERLOCK                  (RTAS_TOKEN_BASE + 0x21)
 
-#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x20)
+#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x22)
 
 /* RTAS ibm,get-system-parameter token values */
 #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS      20

@@ -488,4 +490,17 @@ int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname,
 #define RTAS_TRAMPOLINE_OFFSET   0x200
 #define RTAS_ERRLOG_OFFSET       0x800
 
+/* Machine Check Trampoline related macros
+ *
+ * These macros should co-relate to the code we
+ * have in pc-bios/spapr-rtas/spapr-rtas.S
+ */
+#define TRAMPOLINE_INSTS           17
+#define TRAMPOLINE_ORI_INST_INDEX  2
+#define TRAMPOLINE_BR_INST_INDEX   15
+
+/* Machine Check Interrupt related macros */
+#define MC_INTERRUPT_VECTOR           0x200
+#define MC_INTERRUPT_VECTOR_SIZE      0x100
+
 #endif /* !defined (__HW_SPAPR_H__) */

pc-bios/spapr-rtas/spapr-rtas.S +0 -0

index 903bec2..c315332 100644

Please add #defines at the top of the file for the register names:

#define r0 0
#define r1 1
...

That way the code below will get much more readable :)

Also, you want a jump table here as we discussed in the last review round. That table would tell you

a) Entry address for RTAS
b) Offset of the NMI code
c) To-be-patched offsets of the instructions inside the NMI code

Then we have all offsets automatically generated inside a single file and don't have to maintain fragile relationships between random headers with offset defines and the .S file.

Alex

Quoted 18 lines

pc-bios/spapr-rtas/spapr-rtas.S +14 -0

@@ -35,3 +35,41 @@ _start:
   ori    3,3,KVMPPC_H_RTAS@l
   sc    1
   blr
+    . = 0x200
+    /*
+     * Trampoline saves r3 in sprg2 and issues private hcall
+     * to request qemu to build error log. QEMU builds the
+     * error log, copies to rtas-blob and returns the address.
+     * The initial 16 bytes in return adress consist of saved
+     * srr0 and srr1 which we restore and pass on the actual error
+     * log address to OS handled mcachine check notification
+     * routine
+     *
+     * All the below instructions are copied to interrupt vector
+     * 0x200 at the time of handling ibm,nmi-register rtas call.
+     */
+    mtsprg  2,3

Quoted 8 lines

Quoted 6 lines

pc-bios/spapr-rtas/spapr-rtas.S _start: · line 57

Quoted 6 lines

pc-bios/spapr-rtas/spapr-rtas.S +6 -0

+    li      3,0
+    /*
+     * ori r3,r3,KVMPPC_H_REPORT_MC_ERR. The KVMPPC_H_REPORT_MC_ERR
+     * value is patched below
+     */
+1:    ori     3,3,0

Why do "li 3,0" followed by "ori 3,3,X"? Isn't this just "li 3,X" ? (aka "addi 3,0,X")

I remember I first tried doing li r3,X but faced some problem (but not able to exactly recall what was the problem) may be due to not familiar with ppc assembly.

I will fix this.

Quoted 3 lines

And, perhaps this was discussed in an earlier patch, but couldn't you just do:

Quoted 3 lines

li 3,KVMPPC_H_REPORT_MC_ERR

here and avoid the patching altogether?

KVMPPC_H_REPORT_MC_ERR def in not visible in spapr-rtas.S, either I can define it in spapr-rtas.S as already done for KVMPPC_H_RTAS or patch it in ibm,nmi-register call.

It is very unlikely that the KVMPPC_H_REPORT_MC_ERR will be changed, but I prefer to patch it to avoid maintaining it in both places. What do you think?

Quoted 22 lines

Quoted 22 lines

Quoted 22 lines

pc-bios/spapr-rtas/spapr-rtas.S +18 -0

+    sc      1               /* Issue H_CALL */
+    cmpdi   cr0,3,0
+    beq     cr0,1b          /* retry KVMPPC_H_REPORT_MC_ERR */
+    mtsprg  2,4
+    ld      4,0(3)
+    mtsrr0  4               /* Restore srr0 */
+    ld      4,8(3)
+    mtsrr1  4               /* Restore srr1 */
+    ld      4,16(3)
+    mtcrf   0,4             /* Restore cr */
+    addi    3,3,24
+    mfsprg  4,2
+    /*
+     * Branch to address registered by OS. The branch address is
+     * patched in the ibm,nmi-register rtas call.
+     */
+    ba      0x0
+    b       .

Signature · 2 lines

Regards,
Aravinda

Quoted 7 lines

Am 06.11.2014 um 11:00 schrieb Aravinda Prasad <[email protected]>:

Quoted 5 lines

Quoted 5 lines

And, perhaps this was discussed in an earlier patch, but couldn't you just do:

li 3,KVMPPC_H_REPORT_MC_ERR

Quoted 5 lines

Quoted 1 line

here and avoid the patching altogether?

KVMPPC_H_REPORT_MC_ERR def in not visible in spapr-rtas.S, either I can define it in spapr-rtas.S as already done for KVMPPC_H_RTAS or patch it in ibm,nmi-register call.

Could you include the header?

Quoted 5 lines

It is very unlikely that the KVMPPC_H_REPORT_MC_ERR will be changed, but I prefer to patch it to avoid maintaining it in both places. What do you think?

Hypercall numbers need to be stable anyway in case we migrate from an older qemu version, so it must not change.

Alex

Quoted 27 lines

Quoted 22 lines

Quoted 22 lines

Quoted 22 lines

pc-bios/spapr-rtas/spapr-rtas.S +18 -0

+    sc      1               /* Issue H_CALL */
+    cmpdi   cr0,3,0
+    beq     cr0,1b          /* retry KVMPPC_H_REPORT_MC_ERR */
+    mtsprg  2,4
+    ld      4,0(3)
+    mtsrr0  4               /* Restore srr0 */
+    ld      4,8(3)
+    mtsrr1  4               /* Restore srr1 */
+    ld      4,16(3)
+    mtcrf   0,4             /* Restore cr */
+    addi    3,3,24
+    mfsprg  4,2
+    /*
+     * Branch to address registered by OS. The branch address is
+     * patched in the ibm,nmi-register rtas call.
+     */
+    ba      0x0
+    b       .

Signature · 2 lines

Regards,
Aravinda

Quoted 2 lines · Tom Musta

Quoted 1 line · Alexander Graf

On 05.11.14 08:13, Aravinda Prasad wrote:

Quoted 7 lines

Quoted 5 lines

pc-bios/spapr-rtas/spapr-rtas.S _start: · line 57

Quoted 5 lines

pc-bios/spapr-rtas/spapr-rtas.S +5 -0

+	/*
+	 * ori r3,r3,KVMPPC_H_REPORT_MC_ERR. The KVMPPC_H_REPORT_MC_ERR
+	 * value is patched below
+	 */
+1:	ori     3,3,0

Why do "li 3,0" followed by "ori 3,3,X"? Isn't this just "li 3,X" ? (aka "addi 3,0,X")

I remember I first tried doing li r3,X but faced some problem (but not able to exactly recall what was the problem) may be due to not familiar with ppc assembly.

Quoted 3 lines · Aravinda Prasad

Quoted 2 lines · Tom Musta

Quoted 1 line · Alexander Graf

On 05.11.14 08:13, Aravinda Prasad wrote:

[snip]

Quoted 8 lines

Quoted 7 lines

Quoted 5 lines

pc-bios/spapr-rtas/spapr-rtas.S _start: · line 57

Quoted 5 lines

pc-bios/spapr-rtas/spapr-rtas.S +5 -0

+	/*
+	 * ori r3,r3,KVMPPC_H_REPORT_MC_ERR. The KVMPPC_H_REPORT_MC_ERR
+	 * value is patched below
+	 */
+1:	ori     3,3,0

Why do "li 3,0" followed by "ori 3,3,X"? Isn't this just "li 3,X" ? (aka "addi 3,0,X")

Quoted 3 lines

I remember I first tried doing li r3,X but faced some problem (but not able to exactly recall what was the problem) may be due to not familiar with ppc assembly.

This would be because with the offset to the private hcalls, the actual hcall number is 0xf003, which means an li instruction will sign extend it incorrectly. So you will need two instructions to load the number.

Quoted 18 lines · David Gibson

Quoted 3 lines · Aravinda Prasad

Quoted 2 lines · Tom Musta

Quoted 1 line · Alexander Graf

On 05.11.14 08:13, Aravinda Prasad wrote:

[snip]

Quoted 11 lines

Quoted 7 lines

Quoted 5 lines

pc-bios/spapr-rtas/spapr-rtas.S _start: · line 57

Quoted 5 lines

pc-bios/spapr-rtas/spapr-rtas.S +5 -0

+	/*
+	 * ori r3,r3,KVMPPC_H_REPORT_MC_ERR. The KVMPPC_H_REPORT_MC_ERR
+	 * value is patched below
+	 */
+1:	ori     3,3,0

Why do "li 3,0" followed by "ori 3,3,X"? Isn't this just "li 3,X" ? (aka "addi 3,0,X")

I remember I first tried doing li r3,X but faced some problem (but not able to exactly recall what was the problem) may be due to not familiar with ppc assembly.

Quoted 4 lines

This would be because with the offset to the private hcalls, the actual hcall number is 0xf003, which means an li instruction will sign extend it incorrectly. So you will need two instructions to load the number.

hmm.. ok

Quoted 26 lines · Aravinda Prasad

Quoted 22 lines · David Gibson

Quoted 3 lines · Aravinda Prasad

Quoted 2 lines · Tom Musta

Quoted 1 line · Alexander Graf

On 05.11.14 08:13, Aravinda Prasad wrote:

[snip]

Quoted 11 lines

Quoted 7 lines

Quoted 5 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+	/*
+	 * ori r3,r3,KVMPPC_H_REPORT_MC_ERR. The KVMPPC_H_REPORT_MC_ERR
+	 * value is patched below
+	 */
+1:	ori     3,3,0

Why do "li 3,0" followed by "ori 3,3,X"? Isn't this just "li 3,X" ? (aka "addi 3,0,X")

I remember I first tried doing li r3,X but faced some problem (but not able to exactly recall what was the problem) may be due to not familiar with ppc assembly.

This would be because with the offset to the private hcalls, the actual hcall number is 0xf003, which means an li instruction will sign extend it incorrectly. So you will need two instructions to load the number.

Quoted 1 line

hmm.. ok

At least, I think you'll need to instructions. I don't remember for certain if 'ori 3,0,X' will OR X with literal 0 or the contents of r0.

From ISA:

Quoted 27 lines · David Gibson

Quoted 26 lines · Aravinda Prasad

Quoted 22 lines · David Gibson

Quoted 3 lines · Aravinda Prasad

Quoted 2 lines · Tom Musta

Quoted 1 line · Alexander Graf

On 05.11.14 08:13, Aravinda Prasad wrote:

[snip]

Quoted 11 lines

Quoted 7 lines

Quoted 5 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+	/*
+	 * ori r3,r3,KVMPPC_H_REPORT_MC_ERR. The KVMPPC_H_REPORT_MC_ERR
+	 * value is patched below
+	 */
+1:	ori     3,3,0

Why do "li 3,0" followed by "ori 3,3,X"? Isn't this just "li 3,X" ? (aka "addi 3,0,X")

I remember I first tried doing li r3,X but faced some problem (but not able to exactly recall what was the problem) may be due to not familiar with ppc assembly.

This would be because with the offset to the private hcalls, the actual hcall number is 0xf003, which means an li instruction will sign extend it incorrectly. So you will need two instructions to load the number.

Quoted 4 lines

Quoted 1 line

hmm.. ok

At least, I think you'll need to instructions. I don't remember for certain if 'ori 3,0,X' will OR X with literal 0 or the contents of r0.

It is ORed with r0

Quoted 1 line

From ISA:

ori RA,RS,UI

(RA) <- (RS) | (0 || UI)

The contents of register RS are ORed with 0 || UI and the result is placed into register RA.

This patch adds FWNMI support in qemu for powerKVM guests by handling the ibm,nmi-register rtas call. Whenever OS issues ibm,nmi-register RTAS call, the machine check notification address is saved and the machine check interrupt vector 0x200 is patched to issue a private hcall.

This patch also handles the cases when multi-processors experience machine check at or about the same time. As per PAPR, subsequent processors serialize waiting for the first processor to issue the ibm,nmi-interlock call. The second processor retries if the first processor which received a machine check is still reading the error log and is yet to issue ibm,nmi-interlock call.

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

hw/ppc/spapr_rtas.c +9 -0

+
+    /* Store the system reset and machine check address */
+    guest_machine_check_addr = rtas_ld(args, 1);
+
+    /*
+     * Read the trampoline instructions from RTAS Blob and patch
+     * the KVMPPC_H_REPORT_MC_ERR hcall number and the guest
+     * machine check address before copying to 0x200 vector
+     */

hw/ppc/spapr_rtas.c +33 -0

+    /*
+     * Sanity check guest_machine_check_addr to prevent clobbering
+     * operator value in branch instruction
+     */
+    if (guest_machine_check_addr & BRANCH_INST_MASK) {
+        fprintf(stderr, "Unable to register ibm,nmi_register: "
+                "Invalid machine check handler address\n");
+        rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);
+        return;
+    }
+
+    /*
+     * Update the branch instruction in trampoline
+     * with the absolute machine check address requested by OS.
+     */
+    branch_inst |= guest_machine_check_addr;
+    memcpy(&trampoline[TRAMPOLINE_BR_INST_INDEX], &branch_inst,
+            sizeof(branch_inst));
+
+    /* Handle all Host/Guest LE/BE combinations */
+    if ((*pcc->interrupts_big_endian)(cpu)) {
+        for (i = 0; i < total_inst; i++) {
+            trampoline[i] = cpu_to_be32(trampoline[i]);
+        }
+    } else {
+        for (i = 0; i < total_inst; i++) {
+            trampoline[i] = cpu_to_le32(trampoline[i]);
+        }
+    }
+
+    /* Patch 0x200 NMI interrupt vector memory area of guest */
+    cpu_physical_memory_write(MC_INTERRUPT_VECTOR, trampoline,
+                              sizeof(trampoline));

hw/ppc/spapr_rtas.c +14 -0

+    /*
+     * VCPU issuing ibm,nmi-interlock is done with NMI handling,
+     * hence unset mc_in_progress.
+     */
+    mc_in_progress = 0;
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+}
+
 static struct rtas_call {
     const char *name;
     spapr_rtas_fn fn;

@@ -419,6 +506,12 @@ static void core_rtas_register_types(void)
                         rtas_ibm_set_system_parameter);
     spapr_rtas_register(RTAS_IBM_OS_TERM, "ibm,os-term",
                         rtas_ibm_os_term);
+    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 +16 -1

index a2d67e9..98d0a6c 100644
@@ -384,8 +384,10 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi);
 #define RTAS_GET_SENSOR_STATE                   (RTAS_TOKEN_BASE + 0x1D)
 #define RTAS_IBM_CONFIGURE_CONNECTOR            (RTAS_TOKEN_BASE + 0x1E)
 #define RTAS_IBM_OS_TERM                        (RTAS_TOKEN_BASE + 0x1F)
+#define RTAS_IBM_NMI_REGISTER                   (RTAS_TOKEN_BASE + 0x20)
+#define RTAS_IBM_NMI_INTERLOCK                  (RTAS_TOKEN_BASE + 0x21)
 
-#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x20)
+#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x22)
 
 /* RTAS ibm,get-system-parameter token values */
 #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS      20

@@ -488,4 +490,17 @@ int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname,
 #define RTAS_TRAMPOLINE_OFFSET   0x200
 #define RTAS_ERRLOG_OFFSET       0x800
 
+/* Machine Check Trampoline related macros
+ *
+ * These macros should co-relate to the code we
+ * have in pc-bios/spapr-rtas/spapr-rtas.S
+ */
+#define TRAMPOLINE_INSTS           17
+#define TRAMPOLINE_ORI_INST_INDEX  2
+#define TRAMPOLINE_BR_INST_INDEX   15
+
+/* Machine Check Interrupt related macros */
+#define MC_INTERRUPT_VECTOR           0x200
+#define MC_INTERRUPT_VECTOR_SIZE      0x100
+
 #endif /* !defined (__HW_SPAPR_H__) */

pc-bios/spapr-rtas/spapr-rtas.S +13 -0

index 903bec2..c315332 100644
@@ -35,3 +35,41 @@ _start:
 	ori	3,3,KVMPPC_H_RTAS@l
 	sc	1
 	blr
+	. = 0x200
+	/*
+	 * Trampoline saves r3 in sprg2 and issues private hcall
+	 * to request qemu to build error log. QEMU builds the
+	 * error log, copies to rtas-blob and returns the address.
+	 * The initial 16 bytes in return adress consist of saved
+	 * srr0 and srr1 which we restore and pass on the actual error
+	 * log address to OS handled mcachine check notification
+	 * routine
+	 *
+	 * All the below instructions are copied to interrupt vector
+	 * 0x200 at the time of handling ibm,nmi-register rtas call.
+	 */

Quoted 16 lines · Aravinda Prasad

This patch adds FWNMI support in qemu for powerKVM guests by handling the ibm,nmi-register rtas call. Whenever OS issues ibm,nmi-register RTAS call, the machine check notification address is saved and the machine check interrupt vector 0x200 is patched to issue a private hcall.

This patch also handles the cases when multi-processors experience machine check at or about the same time. As per PAPR, subsequent processors serialize waiting for the first processor to issue the ibm,nmi-interlock call. The second processor retries if the first processor which received a machine check is still reading the error log and is yet to issue ibm,nmi-interlock call.

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

[snip]

hw/ppc/spapr_rtas.c static void rtas_ibm_os_term(PowerPCCPU *cpu, · line 308

Quoted 13 lines

hw/ppc/spapr_rtas.c +13 -0

+static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
+                                  sPAPREnvironment *spapr,
+                                  uint32_t token, uint32_t nargs,
+                                  target_ulong args,
+                                  uint32_t nret, target_ulong rets)
+{
+    int i;
+    uint32_t ori_inst = 0x60630000;
+    uint32_t branch_inst = 0x48000002;
+    target_ulong guest_machine_check_addr;
+    uint32_t trampoline[TRAMPOLINE_INSTS];
+    int total_inst = sizeof(trampoline) / sizeof(uint32_t);
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);

You should sanity check the RTAS arguments before doing anything - in particular verify that nargs and nrets have the expected values.

Quoted 17 lines

hw/ppc/spapr_rtas.c +16 -0

+
+    /* Store the system reset and machine check address */
+    guest_machine_check_addr = rtas_ld(args, 1);
+
+    /*
+     * Read the trampoline instructions from RTAS Blob and patch
+     * the KVMPPC_H_REPORT_MC_ERR hcall number and the guest
+     * machine check address before copying to 0x200 vector
+     */
+    cpu_physical_memory_read(spapr->rtas_addr + RTAS_TRAMPOLINE_OFFSET,
+                             trampoline, sizeof(trampoline));
+
+    /* Safety Check */
+    QEMU_BUILD_BUG_ON(sizeof(trampoline) > MC_INTERRUPT_VECTOR_SIZE);
+
+    /* Update the KVMPPC_H_REPORT_MC_ERR value in trampoline */

hw/ppc/spapr_rtas.c static void rtas_ibm_os_term(PowerPCCPU *cpu, · line 327

Quoted 3 lines

hw/ppc/spapr_rtas.c +3 -0

+    ori_inst |= KVMPPC_H_REPORT_MC_ERR;
+    memcpy(&trampoline[TRAMPOLINE_ORI_INST_INDEX], &ori_inst,
+            sizeof(ori_inst));

Given that we already code the KVMPPC_H_RTAS value directly into the .S file, I don't think it's worth the trouble of patching the H_REPORT_MC_ERR value. As Alex says, it has to stay the same for migration anyway.

Quoted 39 lines

hw/ppc/spapr_rtas.c +38 -0

+    /*
+     * Sanity check guest_machine_check_addr to prevent clobbering
+     * operator value in branch instruction
+     */
+    if (guest_machine_check_addr & BRANCH_INST_MASK) {
+        fprintf(stderr, "Unable to register ibm,nmi_register: "
+                "Invalid machine check handler address\n");
+        rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);
+        return;
+    }
+
+    /*
+     * Update the branch instruction in trampoline
+     * with the absolute machine check address requested by OS.
+     */
+    branch_inst |= guest_machine_check_addr;
+    memcpy(&trampoline[TRAMPOLINE_BR_INST_INDEX], &branch_inst,
+            sizeof(branch_inst));
+
+    /* Handle all Host/Guest LE/BE combinations */
+    if ((*pcc->interrupts_big_endian)(cpu)) {
+        for (i = 0; i < total_inst; i++) {
+            trampoline[i] = cpu_to_be32(trampoline[i]);
+        }
+    } else {
+        for (i = 0; i < total_inst; i++) {
+            trampoline[i] = cpu_to_le32(trampoline[i]);
+        }
+    }
+
+    /* Patch 0x200 NMI interrupt vector memory area of guest */
+    cpu_physical_memory_write(MC_INTERRUPT_VECTOR, trampoline,
+                              sizeof(trampoline));
+
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+}
+
+static void rtas_ibm_nmi_interlock(PowerPCCPU *cpu,

hw/ppc/spapr_rtas.c static void rtas_ibm_os_term(PowerPCCPU *cpu, · line 301

Quoted 5 lines

hw/ppc/spapr_rtas.c +5 -0

+                                   sPAPREnvironment *spapr,
+                                   uint32_t token, uint32_t nargs,
+                                   target_ulong args,
+                                   uint32_t nret, target_ulong rets)
+{

Again you should sanity check the arguments - at least check nargs and nrets.

Quoted 87 lines

hw/ppc/spapr_rtas.c +14 -0

+    /*
+     * VCPU issuing ibm,nmi-interlock is done with NMI handling,
+     * hence unset mc_in_progress.
+     */
+    mc_in_progress = 0;
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+}
+
 static struct rtas_call {
     const char *name;
     spapr_rtas_fn fn;

@@ -419,6 +506,12 @@ static void core_rtas_register_types(void)
                         rtas_ibm_set_system_parameter);
     spapr_rtas_register(RTAS_IBM_OS_TERM, "ibm,os-term",
                         rtas_ibm_os_term);
+    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 +16 -1

index a2d67e9..98d0a6c 100644
@@ -384,8 +384,10 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi);
 #define RTAS_GET_SENSOR_STATE                   (RTAS_TOKEN_BASE + 0x1D)
 #define RTAS_IBM_CONFIGURE_CONNECTOR            (RTAS_TOKEN_BASE + 0x1E)
 #define RTAS_IBM_OS_TERM                        (RTAS_TOKEN_BASE + 0x1F)
+#define RTAS_IBM_NMI_REGISTER                   (RTAS_TOKEN_BASE + 0x20)
+#define RTAS_IBM_NMI_INTERLOCK                  (RTAS_TOKEN_BASE + 0x21)
 
-#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x20)
+#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x22)
 
 /* RTAS ibm,get-system-parameter token values */
 #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS      20

@@ -488,4 +490,17 @@ int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname,
 #define RTAS_TRAMPOLINE_OFFSET   0x200
 #define RTAS_ERRLOG_OFFSET       0x800
 
+/* Machine Check Trampoline related macros
+ *
+ * These macros should co-relate to the code we
+ * have in pc-bios/spapr-rtas/spapr-rtas.S
+ */
+#define TRAMPOLINE_INSTS           17
+#define TRAMPOLINE_ORI_INST_INDEX  2
+#define TRAMPOLINE_BR_INST_INDEX   15
+
+/* Machine Check Interrupt related macros */
+#define MC_INTERRUPT_VECTOR           0x200
+#define MC_INTERRUPT_VECTOR_SIZE      0x100
+
 #endif /* !defined (__HW_SPAPR_H__) */

pc-bios/spapr-rtas/spapr-rtas.S +20 -0

index 903bec2..c315332 100644
@@ -35,3 +35,41 @@ _start:
 	ori	3,3,KVMPPC_H_RTAS@l
 	sc	1
 	blr
+	. = 0x200
+	/*
+	 * Trampoline saves r3 in sprg2 and issues private hcall
+	 * to request qemu to build error log. QEMU builds the
+	 * error log, copies to rtas-blob and returns the address.
+	 * The initial 16 bytes in return adress consist of saved
+	 * srr0 and srr1 which we restore and pass on the actual error
+	 * log address to OS handled mcachine check notification
+	 * routine
+	 *
+	 * All the below instructions are copied to interrupt vector
+	 * 0x200 at the time of handling ibm,nmi-register rtas call.
+	 */
+	mtsprg  2,3
+	li      3,0
+	/*
+	 * ori r3,r3,KVMPPC_H_REPORT_MC_ERR. The KVMPPC_H_REPORT_MC_ERR
+	 * value is patched below
+	 */
+1:	ori     3,3,0

pc-bios/spapr-rtas/spapr-rtas.S _start: · line 60

Quoted 3 lines

pc-bios/spapr-rtas/spapr-rtas.S +3 -0

+	sc      1               /* Issue H_CALL */
+	cmpdi   cr0,3,0
+	beq     cr0,1b          /* retry KVMPPC_H_REPORT_MC_ERR */

Having to retry the hcall from here seems very awkward. This is a private hcall, so you can define it to do whatever retries are necessary internally (and I don't think your current implementation can fail anyway).

pc-bios/spapr-rtas/spapr-rtas.S _start: · line 61

Quoted 2 lines

pc-bios/spapr-rtas/spapr-rtas.S +1 -0

+	mtsprg  2,4

Um.. doesn't this clobber the value of r3 you saved in SPRG2 just above.

pc-bios/spapr-rtas/spapr-rtas.S _start: · line 67

Quoted 7 lines

pc-bios/spapr-rtas/spapr-rtas.S +6 -0

+	ld      4,0(3)
+	mtsrr0  4               /* Restore srr0 */
+	ld      4,8(3)
+	mtsrr1  4               /* Restore srr1 */
+	ld      4,16(3)
+	mtcrf   0,4             /* Restore cr */

mtcrf? aren't you restoring the whole CR?

pc-bios/spapr-rtas/spapr-rtas.S _start: · line 75

Quoted 9 lines

pc-bios/spapr-rtas/spapr-rtas.S +8 -0

+	addi    3,3,24
+	mfsprg  4,2
+	/*
+	 * Branch to address registered by OS. The branch address is
+	 * patched in the ibm,nmi-register rtas call.
+	 */
+	ba      0x0
+	b       .

The branch to self is pointless. Even if the instruction above is not patched, or patched incorrectly, it's a ba, so you're not likely to end up at the instruction underneath.

Actually, what would probably make more sense would be to just have a "b ." *instead* of the ba, and have the qemu patching replace it with the correct ba instruction. That will limit the damage if it somehow gets executed without being patched.

Quoted 1 line · David Gibson

On Wed, Nov 05, 2014 at 12:43:15PM +0530, Aravinda Prasad wrote:

Quoted 20 lines

Quoted 20 lines

pc-bios/spapr-rtas/spapr-rtas.S +20 -0

+	. = 0x200
+	/*
+	 * Trampoline saves r3 in sprg2 and issues private hcall
+	 * to request qemu to build error log. QEMU builds the
+	 * error log, copies to rtas-blob and returns the address.
+	 * The initial 16 bytes in return adress consist of saved
+	 * srr0 and srr1 which we restore and pass on the actual error
+	 * log address to OS handled mcachine check notification
+	 * routine
+	 *
+	 * All the below instructions are copied to interrupt vector
+	 * 0x200 at the time of handling ibm,nmi-register rtas call.
+	 */
+	mtsprg  2,3
+	li      3,0
+	/*
+	 * ori r3,r3,KVMPPC_H_REPORT_MC_ERR. The KVMPPC_H_REPORT_MC_ERR
+	 * value is patched below
+	 */
+1:	ori     3,3,0

Quoted 8 lines

pc-bios/spapr-rtas/spapr-rtas.S _start: · line 60

Quoted 3 lines

pc-bios/spapr-rtas/spapr-rtas.S +3 -0

+	sc      1               /* Issue H_CALL */
+	cmpdi   cr0,3,0
+	beq     cr0,1b          /* retry KVMPPC_H_REPORT_MC_ERR */

Having to retry the hcall from here seems very awkward. This is a private hcall, so you can define it to do whatever retries are necessary internally (and I don't think your current implementation can fail anyway).

Retrying is required in the cases when multi-processors experience machine check at or about the same time. As per PAPR, subsequent processors should serialize and wait for the first processor to issue the ibm,nmi-interlock call. The second processor retries if the first processor which received a machine check is still reading the error log and is yet to issue ibm,nmi-interlock call.

Retrying cannot be done internally in h_report_mc_err hcall: only one thread can succeed entering qemu upon parallel hcall and hence retrying inside the hcall will not allow the ibm,nmi-interlock from first CPU to succeed.

Quoted 5 lines

pc-bios/spapr-rtas/spapr-rtas.S _start: · line 61

Quoted 3 lines

pc-bios/spapr-rtas/spapr-rtas.S +1 -0

+	mtsprg  2,4

Um.. doesn't this clobber the value of r3 you saved in SPRG2 just above.

The r3 saved in SPRG2 is moved to rtas area in the private hcall and hence it is fine to clobber r3 here