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.

Quoted 3 lines · Aravinda Prasad

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

Hrm. Putting the reserved space into the actual firmware image file seems really clumsy to me. Wouldn't it be better to reserve the space directly from qemu, then just paste the 20 bytes of code into that area.

Quoted 5 lines · David Gibson

Quoted 3 lines · Aravinda Prasad

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

Quoted 4 lines

Hrm. Putting the reserved space into the actual firmware image file seems really clumsy to me. Wouldn't it be better to reserve the space directly from qemu, then just paste the 20 bytes of code into that area.

Hmm. It is possible to it from qemu. In that case we can get rid of spapr-rtas.S. Let me think about that.

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]>

Quoted 5 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]>

I can't say I'm a big fan of this patch. Can we somehow separate that NMI page from the RTAS blob? Also I'd definitely prefer if we keep rtas_entry == rtas_addr - if nothing else for the sake of backwards compatibility.

So how about we lay out the structure in memory like this:

[ spapr-rtas.bin ]
[ padding to 4k boundary or whatever sPAPR requires ] [ 4k NMI region ]

Then the only thing we'd have to really change internally is the size information of the rtas blob.

Alex

Quoted 20 lines · Alexander Graf

Quoted 5 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]>

I can't say I'm a big fan of this patch. Can we somehow separate that NMI page from the RTAS blob? Also I'd definitely prefer if we keep rtas_entry == rtas_addr - if nothing else for the sake of backwards compatibility.

So how about we lay out the structure in memory like this:

[ spapr-rtas.bin ]
[ padding to 4k boundary or whatever sPAPR requires ] [ 4k NMI region ]

Quoted 2 lines

Then the only thing we'd have to really change internally is the size information of the rtas blob.

Either we can have it like this or completely eliminate spapr-rtas.bin (and spapr-rtas.S) by simply allocating required space in QEMU and then patching the 5 instructions at rtas-entry as earlier discussed with David.

http://lists.nongnu.org/archive/html/qemu-ppc/2014-08/msg00251.html

I strongly disagree with David. Legally there is no difference between a .bin file that contains code and an array made of instructions. And the more target code we can keep outside of QEMU the better.

Quoted 3 lines · Alexander Graf

I strongly disagree with David. Legally there is no difference between a .bin file that contains code and an array made of instructions. And the more target code we can keep outside of QEMU the better.

Ugh ... having a separate file with it's own build system etc... for 5 instructions is borderline ridiculous too :-)

Quoted 7 lines · Benjamin Herrenschmidt

Quoted 3 lines · Alexander Graf

I strongly disagree with David. Legally there is no difference between a .bin file that contains code and an array made of instructions. And the more target code we can keep outside of QEMU the better.

Ugh ... having a separate file with it's own build system etc... for 5 instructions is borderline ridiculous too :-)

Then don't use it - nobody's forcing you to do so :). Just use the provided binary. I fail to see the difference between

  uint32_t rtas_blob[] = { INST_SC1, INST_BLR };

and

  uint32_t *rtas_blob;
  load_file_from_disk(rtas_blob);

except that we're using an actual assembler ;).

Am 29.08.2014 um 03:06 schrieb Benjamin Herrenschmidt <[email protected]>:

Quoted 18 lines · Alexander Graf

Quoted 9 lines · Benjamin Herrenschmidt

Quoted 4 lines · Alexander Graf

I strongly disagree with David. Legally there is no difference between a .bin file that contains code and an array made of instructions. And the more target code we can keep outside of QEMU the better.

Ugh ... having a separate file with it's own build system etc... for 5 instructions is borderline ridiculous too :-)

Then don't use it - nobody's forcing you to do so :). Just use the provided binary. I fail to see the difference between

uint32_t rtas_blob[] = { INST_SC1, INST_BLR };

and

Quoted 4 lines

uint32_t *rtas_blob;
load_file_from_disk(rtas_blob);

except that we're using an actual assembler ;).

So you fail to see the difference with an array of 5 words vs. running through thousands of instructions & syscall to read those same 5 words from disk ? :-)

I fail to see a problem, yeah :). Imagine the same thing on x86 with its completely messed up instruction set. Would you still advocate for in-qemu code or would you prefer to have a compiler between you and the ugly opcodes?

Quoted 27 lines

Am 29.08.2014 um 03:06 schrieb Benjamin Herrenschmidt <[email protected]>:

Quoted 22 lines · Alexander Graf

Quoted 9 lines · Benjamin Herrenschmidt

Quoted 4 lines · Alexander Graf

I strongly disagree with David. Legally there is no difference between a .bin file that contains code and an array made of instructions. And the more target code we can keep outside of QEMU the better.

Ugh ... having a separate file with it's own build system etc... for 5 instructions is borderline ridiculous too :-)

Then don't use it - nobody's forcing you to do so :). Just use the provided binary. I fail to see the difference between

uint32_t rtas_blob[] = { INST_SC1, INST_BLR };

and

uint32_t *rtas_blob;
load_file_from_disk(rtas_blob);

except that we're using an actual assembler ;).

Quoted 3 lines

So you fail to see the difference with an array of 5 words vs. running through thousands of instructions & syscall to read those same 5 words from disk ? :-)

I fail to see a problem, yeah :). Imagine the same thing on x86 with its completely messed up instruction set. Would you still advocate for in-qemu code or would you prefer to have a compiler between you and the ugly opcodes?

Quoted 30 lines · Aravinda Prasad

Quoted 14 lines · Alexander Graf

Quoted 5 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]>

I can't say I'm a big fan of this patch. Can we somehow separate that NMI page from the RTAS blob? Also I'd definitely prefer if we keep rtas_entry == rtas_addr - if nothing else for the sake of backwards compatibility.

Quoted 8 lines

So how about we lay out the structure in memory like this:

[ spapr-rtas.bin ]
[ padding to 4k boundary or whatever sPAPR requires ] [ 4k NMI region ]

Then the only thing we'd have to really change internally is the size information of the rtas blob.

Either we can have it like this or completely eliminate spapr-rtas.bin (and spapr-rtas.S) by simply allocating required space in QEMU and then patching the 5 instructions at rtas-entry as earlier discussed with David.

Quoted 2 lines

http://lists.nongnu.org/archive/html/qemu-ppc/2014-08/msg00251.html

I strongly disagree with David. Legally there is no difference between a .bin file that contains code and an array made of instructions. And the more target code we can keep outside of QEMU the better.

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                  |   13 ++++++++++---
 hw/ppc/spapr_rtas.c             |    4 ++--
 include/hw/ppc/spapr.h          |    2 +-
 pc-bios/spapr-rtas/spapr-rtas.S |   12 ++++++++++++
 4 files changed, 25 insertions(+), 6 deletions(-)

hw/ppc/spapr.c +10 -3

index d01978f..1120988 100644
@@ -85,6 +85,12 @@
 
 #define HTAB_SIZE(spapr)        (1ULL << ((spapr)->htab_shift))
 
+/*
+ * The rtas-entry-offset should match the value specified in
+ * spapr-rtas.S
+ */
+#define RTAS_ENTRY_OFFSET       0x1000
+
 typedef struct sPAPRMachineState sPAPRMachineState;
 
 #define TYPE_SPAPR_MACHINE      "spapr-machine"

@@ -670,7 +676,8 @@ static int spapr_populate_memory(sPAPREnvironment *spapr, void *fdt)
 static void spapr_finalize_fdt(sPAPREnvironment *spapr,
                                hwaddr fdt_addr,
                                hwaddr rtas_addr,
-                               hwaddr rtas_size)
+                               hwaddr rtas_size,
+                               hwaddr rtas_entry)
 {
     int ret, i;
     size_t cb = 0;

@@ -705,7 +712,7 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
     }
 
     /* RTAS */
-    ret = spapr_rtas_device_tree_setup(fdt, rtas_addr, rtas_size);
+    ret = spapr_rtas_device_tree_setup(fdt, rtas_addr, rtas_size, rtas_entry);
     if (ret < 0) {
         fprintf(stderr, "Couldn't set up RTAS device tree properties\n");
     }

@@ -808,7 +815,7 @@ static void ppc_spapr_reset(void)
 
     /* Load the fdt */
     spapr_finalize_fdt(spapr, spapr->fdt_addr, spapr->rtas_addr,
-                       spapr->rtas_size);
+                       spapr->rtas_size, spapr->rtas_addr + RTAS_ENTRY_OFFSET);
 
     /* Set up the entry state */
     first_ppc_cpu = POWERPC_CPU(first_cpu);

hw/ppc/spapr_rtas.c +2 -2

index 9ba1ba6..02ddbf9 100644
@@ -328,7 +328,7 @@ void spapr_rtas_register(int token, const char *name, spapr_rtas_fn fn)
 }
 
 int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
-                                 hwaddr rtas_size)
+                                 hwaddr rtas_size, hwaddr rtas_entry)
 {
     int ret;
     int i;

@@ -349,7 +349,7 @@ int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
     }
 
     ret = qemu_fdt_setprop_cell(fdt, "/rtas", "linux,rtas-entry",
-                                rtas_addr);
+                                rtas_entry);
     if (ret < 0) {
         fprintf(stderr, "Couldn't add linux,rtas-entry property: %s\n",
                 fdt_strerror(ret));

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

index bbba51a..dedfa67 100644
@@ -436,7 +436,7 @@ target_ulong spapr_rtas_call(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                              uint32_t token, uint32_t nargs, target_ulong args,
                              uint32_t nret, target_ulong rets);
 int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
-                                 hwaddr rtas_size);
+                                 hwaddr rtas_size, hwaddr rtas_entry);
 
 #define SPAPR_TCE_PAGE_SHIFT   12
 #define SPAPR_TCE_PAGE_SIZE    (1ULL << SPAPR_TCE_PAGE_SHIFT)

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

index 903bec2..8c9b17e 100644
@@ -30,6 +30,18 @@
 
 .globl	_start
 _start:
+	/*
+	 * Reserve space for error log in RTAS blob.

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

 	mr	4,3
 	lis	3,KVMPPC_H_RTAS@h
 	ori	3,3,KVMPPC_H_RTAS@l

Quoted 102 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                  |   13 ++++++++++---
 hw/ppc/spapr_rtas.c             |    4 ++--
 include/hw/ppc/spapr.h          |    2 +-
 pc-bios/spapr-rtas/spapr-rtas.S |   12 ++++++++++++
 4 files changed, 25 insertions(+), 6 deletions(-)

hw/ppc/spapr.c +10 -3

index d01978f..1120988 100644
@@ -85,6 +85,12 @@
 
 #define HTAB_SIZE(spapr)        (1ULL << ((spapr)->htab_shift))
 
+/*
+ * The rtas-entry-offset should match the value specified in
+ * spapr-rtas.S
+ */
+#define RTAS_ENTRY_OFFSET       0x1000
+
 typedef struct sPAPRMachineState sPAPRMachineState;
 
 #define TYPE_SPAPR_MACHINE      "spapr-machine"

@@ -670,7 +676,8 @@ static int spapr_populate_memory(sPAPREnvironment *spapr, void *fdt)
 static void spapr_finalize_fdt(sPAPREnvironment *spapr,
                                hwaddr fdt_addr,
                                hwaddr rtas_addr,
-                               hwaddr rtas_size)
+                               hwaddr rtas_size,
+                               hwaddr rtas_entry)
 {
     int ret, i;
     size_t cb = 0;

@@ -705,7 +712,7 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
     }
 
     /* RTAS */
-    ret = spapr_rtas_device_tree_setup(fdt, rtas_addr, rtas_size);
+    ret = spapr_rtas_device_tree_setup(fdt, rtas_addr, rtas_size, rtas_entry);
     if (ret < 0) {
         fprintf(stderr, "Couldn't set up RTAS device tree properties\n");
     }

@@ -808,7 +815,7 @@ static void ppc_spapr_reset(void)
 
     /* Load the fdt */
     spapr_finalize_fdt(spapr, spapr->fdt_addr, spapr->rtas_addr,
-                       spapr->rtas_size);
+                       spapr->rtas_size, spapr->rtas_addr + RTAS_ENTRY_OFFSET);
 
     /* Set up the entry state */
     first_ppc_cpu = POWERPC_CPU(first_cpu);

hw/ppc/spapr_rtas.c +2 -2

index 9ba1ba6..02ddbf9 100644
@@ -328,7 +328,7 @@ void spapr_rtas_register(int token, const char *name, spapr_rtas_fn fn)
 }
 
 int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
-                                 hwaddr rtas_size)
+                                 hwaddr rtas_size, hwaddr rtas_entry)
 {
     int ret;
     int i;

@@ -349,7 +349,7 @@ int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
     }
 
     ret = qemu_fdt_setprop_cell(fdt, "/rtas", "linux,rtas-entry",
-                                rtas_addr);
+                                rtas_entry);
     if (ret < 0) {
         fprintf(stderr, "Couldn't add linux,rtas-entry property: %s\n",
                 fdt_strerror(ret));

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

index bbba51a..dedfa67 100644
@@ -436,7 +436,7 @@ target_ulong spapr_rtas_call(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                              uint32_t token, uint32_t nargs, target_ulong args,
                              uint32_t nret, target_ulong rets);
 int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
-                                 hwaddr rtas_size);
+                                 hwaddr rtas_size, hwaddr rtas_entry);
 
 #define SPAPR_TCE_PAGE_SHIFT   12
 #define SPAPR_TCE_PAGE_SIZE    (1ULL << SPAPR_TCE_PAGE_SHIFT)

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

