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

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

Hi Nick,

On 27/02/25 08:37, Nicholas Piggin wrote:
> 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?
Yes, i can move it inside SpaprMachineState. Will put the code in new 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?

Ah, that is problematic agreed. I tried to move around things, but 
arrived at this.

I will spend some time thinking how to arrange this.

Will need some guidance. How should I approach arranging the code in 
such situations ?

My idea was to
* First one is the skeleton: mentions the steps, but doesn't implement 
the steps
* Middle patches implement the steps one by one
* Last patch enables it all. So in future if someone checks out the 
"Enable fadump" commit they would have all the support ready.

The major problem is "everything" remains unused till this last patch. 
But this 1st patch gave me the chance to logically build upon this, eg. 
first implement preserving memory regions, then add the fadump_trigger 
in os-term rtas call, etc.

Any advice to approach this ?

>> +{
>> +    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, will use `spapr_rma_size`. The PAPR says fdm_addr should point 
to a valid RMR buffer, I guess that means it should be in the RMA, ie. 
`< spapr_rma_size()` ?


- Aditya G

>
> Thanks,
> Nick