On 8/14/19 12:36 PM, Hari Bathini wrote:

On 13/08/19 4:11 PM, Mahesh J Salgaonkar wrote:

quoted

On 2019-07-16 17:03:15 Tue, Hari Bathini wrote:

quoted

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

[...]

quoted

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

quoted

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.

quoted

quoted

+
+		if (memblock_reserve(base, size)) {
 			pr_err("Failed to reserve memory\n");
-			return 0;
+			goto error_out;
 		}

[...]

quoted

-
 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