aws-nitro: Separate guest kernel module loading, updates#549
aws-nitro: Separate guest kernel module loading, updates#549tylerfanelli merged 4 commits intocontainers:mainfrom
Conversation
tylerfanelli
requested review from
MatiasVara,
dorindabassey,
jakecorrenti,
mtjhrc and
slp
as code owners
|
/gemini |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request refactors the kernel module loading mechanism by introducing a new mod.c file and separating debug output handling from application output. The debug flag has been consistently renamed to app_output across the C and Rust codebases, improving clarity. The changes also include an optimization to strip symbols from the aws-nitro init binary and a new feature to stream debug output from the serial console. Overall, the changes enhance modularity, clarify intent, and improve the build process.
mz-pdm
left a comment
mz-pdm
left a comment
There was a problem hiding this comment.
The first two commits look fine to me, some comments to the other two.
(BTW I think it would be easier to handle it if different PRs were used for completely unrelated changes.)
tylerfanelli
force-pushed
the
awsnitro-init-modules
branch
from
b5fb457 to
41d7941
Compare
There was a problem hiding this comment.
I find the change from debug -> app_output a little confusing. The output from the application should always be present, and specifying debug was specifically to determine whether we should include the dmesg output. Conditionally, reading it as "if we're not in debug mode, do this..." made more sense to me than "if we want app output (which is functionally always true), do this". If I'm in the minority on that I'm not going to block it from going in as is.
The problem is that debug output needed to be printed earlier, and couldn't be a part of the normal proxy list, as if there's a problem we need to see debug output to find out where. We were unable to do this before because of all other proxies requiring initialization before debug out was viewable. |
tylerfanelli
force-pushed
the
awsnitro-init-modules
branch
from
41d7941 to
de75c57
Compare
Nitro enclaves do not allow user input, so it remains unclear if debugging the init binary would be possible. Strip debug symbols to shrink the binary size. Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>
Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>
Start the debug output proxy immediately after the enclave has booted (if debug mode is enabled). Change the debug enclave argument to instead represent enabling application output, as nothing has to be done by the enclave VM if debug output is enabled. In this case, output will be printed to the console, which libkrun will already be connected to and forwarding data to/from. Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>
The krun-nitro-eif-ctl tool allows users to configure the kernel modules loaded within their enclave. This is done through a directory `/krun_linux_mods` in the initrd. Read this directory and load each module into the enclave kernel within the bootstrap process. Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>
tylerfanelli
force-pushed
the
awsnitro-init-modules
branch
from
de75c57 to
b622ba8
Compare
mz-pdm
left a comment
mz-pdm
left a comment
There was a problem hiding this comment.
Thank you for the updates, it's good from my side now.
The logic behind the change makes sense, I was just commenting on the name change being confusing to me. |
4 participants
No description provided.