Add Support for Persistent OVMF Settings in Pillar #4261

OhmSpectator · 2024-09-16T17:18:15Z

Node 0. This PR is not intended to be perfect. It is sourced from the need to support the predefined resolution feature. During the review, please check for possible regressions. Regarding code format, style, or even design imperfections, please don't be too strict. I will be happy to rework some parts and remove the shortcuts more cleanly later. However, we need this feature soon, and I promise to address the style issues afterward.

This PR introduces support for persistent OVMF settings. By implementing per-domain OVMF_VARS.fd file management, each FML virtual machine can maintain its own UEFI configuration, ensuring that settings persist across reboots. The changes include updating the Dockerfile to include the OVMF_VARS.fd template and enhancing the KVM setup process to create and manage individual NVRAM files for each VM.

Note 1: The current implementation lacks the cleanup of the settings files in the case when the application was removed while the device is off (so, no Cleanup() handler was called)

Note 2: The current implementation does not affect how the OVMF is handled in the case of ARM. ARM also uses this firmware, but we intentionally do not change how it's supported so that fewer changes are tested. For your information, @rene.

TODO:

Run manual test to check that FML guest can persist the UEFI settings
Cleanup pthe settings on App upgrade (???)
Try on the original version of OVMF
Take the latest version of the config setting from [9.4-stable] Add Support for Persistent OVMF Settings in Pillar #4269 or [WIP] Add config for FML mode custom resolution #4264 (depends on where is the most version is)
Add documentaton

rucoder · 2024-09-16T17:30:01Z

pkg/pillar/hypervisor/kvm.go

+
+[drive "drive-ovmf-vars"]
+  if = "pflash"
+  format = "raw"


maybe it makes sense to specify "readonly" "off"

Maybe... This I will understand during the test)))

I've grepped it over the internet, usually code is read-only, vars is not.

Yeah, I also saw it. I'm just not sure we have set the read-only option to "off" explicitly.

in my test, at least ubuntu boots up fine with having readonly for the OVMF_VAR.fd too.

pkg/pillar/hypervisor/kvm.go

pkg/pillar/types/domainmgrtypes.go

pkg/pillar/hypervisor/kvm.go

rouming · 2024-09-17T13:34:45Z

My only concern is compatibility with "legacy" guests: @OhmSpectator @rucoder, can we still boot non-UEFI oses? If not, then we break compatibility and better to have this under an option: guest.uefi.support = yes or similar.

OhmSpectator · 2024-09-17T13:40:10Z

My only concern is compatibility with "legacy" guests: @OhmSpectator @rucoder, can we still boot non-UEFI oses? If not, then we break compatibility and better to have this under an option: guest.uefi.support = yes or similar.

But where do we break it? The new options are used only for FML guests which are always UEFI-based.

rouming · 2024-09-17T13:43:51Z

My only concern is compatibility with "legacy" guests: @OhmSpectator @rucoder, can we still boot non-UEFI oses? If not, then we break compatibility and better to have this under an option: guest.uefi.support = yes or similar.

But where do we break it? The new options are used only for FML guests which are always UEFI-based.

I missed the point that we add only settings, this is not a full switch from legacy bios to uefi firmware. Thanks for the clarification.

rene · 2024-09-18T16:36:55Z

pkg/pillar/types/domainmgrtypes.go

+	RootDev                string // default "/dev/xvda1"
+	ExtraArgs              string // added to bootargs
+	BootLoader             string // default ""
+	BootLoaderSettingsFile string // used to pass bootloader settings file, for example OVMF_VARS.fd


Do you really need this field here? This struct is derived from the API (https://github.com/lf-edge/eve-api/blob/main/proto/config/vm.proto#L31), AFAIK the fixed resources are populated by the controller, if the settings file has a fixed pattern you can just go and look for the file, or is there any idea of allow custom files configured from the cloud? In this case you will need to add this field to the API....

I used it as a part of a status. DomainStatus also includes VmConfig...
But yeah, I can move it somewhere else

yeah, I put it there just to use un the template parsing... maybe it will be cleaner just to add an extra arg to the template itself

rene · 2024-09-18T16:39:30Z

pkg/uefi/Dockerfile

@@ -29,8 +29,8 @@ COPY rpi /rpi

 FROM build AS build-amd64-versions

-ENV EDK_VERSION edk2-stable202005
-ENV EDK_COMMIT ca407c7246bf405da6d9b1b9d93e5e7f17b4b1f9
+ENV EDK_VERSION edk2-stable202208


Please, remove the old patches files for edk2-stable202005

Do it later when everything is tested and we are sure)