index 903bec2..8c9b17e 100644
@@ -30,6 +30,18 @@
 
 .globl	_start
 _start:
+	/*
+	 * Reserve space for error log in RTAS blob.

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

Quoted 10 lines

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

+	 *
+	 * Either we can reserve initial bytes for error log followed by
+	 * rtas-entry or space can be reserved after rtas-entry. I prefer
+	 * former, as we already have rtas-base and rtas-entry (currently
+	 * both pointing to rtas-base) defined in qemu and we can update
+	 * rtas-entry to point to an offset from rtas-base. This avoids
+	 * unnecessary definition of rtas-error-offset while keeping
+	 * rtas-entry redundant.
+	 */
+	. = 0x1000

Why not this (and not changing spapr-rtas.S)?

Quoted 8 lines

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

 	mr	4,3
 	lis	3,KVMPPC_H_RTAS@h
 	ori	3,3,KVMPPC_H_RTAS@l

Quoted 122 lines · Alexey Kardashevskiy

Quoted 102 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                  |   13 ++++++++++---
 hw/ppc/spapr_rtas.c             |    4 ++--
 include/hw/ppc/spapr.h          |    2 +-
 pc-bios/spapr-rtas/spapr-rtas.S |   12 ++++++++++++
 4 files changed, 25 insertions(+), 6 deletions(-)

hw/ppc/spapr.c +10 -3

index d01978f..1120988 100644
@@ -85,6 +85,12 @@
 
 #define HTAB_SIZE(spapr)        (1ULL << ((spapr)->htab_shift))
 
+/*
+ * The rtas-entry-offset should match the value specified in
+ * spapr-rtas.S
+ */
+#define RTAS_ENTRY_OFFSET       0x1000
+
 typedef struct sPAPRMachineState sPAPRMachineState;
 
 #define TYPE_SPAPR_MACHINE      "spapr-machine"

@@ -670,7 +676,8 @@ static int spapr_populate_memory(sPAPREnvironment *spapr, void *fdt)
 static void spapr_finalize_fdt(sPAPREnvironment *spapr,
                                hwaddr fdt_addr,
                                hwaddr rtas_addr,
-                               hwaddr rtas_size)
+                               hwaddr rtas_size,
+                               hwaddr rtas_entry)
 {
     int ret, i;
     size_t cb = 0;

@@ -705,7 +712,7 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
     }
 
     /* RTAS */
-    ret = spapr_rtas_device_tree_setup(fdt, rtas_addr, rtas_size);
+    ret = spapr_rtas_device_tree_setup(fdt, rtas_addr, rtas_size, rtas_entry);
     if (ret < 0) {
         fprintf(stderr, "Couldn't set up RTAS device tree properties\n");
     }

@@ -808,7 +815,7 @@ static void ppc_spapr_reset(void)
 
     /* Load the fdt */
     spapr_finalize_fdt(spapr, spapr->fdt_addr, spapr->rtas_addr,
-                       spapr->rtas_size);
+                       spapr->rtas_size, spapr->rtas_addr + RTAS_ENTRY_OFFSET);
 
     /* Set up the entry state */
     first_ppc_cpu = POWERPC_CPU(first_cpu);

hw/ppc/spapr_rtas.c +2 -2

index 9ba1ba6..02ddbf9 100644
@@ -328,7 +328,7 @@ void spapr_rtas_register(int token, const char *name, spapr_rtas_fn fn)
 }
 
 int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
-                                 hwaddr rtas_size)
+                                 hwaddr rtas_size, hwaddr rtas_entry)
 {
     int ret;
     int i;

@@ -349,7 +349,7 @@ int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
     }
 
     ret = qemu_fdt_setprop_cell(fdt, "/rtas", "linux,rtas-entry",
-                                rtas_addr);
+                                rtas_entry);
     if (ret < 0) {
         fprintf(stderr, "Couldn't add linux,rtas-entry property: %s\n",
                 fdt_strerror(ret));

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

index bbba51a..dedfa67 100644
@@ -436,7 +436,7 @@ target_ulong spapr_rtas_call(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                              uint32_t token, uint32_t nargs, target_ulong args,
                              uint32_t nret, target_ulong rets);
 int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
-                                 hwaddr rtas_size);
+                                 hwaddr rtas_size, hwaddr rtas_entry);
 
 #define SPAPR_TCE_PAGE_SHIFT   12
 #define SPAPR_TCE_PAGE_SIZE    (1ULL << SPAPR_TCE_PAGE_SHIFT)

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

index 903bec2..8c9b17e 100644
@@ -30,6 +30,18 @@
 
 .globl	_start
 _start:
+	/*
+	 * Reserve space for error log in RTAS blob.

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

Quoted 10 lines

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

+	 *
+	 * Either we can reserve initial bytes for error log followed by
+	 * rtas-entry or space can be reserved after rtas-entry. I prefer
+	 * former, as we already have rtas-base and rtas-entry (currently
+	 * both pointing to rtas-base) defined in qemu and we can update
+	 * rtas-entry to point to an offset from rtas-base. This avoids
+	 * unnecessary definition of rtas-error-offset while keeping
+	 * rtas-entry redundant.
+	 */
+	. = 0x1000

Why not this (and not changing spapr-rtas.S)?

hw/ppc/spapr.c +0 -0

@@ -875,7 +875,8 @@ static void ppc_spapr_reset(void)
                        spapr->rtas_size);
 

hw/ppc/spapr.c static void ppc_spapr_reset(void) · line 880

Quoted 5 lines

hw/ppc/spapr.c +2 -1

     /* Copy RTAS over */
-    cpu_physical_memory_write(spapr->rtas_addr, spapr->rtas_blob,
+    cpu_physical_memory_write(spapr->rtas_addr + RTAS_ENTRY_OFFSET,
+                              spapr->rtas_blob,
                               spapr->rtas_size);

This is possible, however requires suitable adjustment to make sure spapr->rtas_addr has enough space allocated.

Quoted 11 lines

Quoted 9 lines

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

 	mr	4,3
 	lis	3,KVMPPC_H_RTAS@h
 	ori	3,3,KVMPPC_H_RTAS@l

Quoted 125 lines · Aravinda Prasad

Quoted 122 lines · Alexey Kardashevskiy

Quoted 102 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                  |   13 ++++++++++---
 hw/ppc/spapr_rtas.c             |    4 ++--
 include/hw/ppc/spapr.h          |    2 +-
 pc-bios/spapr-rtas/spapr-rtas.S |   12 ++++++++++++
 4 files changed, 25 insertions(+), 6 deletions(-)

hw/ppc/spapr.c +10 -3

index d01978f..1120988 100644
@@ -85,6 +85,12 @@
 
 #define HTAB_SIZE(spapr)        (1ULL << ((spapr)->htab_shift))
 
+/*
+ * The rtas-entry-offset should match the value specified in
+ * spapr-rtas.S
+ */
+#define RTAS_ENTRY_OFFSET       0x1000
+
 typedef struct sPAPRMachineState sPAPRMachineState;
 
 #define TYPE_SPAPR_MACHINE      "spapr-machine"

@@ -670,7 +676,8 @@ static int spapr_populate_memory(sPAPREnvironment *spapr, void *fdt)
 static void spapr_finalize_fdt(sPAPREnvironment *spapr,
                                hwaddr fdt_addr,
                                hwaddr rtas_addr,
-                               hwaddr rtas_size)
+                               hwaddr rtas_size,
+                               hwaddr rtas_entry)
 {
     int ret, i;
     size_t cb = 0;

@@ -705,7 +712,7 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
     }
 
     /* RTAS */
-    ret = spapr_rtas_device_tree_setup(fdt, rtas_addr, rtas_size);
+    ret = spapr_rtas_device_tree_setup(fdt, rtas_addr, rtas_size, rtas_entry);
     if (ret < 0) {
         fprintf(stderr, "Couldn't set up RTAS device tree properties\n");
     }

@@ -808,7 +815,7 @@ static void ppc_spapr_reset(void)
 
     /* Load the fdt */
     spapr_finalize_fdt(spapr, spapr->fdt_addr, spapr->rtas_addr,
-                       spapr->rtas_size);
+                       spapr->rtas_size, spapr->rtas_addr + RTAS_ENTRY_OFFSET);
 
     /* Set up the entry state */
     first_ppc_cpu = POWERPC_CPU(first_cpu);

hw/ppc/spapr_rtas.c +2 -2

index 9ba1ba6..02ddbf9 100644
@@ -328,7 +328,7 @@ void spapr_rtas_register(int token, const char *name, spapr_rtas_fn fn)
 }
 
 int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
-                                 hwaddr rtas_size)
+                                 hwaddr rtas_size, hwaddr rtas_entry)
 {
     int ret;
     int i;

@@ -349,7 +349,7 @@ int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
     }
 
     ret = qemu_fdt_setprop_cell(fdt, "/rtas", "linux,rtas-entry",
-                                rtas_addr);
+                                rtas_entry);
     if (ret < 0) {
         fprintf(stderr, "Couldn't add linux,rtas-entry property: %s\n",
                 fdt_strerror(ret));

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

index bbba51a..dedfa67 100644
@@ -436,7 +436,7 @@ target_ulong spapr_rtas_call(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                              uint32_t token, uint32_t nargs, target_ulong args,
                              uint32_t nret, target_ulong rets);
 int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
-                                 hwaddr rtas_size);
+                                 hwaddr rtas_size, hwaddr rtas_entry);
 
 #define SPAPR_TCE_PAGE_SHIFT   12
 #define SPAPR_TCE_PAGE_SIZE    (1ULL << SPAPR_TCE_PAGE_SHIFT)

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

index 903bec2..8c9b17e 100644
@@ -30,6 +30,18 @@
 
 .globl	_start
 _start:
+	/*
+	 * Reserve space for error log in RTAS blob.

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

Quoted 10 lines

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

+	 *
+	 * Either we can reserve initial bytes for error log followed by
+	 * rtas-entry or space can be reserved after rtas-entry. I prefer
+	 * former, as we already have rtas-base and rtas-entry (currently
+	 * both pointing to rtas-base) defined in qemu and we can update
+	 * rtas-entry to point to an offset from rtas-base. This avoids
+	 * unnecessary definition of rtas-error-offset while keeping
+	 * rtas-entry redundant.
+	 */
+	. = 0x1000

Why not this (and not changing spapr-rtas.S)?

hw/ppc/spapr.c +0 -0

@@ -875,7 +875,8 @@ static void ppc_spapr_reset(void)
                        spapr->rtas_size);
 

Quoted 8 lines

hw/ppc/spapr.c static void ppc_spapr_reset(void) · line 880

Quoted 5 lines

hw/ppc/spapr.c +2 -1

     /* Copy RTAS over */
-    cpu_physical_memory_write(spapr->rtas_addr, spapr->rtas_blob,
+    cpu_physical_memory_write(spapr->rtas_addr + RTAS_ENTRY_OFFSET,
+                              spapr->rtas_blob,
                               spapr->rtas_size);

This is possible, however requires suitable adjustment to make sure spapr->rtas_addr has enough space allocated.

How is adding RTAS_ENTRY_OFFSET not enough to make sure that is has enough space? QEMU copies RTAS to guest memory, QEMU makes up rtas_addr/entry properties.

Quoted 14 lines

Quoted 13 lines

Quoted 11 lines

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

 	mr	4,3
 	lis	3,KVMPPC_H_RTAS@h
 	ori	3,3,KVMPPC_H_RTAS@l

Quoted 136 lines · Alexey Kardashevskiy

Quoted 125 lines · Aravinda Prasad

Quoted 122 lines · Alexey Kardashevskiy

Quoted 102 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                  |   13 ++++++++++---
 hw/ppc/spapr_rtas.c             |    4 ++--
 include/hw/ppc/spapr.h          |    2 +-
 pc-bios/spapr-rtas/spapr-rtas.S |   12 ++++++++++++
 4 files changed, 25 insertions(+), 6 deletions(-)

hw/ppc/spapr.c +10 -3

index d01978f..1120988 100644
@@ -85,6 +85,12 @@
 
 #define HTAB_SIZE(spapr)        (1ULL << ((spapr)->htab_shift))
 
+/*
+ * The rtas-entry-offset should match the value specified in
+ * spapr-rtas.S
+ */
+#define RTAS_ENTRY_OFFSET       0x1000
+
 typedef struct sPAPRMachineState sPAPRMachineState;
 
 #define TYPE_SPAPR_MACHINE      "spapr-machine"

@@ -670,7 +676,8 @@ static int spapr_populate_memory(sPAPREnvironment *spapr, void *fdt)
 static void spapr_finalize_fdt(sPAPREnvironment *spapr,
                                hwaddr fdt_addr,
                                hwaddr rtas_addr,
-                               hwaddr rtas_size)
+                               hwaddr rtas_size,
+                               hwaddr rtas_entry)
 {
     int ret, i;
     size_t cb = 0;

@@ -705,7 +712,7 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
     }
 
     /* RTAS */
-    ret = spapr_rtas_device_tree_setup(fdt, rtas_addr, rtas_size);
+    ret = spapr_rtas_device_tree_setup(fdt, rtas_addr, rtas_size, rtas_entry);
     if (ret < 0) {
         fprintf(stderr, "Couldn't set up RTAS device tree properties\n");
     }

