* [PATCH v4 0/8] Implement Firmware Assisted Dump for PSeries
@ 2025-03-23 17:39 Aditya Gupta
2025-03-23 17:40 ` [PATCH v4 1/8] hw/ppc: Implement skeleton code for fadump in PSeries Aditya Gupta
` (9 more replies)
0 siblings, 10 replies; 36+ messages in thread
From: Aditya Gupta @ 2025-03-23 17:39 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Harsh Prateek Bora, Sourabh Jain, Mahesh J Salgaonkar,
Hari Bathini
Overview
=========
Implemented Firmware Assisted Dump (fadump) on PSeries machine in QEMU.
Fadump is an alternative dump mechanism to kdump, in which we the firmware
does a memory preserving boot, and the second/crashkernel is booted fresh
like a normal system reset, instead of the crashed kernel loading the
second/crashkernel in case of kdump.
This requires implementing the "ibm,configure-kernel-dump" RTAS call in
QEMU.
While booting with fadump=on, Linux will register fadump memory regions.
Some memory regions like Real Mode Memory regions, and custom memory
regions declared by OS basically require copying the requested memory
range to a destination
While other memory regions are populated by the firmware/platform (QEMU in
this case), such as CPU State Data and HPTE.
We pass the sizes for these data segment to the kernel as it needs to know
how much memory to reserve (ibm,configure-kernel-dump-sizes).
Then after a crash, once Linux does a OS terminate call, we trigger fadump
if fadump was registered.
Implementing the fadump boot as:
* pause all vcpus (will save registers later)
* preserve memory regions specified by fadump
* do a memory preserving reboot (using GUEST_RESET as it doesn't clear
the memory)
And then we pass a metadata (firmware memory structure) as
"ibm,kernel-dump" in the device tree, containing all details of the
preserved memory regions to the kernel.
Refer the Patch #7/8: "hw/ppc: Enable fadump for PSeries" for logs of a
succesfful fadump crash
Note: HPTE region has not been implemented. It's not planned as of now.
Testing
=======
Has been tested with following QEMU options:
* firmware: x-vof and SLOF
* tcg & kvm
* l1 guest and l2 guest
* with/without smp
* cma/nocma
* default crashkernel values (can fail with big initrd) and crashkernel=1G
Git Tree for Testing
====================
https://github.com/adi-g15-ibm/qemu/tree/fadump-pseries-v4
Note: You will need a way to get the /proc/vmcore out of the VM for testing
with crash-utility
I use the following command line which sets up networking:
"-net user,hostfwd=tcp::10022-:22 -net nic"
And a rootfs with ssh support, then copy the /proc/vmcore with networking
(can do compression using gzip before ssh, but compression might take lot
of time if done inside the VM)
Test vmcore for Testing with crash-utility
==========================================
Can use vmlinux and vmcore available at https://github.com/adi-g15-ibm/qemu/releases/tag/test-images-fadump-pseries-v2
Above vmcore was generated with upstream qemu with these fadump patches
applied, and in a KVM VM
A limitation with above vmcore is it was a single CPU VM
Changelog
=========
v4
+ [patch #8/8]: fixed kvm testcase, add license
v3:
+ [patch #3,7]: fix compile errors (#define declared in a later patch
but used in this patch, unused var)
+ [patch #4/8]: use 'g_autofree' for cpu buffer, and replace g_malloc with
g_try_malloc
+ [patch #5/8]: use 'g_new' instead of 'malloc', add null check for cpu
region
- nothing in other patches has been changed compared to v2
v2:
+ rearrange code so that no unused functions get introduced in any patch
+ add functional test for pseries as suggested by nick
+ fix multiple issues pointed by harsh and nick
+ fix bug in cpu register saving where it was being stored in
little-endian
- removed 'is_next_boot_fadump' and used fadump header's status flag to
store it
+ fixed multiple style issues (naming, unneeded diffs etc)
Aditya Gupta (8):
hw/ppc: Implement skeleton code for fadump in PSeries
hw/ppc: Implement fadump register command
hw/ppc: Trigger Fadump boot if fadump is registered
hw/ppc: Preserve memory regions registered for fadump
hw/ppc: Implement saving CPU state in Fadump
hw/ppc: Pass dump-sizes property for fadump in device tree
hw/ppc: Enable fadump for PSeries
tests/functional: Add test for fadump in PSeries
hw/ppc/meson.build | 1 +
hw/ppc/spapr.c | 72 +++
hw/ppc/spapr_fadump.c | 685 ++++++++++++++++++++++
hw/ppc/spapr_rtas.c | 71 +++
include/hw/ppc/spapr.h | 11 +-
include/hw/ppc/spapr_fadump.h | 121 ++++
tests/functional/meson.build | 2 +
tests/functional/qemu_test/linuxkernel.py | 59 ++
tests/functional/test_ppc64_fadump.py | 182 ++++++
9 files changed, 1203 insertions(+), 1 deletion(-)
create mode 100644 hw/ppc/spapr_fadump.c
create mode 100644 include/hw/ppc/spapr_fadump.h
create mode 100755 tests/functional/test_ppc64_fadump.py
--
2.49.0
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v4 1/8] hw/ppc: Implement skeleton code for fadump in PSeries
2025-03-23 17:39 [PATCH v4 0/8] Implement Firmware Assisted Dump for PSeries Aditya Gupta
@ 2025-03-23 17:40 ` Aditya Gupta
2025-04-21 10:51 ` Harsh Prateek Bora
` (2 more replies)
2025-03-23 17:40 ` [PATCH v4 2/8] hw/ppc: Implement fadump register command Aditya Gupta
` (8 subsequent siblings)
9 siblings, 3 replies; 36+ messages in thread
From: Aditya Gupta @ 2025-03-23 17:40 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Harsh Prateek Bora, Sourabh Jain, Mahesh J Salgaonkar,
Hari Bathini
Add skeleton for handle "ibm,configure-kernel-dump" rtas call in QEMU.
Verify basic details mandated by the PAPR, such as number of
inputs/output, and add handling for the three fadump commands:
regiser/unregister/invalidate.
Currently fadump register will always return HARDWARE ERROR, since it's
not implemented yet. So if the kernel's attempt to register fadump will
itself fail as the support is not there yet in QEMU.
The checks are based on the table in following requirement in PAPR v2.13:
"R1–7.3.30–1. For the Configure Platform Assisted Kernel Dump option ..."
Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
---
hw/ppc/meson.build | 1 +
hw/ppc/spapr_fadump.c | 22 +++++++++++
hw/ppc/spapr_rtas.c | 66 +++++++++++++++++++++++++++++++++
include/hw/ppc/spapr.h | 11 +++++-
include/hw/ppc/spapr_fadump.h | 69 +++++++++++++++++++++++++++++++++++
5 files changed, 168 insertions(+), 1 deletion(-)
create mode 100644 hw/ppc/spapr_fadump.c
create mode 100644 include/hw/ppc/spapr_fadump.h
diff --git a/hw/ppc/meson.build b/hw/ppc/meson.build
index 9893f8adebb0..863972741b15 100644
--- a/hw/ppc/meson.build
+++ b/hw/ppc/meson.build
@@ -26,6 +26,7 @@ ppc_ss.add(when: 'CONFIG_PSERIES', if_true: files(
'spapr_nvdimm.c',
'spapr_rtas_ddw.c',
'spapr_numa.c',
+ 'spapr_fadump.c',
'pef.c',
))
ppc_ss.add(when: ['CONFIG_PSERIES', 'CONFIG_TCG'], if_true: files(
diff --git a/hw/ppc/spapr_fadump.c b/hw/ppc/spapr_fadump.c
new file mode 100644
index 000000000000..20b7b804c485
--- /dev/null
+++ b/hw/ppc/spapr_fadump.c
@@ -0,0 +1,22 @@
+/*
+ * Firmware Assisted Dump in PSeries
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "hw/ppc/spapr.h"
+
+/*
+ * Handle the "FADUMP_CMD_REGISTER" command in 'ibm,configure-kernel-dump'
+ *
+ * Returns:
+ * * RTAS_OUT_HW_ERROR: Not implemented/Misc issue such as memory access
+ * failures
+ */
+uint32_t do_fadump_register(void)
+{
+ /* WIP: FADUMP_CMD_REGISTER implemented in future patch */
+
+ return RTAS_OUT_HW_ERROR;
+}
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 503d441b48e4..b8bfa9c33fb5 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -341,6 +341,68 @@ static void rtas_ibm_set_system_parameter(PowerPCCPU *cpu,
rtas_st(rets, 0, ret);
}
+/* Papr Section 7.4.9 ibm,configure-kernel-dump RTAS call */
+static void rtas_configure_kernel_dump(PowerPCCPU *cpu,
+ SpaprMachineState *spapr,
+ uint32_t token, uint32_t nargs,
+ target_ulong args,
+ uint32_t nret, target_ulong rets)
+{
+ target_ulong cmd = rtas_ld(args, 0);
+ uint32_t ret_val;
+
+ /* Number of 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 of inputs has to be 3 */
+ if (nargs != 3) {
+ rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
+ return;
+ }
+
+ switch (cmd) {
+ case FADUMP_CMD_REGISTER:
+ ret_val = do_fadump_register();
+ if (ret_val != RTAS_OUT_SUCCESS) {
+ rtas_st(rets, 0, ret_val);
+ return;
+ }
+ break;
+ case FADUMP_CMD_UNREGISTER:
+ if (spapr->fadump_dump_active == 1) {
+ rtas_st(rets, 0, RTAS_OUT_DUMP_ACTIVE);
+ return;
+ }
+
+ spapr->fadump_registered = false;
+ spapr->fadump_dump_active = false;
+ memset(&spapr->registered_fdm, 0, sizeof(spapr->registered_fdm));
+ break;
+ case FADUMP_CMD_INVALIDATE:
+ if (spapr->fadump_dump_active) {
+ spapr->fadump_registered = false;
+ spapr->fadump_dump_active = false;
+ memset(&spapr->registered_fdm, 0, sizeof(spapr->registered_fdm));
+ } else {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "FADump: Nothing to invalidate, no dump active\n");
+ }
+ break;
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "FADump: Unknown command: %lu\n", cmd);
+
+ rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
+ return;
+ }
+
+ rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+}
+
static void rtas_ibm_os_term(PowerPCCPU *cpu,
SpaprMachineState *spapr,
uint32_t token, uint32_t nargs,
@@ -656,6 +718,10 @@ static void core_rtas_register_types(void)
spapr_rtas_register(RTAS_IBM_NMI_INTERLOCK, "ibm,nmi-interlock",
rtas_ibm_nmi_interlock);
+ /* Register fadump rtas call */
+ spapr_rtas_register(RTAS_CONFIGURE_KERNEL_DUMP, "ibm,configure-kernel-dump",
+ rtas_configure_kernel_dump);
+
qtest_set_command_cb(spapr_qtest_callback);
}
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 39bd5bd5ed31..4c1636497e30 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -13,6 +13,7 @@
#include "hw/ppc/xics.h" /* For ICSState */
#include "hw/ppc/spapr_tpm_proxy.h"
#include "hw/ppc/spapr_nested.h" /* For SpaprMachineStateNested */
+#include "hw/ppc/spapr_fadump.h" /* For FadumpMemStruct */
struct SpaprVioBus;
struct SpaprPhbState;
@@ -283,6 +284,11 @@ struct SpaprMachineState {
Error *fwnmi_migration_blocker;
SpaprWatchdog wds[WDT_MAX_WATCHDOGS];
+
+ /* Fadump State */
+ bool fadump_registered;
+ bool fadump_dump_active;
+ FadumpMemStruct registered_fdm;
};
#define H_SUCCESS 0
@@ -708,6 +714,8 @@ void push_sregs_to_kvm_pr(SpaprMachineState *spapr);
#define RTAS_OUT_PARAM_ERROR -3
#define RTAS_OUT_NOT_SUPPORTED -3
#define RTAS_OUT_NO_SUCH_INDICATOR -3
+#define RTAS_OUT_DUMP_ALREADY_REGISTERED -9
+#define RTAS_OUT_DUMP_ACTIVE -10
#define RTAS_OUT_NOT_AUTHORIZED -9002
#define RTAS_OUT_SYSPARM_PARAM_ERROR -9999
@@ -770,8 +778,9 @@ void push_sregs_to_kvm_pr(SpaprMachineState *spapr);
#define RTAS_IBM_SUSPEND_ME (RTAS_TOKEN_BASE + 0x2A)
#define RTAS_IBM_NMI_REGISTER (RTAS_TOKEN_BASE + 0x2B)
#define RTAS_IBM_NMI_INTERLOCK (RTAS_TOKEN_BASE + 0x2C)
+#define RTAS_CONFIGURE_KERNEL_DUMP (RTAS_TOKEN_BASE + 0x2D)
-#define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x2D)
+#define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x2E)
/* RTAS ibm,get-system-parameter token values */
#define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS 20
diff --git a/include/hw/ppc/spapr_fadump.h b/include/hw/ppc/spapr_fadump.h
new file mode 100644
index 000000000000..45109fd9e137
--- /dev/null
+++ b/include/hw/ppc/spapr_fadump.h
@@ -0,0 +1,69 @@
+/*
+ * Firmware Assisted Dump in PSeries
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#ifndef PPC_SPAPR_FADUMP_H
+#define PPC_SPAPR_FADUMP_H
+
+#include "qemu/osdep.h"
+#include "cpu.h"
+
+/* Fadump commands */
+#define FADUMP_CMD_REGISTER 1
+#define FADUMP_CMD_UNREGISTER 2
+#define FADUMP_CMD_INVALIDATE 3
+
+#define FADUMP_VERSION 1
+
+/*
+ * The Firmware Assisted Dump Memory structure supports a maximum of 10 sections
+ * in the dump memory structure. Presently, three sections are used for
+ * CPU state data, HPTE & Parameters area, while the remaining seven sections
+ * can be used for boot memory regions.
+ */
+#define FADUMP_MAX_SECTIONS 10
+#define RTAS_FADUMP_MAX_BOOT_MEM_REGS 7
+
+typedef struct FadumpSection FadumpSection;
+typedef struct FadumpSectionHeader FadumpSectionHeader;
+typedef struct FadumpMemStruct FadumpMemStruct;
+
+struct SpaprMachineState;
+
+/* Kernel Dump section info */
+struct FadumpSection {
+ __be32 request_flag;
+ __be16 source_data_type;
+ __be16 error_flags;
+ __be64 source_address;
+ __be64 source_len;
+ __be64 bytes_dumped;
+ __be64 destination_address;
+};
+
+/* ibm,configure-kernel-dump header. */
+struct FadumpSectionHeader {
+ __be32 dump_format_version;
+ __be16 dump_num_sections;
+ __be16 dump_status_flag;
+ __be32 offset_first_dump_section;
+
+ /* Fields for disk dump option. */
+ __be32 dd_block_size;
+ __be64 dd_block_offset;
+ __be64 dd_num_blocks;
+ __be32 dd_offset_disk_path;
+
+ /* Maximum time allowed to prevent an automatic dump-reboot. */
+ __be32 max_time_auto;
+};
+
+/* Note: All the data in these structures is in big-endian */
+struct FadumpMemStruct {
+ FadumpSectionHeader header;
+ FadumpSection rgn[FADUMP_MAX_SECTIONS];
+};
+
+uint32_t do_fadump_register(void);
+#endif /* PPC_SPAPR_FADUMP_H */
--
2.49.0
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v4 2/8] hw/ppc: Implement fadump register command
2025-03-23 17:39 [PATCH v4 0/8] Implement Firmware Assisted Dump for PSeries Aditya Gupta
2025-03-23 17:40 ` [PATCH v4 1/8] hw/ppc: Implement skeleton code for fadump in PSeries Aditya Gupta
@ 2025-03-23 17:40 ` Aditya Gupta
2025-10-17 9:54 ` Sourabh Jain
2025-03-23 17:40 ` [PATCH v4 3/8] hw/ppc: Trigger Fadump boot if fadump is registered Aditya Gupta
` (7 subsequent siblings)
9 siblings, 1 reply; 36+ messages in thread
From: Aditya Gupta @ 2025-03-23 17:40 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Harsh Prateek Bora, Sourabh Jain, Mahesh J Salgaonkar,
Hari Bathini
Implement the register command of "ibm,configure-kernel-dump" RTAS call.
The register just verifies the structure of the fadump memory structure
passed by kernel, and set fadump_registered in spapr state to true.
We also store the passed fadump memory structure, which will later be
used for preserving memory for fadump boot in case of a crash.
The fadump memory structure isn't modified (other than .dump_status_flag
after the fadump is triggered, that is in a later patch).
So if the structure needs to updated, the kernel should first
de-register and re-register the structure again.
Relevant section for the register command in PAPR is:
Section 7.3.30: "ibm,configure-kernel-dump RTAS call" (PAPR v2.13)
Note: The fadump registration is done, but triggering fadump on an
os-term rtas call is done in later patches. Hence QEMU will just shutdown
on a kernel crash due to no special handling for fadump in ibm,os-term
Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
---
hw/ppc/spapr_fadump.c | 111 ++++++++++++++++++++++++++++++++--
hw/ppc/spapr_rtas.c | 2 +-
include/hw/ppc/spapr_fadump.h | 2 +-
3 files changed, 108 insertions(+), 7 deletions(-)
diff --git a/hw/ppc/spapr_fadump.c b/hw/ppc/spapr_fadump.c
index 20b7b804c485..9c7fb9e12b16 100644
--- a/hw/ppc/spapr_fadump.c
+++ b/hw/ppc/spapr_fadump.c
@@ -5,18 +5,119 @@
*/
#include "qemu/osdep.h"
+#include "qemu/log.h"
#include "hw/ppc/spapr.h"
/*
* Handle the "FADUMP_CMD_REGISTER" command in 'ibm,configure-kernel-dump'
*
+ * Note: Any changes made by the kernel to the fadump memory struct won't
+ * reflect in QEMU after the 'ibm,configure-kernel-dump' RTAS call has returned,
+ * as we store the passed fadump memory structure passed during fadump
+ * registration.
+ * Kernel has to invalidate & re-register fadump, if it intends to make any
+ * changes to the fadump memory structure
+ *
* Returns:
- * * RTAS_OUT_HW_ERROR: Not implemented/Misc issue such as memory access
- * failures
+ * * RTAS_OUT_SUCCESS: On successful registration
+ * * RTAS_OUT_PARAM_ERROR: If parameters are not correct, eg. too many
+ * sections, invalid memory addresses that we are
+ * unable to read, etc
+ * * RTAS_OUT_DUMP_ALREADY_REGISTERED: Dump already registered
+ * * RTAS_OUT_HW_ERROR: Misc issue such as memory access failures
*/
-uint32_t do_fadump_register(void)
+uint32_t do_fadump_register(SpaprMachineState *spapr, target_ulong args)
{
- /* WIP: FADUMP_CMD_REGISTER implemented in future patch */
+ FadumpSectionHeader header;
+ FadumpSection regions[FADUMP_MAX_SECTIONS] = {0};
+ target_ulong fdm_addr = rtas_ld(args, 1);
+ target_ulong fdm_size = rtas_ld(args, 2);
+ AddressSpace *default_as = &address_space_memory;
+ MemTxResult io_result;
+ MemTxAttrs attrs;
+ uint64_t next_section_addr;
+ uint16_t dump_num_sections;
+
+ /* Mark the memory transaction as privileged memory access */
+ attrs.user = 0;
+ attrs.memory = 1;
+
+ if (spapr->fadump_registered) {
+ /* FADump already registered */
+ return RTAS_OUT_DUMP_ALREADY_REGISTERED;
+ }
+
+ if (spapr->fadump_dump_active == 1) {
+ return RTAS_OUT_DUMP_ACTIVE;
+ }
+
+ if (fdm_size < sizeof(FadumpSectionHeader)) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "FADump: Header size is invalid: %lu\n", fdm_size);
+ return RTAS_OUT_PARAM_ERROR;
+ }
+
+ /* Ensure fdm_addr points to a valid RMR-memory/RMA-memory buffer */
+ if ((fdm_addr <= 0) || ((fdm_addr + fdm_size) > spapr->rma_size)) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "FADump: Invalid fdm address: %ld\n", fdm_addr);
+ return RTAS_OUT_PARAM_ERROR;
+ }
+
+ /* Try to read the passed fadump header */
+ io_result = address_space_read(default_as, fdm_addr, attrs,
+ &header, sizeof(header));
+ if (io_result != MEMTX_OK) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "FADump: Unable to read fdm address: %ld\n", fdm_addr);
+
+ return RTAS_OUT_HW_ERROR;
+ }
+
+ /* Verify that we understand the fadump header version */
+ if (header.dump_format_version != cpu_to_be32(FADUMP_VERSION)) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "FADump: Unknown fadump header version: 0x%x\n",
+ header.dump_format_version);
+ return RTAS_OUT_PARAM_ERROR;
+ }
+
+ /* Reset dump status flags */
+ header.dump_status_flag = 0;
+
+ dump_num_sections = be16_to_cpu(header.dump_num_sections);
+
+ if (dump_num_sections > FADUMP_MAX_SECTIONS) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "FADump: Too many sections: %d sections\n", dump_num_sections);
+ return RTAS_OUT_PARAM_ERROR;
+ }
+
+ next_section_addr =
+ fdm_addr +
+ be32_to_cpu(header.offset_first_dump_section);
+
+ for (int i = 0; i < dump_num_sections; ++i) {
+ /* Read the fadump section from memory */
+ io_result = address_space_read(default_as, next_section_addr, attrs,
+ ®ions[i], sizeof(regions[i]));
+ if (io_result != MEMTX_OK) {
+ qemu_log_mask(LOG_UNIMP,
+ "FADump: Unable to read fadump %dth section\n", i);
+ return RTAS_OUT_PARAM_ERROR;
+ }
+
+ next_section_addr += sizeof(regions[i]);
+ }
+
+ spapr->fadump_registered = true;
+ spapr->fadump_dump_active = false;
+
+ /* Store the registered fadump memory struct */
+ spapr->registered_fdm.header = header;
+ for (int i = 0; i < dump_num_sections; ++i) {
+ spapr->registered_fdm.rgn[i] = regions[i];
+ }
- return RTAS_OUT_HW_ERROR;
+ return RTAS_OUT_SUCCESS;
}
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index b8bfa9c33fb5..0454938a01e9 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -366,7 +366,7 @@ static void rtas_configure_kernel_dump(PowerPCCPU *cpu,
switch (cmd) {
case FADUMP_CMD_REGISTER:
- ret_val = do_fadump_register();
+ ret_val = do_fadump_register(spapr, args);
if (ret_val != RTAS_OUT_SUCCESS) {
rtas_st(rets, 0, ret_val);
return;
diff --git a/include/hw/ppc/spapr_fadump.h b/include/hw/ppc/spapr_fadump.h
index 45109fd9e137..6abbcb44f353 100644
--- a/include/hw/ppc/spapr_fadump.h
+++ b/include/hw/ppc/spapr_fadump.h
@@ -65,5 +65,5 @@ struct FadumpMemStruct {
FadumpSection rgn[FADUMP_MAX_SECTIONS];
};
-uint32_t do_fadump_register(void);
+uint32_t do_fadump_register(struct SpaprMachineState *, target_ulong);
#endif /* PPC_SPAPR_FADUMP_H */
--
2.49.0
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v4 3/8] hw/ppc: Trigger Fadump boot if fadump is registered
2025-03-23 17:39 [PATCH v4 0/8] Implement Firmware Assisted Dump for PSeries Aditya Gupta
2025-03-23 17:40 ` [PATCH v4 1/8] hw/ppc: Implement skeleton code for fadump in PSeries Aditya Gupta
2025-03-23 17:40 ` [PATCH v4 2/8] hw/ppc: Implement fadump register command Aditya Gupta
@ 2025-03-23 17:40 ` Aditya Gupta
2025-10-17 11:26 ` Sourabh Jain
2025-03-23 17:40 ` [PATCH v4 4/8] hw/ppc: Preserve memory regions registered for fadump Aditya Gupta
` (6 subsequent siblings)
9 siblings, 1 reply; 36+ messages in thread
From: Aditya Gupta @ 2025-03-23 17:40 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Harsh Prateek Bora, Sourabh Jain, Mahesh J Salgaonkar,
Hari Bathini
According to PAPR:
R1–7.3.30–3. When the platform receives an ibm,os-term RTAS call, or
on a system reset without an ibm,nmi-interlock RTAS call, if the
platform has a dump structure registered through the
ibm,configure-kernel-dump call, the platform must process each
registered kernel dump section as required and, when available,
present the dump structure information to the operating system
through the “ibm,kernel-dump” property, updated with status for each
dump section, until the dump has been invalidated through the
ibm,configure-kernel-dump RTAS call.
If Fadump has been registered, trigger an Fadump boot (memory preserving
boot), if QEMU recieves a 'ibm,os-term' rtas call.
Implementing the fadump boot as:
* pause all vcpus (will need to save registers later)
* preserve memory regions specified by fadump (will be implemented
in future)
* do a memory preserving reboot (GUEST_RESET in QEMU doesn't clear
the memory)
Memory regions registered by fadump will be handled in a later patch.
Note: Preserving memory regions is not implemented yet so on an
"ibm,os-term" call will just trigger a reboot in QEMU if fadump is
registered, and the second kernel will boot as a normal boot (not
fadump boot)
Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
---
hw/ppc/spapr_fadump.c | 77 +++++++++++++++++++++++++++++++++++
hw/ppc/spapr_rtas.c | 5 +++
include/hw/ppc/spapr_fadump.h | 6 +++
3 files changed, 88 insertions(+)
diff --git a/hw/ppc/spapr_fadump.c b/hw/ppc/spapr_fadump.c
index 9c7fb9e12b16..fedd2cde9a4f 100644
--- a/hw/ppc/spapr_fadump.c
+++ b/hw/ppc/spapr_fadump.c
@@ -7,6 +7,7 @@
#include "qemu/osdep.h"
#include "qemu/log.h"
#include "hw/ppc/spapr.h"
+#include "system/cpus.h"
/*
* Handle the "FADUMP_CMD_REGISTER" command in 'ibm,configure-kernel-dump'
@@ -121,3 +122,79 @@ uint32_t do_fadump_register(SpaprMachineState *spapr, target_ulong args)
return RTAS_OUT_SUCCESS;
}
+
+/* Preserve the memory locations registered for fadump */
+static bool fadump_preserve_mem(void)
+{
+ /*
+ * TODO: Implement preserving memory regions requested during fadump
+ * registration
+ */
+ return false;
+}
+
+/*
+ * Trigger a fadump boot, ie. next boot will be a crashkernel/fadump boot
+ * with fadump dump active.
+ *
+ * This is triggered by ibm,os-term RTAS call, if fadump was registered.
+ *
+ * It preserves the memory and sets 'FADUMP_STATUS_DUMP_TRIGGERED' as
+ * fadump status, which can be used later to add the "ibm,kernel-dump"
+ * device tree node as presence of 'FADUMP_STATUS_DUMP_TRIGGERED' signifies
+ * next boot as fadump boot in our case
+ */
+void trigger_fadump_boot(SpaprMachineState *spapr, target_ulong spapr_retcode)
+{
+ FadumpSectionHeader *header = &spapr->registered_fdm.header;
+
+ pause_all_vcpus();
+
+ /* Preserve the memory locations registered for fadump */
+ if (!fadump_preserve_mem()) {
+ /* Failed to preserve the registered memory regions */
+ rtas_st(spapr_retcode, 0, RTAS_OUT_HW_ERROR);
+
+ /* Cause a reboot */
+ qemu_system_guest_panicked(NULL);
+ return;
+ }
+
+ /*
+ * Mark next boot as fadump boot
+ *
+ * Note: These is some bit of assumption involved here, as PAPR doesn't
+ * specify any use of the dump status flags, nor does the kernel use it
+ *
+ * But from description in Table 136 in PAPR v2.13, it looks like:
+ * FADUMP_STATUS_DUMP_TRIGGERED
+ * = Dump was triggered by the previous system boot (PAPR says)
+ * = Next boot will be a fadump boot (My perception)
+ *
+ * FADUMP_STATUS_DUMP_PERFORMED
+ * = Dump performed (Set to 0 by caller of the
+ * ibm,configure-kernel-dump call) (PAPR says)
+ * = Firmware has performed the copying/dump of requested regions
+ * (My perception)
+ * = Dump is active for the next boot (My perception)
+ */
+ header->dump_status_flag = cpu_to_be16(
+ FADUMP_STATUS_DUMP_TRIGGERED | /* Next boot will be fadump boot */
+ FADUMP_STATUS_DUMP_PERFORMED /* Dump is active */
+ );
+
+ /* Reset fadump_registered for next boot */
+ spapr->fadump_registered = false;
+ spapr->fadump_dump_active = true;
+
+ /*
+ * Then do a guest reset
+ *
+ * Requirement:
+ * GUEST_RESET is expected to NOT clear the memory, as is the case when
+ * this is merged
+ */
+ qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+
+ rtas_st(spapr_retcode, 0, RTAS_OUT_SUCCESS);
+}
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 0454938a01e9..14b954a05109 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -412,6 +412,11 @@ static void rtas_ibm_os_term(PowerPCCPU *cpu,
target_ulong msgaddr = rtas_ld(args, 0);
char msg[512];
+ if (spapr->fadump_registered) {
+ /* If fadump boot works, control won't come back here */
+ return trigger_fadump_boot(spapr, rets);
+ }
+
cpu_physical_memory_read(msgaddr, msg, sizeof(msg) - 1);
msg[sizeof(msg) - 1] = 0;
diff --git a/include/hw/ppc/spapr_fadump.h b/include/hw/ppc/spapr_fadump.h
index 6abbcb44f353..e484604c1c70 100644
--- a/include/hw/ppc/spapr_fadump.h
+++ b/include/hw/ppc/spapr_fadump.h
@@ -16,6 +16,11 @@
#define FADUMP_VERSION 1
+/* Dump status flags */
+#define FADUMP_STATUS_DUMP_PERFORMED 0x8000
+#define FADUMP_STATUS_DUMP_TRIGGERED 0x4000
+#define FADUMP_STATUS_DUMP_ERROR 0x2000
+
/*
* The Firmware Assisted Dump Memory structure supports a maximum of 10 sections
* in the dump memory structure. Presently, three sections are used for
@@ -66,4 +71,5 @@ struct FadumpMemStruct {
};
uint32_t do_fadump_register(struct SpaprMachineState *, target_ulong);
+void trigger_fadump_boot(struct SpaprMachineState *, target_ulong);
#endif /* PPC_SPAPR_FADUMP_H */
--
2.49.0
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v4 4/8] hw/ppc: Preserve memory regions registered for fadump
2025-03-23 17:39 [PATCH v4 0/8] Implement Firmware Assisted Dump for PSeries Aditya Gupta
` (2 preceding siblings ...)
2025-03-23 17:40 ` [PATCH v4 3/8] hw/ppc: Trigger Fadump boot if fadump is registered Aditya Gupta
@ 2025-03-23 17:40 ` Aditya Gupta
2025-10-17 13:06 ` Sourabh Jain
2025-03-23 17:40 ` [PATCH v4 5/8] hw/ppc: Implement saving CPU state in Fadump Aditya Gupta
` (5 subsequent siblings)
9 siblings, 1 reply; 36+ messages in thread
From: Aditya Gupta @ 2025-03-23 17:40 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Harsh Prateek Bora, Sourabh Jain, Mahesh J Salgaonkar,
Hari Bathini
While the first kernel boots, it registers memory regions for fadump
such as:
* CPU state data (has to be populated by the platform)
* HPTE state data (has to be populated by the platform)
* Real Mode Regions (platform should copy it to requested
destination addresses)
* OS defined regions (such as parameter save area)
Platform is also expected to modify the 'bytes_dumped' to the length of
data preserved/copied by platform (ideally same as the source length
passed by kernel).
The kernel passes source address and length for the memory regions, and
a destination address to where the memory is to be copied.
Implement the preserving/copying of the Real Mode Regions and the
Parameter Save Area in QEMU Pseries
Note: As of this patch, the "kernel-dump" device tree entry is still not
added for the second boot, so after crash, the second kernel will boot
assuming fadump dump is "NOT" active, and try to register for fadump,
but since we already have fadump registered in QEMU internal state, the
register rtas call will fail with: "DUMP ACTIVE"
Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
---
hw/ppc/spapr_fadump.c | 161 ++++++++++++++++++++++++++++++++--
include/hw/ppc/spapr_fadump.h | 18 ++++
2 files changed, 174 insertions(+), 5 deletions(-)
diff --git a/hw/ppc/spapr_fadump.c b/hw/ppc/spapr_fadump.c
index fedd2cde9a4f..c105b8d21da5 100644
--- a/hw/ppc/spapr_fadump.c
+++ b/hw/ppc/spapr_fadump.c
@@ -123,14 +123,165 @@ uint32_t do_fadump_register(SpaprMachineState *spapr, target_ulong args)
return RTAS_OUT_SUCCESS;
}
+/*
+ * Copy the source region of given fadump section, to the destination
+ * address mentioned in the region
+ *
+ * Also set the region's error flag, if the copy fails due to non-existent
+ * address (MEMTX_DECODE_ERROR) or permission issues (MEMTX_ACCESS_ERROR)
+ *
+ * Returns true if successful copy
+ *
+ * Returns false in case of any other error, being treated as hardware
+ * error for fadump purposes
+ */
+static bool do_preserve_region(FadumpSection *region)
+{
+ AddressSpace *default_as = &address_space_memory;
+ MemTxResult io_result;
+ MemTxAttrs attrs;
+ uint64_t src_addr, src_len, dest_addr;
+ g_autofree void *copy_buffer = NULL;
+
+ src_addr = be64_to_cpu(region->source_address);
+ src_len = be64_to_cpu(region->source_len);
+ dest_addr = be64_to_cpu(region->destination_address);
+
+ /* Mark the memory transaction as privileged memory access */
+ attrs.user = 0;
+ attrs.memory = 1;
+
+ /*
+ * Optimisation: Skip copy if source and destination are same
+ * (eg. param area)
+ */
+ if (src_addr != dest_addr) {
+ /*
+ * Using 'g_try_malloc' as the source length is coming from kernel,
+ * which can be invalid/huge, due to which allocation can fail
+ */
+ copy_buffer = g_try_malloc(src_len + 1);
+ if (copy_buffer == NULL) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "FADump: Failed allocating memory (size: %ld) for copying"
+ " reserved memory regions\n", src_len);
+
+ return false;
+ }
+
+ /* Copy the source region to destination */
+ io_result = address_space_read(default_as, src_addr, attrs,
+ copy_buffer, src_len);
+ if ((io_result & MEMTX_DECODE_ERROR) ||
+ (io_result & MEMTX_ACCESS_ERROR)) {
+ /*
+ * Invalid source address is not an hardware error, instead
+ * wrong parameter from the kernel.
+ * Return true to let caller know to continue reading other
+ * sections
+ */
+ region->error_flags = FADUMP_ERROR_INVALID_SOURCE_ADDR;
+ region->bytes_dumped = 0;
+ return true;
+ } else if (io_result != MEMTX_OK) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "FADump: Failed to read source region in section: %d\n",
+ region->source_data_type);
+
+ return false;
+ }
+
+ io_result = address_space_write(default_as, dest_addr, attrs,
+ copy_buffer, src_len);
+ if ((io_result & MEMTX_DECODE_ERROR) ||
+ (io_result & MEMTX_ACCESS_ERROR)) {
+ /*
+ * Invalid destination address is not an hardware error,
+ * instead wrong parameter from the kernel.
+ * Return true to let caller know to continue reading other
+ * sections
+ */
+ region->error_flags = FADUMP_ERROR_INVALID_DEST_ADDR;
+ region->bytes_dumped = 0;
+ return true;
+ } else if (io_result != MEMTX_OK) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "FADump: Failed to write destination in section: %d\n",
+ region->source_data_type);
+
+ return false;
+ }
+ }
+
+ /*
+ * Considering address_space_write would have copied the
+ * complete region
+ */
+ region->bytes_dumped = cpu_to_be64(src_len);
+ return true;
+}
+
/* Preserve the memory locations registered for fadump */
-static bool fadump_preserve_mem(void)
+static bool fadump_preserve_mem(SpaprMachineState *spapr)
{
+ FadumpMemStruct *fdm = &spapr->registered_fdm;
+ uint16_t dump_num_sections, data_type;
+
+ assert(spapr->fadump_registered);
+
/*
- * TODO: Implement preserving memory regions requested during fadump
- * registration
+ * Handle all sections
+ *
+ * CPU State Data and HPTE regions are handled in their own cases
+ *
+ * RMR regions and any custom OS reserved regions such as parameter
+ * save area, are handled by simply copying the source region to
+ * destination address
*/
- return false;
+ dump_num_sections = be16_to_cpu(fdm->header.dump_num_sections);
+ for (int i = 0; i < dump_num_sections; ++i) {
+ data_type = be16_to_cpu(fdm->rgn[i].source_data_type);
+
+ /* Reset error_flags & bytes_dumped for now */
+ fdm->rgn[i].error_flags = 0;
+ fdm->rgn[i].bytes_dumped = 0;
+
+ /* If kernel did not request for the memory region, then skip it */
+ if (be32_to_cpu(fdm->rgn[i].request_flag) != FADUMP_REQUEST_FLAG) {
+ qemu_log_mask(LOG_UNIMP,
+ "FADump: Skipping copying region as not requested\n");
+ continue;
+ }
+
+ switch (data_type) {
+ case FADUMP_CPU_STATE_DATA:
+ /* TODO: Add CPU state data */
+ break;
+ case FADUMP_HPTE_REGION:
+ /* TODO: Add hpte state data */
+ break;
+ case FADUMP_REAL_MODE_REGION:
+ case FADUMP_PARAM_AREA:
+ /* Copy the memory region from region's source to its destination */
+ if (!do_preserve_region(&fdm->rgn[i])) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "FADump: Failed to preserve dump section: %d\n",
+ be16_to_cpu(fdm->rgn[i].source_data_type));
+ fdm->header.dump_status_flag |=
+ cpu_to_be16(FADUMP_STATUS_DUMP_ERROR);
+ }
+
+ break;
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "FADump: Skipping unknown source data type: %d\n", data_type);
+
+ fdm->rgn[i].error_flags =
+ cpu_to_be16(FADUMP_ERROR_INVALID_DATA_TYPE);
+ }
+ }
+
+ return true;
}
/*
@@ -151,7 +302,7 @@ void trigger_fadump_boot(SpaprMachineState *spapr, target_ulong spapr_retcode)
pause_all_vcpus();
/* Preserve the memory locations registered for fadump */
- if (!fadump_preserve_mem()) {
+ if (!fadump_preserve_mem(spapr)) {
/* Failed to preserve the registered memory regions */
rtas_st(spapr_retcode, 0, RTAS_OUT_HW_ERROR);
diff --git a/include/hw/ppc/spapr_fadump.h b/include/hw/ppc/spapr_fadump.h
index e484604c1c70..d56ca1d6d651 100644
--- a/include/hw/ppc/spapr_fadump.h
+++ b/include/hw/ppc/spapr_fadump.h
@@ -16,11 +16,29 @@
#define FADUMP_VERSION 1
+/* Firmware provided dump sections */
+#define FADUMP_CPU_STATE_DATA 0x0001
+#define FADUMP_HPTE_REGION 0x0002
+#define FADUMP_REAL_MODE_REGION 0x0011
+
+/* OS defined sections */
+#define FADUMP_PARAM_AREA 0x0100
+
+/* Dump request flag */
+#define FADUMP_REQUEST_FLAG 0x00000001
+
/* Dump status flags */
#define FADUMP_STATUS_DUMP_PERFORMED 0x8000
#define FADUMP_STATUS_DUMP_TRIGGERED 0x4000
#define FADUMP_STATUS_DUMP_ERROR 0x2000
+/* Region dump error flags */
+#define FADUMP_ERROR_INVALID_DATA_TYPE 0x8000
+#define FADUMP_ERROR_INVALID_SOURCE_ADDR 0x4000
+#define FADUMP_ERROR_LENGTH_EXCEEDS_SOURCE 0x2000
+#define FADUMP_ERROR_INVALID_DEST_ADDR 0x1000
+#define FAUDMP_ERROR_DEST_TOO_SMALL 0x0800
+
/*
* The Firmware Assisted Dump Memory structure supports a maximum of 10 sections
* in the dump memory structure. Presently, three sections are used for
--
2.49.0
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v4 5/8] hw/ppc: Implement saving CPU state in Fadump
2025-03-23 17:39 [PATCH v4 0/8] Implement Firmware Assisted Dump for PSeries Aditya Gupta
` (3 preceding siblings ...)
2025-03-23 17:40 ` [PATCH v4 4/8] hw/ppc: Preserve memory regions registered for fadump Aditya Gupta
@ 2025-03-23 17:40 ` Aditya Gupta
2025-10-18 10:54 ` Sourabh Jain
2025-03-23 17:40 ` [PATCH v4 6/8] hw/ppc: Pass dump-sizes property for fadump in device tree Aditya Gupta
` (4 subsequent siblings)
9 siblings, 1 reply; 36+ messages in thread
From: Aditya Gupta @ 2025-03-23 17:40 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Harsh Prateek Bora, Sourabh Jain, Mahesh J Salgaonkar,
Hari Bathini
Kernel expects CPU states/register states in the format mentioned in
"Register Save Area" in PAPR.
The platform (in our case, QEMU) saves each CPU register in the form of
an array of "register entries", the start and end of this array is
signified by "CPUSTRT" and "CPUEND" register entries respectively.
The CPUSTRT and CPUEND register entry also has 4-byte logical CPU ID,
thus storing the CPU ID corresponding to the array of register entries.
Each register, and CPUSTRT, CPUEND has a predefined identifier.
Implement calculating identifier for a given register in
'fadump_str_to_u64', which has been taken from the linux kernel
Similarly GPRs also have predefined identifiers, and a corresponding
64-bit resiter value (split into two 32-bit cells). Implement
calculation of GPR identifiers with 'fadump_gpr_id_to_u64'
PAPR has restrictions on particular order of few registers, and is
free to be in any order for other registers.
Some registers mentioned in PAPR have not been exported as they are not
implemented in QEMU / don't make sense in QEMU.
Implement saving of CPU state according to the PAPR document
Note: As of this patch, the "kernel-dump" device tree entry is still not
added for the second boot, so after crash, the second kernel will boot
assuming fadump dump is "NOT" active, and try to register for fadump,
but since we already have fadump registered in QEMU internal state, the
register rtas call will fail with: "DUMP ACTIVE"
Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
---
hw/ppc/spapr_fadump.c | 336 +++++++++++++++++++++++++++++++++-
include/hw/ppc/spapr_fadump.h | 28 +++
2 files changed, 363 insertions(+), 1 deletion(-)
diff --git a/hw/ppc/spapr_fadump.c b/hw/ppc/spapr_fadump.c
index c105b8d21da5..fc764b46e726 100644
--- a/hw/ppc/spapr_fadump.c
+++ b/hw/ppc/spapr_fadump.c
@@ -8,6 +8,71 @@
#include "qemu/log.h"
#include "hw/ppc/spapr.h"
#include "system/cpus.h"
+#include "system/hw_accel.h"
+
+/*
+ * Copy the ascii values for first 8 characters from a string into u64
+ * variable at their respective indexes.
+ * e.g.
+ * The string "FADMPINF" will be converted into 0x4641444d50494e46
+ */
+static uint64_t fadump_str_to_u64(const char *str)
+{
+ uint64_t val = 0;
+ int i;
+
+ for (i = 0; i < sizeof(val); i++) {
+ val = (*str) ? (val << 8) | *str++ : val << 8;
+ }
+ return val;
+}
+
+/**
+ * Get the identifier id for register entries of GPRs
+ *
+ * It gives the same id as 'fadump_str_to_u64' when the complete string id
+ * of the GPR is given, ie.
+ *
+ * fadump_str_to_u64("GPR05") == fadump_gpr_id_to_u64(5);
+ * fadump_str_to_u64("GPR12") == fadump_gpr_id_to_u64(12);
+ *
+ * And so on. Hence this can be implemented by creating a dynamic
+ * string for each GPR, such as "GPR00", "GPR01", ... "GPR31"
+ * Instead of allocating a string, an observation from the math of
+ * 'fadump_str_to_u64' or from PAPR tells us that there's a pattern
+ * in the identifier IDs, such that the first 4 bytes are affected only by
+ * whether it is GPR0*, GPR1*, GPR2*, GPR3*.
+ * Upper half of 5th byte is always 0x3. Lower half (nibble) of 5th byte
+ * is the tens digit of the GPR id, ie. GPR ID / 10.
+ * Upper half of 6th byte is always 0x3. Lower half (nibble) of 5th byte
+ * is the ones digit of the GPR id, ie. GPR ID % 10
+ *
+ * For example, for GPR 29, the 5th and 6th byte will be 0x32 and 0x39
+ */
+static uint64_t fadump_gpr_id_to_u64(uint32_t gpr_id)
+{
+ uint64_t val = 0;
+
+ /* Valid range of GPR id is only GPR0 to GPR31 */
+ assert(gpr_id < 32);
+
+ /* Below calculations set the 0th to 5th byte */
+ if (gpr_id <= 9) {
+ val = fadump_str_to_u64("GPR0");
+ } else if (gpr_id <= 19) {
+ val = fadump_str_to_u64("GPR1");
+ } else if (gpr_id <= 29) {
+ val = fadump_str_to_u64("GPR2");
+ } else {
+ val = fadump_str_to_u64("GPR3");
+ }
+
+ /* Set the 6th byte */
+ val |= 0x30000000;
+ val |= ((gpr_id % 10) << 24);
+
+ return val;
+}
/*
* Handle the "FADUMP_CMD_REGISTER" command in 'ibm,configure-kernel-dump'
@@ -221,14 +286,221 @@ static bool do_preserve_region(FadumpSection *region)
return true;
}
+/*
+ * Populate the passed CPUs register entries, in the buffer starting at
+ * the argument 'curr_reg_entry'
+ *
+ * The register entries is an array of pair of register id and register
+ * value, as described in Table 591/592 in section "H.1 Register Save Area"
+ * in PAPR v2.13
+ *
+ * Returns pointer just past this CPU's register entries, which can be used
+ * as the start address for next CPU's register entries
+ */
+static FadumpRegEntry *populate_cpu_reg_entries(CPUState *cpu,
+ FadumpRegEntry *curr_reg_entry)
+{
+ CPUPPCState *env;
+ PowerPCCPU *ppc_cpu;
+ uint32_t num_regs_per_cpu = 0;
+
+ ppc_cpu = POWERPC_CPU(cpu);
+ env = cpu_env(cpu);
+ num_regs_per_cpu = 0;
+
+ curr_reg_entry->reg_id =
+ cpu_to_be64(fadump_str_to_u64("CPUSTRT"));
+ curr_reg_entry->reg_value = ppc_cpu->vcpu_id;
+ ++curr_reg_entry;
+
+#define REG_ENTRY(id, val) \
+ do { \
+ curr_reg_entry->reg_id = \
+ cpu_to_be64(fadump_str_to_u64(#id)); \
+ curr_reg_entry->reg_value = cpu_to_be64(val); \
+ ++curr_reg_entry; \
+ ++num_regs_per_cpu; \
+ } while (0)
+
+ REG_ENTRY(ACOP, env->spr[SPR_ACOP]);
+ REG_ENTRY(AMR, env->spr[SPR_AMR]);
+ REG_ENTRY(BESCR, env->spr[SPR_BESCR]);
+ REG_ENTRY(CFAR, env->spr[SPR_CFAR]);
+ REG_ENTRY(CIABR, env->spr[SPR_CIABR]);
+
+ /* Save the condition register */
+ REG_ENTRY(CR, ppc_get_cr(env));
+
+ REG_ENTRY(CTR, env->spr[SPR_CTR]);
+ REG_ENTRY(CTRL, env->spr[SPR_CTRL]);
+ REG_ENTRY(DABR, env->spr[SPR_DABR]);
+ REG_ENTRY(DABRX, env->spr[SPR_DABRX]);
+ REG_ENTRY(DAR, env->spr[SPR_DAR]);
+ REG_ENTRY(DAWR0, env->spr[SPR_DAWR0]);
+ REG_ENTRY(DAWR1, env->spr[SPR_DAWR1]);
+ REG_ENTRY(DAWRX0, env->spr[SPR_DAWRX0]);
+ REG_ENTRY(DAWRX1, env->spr[SPR_DAWRX1]);
+ REG_ENTRY(DPDES, env->spr[SPR_DPDES]);
+ REG_ENTRY(DSCR, env->spr[SPR_DSCR]);
+ REG_ENTRY(DSISR, env->spr[SPR_DSISR]);
+ REG_ENTRY(EBBHR, env->spr[SPR_EBBHR]);
+ REG_ENTRY(EBBRR, env->spr[SPR_EBBRR]);
+
+ REG_ENTRY(FPSCR, env->fpscr);
+ REG_ENTRY(FSCR, env->spr[SPR_FSCR]);
+
+ /* Save the GPRs */
+ for (int gpr_id = 0; gpr_id < 32; ++gpr_id) {
+ curr_reg_entry->reg_id =
+ cpu_to_be64(fadump_gpr_id_to_u64(gpr_id));
+ curr_reg_entry->reg_value =
+ cpu_to_be64(env->gpr[gpr_id]);
+ ++curr_reg_entry;
+ ++num_regs_per_cpu;
+ }
+
+ REG_ENTRY(IAMR, env->spr[SPR_IAMR]);
+ REG_ENTRY(IC, env->spr[SPR_IC]);
+ REG_ENTRY(LR, env->spr[SPR_LR]);
+
+ REG_ENTRY(MSR, env->msr);
+ REG_ENTRY(NIA, env->nip); /* NIA */
+ REG_ENTRY(PIR, env->spr[SPR_PIR]);
+ REG_ENTRY(PSPB, env->spr[SPR_PSPB]);
+ REG_ENTRY(PVR, env->spr[SPR_PVR]);
+ REG_ENTRY(RPR, env->spr[SPR_RPR]);
+ REG_ENTRY(SPURR, env->spr[SPR_SPURR]);
+ REG_ENTRY(SRR0, env->spr[SPR_SRR0]);
+ REG_ENTRY(SRR1, env->spr[SPR_SRR1]);
+ REG_ENTRY(TAR, env->spr[SPR_TAR]);
+ REG_ENTRY(TEXASR, env->spr[SPR_TEXASR]);
+ REG_ENTRY(TFHAR, env->spr[SPR_TFHAR]);
+ REG_ENTRY(TFIAR, env->spr[SPR_TFIAR]);
+ REG_ENTRY(TIR, env->spr[SPR_TIR]);
+ REG_ENTRY(UAMOR, env->spr[SPR_UAMOR]);
+ REG_ENTRY(VRSAVE, env->spr[SPR_VRSAVE]);
+ REG_ENTRY(VSCR, env->vscr);
+ REG_ENTRY(VTB, env->spr[SPR_VTB]);
+ REG_ENTRY(WORT, env->spr[SPR_WORT]);
+ REG_ENTRY(XER, env->spr[SPR_XER]);
+
+ /*
+ * Ignoring transaction checkpoint and few other registers
+ * mentioned in PAPR as not supported in QEMU
+ */
+#undef REG_ENTRY
+
+ /* End the registers for this CPU with "CPUEND" reg entry */
+ curr_reg_entry->reg_id =
+ cpu_to_be64(fadump_str_to_u64("CPUEND"));
+
+ /*
+ * Ensure the number of registers match (+2 for STRT & END)
+ *
+ * This will help catch an error if in future a new register entry
+ * is added/removed while not modifying FADUMP_NUM_PER_CPU_REGS
+ */
+ assert(FADUMP_NUM_PER_CPU_REGS == num_regs_per_cpu + 2 /*CPUSTRT+CPUEND*/);
+
+ ++curr_reg_entry;
+
+ return curr_reg_entry;
+}
+
+/*
+ * Populate the "Register Save Area"/CPU State as mentioned in section "H.1
+ * Register Save Area" in PAPR v2.13
+ *
+ * It allocates the buffer for this region, then populates the register
+ * entries
+ *
+ * Returns the pointer to the buffer (which should be deallocated by the
+ * callers), and sets the size of this buffer in the argument
+ * 'cpu_state_len'
+ */
+static void *populate_cpu_state_data(uint64_t *cpu_state_len)
+{
+ FadumpRegSaveAreaHeader reg_save_hdr;
+ FadumpRegEntry *reg_entries;
+ FadumpRegEntry *curr_reg_entry;
+ CPUState *cpu;
+
+ uint32_t num_reg_entries;
+ uint32_t reg_entries_size;
+ uint32_t num_cpus = 0;
+
+ void *cpu_state_buffer = NULL;
+ uint64_t offset = 0;
+
+ CPU_FOREACH(cpu) {
+ ++num_cpus;
+ }
+
+ reg_save_hdr.version = cpu_to_be32(0);
+ reg_save_hdr.magic_number =
+ cpu_to_be64(fadump_str_to_u64("REGSAVE"));
+
+ /* Reg save area header is immediately followed by num cpus */
+ reg_save_hdr.num_cpu_offset =
+ cpu_to_be32(sizeof(FadumpRegSaveAreaHeader));
+
+ num_reg_entries = num_cpus * FADUMP_NUM_PER_CPU_REGS;
+ reg_entries_size = num_reg_entries * sizeof(FadumpRegEntry);
+
+ reg_entries = g_new(FadumpRegEntry, num_reg_entries);
+
+ /* Pointer to current CPU's registers */
+ curr_reg_entry = reg_entries;
+
+ /* Populate register entries for all CPUs */
+ CPU_FOREACH(cpu) {
+ cpu_synchronize_state(cpu);
+ curr_reg_entry = populate_cpu_reg_entries(cpu, curr_reg_entry);
+ }
+
+ *cpu_state_len = 0;
+ *cpu_state_len += sizeof(reg_save_hdr); /* reg save header */
+ *cpu_state_len += 0xc; /* padding as in PAPR */
+ *cpu_state_len += sizeof(__be32); /* num_cpus */
+ *cpu_state_len += reg_entries_size; /* reg entries */
+
+ cpu_state_buffer = g_malloc(*cpu_state_len);
+
+ memcpy(cpu_state_buffer + offset,
+ ®_save_hdr, sizeof(reg_save_hdr));
+ offset += sizeof(reg_save_hdr);
+
+ /* Write num_cpus */
+ num_cpus = cpu_to_be32(num_cpus);
+ memcpy(cpu_state_buffer + offset, &num_cpus, sizeof(__be32));
+ offset += sizeof(__be32);
+
+ /* Write the register entries */
+ memcpy(cpu_state_buffer + offset, reg_entries, reg_entries_size);
+ offset += reg_entries_size;
+
+ return cpu_state_buffer;
+}
+
/* Preserve the memory locations registered for fadump */
static bool fadump_preserve_mem(SpaprMachineState *spapr)
{
FadumpMemStruct *fdm = &spapr->registered_fdm;
+ FadumpSection *cpu_state_region = NULL;
+ AddressSpace *default_as = &address_space_memory;
+ MemTxResult io_result;
+ MemTxAttrs attrs;
uint16_t dump_num_sections, data_type;
+ uint64_t dest_addr;
+ uint64_t cpu_state_addr, cpu_state_len = 0;
+ g_autofree void *cpu_state_buffer = NULL;
assert(spapr->fadump_registered);
+ /* Mark the memory transaction as privileged memory access */
+ attrs.user = 0;
+ attrs.memory = 1;
+
/*
* Handle all sections
*
@@ -241,6 +513,7 @@ static bool fadump_preserve_mem(SpaprMachineState *spapr)
dump_num_sections = be16_to_cpu(fdm->header.dump_num_sections);
for (int i = 0; i < dump_num_sections; ++i) {
data_type = be16_to_cpu(fdm->rgn[i].source_data_type);
+ dest_addr = be64_to_cpu(fdm->rgn[i].destination_address);
/* Reset error_flags & bytes_dumped for now */
fdm->rgn[i].error_flags = 0;
@@ -255,7 +528,15 @@ static bool fadump_preserve_mem(SpaprMachineState *spapr)
switch (data_type) {
case FADUMP_CPU_STATE_DATA:
- /* TODO: Add CPU state data */
+ cpu_state_region = &fdm->rgn[i];
+ cpu_state_addr = dest_addr;
+ cpu_state_buffer = populate_cpu_state_data(&cpu_state_len);
+
+ /*
+ * We will write the cpu state data later, as otherwise it
+ * might get overwritten by other fadump regions
+ */
+
break;
case FADUMP_HPTE_REGION:
/* TODO: Add hpte state data */
@@ -281,6 +562,59 @@ static bool fadump_preserve_mem(SpaprMachineState *spapr)
}
}
+ /* CPU State Region has not been requested by kernel */
+ if (!cpu_state_region) {
+ return true;
+ }
+
+ /*
+ * Write the Register Save Area
+ *
+ * CPU State/Register Save Area should be written after dumping the
+ * memory to prevent overwriting while saving other memory regions
+ *
+ * eg. If boot memory region is 1G, then both the first 1GB memory, and
+ * the Register Save Area needs to be saved at 1GB.
+ * And as the CPU_STATE_DATA region comes first than the
+ * REAL_MODE_REGION region to be copied, the CPU_STATE_DATA will get
+ * overwritten if saved before the 0GB - 1GB region is copied after
+ * saving CPU state data
+ */
+ io_result = address_space_write(default_as, cpu_state_addr, attrs,
+ cpu_state_buffer, cpu_state_len);
+ if ((io_result & MEMTX_DECODE_ERROR) ||
+ (io_result & MEMTX_ACCESS_ERROR)) {
+ /*
+ * Invalid destination address is not an hardware error, instead
+ * wrong parameter from the kernel.
+ * Return true to let caller know to continue reading other
+ * sections
+ */
+ cpu_state_region->error_flags = FADUMP_ERROR_INVALID_DEST_ADDR;
+ cpu_state_region->bytes_dumped = 0;
+ return true;
+ } else if (io_result != MEMTX_OK) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "FADump: Failed to write CPU state region\n");
+ cpu_state_region->bytes_dumped = 0;
+
+ return false;
+ }
+
+ /*
+ * Set bytes_dumped in cpu state region, so kernel knows platform have
+ * exported it
+ */
+ cpu_state_region->bytes_dumped = cpu_to_be64(cpu_state_len);
+
+ if (cpu_state_region->source_len != cpu_state_region->bytes_dumped) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "CPU State region's length passed by kernel (0x%lx) !="
+ " CPU State region's length exported by QEMU (0x%lx)\n",
+ be64_to_cpu(cpu_state_region->source_len),
+ be64_to_cpu(cpu_state_region->bytes_dumped));
+ }
+
return true;
}
diff --git a/include/hw/ppc/spapr_fadump.h b/include/hw/ppc/spapr_fadump.h
index d56ca1d6d651..13bdd39a9ec1 100644
--- a/include/hw/ppc/spapr_fadump.h
+++ b/include/hw/ppc/spapr_fadump.h
@@ -48,9 +48,14 @@
#define FADUMP_MAX_SECTIONS 10
#define RTAS_FADUMP_MAX_BOOT_MEM_REGS 7
+/* Number of registers stored per cpu */
+#define FADUMP_NUM_PER_CPU_REGS (32 /*GPR*/ + 45 /*others*/ + 2 /*STRT & END*/)
+
typedef struct FadumpSection FadumpSection;
typedef struct FadumpSectionHeader FadumpSectionHeader;
typedef struct FadumpMemStruct FadumpMemStruct;
+typedef struct FadumpRegSaveAreaHeader FadumpRegSaveAreaHeader;
+typedef struct FadumpRegEntry FadumpRegEntry;
struct SpaprMachineState;
@@ -88,6 +93,29 @@ struct FadumpMemStruct {
FadumpSection rgn[FADUMP_MAX_SECTIONS];
};
+/*
+ * The firmware-assisted dump format.
+ *
+ * The register save area is an area in the partition's memory used to preserve
+ * the register contents (CPU state data) for the active CPUs during a firmware
+ * assisted dump. The dump format contains register save area header followed
+ * by register entries. Each list of registers for a CPU starts with "CPUSTRT"
+ * and ends with "CPUEND".
+ */
+
+/* Register save area header. */
+struct FadumpRegSaveAreaHeader {
+ __be64 magic_number;
+ __be32 version;
+ __be32 num_cpu_offset;
+};
+
+/* Register entry. */
+struct FadumpRegEntry {
+ __be64 reg_id;
+ __be64 reg_value;
+};
+
uint32_t do_fadump_register(struct SpaprMachineState *, target_ulong);
void trigger_fadump_boot(struct SpaprMachineState *, target_ulong);
#endif /* PPC_SPAPR_FADUMP_H */
--
2.49.0
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v4 6/8] hw/ppc: Pass dump-sizes property for fadump in device tree
2025-03-23 17:39 [PATCH v4 0/8] Implement Firmware Assisted Dump for PSeries Aditya Gupta
` (4 preceding siblings ...)
2025-03-23 17:40 ` [PATCH v4 5/8] hw/ppc: Implement saving CPU state in Fadump Aditya Gupta
@ 2025-03-23 17:40 ` Aditya Gupta
2025-10-18 11:20 ` Sourabh Jain
2025-03-23 17:40 ` [PATCH v4 7/8] hw/ppc: Enable fadump for PSeries Aditya Gupta
` (3 subsequent siblings)
9 siblings, 1 reply; 36+ messages in thread
From: Aditya Gupta @ 2025-03-23 17:40 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Harsh Prateek Bora, Sourabh Jain, Mahesh J Salgaonkar,
Hari Bathini
Platform (ie. QEMU) is expected to pass few device tree properties for
details for fadump:
* "ibm,configure-kernel-dump-sizes": Space required to store dump data
for firmware provided dump sections (ie. CPU & HPTE regions)
* "ibm,configure-kernel-dump-version": Versions of fadump supported
Pass the above device tree nodes so that kernel can reserve sufficient
space for preserving the CPU state data
Note: As of this patch, the "kernel-dump" device tree entry is still not
added for the second boot, so after crash, the second kernel will boot
assuming fadump dump is "NOT" active, and try to register for fadump,
but since we already have fadump registered in QEMU internal state, the
register rtas call will fail with: "DUMP ACTIVE"
Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
---
hw/ppc/spapr.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 57 insertions(+)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index a415e51d077a..3cbc6a7409b7 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -900,6 +900,61 @@ static int spapr_dt_rng(void *fdt)
return ret ? -1 : 0;
}
+static void spapr_dt_rtas_fadump(SpaprMachineState *spapr, void *fdt, int rtas)
+{
+ MachineState *ms = MACHINE(spapr);
+ MachineClass *mc = MACHINE_GET_CLASS(ms);
+
+ uint32_t max_possible_cpus = mc->possible_cpu_arch_ids(ms)->len;
+ uint64_t fadump_cpu_state_size = 0;
+ uint16_t fadump_versions[2] = {
+ FADUMP_VERSION /* min supported version */,
+ FADUMP_VERSION /* max supported version */
+ };
+ uint32_t fadump_rgn_sizes[2][3] = {
+ {
+ cpu_to_be32(FADUMP_CPU_STATE_DATA),
+ 0, 0 /* Calculated later */
+ },
+ {
+ cpu_to_be32(FADUMP_HPTE_REGION),
+ 0, 0 /* HPTE region not implemented */
+ }
+ };
+
+ /*
+ * CPU State Data contains multiple fields such as header, num_cpus and
+ * register entries
+ *
+ * Calculate the maximum CPU State Data size, according to maximum
+ * possible CPUs the QEMU VM can have
+ *
+ * This calculation must match the 'cpu_state_len' calculation done in
+ * 'populate_cpu_state_data' in spapr_fadump.c
+ */
+ fadump_cpu_state_size += sizeof(struct FadumpRegSaveAreaHeader);
+ fadump_cpu_state_size += 0xc; /* padding as in PAPR */
+ fadump_cpu_state_size += sizeof(__be32); /* num_cpus */
+ fadump_cpu_state_size += max_possible_cpus * /* reg entries */
+ FADUMP_NUM_PER_CPU_REGS *
+ sizeof(struct FadumpRegEntry);
+
+ /* Set maximum size for CPU state data region */
+ assert(fadump_rgn_sizes[0][0] == cpu_to_be32(FADUMP_CPU_STATE_DATA));
+
+ /* Upper 32 bits of size, usually 0 */
+ fadump_rgn_sizes[0][1] = cpu_to_be32(fadump_cpu_state_size >> 32);
+
+ /* Lower 32 bits of size */
+ fadump_rgn_sizes[0][2] = cpu_to_be32(fadump_cpu_state_size & 0xffffffff);
+
+ /* Add device tree properties required from platform for fadump */
+ _FDT((fdt_setprop(fdt, rtas, "ibm,configure-kernel-dump-version",
+ fadump_versions, sizeof(fadump_versions))));
+ _FDT((fdt_setprop(fdt, rtas, "ibm,configure-kernel-dump-sizes",
+ fadump_rgn_sizes, sizeof(fadump_rgn_sizes))));
+}
+
static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt)
{
MachineState *ms = MACHINE(spapr);
@@ -1012,6 +1067,8 @@ static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt)
_FDT(fdt_setprop(fdt, rtas, "ibm,lrdr-capacity",
lrdr_capacity, sizeof(lrdr_capacity)));
+ spapr_dt_rtas_fadump(spapr, fdt, rtas);
+
spapr_dt_rtas_tokens(fdt, rtas);
}
--
2.49.0
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v4 7/8] hw/ppc: Enable fadump for PSeries
2025-03-23 17:39 [PATCH v4 0/8] Implement Firmware Assisted Dump for PSeries Aditya Gupta
` (5 preceding siblings ...)
2025-03-23 17:40 ` [PATCH v4 6/8] hw/ppc: Pass dump-sizes property for fadump in device tree Aditya Gupta
@ 2025-03-23 17:40 ` Aditya Gupta
2025-10-18 12:04 ` Sourabh Jain
2025-03-23 17:40 ` [PATCH v4 8/8] tests/functional: Add test for fadump in PSeries Aditya Gupta
` (2 subsequent siblings)
9 siblings, 1 reply; 36+ messages in thread
From: Aditya Gupta @ 2025-03-23 17:40 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Harsh Prateek Bora, Sourabh Jain, Mahesh J Salgaonkar,
Hari Bathini
With all support in place for preserving memory regions, enable fadump by
exporting the "ibm,kernel-dump" property in the device tree, representing
the fadump dump information, in case of a crash.
Currently "ibm,configure-kernel-dump" RTAS call is already registered,
which tells the kernel that the platform (QEMU) supports fadump.
Now, in case of a crash, if fadump was registered, we also pass
"ibm,kernel-dump" in device tree, which tells the kernel that the fadump
dump is active.
Pass "fadump=on" to enable Linux to use firmware assisted dump.
Logs of a linux boot with firmware assisted dump:
$ ./build/qemu-system-ppc64 -M pseries,x-vof=on --cpu power10 --smp 4 -m 4G -kernel some-vmlinux -initrd some-initrd -append "debug fadump=on crashkernel=1G" -nographic
[ 0.000000] fadump: Reserved 1024MB of memory at 0x00000040000000 (System RAM: 4096MB)
[ 0.000000] fadump: Initialized 0x40000000 bytes cma area at 1024MB from 0x400102a8 bytes of memory reserved for firmware-assisted dump
...
[ 1.084686] rtas fadump: Registration is successful!
...
# cat /sys/kernel/debug/powerpc/fadump_region
CPU :[0x00000040000000-0x000000400013df] 0x13e0 bytes, Dumped: 0x0
HPTE:[0x000000400013e0-0x000000400013df] 0x0 bytes, Dumped: 0x0
DUMP: Src: 0x00000000000000, Dest: 0x00000040010000, Size: 0x40000000, Dumped: 0x0 bytes
[0x0000000921a000-0x0000000921a7ff]: cmdline append: ''
# echo c > /proc/sysrq-trigger
The fadump boot after crash:
[ 0.000000] rtas fadump: Firmware-assisted dump is active.
[ 0.000000] fadump: Updated cmdline: debug fadump=on crashkernel=1G
[ 0.000000] fadump: Firmware-assisted dump is active.
[ 0.000000] fadump: Reserving 3072MB of memory at 0x00000040000000 for preserving crash data
....
# file /proc/vmcore
/proc/vmcore: ELF 64-bit LSB core file, 64-bit PowerPC or cisco 7500, OpenPOWER ELF V2 ABI, version 1 (SYSV), SVR4-style
Analysing the vmcore with crash-utility:
KERNEL: vmlinux-6.14-rc2
DUMPFILE: vmcore-fc92fb373aa0
CPUS: 4
DATE: Wed Mar 12 23:39:23 CDT 2025
UPTIME: 00:00:22
LOAD AVERAGE: 0.13, 0.03, 0.01
TASKS: 95
NODENAME: buildroot
RELEASE: 6.12.0-rc4+
VERSION: #1 SMP Fri Jan 3 00:15:17 IST 2025
MACHINE: ppc64le (1000 Mhz)
MEMORY: 4 GB
PANIC: "Kernel panic - not syncing: sysrq triggered crash"
PID: 269
COMMAND: "sh"
TASK: c00000000a050b00 [THREAD_INFO: c00000000a050b00]
CPU: 0
STATE: TASK_RUNNING (PANIC)
Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
---
hw/ppc/spapr.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 3cbc6a7409b7..f6e666ad4344 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -904,6 +904,9 @@ static void spapr_dt_rtas_fadump(SpaprMachineState *spapr, void *fdt, int rtas)
{
MachineState *ms = MACHINE(spapr);
MachineClass *mc = MACHINE_GET_CLASS(ms);
+ FadumpMemStruct *fdm = &spapr->registered_fdm;
+ uint16_t dump_status_flag;
+ bool is_next_boot_fadump;
uint32_t max_possible_cpus = mc->possible_cpu_arch_ids(ms)->len;
uint64_t fadump_cpu_state_size = 0;
@@ -953,6 +956,18 @@ static void spapr_dt_rtas_fadump(SpaprMachineState *spapr, void *fdt, int rtas)
fadump_versions, sizeof(fadump_versions))));
_FDT((fdt_setprop(fdt, rtas, "ibm,configure-kernel-dump-sizes",
fadump_rgn_sizes, sizeof(fadump_rgn_sizes))));
+
+ dump_status_flag = be16_to_cpu(fdm->header.dump_status_flag);
+ is_next_boot_fadump =
+ (dump_status_flag & FADUMP_STATUS_DUMP_TRIGGERED) != 0;
+ if (is_next_boot_fadump) {
+ uint64_t fdm_size =
+ sizeof(struct FadumpSectionHeader) +
+ (be16_to_cpu(fdm->header.dump_num_sections) *
+ sizeof(struct FadumpSection));
+
+ _FDT((fdt_setprop(fdt, rtas, "ibm,kernel-dump", fdm, fdm_size)));
+ }
}
static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt)
--
2.49.0
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v4 8/8] tests/functional: Add test for fadump in PSeries
2025-03-23 17:39 [PATCH v4 0/8] Implement Firmware Assisted Dump for PSeries Aditya Gupta
` (6 preceding siblings ...)
2025-03-23 17:40 ` [PATCH v4 7/8] hw/ppc: Enable fadump for PSeries Aditya Gupta
@ 2025-03-23 17:40 ` Aditya Gupta
2025-04-21 6:27 ` [PATCH v4 0/8] Implement Firmware Assisted Dump for PSeries Aditya Gupta
2025-10-21 5:00 ` Harsh Prateek Bora
9 siblings, 0 replies; 36+ messages in thread
From: Aditya Gupta @ 2025-03-23 17:40 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Harsh Prateek Bora, Sourabh Jain, Mahesh J Salgaonkar,
Hari Bathini
Add testcases for testing fadump with PSeries and PSeries+KVM
combinations
It tests if fadump is successfully detected and registered in the first
kernel boot. Then crashes the kernel, and verifies whether we have a
/proc/vmcore in the 2nd boot
Also introduce 'wait_for_regex_console_pattern' to check for cases where
there is a single success message, but can have multiple failure
messages.
This is particularly useful for cases such as fadump, where the
success message is
"Reserved 1024MB ... successfully"
But at the same point, it can fail with multiple errors such as
"Not supported" or "Allocation failed"
'wait_for_regex_console_pattern' also has a timeout, for cases when we
know the success/failure should appear in a short amount of time,
instead of waiting for the much longer test timeout, such as kernels
with support of fadump will print the success/failure in earlyboot of
the kernel, while kernel without support of fadump won't print anything
for long time, and without a timeout the testcase keeps waiting till
longer test timeout
Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
---
PowerNV also can be tested with this, will enable PowerNV tests after
MPIPL patches go in
---
---
tests/functional/meson.build | 2 +
tests/functional/qemu_test/linuxkernel.py | 59 +++++++
tests/functional/test_ppc64_fadump.py | 182 ++++++++++++++++++++++
3 files changed, 243 insertions(+)
create mode 100755 tests/functional/test_ppc64_fadump.py
diff --git a/tests/functional/meson.build b/tests/functional/meson.build
index 96d282892798..a1e8d7bf385a 100644
--- a/tests/functional/meson.build
+++ b/tests/functional/meson.build
@@ -47,6 +47,7 @@ test_timeouts = {
'ppc64_powernv' : 480,
'ppc64_pseries' : 480,
'ppc64_replay' : 210,
+ 'ppc64_fadump' : 480,
'ppc64_tuxrun' : 420,
'ppc64_mac99' : 120,
'riscv64_tuxrun' : 120,
@@ -231,6 +232,7 @@ tests_ppc64_system_thorough = [
'ppc64_replay',
'ppc64_tuxrun',
'ppc64_mac99',
+ 'ppc64_fadump',
]
tests_riscv32_system_quick = [
diff --git a/tests/functional/qemu_test/linuxkernel.py b/tests/functional/qemu_test/linuxkernel.py
index 2aca0ee3cd03..c4767527daf6 100644
--- a/tests/functional/qemu_test/linuxkernel.py
+++ b/tests/functional/qemu_test/linuxkernel.py
@@ -5,6 +5,9 @@
import hashlib
import urllib.request
+import logging
+import re
+import time
from .cmd import wait_for_console_pattern, exec_command_and_wait_for_pattern
from .testcase import QemuSystemTest
@@ -19,6 +22,62 @@ def wait_for_console_pattern(self, success_message, vm=None):
failure_message='Kernel panic - not syncing',
vm=vm)
+ def wait_for_regex_console_pattern(self, success_pattern,
+ failure_pattern=None,
+ timeout=None):
+ """
+ Similar to 'wait_for_console_pattern', but supports regex patterns,
+ hence multiple failure/success patterns can be detected at a time.
+
+ Args:
+ success_pattern (str | re.Pattern): A regex pattern that indicates
+ a successful event. If found, the method exits normally.
+ failure_pattern (str | re.Pattern, optional): A regex pattern that
+ indicates a failure event. If found, the test fails
+ timeout (int, optional): The maximum time (in seconds) to wait for
+ a match.
+ If exceeded, the test fails.
+ """
+
+ console = self.vm.console_file
+ console_logger = logging.getLogger('console')
+
+ self.log.debug(
+ f"Console interaction: success_msg='{success_pattern}' " +
+ f"failure_msg='{failure_pattern}' timeout='{timeout}s'")
+
+ # Only consume console output if waiting for something
+ if success_pattern is None and failure_pattern is None:
+ return
+
+ start_time = time.time()
+
+ while time.time() - start_time < timeout:
+ try:
+ msg = console.readline().decode().strip()
+ except UnicodeDecodeError:
+ msg = None
+ if not msg:
+ continue
+ console_logger.debug(msg)
+ if success_pattern is None or re.search(success_pattern, msg):
+ break
+ if failure_pattern:
+ # Find the matching error to print in log
+ match = re.search(failure_pattern, msg)
+ if not match:
+ continue
+
+ console.close()
+ fail = 'Failure message found in console: "%s".' \
+ ' Expected: "%s"' % \
+ (match.group(), success_pattern)
+ self.fail(fail)
+
+ if time.time() - start_time >= timeout:
+ fail = f"Timeout ({timeout}s) while trying to search pattern"
+ self.fail(fail)
+
def launch_kernel(self, kernel, initrd=None, dtb=None, console_index=0,
wait_for=None):
self.vm.set_console(console_index=console_index)
diff --git a/tests/functional/test_ppc64_fadump.py b/tests/functional/test_ppc64_fadump.py
new file mode 100755
index 000000000000..d21175013490
--- /dev/null
+++ b/tests/functional/test_ppc64_fadump.py
@@ -0,0 +1,182 @@
+#!/usr/bin/env python3
+#
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+from unittest import skip
+from qemu_test import Asset
+from qemu_test import wait_for_console_pattern
+from qemu_test import LinuxKernelTest
+from qemu_test import exec_command, exec_command_and_wait_for_pattern
+
+class QEMUFadump(LinuxKernelTest):
+ """
+ Functional test to verify Fadump is working in following scenarios:
+
+ 1. test_fadump_pseries: PSeries
+ 2. test_fadump_pseries_kvm: PSeries + KVM
+ """
+
+ timeout = 90
+ KERNEL_COMMON_COMMAND_LINE = 'console=hvc0 fadump=on '
+ msg_panic = 'Kernel panic - not syncing'
+ msg_not_supported = 'Firmware-Assisted Dump is not supported on this hardware'
+ msg_registered_success = ''
+ msg_registered_failed = ''
+ msg_dump_active = ''
+
+ ASSET_EPAPR_KERNEL = Asset(
+ ('https://github.com/open-power/op-build/releases/download/v2.7/'
+ 'zImage.epapr'),
+ '0ab237df661727e5392cee97460e8674057a883c5f74381a128fa772588d45cd')
+
+ ASSET_VMLINUZ_KERNEL = Asset(
+ ('https://archives.fedoraproject.org/pub/archive/fedora-secondary/'
+ 'releases/39/Everything/ppc64le/os/ppc/ppc64/vmlinuz'),
+ ('81e5541d243b50c8f9568906c6918dda22239744d637bb9a7b22d23c3d661226'
+ '8d5302beb2ca5c06f93bdbc9736c414ef5120756c8bf496ff488ad07d116d67f')
+ )
+
+ ASSET_FEDORA_INITRD = Asset(
+ ('https://archives.fedoraproject.org/pub/archive/fedora-secondary/'
+ 'releases/39/Everything/ppc64le/os/ppc/ppc64/initrd.img'),
+ 'e7f24b44cb2aaa67d30e551db6ac8d29cc57c934b158dabca6b7f885f2cfdd9b')
+
+ def do_test_fadump(self, is_kvm=False, is_powernv=False):
+ """
+ Helper Function for Fadump tests below
+
+ It boots the VM with fadump enabled, checks if fadump is correctly
+ registered.
+ Then crashes the system causing a QEMU_SYSTEM_RESET, after which
+ dump should be available in the kernel.
+ Finally it checks the filesize of the exported /proc/vmcore in 2nd
+ kernel to verify it's same as the VM's memory size
+ """
+ if is_kvm:
+ self.require_accelerator("kvm")
+ self.vm.add_args("-accel", "kvm")
+ else:
+ self.require_accelerator("tcg")
+
+ if is_powernv:
+ self.set_machine("powernv10")
+ else:
+ # SLOF takes upto >20s in startup time, use VOF
+ self.set_machine("pseries")
+ self.vm.add_args("-machine", "x-vof=on")
+ self.vm.add_args("-m", "6G")
+
+ self.vm.set_console()
+
+ kernel_path = None
+
+ if is_powernv:
+ kernel_path = self.ASSET_EPAPR_KERNEL.fetch()
+ else:
+ kernel_path = self.ASSET_VMLINUZ_KERNEL.fetch()
+
+ initrd_path = self.ASSET_FEDORA_INITRD.fetch()
+
+ self.vm.add_args('-kernel', kernel_path)
+ self.vm.add_args('-initrd', initrd_path)
+ self.vm.add_args('-append', "fadump=on"\
+ " -nodefaults -serial mon:stdio crashkernel=2G"\
+ " rdinit=/bin/sh ")
+
+ self.vm.launch()
+
+ # If kernel detects fadump support, and "fadump=on" is in command
+ # line which we add above, it will print something like:
+ #
+ # fadump: Reserved 1024MB of memory at 0x00000040000000 ...
+ #
+ # Else, if the kernel doesn't detect fadump support, it prints:
+ #
+ # fadump: Firmware-Assisted Dump is not supported on this hardware
+ #
+ # Timeout after 10s if kernel doesn't print any fadump logs, this
+ # can happen due to fadump being disabled in the kernel
+ self.wait_for_regex_console_pattern(
+ success_pattern="fadump: Reserved ",
+ failure_pattern=r"fadump: (Firmware-Assisted Dump is not"\
+ " supported on this hardware|Failed to find memory chunk for"\
+ " reservation!)",
+ timeout=10
+ )
+
+ # Ensure fadump is registered successfully, if registration
+ # succeeds, we get a log from rtas fadump:
+ #
+ # rtas fadump: Registration is successful!
+ self.wait_for_console_pattern(
+ "rtas fadump: Registration is successful!"
+ )
+
+ # Wait for the shell
+ self.wait_for_console_pattern("#")
+
+ # Mount /proc since not available in the initrd used
+ exec_command(self, command="mount -t proc proc /proc")
+
+ # Crash the kernel
+ exec_command(self, command="echo c > /proc/sysrq-trigger")
+
+ # Check for the kernel panic message, setting timeout to 10s as it
+ # should occur almost immediately after previous echo c
+ self.wait_for_regex_console_pattern(
+ success_pattern="Kernel panic - not syncing: sysrq" \
+ " triggered crash",
+ timeout=10
+ )
+
+ # Check if fadump is active
+ # If the kernel shows that fadump is active, that implies it's a
+ # crashkernel boot
+ # Else if the kernel shows "fadump: Reserved ..." then it's
+ # treating this as the first kernel boot, this is likely the case
+ # that qemu didn't pass the 'ibm,kernel-dump' device tree node
+ wait_for_console_pattern(
+ test=self,
+ success_message="rtas fadump: Firmware-assisted dump is active",
+ failure_message="fadump: Reserved "
+ )
+
+ # In a successful fadump boot, we get these logs:
+ #
+ # [ 0.000000] fadump: Firmware-assisted dump is active.
+ # [ 0.000000] fadump: Reserving <>MB of memory at <> for preserving crash data
+ #
+ # Check if these logs are present in the fadump boot
+ self.wait_for_console_pattern("preserving crash data")
+
+ # Wait for prompt
+ self.wait_for_console_pattern("sh-5.2#")
+
+ # Mount /proc since not available in the initrd used
+ exec_command_and_wait_for_pattern(self,
+ command="mount -t proc proc /proc",
+ success_message="#"
+ )
+
+ # Check if vmcore exists
+ exec_command_and_wait_for_pattern(self,
+ command="stat /proc/vmcore",
+ success_message="File: /proc/vmcore",
+ failure_message="No such file or directory"
+ )
+
+ def test_fadump_pseries(self):
+ return self.do_test_fadump(is_kvm=False, is_powernv=False)
+
+ @skip("PowerNV Fadump not supported yet")
+ def test_fadump_powernv(self):
+ return
+
+ def test_fadump_pseries_kvm(self):
+ """
+ Test Fadump in PSeries with KVM accel
+ """
+ self.do_test_fadump(is_kvm=True, is_powernv=False)
+
+if __name__ == '__main__':
+ QEMUFadump.main()
--
2.49.0
^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v4 0/8] Implement Firmware Assisted Dump for PSeries
2025-03-23 17:39 [PATCH v4 0/8] Implement Firmware Assisted Dump for PSeries Aditya Gupta
` (7 preceding siblings ...)
2025-03-23 17:40 ` [PATCH v4 8/8] tests/functional: Add test for fadump in PSeries Aditya Gupta
@ 2025-04-21 6:27 ` Aditya Gupta
2025-10-21 5:00 ` Harsh Prateek Bora
9 siblings, 0 replies; 36+ messages in thread
From: Aditya Gupta @ 2025-04-21 6:27 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Harsh Prateek Bora, Sourabh Jain, Mahesh J Salgaonkar,
Hari Bathini
Hi,
Any comments on the series ?
Thanks,
- Aditya Gupta
On 23/03/25 23:09, Aditya Gupta wrote:
> Overview
> =========
>
> Implemented Firmware Assisted Dump (fadump) on PSeries machine in QEMU.
>
> Fadump is an alternative dump mechanism to kdump, in which we the firmware
> does a memory preserving boot, and the second/crashkernel is booted fresh
> like a normal system reset, instead of the crashed kernel loading the
> second/crashkernel in case of kdump.
>
> This requires implementing the "ibm,configure-kernel-dump" RTAS call in
> QEMU.
>
> While booting with fadump=on, Linux will register fadump memory regions.
>
> Some memory regions like Real Mode Memory regions, and custom memory
> regions declared by OS basically require copying the requested memory
> range to a destination
>
> While other memory regions are populated by the firmware/platform (QEMU in
> this case), such as CPU State Data and HPTE.
> We pass the sizes for these data segment to the kernel as it needs to know
> how much memory to reserve (ibm,configure-kernel-dump-sizes).
>
> Then after a crash, once Linux does a OS terminate call, we trigger fadump
> if fadump was registered.
>
> Implementing the fadump boot as:
> * pause all vcpus (will save registers later)
> * preserve memory regions specified by fadump
> * do a memory preserving reboot (using GUEST_RESET as it doesn't clear
> the memory)
>
> And then we pass a metadata (firmware memory structure) as
> "ibm,kernel-dump" in the device tree, containing all details of the
> preserved memory regions to the kernel.
>
> Refer the Patch #7/8: "hw/ppc: Enable fadump for PSeries" for logs of a
> succesfful fadump crash
>
> Note: HPTE region has not been implemented. It's not planned as of now.
>
> Testing
> =======
>
> Has been tested with following QEMU options:
>
> * firmware: x-vof and SLOF
> * tcg & kvm
> * l1 guest and l2 guest
> * with/without smp
> * cma/nocma
> * default crashkernel values (can fail with big initrd) and crashkernel=1G
>
> Git Tree for Testing
> ====================
>
> https://github.com/adi-g15-ibm/qemu/tree/fadump-pseries-v4
>
> Note: You will need a way to get the /proc/vmcore out of the VM for testing
> with crash-utility
>
> I use the following command line which sets up networking:
> "-net user,hostfwd=tcp::10022-:22 -net nic"
>
> And a rootfs with ssh support, then copy the /proc/vmcore with networking
> (can do compression using gzip before ssh, but compression might take lot
> of time if done inside the VM)
>
> Test vmcore for Testing with crash-utility
> ==========================================
>
> Can use vmlinux and vmcore available at https://github.com/adi-g15-ibm/qemu/releases/tag/test-images-fadump-pseries-v2
> Above vmcore was generated with upstream qemu with these fadump patches
> applied, and in a KVM VM
> A limitation with above vmcore is it was a single CPU VM
>
> Changelog
> =========
> v4
> + [patch #8/8]: fixed kvm testcase, add license
>
> v3:
> + [patch #3,7]: fix compile errors (#define declared in a later patch
> but used in this patch, unused var)
> + [patch #4/8]: use 'g_autofree' for cpu buffer, and replace g_malloc with
> g_try_malloc
> + [patch #5/8]: use 'g_new' instead of 'malloc', add null check for cpu
> region
> - nothing in other patches has been changed compared to v2
>
> v2:
> + rearrange code so that no unused functions get introduced in any patch
> + add functional test for pseries as suggested by nick
> + fix multiple issues pointed by harsh and nick
> + fix bug in cpu register saving where it was being stored in
> little-endian
> - removed 'is_next_boot_fadump' and used fadump header's status flag to
> store it
> + fixed multiple style issues (naming, unneeded diffs etc)
>
> Aditya Gupta (8):
> hw/ppc: Implement skeleton code for fadump in PSeries
> hw/ppc: Implement fadump register command
> hw/ppc: Trigger Fadump boot if fadump is registered
> hw/ppc: Preserve memory regions registered for fadump
> hw/ppc: Implement saving CPU state in Fadump
> hw/ppc: Pass dump-sizes property for fadump in device tree
> hw/ppc: Enable fadump for PSeries
> tests/functional: Add test for fadump in PSeries
>
> hw/ppc/meson.build | 1 +
> hw/ppc/spapr.c | 72 +++
> hw/ppc/spapr_fadump.c | 685 ++++++++++++++++++++++
> hw/ppc/spapr_rtas.c | 71 +++
> include/hw/ppc/spapr.h | 11 +-
> include/hw/ppc/spapr_fadump.h | 121 ++++
> tests/functional/meson.build | 2 +
> tests/functional/qemu_test/linuxkernel.py | 59 ++
> tests/functional/test_ppc64_fadump.py | 182 ++++++
> 9 files changed, 1203 insertions(+), 1 deletion(-)
> create mode 100644 hw/ppc/spapr_fadump.c
> create mode 100644 include/hw/ppc/spapr_fadump.h
> create mode 100755 tests/functional/test_ppc64_fadump.py
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 1/8] hw/ppc: Implement skeleton code for fadump in PSeries
2025-03-23 17:40 ` [PATCH v4 1/8] hw/ppc: Implement skeleton code for fadump in PSeries Aditya Gupta
@ 2025-04-21 10:51 ` Harsh Prateek Bora
2025-04-22 4:48 ` Aditya Gupta
2025-10-17 8:40 ` Sourabh Jain
2025-10-18 11:50 ` Sourabh Jain
2 siblings, 1 reply; 36+ messages in thread
From: Harsh Prateek Bora @ 2025-04-21 10:51 UTC (permalink / raw)
To: Aditya Gupta, qemu-devel
Cc: qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza, Sourabh Jain,
Mahesh J Salgaonkar, Hari Bathini
On 3/23/25 23:10, Aditya Gupta wrote:
> Add skeleton for handle "ibm,configure-kernel-dump" rtas call in QEMU.
>
> Verify basic details mandated by the PAPR, such as number of
> inputs/output, and add handling for the three fadump commands:
> regiser/unregister/invalidate.
>
> Currently fadump register will always return HARDWARE ERROR, since it's
> not implemented yet. So if the kernel's attempt to register fadump will
> itself fail as the support is not there yet in QEMU.
>
> The checks are based on the table in following requirement in PAPR v2.13:
> "R1–7.3.30–1. For the Configure Platform Assisted Kernel Dump option ..."
>
> Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
> ---
> hw/ppc/meson.build | 1 +
> hw/ppc/spapr_fadump.c | 22 +++++++++++
> hw/ppc/spapr_rtas.c | 66 +++++++++++++++++++++++++++++++++
> include/hw/ppc/spapr.h | 11 +++++-
> include/hw/ppc/spapr_fadump.h | 69 +++++++++++++++++++++++++++++++++++
> 5 files changed, 168 insertions(+), 1 deletion(-)
> create mode 100644 hw/ppc/spapr_fadump.c
> create mode 100644 include/hw/ppc/spapr_fadump.h
>
> diff --git a/hw/ppc/meson.build b/hw/ppc/meson.build
> index 9893f8adebb0..863972741b15 100644
> --- a/hw/ppc/meson.build
> +++ b/hw/ppc/meson.build
> @@ -26,6 +26,7 @@ ppc_ss.add(when: 'CONFIG_PSERIES', if_true: files(
> 'spapr_nvdimm.c',
> 'spapr_rtas_ddw.c',
> 'spapr_numa.c',
> + 'spapr_fadump.c',
> 'pef.c',
> ))
> ppc_ss.add(when: ['CONFIG_PSERIES', 'CONFIG_TCG'], if_true: files(
> diff --git a/hw/ppc/spapr_fadump.c b/hw/ppc/spapr_fadump.c
> new file mode 100644
> index 000000000000..20b7b804c485
> --- /dev/null
> +++ b/hw/ppc/spapr_fadump.c
> @@ -0,0 +1,22 @@
> +/*
> + * Firmware Assisted Dump in PSeries
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/ppc/spapr.h"
> +
> +/*
> + * Handle the "FADUMP_CMD_REGISTER" command in 'ibm,configure-kernel-dump'
> + *
> + * Returns:
> + * * RTAS_OUT_HW_ERROR: Not implemented/Misc issue such as memory access
> + * failures
> + */
> +uint32_t do_fadump_register(void)
> +{
> + /* WIP: FADUMP_CMD_REGISTER implemented in future patch */
> +
> + return RTAS_OUT_HW_ERROR;
> +}
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 503d441b48e4..b8bfa9c33fb5 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -341,6 +341,68 @@ static void rtas_ibm_set_system_parameter(PowerPCCPU *cpu,
> rtas_st(rets, 0, ret);
> }
>
> +/* Papr Section 7.4.9 ibm,configure-kernel-dump RTAS call */
> +static void rtas_configure_kernel_dump(PowerPCCPU *cpu,
> + SpaprMachineState *spapr,
> + uint32_t token, uint32_t nargs,
> + target_ulong args,
> + uint32_t nret, target_ulong rets)
> +{
> + target_ulong cmd = rtas_ld(args, 0);
> + uint32_t ret_val;
> +
> + /* Number of 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 of inputs has to be 3 */
> + if (nargs != 3) {
> + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> + return;
> + }
> +
> + switch (cmd) {
> + case FADUMP_CMD_REGISTER:
> + ret_val = do_fadump_register();
> + if (ret_val != RTAS_OUT_SUCCESS) {
> + rtas_st(rets, 0, ret_val);
> + return;
> + }
> + break;
I would suggest to keep the first patch as implementing the logic for
FADUMP_CMD_REGISTER (and _UNREGISTER) handling.
> + case FADUMP_CMD_UNREGISTER:
> + if (spapr->fadump_dump_active == 1) {
> + rtas_st(rets, 0, RTAS_OUT_DUMP_ACTIVE);
> + return;
> + }
> +
> + spapr->fadump_registered = false;
> + spapr->fadump_dump_active = false;
> + memset(&spapr->registered_fdm, 0, sizeof(spapr->registered_fdm));
> + break;
> + case FADUMP_CMD_INVALIDATE:
> + if (spapr->fadump_dump_active) {
> + spapr->fadump_registered = false;
> + spapr->fadump_dump_active = false;
> + memset(&spapr->registered_fdm, 0, sizeof(spapr->registered_fdm));
> + } else {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "FADump: Nothing to invalidate, no dump active\n");
> + }
> + break;
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "FADump: Unknown command: %lu\n", cmd);
> +
> + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> + return;
> + }
> +
> + rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> +}
> +
> static void rtas_ibm_os_term(PowerPCCPU *cpu,
> SpaprMachineState *spapr,
> uint32_t token, uint32_t nargs,
> @@ -656,6 +718,10 @@ static void core_rtas_register_types(void)
> spapr_rtas_register(RTAS_IBM_NMI_INTERLOCK, "ibm,nmi-interlock",
> rtas_ibm_nmi_interlock);
>
> + /* Register fadump rtas call */
> + spapr_rtas_register(RTAS_CONFIGURE_KERNEL_DUMP, "ibm,configure-kernel-dump",
> + rtas_configure_kernel_dump);
> +
> qtest_set_command_cb(spapr_qtest_callback);
> }
>
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 39bd5bd5ed31..4c1636497e30 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -13,6 +13,7 @@
> #include "hw/ppc/xics.h" /* For ICSState */
> #include "hw/ppc/spapr_tpm_proxy.h"
> #include "hw/ppc/spapr_nested.h" /* For SpaprMachineStateNested */
> +#include "hw/ppc/spapr_fadump.h" /* For FadumpMemStruct */
>
> struct SpaprVioBus;
> struct SpaprPhbState;
> @@ -283,6 +284,11 @@ struct SpaprMachineState {
> Error *fwnmi_migration_blocker;
>
> SpaprWatchdog wds[WDT_MAX_WATCHDOGS];
> +
> + /* Fadump State */
> + bool fadump_registered;
> + bool fadump_dump_active;
> + FadumpMemStruct registered_fdm;
> };
>
> #define H_SUCCESS 0
> @@ -708,6 +714,8 @@ void push_sregs_to_kvm_pr(SpaprMachineState *spapr);
> #define RTAS_OUT_PARAM_ERROR -3
> #define RTAS_OUT_NOT_SUPPORTED -3
> #define RTAS_OUT_NO_SUCH_INDICATOR -3
> +#define RTAS_OUT_DUMP_ALREADY_REGISTERED -9
> +#define RTAS_OUT_DUMP_ACTIVE -10
> #define RTAS_OUT_NOT_AUTHORIZED -9002
> #define RTAS_OUT_SYSPARM_PARAM_ERROR -9999
>
> @@ -770,8 +778,9 @@ void push_sregs_to_kvm_pr(SpaprMachineState *spapr);
> #define RTAS_IBM_SUSPEND_ME (RTAS_TOKEN_BASE + 0x2A)
> #define RTAS_IBM_NMI_REGISTER (RTAS_TOKEN_BASE + 0x2B)
> #define RTAS_IBM_NMI_INTERLOCK (RTAS_TOKEN_BASE + 0x2C)
> +#define RTAS_CONFIGURE_KERNEL_DUMP (RTAS_TOKEN_BASE + 0x2D)
>
> -#define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x2D)
> +#define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x2E)
>
> /* RTAS ibm,get-system-parameter token values */
> #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS 20
> diff --git a/include/hw/ppc/spapr_fadump.h b/include/hw/ppc/spapr_fadump.h
> new file mode 100644
> index 000000000000..45109fd9e137
> --- /dev/null
> +++ b/include/hw/ppc/spapr_fadump.h
> @@ -0,0 +1,69 @@
> +/*
> + * Firmware Assisted Dump in PSeries
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +#ifndef PPC_SPAPR_FADUMP_H
> +#define PPC_SPAPR_FADUMP_H
> +
> +#include "qemu/osdep.h"
> +#include "cpu.h"
> +
> +/* Fadump commands */
> +#define FADUMP_CMD_REGISTER 1
> +#define FADUMP_CMD_UNREGISTER 2
> +#define FADUMP_CMD_INVALIDATE 3
> +
> +#define FADUMP_VERSION 1
> +
> +/*
> + * The Firmware Assisted Dump Memory structure supports a maximum of 10 sections
> + * in the dump memory structure. Presently, three sections are used for
> + * CPU state data, HPTE & Parameters area, while the remaining seven sections
> + * can be used for boot memory regions.
> + */
> +#define FADUMP_MAX_SECTIONS 10
> +#define RTAS_FADUMP_MAX_BOOT_MEM_REGS 7
> +
> +typedef struct FadumpSection FadumpSection;
> +typedef struct FadumpSectionHeader FadumpSectionHeader;
> +typedef struct FadumpMemStruct FadumpMemStruct;
> +
> +struct SpaprMachineState;
> +
> +/* Kernel Dump section info */
> +struct FadumpSection {
> + __be32 request_flag;
> + __be16 source_data_type;
> + __be16 error_flags;
> + __be64 source_address;
> + __be64 source_len;
> + __be64 bytes_dumped;
> + __be64 destination_address;
> +};
> +
> +/* ibm,configure-kernel-dump header. */
> +struct FadumpSectionHeader {
> + __be32 dump_format_version;
> + __be16 dump_num_sections;
> + __be16 dump_status_flag;
> + __be32 offset_first_dump_section;
> +
> + /* Fields for disk dump option. */
> + __be32 dd_block_size;
> + __be64 dd_block_offset;
> + __be64 dd_num_blocks;
> + __be32 dd_offset_disk_path;
> +
> + /* Maximum time allowed to prevent an automatic dump-reboot. */
> + __be32 max_time_auto;
> +};
Also, you may introduce struct members in the patches as they are
used/accessed. No need to have entire struct introduced in the first
patch unless the members are being used/accessed.
regards,
Harsh
> +
> +/* Note: All the data in these structures is in big-endian */
> +struct FadumpMemStruct {
> + FadumpSectionHeader header;
> + FadumpSection rgn[FADUMP_MAX_SECTIONS];
> +};
> +
> +uint32_t do_fadump_register(void);
> +#endif /* PPC_SPAPR_FADUMP_H */
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 1/8] hw/ppc: Implement skeleton code for fadump in PSeries
2025-04-21 10:51 ` Harsh Prateek Bora
@ 2025-04-22 4:48 ` Aditya Gupta
0 siblings, 0 replies; 36+ messages in thread
From: Aditya Gupta @ 2025-04-22 4:48 UTC (permalink / raw)
To: Harsh Prateek Bora
Cc: qemu-devel, qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Sourabh Jain, Mahesh J Salgaonkar, Hari Bathini
Hi Harsh,
Thanks for reviewing.
On 25/04/21 04:21PM, Harsh Prateek Bora wrote:
>
>
> On 3/23/25 23:10, Aditya Gupta wrote:
> > <...snip...>
> >
> > + switch (cmd) {
> > + case FADUMP_CMD_REGISTER:
> > + ret_val = do_fadump_register();
> > + if (ret_val != RTAS_OUT_SUCCESS) {
> > + rtas_st(rets, 0, ret_val);
> > + return;
> > + }
> > + break;
>
> I would suggest to keep the first patch as implementing the logic for
> FADUMP_CMD_REGISTER (and _UNREGISTER) handling.
Tried that, but that is either introducing an unused function or would
mean squashing this patch and the subsequent patch implementing register
command.
I am preferring to keep current patch split.
What do you think ?
>
> > + case FADUMP_CMD_UNREGISTER:
> > + if (spapr->fadump_dump_active == 1) {
> > + rtas_st(rets, 0, RTAS_OUT_DUMP_ACTIVE);
> > + return;
> > + }
> > +
> > + spapr->fadump_registered = false;
> > + spapr->fadump_dump_active = false;
> > + memset(&spapr->registered_fdm, 0, sizeof(spapr->registered_fdm));
> > + break;
> >
> > <...snip...>
> >
> > +/* ibm,configure-kernel-dump header. */
> > +struct FadumpSectionHeader {
> > + __be32 dump_format_version;
> > + __be16 dump_num_sections;
> > + __be16 dump_status_flag;
> > + __be32 offset_first_dump_section;
> > +
> > + /* Fields for disk dump option. */
> > + __be32 dd_block_size;
> > + __be64 dd_block_offset;
> > + __be64 dd_num_blocks;
> > + __be32 dd_offset_disk_path;
> > +
> > + /* Maximum time allowed to prevent an automatic dump-reboot. */
> > + __be32 max_time_auto;
> > +};
>
> Also, you may introduce struct members in the patches as they are
> used/accessed. No need to have entire struct introduced in the first patch
> unless the members are being used/accessed.
Did that. The FadumpMemStruct gets used in SpaprMachineState and by
unregister command.
Agree with you about introducing fields as they are used, but kept the
complete structure with all fields to be compliant with PAPR, as having
the complete structure ensures the fields are at correct offsets as per
PAPR.
Thanks,
- Aditya G
>
> regards,
> Harsh
>
> > +
> > +/* Note: All the data in these structures is in big-endian */
> > +struct FadumpMemStruct {
> > + FadumpSectionHeader header;
> > + FadumpSection rgn[FADUMP_MAX_SECTIONS];
> > +};
> > +
> > +uint32_t do_fadump_register(void);
> > +#endif /* PPC_SPAPR_FADUMP_H */
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 1/8] hw/ppc: Implement skeleton code for fadump in PSeries
2025-03-23 17:40 ` [PATCH v4 1/8] hw/ppc: Implement skeleton code for fadump in PSeries Aditya Gupta
2025-04-21 10:51 ` Harsh Prateek Bora
@ 2025-10-17 8:40 ` Sourabh Jain
2025-10-17 11:38 ` Aditya Gupta
2025-10-18 11:50 ` Sourabh Jain
2 siblings, 1 reply; 36+ messages in thread
From: Sourabh Jain @ 2025-10-17 8:40 UTC (permalink / raw)
To: Aditya Gupta, qemu-devel
Cc: qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Harsh Prateek Bora, Mahesh J Salgaonkar, Hari Bathini
Hello Aditya,
On 23/03/25 23:10, Aditya Gupta wrote:
> Add skeleton for handle "ibm,configure-kernel-dump" rtas call in QEMU.
>
> Verify basic details mandated by the PAPR, such as number of
> inputs/output, and add handling for the three fadump commands:
> regiser/unregister/invalidate.
>
> Currently fadump register will always return HARDWARE ERROR, since it's
> not implemented yet. So if the kernel's attempt to register fadump will
> itself fail as the support is not there yet in QEMU.
>
> The checks are based on the table in following requirement in PAPR v2.13:
> "R1–7.3.30–1. For the Configure Platform Assisted Kernel Dump option ..."
>
> Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
> ---
> hw/ppc/meson.build | 1 +
> hw/ppc/spapr_fadump.c | 22 +++++++++++
> hw/ppc/spapr_rtas.c | 66 +++++++++++++++++++++++++++++++++
> include/hw/ppc/spapr.h | 11 +++++-
> include/hw/ppc/spapr_fadump.h | 69 +++++++++++++++++++++++++++++++++++
> 5 files changed, 168 insertions(+), 1 deletion(-)
> create mode 100644 hw/ppc/spapr_fadump.c
> create mode 100644 include/hw/ppc/spapr_fadump.h
>
> diff --git a/hw/ppc/meson.build b/hw/ppc/meson.build
> index 9893f8adebb0..863972741b15 100644
> --- a/hw/ppc/meson.build
> +++ b/hw/ppc/meson.build
> @@ -26,6 +26,7 @@ ppc_ss.add(when: 'CONFIG_PSERIES', if_true: files(
> 'spapr_nvdimm.c',
> 'spapr_rtas_ddw.c',
> 'spapr_numa.c',
> + 'spapr_fadump.c',
> 'pef.c',
> ))
> ppc_ss.add(when: ['CONFIG_PSERIES', 'CONFIG_TCG'], if_true: files(
> diff --git a/hw/ppc/spapr_fadump.c b/hw/ppc/spapr_fadump.c
> new file mode 100644
> index 000000000000..20b7b804c485
> --- /dev/null
> +++ b/hw/ppc/spapr_fadump.c
> @@ -0,0 +1,22 @@
> +/*
> + * Firmware Assisted Dump in PSeries
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/ppc/spapr.h"
> +
> +/*
> + * Handle the "FADUMP_CMD_REGISTER" command in 'ibm,configure-kernel-dump'
> + *
> + * Returns:
> + * * RTAS_OUT_HW_ERROR: Not implemented/Misc issue such as memory access
> + * failures
> + */
> +uint32_t do_fadump_register(void)
> +{
> + /* WIP: FADUMP_CMD_REGISTER implemented in future patch */
> +
> + return RTAS_OUT_HW_ERROR;
> +}
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 503d441b48e4..b8bfa9c33fb5 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -341,6 +341,68 @@ static void rtas_ibm_set_system_parameter(PowerPCCPU *cpu,
> rtas_st(rets, 0, ret);
> }
>
> +/* Papr Section 7.4.9 ibm,configure-kernel-dump RTAS call */
> +static void rtas_configure_kernel_dump(PowerPCCPU *cpu,
> + SpaprMachineState *spapr,
> + uint32_t token, uint32_t nargs,
> + target_ulong args,
> + uint32_t nret, target_ulong rets)
> +{
> + target_ulong cmd = rtas_ld(args, 0);
> + uint32_t ret_val;
> +
> + /* Number of 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");
No rtas_st for above failure?
> + return;
> + }
> +
> + /* Number of inputs has to be 3 */
> + if (nargs != 3) {
> + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
No qemu_log_mask for the above failure?
> + return;
> + }
> +
> + switch (cmd) {
> + case FADUMP_CMD_REGISTER:
> + ret_val = do_fadump_register();
> + if (ret_val != RTAS_OUT_SUCCESS) {
> + rtas_st(rets, 0, ret_val);
> + return;
> + }
> + break;
> + case FADUMP_CMD_UNREGISTER:
> + if (spapr->fadump_dump_active == 1) {
fadump_dump_active is bool, so comparing with an integer is not needed.
> + rtas_st(rets, 0, RTAS_OUT_DUMP_ACTIVE);
> + return;
> + }
> +
> + spapr->fadump_registered = false;
> + spapr->fadump_dump_active = false;
> + memset(&spapr->registered_fdm, 0, sizeof(spapr->registered_fdm));
> + break;
> + case FADUMP_CMD_INVALIDATE:
> + if (spapr->fadump_dump_active) {
> + spapr->fadump_registered = false;
> + spapr->fadump_dump_active = false;
> + memset(&spapr->registered_fdm, 0, sizeof(spapr->registered_fdm));
> + } else {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "FADump: Nothing to invalidate, no dump active\n");
> + }
> + break;
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "FADump: Unknown command: %lu\n", cmd);
> +
> + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> + return;
> + }
> +
> + rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> +}
> +
> static void rtas_ibm_os_term(PowerPCCPU *cpu,
> SpaprMachineState *spapr,
> uint32_t token, uint32_t nargs,
> @@ -656,6 +718,10 @@ static void core_rtas_register_types(void)
> spapr_rtas_register(RTAS_IBM_NMI_INTERLOCK, "ibm,nmi-interlock",
> rtas_ibm_nmi_interlock);
>
> + /* Register fadump rtas call */
> + spapr_rtas_register(RTAS_CONFIGURE_KERNEL_DUMP, "ibm,configure-kernel-dump",
> + rtas_configure_kernel_dump);
> +
> qtest_set_command_cb(spapr_qtest_callback);
> }
>
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 39bd5bd5ed31..4c1636497e30 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -13,6 +13,7 @@
> #include "hw/ppc/xics.h" /* For ICSState */
> #include "hw/ppc/spapr_tpm_proxy.h"
> #include "hw/ppc/spapr_nested.h" /* For SpaprMachineStateNested */
> +#include "hw/ppc/spapr_fadump.h" /* For FadumpMemStruct */
>
> struct SpaprVioBus;
> struct SpaprPhbState;
> @@ -283,6 +284,11 @@ struct SpaprMachineState {
> Error *fwnmi_migration_blocker;
>
> SpaprWatchdog wds[WDT_MAX_WATCHDOGS];
> +
> + /* Fadump State */
> + bool fadump_registered;
> + bool fadump_dump_active;
> + FadumpMemStruct registered_fdm;
> };
>
> #define H_SUCCESS 0
> @@ -708,6 +714,8 @@ void push_sregs_to_kvm_pr(SpaprMachineState *spapr);
> #define RTAS_OUT_PARAM_ERROR -3
> #define RTAS_OUT_NOT_SUPPORTED -3
> #define RTAS_OUT_NO_SUCH_INDICATOR -3
> +#define RTAS_OUT_DUMP_ALREADY_REGISTERED -9
> +#define RTAS_OUT_DUMP_ACTIVE -10
> #define RTAS_OUT_NOT_AUTHORIZED -9002
> #define RTAS_OUT_SYSPARM_PARAM_ERROR -9999
>
> @@ -770,8 +778,9 @@ void push_sregs_to_kvm_pr(SpaprMachineState *spapr);
> #define RTAS_IBM_SUSPEND_ME (RTAS_TOKEN_BASE + 0x2A)
> #define RTAS_IBM_NMI_REGISTER (RTAS_TOKEN_BASE + 0x2B)
> #define RTAS_IBM_NMI_INTERLOCK (RTAS_TOKEN_BASE + 0x2C)
> +#define RTAS_CONFIGURE_KERNEL_DUMP (RTAS_TOKEN_BASE + 0x2D)
>
> -#define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x2D)
> +#define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x2E)
>
> /* RTAS ibm,get-system-parameter token values */
> #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS 20
> diff --git a/include/hw/ppc/spapr_fadump.h b/include/hw/ppc/spapr_fadump.h
> new file mode 100644
> index 000000000000..45109fd9e137
> --- /dev/null
> +++ b/include/hw/ppc/spapr_fadump.h
> @@ -0,0 +1,69 @@
> +/*
> + * Firmware Assisted Dump in PSeries
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +#ifndef PPC_SPAPR_FADUMP_H
> +#define PPC_SPAPR_FADUMP_H
> +
> +#include "qemu/osdep.h"
> +#include "cpu.h"
> +
> +/* Fadump commands */
> +#define FADUMP_CMD_REGISTER 1
> +#define FADUMP_CMD_UNREGISTER 2
> +#define FADUMP_CMD_INVALIDATE 3
> +
> +#define FADUMP_VERSION 1
> +
> +/*
> + * The Firmware Assisted Dump Memory structure supports a maximum of 10 sections
> + * in the dump memory structure. Presently, three sections are used for
> + * CPU state data, HPTE & Parameters area, while the remaining seven sections
> + * can be used for boot memory regions.
> + */
> +#define FADUMP_MAX_SECTIONS 10
> +#define RTAS_FADUMP_MAX_BOOT_MEM_REGS 7
Please move RTAS_FADUMP_MAX_BOOT_MEM_REGS to the respective patch
if it’s used; otherwise, remove it if it’s not needed.
> +
> +typedef struct FadumpSection FadumpSection;
> +typedef struct FadumpSectionHeader FadumpSectionHeader;
> +typedef struct FadumpMemStruct FadumpMemStruct;
> +
> +struct SpaprMachineState;
Didn't understand the reason for the above forward declaration.
> +
> +/* Kernel Dump section info */
> +struct FadumpSection {
> + __be32 request_flag;
> + __be16 source_data_type;
> + __be16 error_flags;
> + __be64 source_address;
> + __be64 source_len;
> + __be64 bytes_dumped;
> + __be64 destination_address;
> +};
> +
> +/* ibm,configure-kernel-dump header. */
> +struct FadumpSectionHeader {
> + __be32 dump_format_version;
> + __be16 dump_num_sections;
> + __be16 dump_status_flag;
> + __be32 offset_first_dump_section;
> +
> + /* Fields for disk dump option. */
> + __be32 dd_block_size;
> + __be64 dd_block_offset;
> + __be64 dd_num_blocks;
> + __be32 dd_offset_disk_path;
> +
> + /* Maximum time allowed to prevent an automatic dump-reboot. */
> + __be32 max_time_auto;
> +};
> +
> +/* Note: All the data in these structures is in big-endian */
> +struct FadumpMemStruct {
> + FadumpSectionHeader header;
> + FadumpSection rgn[FADUMP_MAX_SECTIONS];
> +};
> +
> +uint32_t do_fadump_register(void);
> +#endif /* PPC_SPAPR_FADUMP_H */
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 2/8] hw/ppc: Implement fadump register command
2025-03-23 17:40 ` [PATCH v4 2/8] hw/ppc: Implement fadump register command Aditya Gupta
@ 2025-10-17 9:54 ` Sourabh Jain
2025-10-17 11:55 ` Aditya Gupta
0 siblings, 1 reply; 36+ messages in thread
From: Sourabh Jain @ 2025-10-17 9:54 UTC (permalink / raw)
To: Aditya Gupta, qemu-devel
Cc: qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Harsh Prateek Bora, Mahesh J Salgaonkar, Hari Bathini
On 23/03/25 23:10, Aditya Gupta wrote:
> Implement the register command of "ibm,configure-kernel-dump" RTAS call.
> The register just verifies the structure of the fadump memory structure
> passed by kernel, and set fadump_registered in spapr state to true.
>
> We also store the passed fadump memory structure, which will later be
> used for preserving memory for fadump boot in case of a crash.
>
> The fadump memory structure isn't modified (other than .dump_status_flag
> after the fadump is triggered, that is in a later patch).
> So if the structure needs to updated, the kernel should first
> de-register and re-register the structure again.
>
> Relevant section for the register command in PAPR is:
> Section 7.3.30: "ibm,configure-kernel-dump RTAS call" (PAPR v2.13)
>
> Note: The fadump registration is done, but triggering fadump on an
> os-term rtas call is done in later patches. Hence QEMU will just shutdown
> on a kernel crash due to no special handling for fadump in ibm,os-term
>
> Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
> ---
> hw/ppc/spapr_fadump.c | 111 ++++++++++++++++++++++++++++++++--
> hw/ppc/spapr_rtas.c | 2 +-
> include/hw/ppc/spapr_fadump.h | 2 +-
> 3 files changed, 108 insertions(+), 7 deletions(-)
>
> diff --git a/hw/ppc/spapr_fadump.c b/hw/ppc/spapr_fadump.c
> index 20b7b804c485..9c7fb9e12b16 100644
> --- a/hw/ppc/spapr_fadump.c
> +++ b/hw/ppc/spapr_fadump.c
> @@ -5,18 +5,119 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/log.h"
> #include "hw/ppc/spapr.h"
>
> /*
> * Handle the "FADUMP_CMD_REGISTER" command in 'ibm,configure-kernel-dump'
> *
> + * Note: Any changes made by the kernel to the fadump memory struct won't
> + * reflect in QEMU after the 'ibm,configure-kernel-dump' RTAS call has returned,
> + * as we store the passed fadump memory structure passed during fadump
passed fadump memory structure passed... is not easy to understand; how
about?
Note: Any modifications made by the kernel to the fadump memory structure
after the 'ibm,configure-kernel-dump' RTAS call returns will not be
reflected
in QEMU as QEMU retains the fadump memory structure that was provided
during fadump registration.
The kernel must invalidate and re-register fadump to apply any changes
to the fadump memory structure.
> + * registration.
> + * Kernel has to invalidate & re-register fadump, if it intends to make any
> + * changes to the fadump memory structure
> + *
> * Returns:
> - * * RTAS_OUT_HW_ERROR: Not implemented/Misc issue such as memory access
> - * failures
> + * * RTAS_OUT_SUCCESS: On successful registration
> + * * RTAS_OUT_PARAM_ERROR: If parameters are not correct, eg. too many
> + * sections, invalid memory addresses that we are
> + * unable to read, etc
> + * * RTAS_OUT_DUMP_ALREADY_REGISTERED: Dump already registered
> + * * RTAS_OUT_HW_ERROR: Misc issue such as memory access failures
> */
> -uint32_t do_fadump_register(void)
> +uint32_t do_fadump_register(SpaprMachineState *spapr, target_ulong args)
> {
> - /* WIP: FADUMP_CMD_REGISTER implemented in future patch */
> + FadumpSectionHeader header;
> + FadumpSection regions[FADUMP_MAX_SECTIONS] = {0};
> + target_ulong fdm_addr = rtas_ld(args, 1);
> + target_ulong fdm_size = rtas_ld(args, 2);
> + AddressSpace *default_as = &address_space_memory;
> + MemTxResult io_result;
> + MemTxAttrs attrs;
> + uint64_t next_section_addr;
> + uint16_t dump_num_sections;
> +
> + /* Mark the memory transaction as privileged memory access */
> + attrs.user = 0;
> + attrs.memory = 1;
> +
> + if (spapr->fadump_registered) {
> + /* FADump already registered */
> + return RTAS_OUT_DUMP_ALREADY_REGISTERED;
> + }
> +
> + if (spapr->fadump_dump_active == 1) {
fadump_dump_active is bool, so comparing with an integer is not needed.
> + return RTAS_OUT_DUMP_ACTIVE;
> + }
> +
> + if (fdm_size < sizeof(FadumpSectionHeader)) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "FADump: Header size is invalid: %lu\n", fdm_size);
> + return RTAS_OUT_PARAM_ERROR;
> + }
> +
> + /* Ensure fdm_addr points to a valid RMR-memory/RMA-memory buffer */
> + if ((fdm_addr <= 0) || ((fdm_addr + fdm_size) > spapr->rma_size)) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "FADump: Invalid fdm address: %ld\n", fdm_addr);
> + return RTAS_OUT_PARAM_ERROR;
> + }
> +
> + /* Try to read the passed fadump header */
> + io_result = address_space_read(default_as, fdm_addr, attrs,
> + &header, sizeof(header));
> + if (io_result != MEMTX_OK) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "FADump: Unable to read fdm address: %ld\n", fdm_addr);
> +
> + return RTAS_OUT_HW_ERROR;
> + }
> +
> + /* Verify that we understand the fadump header version */
> + if (header.dump_format_version != cpu_to_be32(FADUMP_VERSION)) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "FADump: Unknown fadump header version: 0x%x\n",
> + header.dump_format_version);
> + return RTAS_OUT_PARAM_ERROR;
> + }
> +
> + /* Reset dump status flags */
> + header.dump_status_flag = 0;
> +
> + dump_num_sections = be16_to_cpu(header.dump_num_sections);
> +
> + if (dump_num_sections > FADUMP_MAX_SECTIONS) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "FADump: Too many sections: %d sections\n", dump_num_sections);
> + return RTAS_OUT_PARAM_ERROR;
> + }
> +
> + next_section_addr =
> + fdm_addr +
> + be32_to_cpu(header.offset_first_dump_section);
> +
> + for (int i = 0; i < dump_num_sections; ++i) {
> + /* Read the fadump section from memory */
> + io_result = address_space_read(default_as, next_section_addr, attrs,
> + ®ions[i], sizeof(regions[i]));
> + if (io_result != MEMTX_OK) {
> + qemu_log_mask(LOG_UNIMP,
> + "FADump: Unable to read fadump %dth section\n", i);
> + return RTAS_OUT_PARAM_ERROR;
> + }
> +
> + next_section_addr += sizeof(regions[i]);
> + }
> +
> + spapr->fadump_registered = true;
> + spapr->fadump_dump_active = false;
Setting fadump_dump_active is not really required. Ideally, we shouldn't
reach here
if it is set to true, or am I missing something?
> +
> + /* Store the registered fadump memory struct */
> + spapr->registered_fdm.header = header;
> + for (int i = 0; i < dump_num_sections; ++i) {
> + spapr->registered_fdm.rgn[i] = regions[i];
> + }
>
> - return RTAS_OUT_HW_ERROR;
> + return RTAS_OUT_SUCCESS;
> }
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index b8bfa9c33fb5..0454938a01e9 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -366,7 +366,7 @@ static void rtas_configure_kernel_dump(PowerPCCPU *cpu,
>
> switch (cmd) {
> case FADUMP_CMD_REGISTER:
> - ret_val = do_fadump_register();
> + ret_val = do_fadump_register(spapr, args);
> if (ret_val != RTAS_OUT_SUCCESS) {
> rtas_st(rets, 0, ret_val);
> return;
> diff --git a/include/hw/ppc/spapr_fadump.h b/include/hw/ppc/spapr_fadump.h
> index 45109fd9e137..6abbcb44f353 100644
> --- a/include/hw/ppc/spapr_fadump.h
> +++ b/include/hw/ppc/spapr_fadump.h
> @@ -65,5 +65,5 @@ struct FadumpMemStruct {
> FadumpSection rgn[FADUMP_MAX_SECTIONS];
> };
>
> -uint32_t do_fadump_register(void);
> +uint32_t do_fadump_register(struct SpaprMachineState *, target_ulong);
Seems like the forward declaration of struct SpaprMachineState done in
01/08 is
because of the above change. Should we do there forward declaration here
instead?
> #endif /* PPC_SPAPR_FADUMP_H */
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 3/8] hw/ppc: Trigger Fadump boot if fadump is registered
2025-03-23 17:40 ` [PATCH v4 3/8] hw/ppc: Trigger Fadump boot if fadump is registered Aditya Gupta
@ 2025-10-17 11:26 ` Sourabh Jain
2025-10-17 11:59 ` Aditya Gupta
0 siblings, 1 reply; 36+ messages in thread
From: Sourabh Jain @ 2025-10-17 11:26 UTC (permalink / raw)
To: Aditya Gupta, qemu-devel
Cc: qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Harsh Prateek Bora, Mahesh J Salgaonkar, Hari Bathini
On 23/03/25 23:10, Aditya Gupta wrote:
> According to PAPR:
>
> R1–7.3.30–3. When the platform receives an ibm,os-term RTAS call, or
> on a system reset without an ibm,nmi-interlock RTAS call, if the
> platform has a dump structure registered through the
> ibm,configure-kernel-dump call, the platform must process each
> registered kernel dump section as required and, when available,
> present the dump structure information to the operating system
> through the “ibm,kernel-dump” property, updated with status for each
> dump section, until the dump has been invalidated through the
> ibm,configure-kernel-dump RTAS call.
>
> If Fadump has been registered, trigger an Fadump boot (memory preserving
> boot), if QEMU recieves a 'ibm,os-term' rtas call.
>
> Implementing the fadump boot as:
> * pause all vcpus (will need to save registers later)
> * preserve memory regions specified by fadump (will be implemented
> in future)
> * do a memory preserving reboot (GUEST_RESET in QEMU doesn't clear
> the memory)
>
> Memory regions registered by fadump will be handled in a later patch.
>
> Note: Preserving memory regions is not implemented yet so on an
> "ibm,os-term" call will just trigger a reboot in QEMU if fadump is
> registered, and the second kernel will boot as a normal boot (not
> fadump boot)
>
> Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
> ---
> hw/ppc/spapr_fadump.c | 77 +++++++++++++++++++++++++++++++++++
> hw/ppc/spapr_rtas.c | 5 +++
> include/hw/ppc/spapr_fadump.h | 6 +++
> 3 files changed, 88 insertions(+)
>
> diff --git a/hw/ppc/spapr_fadump.c b/hw/ppc/spapr_fadump.c
> index 9c7fb9e12b16..fedd2cde9a4f 100644
> --- a/hw/ppc/spapr_fadump.c
> +++ b/hw/ppc/spapr_fadump.c
> @@ -7,6 +7,7 @@
> #include "qemu/osdep.h"
> #include "qemu/log.h"
> #include "hw/ppc/spapr.h"
> +#include "system/cpus.h"
>
> /*
> * Handle the "FADUMP_CMD_REGISTER" command in 'ibm,configure-kernel-dump'
> @@ -121,3 +122,79 @@ uint32_t do_fadump_register(SpaprMachineState *spapr, target_ulong args)
>
> return RTAS_OUT_SUCCESS;
> }
> +
> +/* Preserve the memory locations registered for fadump */
> +static bool fadump_preserve_mem(void)
> +{
> + /*
> + * TODO: Implement preserving memory regions requested during fadump
> + * registration
> + */
> + return false;
> +}
> +
> +/*
> + * Trigger a fadump boot, ie. next boot will be a crashkernel/fadump boot
> + * with fadump dump active.
> + *
> + * This is triggered by ibm,os-term RTAS call, if fadump was registered.
> + *
> + * It preserves the memory and sets 'FADUMP_STATUS_DUMP_TRIGGERED' as
> + * fadump status, which can be used later to add the "ibm,kernel-dump"
> + * device tree node as presence of 'FADUMP_STATUS_DUMP_TRIGGERED' signifies
> + * next boot as fadump boot in our case
> + */
> +void trigger_fadump_boot(SpaprMachineState *spapr, target_ulong spapr_retcode)
> +{
> + FadumpSectionHeader *header = &spapr->registered_fdm.header;
> +
> + pause_all_vcpus();
> +
> + /* Preserve the memory locations registered for fadump */
> + if (!fadump_preserve_mem()) {
> + /* Failed to preserve the registered memory regions */
> + rtas_st(spapr_retcode, 0, RTAS_OUT_HW_ERROR);
> +
> + /* Cause a reboot */
> + qemu_system_guest_panicked(NULL);
> + return;
> + }
> +
> + /*
> + * Mark next boot as fadump boot
> + *
> + * Note: These is some bit of assumption involved here, as PAPR doesn't
> + * specify any use of the dump status flags, nor does the kernel use it
> + *
> + * But from description in Table 136 in PAPR v2.13, it looks like:
> + * FADUMP_STATUS_DUMP_TRIGGERED
> + * = Dump was triggered by the previous system boot (PAPR says)
> + * = Next boot will be a fadump boot (My perception)
Can we say assumption made or assumed instead of My perception.
> + *
> + * FADUMP_STATUS_DUMP_PERFORMED
> + * = Dump performed (Set to 0 by caller of the
> + * ibm,configure-kernel-dump call) (PAPR says)
> + * = Firmware has performed the copying/dump of requested regions
> + * (My perception)
> + * = Dump is active for the next boot (My perception)
Same here.
> + */
> + header->dump_status_flag = cpu_to_be16(
> + FADUMP_STATUS_DUMP_TRIGGERED | /* Next boot will be fadump boot */
> + FADUMP_STATUS_DUMP_PERFORMED /* Dump is active */
> + );
> +
> + /* Reset fadump_registered for next boot */
> + spapr->fadump_registered = false;
> + spapr->fadump_dump_active = true;
> +
> + /*
> + * Then do a guest reset
> + *
> + * Requirement:
> + * GUEST_RESET is expected to NOT clear the memory, as is the case when
> + * this is merged
> + */
> + qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> +
> + rtas_st(spapr_retcode, 0, RTAS_OUT_SUCCESS);
> +}
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 0454938a01e9..14b954a05109 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -412,6 +412,11 @@ static void rtas_ibm_os_term(PowerPCCPU *cpu,
> target_ulong msgaddr = rtas_ld(args, 0);
> char msg[512];
>
> + if (spapr->fadump_registered) {
> + /* If fadump boot works, control won't come back here */
> + return trigger_fadump_boot(spapr, rets);
> + }
> +
> cpu_physical_memory_read(msgaddr, msg, sizeof(msg) - 1);
> msg[sizeof(msg) - 1] = 0;
>
> diff --git a/include/hw/ppc/spapr_fadump.h b/include/hw/ppc/spapr_fadump.h
> index 6abbcb44f353..e484604c1c70 100644
> --- a/include/hw/ppc/spapr_fadump.h
> +++ b/include/hw/ppc/spapr_fadump.h
> @@ -16,6 +16,11 @@
>
> #define FADUMP_VERSION 1
>
> +/* Dump status flags */
> +#define FADUMP_STATUS_DUMP_PERFORMED 0x8000
> +#define FADUMP_STATUS_DUMP_TRIGGERED 0x4000
> +#define FADUMP_STATUS_DUMP_ERROR 0x2000
> +
> /*
> * The Firmware Assisted Dump Memory structure supports a maximum of 10 sections
> * in the dump memory structure. Presently, three sections are used for
> @@ -66,4 +71,5 @@ struct FadumpMemStruct {
> };
>
> uint32_t do_fadump_register(struct SpaprMachineState *, target_ulong);
> +void trigger_fadump_boot(struct SpaprMachineState *, target_ulong);
> #endif /* PPC_SPAPR_FADUMP_H */
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 1/8] hw/ppc: Implement skeleton code for fadump in PSeries
2025-10-17 8:40 ` Sourabh Jain
@ 2025-10-17 11:38 ` Aditya Gupta
2025-10-17 11:44 ` Aditya Gupta
2025-10-17 11:46 ` Sourabh Jain
0 siblings, 2 replies; 36+ messages in thread
From: Aditya Gupta @ 2025-10-17 11:38 UTC (permalink / raw)
To: Sourabh Jain
Cc: qemu-devel, qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Harsh Prateek Bora, Mahesh J Salgaonkar, Hari Bathini
Hello Sourabh,
Thanks for your detailed reviews.
On 25/10/17 02:10PM, Sourabh Jain wrote:
> Hello Aditya,
>
> > <...snip...>
> > +/* Papr Section 7.4.9 ibm,configure-kernel-dump RTAS call */
> > +static void rtas_configure_kernel_dump(PowerPCCPU *cpu,
> > + SpaprMachineState *spapr,
> > + uint32_t token, uint32_t nargs,
> > + target_ulong args,
> > + uint32_t nret, target_ulong rets)
> > +{
> > + target_ulong cmd = rtas_ld(args, 0);
> > + uint32_t ret_val;
> > +
> > + /* Number of 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");
>
>
> No rtas_st for above failure?
Will add.
Also I think I should remove the LOG_GUEST_ERROR, since I mostly use it
for qemu side errors, wrong parameters is an invalid usage rather than>
guest/qemu error.
What do you say ? Should I remove qemu_log_mask here ?
>
> > + return;
> > + }
> > +
> > + /* Number of inputs has to be 3 */
> > + if (nargs != 3) {
> > + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>
> No qemu_log_mask for the above failure?
Thinking to remove it, as mentioned above.
>
> > + return;
> > + }
> > +
> > + switch (cmd) {
> > + case FADUMP_CMD_REGISTER:
> > + ret_val = do_fadump_register();
> > + if (ret_val != RTAS_OUT_SUCCESS) {
> > + rtas_st(rets, 0, ret_val);
> > + return;
> > + }
> > + break;
> > + case FADUMP_CMD_UNREGISTER:
> > + if (spapr->fadump_dump_active == 1) {
>
>
> fadump_dump_active is bool, so comparing with an integer is not needed.
Nice catch. Thanks !
>
> > + rtas_st(rets, 0, RTAS_OUT_DUMP_ACTIVE);
> > + return;
> > + }
> > <...snip...>
> > +#define FADUMP_VERSION 1
> > +
> > +/*
> > + * The Firmware Assisted Dump Memory structure supports a maximum of 10 sections
> > + * in the dump memory structure. Presently, three sections are used for
> > + * CPU state data, HPTE & Parameters area, while the remaining seven sections
> > + * can be used for boot memory regions.
> > + */
> > +#define FADUMP_MAX_SECTIONS 10
> > +#define RTAS_FADUMP_MAX_BOOT_MEM_REGS 7
>
> Please move RTAS_FADUMP_MAX_BOOT_MEM_REGS to the respective patch
> if it’s used; otherwise, remove it if it’s not needed.
Again nice catch. It's not used anymore, will remove.
>
> > +
> > +typedef struct FadumpSection FadumpSection;
> > +typedef struct FadumpSectionHeader FadumpSectionHeader;
> > +typedef struct FadumpMemStruct FadumpMemStruct;
> > +
> > +struct SpaprMachineState;
>
>
> Didn't understand the reason for the above forward declaration.
Yes, in this patch it doesn't make sense, when merging the 1st and 2nd
patch, then we see the use. Idea is as explained below.
It is because it's defined in spapr.h, and we use SpaprMachineState at
the end of this header in declaration of 'do_fadump_register'.
But spapr.h includes spapr_fadump.h, so can't include that else it will
become a cyclic dependency.
Hence forward declaration was a way to solve that cyclic dependency.
Thanks !
- Aditya G
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 1/8] hw/ppc: Implement skeleton code for fadump in PSeries
2025-10-17 11:38 ` Aditya Gupta
@ 2025-10-17 11:44 ` Aditya Gupta
2025-10-20 5:23 ` Sourabh Jain
2025-10-17 11:46 ` Sourabh Jain
1 sibling, 1 reply; 36+ messages in thread
From: Aditya Gupta @ 2025-10-17 11:44 UTC (permalink / raw)
To: Sourabh Jain
Cc: qemu-devel, qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Harsh Prateek Bora, Mahesh J Salgaonkar, Hari Bathini
On 25/10/17 05:08PM, Aditya Gupta wrote:
> Hello Sourabh,
>
> Thanks for your detailed reviews.
>
> On 25/10/17 02:10PM, Sourabh Jain wrote:
> > Hello Aditya,
> >
> > > <...snip...>
> > > +/* Papr Section 7.4.9 ibm,configure-kernel-dump RTAS call */
> > > +static void rtas_configure_kernel_dump(PowerPCCPU *cpu,
> > > + SpaprMachineState *spapr,
> > > + uint32_t token, uint32_t nargs,
> > > + target_ulong args,
> > > + uint32_t nret, target_ulong rets)
> > > +{
> > > + target_ulong cmd = rtas_ld(args, 0);
> > > + uint32_t ret_val;
> > > +
> > > + /* Number of 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");
> >
> >
> > No rtas_st for above failure?
>
> Will add.
>
> Also I think I should remove the LOG_GUEST_ERROR, since I mostly use it
> for qemu side errors, wrong parameters is an invalid usage rather than>
> guest/qemu error.
>
> What do you say ? Should I remove qemu_log_mask here ?
>
> >
> > > + return;
> > > + }
> > > +
> > > + /* Number of inputs has to be 3 */
> > > + if (nargs != 3) {
> > > + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> >
> > No qemu_log_mask for the above failure?
>
> Thinking to remove it, as mentioned above.
On a second thought, I will keep the qemu_log_mask as suggested.
More logs helps for debug if kernel passes invalid arguments to fadump.
Is that okay ?
Thanks,
- Aditya G
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 1/8] hw/ppc: Implement skeleton code for fadump in PSeries
2025-10-17 11:38 ` Aditya Gupta
2025-10-17 11:44 ` Aditya Gupta
@ 2025-10-17 11:46 ` Sourabh Jain
2025-10-17 11:56 ` Aditya Gupta
1 sibling, 1 reply; 36+ messages in thread
From: Sourabh Jain @ 2025-10-17 11:46 UTC (permalink / raw)
To: Aditya Gupta
Cc: qemu-devel, qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Harsh Prateek Bora, Mahesh J Salgaonkar, Hari Bathini
On 17/10/25 17:08, Aditya Gupta wrote:
> Hello Sourabh,
>
> Thanks for your detailed reviews.
>
> On 25/10/17 02:10PM, Sourabh Jain wrote:
>> Hello Aditya,
>>
>>> <...snip...>
>>> +/* Papr Section 7.4.9 ibm,configure-kernel-dump RTAS call */
>>> +static void rtas_configure_kernel_dump(PowerPCCPU *cpu,
>>> + SpaprMachineState *spapr,
>>> + uint32_t token, uint32_t nargs,
>>> + target_ulong args,
>>> + uint32_t nret, target_ulong rets)
>>> +{
>>> + target_ulong cmd = rtas_ld(args, 0);
>>> + uint32_t ret_val;
>>> +
>>> + /* Number of 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");
>>
>> No rtas_st for above failure?
> Will add.
>
> Also I think I should remove the LOG_GUEST_ERROR, since I mostly use it
> for qemu side errors, wrong parameters is an invalid usage rather than>
> guest/qemu error.
>
> What do you say ? Should I remove qemu_log_mask here ?
Having such log helps. Not sure qemu_log_mask with LOG_GUEST_ERROR is right
a function to use but keep the logging with whatever logging function
you think
works best here.
>
>>> + return;
>>> + }
>>> +
>>> + /* Number of inputs has to be 3 */
>>> + if (nargs != 3) {
>>> + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>> No qemu_log_mask for the above failure?
> Thinking to remove it, as mentioned above.
Same as above.
>
>>> + return;
>>> + }
>>> +
>>> + switch (cmd) {
>>> + case FADUMP_CMD_REGISTER:
>>> + ret_val = do_fadump_register();
>>> + if (ret_val != RTAS_OUT_SUCCESS) {
>>> + rtas_st(rets, 0, ret_val);
>>> + return;
>>> + }
>>> + break;
>>> + case FADUMP_CMD_UNREGISTER:
>>> + if (spapr->fadump_dump_active == 1) {
>>
>> fadump_dump_active is bool, so comparing with an integer is not needed.
> Nice catch. Thanks !
>
>>> + rtas_st(rets, 0, RTAS_OUT_DUMP_ACTIVE);
>>> + return;
>>> + }
>>> <...snip...>
>>> +#define FADUMP_VERSION 1
>>> +
>>> +/*
>>> + * The Firmware Assisted Dump Memory structure supports a maximum of 10 sections
>>> + * in the dump memory structure. Presently, three sections are used for
>>> + * CPU state data, HPTE & Parameters area, while the remaining seven sections
>>> + * can be used for boot memory regions.
>>> + */
>>> +#define FADUMP_MAX_SECTIONS 10
>>> +#define RTAS_FADUMP_MAX_BOOT_MEM_REGS 7
>> Please move RTAS_FADUMP_MAX_BOOT_MEM_REGS to the respective patch
>> if it’s used; otherwise, remove it if it’s not needed.
> Again nice catch. It's not used anymore, will remove.
>
>>> +
>>> +typedef struct FadumpSection FadumpSection;
>>> +typedef struct FadumpSectionHeader FadumpSectionHeader;
>>> +typedef struct FadumpMemStruct FadumpMemStruct;
>>> +
>>> +struct SpaprMachineState;
>>
>> Didn't understand the reason for the above forward declaration.
> Yes, in this patch it doesn't make sense, when merging the 1st and 2nd
> patch, then we see the use. Idea is as explained below.
>
> It is because it's defined in spapr.h, and we use SpaprMachineState at
> the end of this header in declaration of 'do_fadump_register'.
>
> But spapr.h includes spapr_fadump.h, so can't include that else it will
> become a cyclic dependency.
>
> Hence forward declaration was a way to solve that cyclic dependency.
Yeah I got that while reviewing the second patch in the series.
Why are we merging 1st and 2nd patch?
- Sourabh
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 2/8] hw/ppc: Implement fadump register command
2025-10-17 9:54 ` Sourabh Jain
@ 2025-10-17 11:55 ` Aditya Gupta
2025-10-20 5:26 ` Sourabh Jain
0 siblings, 1 reply; 36+ messages in thread
From: Aditya Gupta @ 2025-10-17 11:55 UTC (permalink / raw)
To: Sourabh Jain
Cc: qemu-devel, qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Harsh Prateek Bora, Mahesh J Salgaonkar, Hari Bathini
On 25/10/17 03:24PM, Sourabh Jain wrote:
>
>
> On 23/03/25 23:10, Aditya Gupta wrote:
> > Implement the register command of "ibm,configure-kernel-dump" RTAS call.
> > The register just verifies the structure of the fadump memory structure
> > passed by kernel, and set fadump_registered in spapr state to true.
> >
> > We also store the passed fadump memory structure, which will later be
> > used for preserving memory for fadump boot in case of a crash.
> >
> > The fadump memory structure isn't modified (other than .dump_status_flag
> > after the fadump is triggered, that is in a later patch).
> > So if the structure needs to updated, the kernel should first
> > de-register and re-register the structure again.
> >
> > Relevant section for the register command in PAPR is:
> > Section 7.3.30: "ibm,configure-kernel-dump RTAS call" (PAPR v2.13)
> >
> > Note: The fadump registration is done, but triggering fadump on an
> > os-term rtas call is done in later patches. Hence QEMU will just shutdown
> > on a kernel crash due to no special handling for fadump in ibm,os-term
> >
> > Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
> > ---
> > hw/ppc/spapr_fadump.c | 111 ++++++++++++++++++++++++++++++++--
> > hw/ppc/spapr_rtas.c | 2 +-
> > include/hw/ppc/spapr_fadump.h | 2 +-
> > 3 files changed, 108 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/ppc/spapr_fadump.c b/hw/ppc/spapr_fadump.c
> > index 20b7b804c485..9c7fb9e12b16 100644
> > --- a/hw/ppc/spapr_fadump.c
> > +++ b/hw/ppc/spapr_fadump.c
> > @@ -5,18 +5,119 @@
> > */
> > #include "qemu/osdep.h"
> > +#include "qemu/log.h"
> > #include "hw/ppc/spapr.h"
> > /*
> > * Handle the "FADUMP_CMD_REGISTER" command in 'ibm,configure-kernel-dump'
> > *
> > + * Note: Any changes made by the kernel to the fadump memory struct won't
> > + * reflect in QEMU after the 'ibm,configure-kernel-dump' RTAS call has returned,
> > + * as we store the passed fadump memory structure passed during fadump
> passed fadump memory structure passed... is not easy to understand; how
> about?
>
> Note: Any modifications made by the kernel to the fadump memory structure
> after the 'ibm,configure-kernel-dump' RTAS call returns will not be
> reflected
> in QEMU as QEMU retains the fadump memory structure that was provided
> during fadump registration.
>
> The kernel must invalidate and re-register fadump to apply any changes
> to the fadump memory structure.
Makes sense. Will use the suggested text.
>
> > + * registration.
> > + * Kernel has to invalidate & re-register fadump, if it intends to make any
> > + * changes to the fadump memory structure
> > + *
> > * Returns:
> > - * * RTAS_OUT_HW_ERROR: Not implemented/Misc issue such as memory access
> > - * failures
> > + * * RTAS_OUT_SUCCESS: On successful registration
> > + * * RTAS_OUT_PARAM_ERROR: If parameters are not correct, eg. too many
> > + * sections, invalid memory addresses that we are
> > + * unable to read, etc
> > + * * RTAS_OUT_DUMP_ALREADY_REGISTERED: Dump already registered
> > + * * RTAS_OUT_HW_ERROR: Misc issue such as memory access failures
> > */
> > -uint32_t do_fadump_register(void)
> > +uint32_t do_fadump_register(SpaprMachineState *spapr, target_ulong args)
> > {
> > - /* WIP: FADUMP_CMD_REGISTER implemented in future patch */
> > + FadumpSectionHeader header;
> > + FadumpSection regions[FADUMP_MAX_SECTIONS] = {0};
> > + target_ulong fdm_addr = rtas_ld(args, 1);
> > + target_ulong fdm_size = rtas_ld(args, 2);
> > + AddressSpace *default_as = &address_space_memory;
> > + MemTxResult io_result;
> > + MemTxAttrs attrs;
> > + uint64_t next_section_addr;
> > + uint16_t dump_num_sections;
> > +
> > + /* Mark the memory transaction as privileged memory access */
> > + attrs.user = 0;
> > + attrs.memory = 1;
> > +
> > + if (spapr->fadump_registered) {
> > + /* FADump already registered */
> > + return RTAS_OUT_DUMP_ALREADY_REGISTERED;
> > + }
> > +
> > + if (spapr->fadump_dump_active == 1) {
>
> fadump_dump_active is bool, so comparing with an integer is not needed.
Will remove == 1.
>
> > + return RTAS_OUT_DUMP_ACTIVE;
> > + }
> > +
> > + if (fdm_size < sizeof(FadumpSectionHeader)) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "FADump: Header size is invalid: %lu\n", fdm_size);
> > + return RTAS_OUT_PARAM_ERROR;
> > + }
> > +
> > + /* Ensure fdm_addr points to a valid RMR-memory/RMA-memory buffer */
> > + if ((fdm_addr <= 0) || ((fdm_addr + fdm_size) > spapr->rma_size)) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "FADump: Invalid fdm address: %ld\n", fdm_addr);
> > + return RTAS_OUT_PARAM_ERROR;
> > + }
> > +
> > + /* Try to read the passed fadump header */
> > + io_result = address_space_read(default_as, fdm_addr, attrs,
> > + &header, sizeof(header));
> > + if (io_result != MEMTX_OK) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "FADump: Unable to read fdm address: %ld\n", fdm_addr);
> > +
> > + return RTAS_OUT_HW_ERROR;
> > + }
> > +
> > + /* Verify that we understand the fadump header version */
> > + if (header.dump_format_version != cpu_to_be32(FADUMP_VERSION)) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "FADump: Unknown fadump header version: 0x%x\n",
> > + header.dump_format_version);
> > + return RTAS_OUT_PARAM_ERROR;
> > + }
> > +
> > + /* Reset dump status flags */
> > + header.dump_status_flag = 0;
> > +
> > + dump_num_sections = be16_to_cpu(header.dump_num_sections);
> > +
> > + if (dump_num_sections > FADUMP_MAX_SECTIONS) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "FADump: Too many sections: %d sections\n", dump_num_sections);
> > + return RTAS_OUT_PARAM_ERROR;
> > + }
> > +
> > + next_section_addr =
> > + fdm_addr +
> > + be32_to_cpu(header.offset_first_dump_section);
> > +
> > + for (int i = 0; i < dump_num_sections; ++i) {
> > + /* Read the fadump section from memory */
> > + io_result = address_space_read(default_as, next_section_addr, attrs,
> > + ®ions[i], sizeof(regions[i]));
> > + if (io_result != MEMTX_OK) {
> > + qemu_log_mask(LOG_UNIMP,
> > + "FADump: Unable to read fadump %dth section\n", i);
> > + return RTAS_OUT_PARAM_ERROR;
> > + }
> > +
> > + next_section_addr += sizeof(regions[i]);
> > + }
> > +
> > + spapr->fadump_registered = true;
> > + spapr->fadump_dump_active = false;
>
> Setting fadump_dump_active is not really required. Ideally, we shouldn't
> reach here
> if it is set to true, or am I missing something?
True, with current change it's not really needed. But to be on safe
side, it's better to keep valid values hence ensuring dump_active is
false, after register.
>
> > +
> > + /* Store the registered fadump memory struct */
> > + spapr->registered_fdm.header = header;
> > + for (int i = 0; i < dump_num_sections; ++i) {
> > + spapr->registered_fdm.rgn[i] = regions[i];
> > + }
> > - return RTAS_OUT_HW_ERROR;
> > + return RTAS_OUT_SUCCESS;
> > }
> > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> > index b8bfa9c33fb5..0454938a01e9 100644
> > --- a/hw/ppc/spapr_rtas.c
> > +++ b/hw/ppc/spapr_rtas.c
> > @@ -366,7 +366,7 @@ static void rtas_configure_kernel_dump(PowerPCCPU *cpu,
> > switch (cmd) {
> > case FADUMP_CMD_REGISTER:
> > - ret_val = do_fadump_register();
> > + ret_val = do_fadump_register(spapr, args);
> > if (ret_val != RTAS_OUT_SUCCESS) {
> > rtas_st(rets, 0, ret_val);
> > return;
> > diff --git a/include/hw/ppc/spapr_fadump.h b/include/hw/ppc/spapr_fadump.h
> > index 45109fd9e137..6abbcb44f353 100644
> > --- a/include/hw/ppc/spapr_fadump.h
> > +++ b/include/hw/ppc/spapr_fadump.h
> > @@ -65,5 +65,5 @@ struct FadumpMemStruct {
> > FadumpSection rgn[FADUMP_MAX_SECTIONS];
> > };
> > -uint32_t do_fadump_register(void);
> > +uint32_t do_fadump_register(struct SpaprMachineState *, target_ulong);
>
> Seems like the forward declaration of struct SpaprMachineState done in 01/08
> is
> because of the above change. Should we do there forward declaration here
> instead?
Yes, it was for this.
Merging patch 1 and 2 in v5, then this will be fine.
Thanks Sourabh !
- Aditya G
>
> > #endif /* PPC_SPAPR_FADUMP_H */
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 1/8] hw/ppc: Implement skeleton code for fadump in PSeries
2025-10-17 11:46 ` Sourabh Jain
@ 2025-10-17 11:56 ` Aditya Gupta
0 siblings, 0 replies; 36+ messages in thread
From: Aditya Gupta @ 2025-10-17 11:56 UTC (permalink / raw)
To: Sourabh Jain
Cc: qemu-devel, qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Harsh Prateek Bora, Mahesh J Salgaonkar, Hari Bathini
> > > > +
> > > > +struct SpaprMachineState;
> > >
> > > Didn't understand the reason for the above forward declaration.
> > Yes, in this patch it doesn't make sense, when merging the 1st and 2nd
> > patch, then we see the use. Idea is as explained below.
> >
> > It is because it's defined in spapr.h, and we use SpaprMachineState at
> > the end of this header in declaration of 'do_fadump_register'.
> >
> > But spapr.h includes spapr_fadump.h, so can't include that else it will
> > become a cyclic dependency.
> >
> > Hence forward declaration was a way to solve that cyclic dependency.
> Yeah I got that while reviewing the second patch in the series.
>
> Why are we merging 1st and 2nd patch?
Based on Harsh's review that it made sense to not have a stub for
register, and implement all register/unregister/invalidate commands in
single patch, hence merging 1st and 2nd patch.
- Aditya G
>
> - Sourabh
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 3/8] hw/ppc: Trigger Fadump boot if fadump is registered
2025-10-17 11:26 ` Sourabh Jain
@ 2025-10-17 11:59 ` Aditya Gupta
0 siblings, 0 replies; 36+ messages in thread
From: Aditya Gupta @ 2025-10-17 11:59 UTC (permalink / raw)
To: Sourabh Jain
Cc: qemu-devel, qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Harsh Prateek Bora, Mahesh J Salgaonkar, Hari Bathini
On 25/10/17 04:56PM, Sourabh Jain wrote:
>
> > <...snip...>
> > + /*
> > + * Mark next boot as fadump boot
> > + *
> > + * Note: These is some bit of assumption involved here, as PAPR doesn't
> > + * specify any use of the dump status flags, nor does the kernel use it
> > + *
> > + * But from description in Table 136 in PAPR v2.13, it looks like:
> > + * FADUMP_STATUS_DUMP_TRIGGERED
> > + * = Dump was triggered by the previous system boot (PAPR says)
> > + * = Next boot will be a fadump boot (My perception)
>
> Can we say assumption made or assumed instead of My perception.
>
> > + *
> > + * FADUMP_STATUS_DUMP_PERFORMED
> > + * = Dump performed (Set to 0 by caller of the
> > + * ibm,configure-kernel-dump call) (PAPR says)
> > + * = Firmware has performed the copying/dump of requested regions
> > + * (My perception)
> > + * = Dump is active for the next boot (My perception)
>
> Same here.
Done. Thanks Sourabh !
- Aditya G
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 4/8] hw/ppc: Preserve memory regions registered for fadump
2025-03-23 17:40 ` [PATCH v4 4/8] hw/ppc: Preserve memory regions registered for fadump Aditya Gupta
@ 2025-10-17 13:06 ` Sourabh Jain
2025-10-17 18:13 ` Aditya Gupta
0 siblings, 1 reply; 36+ messages in thread
From: Sourabh Jain @ 2025-10-17 13:06 UTC (permalink / raw)
To: Aditya Gupta, qemu-devel
Cc: qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Harsh Prateek Bora, Mahesh J Salgaonkar, Hari Bathini
On 23/03/25 23:10, Aditya Gupta wrote:
> While the first kernel boots, it registers memory regions for fadump
> such as:
> * CPU state data (has to be populated by the platform)
> * HPTE state data (has to be populated by the platform)
> * Real Mode Regions (platform should copy it to requested
> destination addresses)
> * OS defined regions (such as parameter save area)
>
> Platform is also expected to modify the 'bytes_dumped' to the length of
> data preserved/copied by platform (ideally same as the source length
> passed by kernel).
>
> The kernel passes source address and length for the memory regions, and
> a destination address to where the memory is to be copied.
>
> Implement the preserving/copying of the Real Mode Regions and the
> Parameter Save Area in QEMU Pseries
>
> Note: As of this patch, the "kernel-dump" device tree entry is still not
> added for the second boot, so after crash, the second kernel will boot
> assuming fadump dump is "NOT" active, and try to register for fadump,
> but since we already have fadump registered in QEMU internal state, the
> register rtas call will fail with: "DUMP ACTIVE"
>
> Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
> ---
> hw/ppc/spapr_fadump.c | 161 ++++++++++++++++++++++++++++++++--
> include/hw/ppc/spapr_fadump.h | 18 ++++
> 2 files changed, 174 insertions(+), 5 deletions(-)
>
> diff --git a/hw/ppc/spapr_fadump.c b/hw/ppc/spapr_fadump.c
> index fedd2cde9a4f..c105b8d21da5 100644
> --- a/hw/ppc/spapr_fadump.c
> +++ b/hw/ppc/spapr_fadump.c
> @@ -123,14 +123,165 @@ uint32_t do_fadump_register(SpaprMachineState *spapr, target_ulong args)
> return RTAS_OUT_SUCCESS;
> }
>
> +/*
> + * Copy the source region of given fadump section, to the destination
> + * address mentioned in the region
> + *
> + * Also set the region's error flag, if the copy fails due to non-existent
> + * address (MEMTX_DECODE_ERROR) or permission issues (MEMTX_ACCESS_ERROR)
> + *
> + * Returns true if successful copy
> + *
> + * Returns false in case of any other error, being treated as hardware
> + * error for fadump purposes
> + */
> +static bool do_preserve_region(FadumpSection *region)
> +{
> + AddressSpace *default_as = &address_space_memory;
> + MemTxResult io_result;
> + MemTxAttrs attrs;
> + uint64_t src_addr, src_len, dest_addr;
> + g_autofree void *copy_buffer = NULL;
> +
> + src_addr = be64_to_cpu(region->source_address);
> + src_len = be64_to_cpu(region->source_len);
> + dest_addr = be64_to_cpu(region->destination_address);
> +
> + /* Mark the memory transaction as privileged memory access */
> + attrs.user = 0;
> + attrs.memory = 1;
> +
> + /*
> + * Optimisation: Skip copy if source and destination are same
> + * (eg. param area)
> + */
> + if (src_addr != dest_addr) {
> + /*
> + * Using 'g_try_malloc' as the source length is coming from kernel,
> + * which can be invalid/huge, due to which allocation can fail
> + */
> + copy_buffer = g_try_malloc(src_len + 1);
The region size could be in GBs. I think it is better to do it in chunks.
And don't we need to free the copy_buffer?
> + if (copy_buffer == NULL) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "FADump: Failed allocating memory (size: %ld) for copying"
> + " reserved memory regions\n", src_len);
> +
> + return false;
> + }
> +
> + /* Copy the source region to destination */
> + io_result = address_space_read(default_as, src_addr, attrs,
> + copy_buffer, src_len);
> + if ((io_result & MEMTX_DECODE_ERROR) ||
> + (io_result & MEMTX_ACCESS_ERROR)) {
> + /*
> + * Invalid source address is not an hardware error, instead
> + * wrong parameter from the kernel.
> + * Return true to let caller know to continue reading other
> + * sections
> + */
> + region->error_flags = FADUMP_ERROR_INVALID_SOURCE_ADDR;
> + region->bytes_dumped = 0;
> + return true;
> + } else if (io_result != MEMTX_OK) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "FADump: Failed to read source region in section: %d\n",
> + region->source_data_type);
> +
> + return false;
> + }
> +
> + io_result = address_space_write(default_as, dest_addr, attrs,
> + copy_buffer, src_len);
> + if ((io_result & MEMTX_DECODE_ERROR) ||
> + (io_result & MEMTX_ACCESS_ERROR)) {
> + /*
> + * Invalid destination address is not an hardware error,
> + * instead wrong parameter from the kernel.
> + * Return true to let caller know to continue reading other
> + * sections
Logging MEMTX_DECODE_ERROR would help identify kernel bugs. I think
we should log this for both read and write.
> + */
> + region->error_flags = FADUMP_ERROR_INVALID_DEST_ADDR;
> + region->bytes_dumped = 0;
Seems like caller is already setting bytes_dump to 0. But even if you
wanna do here.
How about setting region->bytes_dumped = 0 early to avoid setting at
multiple
places?
In cases you need to free the copy_buffer I recommend having an exit label
in this function.
> + return true;
> + } else if (io_result != MEMTX_OK) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "FADump: Failed to write destination in section: %d\n",
> + region->source_data_type);
> +
> + return false;
> + }
> + }
> +
> + /*
> + * Considering address_space_write would have copied the
> + * complete region
> + */
> + region->bytes_dumped = cpu_to_be64(src_len);
> + return true;
> +}
> +
> /* Preserve the memory locations registered for fadump */
> -static bool fadump_preserve_mem(void)
> +static bool fadump_preserve_mem(SpaprMachineState *spapr)
> {
> + FadumpMemStruct *fdm = &spapr->registered_fdm;
> + uint16_t dump_num_sections, data_type;
> +
> + assert(spapr->fadump_registered);
> +
> /*
> - * TODO: Implement preserving memory regions requested during fadump
> - * registration
> + * Handle all sections
> + *
> + * CPU State Data and HPTE regions are handled in their own cases
> + *
> + * RMR regions and any custom OS reserved regions such as parameter
> + * save area, are handled by simply copying the source region to
> + * destination address
> */
> - return false;
> + dump_num_sections = be16_to_cpu(fdm->header.dump_num_sections);
> + for (int i = 0; i < dump_num_sections; ++i) {
> + data_type = be16_to_cpu(fdm->rgn[i].source_data_type);
> +
> + /* Reset error_flags & bytes_dumped for now */
> + fdm->rgn[i].error_flags = 0;
> + fdm->rgn[i].bytes_dumped = 0;
> +
> + /* If kernel did not request for the memory region, then skip it */
> + if (be32_to_cpu(fdm->rgn[i].request_flag) != FADUMP_REQUEST_FLAG) {
> + qemu_log_mask(LOG_UNIMP,
> + "FADump: Skipping copying region as not requested\n");
> + continue;
> + }
> +
> + switch (data_type) {
> + case FADUMP_CPU_STATE_DATA:
> + /* TODO: Add CPU state data */
> + break;
> + case FADUMP_HPTE_REGION:
> + /* TODO: Add hpte state data */
> + break;
> + case FADUMP_REAL_MODE_REGION:
> + case FADUMP_PARAM_AREA:
> + /* Copy the memory region from region's source to its destination */
> + if (!do_preserve_region(&fdm->rgn[i])) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "FADump: Failed to preserve dump section: %d\n",
> + be16_to_cpu(fdm->rgn[i].source_data_type));
> + fdm->header.dump_status_flag |=
> + cpu_to_be16(FADUMP_STATUS_DUMP_ERROR);
> + }
> +
> + break;
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "FADump: Skipping unknown source data type: %d\n", data_type);
> +
> + fdm->rgn[i].error_flags =
> + cpu_to_be16(FADUMP_ERROR_INVALID_DATA_TYPE);
> + }
> + }
> +
> + return true;
This function only returns true. Since caller has some action when this
function
returns false I am considering there is something wrong in this function.
If this suppose to return true always then lets not have return at all.
> }
>
> /*
> @@ -151,7 +302,7 @@ void trigger_fadump_boot(SpaprMachineState *spapr, target_ulong spapr_retcode)
> pause_all_vcpus();
>
> /* Preserve the memory locations registered for fadump */
> - if (!fadump_preserve_mem()) {
> + if (!fadump_preserve_mem(spapr)) {
> /* Failed to preserve the registered memory regions */
> rtas_st(spapr_retcode, 0, RTAS_OUT_HW_ERROR);
>
> diff --git a/include/hw/ppc/spapr_fadump.h b/include/hw/ppc/spapr_fadump.h
> index e484604c1c70..d56ca1d6d651 100644
> --- a/include/hw/ppc/spapr_fadump.h
> +++ b/include/hw/ppc/spapr_fadump.h
> @@ -16,11 +16,29 @@
>
> #define FADUMP_VERSION 1
>
> +/* Firmware provided dump sections */
> +#define FADUMP_CPU_STATE_DATA 0x0001
> +#define FADUMP_HPTE_REGION 0x0002
> +#define FADUMP_REAL_MODE_REGION 0x0011
> +
> +/* OS defined sections */
> +#define FADUMP_PARAM_AREA 0x0100
> +
> +/* Dump request flag */
> +#define FADUMP_REQUEST_FLAG 0x00000001
> +
> /* Dump status flags */
> #define FADUMP_STATUS_DUMP_PERFORMED 0x8000
> #define FADUMP_STATUS_DUMP_TRIGGERED 0x4000
> #define FADUMP_STATUS_DUMP_ERROR 0x2000
>
> +/* Region dump error flags */
> +#define FADUMP_ERROR_INVALID_DATA_TYPE 0x8000
> +#define FADUMP_ERROR_INVALID_SOURCE_ADDR 0x4000
> +#define FADUMP_ERROR_LENGTH_EXCEEDS_SOURCE 0x2000
> +#define FADUMP_ERROR_INVALID_DEST_ADDR 0x1000
> +#define FAUDMP_ERROR_DEST_TOO_SMALL 0x0800
> +
> /*
> * The Firmware Assisted Dump Memory structure supports a maximum of 10 sections
> * in the dump memory structure. Presently, three sections are used for
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 4/8] hw/ppc: Preserve memory regions registered for fadump
2025-10-17 13:06 ` Sourabh Jain
@ 2025-10-17 18:13 ` Aditya Gupta
0 siblings, 0 replies; 36+ messages in thread
From: Aditya Gupta @ 2025-10-17 18:13 UTC (permalink / raw)
To: Sourabh Jain
Cc: qemu-devel, qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Harsh Prateek Bora, Mahesh J Salgaonkar, Hari Bathini
On 25/10/17 06:36PM, Sourabh Jain wrote:
>
>
> On 23/03/25 23:10, Aditya Gupta wrote:
> > <...snip...>
> > + /*
> > + * Optimisation: Skip copy if source and destination are same
> > + * (eg. param area)
> > + */
> > + if (src_addr != dest_addr) {
> > + /*
> > + * Using 'g_try_malloc' as the source length is coming from kernel,
> > + * which can be invalid/huge, due to which allocation can fail
> > + */
> > + copy_buffer = g_try_malloc(src_len + 1);
>
> The region size could be in GBs. I think it is better to do it in chunks.
Sure Sourabh, will do in chunks of 32MB in v5.
>
> And don't we need to free the copy_buffer?
No, since I declare it as 'g_autofree', so it's freed automatically at
all exits.
>
> > + if (copy_buffer == NULL) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "FADump: Failed allocating memory (size: %ld) for copying"
> > + " reserved memory regions\n", src_len);
> > +
> > + return false;
> > + }
> > +
> > + /* Copy the source region to destination */
> > + io_result = address_space_read(default_as, src_addr, attrs,
> > + copy_buffer, src_len);
> > + if ((io_result & MEMTX_DECODE_ERROR) ||
> > + (io_result & MEMTX_ACCESS_ERROR)) {
> > + /*
> > + * Invalid source address is not an hardware error, instead
> > + * wrong parameter from the kernel.
> > + * Return true to let caller know to continue reading other
> > + * sections
> > + */
> > + region->error_flags = FADUMP_ERROR_INVALID_SOURCE_ADDR;
> > + region->bytes_dumped = 0;
> > + return true;
> > + } else if (io_result != MEMTX_OK) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "FADump: Failed to read source region in section: %d\n",
> > + region->source_data_type);
> > +
> > + return false;
> > + }
> > +
> > + io_result = address_space_write(default_as, dest_addr, attrs,
> > + copy_buffer, src_len);
> > + if ((io_result & MEMTX_DECODE_ERROR) ||
> > + (io_result & MEMTX_ACCESS_ERROR)) {
> > + /*
> > + * Invalid destination address is not an hardware error,
> > + * instead wrong parameter from the kernel.
> > + * Return true to let caller know to continue reading other
> > + * sections
>
> Logging MEMTX_DECODE_ERROR would help identify kernel bugs. I think
> we should log this for both read and write.
Makes sense. Will do.
>
> > + */
> > + region->error_flags = FADUMP_ERROR_INVALID_DEST_ADDR;
> > + region->bytes_dumped = 0;
>
> Seems like caller is already setting bytes_dump to 0. But even if you wanna
> do here.
> How about setting region->bytes_dumped = 0 early to avoid setting at
> multiple
> places?
>
> In cases you need to free the copy_buffer I recommend having an exit label
> in this function.
As we declare it as 'g_autofree', 'g_free' will automatically get
invoked when the variable goes out of scope.
>
> > + return true;
> > + } else if (io_result != MEMTX_OK) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "FADump: Failed to write destination in section: %d\n",
> > + region->source_data_type);
> > +
> > + return false;
> > + }
> > + }
> > +
> > <...snip...>
> > + fdm->rgn[i].error_flags =
> > + cpu_to_be16(FADUMP_ERROR_INVALID_DATA_TYPE);
> > + }
> > + }
> > +
> > + return true;
>
> This function only returns true. Since caller has some action when this
> function
> returns false I am considering there is something wrong in this function.
>
> If this suppose to return true always then lets not have return at all.
Agreed return type is of no use as of this patch. The return type being
bool is needed when CPU register saving is implemented, where we return
false, signalling a HW error that we failed to save registers.
Thanks,
- Aditya G
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 5/8] hw/ppc: Implement saving CPU state in Fadump
2025-03-23 17:40 ` [PATCH v4 5/8] hw/ppc: Implement saving CPU state in Fadump Aditya Gupta
@ 2025-10-18 10:54 ` Sourabh Jain
2025-10-19 19:22 ` Aditya Gupta
0 siblings, 1 reply; 36+ messages in thread
From: Sourabh Jain @ 2025-10-18 10:54 UTC (permalink / raw)
To: Aditya Gupta, qemu-devel
Cc: qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Harsh Prateek Bora, Mahesh J Salgaonkar, Hari Bathini
On 23/03/25 23:10, Aditya Gupta wrote:
> Kernel expects CPU states/register states in the format mentioned in
> "Register Save Area" in PAPR.
>
> The platform (in our case, QEMU) saves each CPU register in the form of
> an array of "register entries", the start and end of this array is
> signified by "CPUSTRT" and "CPUEND" register entries respectively.
>
> The CPUSTRT and CPUEND register entry also has 4-byte logical CPU ID,
> thus storing the CPU ID corresponding to the array of register entries.
>
> Each register, and CPUSTRT, CPUEND has a predefined identifier.
> Implement calculating identifier for a given register in
> 'fadump_str_to_u64', which has been taken from the linux kernel
>
> Similarly GPRs also have predefined identifiers, and a corresponding
> 64-bit resiter value (split into two 32-bit cells). Implement
> calculation of GPR identifiers with 'fadump_gpr_id_to_u64'
>
> PAPR has restrictions on particular order of few registers, and is
> free to be in any order for other registers.
> Some registers mentioned in PAPR have not been exported as they are not
> implemented in QEMU / don't make sense in QEMU.
>
> Implement saving of CPU state according to the PAPR document
>
> Note: As of this patch, the "kernel-dump" device tree entry is still not
> added for the second boot, so after crash, the second kernel will boot
> assuming fadump dump is "NOT" active, and try to register for fadump,
> but since we already have fadump registered in QEMU internal state, the
> register rtas call will fail with: "DUMP ACTIVE"
>
> Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
> ---
> hw/ppc/spapr_fadump.c | 336 +++++++++++++++++++++++++++++++++-
> include/hw/ppc/spapr_fadump.h | 28 +++
> 2 files changed, 363 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ppc/spapr_fadump.c b/hw/ppc/spapr_fadump.c
> index c105b8d21da5..fc764b46e726 100644
> --- a/hw/ppc/spapr_fadump.c
> +++ b/hw/ppc/spapr_fadump.c
> @@ -8,6 +8,71 @@
> #include "qemu/log.h"
> #include "hw/ppc/spapr.h"
> #include "system/cpus.h"
> +#include "system/hw_accel.h"
> +
> +/*
> + * Copy the ascii values for first 8 characters from a string into u64
> + * variable at their respective indexes.
> + * e.g.
> + * The string "FADMPINF" will be converted into 0x4641444d50494e46
> + */
> +static uint64_t fadump_str_to_u64(const char *str)
> +{
> + uint64_t val = 0;
> + int i;
> +
> + for (i = 0; i < sizeof(val); i++) {
> + val = (*str) ? (val << 8) | *str++ : val << 8;
> + }
> + return val;
> +}
> +
> +/**
> + * Get the identifier id for register entries of GPRs
> + *
> + * It gives the same id as 'fadump_str_to_u64' when the complete string id
> + * of the GPR is given, ie.
> + *
> + * fadump_str_to_u64("GPR05") == fadump_gpr_id_to_u64(5);
> + * fadump_str_to_u64("GPR12") == fadump_gpr_id_to_u64(12);
> + *
> + * And so on. Hence this can be implemented by creating a dynamic
> + * string for each GPR, such as "GPR00", "GPR01", ... "GPR31"
> + * Instead of allocating a string, an observation from the math of
> + * 'fadump_str_to_u64' or from PAPR tells us that there's a pattern
> + * in the identifier IDs, such that the first 4 bytes are affected only by
> + * whether it is GPR0*, GPR1*, GPR2*, GPR3*.
> + * Upper half of 5th byte is always 0x3. Lower half (nibble) of 5th byte
> + * is the tens digit of the GPR id, ie. GPR ID / 10.
> + * Upper half of 6th byte is always 0x3. Lower half (nibble) of 5th byte
> + * is the ones digit of the GPR id, ie. GPR ID % 10
> + *
> + * For example, for GPR 29, the 5th and 6th byte will be 0x32 and 0x39
> + */
> +static uint64_t fadump_gpr_id_to_u64(uint32_t gpr_id)
> +{
> + uint64_t val = 0;
> +
> + /* Valid range of GPR id is only GPR0 to GPR31 */
> + assert(gpr_id < 32);
> +
> + /* Below calculations set the 0th to 5th byte */
> + if (gpr_id <= 9) {
> + val = fadump_str_to_u64("GPR0");
> + } else if (gpr_id <= 19) {
> + val = fadump_str_to_u64("GPR1");
> + } else if (gpr_id <= 29) {
> + val = fadump_str_to_u64("GPR2");
> + } else {
> + val = fadump_str_to_u64("GPR3");
> + }
> +
> + /* Set the 6th byte */
> + val |= 0x30000000;
> + val |= ((gpr_id % 10) << 24);
> +
> + return val;
> +}
>
> /*
> * Handle the "FADUMP_CMD_REGISTER" command in 'ibm,configure-kernel-dump'
> @@ -221,14 +286,221 @@ static bool do_preserve_region(FadumpSection *region)
> return true;
> }
>
> +/*
> + * Populate the passed CPUs register entries, in the buffer starting at
> + * the argument 'curr_reg_entry'
> + *
> + * The register entries is an array of pair of register id and register
> + * value, as described in Table 591/592 in section "H.1 Register Save Area"
> + * in PAPR v2.13
> + *
> + * Returns pointer just past this CPU's register entries, which can be used
> + * as the start address for next CPU's register entries
> + */
> +static FadumpRegEntry *populate_cpu_reg_entries(CPUState *cpu,
> + FadumpRegEntry *curr_reg_entry)
> +{
> + CPUPPCState *env;
> + PowerPCCPU *ppc_cpu;
> + uint32_t num_regs_per_cpu = 0;
> +
> + ppc_cpu = POWERPC_CPU(cpu);
> + env = cpu_env(cpu);
> + num_regs_per_cpu = 0;
> +
> + curr_reg_entry->reg_id =
> + cpu_to_be64(fadump_str_to_u64("CPUSTRT"));
> + curr_reg_entry->reg_value = ppc_cpu->vcpu_id;
Aren't we suppose to store CPU ID in big endian?
As per PAPR the format CPUSTRT and CPUEND node is
8 Bytes Identifier (BE) | 4 Bytes Reserved (0x0) | 4 Bytes Logical CPU
ID (BE)
It feels like storing vcpu_id as little endian may not get the
reg entrie for CPUSTRT in above format, isn't it?
Ideally we should have two struct to mange two types of Reg Entries.
That way we can manage reserved bytes in CPUSTRT/CPUEND node
easily. I understand that it will bring unnecessary complexity in
populate_cpu_state_data function. So how about adding a note for
future reference?
> + ++curr_reg_entry;
> +
> +#define REG_ENTRY(id, val) \
> + do { \
> + curr_reg_entry->reg_id = \
> + cpu_to_be64(fadump_str_to_u64(#id)); \
> + curr_reg_entry->reg_value = cpu_to_be64(val); \
> + ++curr_reg_entry; \
> + ++num_regs_per_cpu; \
> + } while (0)
> +
> + REG_ENTRY(ACOP, env->spr[SPR_ACOP]);
> + REG_ENTRY(AMR, env->spr[SPR_AMR]);
> + REG_ENTRY(BESCR, env->spr[SPR_BESCR]);
> + REG_ENTRY(CFAR, env->spr[SPR_CFAR]);
> + REG_ENTRY(CIABR, env->spr[SPR_CIABR]);
> +
> + /* Save the condition register */
> + REG_ENTRY(CR, ppc_get_cr(env));
> +
> + REG_ENTRY(CTR, env->spr[SPR_CTR]);
> + REG_ENTRY(CTRL, env->spr[SPR_CTRL]);
> + REG_ENTRY(DABR, env->spr[SPR_DABR]);
> + REG_ENTRY(DABRX, env->spr[SPR_DABRX]);
> + REG_ENTRY(DAR, env->spr[SPR_DAR]);
> + REG_ENTRY(DAWR0, env->spr[SPR_DAWR0]);
> + REG_ENTRY(DAWR1, env->spr[SPR_DAWR1]);
> + REG_ENTRY(DAWRX0, env->spr[SPR_DAWRX0]);
> + REG_ENTRY(DAWRX1, env->spr[SPR_DAWRX1]);
> + REG_ENTRY(DPDES, env->spr[SPR_DPDES]);
> + REG_ENTRY(DSCR, env->spr[SPR_DSCR]);
> + REG_ENTRY(DSISR, env->spr[SPR_DSISR]);
> + REG_ENTRY(EBBHR, env->spr[SPR_EBBHR]);
> + REG_ENTRY(EBBRR, env->spr[SPR_EBBRR]);
> +
> + REG_ENTRY(FPSCR, env->fpscr);
> + REG_ENTRY(FSCR, env->spr[SPR_FSCR]);
> +
> + /* Save the GPRs */
> + for (int gpr_id = 0; gpr_id < 32; ++gpr_id) {
> + curr_reg_entry->reg_id =
> + cpu_to_be64(fadump_gpr_id_to_u64(gpr_id));
> + curr_reg_entry->reg_value =
> + cpu_to_be64(env->gpr[gpr_id]);
> + ++curr_reg_entry;
> + ++num_regs_per_cpu;
> + }
> +
> + REG_ENTRY(IAMR, env->spr[SPR_IAMR]);
> + REG_ENTRY(IC, env->spr[SPR_IC]);
> + REG_ENTRY(LR, env->spr[SPR_LR]);
> +
> + REG_ENTRY(MSR, env->msr);
> + REG_ENTRY(NIA, env->nip); /* NIA */
> + REG_ENTRY(PIR, env->spr[SPR_PIR]);
> + REG_ENTRY(PSPB, env->spr[SPR_PSPB]);
> + REG_ENTRY(PVR, env->spr[SPR_PVR]);
> + REG_ENTRY(RPR, env->spr[SPR_RPR]);
> + REG_ENTRY(SPURR, env->spr[SPR_SPURR]);
> + REG_ENTRY(SRR0, env->spr[SPR_SRR0]);
> + REG_ENTRY(SRR1, env->spr[SPR_SRR1]);
> + REG_ENTRY(TAR, env->spr[SPR_TAR]);
> + REG_ENTRY(TEXASR, env->spr[SPR_TEXASR]);
> + REG_ENTRY(TFHAR, env->spr[SPR_TFHAR]);
> + REG_ENTRY(TFIAR, env->spr[SPR_TFIAR]);
> + REG_ENTRY(TIR, env->spr[SPR_TIR]);
> + REG_ENTRY(UAMOR, env->spr[SPR_UAMOR]);
> + REG_ENTRY(VRSAVE, env->spr[SPR_VRSAVE]);
> + REG_ENTRY(VSCR, env->vscr);
> + REG_ENTRY(VTB, env->spr[SPR_VTB]);
> + REG_ENTRY(WORT, env->spr[SPR_WORT]);
> + REG_ENTRY(XER, env->spr[SPR_XER]);
> +
> + /*
> + * Ignoring transaction checkpoint and few other registers
> + * mentioned in PAPR as not supported in QEMU
> + */
> +#undef REG_ENTRY
> +
> + /* End the registers for this CPU with "CPUEND" reg entry */
> + curr_reg_entry->reg_id =
> + cpu_to_be64(fadump_str_to_u64("CPUEND"));
CPU ID as value to CPUEND reg entry needed, right?
> +
> + /*
> + * Ensure the number of registers match (+2 for STRT & END)
> + *
> + * This will help catch an error if in future a new register entry
> + * is added/removed while not modifying FADUMP_NUM_PER_CPU_REGS
> + */
> + assert(FADUMP_NUM_PER_CPU_REGS == num_regs_per_cpu + 2 /*CPUSTRT+CPUEND*/);
> +
> + ++curr_reg_entry;
> +
> + return curr_reg_entry;
> +}
> +
> +/*
> + * Populate the "Register Save Area"/CPU State as mentioned in section "H.1
> + * Register Save Area" in PAPR v2.13
> + *
> + * It allocates the buffer for this region, then populates the register
> + * entries
> + *
> + * Returns the pointer to the buffer (which should be deallocated by the
> + * callers), and sets the size of this buffer in the argument
> + * 'cpu_state_len'
> + */
> +static void *populate_cpu_state_data(uint64_t *cpu_state_len)
> +{
> + FadumpRegSaveAreaHeader reg_save_hdr;
> + FadumpRegEntry *reg_entries;
> + FadumpRegEntry *curr_reg_entry;
> + CPUState *cpu;
> +
> + uint32_t num_reg_entries;
> + uint32_t reg_entries_size;
> + uint32_t num_cpus = 0;
> +
> + void *cpu_state_buffer = NULL;
> + uint64_t offset = 0;
> +
> + CPU_FOREACH(cpu) {
> + ++num_cpus;
> + }
> +
> + reg_save_hdr.version = cpu_to_be32(0);
> + reg_save_hdr.magic_number =
> + cpu_to_be64(fadump_str_to_u64("REGSAVE"));
> +
> + /* Reg save area header is immediately followed by num cpus */
> + reg_save_hdr.num_cpu_offset =
> + cpu_to_be32(sizeof(FadumpRegSaveAreaHeader));
> +
> + num_reg_entries = num_cpus * FADUMP_NUM_PER_CPU_REGS;
> + reg_entries_size = num_reg_entries * sizeof(FadumpRegEntry);
> +
> + reg_entries = g_new(FadumpRegEntry, num_reg_entries);
> +
> + /* Pointer to current CPU's registers */
> + curr_reg_entry = reg_entries;
> +
> + /* Populate register entries for all CPUs */
> + CPU_FOREACH(cpu) {
> + cpu_synchronize_state(cpu);
> + curr_reg_entry = populate_cpu_reg_entries(cpu, curr_reg_entry);
> + }
> +
> + *cpu_state_len = 0;
> + *cpu_state_len += sizeof(reg_save_hdr); /* reg save header */
> + *cpu_state_len += 0xc; /* padding as in PAPR */
> + *cpu_state_len += sizeof(__be32); /* num_cpus */
> + *cpu_state_len += reg_entries_size; /* reg entries */
> +
> + cpu_state_buffer = g_malloc(*cpu_state_len);
> +
> + memcpy(cpu_state_buffer + offset,
> + ®_save_hdr, sizeof(reg_save_hdr));
> + offset += sizeof(reg_save_hdr);
> +
> + /* Write num_cpus */
> + num_cpus = cpu_to_be32(num_cpus);
> + memcpy(cpu_state_buffer + offset, &num_cpus, sizeof(__be32));
> + offset += sizeof(__be32);
> +
> + /* Write the register entries */
> + memcpy(cpu_state_buffer + offset, reg_entries, reg_entries_size);
> + offset += reg_entries_size;
> +
> + return cpu_state_buffer;
> +}
> +
> /* Preserve the memory locations registered for fadump */
> static bool fadump_preserve_mem(SpaprMachineState *spapr)
> {
> FadumpMemStruct *fdm = &spapr->registered_fdm;
> + FadumpSection *cpu_state_region = NULL;
> + AddressSpace *default_as = &address_space_memory;
> + MemTxResult io_result;
> + MemTxAttrs attrs;
> uint16_t dump_num_sections, data_type;
> + uint64_t dest_addr;
> + uint64_t cpu_state_addr, cpu_state_len = 0;
> + g_autofree void *cpu_state_buffer = NULL;
>
> assert(spapr->fadump_registered);
>
> + /* Mark the memory transaction as privileged memory access */
> + attrs.user = 0;
> + attrs.memory = 1;
> +
> /*
> * Handle all sections
> *
> @@ -241,6 +513,7 @@ static bool fadump_preserve_mem(SpaprMachineState *spapr)
> dump_num_sections = be16_to_cpu(fdm->header.dump_num_sections);
> for (int i = 0; i < dump_num_sections; ++i) {
> data_type = be16_to_cpu(fdm->rgn[i].source_data_type);
> + dest_addr = be64_to_cpu(fdm->rgn[i].destination_address);
>
> /* Reset error_flags & bytes_dumped for now */
> fdm->rgn[i].error_flags = 0;
> @@ -255,7 +528,15 @@ static bool fadump_preserve_mem(SpaprMachineState *spapr)
>
> switch (data_type) {
> case FADUMP_CPU_STATE_DATA:
> - /* TODO: Add CPU state data */
> + cpu_state_region = &fdm->rgn[i];
> + cpu_state_addr = dest_addr;
> + cpu_state_buffer = populate_cpu_state_data(&cpu_state_len);
> +
> + /*
> + * We will write the cpu state data later, as otherwise it
> + * might get overwritten by other fadump regions
Isn't the destination address of cpu state data and RMR region are
different?
I don't understand the above comment. Can you please give more details.
> + */
> +
> break;
> case FADUMP_HPTE_REGION:
> /* TODO: Add hpte state data */
> @@ -281,6 +562,59 @@ static bool fadump_preserve_mem(SpaprMachineState *spapr)
> }
> }
>
> + /* CPU State Region has not been requested by kernel */
> + if (!cpu_state_region) {
> + return true;
> + }
> +
> + /*
> + * Write the Register Save Area
> + *
> + * CPU State/Register Save Area should be written after dumping the
> + * memory to prevent overwriting while saving other memory regions
> + *
> + * eg. If boot memory region is 1G, then both the first 1GB memory, and
> + * the Register Save Area needs to be saved at 1GB.
Every region is copied to their destination address and as per below
kernel function:
https://github.com/torvalds/linux/blob/f406055cb18c6e299c4a783fc1effeb16be41803/arch/powerpc/platforms/pseries/rtas-fadump.c#L98
the destination address shouldn't be same for fadump region then how
come there
is overwrite scenario? Am I missing something?
> + * And as the CPU_STATE_DATA region comes first than the
> + * REAL_MODE_REGION region to be copied, the CPU_STATE_DATA will get
> + * overwritten if saved before the 0GB - 1GB region is copied after
> + * saving CPU state data
> + */
> + io_result = address_space_write(default_as, cpu_state_addr, attrs,
> + cpu_state_buffer, cpu_state_len);
Hope cpu_state_buffer will be freed automatically...
> + if ((io_result & MEMTX_DECODE_ERROR) ||
> + (io_result & MEMTX_ACCESS_ERROR)) {
> + /*
> + * Invalid destination address is not an hardware error, instead
> + * wrong parameter from the kernel.
> + * Return true to let caller know to continue reading other
> + * sections
> + */
> + cpu_state_region->error_flags = FADUMP_ERROR_INVALID_DEST_ADDR;
> + cpu_state_region->bytes_dumped = 0;
> + return true;
> + } else if (io_result != MEMTX_OK) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "FADump: Failed to write CPU state region\n");
> + cpu_state_region->bytes_dumped = 0;
> +
> + return false;
> + }
> +
> + /*
> + * Set bytes_dumped in cpu state region, so kernel knows platform have
> + * exported it
> + */
> + cpu_state_region->bytes_dumped = cpu_to_be64(cpu_state_len);
> +
> + if (cpu_state_region->source_len != cpu_state_region->bytes_dumped) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "CPU State region's length passed by kernel (0x%lx) !="
> + " CPU State region's length exported by QEMU (0x%lx)\n",
> + be64_to_cpu(cpu_state_region->source_len),
> + be64_to_cpu(cpu_state_region->bytes_dumped));
> + }
> +
> return true;
> }
>
> diff --git a/include/hw/ppc/spapr_fadump.h b/include/hw/ppc/spapr_fadump.h
> index d56ca1d6d651..13bdd39a9ec1 100644
> --- a/include/hw/ppc/spapr_fadump.h
> +++ b/include/hw/ppc/spapr_fadump.h
> @@ -48,9 +48,14 @@
> #define FADUMP_MAX_SECTIONS 10
> #define RTAS_FADUMP_MAX_BOOT_MEM_REGS 7
>
> +/* Number of registers stored per cpu */
> +#define FADUMP_NUM_PER_CPU_REGS (32 /*GPR*/ + 45 /*others*/ + 2 /*STRT & END*/)
#nit-pick
How about FADUMP_PER_CPU_REG_ENTRY ?
> +
> typedef struct FadumpSection FadumpSection;
> typedef struct FadumpSectionHeader FadumpSectionHeader;
> typedef struct FadumpMemStruct FadumpMemStruct;
> +typedef struct FadumpRegSaveAreaHeader FadumpRegSaveAreaHeader;
> +typedef struct FadumpRegEntry FadumpRegEntry;
>
> struct SpaprMachineState;
>
> @@ -88,6 +93,29 @@ struct FadumpMemStruct {
> FadumpSection rgn[FADUMP_MAX_SECTIONS];
> };
>
> +/*
> + * The firmware-assisted dump format.
> + *
> + * The register save area is an area in the partition's memory used to preserve
> + * the register contents (CPU state data) for the active CPUs during a firmware
> + * assisted dump. The dump format contains register save area header followed
> + * by register entries. Each list of registers for a CPU starts with "CPUSTRT"
> + * and ends with "CPUEND".
> + */
> +
> +/* Register save area header. */
> +struct FadumpRegSaveAreaHeader {
> + __be64 magic_number;
> + __be32 version;
> + __be32 num_cpu_offset;
> +};
> +
> +/* Register entry. */
> +struct FadumpRegEntry {
> + __be64 reg_id;
> + __be64 reg_value;
> +};
> +
> uint32_t do_fadump_register(struct SpaprMachineState *, target_ulong);
> void trigger_fadump_boot(struct SpaprMachineState *, target_ulong);
> #endif /* PPC_SPAPR_FADUMP_H */
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 6/8] hw/ppc: Pass dump-sizes property for fadump in device tree
2025-03-23 17:40 ` [PATCH v4 6/8] hw/ppc: Pass dump-sizes property for fadump in device tree Aditya Gupta
@ 2025-10-18 11:20 ` Sourabh Jain
2025-10-19 19:30 ` Aditya Gupta
0 siblings, 1 reply; 36+ messages in thread
From: Sourabh Jain @ 2025-10-18 11:20 UTC (permalink / raw)
To: Aditya Gupta, qemu-devel
Cc: qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Harsh Prateek Bora, Mahesh J Salgaonkar, Hari Bathini
On 23/03/25 23:10, Aditya Gupta wrote:
> Platform (ie. QEMU) is expected to pass few device tree properties for
> details for fadump:
>
> * "ibm,configure-kernel-dump-sizes": Space required to store dump data
> for firmware provided dump sections (ie. CPU & HPTE regions)
> * "ibm,configure-kernel-dump-version": Versions of fadump supported
>
> Pass the above device tree nodes so that kernel can reserve sufficient
> space for preserving the CPU state data
>
> Note: As of this patch, the "kernel-dump" device tree entry is still not
> added for the second boot, so after crash, the second kernel will boot
> assuming fadump dump is "NOT" active, and try to register for fadump,
> but since we already have fadump registered in QEMU internal state, the
> register rtas call will fail with: "DUMP ACTIVE"
>
> Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
> ---
> hw/ppc/spapr.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 57 insertions(+)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a415e51d077a..3cbc6a7409b7 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -900,6 +900,61 @@ static int spapr_dt_rng(void *fdt)
> return ret ? -1 : 0;
> }
>
> +static void spapr_dt_rtas_fadump(SpaprMachineState *spapr, void *fdt, int rtas)
> +{
> + MachineState *ms = MACHINE(spapr);
> + MachineClass *mc = MACHINE_GET_CLASS(ms);
> +
> + uint32_t max_possible_cpus = mc->possible_cpu_arch_ids(ms)->len;
> + uint64_t fadump_cpu_state_size = 0;
> + uint16_t fadump_versions[2] = {
> + FADUMP_VERSION /* min supported version */,
> + FADUMP_VERSION /* max supported version */
> + };
> + uint32_t fadump_rgn_sizes[2][3] = {
> + {
> + cpu_to_be32(FADUMP_CPU_STATE_DATA),
> + 0, 0 /* Calculated later */
> + },
> + {
> + cpu_to_be32(FADUMP_HPTE_REGION),
> + 0, 0 /* HPTE region not implemented */
> + }
#nit-pick
Why to advertise if we don't support it? Kernel anyways ignores this for
now.
> + };
> +
> + /*
> + * CPU State Data contains multiple fields such as header, num_cpus and
> + * register entries
> + *
> + * Calculate the maximum CPU State Data size, according to maximum
> + * possible CPUs the QEMU VM can have
> + *
> + * This calculation must match the 'cpu_state_len' calculation done in
> + * 'populate_cpu_state_data' in spapr_fadump.c
> + */
> + fadump_cpu_state_size += sizeof(struct FadumpRegSaveAreaHeader);
> + fadump_cpu_state_size += 0xc; /* padding as in PAPR */
> + fadump_cpu_state_size += sizeof(__be32); /* num_cpus */
> + fadump_cpu_state_size += max_possible_cpus * /* reg entries */
> + FADUMP_NUM_PER_CPU_REGS *
> + sizeof(struct FadumpRegEntry);
> +
> + /* Set maximum size for CPU state data region */
> + assert(fadump_rgn_sizes[0][0] == cpu_to_be32(FADUMP_CPU_STATE_DATA));
> +
> + /* Upper 32 bits of size, usually 0 */
> + fadump_rgn_sizes[0][1] = cpu_to_be32(fadump_cpu_state_size >> 32);
> +
> + /* Lower 32 bits of size */
> + fadump_rgn_sizes[0][2] = cpu_to_be32(fadump_cpu_state_size & 0xffffffff);
> +
> + /* Add device tree properties required from platform for fadump */
> + _FDT((fdt_setprop(fdt, rtas, "ibm,configure-kernel-dump-version",
> + fadump_versions, sizeof(fadump_versions))));
> + _FDT((fdt_setprop(fdt, rtas, "ibm,configure-kernel-dump-sizes",
> + fadump_rgn_sizes, sizeof(fadump_rgn_sizes))));
> +}
> +
> static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt)
> {
> MachineState *ms = MACHINE(spapr);
> @@ -1012,6 +1067,8 @@ static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt)
> _FDT(fdt_setprop(fdt, rtas, "ibm,lrdr-capacity",
> lrdr_capacity, sizeof(lrdr_capacity)));
>
> + spapr_dt_rtas_fadump(spapr, fdt, rtas);
> +
> spapr_dt_rtas_tokens(fdt, rtas);
> }
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 1/8] hw/ppc: Implement skeleton code for fadump in PSeries
2025-03-23 17:40 ` [PATCH v4 1/8] hw/ppc: Implement skeleton code for fadump in PSeries Aditya Gupta
2025-04-21 10:51 ` Harsh Prateek Bora
2025-10-17 8:40 ` Sourabh Jain
@ 2025-10-18 11:50 ` Sourabh Jain
2025-10-19 11:30 ` Aditya Gupta
2 siblings, 1 reply; 36+ messages in thread
From: Sourabh Jain @ 2025-10-18 11:50 UTC (permalink / raw)
To: Aditya Gupta, qemu-devel
Cc: qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Harsh Prateek Bora, Mahesh J Salgaonkar, Hari Bathini
On 23/03/25 23:10, Aditya Gupta wrote:
> Add skeleton for handle "ibm,configure-kernel-dump" rtas call in QEMU.
>
> Verify basic details mandated by the PAPR, such as number of
> inputs/output, and add handling for the three fadump commands:
> regiser/unregister/invalidate.
>
> Currently fadump register will always return HARDWARE ERROR, since it's
> not implemented yet. So if the kernel's attempt to register fadump will
> itself fail as the support is not there yet in QEMU.
>
> The checks are based on the table in following requirement in PAPR v2.13:
> "R1–7.3.30–1. For the Configure Platform Assisted Kernel Dump option ..."
>
> Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
> ---
> hw/ppc/meson.build | 1 +
> hw/ppc/spapr_fadump.c | 22 +++++++++++
> hw/ppc/spapr_rtas.c | 66 +++++++++++++++++++++++++++++++++
> include/hw/ppc/spapr.h | 11 +++++-
> include/hw/ppc/spapr_fadump.h | 69 +++++++++++++++++++++++++++++++++++
> 5 files changed, 168 insertions(+), 1 deletion(-)
> create mode 100644 hw/ppc/spapr_fadump.c
> create mode 100644 include/hw/ppc/spapr_fadump.h
>
> diff --git a/hw/ppc/meson.build b/hw/ppc/meson.build
> index 9893f8adebb0..863972741b15 100644
> --- a/hw/ppc/meson.build
> +++ b/hw/ppc/meson.build
> @@ -26,6 +26,7 @@ ppc_ss.add(when: 'CONFIG_PSERIES', if_true: files(
> 'spapr_nvdimm.c',
> 'spapr_rtas_ddw.c',
> 'spapr_numa.c',
> + 'spapr_fadump.c',
> 'pef.c',
> ))
> ppc_ss.add(when: ['CONFIG_PSERIES', 'CONFIG_TCG'], if_true: files(
> diff --git a/hw/ppc/spapr_fadump.c b/hw/ppc/spapr_fadump.c
> new file mode 100644
> index 000000000000..20b7b804c485
> --- /dev/null
> +++ b/hw/ppc/spapr_fadump.c
> @@ -0,0 +1,22 @@
> +/*
> + * Firmware Assisted Dump in PSeries
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/ppc/spapr.h"
> +
> +/*
> + * Handle the "FADUMP_CMD_REGISTER" command in 'ibm,configure-kernel-dump'
> + *
> + * Returns:
> + * * RTAS_OUT_HW_ERROR: Not implemented/Misc issue such as memory access
> + * failures
> + */
> +uint32_t do_fadump_register(void)
> +{
> + /* WIP: FADUMP_CMD_REGISTER implemented in future patch */
> +
> + return RTAS_OUT_HW_ERROR;
> +}
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 503d441b48e4..b8bfa9c33fb5 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -341,6 +341,68 @@ static void rtas_ibm_set_system_parameter(PowerPCCPU *cpu,
> rtas_st(rets, 0, ret);
> }
>
> +/* Papr Section 7.4.9 ibm,configure-kernel-dump RTAS call */
> +static void rtas_configure_kernel_dump(PowerPCCPU *cpu,
> + SpaprMachineState *spapr,
> + uint32_t token, uint32_t nargs,
> + target_ulong args,
> + uint32_t nret, target_ulong rets)
> +{
> + target_ulong cmd = rtas_ld(args, 0);
> + uint32_t ret_val;
> +
> + /* Number of 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 of inputs has to be 3 */
> + if (nargs != 3) {
> + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> + return;
> + }
> +
> + switch (cmd) {
> + case FADUMP_CMD_REGISTER:
> + ret_val = do_fadump_register();
> + if (ret_val != RTAS_OUT_SUCCESS) {
> + rtas_st(rets, 0, ret_val);
> + return;
> + }
> + break;
> + case FADUMP_CMD_UNREGISTER:
> + if (spapr->fadump_dump_active == 1) {
> + rtas_st(rets, 0, RTAS_OUT_DUMP_ACTIVE);
> + return;
> + }
> +
> + spapr->fadump_registered = false;
> + spapr->fadump_dump_active = false;
> + memset(&spapr->registered_fdm, 0, sizeof(spapr->registered_fdm));
> + break;
> + case FADUMP_CMD_INVALIDATE:
> + if (spapr->fadump_dump_active) {
> + spapr->fadump_registered = false;
> + spapr->fadump_dump_active = false;
> + memset(&spapr->registered_fdm, 0, sizeof(spapr->registered_fdm));
I hope the above actions are good enough to make qemu ready for fadump
registration.
> + } else {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "FADump: Nothing to invalidate, no dump active\n");
> + }
No error code if the kernel issues an invalidate and fadump_dump_active
is false?
As per the current kernel code, this situation should not occur, but to
be on the safe side,
QEMU should not return RTAS_OUT_SUCCESS.
> + break;
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "FADump: Unknown command: %lu\n", cmd);
> +
> + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> + return;
> + }
> +
> + rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> +}
> +
> static void rtas_ibm_os_term(PowerPCCPU *cpu,
> SpaprMachineState *spapr,
> uint32_t token, uint32_t nargs,
> @@ -656,6 +718,10 @@ static void core_rtas_register_types(void)
> spapr_rtas_register(RTAS_IBM_NMI_INTERLOCK, "ibm,nmi-interlock",
> rtas_ibm_nmi_interlock);
>
> + /* Register fadump rtas call */
> + spapr_rtas_register(RTAS_CONFIGURE_KERNEL_DUMP, "ibm,configure-kernel-dump",
> + rtas_configure_kernel_dump);
> +
> qtest_set_command_cb(spapr_qtest_callback);
> }
>
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 39bd5bd5ed31..4c1636497e30 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -13,6 +13,7 @@
> #include "hw/ppc/xics.h" /* For ICSState */
> #include "hw/ppc/spapr_tpm_proxy.h"
> #include "hw/ppc/spapr_nested.h" /* For SpaprMachineStateNested */
> +#include "hw/ppc/spapr_fadump.h" /* For FadumpMemStruct */
>
> struct SpaprVioBus;
> struct SpaprPhbState;
> @@ -283,6 +284,11 @@ struct SpaprMachineState {
> Error *fwnmi_migration_blocker;
>
> SpaprWatchdog wds[WDT_MAX_WATCHDOGS];
> +
> + /* Fadump State */
> + bool fadump_registered;
> + bool fadump_dump_active;
> + FadumpMemStruct registered_fdm;
> };
>
> #define H_SUCCESS 0
> @@ -708,6 +714,8 @@ void push_sregs_to_kvm_pr(SpaprMachineState *spapr);
> #define RTAS_OUT_PARAM_ERROR -3
> #define RTAS_OUT_NOT_SUPPORTED -3
> #define RTAS_OUT_NO_SUCH_INDICATOR -3
> +#define RTAS_OUT_DUMP_ALREADY_REGISTERED -9
> +#define RTAS_OUT_DUMP_ACTIVE -10
> #define RTAS_OUT_NOT_AUTHORIZED -9002
> #define RTAS_OUT_SYSPARM_PARAM_ERROR -9999
>
> @@ -770,8 +778,9 @@ void push_sregs_to_kvm_pr(SpaprMachineState *spapr);
> #define RTAS_IBM_SUSPEND_ME (RTAS_TOKEN_BASE + 0x2A)
> #define RTAS_IBM_NMI_REGISTER (RTAS_TOKEN_BASE + 0x2B)
> #define RTAS_IBM_NMI_INTERLOCK (RTAS_TOKEN_BASE + 0x2C)
> +#define RTAS_CONFIGURE_KERNEL_DUMP (RTAS_TOKEN_BASE + 0x2D)
>
> -#define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x2D)
> +#define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x2E)
>
> /* RTAS ibm,get-system-parameter token values */
> #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS 20
> diff --git a/include/hw/ppc/spapr_fadump.h b/include/hw/ppc/spapr_fadump.h
> new file mode 100644
> index 000000000000..45109fd9e137
> --- /dev/null
> +++ b/include/hw/ppc/spapr_fadump.h
> @@ -0,0 +1,69 @@
> +/*
> + * Firmware Assisted Dump in PSeries
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +#ifndef PPC_SPAPR_FADUMP_H
> +#define PPC_SPAPR_FADUMP_H
> +
> +#include "qemu/osdep.h"
> +#include "cpu.h"
> +
> +/* Fadump commands */
> +#define FADUMP_CMD_REGISTER 1
> +#define FADUMP_CMD_UNREGISTER 2
> +#define FADUMP_CMD_INVALIDATE 3
> +
> +#define FADUMP_VERSION 1
> +
> +/*
> + * The Firmware Assisted Dump Memory structure supports a maximum of 10 sections
> + * in the dump memory structure. Presently, three sections are used for
> + * CPU state data, HPTE & Parameters area, while the remaining seven sections
> + * can be used for boot memory regions.
> + */
> +#define FADUMP_MAX_SECTIONS 10
> +#define RTAS_FADUMP_MAX_BOOT_MEM_REGS 7
> +
> +typedef struct FadumpSection FadumpSection;
> +typedef struct FadumpSectionHeader FadumpSectionHeader;
> +typedef struct FadumpMemStruct FadumpMemStruct;
> +
> +struct SpaprMachineState;
> +
> +/* Kernel Dump section info */
> +struct FadumpSection {
> + __be32 request_flag;
> + __be16 source_data_type;
> + __be16 error_flags;
> + __be64 source_address;
> + __be64 source_len;
> + __be64 bytes_dumped;
> + __be64 destination_address;
> +};
> +
> +/* ibm,configure-kernel-dump header. */
> +struct FadumpSectionHeader {
> + __be32 dump_format_version;
> + __be16 dump_num_sections;
> + __be16 dump_status_flag;
> + __be32 offset_first_dump_section;
> +
> + /* Fields for disk dump option. */
> + __be32 dd_block_size;
> + __be64 dd_block_offset;
> + __be64 dd_num_blocks;
> + __be32 dd_offset_disk_path;
> +
> + /* Maximum time allowed to prevent an automatic dump-reboot. */
> + __be32 max_time_auto;
> +};
> +
> +/* Note: All the data in these structures is in big-endian */
> +struct FadumpMemStruct {
> + FadumpSectionHeader header;
> + FadumpSection rgn[FADUMP_MAX_SECTIONS];
> +};
> +
> +uint32_t do_fadump_register(void);
> +#endif /* PPC_SPAPR_FADUMP_H */
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 7/8] hw/ppc: Enable fadump for PSeries
2025-03-23 17:40 ` [PATCH v4 7/8] hw/ppc: Enable fadump for PSeries Aditya Gupta
@ 2025-10-18 12:04 ` Sourabh Jain
2025-10-20 19:44 ` Aditya Gupta
0 siblings, 1 reply; 36+ messages in thread
From: Sourabh Jain @ 2025-10-18 12:04 UTC (permalink / raw)
To: Aditya Gupta, qemu-devel
Cc: qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Harsh Prateek Bora, Mahesh J Salgaonkar, Hari Bathini
On 23/03/25 23:10, Aditya Gupta wrote:
> With all support in place for preserving memory regions, enable fadump by
> exporting the "ibm,kernel-dump" property in the device tree, representing
> the fadump dump information, in case of a crash.
>
> Currently "ibm,configure-kernel-dump" RTAS call is already registered,
> which tells the kernel that the platform (QEMU) supports fadump.
>
> Now, in case of a crash, if fadump was registered, we also pass
> "ibm,kernel-dump" in device tree, which tells the kernel that the fadump
> dump is active.
>
> Pass "fadump=on" to enable Linux to use firmware assisted dump.
>
> Logs of a linux boot with firmware assisted dump:
>
> $ ./build/qemu-system-ppc64 -M pseries,x-vof=on --cpu power10 --smp 4 -m 4G -kernel some-vmlinux -initrd some-initrd -append "debug fadump=on crashkernel=1G" -nographic
>
> [ 0.000000] fadump: Reserved 1024MB of memory at 0x00000040000000 (System RAM: 4096MB)
> [ 0.000000] fadump: Initialized 0x40000000 bytes cma area at 1024MB from 0x400102a8 bytes of memory reserved for firmware-assisted dump
> ...
> [ 1.084686] rtas fadump: Registration is successful!
> ...
> # cat /sys/kernel/debug/powerpc/fadump_region
> CPU :[0x00000040000000-0x000000400013df] 0x13e0 bytes, Dumped: 0x0
> HPTE:[0x000000400013e0-0x000000400013df] 0x0 bytes, Dumped: 0x0
> DUMP: Src: 0x00000000000000, Dest: 0x00000040010000, Size: 0x40000000, Dumped: 0x0 bytes
>
> [0x0000000921a000-0x0000000921a7ff]: cmdline append: ''
> # echo c > /proc/sysrq-trigger
>
> The fadump boot after crash:
>
> [ 0.000000] rtas fadump: Firmware-assisted dump is active.
> [ 0.000000] fadump: Updated cmdline: debug fadump=on crashkernel=1G
> [ 0.000000] fadump: Firmware-assisted dump is active.
Kernel is printing twice about fadump is active. :)
> [ 0.000000] fadump: Reserving 3072MB of memory at 0x00000040000000 for preserving crash data
> ....
> # file /proc/vmcore
> /proc/vmcore: ELF 64-bit LSB core file, 64-bit PowerPC or cisco 7500, OpenPOWER ELF V2 ABI, version 1 (SYSV), SVR4-style
>
> Analysing the vmcore with crash-utility:
>
> KERNEL: vmlinux-6.14-rc2
> DUMPFILE: vmcore-fc92fb373aa0
> CPUS: 4
> DATE: Wed Mar 12 23:39:23 CDT 2025
> UPTIME: 00:00:22
> LOAD AVERAGE: 0.13, 0.03, 0.01
> TASKS: 95
> NODENAME: buildroot
> RELEASE: 6.12.0-rc4+
> VERSION: #1 SMP Fri Jan 3 00:15:17 IST 2025
> MACHINE: ppc64le (1000 Mhz)
> MEMORY: 4 GB
> PANIC: "Kernel panic - not syncing: sysrq triggered crash"
> PID: 269
> COMMAND: "sh"
> TASK: c00000000a050b00 [THREAD_INFO: c00000000a050b00]
> CPU: 0
> STATE: TASK_RUNNING (PANIC)
>
> Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
> ---
> hw/ppc/spapr.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 3cbc6a7409b7..f6e666ad4344 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -904,6 +904,9 @@ static void spapr_dt_rtas_fadump(SpaprMachineState *spapr, void *fdt, int rtas)
> {
> MachineState *ms = MACHINE(spapr);
> MachineClass *mc = MACHINE_GET_CLASS(ms);
> + FadumpMemStruct *fdm = &spapr->registered_fdm;
> + uint16_t dump_status_flag;
> + bool is_next_boot_fadump;
>
> uint32_t max_possible_cpus = mc->possible_cpu_arch_ids(ms)->len;
> uint64_t fadump_cpu_state_size = 0;
> @@ -953,6 +956,18 @@ static void spapr_dt_rtas_fadump(SpaprMachineState *spapr, void *fdt, int rtas)
> fadump_versions, sizeof(fadump_versions))));
> _FDT((fdt_setprop(fdt, rtas, "ibm,configure-kernel-dump-sizes",
> fadump_rgn_sizes, sizeof(fadump_rgn_sizes))));
> +
> + dump_status_flag = be16_to_cpu(fdm->header.dump_status_flag);
> + is_next_boot_fadump =
Do we really need is_next_boot_fadump variable?
> + (dump_status_flag & FADUMP_STATUS_DUMP_TRIGGERED) != 0;
> + if (is_next_boot_fadump) {
> + uint64_t fdm_size =
> + sizeof(struct FadumpSectionHeader) +
> + (be16_to_cpu(fdm->header.dump_num_sections) *
> + sizeof(struct FadumpSection));
> +
> + _FDT((fdt_setprop(fdt, rtas, "ibm,kernel-dump", fdm, fdm_size)));
Is this common in QEMU to call _FDT to add prop to the FDT? Feels odd.
> + }
> }
>
> static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt)
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 1/8] hw/ppc: Implement skeleton code for fadump in PSeries
2025-10-18 11:50 ` Sourabh Jain
@ 2025-10-19 11:30 ` Aditya Gupta
0 siblings, 0 replies; 36+ messages in thread
From: Aditya Gupta @ 2025-10-19 11:30 UTC (permalink / raw)
To: Sourabh Jain
Cc: qemu-devel, qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Harsh Prateek Bora, Mahesh J Salgaonkar, Hari Bathini
On 25/10/18 05:20PM, Sourabh Jain wrote:
>
> > <...snip...>
> > + switch (cmd) {
> > + case FADUMP_CMD_REGISTER:
> > + ret_val = do_fadump_register();
> > + if (ret_val != RTAS_OUT_SUCCESS) {
> > + rtas_st(rets, 0, ret_val);
> > + return;
> > + }
> > + break;
> > + case FADUMP_CMD_UNREGISTER:
> > + if (spapr->fadump_dump_active == 1) {
> > + rtas_st(rets, 0, RTAS_OUT_DUMP_ACTIVE);
> > + return;
> > + }
> > +
> > + spapr->fadump_registered = false;
> > + spapr->fadump_dump_active = false;
> > + memset(&spapr->registered_fdm, 0, sizeof(spapr->registered_fdm));
> > + break;
> > + case FADUMP_CMD_INVALIDATE:
> > + if (spapr->fadump_dump_active) {
> > + spapr->fadump_registered = false;
> > + spapr->fadump_dump_active = false;
> > + memset(&spapr->registered_fdm, 0, sizeof(spapr->registered_fdm));
>
> I hope the above actions are good enough to make qemu ready for fadump
> registration.
I believe so. The various flags are set in do_fadump_register for
register command, maybe that's why that switch case might not look
enough, but I think the current one makes sense.
>
> > + } else {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "FADump: Nothing to invalidate, no dump active\n");
> > + }
>
> No error code if the kernel issues an invalidate and fadump_dump_active is
> false?
>
> As per the current kernel code, this situation should not occur, but to be
> on the safe side,
> QEMU should not return RTAS_OUT_SUCCESS.
Makes sense. PAPR doesn't say anything about this, but I guess we can
return PARAM_ERROR (for lack of any better error) in this case.
Will do.
Thanks,
- Aditya G
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 5/8] hw/ppc: Implement saving CPU state in Fadump
2025-10-18 10:54 ` Sourabh Jain
@ 2025-10-19 19:22 ` Aditya Gupta
2025-10-20 5:40 ` Sourabh Jain
0 siblings, 1 reply; 36+ messages in thread
From: Aditya Gupta @ 2025-10-19 19:22 UTC (permalink / raw)
To: Sourabh Jain
Cc: qemu-devel, qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Harsh Prateek Bora, Mahesh J Salgaonkar, Hari Bathini
On 25/10/18 04:24PM, Sourabh Jain wrote:
>
> > <...snip...>
> > + curr_reg_entry->reg_id =
> > + cpu_to_be64(fadump_str_to_u64("CPUSTRT"));
> > + curr_reg_entry->reg_value = ppc_cpu->vcpu_id;
>
> Aren't we suppose to store CPU ID in big endian?
> As per PAPR the format CPUSTRT and CPUEND node is
>
> 8 Bytes Identifier (BE) | 4 Bytes Reserved (0x0) | 4 Bytes Logical CPU ID
> (BE)
>
> It feels like storing vcpu_id as little endian may not get the
> reg entrie for CPUSTRT in above format, isn't it?
Will fix in v5.
>
> Ideally we should have two struct to mange two types of Reg Entries.
> That way we can manage reserved bytes in CPUSTRT/CPUEND node
> easily. I understand that it will bring unnecessary complexity in
> populate_cpu_state_data function. So how about adding a note for
> future reference?
Will add a note about the format.
I also think another struct might be unnecessary complexity since we
just need it at 2 instances (CPUSTRT & CPUEND), and can be done with
simple bit math.
>
> > + ++curr_reg_entry;
> > +
> > <...snip...>
> > + /* End the registers for this CPU with "CPUEND" reg entry */
> > + curr_reg_entry->reg_id =
> > + cpu_to_be64(fadump_str_to_u64("CPUEND"));
>
> CPU ID as value to CPUEND reg entry needed, right?
Yes, will add in v5.
>
> > +
> > <...snip...>
> > + /*
> > + * We will write the cpu state data later, as otherwise it
> > + * might get overwritten by other fadump regions
>
> Isn't the destination address of cpu state data and RMR region are
> different?
>
> I don't understand the above comment. Can you please give more details.
>
> > + */
> > +
> > <...snip...>
> > + /*
> > + * Write the Register Save Area
> > + *
> > + * CPU State/Register Save Area should be written after dumping the
> > + * memory to prevent overwriting while saving other memory regions
> > + *
> > + * eg. If boot memory region is 1G, then both the first 1GB memory, and
> > + * the Register Save Area needs to be saved at 1GB.
>
> Every region is copied to their destination address and as per below kernel
> function:
> https://github.com/torvalds/linux/blob/f406055cb18c6e299c4a783fc1effeb16be41803/arch/powerpc/platforms/pseries/rtas-fadump.c#L98
>
> the destination address shouldn't be same for fadump region then how come
> there
> is overwrite scenario? Am I missing something?
Yes, with current kernel logic, destination address of cpu state data
and RMR region are different.
I remember having faced some issue in past (don't recall, might have
been with powernv, or it might not exist anymore) hence the comment.
Ideally kernel should ensure CPU and memory regions don't overlap.
Possible overlap can be like, memory region being 0x0 - 512MB, and
generally the cpu region's destination also lies in this low memory.
PAPR says nothing about order of saving iirc. I kept current way to
ensure we have CPU registers even if such overlap happens.
What do you say ? Should we save CPU region first and then go with
saving memory regions ?
>
> > + * And as the CPU_STATE_DATA region comes first than the
> > + * REAL_MODE_REGION region to be copied, the CPU_STATE_DATA will get
> > + * overwritten if saved before the 0GB - 1GB region is copied after
> > + * saving CPU state data
> > + */
> > + io_result = address_space_write(default_as, cpu_state_addr, attrs,
> > + cpu_state_buffer, cpu_state_len);
>
> Hope cpu_state_buffer will be freed automatically...
Yes, as it's declared with g_autofree, glibc/compiler takes care of
adding cleanup functions ensuring it gets freed.
>
> > <...snip...>
> > +/* Number of registers stored per cpu */
> > +#define FADUMP_NUM_PER_CPU_REGS (32 /*GPR*/ + 45 /*others*/ + 2 /*STRT & END*/)
>
> #nit-pick
> How about FADUMP_PER_CPU_REG_ENTRY ?
How about FADUMP_PER_CPU_NUM_REGS ? Basically explicitly saying it's
number of registers per cpu ?
Thanks for your reviews Sourabh,
- Aditya G
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 6/8] hw/ppc: Pass dump-sizes property for fadump in device tree
2025-10-18 11:20 ` Sourabh Jain
@ 2025-10-19 19:30 ` Aditya Gupta
2025-10-20 5:44 ` Sourabh Jain
0 siblings, 1 reply; 36+ messages in thread
From: Aditya Gupta @ 2025-10-19 19:30 UTC (permalink / raw)
To: Sourabh Jain
Cc: qemu-devel, qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Harsh Prateek Bora, Mahesh J Salgaonkar, Hari Bathini
On 25/10/18 04:50PM, Sourabh Jain wrote:
>
>
> > <...snip...>
> > + uint32_t fadump_rgn_sizes[2][3] = {
> > + {
> > + cpu_to_be32(FADUMP_CPU_STATE_DATA),
> > + 0, 0 /* Calculated later */
> > + },
> > + {
> > + cpu_to_be32(FADUMP_HPTE_REGION),
> > + 0, 0 /* HPTE region not implemented */
> > + }
>
> #nit-pick
> Why to advertise if we don't support it? Kernel anyways ignores this for
> now.
Nice catch.
PAPR doesn't seem to say about HPTE being optional anywhere, nor being
mandatory, so to be on safe side, exported it with 0 size until/if
it's implemented.
PAPR R1–7.3.30–7 says this (trimmed, emphasis mine):
The platform 'must' present the RTAS property,
“ibm,configure-kernel-dump-sizes” in the OF device tree, which
describes space required for 'firmware defined dump sections', where
the 'firmware defined dump sections' are:
0x0001 = CPU State Data
0x0002 = Hardware Page Table for Real Mode Region
Thanks,
- Aditya G
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 1/8] hw/ppc: Implement skeleton code for fadump in PSeries
2025-10-17 11:44 ` Aditya Gupta
@ 2025-10-20 5:23 ` Sourabh Jain
0 siblings, 0 replies; 36+ messages in thread
From: Sourabh Jain @ 2025-10-20 5:23 UTC (permalink / raw)
To: Aditya Gupta
Cc: qemu-devel, qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Harsh Prateek Bora, Mahesh J Salgaonkar, Hari Bathini
On 17/10/25 17:14, Aditya Gupta wrote:
> On 25/10/17 05:08PM, Aditya Gupta wrote:
>> Hello Sourabh,
>>
>> Thanks for your detailed reviews.
>>
>> On 25/10/17 02:10PM, Sourabh Jain wrote:
>>> Hello Aditya,
>>>
>>>> <...snip...>
>>>> +/* Papr Section 7.4.9 ibm,configure-kernel-dump RTAS call */
>>>> +static void rtas_configure_kernel_dump(PowerPCCPU *cpu,
>>>> + SpaprMachineState *spapr,
>>>> + uint32_t token, uint32_t nargs,
>>>> + target_ulong args,
>>>> + uint32_t nret, target_ulong rets)
>>>> +{
>>>> + target_ulong cmd = rtas_ld(args, 0);
>>>> + uint32_t ret_val;
>>>> +
>>>> + /* Number of 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");
>>>
>>> No rtas_st for above failure?
>> Will add.
>>
>> Also I think I should remove the LOG_GUEST_ERROR, since I mostly use it
>> for qemu side errors, wrong parameters is an invalid usage rather than>
>> guest/qemu error.
>>
>> What do you say ? Should I remove qemu_log_mask here ?
>>
>>>> + return;
>>>> + }
>>>> +
>>>> + /* Number of inputs has to be 3 */
>>>> + if (nargs != 3) {
>>>> + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>>> No qemu_log_mask for the above failure?
>> Thinking to remove it, as mentioned above.
> On a second thought, I will keep the qemu_log_mask as suggested.
> More logs helps for debug if kernel passes invalid arguments to fadump.
>
> Is that okay ?
Yes, lets keep the debug logs.
- Sourabh
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 2/8] hw/ppc: Implement fadump register command
2025-10-17 11:55 ` Aditya Gupta
@ 2025-10-20 5:26 ` Sourabh Jain
0 siblings, 0 replies; 36+ messages in thread
From: Sourabh Jain @ 2025-10-20 5:26 UTC (permalink / raw)
To: Aditya Gupta
Cc: qemu-devel, qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Harsh Prateek Bora, Mahesh J Salgaonkar, Hari Bathini
On 17/10/25 17:25, Aditya Gupta wrote:
> On 25/10/17 03:24PM, Sourabh Jain wrote:
>>
>> On 23/03/25 23:10, Aditya Gupta wrote:
>>> Implement the register command of "ibm,configure-kernel-dump" RTAS call.
>>> The register just verifies the structure of the fadump memory structure
>>> passed by kernel, and set fadump_registered in spapr state to true.
>>>
>>> We also store the passed fadump memory structure, which will later be
>>> used for preserving memory for fadump boot in case of a crash.
>>>
>>> The fadump memory structure isn't modified (other than .dump_status_flag
>>> after the fadump is triggered, that is in a later patch).
>>> So if the structure needs to updated, the kernel should first
>>> de-register and re-register the structure again.
>>>
>>> Relevant section for the register command in PAPR is:
>>> Section 7.3.30: "ibm,configure-kernel-dump RTAS call" (PAPR v2.13)
>>>
>>> Note: The fadump registration is done, but triggering fadump on an
>>> os-term rtas call is done in later patches. Hence QEMU will just shutdown
>>> on a kernel crash due to no special handling for fadump in ibm,os-term
>>>
>>> Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
>>> ---
>>> hw/ppc/spapr_fadump.c | 111 ++++++++++++++++++++++++++++++++--
>>> hw/ppc/spapr_rtas.c | 2 +-
>>> include/hw/ppc/spapr_fadump.h | 2 +-
>>> 3 files changed, 108 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/hw/ppc/spapr_fadump.c b/hw/ppc/spapr_fadump.c
>>> index 20b7b804c485..9c7fb9e12b16 100644
>>> --- a/hw/ppc/spapr_fadump.c
>>> +++ b/hw/ppc/spapr_fadump.c
>>> @@ -5,18 +5,119 @@
>>> */
>>> #include "qemu/osdep.h"
>>> +#include "qemu/log.h"
>>> #include "hw/ppc/spapr.h"
>>> /*
>>> * Handle the "FADUMP_CMD_REGISTER" command in 'ibm,configure-kernel-dump'
>>> *
>>> + * Note: Any changes made by the kernel to the fadump memory struct won't
>>> + * reflect in QEMU after the 'ibm,configure-kernel-dump' RTAS call has returned,
>>> + * as we store the passed fadump memory structure passed during fadump
>> passed fadump memory structure passed... is not easy to understand; how
>> about?
>>
>> Note: Any modifications made by the kernel to the fadump memory structure
>> after the 'ibm,configure-kernel-dump' RTAS call returns will not be
>> reflected
>> in QEMU as QEMU retains the fadump memory structure that was provided
>> during fadump registration.
>>
>> The kernel must invalidate and re-register fadump to apply any changes
>> to the fadump memory structure.
> Makes sense. Will use the suggested text.
>
>>> + * registration.
>>> + * Kernel has to invalidate & re-register fadump, if it intends to make any
>>> + * changes to the fadump memory structure
>>> + *
>>> * Returns:
>>> - * * RTAS_OUT_HW_ERROR: Not implemented/Misc issue such as memory access
>>> - * failures
>>> + * * RTAS_OUT_SUCCESS: On successful registration
>>> + * * RTAS_OUT_PARAM_ERROR: If parameters are not correct, eg. too many
>>> + * sections, invalid memory addresses that we are
>>> + * unable to read, etc
>>> + * * RTAS_OUT_DUMP_ALREADY_REGISTERED: Dump already registered
>>> + * * RTAS_OUT_HW_ERROR: Misc issue such as memory access failures
>>> */
>>> -uint32_t do_fadump_register(void)
>>> +uint32_t do_fadump_register(SpaprMachineState *spapr, target_ulong args)
>>> {
>>> - /* WIP: FADUMP_CMD_REGISTER implemented in future patch */
>>> + FadumpSectionHeader header;
>>> + FadumpSection regions[FADUMP_MAX_SECTIONS] = {0};
>>> + target_ulong fdm_addr = rtas_ld(args, 1);
>>> + target_ulong fdm_size = rtas_ld(args, 2);
>>> + AddressSpace *default_as = &address_space_memory;
>>> + MemTxResult io_result;
>>> + MemTxAttrs attrs;
>>> + uint64_t next_section_addr;
>>> + uint16_t dump_num_sections;
>>> +
>>> + /* Mark the memory transaction as privileged memory access */
>>> + attrs.user = 0;
>>> + attrs.memory = 1;
>>> +
>>> + if (spapr->fadump_registered) {
>>> + /* FADump already registered */
>>> + return RTAS_OUT_DUMP_ALREADY_REGISTERED;
>>> + }
>>> +
>>> + if (spapr->fadump_dump_active == 1) {
>> fadump_dump_active is bool, so comparing with an integer is not needed.
> Will remove == 1.
>
>>> + return RTAS_OUT_DUMP_ACTIVE;
>>> + }
>>> +
>>> + if (fdm_size < sizeof(FadumpSectionHeader)) {
>>> + qemu_log_mask(LOG_GUEST_ERROR,
>>> + "FADump: Header size is invalid: %lu\n", fdm_size);
>>> + return RTAS_OUT_PARAM_ERROR;
>>> + }
>>> +
>>> + /* Ensure fdm_addr points to a valid RMR-memory/RMA-memory buffer */
>>> + if ((fdm_addr <= 0) || ((fdm_addr + fdm_size) > spapr->rma_size)) {
>>> + qemu_log_mask(LOG_GUEST_ERROR,
>>> + "FADump: Invalid fdm address: %ld\n", fdm_addr);
>>> + return RTAS_OUT_PARAM_ERROR;
>>> + }
>>> +
>>> + /* Try to read the passed fadump header */
>>> + io_result = address_space_read(default_as, fdm_addr, attrs,
>>> + &header, sizeof(header));
>>> + if (io_result != MEMTX_OK) {
>>> + qemu_log_mask(LOG_GUEST_ERROR,
>>> + "FADump: Unable to read fdm address: %ld\n", fdm_addr);
>>> +
>>> + return RTAS_OUT_HW_ERROR;
>>> + }
>>> +
>>> + /* Verify that we understand the fadump header version */
>>> + if (header.dump_format_version != cpu_to_be32(FADUMP_VERSION)) {
>>> + qemu_log_mask(LOG_GUEST_ERROR,
>>> + "FADump: Unknown fadump header version: 0x%x\n",
>>> + header.dump_format_version);
>>> + return RTAS_OUT_PARAM_ERROR;
>>> + }
>>> +
>>> + /* Reset dump status flags */
>>> + header.dump_status_flag = 0;
>>> +
>>> + dump_num_sections = be16_to_cpu(header.dump_num_sections);
>>> +
>>> + if (dump_num_sections > FADUMP_MAX_SECTIONS) {
>>> + qemu_log_mask(LOG_GUEST_ERROR,
>>> + "FADump: Too many sections: %d sections\n", dump_num_sections);
>>> + return RTAS_OUT_PARAM_ERROR;
>>> + }
>>> +
>>> + next_section_addr =
>>> + fdm_addr +
>>> + be32_to_cpu(header.offset_first_dump_section);
>>> +
>>> + for (int i = 0; i < dump_num_sections; ++i) {
>>> + /* Read the fadump section from memory */
>>> + io_result = address_space_read(default_as, next_section_addr, attrs,
>>> + ®ions[i], sizeof(regions[i]));
>>> + if (io_result != MEMTX_OK) {
>>> + qemu_log_mask(LOG_UNIMP,
>>> + "FADump: Unable to read fadump %dth section\n", i);
>>> + return RTAS_OUT_PARAM_ERROR;
>>> + }
>>> +
>>> + next_section_addr += sizeof(regions[i]);
>>> + }
>>> +
>>> + spapr->fadump_registered = true;
>>> + spapr->fadump_dump_active = false;
>> Setting fadump_dump_active is not really required. Ideally, we shouldn't
>> reach here
>> if it is set to true, or am I missing something?
> True, with current change it's not really needed. But to be on safe
> side, it's better to keep valid values hence ensuring dump_active is
> false, after register.
Ok.
>
>>> +
>>> + /* Store the registered fadump memory struct */
>>> + spapr->registered_fdm.header = header;
>>> + for (int i = 0; i < dump_num_sections; ++i) {
>>> + spapr->registered_fdm.rgn[i] = regions[i];
>>> + }
>>> - return RTAS_OUT_HW_ERROR;
>>> + return RTAS_OUT_SUCCESS;
>>> }
>>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>>> index b8bfa9c33fb5..0454938a01e9 100644
>>> --- a/hw/ppc/spapr_rtas.c
>>> +++ b/hw/ppc/spapr_rtas.c
>>> @@ -366,7 +366,7 @@ static void rtas_configure_kernel_dump(PowerPCCPU *cpu,
>>> switch (cmd) {
>>> case FADUMP_CMD_REGISTER:
>>> - ret_val = do_fadump_register();
>>> + ret_val = do_fadump_register(spapr, args);
>>> if (ret_val != RTAS_OUT_SUCCESS) {
>>> rtas_st(rets, 0, ret_val);
>>> return;
>>> diff --git a/include/hw/ppc/spapr_fadump.h b/include/hw/ppc/spapr_fadump.h
>>> index 45109fd9e137..6abbcb44f353 100644
>>> --- a/include/hw/ppc/spapr_fadump.h
>>> +++ b/include/hw/ppc/spapr_fadump.h
>>> @@ -65,5 +65,5 @@ struct FadumpMemStruct {
>>> FadumpSection rgn[FADUMP_MAX_SECTIONS];
>>> };
>>> -uint32_t do_fadump_register(void);
>>> +uint32_t do_fadump_register(struct SpaprMachineState *, target_ulong);
>> Seems like the forward declaration of struct SpaprMachineState done in 01/08
>> is
>> because of the above change. Should we do there forward declaration here
>> instead?
> Yes, it was for this.
> Merging patch 1 and 2 in v5, then this will be fine.
Yeah it make sense.
- Sourabh
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 5/8] hw/ppc: Implement saving CPU state in Fadump
2025-10-19 19:22 ` Aditya Gupta
@ 2025-10-20 5:40 ` Sourabh Jain
0 siblings, 0 replies; 36+ messages in thread
From: Sourabh Jain @ 2025-10-20 5:40 UTC (permalink / raw)
To: Aditya Gupta
Cc: qemu-devel, qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Harsh Prateek Bora, Mahesh J Salgaonkar, Hari Bathini
On 20/10/25 00:52, Aditya Gupta wrote:
> On 25/10/18 04:24PM, Sourabh Jain wrote:
>>> <...snip...>
>>> + curr_reg_entry->reg_id =
>>> + cpu_to_be64(fadump_str_to_u64("CPUSTRT"));
>>> + curr_reg_entry->reg_value = ppc_cpu->vcpu_id;
>> Aren't we suppose to store CPU ID in big endian?
>> As per PAPR the format CPUSTRT and CPUEND node is
>>
>> 8 Bytes Identifier (BE) | 4 Bytes Reserved (0x0) | 4 Bytes Logical CPU ID
>> (BE)
>>
>> It feels like storing vcpu_id as little endian may not get the
>> reg entrie for CPUSTRT in above format, isn't it?
> Will fix in v5.
>
>> Ideally we should have two struct to mange two types of Reg Entries.
>> That way we can manage reserved bytes in CPUSTRT/CPUEND node
>> easily. I understand that it will bring unnecessary complexity in
>> populate_cpu_state_data function. So how about adding a note for
>> future reference?
> Will add a note about the format.
> I also think another struct might be unnecessary complexity since we
> just need it at 2 instances (CPUSTRT & CPUEND), and can be done with
> simple bit math.
>
>>> + ++curr_reg_entry;
>>> +
>>> <...snip...>
>>> + /* End the registers for this CPU with "CPUEND" reg entry */
>>> + curr_reg_entry->reg_id =
>>> + cpu_to_be64(fadump_str_to_u64("CPUEND"));
>> CPU ID as value to CPUEND reg entry needed, right?
> Yes, will add in v5.
>
>>> +
>>> <...snip...>
>>> + /*
>>> + * We will write the cpu state data later, as otherwise it
>>> + * might get overwritten by other fadump regions
>> Isn't the destination address of cpu state data and RMR region are
>> different?
>>
>> I don't understand the above comment. Can you please give more details.
>>
>>> + */
>>> +
>>> <...snip...>
>>> + /*
>>> + * Write the Register Save Area
>>> + *
>>> + * CPU State/Register Save Area should be written after dumping the
>>> + * memory to prevent overwriting while saving other memory regions
>>> + *
>>> + * eg. If boot memory region is 1G, then both the first 1GB memory, and
>>> + * the Register Save Area needs to be saved at 1GB.
>> Every region is copied to their destination address and as per below kernel
>> function:
>> https://github.com/torvalds/linux/blob/f406055cb18c6e299c4a783fc1effeb16be41803/arch/powerpc/platforms/pseries/rtas-fadump.c#L98
>>
>> the destination address shouldn't be same for fadump region then how come
>> there
>> is overwrite scenario? Am I missing something?
> Yes, with current kernel logic, destination address of cpu state data
> and RMR region are different.
>
> I remember having faced some issue in past (don't recall, might have
> been with powernv, or it might not exist anymore) hence the comment.
>
> Ideally kernel should ensure CPU and memory regions don't overlap.
>
> Possible overlap can be like, memory region being 0x0 - 512MB, and
> generally the cpu region's destination also lies in this low memory.
>
> PAPR says nothing about order of saving iirc. I kept current way to
> ensure we have CPU registers even if such overlap happens.
>
> What do you say ? Should we save CPU region first and then go with
> saving memory regions ?
Yeah, I think we shouldn’t delay the CPU region. Even in the case of
such overlap, corruption in register values is easier to identify and fix
it in the kernel than corruption in the kernel memory dump.
>
>>> + * And as the CPU_STATE_DATA region comes first than the
>>> + * REAL_MODE_REGION region to be copied, the CPU_STATE_DATA will get
>>> + * overwritten if saved before the 0GB - 1GB region is copied after
>>> + * saving CPU state data
>>> + */
>>> + io_result = address_space_write(default_as, cpu_state_addr, attrs,
>>> + cpu_state_buffer, cpu_state_len);
>> Hope cpu_state_buffer will be freed automatically...
> Yes, as it's declared with g_autofree, glibc/compiler takes care of
> adding cleanup functions ensuring it gets freed.
That's nice.
>
>>> <...snip...>
>>> +/* Number of registers stored per cpu */
>>> +#define FADUMP_NUM_PER_CPU_REGS (32 /*GPR*/ + 45 /*others*/ + 2 /*STRT & END*/)
>> #nit-pick
>> How about FADUMP_PER_CPU_REG_ENTRY ?
> How about FADUMP_PER_CPU_NUM_REGS ? Basically explicitly saying it's
> number of registers per cpu ?
I suggested FADUMP_PER_CPU_REG_ENTRY because it’s a reg entry count
which include also includes the start and end. But it’s okay, I’m not
picky about it. Feel free to use any of the three. - Sourabh
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 6/8] hw/ppc: Pass dump-sizes property for fadump in device tree
2025-10-19 19:30 ` Aditya Gupta
@ 2025-10-20 5:44 ` Sourabh Jain
0 siblings, 0 replies; 36+ messages in thread
From: Sourabh Jain @ 2025-10-20 5:44 UTC (permalink / raw)
To: Aditya Gupta
Cc: qemu-devel, qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Harsh Prateek Bora, Mahesh J Salgaonkar, Hari Bathini
On 20/10/25 01:00, Aditya Gupta wrote:
> On 25/10/18 04:50PM, Sourabh Jain wrote:
>>
>>> <...snip...>
>>> + uint32_t fadump_rgn_sizes[2][3] = {
>>> + {
>>> + cpu_to_be32(FADUMP_CPU_STATE_DATA),
>>> + 0, 0 /* Calculated later */
>>> + },
>>> + {
>>> + cpu_to_be32(FADUMP_HPTE_REGION),
>>> + 0, 0 /* HPTE region not implemented */
>>> + }
>> #nit-pick
>> Why to advertise if we don't support it? Kernel anyways ignores this for
>> now.
> Nice catch.
> PAPR doesn't seem to say about HPTE being optional anywhere, nor being
> mandatory, so to be on safe side, exported it with 0 size until/if
> it's implemented.
>
> PAPR R1–7.3.30–7 says this (trimmed, emphasis mine):
>
> The platform 'must' present the RTAS property,
> “ibm,configure-kernel-dump-sizes” in the OF device tree, which
> describes space required for 'firmware defined dump sections', where
> the 'firmware defined dump sections' are:
> 0x0001 = CPU State Data
> 0x0002 = Hardware Page Table for Real Mode Region
Yeah lets advertise HPTE with zero size.
- Sourabh
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 7/8] hw/ppc: Enable fadump for PSeries
2025-10-18 12:04 ` Sourabh Jain
@ 2025-10-20 19:44 ` Aditya Gupta
0 siblings, 0 replies; 36+ messages in thread
From: Aditya Gupta @ 2025-10-20 19:44 UTC (permalink / raw)
To: Sourabh Jain
Cc: qemu-devel, qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
Harsh Prateek Bora, Mahesh J Salgaonkar, Hari Bathini
Hello Sourabh,
On 25/10/18 05:34PM, Sourabh Jain wrote:
>
>
> > <...snip...>
> > The fadump boot after crash:
> >
> > [ 0.000000] rtas fadump: Firmware-assisted dump is active.
> > [ 0.000000] fadump: Updated cmdline: debug fadump=on crashkernel=1G
> > [ 0.000000] fadump: Firmware-assisted dump is active.
>
> Kernel is printing twice about fadump is active. :)
Yeah i noticed, that seems to be the case atleast with 6.14
>
> > <...snip...>
> > MachineState *ms = MACHINE(spapr);
> > MachineClass *mc = MACHINE_GET_CLASS(ms);
> > + FadumpMemStruct *fdm = &spapr->registered_fdm;
> > + uint16_t dump_status_flag;
> > + bool is_next_boot_fadump;
> > uint32_t max_possible_cpus = mc->possible_cpu_arch_ids(ms)->len;
> > uint64_t fadump_cpu_state_size = 0;
> > @@ -953,6 +956,18 @@ static void spapr_dt_rtas_fadump(SpaprMachineState *spapr, void *fdt, int rtas)
> > fadump_versions, sizeof(fadump_versions))));
> > _FDT((fdt_setprop(fdt, rtas, "ibm,configure-kernel-dump-sizes",
> > fadump_rgn_sizes, sizeof(fadump_rgn_sizes))));
> > +
> > + dump_status_flag = be16_to_cpu(fdm->header.dump_status_flag);
> > + is_next_boot_fadump =
>
> Do we really need is_next_boot_fadump variable?
Agreed, not needed now, will remove it in v5.
>
> > + (dump_status_flag & FADUMP_STATUS_DUMP_TRIGGERED) != 0;
> > + if (is_next_boot_fadump) {
> > + uint64_t fdm_size =
> > + sizeof(struct FadumpSectionHeader) +
> > + (be16_to_cpu(fdm->header.dump_num_sections) *
> > + sizeof(struct FadumpSection));
> > +
> > + _FDT((fdt_setprop(fdt, rtas, "ibm,kernel-dump", fdm, fdm_size)));
>
> Is this common in QEMU to call _FDT to add prop to the FDT? Feels odd.
Yes, that's just a wrapper for error checking in QEMU, used extensively
atleast in hw/ppc
Thanks for your reviews Sourabh !
- Aditya G
>
> > + }
> > }
> > static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt)
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 0/8] Implement Firmware Assisted Dump for PSeries
2025-03-23 17:39 [PATCH v4 0/8] Implement Firmware Assisted Dump for PSeries Aditya Gupta
` (8 preceding siblings ...)
2025-04-21 6:27 ` [PATCH v4 0/8] Implement Firmware Assisted Dump for PSeries Aditya Gupta
@ 2025-10-21 5:00 ` Harsh Prateek Bora
9 siblings, 0 replies; 36+ messages in thread
From: Harsh Prateek Bora @ 2025-10-21 5:00 UTC (permalink / raw)
To: Aditya Gupta, qemu-devel, shivangu
Cc: qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza, Sourabh Jain,
Mahesh J Salgaonkar, Hari Bathini
+ Shivang - FYI
Hi Aditya,
Could you please keep Shivang in Cc when you post v5 ? TIA.
regards,
Harsh
On 3/23/25 23:09, Aditya Gupta wrote:
> Overview
> =========
>
> Implemented Firmware Assisted Dump (fadump) on PSeries machine in QEMU.
>
> Fadump is an alternative dump mechanism to kdump, in which we the firmware
> does a memory preserving boot, and the second/crashkernel is booted fresh
> like a normal system reset, instead of the crashed kernel loading the
> second/crashkernel in case of kdump.
>
> This requires implementing the "ibm,configure-kernel-dump" RTAS call in
> QEMU.
>
> While booting with fadump=on, Linux will register fadump memory regions.
>
> Some memory regions like Real Mode Memory regions, and custom memory
> regions declared by OS basically require copying the requested memory
> range to a destination
>
> While other memory regions are populated by the firmware/platform (QEMU in
> this case), such as CPU State Data and HPTE.
> We pass the sizes for these data segment to the kernel as it needs to know
> how much memory to reserve (ibm,configure-kernel-dump-sizes).
>
> Then after a crash, once Linux does a OS terminate call, we trigger fadump
> if fadump was registered.
>
> Implementing the fadump boot as:
> * pause all vcpus (will save registers later)
> * preserve memory regions specified by fadump
> * do a memory preserving reboot (using GUEST_RESET as it doesn't clear
> the memory)
>
> And then we pass a metadata (firmware memory structure) as
> "ibm,kernel-dump" in the device tree, containing all details of the
> preserved memory regions to the kernel.
>
> Refer the Patch #7/8: "hw/ppc: Enable fadump for PSeries" for logs of a
> succesfful fadump crash
>
> Note: HPTE region has not been implemented. It's not planned as of now.
>
> Testing
> =======
>
> Has been tested with following QEMU options:
>
> * firmware: x-vof and SLOF
> * tcg & kvm
> * l1 guest and l2 guest
> * with/without smp
> * cma/nocma
> * default crashkernel values (can fail with big initrd) and crashkernel=1G
>
> Git Tree for Testing
> ====================
>
> https://github.com/adi-g15-ibm/qemu/tree/fadump-pseries-v4
>
> Note: You will need a way to get the /proc/vmcore out of the VM for testing
> with crash-utility
>
> I use the following command line which sets up networking:
> "-net user,hostfwd=tcp::10022-:22 -net nic"
>
> And a rootfs with ssh support, then copy the /proc/vmcore with networking
> (can do compression using gzip before ssh, but compression might take lot
> of time if done inside the VM)
>
> Test vmcore for Testing with crash-utility
> ==========================================
>
> Can use vmlinux and vmcore available at https://github.com/adi-g15-ibm/qemu/releases/tag/test-images-fadump-pseries-v2
> Above vmcore was generated with upstream qemu with these fadump patches
> applied, and in a KVM VM
> A limitation with above vmcore is it was a single CPU VM
>
> Changelog
> =========
> v4
> + [patch #8/8]: fixed kvm testcase, add license
>
> v3:
> + [patch #3,7]: fix compile errors (#define declared in a later patch
> but used in this patch, unused var)
> + [patch #4/8]: use 'g_autofree' for cpu buffer, and replace g_malloc with
> g_try_malloc
> + [patch #5/8]: use 'g_new' instead of 'malloc', add null check for cpu
> region
> - nothing in other patches has been changed compared to v2
>
> v2:
> + rearrange code so that no unused functions get introduced in any patch
> + add functional test for pseries as suggested by nick
> + fix multiple issues pointed by harsh and nick
> + fix bug in cpu register saving where it was being stored in
> little-endian
> - removed 'is_next_boot_fadump' and used fadump header's status flag to
> store it
> + fixed multiple style issues (naming, unneeded diffs etc)
>
> Aditya Gupta (8):
> hw/ppc: Implement skeleton code for fadump in PSeries
> hw/ppc: Implement fadump register command
> hw/ppc: Trigger Fadump boot if fadump is registered
> hw/ppc: Preserve memory regions registered for fadump
> hw/ppc: Implement saving CPU state in Fadump
> hw/ppc: Pass dump-sizes property for fadump in device tree
> hw/ppc: Enable fadump for PSeries
> tests/functional: Add test for fadump in PSeries
>
> hw/ppc/meson.build | 1 +
> hw/ppc/spapr.c | 72 +++
> hw/ppc/spapr_fadump.c | 685 ++++++++++++++++++++++
> hw/ppc/spapr_rtas.c | 71 +++
> include/hw/ppc/spapr.h | 11 +-
> include/hw/ppc/spapr_fadump.h | 121 ++++
> tests/functional/meson.build | 2 +
> tests/functional/qemu_test/linuxkernel.py | 59 ++
> tests/functional/test_ppc64_fadump.py | 182 ++++++
> 9 files changed, 1203 insertions(+), 1 deletion(-)
> create mode 100644 hw/ppc/spapr_fadump.c
> create mode 100644 include/hw/ppc/spapr_fadump.h
> create mode 100755 tests/functional/test_ppc64_fadump.py
>
^ permalink raw reply [flat|nested] 36+ messages in thread
end of thread, other threads:[~2025-10-21 5:01 UTC | newest]
Thread overview: 36+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-23 17:39 [PATCH v4 0/8] Implement Firmware Assisted Dump for PSeries Aditya Gupta
2025-03-23 17:40 ` [PATCH v4 1/8] hw/ppc: Implement skeleton code for fadump in PSeries Aditya Gupta
2025-04-21 10:51 ` Harsh Prateek Bora
2025-04-22 4:48 ` Aditya Gupta
2025-10-17 8:40 ` Sourabh Jain
2025-10-17 11:38 ` Aditya Gupta
2025-10-17 11:44 ` Aditya Gupta
2025-10-20 5:23 ` Sourabh Jain
2025-10-17 11:46 ` Sourabh Jain
2025-10-17 11:56 ` Aditya Gupta
2025-10-18 11:50 ` Sourabh Jain
2025-10-19 11:30 ` Aditya Gupta
2025-03-23 17:40 ` [PATCH v4 2/8] hw/ppc: Implement fadump register command Aditya Gupta
2025-10-17 9:54 ` Sourabh Jain
2025-10-17 11:55 ` Aditya Gupta
2025-10-20 5:26 ` Sourabh Jain
2025-03-23 17:40 ` [PATCH v4 3/8] hw/ppc: Trigger Fadump boot if fadump is registered Aditya Gupta
2025-10-17 11:26 ` Sourabh Jain
2025-10-17 11:59 ` Aditya Gupta
2025-03-23 17:40 ` [PATCH v4 4/8] hw/ppc: Preserve memory regions registered for fadump Aditya Gupta
2025-10-17 13:06 ` Sourabh Jain
2025-10-17 18:13 ` Aditya Gupta
2025-03-23 17:40 ` [PATCH v4 5/8] hw/ppc: Implement saving CPU state in Fadump Aditya Gupta
2025-10-18 10:54 ` Sourabh Jain
2025-10-19 19:22 ` Aditya Gupta
2025-10-20 5:40 ` Sourabh Jain
2025-03-23 17:40 ` [PATCH v4 6/8] hw/ppc: Pass dump-sizes property for fadump in device tree Aditya Gupta
2025-10-18 11:20 ` Sourabh Jain
2025-10-19 19:30 ` Aditya Gupta
2025-10-20 5:44 ` Sourabh Jain
2025-03-23 17:40 ` [PATCH v4 7/8] hw/ppc: Enable fadump for PSeries Aditya Gupta
2025-10-18 12:04 ` Sourabh Jain
2025-10-20 19:44 ` Aditya Gupta
2025-03-23 17:40 ` [PATCH v4 8/8] tests/functional: Add test for fadump in PSeries Aditya Gupta
2025-04-21 6:27 ` [PATCH v4 0/8] Implement Firmware Assisted Dump for PSeries Aditya Gupta
2025-10-21 5:00 ` Harsh Prateek Bora
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).