Hi Harsh,
Thanks for reviewing.
On 25/04/21 04:21PM, Harsh Prateek Bora wrote:
>
>
> On 3/23/25 23:10, Aditya Gupta wrote:
> > <...snip...>
> >
> > + 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;
>
> I would suggest to keep the first patch as implementing the logic for
> FADUMP_CMD_REGISTER (and _UNREGISTER) handling.
Tried that, but that is either introducing an unused function or would
mean squashing this patch and the subsequent patch implementing register
command.
I am preferring to keep current patch split.
What do you think ?
>
> > + case FADUMP_CMD_UNREGISTER:
> > + if (spapr->fadump_dump_active == 1) {
> > + rtas_st(rets, 0, RTAS_OUT_DUMP_ACTIVE);
> > + return;
> > + }
> > +
> > + spapr->fadump_registered = false;
> > + spapr->fadump_dump_active = false;
> > + memset(&spapr->registered_fdm, 0, sizeof(spapr->registered_fdm));
> > + break;
> >
> > <...snip...>
> >
> > +/* ibm,configure-kernel-dump header. */
> > +struct FadumpSectionHeader {
> > + __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;
> > +};
>
> Also, you may introduce struct members in the patches as they are
> used/accessed. No need to have entire struct introduced in the first patch
> unless the members are being used/accessed.
Did that. The FadumpMemStruct gets used in SpaprMachineState and by
unregister command.
Agree with you about introducing fields as they are used, but kept the
complete structure with all fields to be compliant with PAPR, as having
the complete structure ensures the fields are at correct offsets as per
PAPR.
Thanks,
- Aditya G
>
> regards,
> Harsh
>
> > +
> > +/* Note: All the data in these structures is in big-endian */
> > +struct FadumpMemStruct {
> > + FadumpSectionHeader header;
> > + FadumpSection rgn[FADUMP_MAX_SECTIONS];
> > +};
> > +
> > +uint32_t do_fadump_register(void);
> > +#endif /* PPC_SPAPR_FADUMP_H */