@@ -808,7 +815,7 @@ static void ppc_spapr_reset(void)
 
     /* Load the fdt */
     spapr_finalize_fdt(spapr, spapr->fdt_addr, spapr->rtas_addr,
-                       spapr->rtas_size);
+                       spapr->rtas_size, spapr->rtas_addr + RTAS_ENTRY_OFFSET);
 
     /* Set up the entry state */
     first_ppc_cpu = POWERPC_CPU(first_cpu);

hw/ppc/spapr_rtas.c +2 -2

index 9ba1ba6..02ddbf9 100644
@@ -328,7 +328,7 @@ void spapr_rtas_register(int token, const char *name, spapr_rtas_fn fn)
 }
 
 int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
-                                 hwaddr rtas_size)
+                                 hwaddr rtas_size, hwaddr rtas_entry)
 {
     int ret;
     int i;

@@ -349,7 +349,7 @@ int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
     }
 
     ret = qemu_fdt_setprop_cell(fdt, "/rtas", "linux,rtas-entry",
-                                rtas_addr);
+                                rtas_entry);
     if (ret < 0) {
         fprintf(stderr, "Couldn't add linux,rtas-entry property: %s\n",
                 fdt_strerror(ret));

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

index bbba51a..dedfa67 100644
@@ -436,7 +436,7 @@ target_ulong spapr_rtas_call(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                              uint32_t token, uint32_t nargs, target_ulong args,
                              uint32_t nret, target_ulong rets);
 int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
-                                 hwaddr rtas_size);
+                                 hwaddr rtas_size, hwaddr rtas_entry);
 
 #define SPAPR_TCE_PAGE_SHIFT   12
 #define SPAPR_TCE_PAGE_SIZE    (1ULL << SPAPR_TCE_PAGE_SHIFT)

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

index 903bec2..8c9b17e 100644
@@ -30,6 +30,18 @@
 
 .globl	_start
 _start:
+	/*
+	 * Reserve space for error log in RTAS blob.

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

Quoted 10 lines

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

+	 *
+	 * Either we can reserve initial bytes for error log followed by
+	 * rtas-entry or space can be reserved after rtas-entry. I prefer
+	 * former, as we already have rtas-base and rtas-entry (currently
+	 * both pointing to rtas-base) defined in qemu and we can update
+	 * rtas-entry to point to an offset from rtas-base. This avoids
+	 * unnecessary definition of rtas-error-offset while keeping
+	 * rtas-entry redundant.
+	 */
+	. = 0x1000

Why not this (and not changing spapr-rtas.S)?

hw/ppc/spapr.c +0 -0

@@ -875,7 +875,8 @@ static void ppc_spapr_reset(void)
                        spapr->rtas_size);
 

Quoted 8 lines

hw/ppc/spapr.c static void ppc_spapr_reset(void) · line 880

Quoted 5 lines

hw/ppc/spapr.c +2 -1

     /* Copy RTAS over */
-    cpu_physical_memory_write(spapr->rtas_addr, spapr->rtas_blob,
+    cpu_physical_memory_write(spapr->rtas_addr + RTAS_ENTRY_OFFSET,
+                              spapr->rtas_blob,
                               spapr->rtas_size);

This is possible, however requires suitable adjustment to make sure spapr->rtas_addr has enough space allocated.

Quoted 3 lines

How is adding RTAS_ENTRY_OFFSET not enough to make sure that is has enough space? QEMU copies RTAS to guest memory, QEMU makes up rtas_addr/entry properties.

QEMU adds spapr-rtas.bin as a rom, with rom->addr set to spapr->rtas_addr, rom->datasize set to 20 bytes (the size of current spapr-rtas.bin) and contents of spapr-rtas.bin read into rom->data (malloc-ed region).

I think, access to spapr->rtas_addr is mapped to this rom. Hence it is necessary to have rtas_addr and rtas_size consistent with the Rom struct. If we use spapr->rtas_addr + RTAS_ENTRY_OFFSET then we are trying to access an invalid offset in rom region.

Quoted 18 lines

Quoted 16 lines

Quoted 15 lines

Quoted 13 lines

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

 	mr	4,3
 	lis	3,KVMPPC_H_RTAS@h
 	ori	3,3,KVMPPC_H_RTAS@l

Quoted 148 lines · Aravinda Prasad

Quoted 136 lines · Alexey Kardashevskiy

Quoted 125 lines · Aravinda Prasad

Quoted 122 lines · Alexey Kardashevskiy

Quoted 102 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                  |   13 ++++++++++---
 hw/ppc/spapr_rtas.c             |    4 ++--
 include/hw/ppc/spapr.h          |    2 +-
 pc-bios/spapr-rtas/spapr-rtas.S |   12 ++++++++++++
 4 files changed, 25 insertions(+), 6 deletions(-)

hw/ppc/spapr.c +10 -3

index d01978f..1120988 100644
@@ -85,6 +85,12 @@
 
 #define HTAB_SIZE(spapr)        (1ULL << ((spapr)->htab_shift))
 
+/*
+ * The rtas-entry-offset should match the value specified in
+ * spapr-rtas.S
+ */
+#define RTAS_ENTRY_OFFSET       0x1000
+
 typedef struct sPAPRMachineState sPAPRMachineState;
 
 #define TYPE_SPAPR_MACHINE      "spapr-machine"

@@ -670,7 +676,8 @@ static int spapr_populate_memory(sPAPREnvironment *spapr, void *fdt)
 static void spapr_finalize_fdt(sPAPREnvironment *spapr,
                                hwaddr fdt_addr,
                                hwaddr rtas_addr,
-                               hwaddr rtas_size)
+                               hwaddr rtas_size,
+                               hwaddr rtas_entry)
 {
     int ret, i;
     size_t cb = 0;

@@ -705,7 +712,7 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
     }
 
     /* RTAS */
-    ret = spapr_rtas_device_tree_setup(fdt, rtas_addr, rtas_size);
+    ret = spapr_rtas_device_tree_setup(fdt, rtas_addr, rtas_size, rtas_entry);
     if (ret < 0) {
         fprintf(stderr, "Couldn't set up RTAS device tree properties\n");
     }

@@ -808,7 +815,7 @@ static void ppc_spapr_reset(void)
 
     /* Load the fdt */
     spapr_finalize_fdt(spapr, spapr->fdt_addr, spapr->rtas_addr,
-                       spapr->rtas_size);
+                       spapr->rtas_size, spapr->rtas_addr + RTAS_ENTRY_OFFSET);
 
     /* Set up the entry state */
     first_ppc_cpu = POWERPC_CPU(first_cpu);

hw/ppc/spapr_rtas.c +2 -2

index 9ba1ba6..02ddbf9 100644
@@ -328,7 +328,7 @@ void spapr_rtas_register(int token, const char *name, spapr_rtas_fn fn)
 }
 
 int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
-                                 hwaddr rtas_size)
+                                 hwaddr rtas_size, hwaddr rtas_entry)
 {
     int ret;
     int i;

@@ -349,7 +349,7 @@ int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
     }
 
     ret = qemu_fdt_setprop_cell(fdt, "/rtas", "linux,rtas-entry",
-                                rtas_addr);
+                                rtas_entry);
     if (ret < 0) {
         fprintf(stderr, "Couldn't add linux,rtas-entry property: %s\n",
                 fdt_strerror(ret));

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

index bbba51a..dedfa67 100644
@@ -436,7 +436,7 @@ target_ulong spapr_rtas_call(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                              uint32_t token, uint32_t nargs, target_ulong args,
                              uint32_t nret, target_ulong rets);
 int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
-                                 hwaddr rtas_size);
+                                 hwaddr rtas_size, hwaddr rtas_entry);
 
 #define SPAPR_TCE_PAGE_SHIFT   12
 #define SPAPR_TCE_PAGE_SIZE    (1ULL << SPAPR_TCE_PAGE_SHIFT)

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

index 903bec2..8c9b17e 100644
@@ -30,6 +30,18 @@
 
 .globl	_start
 _start:
+	/*
+	 * Reserve space for error log in RTAS blob.

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

Quoted 10 lines

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

+	 *
+	 * Either we can reserve initial bytes for error log followed by
+	 * rtas-entry or space can be reserved after rtas-entry. I prefer
+	 * former, as we already have rtas-base and rtas-entry (currently
+	 * both pointing to rtas-base) defined in qemu and we can update
+	 * rtas-entry to point to an offset from rtas-base. This avoids
+	 * unnecessary definition of rtas-error-offset while keeping
+	 * rtas-entry redundant.
+	 */
+	. = 0x1000

Why not this (and not changing spapr-rtas.S)?

hw/ppc/spapr.c +0 -0

@@ -875,7 +875,8 @@ static void ppc_spapr_reset(void)
                        spapr->rtas_size);
 

Quoted 8 lines

hw/ppc/spapr.c static void ppc_spapr_reset(void) · line 880

Quoted 5 lines

hw/ppc/spapr.c +2 -1

     /* Copy RTAS over */
-    cpu_physical_memory_write(spapr->rtas_addr, spapr->rtas_blob,
+    cpu_physical_memory_write(spapr->rtas_addr + RTAS_ENTRY_OFFSET,
+                              spapr->rtas_blob,
                               spapr->rtas_size);

This is possible, however requires suitable adjustment to make sure spapr->rtas_addr has enough space allocated.

Quoted 3 lines

How is adding RTAS_ENTRY_OFFSET not enough to make sure that is has enough space? QEMU copies RTAS to guest memory, QEMU makes up rtas_addr/entry properties.

QEMU adds spapr-rtas.bin as a rom, with rom->addr set to spapr->rtas_addr, rom->datasize set to 20 bytes (the size of current spapr-rtas.bin) and contents of spapr-rtas.bin read into rom->data (malloc-ed region).

Quoted 4 lines

I think, access to spapr->rtas_addr is mapped to this rom. Hence it is necessary to have rtas_addr and rtas_size consistent with the Rom struct. If we use spapr->rtas_addr + RTAS_ENTRY_OFFSET then we are trying to access an invalid offset in rom region.

What is that "rom" struct you are referring to? In upstream QEMU, I can only see:

ppc_spapr_init():
[...]
filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, "spapr-rtas.bin")

spapr->rtas_size = get_image_size(filename); spapr->rtas_blob = g_malloc(spapr->rtas_size);
if (load_image_size(filename, spapr->rtas_blob, spapr->rtas_size) < 0) { [...]

and then

ppc_spapr_reset():
[...]
spapr->rtas_addr = rtas_limit - RTAS_MAX_SIZE
[...]
cpu_physical_memory_write(spapr->rtas_addr, spapr->rtas_blob,

                          spapr->rtas_size);

[...]

Quoted 19 lines

Quoted 18 lines

Quoted 16 lines

Quoted 15 lines

Quoted 13 lines

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

 	mr	4,3
 	lis	3,KVMPPC_H_RTAS@h
 	ori	3,3,KVMPPC_H_RTAS@l

Quoted 158 lines · Alexey Kardashevskiy

Quoted 148 lines · Aravinda Prasad

Quoted 136 lines · Alexey Kardashevskiy

Quoted 125 lines · Aravinda Prasad

Quoted 122 lines · Alexey Kardashevskiy

Quoted 102 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                  |   13 ++++++++++---
 hw/ppc/spapr_rtas.c             |    4 ++--
 include/hw/ppc/spapr.h          |    2 +-
 pc-bios/spapr-rtas/spapr-rtas.S |   12 ++++++++++++
 4 files changed, 25 insertions(+), 6 deletions(-)

hw/ppc/spapr.c +10 -3

index d01978f..1120988 100644
@@ -85,6 +85,12 @@
 
 #define HTAB_SIZE(spapr)        (1ULL << ((spapr)->htab_shift))
 
+/*
+ * The rtas-entry-offset should match the value specified in
+ * spapr-rtas.S
+ */
+#define RTAS_ENTRY_OFFSET       0x1000
+
 typedef struct sPAPRMachineState sPAPRMachineState;
 
 #define TYPE_SPAPR_MACHINE      "spapr-machine"

@@ -670,7 +676,8 @@ static int spapr_populate_memory(sPAPREnvironment *spapr, void *fdt)
 static void spapr_finalize_fdt(sPAPREnvironment *spapr,
                                hwaddr fdt_addr,
                                hwaddr rtas_addr,
-                               hwaddr rtas_size)
+                               hwaddr rtas_size,
+                               hwaddr rtas_entry)
 {
     int ret, i;
     size_t cb = 0;

@@ -705,7 +712,7 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
     }
 
     /* RTAS */
-    ret = spapr_rtas_device_tree_setup(fdt, rtas_addr, rtas_size);
+    ret = spapr_rtas_device_tree_setup(fdt, rtas_addr, rtas_size, rtas_entry);
     if (ret < 0) {
         fprintf(stderr, "Couldn't set up RTAS device tree properties\n");
     }

@@ -808,7 +815,7 @@ static void ppc_spapr_reset(void)
 
     /* Load the fdt */
     spapr_finalize_fdt(spapr, spapr->fdt_addr, spapr->rtas_addr,
-                       spapr->rtas_size);
+                       spapr->rtas_size, spapr->rtas_addr + RTAS_ENTRY_OFFSET);
 
     /* Set up the entry state */
     first_ppc_cpu = POWERPC_CPU(first_cpu);

