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

["Nicholas Piggin" <npiggin@gmail.com>]

On Mon Feb 17, 2025 at 5:17 PM AEST, 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.
>
> 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;

Can this (and other globals added in other patches) come under
SpaprMachineState?

And could most of the fadump code and structures go under new
spapr_fadump.[ch] files?

> +
> +/* Papr Section 7.4.9 ibm,configure-kernel-dump RTAS call */
> +static __attribute((unused)) void rtas_configure_kernel_dump(PowerPCCPU *cpu,
> +                                   SpaprMachineState *spapr,
> +                                   uint32_t token, uint32_t nargs,
> +                                   target_ulong args,
> +                                   uint32_t nret, target_ulong rets)

I don't know about adding a new unused function like this, is there
a way to juggle patches around to add it when it's wired up?

> +{
> +    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");
> +        return;
> +    }
> +
> +    /* Number inputs has to be 3 */
> +    if (nargs != 3) {
> +        rtas_st(rets, 0, RTAS_OUT_PARAM_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);
> +            return;
> +        }
> +
> +        if (fadump_metadata.fadump_dump_active == 1) {
> +            rtas_st(rets, 0, RTAS_OUT_DUMP_ACTIVE);
> +            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 ? */

RMR memory? There is spapr_rma_size() if that's what you need?

Thanks,
Nick