Re: [for-next][PATCH 04/26] bootconfig: Add Extra Boot Config support

[Borislav Petkov <bp@alien8.de>]

On Tue, Feb 11, 2020 at 11:02:07AM +0900, Masami Hiramatsu wrote:
> Sorry, you might misunderstand that the bootconfig replaces legacy command
> line. No, the legacy command line is still there and at least the part of
> suppremental command line parts of bootconfig are merged into the command
> line and shown in dmesg. So even if the user use bootconfig, their kernel
> command line was just updated, they can grub it and simply paste on other
> machine (if it accepts longer command line)

Ok, I did just play with the thing. I used this boot config for my guest:

---
kernel {
	root=/dev/sda1
	resume=/dev/sda2
	debug
	ignore_loglevel
	log_buf_len=16M
	earlyprintk=ttyS0,115200
	console=ttyS0,115200
	console=tty0
	no_console_suspend
}

init {
	net.ifnames=0
}
---

and added it to an initrd which I created especially for this. Which
brings me to the next issue I see: AFAIU, bootconfig requires people
to have initrd. And the majority of deployments do have initrd enabled
but there are use cases where people don't need it. So if bootconfig is
going to be default Y, it better support that case.

For example and for a similar reason I support builtin microcode in the
microcode loader - when you don't want to enable initrd.

Then, I did add the above bootconfig to the initrd:

./tools/bootconfig/bootconfig -a bootconfig /tmp/initrd.img
Apply bootconfig to /tmp/initrd.img
        Number of nodes: 21
        Size: 194 bytes
        Checksum: 16268

and booted my guest.

The thing I noticed is that the guest doesn't show any output in its
window anymore but only shows the login window when it is done booting.

And that is probably because of the console= thing. /proc/bootconfig has

$ cat /proc/bootconfig
kernel.root = "/dev/sda1"
kernel.resume = "/dev/sda2"
kernel.debug = ""
kernel.ignore_loglevel = ""
kernel.log_buf_len = "16M"
kernel.earlyprintk = "ttyS0", "115200"
kernel.console = "ttyS0", "115200", "tty0"
kernel.no_console_suspend = ""
init.net.ifnames = "0"

and bootconfig parsing has merged the console= parameters. And the same
thing happens if I boot the guest with "console=ttyS0,115200,tty0" on
the normal cmdline. But

console=ttyS0,115200 console=tty0

is not the same as

console=ttyS0,115200,tty0

Former has been this way since forever if you wanna have serial output
to ttyS* *and* tty output at the same time.

Also, I wouldn't want to rule out any other changes in behavior in
other command line options due to this change in parsing and merging of
options.

So again, I'm still not convinced bootconfig should be used everywhere
and by default.

In addition to the initrd requirement, it needs a special tool to glue
it to the end of the initrd. Yeah yeah, it is part of the kernel and the
glueing can be made part of the initrd creation tool but still, it is
one more and new step.

Now, if bootconfig can *supplement* the normal command line so that you
can add more complex configuration directives with it, in *addition* to
the normal command line, then sure, by all means.

And it seems that it does that: I did

---
kernel {
        initcall_debug
}

init {
}
---

and added it to the cmdline:

[    0.047358] Kernel command line: initcall_debug root=/dev/sda1 resume=/dev/sda2 debug ignore_loglevel log_buf_len=16M earlyprintk=ttyS0,115200 console=ttyS0,115200 console=tty0 no_console_suspend net.ifnames=0  bootconfig

and you can see "initcall_debug" there and it did enable the initcall
debugging output so yeah, that seems to work.

Btw, if you remove the boot config with the tool (-d) and still boot
with "bootconfig" it says:

[    0.043958] bootconfig size -23483140 greater than max size 32767

so you need to check presence of bootconfig blob and limits in the
parsing code too or so.

In any case, this is only my opinion, of course, and I might very well
be missing something.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette