-
Notifications
You must be signed in to change notification settings - Fork 36
feat: add support for efuse in qemu #291
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
|
@hfudev Hello! This MR has added support for qemu efuse. |
|
@igrr PTAL, thanks! |
| '-global', | ||
| f'driver=nvram.{self.app.target}.efuse,property=drive,value=efuse', | ||
| '-serial', | ||
| f'tcp::{available_port},server,nowait', |
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.
When launching QEMU and expecting that it should communicate with another application, it is better to use -daemonize flag — as recommended by QEMU documentation. Otherwise there may be a race condition between QEMU getting ready to accept TCP connections and the launch of the other application. It seems child.expect('qemu') aims to work around that, but I am not sure this is reliable enough.
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.
Also, now that I'm thinking of this... don't you end up with two QEMU processes running at the same time, and accessing the same eFuse file?.. One QEMU process is already launched when dut is created, another one is launched here.
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.
When launching QEMU and expecting that it should communicate with another application, it is better to use
-daemonizeflag — as recommended by QEMU documentation. Otherwise there may be a race condition between QEMU getting ready to accept TCP connections and the launch of the other application. It seemschild.expect('qemu')aims to work around that, but I am not sure this is reliable enough.
I think solution using pexpect and nowait flag is reliable enough. Once qemu appears in the output, it indicates that QEMU is ready to accept connections. Another solution using -daemonize also works:
run_qemu_command = [
'-daemonize',
'-display',
'none',
'-machine',
self.app.target,
'-drive',
f'file={self.image_path},if=mtd,format=raw',
'-global',
self.QEMU_STRAP_MODE_FMT.format(self.app.target, QEMU_TARGETS[self.app.target].strap_mode),
'-drive',
f'file={self.efuse_path},if=none,format=raw,id=efuse',
'-global',
f'driver=nvram.{self.app.target}.efuse,property=drive,value=efuse',
'-serial',
f'tcp::{available_port},server,nowait',
]
try:
proc = subprocess.run(
[self.current_qemu_executable_path, *run_qemu_command],
check=True
)
res = shlex.split(command)
...But I suggest to keep the current implementation.
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.
Also, now that I'm thinking of this... don't you end up with two QEMU processes running at the same time, and accessing the same eFuse file?.. One QEMU process is already launched when
dutis created, another one is launched here.
Yes, but the first process never writes to the eFuse file. All write operations are performed via TCP using espefuse. So it should be ok.
ee38c42 to
3083101
Compare
3083101 to
603545a
Compare
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!
|
@igrr PTAL again. Thank you! |
|
Two Wokwi tests are failing due to missing access to secrets. These failures are unrelated to the current changes. |
603545a to
ca48197
Compare
|
@achxkloel Thank you! |
|
@hfudev Let's merge it first, and if there are any requests for improvement/bug, we can just fix them? |
ca48197 to
e682c45
Compare
Closes #269