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