Hi Nick,

On 27/02/25 08:37, Nicholas Piggin wrote:
On Mon Feb 17, 2025 at 5:17 PM AEST, Aditya Gupta wrote:
Implement the handler for "ibm,configure-kernel-dump" rtas call in QEMU.

Currently the handler just does basic checks and handles
register/unregister/invalidate requests from kernel.

Fadump will be enabled in a later patch.

Signed-off-by: Aditya Gupta <adit...@linux.ibm.com>
---
  hw/ppc/spapr_rtas.c    | 99 ++++++++++++++++++++++++++++++++++++++++++
  include/hw/ppc/spapr.h | 59 +++++++++++++++++++++++++
  2 files changed, 158 insertions(+)

diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index df2e837632aa..eebdf13b1552 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -341,6 +341,105 @@ static void rtas_ibm_set_system_parameter(PowerPCCPU *cpu,
      rtas_st(rets, 0, ret);
  }
+struct fadump_metadata fadump_metadata;
Can this (and other globals added in other patches) come under
SpaprMachineState?

And could most of the fadump code and structures go under new
spapr_fadump.[ch] files?
Yes, i can move it inside SpaprMachineState. Will put the code in new files.
+
+/* Papr Section 7.4.9 ibm,configure-kernel-dump RTAS call */
+static __attribute((unused)) void rtas_configure_kernel_dump(PowerPCCPU *cpu,
+                                   SpaprMachineState *spapr,
+                                   uint32_t token, uint32_t nargs,
+                                   target_ulong args,
+                                   uint32_t nret, target_ulong rets)
I don't know about adding a new unused function like this, is there
a way to juggle patches around to add it when it's wired up?

Ah, that is problematic agreed. I tried to move around things, but arrived at this.

I will spend some time thinking how to arrange this.

Will need some guidance. How should I approach arranging the code in such situations ?

My idea was to
* First one is the skeleton: mentions the steps, but doesn't implement the steps
* Middle patches implement the steps one by one
* Last patch enables it all. So in future if someone checks out the "Enable fadump" commit they would have all the support ready.

The major problem is "everything" remains unused till this last patch. But this 1st patch gave me the chance to logically build upon this, eg. first implement preserving memory regions, then add the fadump_trigger in os-term rtas call, etc.

Any advice to approach this ?

+{
+    struct rtas_fadump_section_header header;
+    target_ulong cmd = rtas_ld(args, 0);
+    target_ulong fdm_addr = rtas_ld(args, 1);
+    target_ulong fdm_size = rtas_ld(args, 2);
+
+    /* Number outputs has to be 1 */
+    if (nret != 1) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                "FADUMP: ibm,configure-kernel-dump RTAS called with nret != 
1.\n");
+        return;
+    }
+
+    /* Number inputs has to be 3 */
+    if (nargs != 3) {
+        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
+        return;
+    }
+
+    switch (cmd) {
+    case FADUMP_CMD_REGISTER:
+        if (fadump_metadata.fadump_registered) {
+            /* Fadump already registered */
+            rtas_st(rets, 0, RTAS_OUT_DUMP_ALREADY_REGISTERED);
+            return;
+        }
+
+        if (fadump_metadata.fadump_dump_active == 1) {
+            rtas_st(rets, 0, RTAS_OUT_DUMP_ACTIVE);
+            return;
+        }
+
+        if (fdm_size < sizeof(struct rtas_fadump_section_header)) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                "FADUMP: Header size is invalid: %lu\n", fdm_size);
+            rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
+            return;
+        }
+
+        /* XXX: Can we ensure fdm_addr points to a valid RMR-memory buffer ? */
RMR memory? There is spapr_rma_size() if that's what you need?


Thanks, will use `spapr_rma_size`. The PAPR says fdm_addr should point to a valid RMR buffer, I guess that means it should be in the RMA, ie. `< spapr_rma_size()` ?


- Aditya G


Thanks,
Nick

Reply via email to