Re: [PATCH v4 2/8] hw/ppc: Implement fadump register command

[Sourabh Jain <sourabhjain@linux.ibm.com>]

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,
> +                &regions[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 */