Re: [PATCH v5 2/2] powerpc/fadump: update documentation about 'fadump_append=' parameter

[Hari Bathini <hbathini@linux.vnet.ibm.com>]

On Friday 12 May 2017 09:12 PM, Michal Suchánek wrote:
> On Fri, 12 May 2017 15:15:33 +0530
> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote:
>
>> On Thursday 11 May 2017 06:46 PM, Michal Suchánek wrote:
>>> On Thu, 11 May 2017 02:00:11 +0530
>>> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote:
>>>   
>>>> Hello Michal,
>>>>
>>>> On Wednesday 10 May 2017 09:31 PM, Michal Suchánek wrote:
>>>>> Hello,
>>>>>
>>>>> On Wed, 03 May 2017 23:52:52 +0530
>>>>> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote:
>>>>>      
>>>>>> With the introduction of 'fadump_append=' parameter to pass
>>>>>> additional parameters to fadump (capture) kernel, update
>>>>>> documentation about it.
>>>>>>
>>>>>> Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com>
>>>>>> ---
>>>>>>
>>>>>> Changes from v4:
>>>>>> * Based on top of patchset that includes
>>>>>>      https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=akpm&id=05f383cdfba8793240e73f9a9fbff4e25d66003f
>>>>>>
>>>>>>
>>>>>>     Documentation/powerpc/firmware-assisted-dump.txt |   10
>>>>>> +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/Documentation/powerpc/firmware-assisted-dump.txt
>>>>>> b/Documentation/powerpc/firmware-assisted-dump.txt index
>>>>>> 8394bc8..6327193 100644 ---
>>>>>> a/Documentation/powerpc/firmware-assisted-dump.txt +++
>>>>>> b/Documentation/powerpc/firmware-assisted-dump.txt @@ -162,7
>>>>>> +162,15 @@ How to enable firmware-assisted dump (fadump):
>>>>>>     1. Set config option CONFIG_FA_DUMP=y and build kernel.
>>>>>>     2. Boot into linux kernel with 'fadump=on' kernel cmdline
>>>>>> option. -3. Optionally, user can also set 'crashkernel=' kernel
>>>>>> cmdline +3. A user can pass additional command line parameters
>>>>>> as a comma
>>>>>> +   separated list through 'fadump_append=' parameter, to be
>>>>>> enforced
>>>>>> +   when fadump is active. For example, if parameters like
>>>>>> nr_cpus=1,
>>>>>> +   numa=off & udev.children-max=2 are to be enforced when fadump
>>>>>> is
>>>>>> +   active,
>>>>>> 'fadump_append=nr_cpus=1,numa=off,udev.children-max=2'
>>>>>> +   can be passed in command line, which will be replaced with
>>>>>> +   "nr_cpus=1 numa=off udev.children-max=2" when fadump is
>>>>>> active.
>>>>>> +   This helps in reducing memory consumption during dump
>>>>>> capture. +4. Optionally, user can also set 'crashkernel=' kernel
>>>>>> cmdline to specify size of the memory to reserve for boot memory
>>>>>> dump preservation.
>>>>>>     
>>>>>>      
>>>>> Writing your own deficient parser for comma separated arguments
>>>>> when perfectly fine parser for space separated quoted arguments
>>>>> exists in the kernel and the bootloader does not seem like a good
>>>>> idea to me.
>>>> Couple of things that prompted me for v5 are:
>>>>      1. Using parse_early_options() limits the kind of parameters
>>>>         that can be passed to fadump capture kernel. Passing
>>>> parameters like systemd.unit= & udev.childern.max= has no effect
>>>> with v4. Updating
>>>>         boot_command_line parameter, when fadump is active, seems a
>>>> better alternative.
>>>>
>>>>      2. Passing space-separated quoted arguments is not working
>>>>         as intended with lilo. Updating bootloader with the below
>>>>         entry in /etc/lilo.conf file results in a missing append
>>>>         entry in /etc/yaboot.conf file.
>>>>
>>>>           append = "quiet sysrq=1 insmod=sym53c8xx insmod=ipr
>>>> crashkernel=512M-:256M fadump_append=\"nr_cpus=1 numa=off
>>>> udev.children-max=2\""
>>> Meaning that a script that emulates LILO semantics on top of yaboot
>>> which is completely capable of passing qouted space separated
>>> arguments fails. IMHO it is more reasonable to fix the script or
>>> whatever adaptation layer or use yaboot directly than working
>>> around bug in said script by introducing a new argument parser in
>>> the kernel.
>>>
>>>   
>> Hmmm.. while trying to implement space-separated parameter list with
>> quotes as syntax for fadump_append parameter, noticed that it can
>> make implemenation
>> more vulnerable. Here are some problems I am facing while
>> implementing this..
> How so?
>
> presumably you can reuse parse_args even if you do not register with
> early_param and call it yourself. Then your parsing of fadump_append is


I wasn't aware of that. Thanks for pointing it out, Michal.
Will try to use parse_args and get back.

Thanks
Hari