Skip to content
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

Minor QEMU tweaks for x86 and custom firmware #1484

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sjg20
Copy link
Contributor

@sjg20 sjg20 commented Aug 22, 2024

This PR increases the flexibility of the QEMU driver so that it can support booting different firmware (i.e. U-Boot) and also running on x86 where some parameters are typically not specified.

  • Documentation for the feature
  • Tests for the feature
  • The arguments and description in doc/configuration.rst have been updated
  • PR has been tested

@@ -192,6 +204,9 @@ def get_qemu_base_args(self):
cmd.append("-bios")
cmd.append(
self.target.env.config.get_image_path(self.bios))
elif self._bios_fname:
cmd.append("-bios")
cmd.append(self._bios_fname)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we can provide bios via the client arguments and than set _bios_fname via the set_bios() function. Why don't you modify bios instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well in this case 'bios' would not be provided.

The point here is that the strategy can provide the firmware, no matter what board type is chosen. We don't have to have a different 'bios' for every architecture / target.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case this driver is used by your strategy it is not provided, but it is still possible. Please modify this to use self.bios instead of the new hidden self._bios_fname.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I can check for an existing file and then that can work.

sjg20 added 4 commits January 20, 2025 15:03
At least for x86 devices it is not necessary to provide these arguments.
Make them optional.

Add an x86 example to the documentation too.

Signed-off-by: Simon Glass <[email protected]>
These arguments are currently set on activation. Where the driver is
being used by a strategy, it may need to set the arguments after that.
There is no need to have everything set in stone until the on() method
is called, so move it there.

For existing users, no functional change is intended.

Signed-off-by: Simon Glass <[email protected]>
The U-Boot strategy will need to provide the filename of the BIOS
directly when booting U-Boot on QEMU. Add a method to support this.

Signed-off-by: Simon Glass <[email protected]>
Rather than provide a strange exception, complain when a strategy does
not use the QEMU driver properly, perhaps due to user settings.

Series-changes: 6
- Add new patch to report an error if QEMU is not turned on

Signed-off-by: Simon Glass <[email protected]>
Copy link

codecov bot commented Jan 20, 2025

Codecov Report

Attention: Patch coverage is 60.60606% with 13 lines in your changes missing coverage. Please review.

Project coverage is 56.1%. Comparing base (fff35ee) to head (a46870d).

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
labgrid/driver/qemudriver.py 60.6% 13 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           master   #1484     +/-   ##
========================================
- Coverage    56.2%   56.1%   -0.1%     
========================================
  Files         170     170             
  Lines       13248   13260     +12     
========================================
+ Hits         7449    7451      +2     
- Misses       5799    5809     +10     
Flag Coverage Δ
3.10 56.1% <60.6%> (-0.1%) ⬇️
3.11 56.1% <60.6%> (-0.1%) ⬇️
3.12 56.1% <60.6%> (-0.1%) ⬇️
3.13 56.1% <60.6%> (-0.1%) ⬇️
3.9 56.2% <60.6%> (-0.1%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants