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

[Mahesh Jagannath Salgaonkar <mahesh@linux.vnet.ibm.com>]

On 8/14/19 12:36 PM, Hari Bathini wrote:
> 
> 
> 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. 

That's less likely.. so is true with opal_mpipl_update() as well.

> So, we might as well try that early and fall back to KDump in case of an error..

ok. Yeah but not uninitialized metadata.

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

Do you want to do that before memblock reservation ? Should we move this
to setup_fadump() ?

Thanks,
-Mahesh.

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