OhmSpectator · 2024-09-20T06:51:45Z

The RISC-V build failed... Will take a look. Buah.
UPD. THe build works... It was a dockerhub issue.

shjala · 2024-09-20T07:44:05Z

Get the latest version of #4264 .

OhmSpectator · 2024-09-20T08:37:27Z

@shjala, thanks, will take it!

pkg/pillar/types/locationconsts.go

eriknordmark

I see the current usage is only about display resolution. But will this usage grow to cover other settings?

Would it make sense to descrive that in a short markdown file in docs?

And any hope of later seeing some regression tests for this capability?

OhmSpectator · 2024-09-20T09:20:20Z

I see the current usage is only about display resolution. But will this usage grow to cover other settings?

I would prefer to rework the mechanism before providing more options. Precooked binaries is not a good idea for a big set of different options. We should figure out how to alter the settings in better way or to learn how to apply them in runtime. That would be a proper solution. Unfortunatelly OVMF_VAR file format is binary and pretty complicated.

Would it make sense to descrive that in a short markdown file in docs?

I would put info about how the OVMF settings are generated. Also would mention what would be a better approach. But no sure, where is the best pace for such a doc

And any hope of later seeing some regression tests for this capability?

There is always hope for better testing! @shjala, do you think it's quickly doable? In any case I would add the tests after the merge of the feature...

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>

shjala · 2024-09-20T15:45:24Z

And any hope of later seeing some regression tests for this capability?

There is always hope for better testing! @shjala, do you think it's quickly doable? In any case I would add the tests after the merge of the feature...

Yes, we can have Eden test for it, but I need to cook some smaller images, with what we have now it takes about half an hour to run it in a local setup (cloud+datastore+eve all in e same machine).

pkg/pillar/hypervisor/kvm.go

shjala · 2024-09-20T15:56:53Z

I see the current usage is only about display resolution. But will this usage grow to cover other settings?

I would prefer to rework the mechanism before providing more options. Precooked binaries is not a good idea for a big set of different options. We should figure out how to alter the settings in better way or to learn how to apply them in runtime. That would be a proper solution. Unfortunatelly OVMF_VAR file format is binary and pretty complicated.

Yes, maybe an Efi application to set the custom variables and dump a custom OVMF_VAR, Ubuntu is doing it this way to pre-populate secure-boot keys for example.

shjala

LGTM, just the debug logError.

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>

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>

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

LGTM

OhmSpectator requested review from rene, rouming and eriknordmark as code owners September 16, 2024 17:18

OhmSpectator marked this pull request as draft September 16, 2024 17:18

github-actions bot requested review from rucoder and shjala September 16, 2024 17:18

OhmSpectator force-pushed the feature/persist-ovmf-settings branch from c0426ac to 72c96a2 Compare September 16, 2024 17:31

rucoder reviewed Sep 16, 2024

View reviewed changes

christoph-zededa reviewed Sep 16, 2024

View reviewed changes

pkg/pillar/hypervisor/kvm.go Outdated Show resolved Hide resolved

christoph-zededa reviewed Sep 16, 2024

View reviewed changes

pkg/pillar/hypervisor/kvm.go Outdated Show resolved Hide resolved

OhmSpectator force-pushed the feature/persist-ovmf-settings branch from 72c96a2 to 879c0ea Compare September 16, 2024 20:48

github-actions bot requested a review from rucoder September 16, 2024 20:48

OhmSpectator mentioned this pull request Sep 16, 2024

[WIP] Backport "Add Support for Persistent OVMF Settings in Pillar" #4262

Closed

2 tasks

OhmSpectator force-pushed the feature/persist-ovmf-settings branch from 879c0ea to 7ab829d Compare September 17, 2024 09:08

christoph-zededa reviewed Sep 17, 2024

View reviewed changes

pkg/pillar/hypervisor/kvm.go Outdated Show resolved Hide resolved

OhmSpectator force-pushed the feature/persist-ovmf-settings branch from 7ab829d to cc56e06 Compare September 17, 2024 09:22

shjala mentioned this pull request Sep 17, 2024

[WIP] Add config for FML mode custom resolution #4264

Closed

OhmSpectator force-pushed the feature/persist-ovmf-settings branch from cc56e06 to afd5710 Compare September 17, 2024 10:38

OhmSpectator changed the title ~~Add Support for Persistent OVMF Settings in Pillar~~ [WIP] Add Support for Persistent OVMF Settings in Pillar Sep 18, 2024

