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

[Hari Bathini]

Hi Sourabh,

On 15/12/23 2:12 pm, Sourabh Jain wrote:
> Hello Hari,
> 
> On 06/12/23 01:48, Hari Bathini wrote:
>> 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 at 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