On Thu, Dec 19, 2019 at 11:27 PM Eric DeVolder <eric.devolder@xxxxxxxxxx> wrote:
>
> Bhupesh,
> Thank you. For the formal patch, would you be ok with a two phase approach, first where we add in
> the dbgprintf(),
Sure Eric, I think you can send the patch with the dbgprintf() right
away. It seems a straight forward change and should be acceptable to
other reviewers I believe.
> and followed later by a consolidation of the --command-line, --append,
> --reuse-cmdline option code?
Actually, I did some work a few months ago (on the request of an arm32
kexec-tools user) on this consolidation, but I never got the time to
complete the same.
I will try to find out some time over this week to consolidate these
features and send an RFC patch. I will Cc you to the same. Hopefully
that should do the trick.
Thanks,
Bhupesh
> On 12/19/19 7:34 AM, Bhupesh Sharma wrote:
> > Hi Eric,
> >
> > On 12/19/2019 12:30 AM, Eric DeVolder wrote:
> >> Thanks Bhupesh for the feedback, responses below!
> >> eric
> >>
> >> On 12/17/19 1:59 PM, Bhupesh Sharma wrote:
> >>> Hi Eric,
> >>>
> >>> On 12/17/2019 02:02 AM, Eric DeVolder wrote:
> >>>> The --command-line, --append, and --reuse-cmdline options to kexec can
> >>>> be used in combination to craft a kernel command line for a kernel
> >>>> loaded via kexec. In addition, the kexec tool may also manipulate
> >>>> further the command line, eg. elfcorehdr addition.
> >>>
> >>> Thanks for proposing this change. I have some comments/queries (see below).
> >>>
> >>>> To aid in debugging kdump/kexec related issues, it would be helpful
> >>>> for kexec to print the final constructed kernel command line argument.
> >>>>
> >>>> For example, the following simple change (for i386/x86_64):
> >>>>
> >>>> diff --git a/kexec/arch/i386/x86-linux-setup.c b/kexec/arch/i386/x86-linux-setup.c
> >>>> index 057ee14..6dc4adc 100644
> >>>> --- a/kexec/arch/i386/x86-linux-setup.c
> >>>> +++ b/kexec/arch/i386/x86-linux-setup.c
> >>>> @@ -57,6 +57,8 @@ void setup_linux_bootloader_parameters_high(
> >>>> char *cmdline_ptr;
> >>>> unsigned long initrd_base, initrd_addr_max;
> >>>>
> >>>> + printf("Final kernel cmdline: '%s'\n", cmdline);
> >>>> +
> >>>
> >>> If we want to add this for debugging purposes, its better to have dbgprintf() instead of printf()
> >>> here. This will make sure that the debug message is printed only when '-d' flag is specified
> >>> while calling kexec utility from command-line.
> >>
> >> Yes! I used printf() merely to provide an example of what is possible.
> >
> > Ok.
> >
> >>>> /* Say I'm a boot loader */
> >>>> real_mode->loader_type = LOADER_TYPE_KEXEC << 4;
> >>>>
> >>>> results in the following on a systemd-based system (formatted to fit
> >>>> in 70 char lines):
> >>>>
> >>>> % systemctl status -l kdump.service
> >>>> ● kdump.service - Crash recovery kernel arming
> >>>> Loaded: loaded (/usr/lib/systemd/system/kdump.service; enabled;
> >>>> vendor preset: enabled)
> >>>> Active: active (exited) since Mon 2019-12-16 14:59:21 EST;
> >>>> 2min 53s ago
> >>>> Process: 14058 ExecStop=/usr/bin/kdumpctl stop (code=exited,
> >>>> status=0/SUCCESS)
> >>>> Process: 14073 ExecStart=/usr/bin/kdumpctl start (code=exited,
> >>>> status=0/SUCCESS)
> >>>> Main PID: 14073 (code=exited, status=0/SUCCESS)
> >>>>
> >>>> Dec 16 14:59:18 vm364 kdumpctl[14058]: Stopping kdump: [OK]
> >>>> Dec 16 14:59:18 vm364 systemd[1]: Stopped Crash recovery kernel arming.
> >>>> Dec 16 14:59:18 vm364 systemd[1]: Starting Crash recovery kernel arming...
> >>>> Dec 16 14:59:21 vm364 kdumpctl[14073]: Final kernel cmdline: 'BOOT_IMAGE=
> >>>> /vmlinuz-4.14.35-1902.7.3.1.el7uek.x86_64 ro rhgb quiet LANG=en_US.UTF-8
> >>>> irqpoll nr_cpus=1 reset_devices cgroup_disable=memory mce=off numa=off
> >>>> udev.children-max=2 panic=10 rootflags=nofail acpi_no_memhotplug
> >>>> transparent_hugepage=never nokaslr novmcoredd disable_cpu_apicid=0
> >>>> elfcorehdr=901492K'
> >>>> Dec 16 14:59:21 vm364 systemd[1]: Started Crash recovery kernel arming.
> >>>> Dec 16 14:59:21 vm364 kdumpctl[14073]: kexec: loaded kdump kernel
> >>>> Dec 16 14:59:21 vm364 kdumpctl[14073]: Starting kdump: [OK]
> >>>>
> >>>> and the output is also available in /var/log/messages.
> >>>
> >>> Since kdumpctl is a distribution specific script (Used by Fedora/RHEL), which invokes 'kexec'
> >>> under the hood, we can discuss the features supported by 'kexec' rather than the distribution
> >>> specific scripts (discussion regarding which are probably more suited to the Fedora kexec list:
> >>> kexec@xxxxxxxxxxxxxxxxxxxxxxx)
> >>
> >> Agreed, this RFC is for a change to kexec, noting that wrapper scripts such as kdumpctl are
> >> insufficient to provide the functionality requested.
> >>
> >>>
> >>>> There might also be an opportunity to consolidate handling of the
> >>>> kernel command line, as most arch targets have the --command-line,
> >>>> --append, and --reuse-cmdline options, though each arch independently
> >>>> codes the support for these options.
> >>>
> >>> This seems like a good idea, more on the same below ...
> >>>
> >>>> Note: Simply printing the cmdline in scripts such as kdumpctl may not
> >>>> result in the same ordering, and will omit any addition made internally
> >>>> by kexec, such as the elfcorehdr.
> >>>>
> >>>> I propose the addition of an option to kexec, --print-kcl (to mirror
> >>>> --print-ckr), that would control such printing, as well as the needed
> >>>> per arch conditional print statements similar to the above to print the
> >>>> final constructed kernel command line.
> >>>
> >>> ... I am not sure I understand the above point. Looking at the latest kexec-tools man page (see:
> >>> git://git.kernel.org/pub/scm/utils/kernel/kexec/kexec-tools.git), I couldn't find '--print-ckr'
> >>> option:
> >>>
> >>> --print-ckr-size: Print crash kernel region size, if available.
> >>>
> >>> Can you please elaborate on '--print-ckr' and '--print-kcl' options more.
> >>
> >> You proposed using dbgprintf() in conjunction with the -d option; and that makes great sense; I
> >> had not in my eagerness to produce this RFC.
> >
> > Ok, no problem.
> >
> >> Instead, I proposed another option --print-kcl (for print kernel command line) to conditionally
> >> print the information. I was using --print-ckr as an example of similar option used to print
> >> information (in this case, the crash kernel region). Other than a similar naming convention, there
> >> is no correlation between --print-ckr and --print-kcl, and indeed even --print-kcl might be
> >> un-necessary given dbgprintf().
> >
> > Sure, makes sense.
> > I think you can go ahead and post a formal patch with 'dbgprintf()' being used. Or if you want, I
> > can try and propose a similar patch for upstream kexec-tools.
> >
> > Please let me know.
> >
> > Thanks,
> > Bhupesh
> >
>
_______________________________________________
kexec mailing list
kexec@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/kexec