OhmSpectator force-pushed the feature/persist-ovmf-settings branch 2 times, most recently from 626374e to f8ed3c6 Compare September 18, 2024 16:15

rene reviewed Sep 18, 2024

View reviewed changes

OhmSpectator force-pushed the feature/persist-ovmf-settings branch from f8ed3c6 to 61569b0 Compare September 18, 2024 16:52

github-actions bot requested a review from rene September 18, 2024 16:52

OhmSpectator force-pushed the feature/persist-ovmf-settings branch 2 times, most recently from 1753a04 to 56f44cd Compare September 18, 2024 17:15

OhmSpectator marked this pull request as draft September 20, 2024 09:10

eriknordmark reviewed Sep 20, 2024

View reviewed changes

pkg/pillar/types/locationconsts.go Outdated Show resolved Hide resolved

eriknordmark reviewed Sep 20, 2024

View reviewed changes

OhmSpectator force-pushed the feature/persist-ovmf-settings branch from 26a960b to c567bbd Compare September 20, 2024 09:32

OhmSpectator marked this pull request as ready for review September 20, 2024 09:32

github-actions bot requested a review from eriknordmark September 20, 2024 09:33

OhmSpectator force-pushed the feature/persist-ovmf-settings branch from c567bbd to 443f73a Compare September 20, 2024 12:40

OhmSpectator added 2 commits September 20, 2024 14:43

OhmSpectator force-pushed the feature/persist-ovmf-settings branch 2 times, most recently from 9dbba62 to f23e947 Compare September 20, 2024 13:04

shjala reviewed Sep 20, 2024

View reviewed changes

pkg/pillar/hypervisor/kvm.go Outdated Show resolved Hide resolved

shjala approved these changes Sep 20, 2024

View reviewed changes

OhmSpectator force-pushed the feature/persist-ovmf-settings branch from f23e947 to 48cdfd1 Compare September 20, 2024 16:12

github-actions bot requested a review from shjala September 20, 2024 16:12

OhmSpectator and others added 4 commits September 20, 2024 18:29

OhmSpectator force-pushed the feature/persist-ovmf-settings branch from 48cdfd1 to a39040c Compare September 20, 2024 16:30

docs: Describe OVMF usage.

abdf9b4

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 approved these changes Sep 20, 2024

View reviewed changes

eriknordmark merged commit 4447749 into lf-edge:master Sep 22, 2024
38 checks passed

OhmSpectator mentioned this pull request Sep 23, 2024

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Support for Persistent OVMF Settings in Pillar #4261

Add Support for Persistent OVMF Settings in Pillar #4261

OhmSpectator commented Sep 16, 2024 •

edited

Loading

rucoder Sep 16, 2024

OhmSpectator Sep 16, 2024

shjala Sep 17, 2024

OhmSpectator Sep 17, 2024

shjala Sep 18, 2024

rouming commented Sep 17, 2024

OhmSpectator commented Sep 17, 2024

rouming commented Sep 17, 2024

rene Sep 18, 2024

OhmSpectator Sep 18, 2024

OhmSpectator Sep 18, 2024

rene Sep 18, 2024

OhmSpectator Sep 18, 2024

OhmSpectator commented Sep 20, 2024 •

edited

Loading

shjala commented Sep 20, 2024 •

edited

Loading

OhmSpectator commented Sep 20, 2024

eriknordmark left a comment

OhmSpectator commented Sep 20, 2024

shjala commented Sep 20, 2024

shjala commented Sep 20, 2024

shjala left a comment

eriknordmark left a comment

Add Support for Persistent OVMF Settings in Pillar #4261

Add Support for Persistent OVMF Settings in Pillar #4261

Conversation

OhmSpectator commented Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rouming commented Sep 17, 2024

OhmSpectator commented Sep 17, 2024

rouming commented Sep 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OhmSpectator commented Sep 20, 2024 • edited Loading

shjala commented Sep 20, 2024 • edited Loading

OhmSpectator commented Sep 20, 2024

eriknordmark left a comment

Choose a reason for hiding this comment

OhmSpectator commented Sep 20, 2024

shjala commented Sep 20, 2024

shjala commented Sep 20, 2024

shjala left a comment

Choose a reason for hiding this comment

eriknordmark left a comment

Choose a reason for hiding this comment

OhmSpectator commented Sep 16, 2024 •

edited

Loading

OhmSpectator commented Sep 20, 2024 •

edited

Loading

shjala commented Sep 20, 2024 •

edited

Loading