Re: [PATCH 1/6] hw/ppc: Implement skeleton code for fadump in PSeries

[Harsh Prateek Bora <harshpb@linux.ibm.com>]

On 2/17/25 12:47, Aditya Gupta wrote:
> Implement the handler for "ibm,configure-kernel-dump" rtas call in QEMU.
> 
> Currently the handler just does basic checks and handles
> register/unregister/invalidate requests from kernel.
> 
> Fadump will be enabled in a later patch.

Let's use FADump or fadump for consistency.

> 
> Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
> ---
>   hw/ppc/spapr_rtas.c    | 99 ++++++++++++++++++++++++++++++++++++++++++
>   include/hw/ppc/spapr.h | 59 +++++++++++++++++++++++++
>   2 files changed, 158 insertions(+)
> 
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index df2e837632aa..eebdf13b1552 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -341,6 +341,105 @@ static void rtas_ibm_set_system_parameter(PowerPCCPU *cpu,
>       rtas_st(rets, 0, ret);
>   }
>   
> +struct fadump_metadata fadump_metadata;
> +
> +/* Papr Section 7.4.9 ibm,configure-kernel-dump RTAS call */
> +static __attribute((unused)) void rtas_configure_kernel_dump(PowerPCCPU *cpu,

This __attribute shall be avoided if the function can be introduced when 
actually get used.

> +                                   SpaprMachineState *spapr,
> +                                   uint32_t token, uint32_t nargs,
> +                                   target_ulong args,
> +                                   uint32_t nret, target_ulong rets)
> +{
> +    struct rtas_fadump_section_header header;
> +    target_ulong cmd = rtas_ld(args, 0);
> +    target_ulong fdm_addr = rtas_ld(args, 1);
> +    target_ulong fdm_size = rtas_ld(args, 2);
> +
> +    /* Number outputs has to be 1 */
> +    if (nret != 1) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                "FADUMP: ibm,configure-kernel-dump RTAS called with nret != 1.\n");

Some of the error cases are using hcall_dprintf below. Let's use same
for consistency. Also, shouldn't this case also return 
RTAS_OUT_PARAM_ERROR ?

> +        return;
> +    }
> +
> +    /* Number inputs has to be 3 */
> +    if (nargs != 3) {
> +        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);

Log error ?

> +        return;
> +    }
> +
> +    switch (cmd) {
> +    case FADUMP_CMD_REGISTER:
> +        if (fadump_metadata.fadump_registered) {
> +            /* Fadump already registered */
> +            rtas_st(rets, 0, RTAS_OUT_DUMP_ALREADY_REGISTERED);

Log error ?

> +            return;
> +        }
> +
> +        if (fadump_metadata.fadump_dump_active == 1) {
> +            rtas_st(rets, 0, RTAS_OUT_DUMP_ACTIVE);

Log error?

> +            return;
> +        }
> +
> +        if (fdm_size < sizeof(struct rtas_fadump_section_header)) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                "FADUMP: Header size is invalid: %lu\n", fdm_size);
> +            rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> +            return;
> +        }
> +
> +        /* XXX: Can we ensure fdm_addr points to a valid RMR-memory buffer ? */
> +        if (fdm_addr <= 0) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                "FADUMP: Invalid fdm address: %ld\n", fdm_addr);
> +            rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> +            return;
> +        }
> +
> +        /* Verify that we understand the fadump header version */
> +        cpu_physical_memory_read(fdm_addr, &header, sizeof(header));
> +        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);
> +            rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> +            return;
> +        }
> +
> +        fadump_metadata.fadump_registered = true;
> +        fadump_metadata.fadump_dump_active = false;
> +        fadump_metadata.fdm_addr = fdm_addr;
> +        break;
> +    case FADUMP_CMD_UNREGISTER:
> +        if (fadump_metadata.fadump_dump_active == 1) {
> +            rtas_st(rets, 0, RTAS_OUT_DUMP_ACTIVE);

Log error?

> +            return;
> +        }
> +
> +        fadump_metadata.fadump_registered = false;
> +        fadump_metadata.fadump_dump_active = false;
> +        fadump_metadata.fdm_addr = -1;
> +        break;
> +    case FADUMP_CMD_INVALIDATE:
> +        if (fadump_metadata.fadump_dump_active) {
> +            fadump_metadata.fadump_registered = false;
> +            fadump_metadata.fadump_dump_active = false;
> +            fadump_metadata.fdm_addr = -1;
> +            memset(&fadump_metadata.registered_fdm, 0,
> +                    sizeof(fadump_metadata.registered_fdm));
> +        } else {
> +            hcall_dprintf("fadump: Nothing to invalidate, no dump active.\n");

Isnt this an error case? Should it return status as error or success ?

> +        }
> +        break;
> +    default:
> +        hcall_dprintf("Unknown RTAS token 0x%x\n", token);
> +        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,
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index a6c0547e313d..efa2f891a8a7 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -704,6 +704,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
>   
> @@ -769,6 +771,63 @@ void push_sregs_to_kvm_pr(SpaprMachineState *spapr);
>   
>   #define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x2D)
>   
> +/* 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
> +
> +/* Kernel Dump section info */
> +struct rtas_fadump_section {
> +    __be32    request_flag;
> +    __be16    source_data_type;
> +    __be16    error_flags;
> +    __be64    source_address;
> +    __be64    source_len;
> +    __be64    bytes_dumped;
> +    __be64    destination_address;
> +};

Please refer docs/devel/style.rst for Naming style. CamelCase for structs.

> +
> +/* ibm,configure-kernel-dump header. */
> +struct rtas_fadump_section_header {
> +    __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;
> +};
> +
> +struct rtas_fadump_mem_struct {
> +    struct rtas_fadump_section_header header;
> +    struct rtas_fadump_section        rgn[FADUMP_MAX_SECTIONS];
> +};
> +
> +struct fadump_metadata {
> +    bool fadump_registered;
> +    bool fadump_dump_active;
> +    target_ulong fdm_addr;
> +    struct rtas_fadump_mem_struct registered_fdm;
> +};
> +extern struct fadump_metadata fadump_metadata;
> +
>   /* RTAS ibm,get-system-parameter token values */
>   #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS      20
>   #define RTAS_SYSPARM_DIAGNOSTICS_RUN_MODE        42