-
Notifications
You must be signed in to change notification settings - Fork 125
FreeBSD guest: port init.c, make libkrun adjustments for processing kernel cmdline #480
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Marked as draft until I squash the commits and make them prettier. |
|
|
||
| krun_root = getenv("KRUN_BLOCK_ROOT_DEVICE"); | ||
| #if __linux__ | ||
| krun_root = clone_str(getenv("KRUN_BLOCK_ROOT_DEVICE")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixing a potential issue here:
The string pointed to by the return value of getenv() may be statically allocated, and can be modified by a subsequent call to getenv(), putenv(3), setenv(3), or unsetenv(3).
e4dbd22 to
0cd9308
Compare
| // KRUN_INIT_ARGVXX="<base64>" must have at most 128 bytes (KENV_MVALLEN on FreeBSD) | ||
| for (i, args_part) in args_raw.chunks(78).enumerate() { | ||
| env += &format!(" KRUN_INIT_ARGV{}={}", i, BASE64_STANDARD.encode(args_part)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are needed to process command-line in a FreeBSD VM but I've been thinking they might be useful in general. The current method of passing arguments (concat everything into one string) isn't exactly lossless.
|
@nohajc do you need this one to be present in the next release? Since it changes the way in which the kernel command line is generated, I'd like to give it more time to mature. |
|
It shouldn't change anything for Linux guests. Apart from the extra environment variables which are currently unused by that version of init. It would be nice to include it in the release but of course I don't want to cause any trouble. |
|
@slp Maybe my original description was a bit misleading. I only use the base64 mechanism for BSD. The old plain-text encoding is still applied to Linux. |
|
@nohajc OK, I'll start reviewing it now then. |
jakecorrenti
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code seems fine to me, but I also know nothing about BSD systems
|
@nohajc I would like to deprecate passing arguments to the entry point using the kernel command line because, as you noticed, it breaks easily. On Linux we favor putting the arguments in The parser is very flexible. You basically just need to put |
|
Sounds good, except (Free)BSD still doesn't properly support virtiofs. It has 9p support but that's missing in libkrun. What I'm doing to pass any files into a VM (short of regenerating the entire root image) is to generate ad hoc iso images using bsdtar and attaching them as secondary drives. Works for my needs but I'm not sure I'd like it for passing arguments. |
Ah, yes, lack of virtio-fs support is a PITA. Okay, let's rely on the kernel command line until there's a better alternative in BSDs. |
|
Anyway, this should solve #454. Along with the console fixes which are already merged. |
|
I was thinking about maintenance once this is merged. To be clear, I don't expect feature parity between Linux and BSD. Sometimes it's not even possible. What I would propose is to have a test build so we make sure we don't break the port. Other than that, any time a feature is added which is Linux-specific, I'd just disable it in the BSD version. One example is the various mount calls which are currently only built on Linux. FreeBSD mounts the bare minimum before executing init, so I didn't have to deal with that. Otherwise porting would mean abstracting the differences between mount APIs which are a bit different. |
| #include <sys/resource.h> | ||
| #include <sys/socket.h> | ||
| #include <sys/stat.h> | ||
| #if __FreeBSD__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, why are all of them just if and not ifdef?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No particular reason. Both should work. This way the variable must evaluate to true not just "be defined".
Makefile: add optional BSD build target Signed-off-by: Jan Noha <[email protected]>
…ariables) which is needed when booting FreeBSD Signed-off-by: Jan Noha <[email protected]>
| #if __FreeBSD__ | ||
| int i = 1; | ||
| static char argv_flat[512]; | ||
| int argv_flat_len = get_krun_init_argv_flat(argv_flat, sizeof(argv_flat)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given than most of the VMM context works fine with both Linux and FreeBSD without any changes, it's kind of a bummer having to make a difference for argument passing. Please consider implementing this unconditionally, so it's used on Linux too.
Most use cases should be using with /.krun_config.json, but for those that aren't, I think it's better to have a single path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it would be better to unify this. At the same time, I didn't want to do any disruptive changes for Linux, especially before release.
So, would it be OK to merge it like this (as phase 1) and then unify in a separate PR?
Or do you prefer to do it properly in one go? I can also live with this not being included in 1.17.
|
|
||
| #if __FreeBSD__ | ||
| int i = 1; | ||
| static char argv_flat[512]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you go for the unified solution for both Linux and FreeBSD, please make this a value that can be "reallocatable" so we aren't limited to 512 bytes.
slp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to the other comments, please limit the commit title in the second commit to 50 characters.
| #[allow(unused_mut)] | ||
| let mut kernel_cmdline = Cmdline::new(arch::CMDLINE_MAX_SIZE); | ||
| if let Some(cmdline) = payload_config.kernel_cmdline { | ||
| if cmdline.starts_with("FreeBSD:") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As stated above, I'd prefer to avoid this, having the same behavior for both Linux and FreeBSD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Especially this if is kind of ugly.
|
Does FreeBSD support multiport virtio console? This also wouldn't put any limit on the maximum length of the arguments. |
|
I'm not really sure what the extent of virtio console support is. There is a kernel module but as @slp already found out, for example, it is not possible to use virtio console as the primary output for the VM (it won't boot). |
This is a pretty cool idea, indeed.
There's multiport support and, while can't be used as kernel console, should work great for this kind of thing. I can help you prototype something on |
|
@slp I use this with libkrun: https://github.com/nohajc/freebsd/releases/download/alfs%2F15.0/kernel.txz If you also need a working VM image, I can provide that too. |
|
@mtjhrc Love the idea. If it works reasonably well, we can ditch all the hacky b64 encoding. I didn't like it in the first place. Just needed to pass the arguments somehow. |
|
@slp Ok, actually, for the prototyping, you could use my launcher which is able to inject a newly built Edit Now, every time you replace |
For some reason, this kernel hangs on boot for me when there's a virtio-console configured. Using a generic kernel from a FreeBSD VM image works fine though. As a PoC, I've extended Paired with this on the guest: This works fine here. It should be a matter of putting that in |
|
Thanks! I should be able to test that. It doesn't depend on EFI though, does it? I use direct kernel boot in my setup. |
|
Btw, what about auto-generating the json config based on configured command-line? As a future usability improvement... |
As a PoC it doesn't matter, but can't you just
Yes, I think we should do this, in fact I think we should replace the In addition to that we might want to consider |
I've tried that but doesn't seem to get propagated to the guest. Sending EOF works fine though. |
|
Ok, I am going to rework this PR soon but since I don't need any changes in libkrun itself to actually pass arguments to the VM (just the init, right?), this really doesn't have the priority to be included in the upcoming 1.17 release. I didn't count with any binary distribution of FreeBSD init along with libkrun anyway. I can easily build it separately. |
It's possible to do this in 2 stages, here you add support for for this in the init and you can manually set up the host side to pass in the arguments (like Sergio's PoC). For a second part, it would be nice to make I’m happy to handle the second part, but if you're inclined to work on that as well, just let me know! 👍 |
|
Also, I would make the init behavior be enabled by passing in a kernel arg (kenv on freebsd like you mentioned). |
|
@mtjhrc I'd also split the task as you say. Anyway, I was thinking I could handle both parts. About the release, @slp originally asked if I needed this in 1.17. Since there's no need for the base64 handling, all the pieces for constructing the json config are already in place so we just need the init support (and I'm building bsd init separately for my project anyway). If I'm able to make it work with 1.17 libkrun and a custom build of init, I'm happy. All the library changes are just automation. |
This PR makes it possible to use
initon FreeBSD.It also deals with the differences between BSD and Linux processing of the kernel command-line.
For one thing, FreeBSD kernel parameters are strictly
key=value.There's also no such thing as injecting arguments after
--into user space environment.All key/value pairs are available from kenv instead.
If you try to pass an argument without
=, the kernel will append="1"to it.In order to properly pass any space-delimited command-line, I store it as a sequence of null-terminated byte strings, apply base64 encoding, split it into chunks so that I don't exceed the maximum length of a kernel param value and pass it like
KRUN_INIT_ARGV0="<chunk0>",KRUN_INIT_ARGV1="<chunk1>", etc.Maximum length of the entire command-line is 512 bytes (
LBABI_MAX_COMMAND_LINE) unlike 2048 on Linux.