hw/ppc/spapr_rtas.c +2 -2

index 9ba1ba6..02ddbf9 100644
@@ -328,7 +328,7 @@ void spapr_rtas_register(int token, const char *name, spapr_rtas_fn fn)
 }
 
 int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
-                                 hwaddr rtas_size)
+                                 hwaddr rtas_size, hwaddr rtas_entry)
 {
     int ret;
     int i;

@@ -349,7 +349,7 @@ int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
     }
 
     ret = qemu_fdt_setprop_cell(fdt, "/rtas", "linux,rtas-entry",
-                                rtas_addr);
+                                rtas_entry);
     if (ret < 0) {
         fprintf(stderr, "Couldn't add linux,rtas-entry property: %s\n",
                 fdt_strerror(ret));

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

index bbba51a..dedfa67 100644
@@ -436,7 +436,7 @@ target_ulong spapr_rtas_call(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                              uint32_t token, uint32_t nargs, target_ulong args,
                              uint32_t nret, target_ulong rets);
 int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
-                                 hwaddr rtas_size);
+                                 hwaddr rtas_size, hwaddr rtas_entry);
 
 #define SPAPR_TCE_PAGE_SHIFT   12
 #define SPAPR_TCE_PAGE_SIZE    (1ULL << SPAPR_TCE_PAGE_SHIFT)

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

index 903bec2..8c9b17e 100644
@@ -30,6 +30,18 @@
 
 .globl	_start
 _start:
+	/*
+	 * Reserve space for error log in RTAS blob.

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

Quoted 10 lines

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

+	 *
+	 * Either we can reserve initial bytes for error log followed by
+	 * rtas-entry or space can be reserved after rtas-entry. I prefer
+	 * former, as we already have rtas-base and rtas-entry (currently
+	 * both pointing to rtas-base) defined in qemu and we can update
+	 * rtas-entry to point to an offset from rtas-base. This avoids
+	 * unnecessary definition of rtas-error-offset while keeping
+	 * rtas-entry redundant.
+	 */
+	. = 0x1000

Why not this (and not changing spapr-rtas.S)?

hw/ppc/spapr.c +0 -0

@@ -875,7 +875,8 @@ static void ppc_spapr_reset(void)
                        spapr->rtas_size);
 

Quoted 8 lines

hw/ppc/spapr.c static void ppc_spapr_reset(void) · line 880

Quoted 5 lines

hw/ppc/spapr.c +2 -1

     /* Copy RTAS over */
-    cpu_physical_memory_write(spapr->rtas_addr, spapr->rtas_blob,
+    cpu_physical_memory_write(spapr->rtas_addr + RTAS_ENTRY_OFFSET,
+                              spapr->rtas_blob,
                               spapr->rtas_size);

This is possible, however requires suitable adjustment to make sure spapr->rtas_addr has enough space allocated.

Quoted 3 lines

How is adding RTAS_ENTRY_OFFSET not enough to make sure that is has enough space? QEMU copies RTAS to guest memory, QEMU makes up rtas_addr/entry properties.

QEMU adds spapr-rtas.bin as a rom, with rom->addr set to spapr->rtas_addr, rom->datasize set to 20 bytes (the size of current spapr-rtas.bin) and contents of spapr-rtas.bin read into rom->data (malloc-ed region).

Quoted 4 lines

I think, access to spapr->rtas_addr is mapped to this rom. Hence it is necessary to have rtas_addr and rtas_size consistent with the Rom struct. If we use spapr->rtas_addr + RTAS_ENTRY_OFFSET then we are trying to access an invalid offset in rom region.

What is that "rom" struct you are referring to? In upstream QEMU, I can only see:

Quoted 3 lines

ppc_spapr_init():
[...]
filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, "spapr-rtas.bin")

In ppc_spapr_init() just after qemu_find_file() we have:

spapr->rtas_size = load_image_targphys(filename, spapr->rtas_addr, ...);

load_image_targphys() -> rom_add_file_fixed() -> rom_add_file(), where Rom is initialized.

Quoted 38 lines

spapr->rtas_size = get_image_size(filename); spapr->rtas_blob = g_malloc(spapr->rtas_size);
if (load_image_size(filename, spapr->rtas_blob, spapr->rtas_size) < 0) { [...]

and then

ppc_spapr_reset():
[...]
spapr->rtas_addr = rtas_limit - RTAS_MAX_SIZE
[...]
cpu_physical_memory_write(spapr->rtas_addr, spapr->rtas_blob,

                          spapr->rtas_size);

[...]

Quoted 19 lines

Quoted 18 lines

Quoted 16 lines

Quoted 15 lines

Quoted 13 lines

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

 	mr	4,3
 	lis	3,KVMPPC_H_RTAS@h
 	ori	3,3,KVMPPC_H_RTAS@l

Quoted 161 lines · Aravinda Prasad

Quoted 158 lines · Alexey Kardashevskiy

Quoted 148 lines · Aravinda Prasad

Quoted 136 lines · Alexey Kardashevskiy

Quoted 125 lines · Aravinda Prasad

Quoted 122 lines · Alexey Kardashevskiy

Quoted 102 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                  |   13 ++++++++++---
 hw/ppc/spapr_rtas.c             |    4 ++--
 include/hw/ppc/spapr.h          |    2 +-
 pc-bios/spapr-rtas/spapr-rtas.S |   12 ++++++++++++
 4 files changed, 25 insertions(+), 6 deletions(-)

hw/ppc/spapr.c +10 -3

index d01978f..1120988 100644
@@ -85,6 +85,12 @@
 
 #define HTAB_SIZE(spapr)        (1ULL << ((spapr)->htab_shift))
 
+/*
+ * The rtas-entry-offset should match the value specified in
+ * spapr-rtas.S
+ */
+#define RTAS_ENTRY_OFFSET       0x1000
+
 typedef struct sPAPRMachineState sPAPRMachineState;
 
 #define TYPE_SPAPR_MACHINE      "spapr-machine"

@@ -670,7 +676,8 @@ static int spapr_populate_memory(sPAPREnvironment *spapr, void *fdt)
 static void spapr_finalize_fdt(sPAPREnvironment *spapr,
                                hwaddr fdt_addr,
                                hwaddr rtas_addr,
-                               hwaddr rtas_size)
+                               hwaddr rtas_size,
+                               hwaddr rtas_entry)
 {
     int ret, i;
     size_t cb = 0;

@@ -705,7 +712,7 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
     }
 
     /* RTAS */
-    ret = spapr_rtas_device_tree_setup(fdt, rtas_addr, rtas_size);
+    ret = spapr_rtas_device_tree_setup(fdt, rtas_addr, rtas_size, rtas_entry);
     if (ret < 0) {
         fprintf(stderr, "Couldn't set up RTAS device tree properties\n");
     }

@@ -808,7 +815,7 @@ static void ppc_spapr_reset(void)
 
     /* Load the fdt */
     spapr_finalize_fdt(spapr, spapr->fdt_addr, spapr->rtas_addr,
-                       spapr->rtas_size);
+                       spapr->rtas_size, spapr->rtas_addr + RTAS_ENTRY_OFFSET);
 
     /* Set up the entry state */
     first_ppc_cpu = POWERPC_CPU(first_cpu);

hw/ppc/spapr_rtas.c +2 -2

index 9ba1ba6..02ddbf9 100644
@@ -328,7 +328,7 @@ void spapr_rtas_register(int token, const char *name, spapr_rtas_fn fn)
 }
 
 int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
-                                 hwaddr rtas_size)
+                                 hwaddr rtas_size, hwaddr rtas_entry)
 {
     int ret;
     int i;

@@ -349,7 +349,7 @@ int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
     }
 
     ret = qemu_fdt_setprop_cell(fdt, "/rtas", "linux,rtas-entry",
-                                rtas_addr);
+                                rtas_entry);
     if (ret < 0) {
         fprintf(stderr, "Couldn't add linux,rtas-entry property: %s\n",
                 fdt_strerror(ret));

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

index bbba51a..dedfa67 100644
@@ -436,7 +436,7 @@ target_ulong spapr_rtas_call(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                              uint32_t token, uint32_t nargs, target_ulong args,
                              uint32_t nret, target_ulong rets);
 int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
-                                 hwaddr rtas_size);
+                                 hwaddr rtas_size, hwaddr rtas_entry);
 
 #define SPAPR_TCE_PAGE_SHIFT   12
 #define SPAPR_TCE_PAGE_SIZE    (1ULL << SPAPR_TCE_PAGE_SHIFT)

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

index 903bec2..8c9b17e 100644
@@ -30,6 +30,18 @@
 
 .globl	_start
 _start:
+	/*
+	 * Reserve space for error log in RTAS blob.

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

Quoted 10 lines

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

+	 *
+	 * Either we can reserve initial bytes for error log followed by
+	 * rtas-entry or space can be reserved after rtas-entry. I prefer
+	 * former, as we already have rtas-base and rtas-entry (currently
+	 * both pointing to rtas-base) defined in qemu and we can update
+	 * rtas-entry to point to an offset from rtas-base. This avoids
+	 * unnecessary definition of rtas-error-offset while keeping
+	 * rtas-entry redundant.
+	 */
+	. = 0x1000

Why not this (and not changing spapr-rtas.S)?

hw/ppc/spapr.c +0 -0

@@ -875,7 +875,8 @@ static void ppc_spapr_reset(void)
                        spapr->rtas_size);
 

Quoted 8 lines

hw/ppc/spapr.c static void ppc_spapr_reset(void) · line 880

Quoted 5 lines

hw/ppc/spapr.c +2 -1

     /* Copy RTAS over */
-    cpu_physical_memory_write(spapr->rtas_addr, spapr->rtas_blob,
+    cpu_physical_memory_write(spapr->rtas_addr + RTAS_ENTRY_OFFSET,
+                              spapr->rtas_blob,
                               spapr->rtas_size);

This is possible, however requires suitable adjustment to make sure spapr->rtas_addr has enough space allocated.

Quoted 3 lines

How is adding RTAS_ENTRY_OFFSET not enough to make sure that is has enough space? QEMU copies RTAS to guest memory, QEMU makes up rtas_addr/entry properties.

QEMU adds spapr-rtas.bin as a rom, with rom->addr set to spapr->rtas_addr, rom->datasize set to 20 bytes (the size of current spapr-rtas.bin) and contents of spapr-rtas.bin read into rom->data (malloc-ed region).

Quoted 4 lines

I think, access to spapr->rtas_addr is mapped to this rom. Hence it is necessary to have rtas_addr and rtas_size consistent with the Rom struct. If we use spapr->rtas_addr + RTAS_ENTRY_OFFSET then we are trying to access an invalid offset in rom region.

What is that "rom" struct you are referring to? In upstream QEMU, I can only see:

Quoted 7 lines

Quoted 3 lines

ppc_spapr_init():
[...]
filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, "spapr-rtas.bin")

In ppc_spapr_init() just after qemu_find_file() we have:

spapr->rtas_size = load_image_targphys(filename, spapr->rtas_addr, ...);

What tree has this? Mine does not.

ka1:~/p/qemu$ grep load_image_targphys hw/ppc/spapr.c
            initrd_size = load_image_targphys(initrd_filename, initrd_base,
    fw_size = load_image_targphys(filename, 0, FW_MAX_SIZE);
ka1:~/p/qemu$

Quoted 45 lines

load_image_targphys() -> rom_add_file_fixed() -> rom_add_file(), where Rom is initialized.

Quoted 38 lines

spapr->rtas_size = get_image_size(filename); spapr->rtas_blob = g_malloc(spapr->rtas_size);
if (load_image_size(filename, spapr->rtas_blob, spapr->rtas_size) < 0) { [...]

and then

ppc_spapr_reset():
[...]
spapr->rtas_addr = rtas_limit - RTAS_MAX_SIZE
[...]
cpu_physical_memory_write(spapr->rtas_addr, spapr->rtas_blob,

                          spapr->rtas_size);

[...]

Quoted 19 lines

Quoted 18 lines

Quoted 16 lines

Quoted 15 lines

Quoted 13 lines

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

 	mr	4,3
 	lis	3,KVMPPC_H_RTAS@h
 	ori	3,3,KVMPPC_H_RTAS@l

Quoted 173 lines · Alexey Kardashevskiy

Quoted 161 lines · Aravinda Prasad

Quoted 158 lines · Alexey Kardashevskiy

Quoted 148 lines · Aravinda Prasad

Quoted 136 lines · Alexey Kardashevskiy

Quoted 125 lines · Aravinda Prasad

Quoted 122 lines · Alexey Kardashevskiy

Quoted 102 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                  |   13 ++++++++++---
 hw/ppc/spapr_rtas.c             |    4 ++--
 include/hw/ppc/spapr.h          |    2 +-
 pc-bios/spapr-rtas/spapr-rtas.S |   12 ++++++++++++
 4 files changed, 25 insertions(+), 6 deletions(-)

hw/ppc/spapr.c +10 -3

index d01978f..1120988 100644
@@ -85,6 +85,12 @@
 
 #define HTAB_SIZE(spapr)        (1ULL << ((spapr)->htab_shift))
 
+/*
+ * The rtas-entry-offset should match the value specified in
+ * spapr-rtas.S
+ */
+#define RTAS_ENTRY_OFFSET       0x1000
+
 typedef struct sPAPRMachineState sPAPRMachineState;
 
 #define TYPE_SPAPR_MACHINE      "spapr-machine"

@@ -670,7 +676,8 @@ static int spapr_populate_memory(sPAPREnvironment *spapr, void *fdt)
 static void spapr_finalize_fdt(sPAPREnvironment *spapr,
                                hwaddr fdt_addr,
                                hwaddr rtas_addr,
-                               hwaddr rtas_size)
+                               hwaddr rtas_size,
+                               hwaddr rtas_entry)
 {
     int ret, i;
     size_t cb = 0;

@@ -705,7 +712,7 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
     }
 
     /* RTAS */
-    ret = spapr_rtas_device_tree_setup(fdt, rtas_addr, rtas_size);
+    ret = spapr_rtas_device_tree_setup(fdt, rtas_addr, rtas_size, rtas_entry);
     if (ret < 0) {
         fprintf(stderr, "Couldn't set up RTAS device tree properties\n");
     }

@@ -808,7 +815,7 @@ static void ppc_spapr_reset(void)
 
     /* Load the fdt */
     spapr_finalize_fdt(spapr, spapr->fdt_addr, spapr->rtas_addr,
-                       spapr->rtas_size);
+                       spapr->rtas_size, spapr->rtas_addr + RTAS_ENTRY_OFFSET);
 
     /* Set up the entry state */
     first_ppc_cpu = POWERPC_CPU(first_cpu);

hw/ppc/spapr_rtas.c +2 -2

index 9ba1ba6..02ddbf9 100644
@@ -328,7 +328,7 @@ void spapr_rtas_register(int token, const char *name, spapr_rtas_fn fn)
 }
 
 int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
-                                 hwaddr rtas_size)
+                                 hwaddr rtas_size, hwaddr rtas_entry)
 {
     int ret;
     int i;

@@ -349,7 +349,7 @@ int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
     }
 
     ret = qemu_fdt_setprop_cell(fdt, "/rtas", "linux,rtas-entry",
-                                rtas_addr);
+                                rtas_entry);
     if (ret < 0) {
         fprintf(stderr, "Couldn't add linux,rtas-entry property: %s\n",
                 fdt_strerror(ret));

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

index bbba51a..dedfa67 100644
@@ -436,7 +436,7 @@ target_ulong spapr_rtas_call(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                              uint32_t token, uint32_t nargs, target_ulong args,
                              uint32_t nret, target_ulong rets);
 int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
-                                 hwaddr rtas_size);
+                                 hwaddr rtas_size, hwaddr rtas_entry);
 
 #define SPAPR_TCE_PAGE_SHIFT   12
 #define SPAPR_TCE_PAGE_SIZE    (1ULL << SPAPR_TCE_PAGE_SHIFT)

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

index 903bec2..8c9b17e 100644
@@ -30,6 +30,18 @@
 
 .globl	_start
 _start:
+	/*
+	 * Reserve space for error log in RTAS blob.

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

Quoted 10 lines

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

+	 *
+	 * Either we can reserve initial bytes for error log followed by
+	 * rtas-entry or space can be reserved after rtas-entry. I prefer
+	 * former, as we already have rtas-base and rtas-entry (currently
+	 * both pointing to rtas-base) defined in qemu and we can update
+	 * rtas-entry to point to an offset from rtas-base. This avoids
+	 * unnecessary definition of rtas-error-offset while keeping
+	 * rtas-entry redundant.
+	 */
+	. = 0x1000

Why not this (and not changing spapr-rtas.S)?

hw/ppc/spapr.c +0 -0

@@ -875,7 +875,8 @@ static void ppc_spapr_reset(void)
                        spapr->rtas_size);
 

Quoted 8 lines

hw/ppc/spapr.c static void ppc_spapr_reset(void) · line 880

Quoted 5 lines

hw/ppc/spapr.c +2 -1

     /* Copy RTAS over */
-    cpu_physical_memory_write(spapr->rtas_addr, spapr->rtas_blob,
+    cpu_physical_memory_write(spapr->rtas_addr + RTAS_ENTRY_OFFSET,
+                              spapr->rtas_blob,
                               spapr->rtas_size);

This is possible, however requires suitable adjustment to make sure spapr->rtas_addr has enough space allocated.

Quoted 3 lines

How is adding RTAS_ENTRY_OFFSET not enough to make sure that is has enough space? QEMU copies RTAS to guest memory, QEMU makes up rtas_addr/entry properties.

QEMU adds spapr-rtas.bin as a rom, with rom->addr set to spapr->rtas_addr, rom->datasize set to 20 bytes (the size of current spapr-rtas.bin) and contents of spapr-rtas.bin read into rom->data (malloc-ed region).

Quoted 4 lines

I think, access to spapr->rtas_addr is mapped to this rom. Hence it is necessary to have rtas_addr and rtas_size consistent with the Rom struct. If we use spapr->rtas_addr + RTAS_ENTRY_OFFSET then we are trying to access an invalid offset in rom region.

What is that "rom" struct you are referring to? In upstream QEMU, I can only see:

Quoted 7 lines

Quoted 3 lines

ppc_spapr_init():
[...]
filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, "spapr-rtas.bin")

In ppc_spapr_init() just after qemu_find_file() we have:

spapr->rtas_size = load_image_targphys(filename, spapr->rtas_addr, ...);

What tree has this? Mine does not.

Quoted 5 lines

ka1:~/p/qemu$ grep load_image_targphys hw/ppc/spapr.c
            initrd_size = load_image_targphys(initrd_filename, initrd_base,
    fw_size = load_image_targphys(filename, 0, FW_MAX_SIZE);
ka1:~/p/qemu$

I am using git://git.qemu.org/qemu.git, master branch.

I have 88e89a57f9 as the latest commit and I see:

$ grep load_image_targphys hw/ppc/spapr.c
    spapr->rtas_size = load_image_targphys(filename, spapr->rtas_addr,
         initrd_size = load_image_targphys(initrd_filename, initrd_base,
    fw_size = load_image_targphys(filename, 0, FW_MAX_SIZE);

This was introduced in commit a3467baa.

Regards,
Aravinda

Quoted 47 lines

Quoted 45 lines

load_image_targphys() -> rom_add_file_fixed() -> rom_add_file(), where Rom is initialized.

Quoted 38 lines

spapr->rtas_size = get_image_size(filename); spapr->rtas_blob = g_malloc(spapr->rtas_size);
if (load_image_size(filename, spapr->rtas_blob, spapr->rtas_size) < 0) { [...]

and then

ppc_spapr_reset():
[...]
spapr->rtas_addr = rtas_limit - RTAS_MAX_SIZE
[...]
cpu_physical_memory_write(spapr->rtas_addr, spapr->rtas_blob,

                          spapr->rtas_size);

[...]

Quoted 19 lines

Quoted 18 lines

Quoted 16 lines

Quoted 15 lines

Quoted 13 lines

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

 	mr	4,3
 	lis	3,KVMPPC_H_RTAS@h
 	ori	3,3,KVMPPC_H_RTAS@l

Quoted 176 lines · Aravinda Prasad

Quoted 173 lines · Alexey Kardashevskiy

Quoted 161 lines · Aravinda Prasad

Quoted 158 lines · Alexey Kardashevskiy

Quoted 148 lines · Aravinda Prasad

Quoted 136 lines · Alexey Kardashevskiy

Quoted 125 lines · Aravinda Prasad

Quoted 122 lines · Alexey Kardashevskiy

Quoted 102 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                  |   13 ++++++++++---
 hw/ppc/spapr_rtas.c             |    4 ++--
 include/hw/ppc/spapr.h          |    2 +-
 pc-bios/spapr-rtas/spapr-rtas.S |   12 ++++++++++++
 4 files changed, 25 insertions(+), 6 deletions(-)

hw/ppc/spapr.c +10 -3

index d01978f..1120988 100644
@@ -85,6 +85,12 @@
 
 #define HTAB_SIZE(spapr)        (1ULL << ((spapr)->htab_shift))
 
+/*
+ * The rtas-entry-offset should match the value specified in
+ * spapr-rtas.S
+ */
+#define RTAS_ENTRY_OFFSET       0x1000
+
 typedef struct sPAPRMachineState sPAPRMachineState;
 
 #define TYPE_SPAPR_MACHINE      "spapr-machine"

@@ -670,7 +676,8 @@ static int spapr_populate_memory(sPAPREnvironment *spapr, void *fdt)
 static void spapr_finalize_fdt(sPAPREnvironment *spapr,
                                hwaddr fdt_addr,
                                hwaddr rtas_addr,
-                               hwaddr rtas_size)
+                               hwaddr rtas_size,
+                               hwaddr rtas_entry)
 {
     int ret, i;
     size_t cb = 0;

@@ -705,7 +712,7 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
     }
 
     /* RTAS */
-    ret = spapr_rtas_device_tree_setup(fdt, rtas_addr, rtas_size);
+    ret = spapr_rtas_device_tree_setup(fdt, rtas_addr, rtas_size, rtas_entry);
     if (ret < 0) {
         fprintf(stderr, "Couldn't set up RTAS device tree properties\n");
     }

@@ -808,7 +815,7 @@ static void ppc_spapr_reset(void)
 
     /* Load the fdt */
     spapr_finalize_fdt(spapr, spapr->fdt_addr, spapr->rtas_addr,
-                       spapr->rtas_size);
+                       spapr->rtas_size, spapr->rtas_addr + RTAS_ENTRY_OFFSET);
 
     /* Set up the entry state */
     first_ppc_cpu = POWERPC_CPU(first_cpu);

hw/ppc/spapr_rtas.c +2 -2

index 9ba1ba6..02ddbf9 100644
@@ -328,7 +328,7 @@ void spapr_rtas_register(int token, const char *name, spapr_rtas_fn fn)
 }
 
 int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
-                                 hwaddr rtas_size)
+                                 hwaddr rtas_size, hwaddr rtas_entry)
 {
     int ret;
     int i;

@@ -349,7 +349,7 @@ int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
     }
 
     ret = qemu_fdt_setprop_cell(fdt, "/rtas", "linux,rtas-entry",
-                                rtas_addr);
+                                rtas_entry);
     if (ret < 0) {
         fprintf(stderr, "Couldn't add linux,rtas-entry property: %s\n",
                 fdt_strerror(ret));

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

index bbba51a..dedfa67 100644
@@ -436,7 +436,7 @@ target_ulong spapr_rtas_call(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                              uint32_t token, uint32_t nargs, target_ulong args,
                              uint32_t nret, target_ulong rets);
 int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
-                                 hwaddr rtas_size);
+                                 hwaddr rtas_size, hwaddr rtas_entry);
 
 #define SPAPR_TCE_PAGE_SHIFT   12
 #define SPAPR_TCE_PAGE_SIZE    (1ULL << SPAPR_TCE_PAGE_SHIFT)

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

index 903bec2..8c9b17e 100644
@@ -30,6 +30,18 @@
 
 .globl	_start
 _start:
+	/*
+	 * Reserve space for error log in RTAS blob.

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

Quoted 10 lines

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

+	 *
+	 * Either we can reserve initial bytes for error log followed by
+	 * rtas-entry or space can be reserved after rtas-entry. I prefer
+	 * former, as we already have rtas-base and rtas-entry (currently
+	 * both pointing to rtas-base) defined in qemu and we can update
+	 * rtas-entry to point to an offset from rtas-base. This avoids
+	 * unnecessary definition of rtas-error-offset while keeping
+	 * rtas-entry redundant.
+	 */
+	. = 0x1000

Why not this (and not changing spapr-rtas.S)?

hw/ppc/spapr.c +0 -0

@@ -875,7 +875,8 @@ static void ppc_spapr_reset(void)
                        spapr->rtas_size);
 

Quoted 8 lines

hw/ppc/spapr.c static void ppc_spapr_reset(void) · line 880

Quoted 5 lines

hw/ppc/spapr.c +2 -1

     /* Copy RTAS over */
-    cpu_physical_memory_write(spapr->rtas_addr, spapr->rtas_blob,
+    cpu_physical_memory_write(spapr->rtas_addr + RTAS_ENTRY_OFFSET,
+                              spapr->rtas_blob,
                               spapr->rtas_size);

This is possible, however requires suitable adjustment to make sure spapr->rtas_addr has enough space allocated.

Quoted 3 lines

How is adding RTAS_ENTRY_OFFSET not enough to make sure that is has enough space? QEMU copies RTAS to guest memory, QEMU makes up rtas_addr/entry properties.

QEMU adds spapr-rtas.bin as a rom, with rom->addr set to spapr->rtas_addr, rom->datasize set to 20 bytes (the size of current spapr-rtas.bin) and contents of spapr-rtas.bin read into rom->data (malloc-ed region).

Quoted 4 lines

I think, access to spapr->rtas_addr is mapped to this rom. Hence it is necessary to have rtas_addr and rtas_size consistent with the Rom struct. If we use spapr->rtas_addr + RTAS_ENTRY_OFFSET then we are trying to access an invalid offset in rom region.

What is that "rom" struct you are referring to? In upstream QEMU, I can only see:

Quoted 7 lines

Quoted 3 lines

ppc_spapr_init():
[...]
filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, "spapr-rtas.bin")

In ppc_spapr_init() just after qemu_find_file() we have:

