On 11/25/2011 04:32 AM, Paul Mackerras wrote:
> On Tue, Nov 15, 2011 at 08:43:43PM +0530, Mahesh J Salgaonkar wrote:
>> From: Mahesh Salgaonkar <mahesh at linux.vnet.ibm.com>
>>
>> Reserve the memory during early boot to preserve CPU state data, HPTE region
>> and RMR region data in case of kernel crash. At the time of crash, powerpc
>> firmware will store CPU state data, HPTE region data and move RMR region
>> data to the reserved memory area.
>
> What is "RMR"? I don't see anywhere that you explain this acronym.
> Is it the same as the RMA (real mode area)?
>
Yes, it is the same as the RMA. I think I will replace all RMR/RMO with RMA.
>> +config FA_DUMP
>> + bool "Firmware-assisted dump"
>
> Is this new fadump infrastructure intended to supersede the existing
> phyp dump code? Does it use the same phyp interfaces as phyp dump?
> If so, you should probably remove the phyp dump code and config option
> as the final patch in your series.
>
>> +/*
>> + * The RMR region will be saved for later dumping when kernel crashes.
>> + * Set this to RMO size.
>> + */
>> +#define RMR_START 0x0
>> +#define RMR_END (ppc64_rma_size)
>
> An explanation of "RMR" here, and what the distinction (if any)
> between RMR and RMA/RMO is, would help future readers.
>
Will change this to RMA_START and RMA_END
>> + sections = of_get_flat_dt_prop(node, "ibm,configure-kernel-dump-sizes",
>> + &size);
>> +
>> + if (!sections)
>> + return 0;
>> +
>> + num_sections = size / sizeof(struct dump_section);
>> +
>> + for (i = 0; i < num_sections; i++) {
>> + switch (sections[i].dump_section) {
>> + case FADUMP_CPU_STATE_DATA:
>> + fw_dump.cpu_state_data_size = sections[i].section_size;
>> + break;
>> + case FADUMP_HPTE_REGION:
>> + fw_dump.hpte_region_size = sections[i].section_size;
>> + break;
>
> It's generally better to use of_read_number() or of_read_ulong() to
> parse OF properties, rather than using a structure like this.
>
>> + /* divide by 20 to get 5% of value */
>> + size = memblock_end_of_DRAM();
>> + do_div(size, 20);
>
> You could just say size = memblock_end_of_DRAM() / 20 here; no need to
> use do_div, since we won't be using this code on 32-bit platforms.
>
>> + if (!fw_dump.fadump_supported) {
>> + printk(KERN_ERR "Firmware-assisted dump is not supported on"
>> + " this hardware\n");
>
> This shouldn't be KERN_ERR; it's not an error to boot a kernel with
> fadump configured in on a machine that doesn't have firmware fadump
> support. I don't think we really need any message, but if we have one
> it should be KERN_INFO at most.
>
>> +/* Look for fadump= cmdline option. */
>> +static int __init early_fadump_param(char *p)
>> +{
>> + if (!p)
>> + return 1;
>> +
>> + if (p[0] == '1')
>> + fw_dump.fadump_enabled = 1;
>> + else if (p[0] == '0')
>> + fw_dump.fadump_enabled = 0;
>
> I think it's usual to allow "on" and "off" as values for this kind of
> option. There might be a handy little helper function to parse this
> sort of thing (but if there is I don't know what it is called).
Will rework on your suggestions. Thanks for the review.
Thanks,
-Mahesh.
