[PATCH v4 1/8] hw/ppc: Implement skeleton code for fadump in PSeries

Aditya Gupta posted 8 patches 10 months, 3 weeks ago
Maintainers: Nicholas Piggin <npiggin@gmail.com>, Daniel Henrique Barboza <danielhb413@gmail.com>, Harsh Prateek Bora <harshpb@linux.ibm.com>, Thomas Huth <thuth@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, "Daniel P. Berrangé" <berrange@redhat.com>
There is a newer version of this series
[PATCH v4 1/8] hw/ppc: Implement skeleton code for fadump in PSeries
Posted by Aditya Gupta 10 months, 3 weeks ago
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


Re: [PATCH v4 1/8] hw/ppc: Implement skeleton code for fadump in PSeries
Posted by Sourabh Jain 3 months, 3 weeks ago

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


Re: [PATCH v4 1/8] hw/ppc: Implement skeleton code for fadump in PSeries
Posted by Aditya Gupta 3 months, 3 weeks ago
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
Re: [PATCH v4 1/8] hw/ppc: Implement skeleton code for fadump in PSeries
Posted by Sourabh Jain 3 months, 3 weeks ago
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 */


Re: [PATCH v4 1/8] hw/ppc: Implement skeleton code for fadump in PSeries
Posted by Aditya Gupta 3 months, 3 weeks ago
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


Re: [PATCH v4 1/8] hw/ppc: Implement skeleton code for fadump in PSeries
Posted by Sourabh Jain 3 months, 3 weeks ago

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


Re: [PATCH v4 1/8] hw/ppc: Implement skeleton code for fadump in PSeries
Posted by Aditya Gupta 3 months, 3 weeks ago
> > > > +
> > > > +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
>
Re: [PATCH v4 1/8] hw/ppc: Implement skeleton code for fadump in PSeries
Posted by Aditya Gupta 3 months, 3 weeks ago
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
Re: [PATCH v4 1/8] hw/ppc: Implement skeleton code for fadump in PSeries
Posted by Sourabh Jain 3 months, 3 weeks ago

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
Re: [PATCH v4 1/8] hw/ppc: Implement skeleton code for fadump in PSeries
Posted by Harsh Prateek Bora 9 months, 3 weeks ago

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

Re: [PATCH v4 1/8] hw/ppc: Implement skeleton code for fadump in PSeries
Posted by Aditya Gupta 9 months, 3 weeks ago
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 */