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
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 */
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 */
>
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
© 2016 - 2026 Red Hat, Inc.