Re: [PATCH v4 11/25] powernv/fadump: register kernel metadata address with opal

[Hari Bathini <hbathini@linux.ibm.com>]

On 13/08/19 4:11 PM, Mahesh J Salgaonkar wrote:
> On 2019-07-16 17:03:15 Tue, Hari Bathini wrote:
>> OPAL allows registering address with it in the first kernel and
>> retrieving it after MPIPL. Setup kernel metadata and register its
>> address with OPAL to use it for processing the crash dump.
>>
>> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
>> ---
>>  arch/powerpc/kernel/fadump-common.h          |    4 +
>>  arch/powerpc/kernel/fadump.c                 |   65 ++++++++++++++---------
>>  arch/powerpc/platforms/powernv/opal-fadump.c |   73 ++++++++++++++++++++++++++
>>  arch/powerpc/platforms/powernv/opal-fadump.h |   37 +++++++++++++
>>  arch/powerpc/platforms/pseries/rtas-fadump.c |   32 +++++++++--
>>  5 files changed, 177 insertions(+), 34 deletions(-)
>>  create mode 100644 arch/powerpc/platforms/powernv/opal-fadump.h
>>
> [...]
>> @@ -346,30 +349,42 @@ int __init fadump_reserve_mem(void)
>>  		 * use memblock_find_in_range() here since it doesn't allocate
>>  		 * from bottom to top.
>>  		 */
>> -		for (base = fw_dump.boot_memory_size;
>> -		     base <= (memory_boundary - size);
>> -		     base += size) {
>> +		while (base <= (memory_boundary - size)) {
>>  			if (memblock_is_region_memory(base, size) &&
>>  			    !memblock_is_region_reserved(base, size))
>>  				break;
>> +
>> +			base += size;
>>  		}
>> -		if ((base > (memory_boundary - size)) ||
>> -		    memblock_reserve(base, size)) {
>> +
>> +		if (base > (memory_boundary - size)) {
>> +			pr_err("Failed to find memory chunk for reservation\n");
>> +			goto error_out;
>> +		}
>> +		fw_dump.reserve_dump_area_start = base;
>> +
>> +		/*
>> +		 * Calculate the kernel metadata address and register it with
>> +		 * f/w if the platform supports.
>> +		 */
>> +		if (fw_dump.ops->setup_kernel_metadata(&fw_dump) < 0)
>> +			goto error_out;
> 
> I see setup_kernel_metadata() registers the metadata address with opal without
> having any minimum data initialized in it. Secondaly, why can't this wait until> registration ? I think we should defer this until fadump registration.

If setting up metadata address fails (it should ideally not fail, but..), everything else
is useless. So, we might as well try that early and fall back to KDump in case of an error..

> What if kernel crashes before metadata area is initialized ?

registered_regions would be '0'. So, it is treated as fadump is not registered case. Let me
initialize metadata explicitly before registering the address with f/w to avoid any assumption...

> 
>> +
>> +		if (memblock_reserve(base, size)) {
>>  			pr_err("Failed to reserve memory\n");
>> -			return 0;
>> +			goto error_out;
>>  		}
> [...]
>> -
>>  static struct fadump_ops rtas_fadump_ops = {
>> -	.init_fadump_mem_struct	= rtas_fadump_init_mem_struct,
>> -	.register_fadump	= rtas_fadump_register_fadump,
>> -	.unregister_fadump	= rtas_fadump_unregister_fadump,
>> -	.invalidate_fadump	= rtas_fadump_invalidate_fadump,
>> -	.process_fadump		= rtas_fadump_process_fadump,
>> -	.fadump_region_show	= rtas_fadump_region_show,
>> -	.fadump_trigger		= rtas_fadump_trigger,
>> +	.init_fadump_mem_struct		= rtas_fadump_init_mem_struct,
>> +	.get_kernel_metadata_size	= rtas_fadump_get_kernel_metadata_size,
>> +	.setup_kernel_metadata		= rtas_fadump_setup_kernel_metadata,
>> +	.register_fadump		= rtas_fadump_register_fadump,
>> +	.unregister_fadump		= rtas_fadump_unregister_fadump,
>> +	.invalidate_fadump		= rtas_fadump_invalidate_fadump,
>> +	.process_fadump			= rtas_fadump_process_fadump,
>> +	.fadump_region_show		= rtas_fadump_region_show,
>> +	.fadump_trigger			= rtas_fadump_trigger,
> 
> Can you make the tab space changes in your previous patch where these
> were initially introduced ? So that this patch can only show new members
> that are added.

done.

Thanks
Hari