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

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

On Thu Feb 27, 2025 at 4:49 PM AEST, Aditya Gupta wrote:
> 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 ?

Yeah, sometimes it's difficult to avoid. Especially with a new
feature like this. If you can't find a better way, that's okay.

One thing could be to return errors from calls. RTAS is a little
bit tricky since there is no general "unsupported" error because
the presence of the token implies some support. You could return
-1 hardware error perhaps.

Another option is implement the call but not all functionality.
E.g., permit dump register/unregister, but don't actually provide
a valid dump on reboot (you could ignore, or provide empty or
invalid format). Downside of that is that if you bisect, a kernel
test case could go bad because it appears to be supported but
produces invalid result.

To avoid that, perhaps you could trip an assert or just log an
error message when performing a reboot with crash dump registered.

But as I said, don't make it too convoluted or lots more work if
it's not easy to rework.

>
>>> +{
>>> +    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()` ?

Ah yes, PAPR glossray says:

Real Mode Region. This is an obsolete term that is deprecated in favor of RMA.

So that should do what you want.

Thanks,
Nick