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

[Hari Bathini]

On Friday 09 June 2017 05:34 PM, Michal Suchánek wrote:
> On Thu, 8 Jun 2017 23:30:37 +0530
> Hari Bathini <hbathini at linux.vnet.ibm.com> wrote:
>
>> 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 at 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 at 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 at 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 at 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 at 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.
>
> You can still do that relatively easily. parse_args() gives you the
> content of fadump_capture argument. You should probably add a variant
> that gives you also the position of the fadump_capture argument in case
> user specified it multiple times - picking the wrong one would cause
> errors.

There is no denying that this can be done with the use of parse_args() 
function.
But my contention is a custom parser is no more error prone, considering it
only has to search/replace commas in fadump_extra_args= till the first
occurrence of space/nul, without having to deal with the naunces of
parse_args() function. Also, to get the last occurence, could use something
like get_last_crashkernel() instead of adding a variant to parse_args().

> Then you can just replace the fadump_append="appended arguments" with
> the dequoted "appended arguments" parse_args() gives you. Dequating
> should shorten the arguments as would removing the fadump_append=
> prefix so it should be quite possible in-place. It is in fact even
> easier than the comma separated list since you do not need to do any
> parsing yourself - only strlen, memset and memcpy.
>
> That said, if you replace the fadump_append argument the running kernel
> will have extra arguments without knowing where they came from making
> detection of this happening all the more difficult.

How about replacing "fadump_extra_args=x,y,z" with "fadump_extra_args x 
y z"?
Presence of fadump_extra_args hints the user about the extra parameters 
passed
to fadump capture kernel and also lets parsing/replacing be simple - replace
the first '=' and subsequent commas with ' ' in this fadump_extra_args= 
parameter
and we are good to go. No additional handling. Even if we go with 
space-separated
quoted string, simply replacing "fadump_extra_args=" with 
"fadump_extra_args "
should suffice, no need to worry about parse_args as well. I prefer 
comma-separated
list though, for it leaves no dependency on how bootloader handles 
space-separated
quoted strings, considering my encounter with yast bootloader which is 
not so
impressive in handling quoted strings. Also, the code change is not so 
complicated.


>> 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 comma separated list still can contain a -- argument so you still
> have to do something about that. Or not and just document that people
> should not use it.

The user has the flexibility to use fadump_extra_args= to pass special 
parameters
keeping in mind that "fadump_extra_args=x,y,z" will be replaced in-place 
with
"fadump_extra_args x y z" and it happens only in fadump capture kernel. 
No additional
code changes needed to handle this except for documentation talking 
about it?

> As for the parameter name fadump_args or fadump_extra_args is more
> generic than fadump_append giving you more room for changing the
> implementation details without making the argument name non-sensical.
>
>>
>>> 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.
> Which you can do for example using kexec.
>
>> 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?
> Why? They leave that to the initscripts and since you just changed the
> semantics of the kernel parameters significantly those can produce
> entertaining results.
>
>

Yeah. That is tricky but we are talking about a capture kernel here 
which should
typically reboot after dump capture (just like kdump kernel does). So, 
the case
you are mentioning about can be considered an exception a script should 
be able
to tackle?


Thanks
Hari