- Notifications
You must be signed in to change notification settings - Fork123
Generalize external firmware support#412
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ce6d5f0 to65b2c09Compare
tylerfanelli 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.
LGTM. Two non-blocking comments.
| run:brew tap slp/krunkit && brew install virglrenderer clang-format llvm | ||
| -name:Create a fake init | ||
| run:touch init/init |
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.
Why is this needed?
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.
To satisfy this in virtio-fs:
static INIT_BINARY: &[u8] = include_bytes!("../../../../../../init/init");This was gated out in the EFI variant, but with the unification we need it for building both variants.
include/libkrun.h Outdated
| * Zero on success or a negative error number on failure. | ||
| */ | ||
| int32_tkrun_init_log(inttarget_fd,uint32_tlevel,uint32_tstyle,uint32_toptions); | ||
| int32_tkrun_init_log(inttarget_fd,uint32_tlevel,uint32_tstyle, |
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.
Seems like there' some unrelated changes other than addingkrun_set_firmware in this commit. Not a problem, just making sure you're aware (perhaps this was from some C formatting tool?)
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.
Yeah, please remove this, the file doesn't currentlyclang-format nicely, because of our syntax for the doc comments (we should consider maybe using doxygen, which it might understand).
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.
Thanks, vscode did this silently for me. I've restored sanity...
We were only use that struct for storing the kernel command line, solet's give it a better name.Signed-off-by: Sergio Lopez <slp@redhat.com>
We'll use it to store the firmware properties.Signed-off-by: Sergio Lopez <slp@redhat.com>
In the next commit we'll remove the EFI-specific behavior ofvirtio-fs, meaning the EFI build will also require an existinginit/init to build.Let's change our CI to create one, same as we do with otherenvironments.Signed-off-by: Sergio Lopez <slp@redhat.com>
Now that we have an object to store the firmware properties, let's useit in the EFI flavor to unify its behavior with the generic flavor, asa first step towards deprecating the former.Signed-off-by: Sergio Lopez <slp@redhat.com>
Introduce a new API to allow users setting the path of the firmware tobe loaded into the VM.Signed-off-by: Sergio Lopez <slp@redhat.com>
We have a number of layout consts in mod.rs, let's move them tolayout.rs.Signed-off-by: Sergio Lopez <slp@redhat.com>
Reuse the firmware support we have for aarch64 and enable x86_64 tomake use of it.Signed-off-by: Sergio Lopez <slp@redhat.com>
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.
Changes lgtm.
I'm still trying to understand the bigger picture... Am I understanding correctly that this opens the door to allowing the user to specify, for example, they want to use edk2 instead of libkrunfw?
slp commentedSep 26, 2025
The door was already open with |
2ecba0f intocontainers:mainUh oh!
There was an error while loading.Please reload this page.
So far, only the EFI flavor was capable of booting from a firmware. Let's generalize firmware loading into the generic flavor, introducing a new API
krun_set_firmwareto allow users configuring the path to the firmware to be loaded into the VM.