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

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

Hi Michal,

Sorry for taking this long to respond. I was working on a few other things.

On Monday 15 May 2017 02:59 PM, Michal Suchánek wrote:
> Hello,
>
> On Mon, 15 May 2017 12:59:46 +0530
> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote:
>
>> 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.
>>
> I was thinking a bit more about the uses of the commandline and how
> fadump_append potentially breaks it.
>
> The first thing that should be addressed and is the special -- argument
> which denotes the start of init arguments that are not to be parsed by
> the kernel. Incidentally the initial implementation using early_param
> happened to handles that without issue. parse_args surely handles that
> so adding a hook somewhere should give you location of that argument
> (if any). And interesting thing that can happen is passing an -- inside
> the fadump_append argument. It should be handled (or not) in some way
> or other and the handling documented.


The intention with this patch is to replace

   "root=/dev/sda2 ro fadump_append=nr_cpus=1,numa=off crashkernel=1024M"

with

   "root=/dev/sda2 ro nr_cpus=1 numa=off crashkernel=1024M"

when kernel is booting for dump capture.
While parse_args() is making parsing relatively easy, the main idea is
to replace parameters as above when fadump capture kernel is booting.
The code is relatively complicated to replace parameters using 
space-separated
quoted string even though parse_args() may parse them easily. 
Comma-separated
list was easier to implement and seemed less error prone for replacing 
parameters.

Want to replace fadump_append=, to avoid command line overflow issues and
also to not have to deal with special cases like --. Actually, 
fadump_append=
seems to be confusing in that sense. How about 
fadump_args=comma-separated-list-
of-params-for-capture-kernel ? Please share your thoughts.


> The second thing that breaks is reusing content of /proc/cmdline in
> kexec calls for passing arguments to the loaded kernel. It works
> flawlessly passing the same arguments the currently running kernel was
> started with unless fadump_append argument handler appends content of
> the fadump_append argument to actual commandline that appears there. So
> this would be an argument against modifying the commandline. You could
> argue using fadump and kexec together is an exotic use case but I would
> expect that the very machines that require fadump_append to reduce dump
> memory requirement benefit from using kexec for reboots because the BIOS
> probing the hardware takes quite a while as well. If rewriting the
> commandline is desired then this side effect of recursively adding the
> content of fadump_append on kexecs which reuse /proc/cmdline should be
> documented.
>
>

fadump capture kernel (the kernel where we expand "fadump_append=x,y,z"
to "x y z") only advised to be used for saving dump (just like kdump 
kernel).
We are expected to boot into production kernel immediately after saving 
dump.
Only in special cases where a user configures to stay in fadump capture 
kernel
will he encounter the above problem. And someone choosing such scenario 
is most
likely aware of what to make of /proc/cmdline?

Thanks
Hari