Re: [RFC PATCH 2/3] powerpc/fadump: pass additional parameters to dump capture kernel

Hi Sourabh,

On 15/12/23 2:12 pm, Sourabh Jain wrote:

Hello Hari,

On 06/12/23 01:48, Hari Bathini wrote:

quoted

For fadump case, passing additional parameters to dump capture kernel
helps in minimizing the memory footprint for it and also provides the
flexibility to disable components/modules, like hugepages, that are
hindering the boot process of the special dump capture environment.

Set up a dedicated parameter area to be passed to the capture kernel.
This area type is defined as RTAS_FADUMP_PARAM_AREA. Sysfs attribute
'/sys/kernel/fadump/bootargs_append' is exported to the userspace to
specify the additional parameters to be passed to the capture kernel

Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
---
  arch/powerpc/include/asm/fadump-internal.h   |  3 +
  arch/powerpc/kernel/fadump.c                 | 80 ++++++++++++++++++++
  arch/powerpc/platforms/powernv/opal-fadump.c |  6 +-
  arch/powerpc/platforms/pseries/rtas-fadump.c | 35 ++++++++-
  arch/powerpc/platforms/pseries/rtas-fadump.h | 11 ++-
  5 files changed, 126 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/include/asm/fadump-internal.h 

b/arch/powerpc/include/asm/fadump-internal.h
index b3956c400519..81629226b15f 100644

--- a/arch/powerpc/include/asm/fadump-internal.h
+++ b/arch/powerpc/include/asm/fadump-internal.h

@@ -97,6 +97,8 @@ struct fw_dump {

      unsigned long    cpu_notes_buf_vaddr;
      unsigned long    cpu_notes_buf_size;
+    unsigned long    param_area;
+
      /*
       * Maximum size supported by firmware to copy from source to
       * destination address per entry.

@@ -111,6 +113,7 @@ struct fw_dump {

      unsigned long    dump_active:1;
      unsigned long    dump_registered:1;
      unsigned long    nocma:1;
+    unsigned long    param_area_supported:1;
      struct fadump_ops    *ops;
  };

diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
index 757681658dda..98f089747ac9 100644
--- a/arch/powerpc/kernel/fadump.c
+++ b/arch/powerpc/kernel/fadump.c

@@ -1470,6 +1470,7 @@ static ssize_t mem_reserved_show(struct kobject 

*kobj,
      return sprintf(buf, "%ld\n", fw_dump.reserve_dump_area_size);
  }
+
  static ssize_t registered_show(struct kobject *kobj,
                     struct kobj_attribute *attr,
                     char *buf)

@@ -1477,6 +1478,43 @@ static ssize_t registered_show(struct kobject 

*kobj,
      return sprintf(buf, "%d\n", fw_dump.dump_registered);
  }
+static ssize_t bootargs_append_show(struct kobject *kobj,
+                   struct kobj_attribute *attr,
+                   char *buf)
+{
+    return sprintf(buf, "%s\n", (char *)__va(fw_dump.param_area));
+}
+
+static ssize_t bootargs_append_store(struct kobject *kobj,
+                   struct kobj_attribute *attr,
+                   const char *buf, size_t count)
+{
+    char *params;
+
+    if (!fw_dump.fadump_enabled || fw_dump.dump_active)
+        return -EPERM;
+
+    if (count >= COMMAND_LINE_SIZE)
+        return -EINVAL;
+
+    /*
+     * Fail here instead of handling this scenario with
+     * some silly workaround in capture kernel.
+     */
+    if (saved_command_line_len + count >= COMMAND_LINE_SIZE) {
+        pr_err("Appending parameters exceeds cmdline size!\n");
+        return -ENOSPC;
+    }
+
+    params = __va(fw_dump.param_area);
+    strscpy_pad(params, buf, COMMAND_LINE_SIZE);
+    /* Remove newline character at the end. */
+    if (params[count-1] == '\n')
+        params[count-1] = '\0';
+
+    return count;
+}
+
  static ssize_t registered_store(struct kobject *kobj,
                  struct kobj_attribute *attr,
                  const char *buf, size_t count)

@@ -1535,6 +1573,7 @@ static struct kobj_attribute release_attr = 

__ATTR_WO(release_mem);
  static struct kobj_attribute enable_attr = __ATTR_RO(enabled);
  static struct kobj_attribute register_attr = __ATTR_RW(registered);
  static struct kobj_attribute mem_reserved_attr = 
__ATTR_RO(mem_reserved);
+static struct kobj_attribute bootargs_append_attr = 
__ATTR_RW(bootargs_append);
  static struct attribute *fadump_attrs[] = {
      &enable_attr.attr,

@@ -1611,6 +1650,46 @@ static void __init fadump_init_files(void)

      return;
  }
+/*
+ * Reserve memory to store additional parameters to be passed
+ * for fadump/capture kernel.
+ */
+static void fadump_setup_param_area(void)
+{
+    phys_addr_t range_start, range_end;
+
+    if (!fw_dump.param_area_supported || fw_dump.dump_active)
+        return;
+
+    /* This memory can't be used by PFW or bootloader as it is shared 
across kernels */
+    if (radix_enabled()) {
+        /*
+         * Anywhere in the upper half should be good enough as all 
memory
+         * is accessible in real mode.
+         */
+        range_start = memblock_end_of_DRAM() / 2;
+        range_end = memblock_end_of_DRAM();
+    } else {
+        /*
+         * Passing additional parameters is supported for hash MMU only
+         * if the first memory block size is 768MB or higher.
+         */
+        if (ppc64_rma_size < 0x30000000)
+            return;
+
+        /* 640 MB to 768 MB is not used by bootloader */
+        range_start = 0x28000000;
+        range_end = range_start + 0x4000000;
+    }
+
+    fw_dump.param_area = memblock_phys_alloc_range(COMMAND_LINE_SIZE,
+                               COMMAND_LINE_SIZE,
+                               range_start,
+                               range_end);

Should we initialize the `param_area` to avoid garbage values, or retrieve
command-line arguments from the previous boot?

I observed that cat /sys/kernel/fadump/bootargs_append prints the same
value which I set before reboot. Is this expected?

I implemented it as such to reuse the arguments set in the
previous boot. Not likely to see garbage there. Even if we
do, unlikely that garbage is going to have a negative impact
on capture kernel boot (even if we don't reset the bootargs
in the userspace before crash)..

Thanks
Hari