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

[Aditya Gupta <adityag@linux.ibm.com>]

Hi Harsh,

Thanks for your reviews.


On 04/03/25 14:31, Harsh Prateek Bora wrote:
>
>
> 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.
>
Sure, will use FADump when starting the line, else fadump ?
>>
>> 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.
Will do it that way in v2, without introducing this unused attribute.
>
>> + 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 ?

Sure, will use qemu_log_mask then.

Thanks for the catch, yes I should have returned PARAM_ERROR, missed it. 
Will do it.

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

Not sure. PAPR doesn't specify it any error for this situation. With 
this current code, software can do invalidate anytime without needing to 
verify if dump is active or not (shouldn't happen though), but final 
state should always be that there won't be any dump active and fadump 
registered is reset.

Or should I return a HARDWARE_ERROR or PARAMETER_ERROR for this (don't 
think either is helpful) ?

>
>> +        }
>> +        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.

Sure, thanks, will follow it.


Thanks,

- Aditya Gupta

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