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

[Sourabh Jain <sourabhjain@linux.ibm.com>]

On 17/10/25 17:08, Aditya Gupta wrote:
> Hello Sourabh,
>
> Thanks for your detailed reviews.
>
> On 25/10/17 02:10PM, Sourabh Jain wrote:
>> Hello Aditya,
>>
>>> <...snip...>
>>> +/* Papr Section 7.4.9 ibm,configure-kernel-dump RTAS call */
>>> +static void rtas_configure_kernel_dump(PowerPCCPU *cpu,
>>> +                                   SpaprMachineState *spapr,
>>> +                                   uint32_t token, uint32_t nargs,
>>> +                                   target_ulong args,
>>> +                                   uint32_t nret, target_ulong rets)
>>> +{
>>> +    target_ulong cmd = rtas_ld(args, 0);
>>> +    uint32_t ret_val;
>>> +
>>> +    /* Number of 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");
>>
>> No rtas_st for above failure?
> Will add.
>
> Also I think I should remove the LOG_GUEST_ERROR, since I mostly use it
> for qemu side errors, wrong parameters is an invalid usage rather than>
> guest/qemu error.
>
> What do you say ? Should I remove qemu_log_mask here ?

Having such log helps. Not sure qemu_log_mask with LOG_GUEST_ERROR is right
a function to use but keep the logging with whatever logging function 
you think
works best here.

>
>>> +        return;
>>> +    }
>>> +
>>> +    /* Number of inputs has to be 3 */
>>> +    if (nargs != 3) {
>>> +        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>> No qemu_log_mask for the above failure?
> Thinking to remove it, as mentioned above.

Same as above.

>
>>> +        return;
>>> +    }
>>> +
>>> +    switch (cmd) {
>>> +    case FADUMP_CMD_REGISTER:
>>> +        ret_val = do_fadump_register();
>>> +        if (ret_val != RTAS_OUT_SUCCESS) {
>>> +            rtas_st(rets, 0, ret_val);
>>> +            return;
>>> +        }
>>> +        break;
>>> +    case FADUMP_CMD_UNREGISTER:
>>> +        if (spapr->fadump_dump_active == 1) {
>>
>> fadump_dump_active is bool, so comparing with an integer is not needed.
> Nice catch. Thanks !
>
>>> +            rtas_st(rets, 0, RTAS_OUT_DUMP_ACTIVE);
>>> +            return;
>>> +        }
>>> <...snip...>
>>> +#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
>> Please move RTAS_FADUMP_MAX_BOOT_MEM_REGS to the respective patch
>> if it’s used; otherwise, remove it if it’s not needed.
> Again nice catch. It's not used anymore, will remove.
>
>>> +
>>> +typedef struct FadumpSection FadumpSection;
>>> +typedef struct FadumpSectionHeader FadumpSectionHeader;
>>> +typedef struct FadumpMemStruct FadumpMemStruct;
>>> +
>>> +struct SpaprMachineState;
>>
>> Didn't understand the reason for the above forward declaration.
> Yes, in this patch it doesn't make sense, when merging the 1st and 2nd
> patch, then we see the use. Idea is as explained below.
>
> It is because it's defined in spapr.h, and we use SpaprMachineState at
> the end of this header in declaration of 'do_fadump_register'.
>
> But spapr.h includes spapr_fadump.h, so can't include that else it will
> become a cyclic dependency.
>
> Hence forward declaration was a way to solve that cyclic dependency.
Yeah I got that while reviewing the second patch in the series.

Why are we merging 1st and 2nd patch?

- Sourabh