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

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

On 27/02/25 14:18, Nicholas Piggin wrote:
> 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:
>>> <...snip...>
>> 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.

Thanks for the ideas Nick. I guess the first one makes sense if we want 
to not need the unused functions.

Only thing I want to check there is, since the 
"ibm,configure-kernel-dump" rtas call is registered, kernel will think 
fadump is supported, and might try registering fadump (if "fadump=on" 
passed in kernel), will see what the kernel does on a failure to 
register fadump in earlyboot.

It generally falls back to kdump, will check.


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

Sure, thanks !


- Aditya Gupta

>
> Thanks,
> Nick
>