spapr->rtas_size = load_image_targphys(filename, spapr->rtas_addr, ...);

What tree has this? Mine does not.

Quoted 7 lines

Quoted 5 lines

ka1:~/p/qemu$ grep load_image_targphys hw/ppc/spapr.c
            initrd_size = load_image_targphys(initrd_filename, initrd_base,
    fw_size = load_image_targphys(filename, 0, FW_MAX_SIZE);
ka1:~/p/qemu$

I am using git://git.qemu.org/qemu.git, master branch.

Ah. I see. My bad, I recommended the wrong tree then, sorry about it...

This one is the current queue of ppc patches and I believe this patchset will go through it:
git://github.com/agraf/qemu.git branch: ppc-next

And it does not have load_image_targphys() for rtas.

Quoted 64 lines

I have 88e89a57f9 as the latest commit and I see:

$ grep load_image_targphys hw/ppc/spapr.c
    spapr->rtas_size = load_image_targphys(filename, spapr->rtas_addr,
         initrd_size = load_image_targphys(initrd_filename, initrd_base,
    fw_size = load_image_targphys(filename, 0, FW_MAX_SIZE);

This was introduced in commit a3467baa.

Regards,
Aravinda

Quoted 47 lines

Quoted 45 lines

load_image_targphys() -> rom_add_file_fixed() -> rom_add_file(), where Rom is initialized.

Quoted 38 lines

spapr->rtas_size = get_image_size(filename); spapr->rtas_blob = g_malloc(spapr->rtas_size);
if (load_image_size(filename, spapr->rtas_blob, spapr->rtas_size) < 0) { [...]

and then

ppc_spapr_reset():
[...]
spapr->rtas_addr = rtas_limit - RTAS_MAX_SIZE
[...]
cpu_physical_memory_write(spapr->rtas_addr, spapr->rtas_blob,

                          spapr->rtas_size);

[...]

Quoted 19 lines

Quoted 18 lines

Quoted 16 lines

Quoted 15 lines

Quoted 13 lines

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

 	mr	4,3
 	lis	3,KVMPPC_H_RTAS@h
 	ori	3,3,KVMPPC_H_RTAS@l

Quoted 191 lines · Alexey Kardashevskiy

Quoted 176 lines · Aravinda Prasad

Quoted 173 lines · Alexey Kardashevskiy

Quoted 161 lines · Aravinda Prasad

Quoted 158 lines · Alexey Kardashevskiy

Quoted 148 lines · Aravinda Prasad

Quoted 136 lines · Alexey Kardashevskiy

Quoted 125 lines · Aravinda Prasad

Quoted 122 lines · Alexey Kardashevskiy

Quoted 102 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                  |   13 ++++++++++---
 hw/ppc/spapr_rtas.c             |    4 ++--
 include/hw/ppc/spapr.h          |    2 +-
 pc-bios/spapr-rtas/spapr-rtas.S |   12 ++++++++++++
 4 files changed, 25 insertions(+), 6 deletions(-)

hw/ppc/spapr.c +10 -3

index d01978f..1120988 100644
@@ -85,6 +85,12 @@
 
 #define HTAB_SIZE(spapr)        (1ULL << ((spapr)->htab_shift))
 
+/*
+ * The rtas-entry-offset should match the value specified in
+ * spapr-rtas.S
+ */
+#define RTAS_ENTRY_OFFSET       0x1000
+
 typedef struct sPAPRMachineState sPAPRMachineState;
 
 #define TYPE_SPAPR_MACHINE      "spapr-machine"

@@ -670,7 +676,8 @@ static int spapr_populate_memory(sPAPREnvironment *spapr, void *fdt)
 static void spapr_finalize_fdt(sPAPREnvironment *spapr,
                                hwaddr fdt_addr,
                                hwaddr rtas_addr,
-                               hwaddr rtas_size)
+                               hwaddr rtas_size,
+                               hwaddr rtas_entry)
 {
     int ret, i;
     size_t cb = 0;

@@ -705,7 +712,7 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
     }
 
     /* RTAS */
-    ret = spapr_rtas_device_tree_setup(fdt, rtas_addr, rtas_size);
+    ret = spapr_rtas_device_tree_setup(fdt, rtas_addr, rtas_size, rtas_entry);
     if (ret < 0) {
         fprintf(stderr, "Couldn't set up RTAS device tree properties\n");
     }

@@ -808,7 +815,7 @@ static void ppc_spapr_reset(void)
 
     /* Load the fdt */
     spapr_finalize_fdt(spapr, spapr->fdt_addr, spapr->rtas_addr,
-                       spapr->rtas_size);
+                       spapr->rtas_size, spapr->rtas_addr + RTAS_ENTRY_OFFSET);
 
     /* Set up the entry state */
     first_ppc_cpu = POWERPC_CPU(first_cpu);

hw/ppc/spapr_rtas.c +2 -2

index 9ba1ba6..02ddbf9 100644
@@ -328,7 +328,7 @@ void spapr_rtas_register(int token, const char *name, spapr_rtas_fn fn)
 }
 
 int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
-                                 hwaddr rtas_size)
+                                 hwaddr rtas_size, hwaddr rtas_entry)
 {
     int ret;
     int i;

@@ -349,7 +349,7 @@ int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
     }
 
     ret = qemu_fdt_setprop_cell(fdt, "/rtas", "linux,rtas-entry",
-                                rtas_addr);
+                                rtas_entry);
     if (ret < 0) {
         fprintf(stderr, "Couldn't add linux,rtas-entry property: %s\n",
                 fdt_strerror(ret));

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

index bbba51a..dedfa67 100644
@@ -436,7 +436,7 @@ target_ulong spapr_rtas_call(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                              uint32_t token, uint32_t nargs, target_ulong args,
                              uint32_t nret, target_ulong rets);
 int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
-                                 hwaddr rtas_size);
+                                 hwaddr rtas_size, hwaddr rtas_entry);
 
 #define SPAPR_TCE_PAGE_SHIFT   12
 #define SPAPR_TCE_PAGE_SIZE    (1ULL << SPAPR_TCE_PAGE_SHIFT)

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

index 903bec2..8c9b17e 100644
@@ -30,6 +30,18 @@
 
 .globl	_start
 _start:
+	/*
+	 * Reserve space for error log in RTAS blob.

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

Quoted 10 lines

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

+	 *
+	 * Either we can reserve initial bytes for error log followed by
+	 * rtas-entry or space can be reserved after rtas-entry. I prefer
+	 * former, as we already have rtas-base and rtas-entry (currently
+	 * both pointing to rtas-base) defined in qemu and we can update
+	 * rtas-entry to point to an offset from rtas-base. This avoids
+	 * unnecessary definition of rtas-error-offset while keeping
+	 * rtas-entry redundant.
+	 */
+	. = 0x1000

Why not this (and not changing spapr-rtas.S)?

hw/ppc/spapr.c +0 -0

@@ -875,7 +875,8 @@ static void ppc_spapr_reset(void)
                        spapr->rtas_size);
 

Quoted 8 lines

hw/ppc/spapr.c static void ppc_spapr_reset(void) · line 880

Quoted 5 lines

hw/ppc/spapr.c +2 -1

     /* Copy RTAS over */
-    cpu_physical_memory_write(spapr->rtas_addr, spapr->rtas_blob,
+    cpu_physical_memory_write(spapr->rtas_addr + RTAS_ENTRY_OFFSET,
+                              spapr->rtas_blob,
                               spapr->rtas_size);

This is possible, however requires suitable adjustment to make sure spapr->rtas_addr has enough space allocated.

Quoted 3 lines

How is adding RTAS_ENTRY_OFFSET not enough to make sure that is has enough space? QEMU copies RTAS to guest memory, QEMU makes up rtas_addr/entry properties.

QEMU adds spapr-rtas.bin as a rom, with rom->addr set to spapr->rtas_addr, rom->datasize set to 20 bytes (the size of current spapr-rtas.bin) and contents of spapr-rtas.bin read into rom->data (malloc-ed region).

Quoted 4 lines

I think, access to spapr->rtas_addr is mapped to this rom. Hence it is necessary to have rtas_addr and rtas_size consistent with the Rom struct. If we use spapr->rtas_addr + RTAS_ENTRY_OFFSET then we are trying to access an invalid offset in rom region.

What is that "rom" struct you are referring to? In upstream QEMU, I can only see:

Quoted 7 lines

Quoted 3 lines

ppc_spapr_init():
[...]
filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, "spapr-rtas.bin")

In ppc_spapr_init() just after qemu_find_file() we have:

spapr->rtas_size = load_image_targphys(filename, spapr->rtas_addr, ...);

What tree has this? Mine does not.

Quoted 7 lines

Quoted 5 lines

