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

[9.4-stable] Add Support for Persistent OVMF Settings in Pillar #4269

Closed
wants to merge 7 commits into from

Conversation

shjala
Copy link
Member

@shjala shjala commented Sep 18, 2024

Backport of #4261.

@shjala shjala marked this pull request as draft September 18, 2024 18:56
@shjala shjala changed the title [WIP] [9.4-backport] Add config for FML mode custom resolution [WIP] [9.4-backport] Add Support for Persistent OVMF Settings in Pillar Sep 19, 2024
@OhmSpectator OhmSpectator force-pushed the 9.4_backport_fml.config branch 2 times, most recently from ddead4c to cf803d2 Compare September 19, 2024 12:35
@shjala shjala changed the title [WIP] [9.4-backport] Add Support for Persistent OVMF Settings in Pillar [9.4-backport] Add Support for Persistent OVMF Settings in Pillar Sep 19, 2024
@shjala
Copy link
Member Author

shjala commented Sep 19, 2024

May God be with us, and may the semicolons fall exactly where they should!

func getFmlCustomResolution(status *types.DomainStatus, globalConfig *types.ConfigItemValueMap) (string, error) {
fmlResolutions := status.FmlCustomResolution
// if not set in the domain status, try to get it from the global config
if fmlResolutions == types.FmlResolutionUnset {
Copy link
Member

Choose a reason for hiding this comment

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

@shjala, are you sure about this line? I the master branch I had to add a comparison with "" here. For some reason, it was a case...

Copy link
Member Author

@shjala shjala Sep 19, 2024

Choose a reason for hiding this comment

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

the status.FmlCustomResolution is always set from the domainmanager, either to a custom resolution that was read from the "cloud-config" or if nothing present in the "cloud-config" is set to Unset.

But now that you mentioned I think I should add a single line in case there is no "cloud-config" at all, but that shouldn't prevent us from testing, still can use the global config or "cloud-config".

Copy link
Member

@OhmSpectator OhmSpectator Sep 19, 2024

Choose a reason for hiding this comment

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

Yesterday, without this extra check for "", I got a situation when file resolution was set to "", and it resulted in the wrong filename and crash on app start.

I HOPE IT WILL NOT BE THE CASE NOW, AFTER HOURS OF WAITING FOR THE HECKING WIN APP TO BE DOWNLOADED. Sorry =D

Copy link
Member Author

Choose a reason for hiding this comment

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

I tested it on 9.4 multiple times, both cloud-config and global config where working fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

you can check for yourself logread -f | grep FmlResolution.

Copy link
Member

Choose a reason for hiding this comment

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

My deployment process has yet to reach the point where this code is called...

@OhmSpectator
Copy link
Member

Manual test failed =( Will have to retest with a smaller app (

Copy link
Member

@OhmSpectator OhmSpectator left a comment

Choose a reason for hiding this comment

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

Approving to see the Eden tests results.

@shjala
Copy link
Member Author

shjala commented Sep 20, 2024

Didn't we updated the UEFI hash? build is failing on Eden.

@OhmSpectator
Copy link
Member

Didn't we updated the UEFI hash? build is failing on Eden.

We should revert the changes in the UEFI package and hence - not update the hash. I remove the 2 corresponding commits from the master PR (updating UEFI, updating hsashes)

@OhmSpectator
Copy link
Member

These commits should be dropped:

@OhmSpectator
Copy link
Member

I'm taking care of the PR to sync it with the master version.

@shjala
Copy link
Member Author

shjala commented Sep 20, 2024

I'm taking care of the PR to sync it with the master version.

awesome, thanks!

OhmSpectator and others added 6 commits September 20, 2024 16:16
This commit copies OVMF_VARS.fd into the pillar container by adding it
to /usr/lib/xen/boot/ovmf_vars.bin. It is important that the file is
available in the pillar container because Pillar will create per-domain
copies of it stored in /persist, which are then accessible to the
xen-tools container. This sets the groundwork for enabling virtual
machines to save and retain UEFI settings across reboots by using
per-domain NVRAM files.

Signed-off-by: Nikolay Martyanov <nikolay@zededa.com>
Introduce support for managing per-domain OVMF_VARS.fd files, which are
essential for maintaining persistent UEFI settings for FML guests. It
adds functionality to prepare and clean up individual OVMF settings
files stored in the persist directory, ensuring that each virtual
machine has its own dedicated NVRAM file. The VM configuration
structures are updated to reference the bootloader settings file,
enabling the creation of unique UEFI variable stores for each domain.

Signed-off-by: Nikolay Martyanov <nikolay@zededa.com>
Switch to using separate OVMF_CODE.fd and OVMF_VARS.fd files for FML x86
modes instead of a combined .bin file. This ensures that settings are
stored correctly and maintains consistent naming conventions. These
changes do not affect containers, ARM or Xen.

To support ARM the OVMF build should produce separate files. Currently
it produces QEMU_EFI that incorporates both code and variable sections.

Signed-off-by: Nikolay Martyanov <nikolay@zededa.com>
Add a new global config value app.fml.resolution to set custom resolution
for FML apps. This is a string value in the format of "widthxheight".

This value can be set device-wide as a global config value or set
in a per-app/vm setting by defining it as top-level variable in
the cloud-config.

Signed-off-by: Shahriyar Jalayeri <shahriyar@zededa.com>
Implemented logic to select predefined OVMF_VARS.fd files for specific
screen resolutions. Added pre-saved OVMF settings for 800x600, 1024x768,
1280x800, and 1920x1080 resolutions, ensuring clean boot entries.

Signed-off-by: Nikolay Martyanov <nikolay@zededa.com>
Adapt the existing tests to account for the addition of the OVMF
settings file for FML guests, ensuring the expected output includes the
BootLoader. It also fixes the ARM tests by ensuring the firmware line is
present in the expected configuration, aligning the tests with the
updated behavior for UEFI boot on both AMD64 and ARM architectures.

Signed-off-by: Nikolay Martyanov <nikolay@zededa.com>
@OhmSpectator OhmSpectator changed the title [9.4-backport] Add Support for Persistent OVMF Settings in Pillar [9.4-stable] Add Support for Persistent OVMF Settings in Pillar Sep 20, 2024
@OhmSpectator OhmSpectator marked this pull request as ready for review September 20, 2024 16:31
Copy link
Member

@OhmSpectator OhmSpectator left a comment

Choose a reason for hiding this comment

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

Approving to run Eden

In the FIRMWARE doc, add a new section explaining what OVMF is, when
it's used in EVE, the key OVMF files, how settings are managed, and
future automation plans.

Signed-off-by: Nikolay Martyanov <nikolay@zededa.com>
@eriknordmark
Copy link
Contributor

Backport of #4261 and #4264 .

Seems like all of the commits are in #4261 so it makes sense to update the description.

@OhmSpectator
Copy link
Member

OhmSpectator commented Sep 22, 2024

Eden fails to start QEMU with EVE (not Application!) with this error:

qemu-system-x86_64:/home/runner/.eden/default-qemu.conf:8: Could not open '/home/runner/work/eve/eve/dist/default-images/eve/OVMF_CODE.fd': No such file or directory

That's strange... I remember we had this problem previously and I hope it does not related to the changes.

I see the following fix by @milan-zededa in the Eden repo that fixes similar issue:
lf-edge/eden@33ed9a7

So I would either ignore the Eden tests failure or ask @milan-zededa and @uncleDecart to help with the Eden tests fix.

@OhmSpectator OhmSpectator changed the base branch from 9.4-stable-EOS to 9.4-stable September 23, 2024 08:16
Copy link
Member

@OhmSpectator OhmSpectator left a comment

Choose a reason for hiding this comment

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

Rerun workflows

@OhmSpectator
Copy link
Member

I'll close this PR and create a new one (into the "-stable" branch) to trigger the necessary workflows.

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.

3 participants