On Thu, 8 Jun 2017 23:30:37 +0530 Hari Bathini <hbath...@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 <hbath...@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 <hbath...@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 <hbath...@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 <hbath...@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 <hbath...@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. 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. > > 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. 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. Thanks Michal