ka1:~/p/qemu$ grep load_image_targphys hw/ppc/spapr.c
            initrd_size = load_image_targphys(initrd_filename, initrd_base,
    fw_size = load_image_targphys(filename, 0, FW_MAX_SIZE);
ka1:~/p/qemu$

I am using git://git.qemu.org/qemu.git, master branch.

Ah. I see. My bad, I recommended the wrong tree then, sorry about it...

This one is the current queue of ppc patches and I believe this patchset will go through it:
git://github.com/agraf/qemu.git branch: ppc-next

Quoted 1 line

And it does not have load_image_targphys() for rtas.

Hmm.. ok. Let me have a look at it.

So my v2 should be based on git://github.com/agraf/qemu.git branch:
ppc-next right?

Quoted 37 lines

Quoted 37 lines

I have 88e89a57f9 as the latest commit and I see:

$ grep load_image_targphys hw/ppc/spapr.c
    spapr->rtas_size = load_image_targphys(filename, spapr->rtas_addr,
         initrd_size = load_image_targphys(initrd_filename, initrd_base,
    fw_size = load_image_targphys(filename, 0, FW_MAX_SIZE);

This was introduced in commit a3467baa.

Regards,
Aravinda

Quoted 20 lines

Quoted 20 lines

load_image_targphys() -> rom_add_file_fixed() -> rom_add_file(), where Rom is initialized.

Quoted 14 lines

spapr->rtas_size = get_image_size(filename); spapr->rtas_blob = g_malloc(spapr->rtas_size);
if (load_image_size(filename, spapr->rtas_blob, spapr->rtas_size) < 0) { [...]

and then

ppc_spapr_reset():
[...]
spapr->rtas_addr = rtas_limit - RTAS_MAX_SIZE
[...]
cpu_physical_memory_write(spapr->rtas_addr, spapr->rtas_blob,

                          spapr->rtas_size);

[...]

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

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

Quoted 8 lines · Aravinda Prasad

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

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

This also seems really awkward. Specifically it seems like a rather arbitrary and complex division of what qemu's responsible for and what SLOF is responsible for.

Instead I'd suggest that we add an H_INSTANTIATE_RTAS hcall, and we move the loading of the spapr-rtas blob from normal reset to when that hcall is invoked.

Quoted 14 lines · David Gibson

Quoted 8 lines · Aravinda Prasad

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

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

This also seems really awkward. Specifically it seems like a rather arbitrary and complex division of what qemu's responsible for and what SLOF is responsible for.

Quoted 3 lines

Instead I'd suggest that we add an H_INSTANTIATE_RTAS hcall, and we move the loading of the spapr-rtas blob from normal reset to when that hcall is invoked.

Beware that SLOF needs to call RTAS for its own reasons...

So it would be instanciated twice.

Quoted 3 lines

Beware that SLOF needs to call RTAS for its own reasons...

So it would be instanciated twice.

I don't see that that would cause a big problem, or am I missing something?

Whenever there is a physical memory error due to bit flips, which cannot be corrected by hardware, the error is passed on to the kernel. If the memory address in error belongs to guest address space then guest kernel is responsible to take action. Hence the error is passed on to guest via KVM by invoking 0x200 NMI vector.

However, guest OS, as per PAPR, expects an error log upon such error. This patch registers a new hcall which is issued from 0x200 interrupt vector and builds the error log, copies the error log to rtas space and passes the address of the error log to guest

Enhancement to KVM to perform above functionality is already in upstream kernel.

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

---
 hw/ppc/spapr_hcall.c   |  147 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h |    4 +
 2 files changed, 150 insertions(+), 1 deletion(-)

hw/ppc/spapr_hcall.c +25 -0

index 118ee77..be063f4 100644
@@ -14,6 +14,86 @@ struct SPRSyncState {
     target_ulong mask;
 };
 
+#define SEVERITY_SHIFT         0x5
+#define DISPOSITION_SHIFT      0x3
+#define INITIATOR_SHIFT        0x4
+
+/*
+ * Only required RTAS event severity, disposition, initiator
+ * target and type are copied from arch/powerpc/include/asm/rtas.h
+ */
+
+/* RTAS event severity */
+#define RTAS_SEVERITY_ERROR_SYNC    0x3
+
+/* RTAS event disposition */
+#define RTAS_DISP_NOT_RECOVERED     0x2
+
+/* RTAS event initiator */
+#define RTAS_INITIATOR_MEMORY       0x4
+
+/* RTAS event target */
+#define RTAS_TARGET_MEMORY          0x4
+
+/* RTAS event type */
+#define RTAS_TYPE_ECC_UNCORR        0x09
+
+/*

hw/ppc/spapr_hcall.c +112 -0

+#define P7_DSISR_MC_UE              (PPC_BIT(48))  /* P8 too */
+
+/* Adopted from kernel source arch/powerpc/include/asm/rtas.h */
+struct rtas_error_log {
+    /* Byte 0 */
+    uint8_t     byte0;          /* Architectural version */
+
+    /* Byte 1 */
+    uint8_t     byte1;
+    /* XXXXXXXX
+     * XXX      3: Severity level of error
+     *    XX    2: Degree of recovery
+     *      X   1: Extended log present?
+     *       XX 2: Reserved
+     */
+
+    /* Byte 2 */
+    uint8_t     byte2;
+    /* XXXXXXXX
+     * XXXX     4: Initiator of event
+     *     XXXX 4: Target of failed operation
+     */
+    uint8_t     byte3;          /* General event or error*/
+};
+
+/*
+ * Data format in RTAS-Blob
+ *
+ * This structure contains error information related to Machine
+ * Check exception. This is filled up and copied to rtas-blob
+ * upon machine check exception.
+ */
+struct rtas_mc_log {
+    target_ulong srr0;
+    target_ulong srr1;
+    /*
+     * Beginning of error log address. This is properly
+     * populated and passed on to OS registered machine
+     * check notification routine upon machine check
+     * exception
+     */
+    target_ulong r3;
+    struct rtas_error_log err_log;
+};
+
 static void do_spr_sync(void *arg)
 {
     struct SPRSyncState *s = arg;

@@ -586,6 +666,72 @@ static target_ulong h_rtas_update(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     return 0;
 }
 
+static target_ulong h_report_mc_err(PowerPCCPU *cpu, sPAPREnvironment *spapr,
+                                 target_ulong opcode, target_ulong *args)
+{
+    struct rtas_mc_log mc_log;
+    CPUPPCState *env = &cpu->env;
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+
+    /*
+     * 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
+     */
+    mc_log.r3 = env->spr[SPR_SPRG2];
+
+    /*
+     * SRR0 and SRR1 contains nip and msr at the time of exception
+     * and are clobbered when we return from this hcall
+     * and hence need to be properly restored. We save srr0
+     * and srr1 in rtas blob and restore it in 0x200 vector
+     * before branching to OS registered machine check handler
+     */
+    mc_log.srr0 = env->spr[SPR_SRR0];
+    mc_log.srr1 = env->spr[SPR_SRR1];
+
+    /* Set error log fields */
+    mc_log.err_log.byte0 = 0x00;
+    mc_log.err_log.byte1 = (RTAS_SEVERITY_ERROR_SYNC << SEVERITY_SHIFT);
+    mc_log.err_log.byte1 |= (RTAS_DISP_NOT_RECOVERED << DISPOSITION_SHIFT);
+    mc_log.err_log.byte2 = (RTAS_INITIATOR_MEMORY << INITIATOR_SHIFT);
+    mc_log.err_log.byte2 |= RTAS_TARGET_MEMORY;
+
+    if (env->spr[SPR_DSISR] & P7_DSISR_MC_UE) {
+        mc_log.err_log.byte3 = RTAS_TYPE_ECC_UNCORR;
+    } else {
+        mc_log.err_log.byte3 = 0x0;
+    }
+
+    /* Handle all Host/Guest LE/BE combinations */
+    if ((*pcc->interrupts_big_endian)(cpu)) {
+        mc_log.srr0 = cpu_to_be64(mc_log.srr0);
+        mc_log.srr1 = cpu_to_be64(mc_log.srr1);
+        mc_log.r3 = cpu_to_be64(mc_log.r3);
+    } else {
+        mc_log.srr0 = cpu_to_le64(mc_log.srr0);
+        mc_log.srr1 = cpu_to_le64(mc_log.srr1);
+        mc_log.r3 = cpu_to_le64(mc_log.r3);
+    }
+
+    cpu_physical_memory_write(spapr->rtas_addr, &mc_log,
+                              sizeof(mc_log));
+
+    /*
+     * spapr->rtas_addr now contains srr0, srr1, original r3, followed
+     * by the error log structure. The address of the error log
+     * should be passed on to guest's machine check notification
+     * routine. As this hcall is directly called from 0x200 interrupt
+     * vector and returns to assembly routine, we return spapr->rtas_addr
+     * instead of H_SUCCESS. Upon return, We restore srr0 and srr1, increment
+     * r3 to point to the error log and branch to machine check notification
+     * routine in 0x200. r3 containing the error address is now argument
+     * to OS registered machine check notification routine. This way we
+     * also avoids clobbering additional registers in 0x200 vector.
+     */
+    return spapr->rtas_addr;
+}
+
 static target_ulong h_logical_load(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                    target_ulong opcode, target_ulong *args)
 {

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

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

index fb8cc63..57199f5 100644
@@ -310,7 +310,9 @@ typedef struct sPAPREnvironment {
 /* Client Architecture support */
 #define KVMPPC_H_CAS            (KVMPPC_HCALL_BASE + 0x2)
 #define KVMPPC_H_RTAS_UPDATE    (KVMPPC_HCALL_BASE + 0x3)
-#define KVMPPC_HCALL_MAX        KVMPPC_H_RTAS_UPDATE
+/* Report Machine Check error */
+#define KVMPPC_H_REPORT_MC_ERR  (KVMPPC_HCALL_BASE + 0x4)
+#define KVMPPC_HCALL_MAX        KVMPPC_H_REPORT_MC_ERR
 
 extern sPAPREnvironment *spapr;
 

Quoted 55 lines · Aravinda Prasad

Whenever there is a physical memory error due to bit flips, which cannot be corrected by hardware, the error is passed on to the kernel. If the memory address in error belongs to guest address space then guest kernel is responsible to take action. Hence the error is passed on to guest via KVM by invoking 0x200 NMI vector.

However, guest OS, as per PAPR, expects an error log upon such error. This patch registers a new hcall which is issued from 0x200 interrupt vector and builds the error log, copies the error log to rtas space and passes the address of the error log to guest

Enhancement to KVM to perform above functionality is already in upstream kernel.

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

---
 hw/ppc/spapr_hcall.c   |  147 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h |    4 +
 2 files changed, 150 insertions(+), 1 deletion(-)

hw/ppc/spapr_hcall.c +25 -0

index 118ee77..be063f4 100644
@@ -14,6 +14,86 @@ struct SPRSyncState {
     target_ulong mask;
 };
 
+#define SEVERITY_SHIFT         0x5
+#define DISPOSITION_SHIFT      0x3
+#define INITIATOR_SHIFT        0x4
+
+/*
+ * Only required RTAS event severity, disposition, initiator
+ * target and type are copied from arch/powerpc/include/asm/rtas.h
+ */
+
+/* RTAS event severity */
+#define RTAS_SEVERITY_ERROR_SYNC    0x3
+
+/* RTAS event disposition */
+#define RTAS_DISP_NOT_RECOVERED     0x2
+
+/* RTAS event initiator */
+#define RTAS_INITIATOR_MEMORY       0x4
+
+/* RTAS event target */
+#define RTAS_TARGET_MEMORY          0x4
+
+/* RTAS event type */
+#define RTAS_TYPE_ECC_UNCORR        0x09
+
+/*

hw/ppc/spapr_hcall.c struct SPRSyncState { · line 51

Quoted 10 lines

hw/ppc/spapr_hcall.c +10 -0

+ * Currently KVM only passes on the uncorrected machine
+ * check memory error to guest. Other machine check errors
+ * such as SLB multi-hit and TLB multi-hit are recovered
+ * in KVM and are not passed on to guest.
+ *
+ * DSISR Bit for uncorrected machine check error. Copied
+ * from arch/powerpc/include/asm/mce.h
+ */
+#define PPC_BITLSHIFT(be)           (64 - 1 - (be))
+#define PPC_BIT(bit)                (1UL << PPC_BITLSHIFT(bit))

Just opencode the BITLSHIFT macro into the BIT macro, it doesn't improve either brevity or clarity as it is.

Or alternatively define PPC_BIT as (0x8000000000000000ULL >> bit) which might be clearer.

Quoted 147 lines

hw/ppc/spapr_hcall.c +112 -0

+#define P7_DSISR_MC_UE              (PPC_BIT(48))  /* P8 too */
+
+/* Adopted from kernel source arch/powerpc/include/asm/rtas.h */
+struct rtas_error_log {
+    /* Byte 0 */
+    uint8_t     byte0;          /* Architectural version */
+
+    /* Byte 1 */
+    uint8_t     byte1;
+    /* XXXXXXXX
+     * XXX      3: Severity level of error
+     *    XX    2: Degree of recovery
+     *      X   1: Extended log present?
+     *       XX 2: Reserved
+     */
+
+    /* Byte 2 */
+    uint8_t     byte2;
+    /* XXXXXXXX
+     * XXXX     4: Initiator of event
+     *     XXXX 4: Target of failed operation
+     */
+    uint8_t     byte3;          /* General event or error*/
+};
+
+/*
+ * Data format in RTAS-Blob
+ *
+ * This structure contains error information related to Machine
+ * Check exception. This is filled up and copied to rtas-blob
+ * upon machine check exception.
+ */
+struct rtas_mc_log {
+    target_ulong srr0;
+    target_ulong srr1;
+    /*
+     * Beginning of error log address. This is properly
+     * populated and passed on to OS registered machine
+     * check notification routine upon machine check
+     * exception
+     */
+    target_ulong r3;
+    struct rtas_error_log err_log;
+};
+
 static void do_spr_sync(void *arg)
 {
     struct SPRSyncState *s = arg;

@@ -586,6 +666,72 @@ static target_ulong h_rtas_update(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     return 0;
 }
 
+static target_ulong h_report_mc_err(PowerPCCPU *cpu, sPAPREnvironment *spapr,
+                                 target_ulong opcode, target_ulong *args)
+{
+    struct rtas_mc_log mc_log;
+    CPUPPCState *env = &cpu->env;
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+
+    /*
+     * 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
+     */
+    mc_log.r3 = env->spr[SPR_SPRG2];
+
+    /*
+     * SRR0 and SRR1 contains nip and msr at the time of exception
+     * and are clobbered when we return from this hcall
+     * and hence need to be properly restored. We save srr0
+     * and srr1 in rtas blob and restore it in 0x200 vector
+     * before branching to OS registered machine check handler
+     */
+    mc_log.srr0 = env->spr[SPR_SRR0];
+    mc_log.srr1 = env->spr[SPR_SRR1];
+
+    /* Set error log fields */
+    mc_log.err_log.byte0 = 0x00;
+    mc_log.err_log.byte1 = (RTAS_SEVERITY_ERROR_SYNC << SEVERITY_SHIFT);
+    mc_log.err_log.byte1 |= (RTAS_DISP_NOT_RECOVERED << DISPOSITION_SHIFT);
+    mc_log.err_log.byte2 = (RTAS_INITIATOR_MEMORY << INITIATOR_SHIFT);
+    mc_log.err_log.byte2 |= RTAS_TARGET_MEMORY;
+
+    if (env->spr[SPR_DSISR] & P7_DSISR_MC_UE) {
+        mc_log.err_log.byte3 = RTAS_TYPE_ECC_UNCORR;
+    } else {
+        mc_log.err_log.byte3 = 0x0;
+    }
+
+    /* Handle all Host/Guest LE/BE combinations */
+    if ((*pcc->interrupts_big_endian)(cpu)) {
+        mc_log.srr0 = cpu_to_be64(mc_log.srr0);
+        mc_log.srr1 = cpu_to_be64(mc_log.srr1);
+        mc_log.r3 = cpu_to_be64(mc_log.r3);
+    } else {
+        mc_log.srr0 = cpu_to_le64(mc_log.srr0);
+        mc_log.srr1 = cpu_to_le64(mc_log.srr1);
+        mc_log.r3 = cpu_to_le64(mc_log.r3);
+    }
+
+    cpu_physical_memory_write(spapr->rtas_addr, &mc_log,
+                              sizeof(mc_log));
+
+    /*
+     * spapr->rtas_addr now contains srr0, srr1, original r3, followed
+     * by the error log structure. The address of the error log
+     * should be passed on to guest's machine check notification
+     * routine. As this hcall is directly called from 0x200 interrupt
+     * vector and returns to assembly routine, we return spapr->rtas_addr
+     * instead of H_SUCCESS. Upon return, We restore srr0 and srr1, increment
+     * r3 to point to the error log and branch to machine check notification
+     * routine in 0x200. r3 containing the error address is now argument
+     * to OS registered machine check notification routine. This way we
+     * also avoids clobbering additional registers in 0x200 vector.
+     */
+    return spapr->rtas_addr;
+}
+
 static target_ulong h_logical_load(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                    target_ulong opcode, target_ulong *args)
 {

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

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

index fb8cc63..57199f5 100644
@@ -310,7 +310,9 @@ typedef struct sPAPREnvironment {
 /* Client Architecture support */
 #define KVMPPC_H_CAS            (KVMPPC_HCALL_BASE + 0x2)
 #define KVMPPC_H_RTAS_UPDATE    (KVMPPC_HCALL_BASE + 0x3)
-#define KVMPPC_HCALL_MAX        KVMPPC_H_RTAS_UPDATE
+/* Report Machine Check error */
+#define KVMPPC_H_REPORT_MC_ERR  (KVMPPC_HCALL_BASE + 0x4)
+#define KVMPPC_HCALL_MAX        KVMPPC_H_REPORT_MC_ERR
 
 extern sPAPREnvironment *spapr;
 

Whenever there is a physical memory error due to bit flips, which cannot be corrected by hardware, the error is passed on to the kernel. If the memory address in error belongs to guest address space then guest kernel is responsible to take action. Hence the error is passed on to guest via KVM by invoking 0x200 NMI vector.

However, guest OS, as per PAPR, expects an error log upon such error. This patch registers a new hcall which is issued from 0x200 interrupt vector and builds the error log, copies the error log to rtas space and passes the address of the error log to guest

Enhancement to KVM to perform above functionality is already in upstream kernel.

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

Quoted 16 lines · Aravinda Prasad

Whenever there is a physical memory error due to bit flips, which cannot be corrected by hardware, the error is passed on to the kernel. If the memory address in error belongs to guest address space then guest kernel is responsible to take action. Hence the error is passed on to guest via KVM by invoking 0x200 NMI vector.

However, guest OS, as per PAPR, expects an error log upon such error. This patch registers a new hcall which is issued from 0x200 interrupt vector and builds the error log, copies the error log to rtas space and passes the address of the error log to guest

Enhancement to KVM to perform above functionality is already in upstream kernel.

Quoted 1 line

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

Why do we have to maintain the error log inside the rtas blob? Can't we just create a fixed MMIO region that is backed by an error log device in QEMU?

Then this call would just always return that specific address and we'd also have a single file that deals with all of the error reporting.

Alex

Quoted 25 lines · Alexander Graf

Quoted 13 lines · Aravinda Prasad

Whenever there is a physical memory error due to bit flips, which cannot be corrected by hardware, the error is passed on to the kernel. If the memory address in error belongs to guest address space then guest kernel is responsible to take action. Hence the error is passed on to guest via KVM by invoking 0x200 NMI vector.

However, guest OS, as per PAPR, expects an error log upon such error. This patch registers a new hcall which is issued from 0x200 interrupt vector and builds the error log, copies the error log to rtas space and passes the address of the error log to guest

Quoted 4 lines

Enhancement to KVM to perform above functionality is already in upstream kernel.

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

Why do we have to maintain the error log inside the rtas blob? Can't we just create a fixed MMIO region that is backed by an error log device in QEMU?

Quoted 2 lines

Then this call would just always return that specific address and we'd also have a single file that deals with all of the error reporting.

PAPR requires error log to be inside rtas space. Hence guest OS explicitly checks if error log is inside rtas space.

VALID_FWNMI_BUFFER does this check inside fwnmi_get_errinfo() in the kernel.

So why not put it at 0x7000 then?

Quoted 1 line · Alexander Graf

So why not put it at 0x7000 then?

Because PAPR says it has to be inside RTAS iirc

Quoted 5 lines · Benjamin Herrenschmidt

Quoted 1 line · Alexander Graf

So why not put it at 0x7000 then?

Because PAPR says it has to be inside RTAS iirc

Please show me the section of PAPR that does say so.

Quoted 1 line

Quoted 9 lines · Alexander Graf

Quoted 5 lines · Benjamin Herrenschmidt

Quoted 1 line · Alexander Graf

So why not put it at 0x7000 then?

Because PAPR says it has to be inside RTAS iirc

Please show me the section of PAPR that does say so.

Actually, we should probably support both options:

PAPR V2.7

7.3.14 Firmware Assisted Non-Maskable Interrupts Option (FWNMI)

<<
The difference between ibm,nmi-register and ibm,nmi-register-2 is that ibm,nmi-register allocates the error reporting
structure in RTAS space while ibm,nmi-register-2 places the error reporting structure in real page 7. New OS designs
should use ibm,nmi-register since support for ibm,nmi-register-2 will be terminated at some future date.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Cheers,
Ben.

Quoted 9 lines · Alexander Graf

Quoted 5 lines · Benjamin Herrenschmidt

Quoted 1 line · Alexander Graf

So why not put it at 0x7000 then?

Because PAPR says it has to be inside RTAS iirc

Please show me the section of PAPR that does say so.

Actually, we should probably support both options:

PAPR V2.7

7.3.14 Firmware Assisted Non-Maskable Interrupts Option (FWNMI)

<<
The difference between ibm,nmi-register and ibm,nmi-register-2 is that ibm,nmi-register allocates the error reporting
structure in RTAS space while ibm,nmi-register-2 places the error reporting structure in real page 7. New OS designs
should use ibm,nmi-register since support for ibm,nmi-register-2 will be terminated at some future date.

Quoted 1 line

Quoted 1 line

Cheers,
Ben.

Should we avoid having error log in 0x7000? As per above only ibm,nmi-register-2 places error log in 0x7000 which will be terminated in future?

Quoted 3 lines · Aravinda Prasad

Should we avoid having error log in 0x7000? As per above only ibm,nmi-register-2 places error log in 0x7000 which will be terminated in future?

The question is which variant Linux uses and which variant are other OSes we might be interested in use ?

We might have to implement both, but I would probably start with just ibm,nmi-register

Cheers,
Ben.

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.

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

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

hw/ppc/spapr_rtas.c +13 -0

index 02ddbf9..1135d2b 100644
@@ -277,6 +277,91 @@ static void rtas_ibm_set_system_parameter(PowerPCCPU *cpu,
     rtas_st(rets, 0, ret);
 }
 
+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 branch_inst = 0x48000002;
+    target_ulong guest_machine_check_addr;
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+    /*
+     * Trampoline saves r3 in sprg2 and issues private hcall
+     * to request qemu to build error log. QEMU builds the

hw/ppc/spapr_rtas.c +16 -0

+        0x6063f004,    /* ori     r3,r3,f004  */
+        /* Issue H_CALL */
+        0x44000022,    /*  sc      1     */
+        0x7c9243a6,    /* mtspr r4 sprg2 */
+        0xe8830000,    /* ld r4, 0(r3) */
+        0x7c9a03a6,    /* mtspr r4, srr0 */
+        0xe8830008,    /* ld r4, 8(r3) */
+        0x7c9b03a6,    /* mtspr r4, srr1 */
+        0x38630010,    /* addi r3,r3,16 */
+        0x7c9242a6,    /* mfspr r4 sprg2 */
+        0x48000002,    /* Branch to address registered
+                        * by OS. The branch address is
+                        * patched below */
+        0x48000000,    /* b . */
+    };
+    int total_inst = sizeof(trampoline) / sizeof(uint32_t);

hw/ppc/spapr_rtas.c +19 -0

+    memcpy(&trampoline[11], &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);
+}

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.

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

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

hw/ppc/spapr_rtas.c +19 -0

index 02ddbf9..1135d2b 100644
@@ -277,6 +277,91 @@ static void rtas_ibm_set_system_parameter(PowerPCCPU *cpu,
     rtas_st(rets, 0, ret);
 }
 
+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 branch_inst = 0x48000002;
+    target_ulong guest_machine_check_addr;
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+    /*
+     * 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 rtas-blob consists of saved srr0
+     * and srr1 which we restore and pass on the actual error
+     * log address to OS handled mcachine check notification
+     * routine
+     */

hw/ppc/spapr_rtas.c static void rtas_ibm_set_system_parameter(PowerPCCPU *cpu, · line 302

Quoted 4 lines

hw/ppc/spapr_rtas.c +4 -0

+    uint32_t trampoline[] = {
+        0x7c7243a6,    /* mtspr   SPRN_SPRG2,r3 */
+        0x38600000,    /* li      r3,0   */
+        /* 0xf004 is the KVMPPC_H_REPORT_ERR private HCALL */

You should construct the instruction from the KVMPPC_H_REPORT_ERR constant, otherwise it's an undocumented magic dependency between parts of the code.

Quoted 24 lines

hw/ppc/spapr_rtas.c +23 -0

+        0x6063f004,    /* ori     r3,r3,f004  */
+        /* Issue H_CALL */
+        0x44000022,    /*  sc      1     */
+        0x7c9243a6,    /* mtspr r4 sprg2 */
+        0xe8830000,    /* ld r4, 0(r3) */
+        0x7c9a03a6,    /* mtspr r4, srr0 */
+        0xe8830008,    /* ld r4, 8(r3) */
+        0x7c9b03a6,    /* mtspr r4, srr1 */
+        0x38630010,    /* addi r3,r3,16 */
+        0x7c9242a6,    /* mfspr r4 sprg2 */
+        0x48000002,    /* Branch to address registered
+                        * by OS. The branch address is
+                        * patched below */
+        0x48000000,    /* b . */
+    };
+    int total_inst = sizeof(trampoline) / sizeof(uint32_t);
+
+    /* Store the system reset and machine check address */
+    guest_machine_check_addr = rtas_ld(args, 1);
+
+    /* Safety Check */
+    if (sizeof(trampoline) >= MC_INTERRUPT_VECTOR_SIZE) {
+        fprintf(stderr, "Unable to register ibm,nmi_register: "

hw/ppc/spapr_rtas.c static void rtas_ibm_set_system_parameter(PowerPCCPU *cpu, · line 328

Quoted 3 lines

hw/ppc/spapr_rtas.c +3 -0

+                "Trampoline size exceeded\n");
+        rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);
+        return;

The guest has absolutely no influence over this, so this should be an assert() rather than returning an rtas error. Or better yet, detect the failure at compile time, since it doesn't depend on anything runtime either.

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

Quoted 8 lines

hw/ppc/spapr_rtas.c +7 -0

+    }
+
+    /*
+     * Update the branch instruction in trampoline with the absolute
+     * machine check address requested by OS
+     */
+    branch_inst |= guest_machine_check_addr;

Surely there should be some sanity checking of the address here, or it can clobber any part of that branch instruction.

Quoted 24 lines

hw/ppc/spapr_rtas.c +23 -0

+    memcpy(&trampoline[11], &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,

hw/ppc/spapr_rtas.c static void rtas_ibm_set_system_parameter(PowerPCCPU *cpu, · line 364

Quoted 6 lines

hw/ppc/spapr_rtas.c +6 -0

+                                   target_ulong args,
+                                   uint32_t nret, target_ulong rets)
+{
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+}
+

This nmi_interlock call isn't described in the commit message.

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.

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

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

hw/ppc/spapr_rtas.c +16 -0

index 02ddbf9..1135d2b 100644
@@ -277,6 +277,91 @@ static void rtas_ibm_set_system_parameter(PowerPCCPU *cpu,
     rtas_st(rets, 0, ret);
 }
 
+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 branch_inst = 0x48000002;
+    target_ulong guest_machine_check_addr;
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+    /*
+     * 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 rtas-blob consists of saved srr0
+     * and srr1 which we restore and pass on the actual error

hw/ppc/spapr_rtas.c +15 -0

+        0x38630010,    /* addi r3,r3,16 */
+        0x7c9242a6,    /* mfspr r4 sprg2 */
 
 
+        0x48000002,    /* Branch to address registered
+                        * by OS. The branch address is
+                        * patched below */
+        0x48000000,    /* b . */
+    };
+    int total_inst = sizeof(trampoline) / sizeof(uint32_t);
+
+    /* Store the system reset and machine check address */
+    guest_machine_check_addr = rtas_ld(args, 1);
+
+    /* Safety Check */
+    if (sizeof(trampoline) >= MC_INTERRUPT_VECTOR_SIZE) {
+        fprintf(stderr, "Unable to register ibm,nmi_register: "

hw/ppc/spapr_rtas.c +29 -0

+    memcpy(&trampoline[11], &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)
+{
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+}
+
 static struct rtas_call {

     const char *name;
     spapr_rtas_fn fn;

hw/ppc/spapr_rtas.c +6 -0

@@ -404,6 +489,12 @@ static void core_rtas_register_types(void)
     spapr_rtas_register(RTAS_IBM_SET_SYSTEM_PARAMETER,
                         "ibm,set-system-parameter",
                         rtas_ibm_set_system_parameter);
+    spapr_rtas_register(RTAS_IBM_NMI_REGISTER,
+                        "ibm,nmi-register",
+                        rtas_ibm_nmi_register);
+    spapr_rtas_register(RTAS_IBM_NMI_INTERLOCK,
+                        "ibm,nmi-interlock",
+                        rtas_ibm_nmi_interlock);
 }
 
 type_init(core_rtas_register_types)

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

index 57199f5..8c854ca 100644
@@ -386,8 +386,10 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi);
 #define RTAS_IBM_CONFIGURE_CONNECTOR            (RTAS_TOKEN_BASE + 0x1E)
 #define RTAS_IBM_OS_TERM                        (RTAS_TOKEN_BASE + 0x1F)
 #define RTAS_IBM_EXTENDED_OS_TERM               (RTAS_TOKEN_BASE + 0x20)
+#define RTAS_IBM_NMI_REGISTER                   (RTAS_TOKEN_BASE + 0x21)
+#define RTAS_IBM_NMI_INTERLOCK                  (RTAS_TOKEN_BASE + 0x22)
 
-#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x21)
+#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x23)
 
 /* RTAS ibm,get-system-parameter token values */
 #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS      20

@@ -485,4 +487,8 @@ 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);
 
+/* Machine Check Interrupt related macros */
+#define MC_INTERRUPT_VECTOR           0x200
+#define MC_INTERRUPT_VECTOR_SIZE      0x100
+
 #endif /* !defined (__HW_SPAPR_H__) */