-
Notifications
You must be signed in to change notification settings - Fork 923
[bazel,qemu] Refactor QEMU rules to allow standalone execution #28859
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
Conversation
87a9b4a to
6d3f04c
Compare
|
For reference I also ran a full QEMU regression with no visible changes from the current results (the |
6d3f04c to
306180b
Compare
ziuziakowska
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, thanks!
pamaury
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.
Thanks Alex, this changes look good. Regarding the templating, I have a suggestion, I will write a comment on the PR.
|
I haven't actually tested it but in theory, an executable or test rule can return, in addition to the usual env = RunEnvironmentInfo({"VAR1": "value 1", "VAR2": "value2"}, [])Unfortunately, the way the exec_env are setup makes it a bit tricky because the dispatch function returns a tuple EDIT: here is an example in |
|
Thanks @pamaury, that's very useful info and makes sense. In the interest of time I think I'll leave this PR as is, but it would be nice to change in the future as I think the process you have described sounds nicer. What we currently have is fine, it just stops us from easily reusing the |
306180b to
6ba9d8f
Compare
Refactor all of the logic for constructing the arguments that must be passed to QEMU to run an OT Earlgrey 1.0.0 machine into a separate `qemu.sh` utility script that can be used to spawn a standalone instance of QEMU, without going through Bazel. This allows for more widespread use of QEMU outside of the current `opentitan_test` environment, which it was previously constrained to. This commit also makes a couple of modifications to the existing flow: - More arguments are now parameterized and can be changed as part of this standalone flow (e.g. the monitor/socket paths). - The cleanup trap handler logic in `qemu_test.sh` is fixed to take the daemonized process ID returned by QEMU, rather than trying to kill the original QEMU process (which should exit after daemonization, hence it was not doing anything and processes could persist through a `bazel run`). The kill output is silenced to clean up some noise. Signed-off-by: Alex Jones <[email protected]> Co-authored-by: James Wainwright <[email protected]>
6ba9d8f to
15a137a
Compare
|
Last couple of force pushes just move the |
|
Failing CI FPGA test |
This PR refactors the logic for constructing the list of arguments that must be passed to QEMU to run an OT Earlgrey 1.0.0 machine into a separate
qemu.shutility script that can be used to spawn a standalone instance of QEMU, without needing to go through anopentitan_test. This allows for more widespread use of QEMU outside of the current standard test environment, which it was previously constrained to.There are a couple of key differences from the existing flow that are worth noting:
qemu.shstart script and can be changed as part of this standalone flow (e.g. the monitor/socket paths). These are also used to be able to clean up leftover CharDev interfaces on exit so that abazel runshould be idempotent and not complain about existing PTYs.qemu_test.shhas been fixed so that it now takes the process ID of the daemonized process that is returned by QEMU, rather than trying to kill the original QEMU process (which should instead exit directly after daemonization). The kill output is also silenced and conditional to help clean up some noise.Aside: I've kept this as a bash script for now since I think it's just around simple enough that it would be more work to maintain an equivalent Python script, but it might be nice to convert these scripts to Python if they are likely to grow much larger.
Also, an open question: is there a nicer way to set environment variables to pass into
qemu_test.shinstead of using templating? I can't figure out all the moving parts to do that but I'm not a huge fan of the existing templating approach, albeit I know we use it in a few shell scripts with Bazel.