-
Notifications
You must be signed in to change notification settings - Fork 261
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
Change save/restore nvram data logic #887
Conversation
Save NVRAM data not only if BiosType is Q35_SECURE_BOOT Also save if Q35_OVMF because it also includes EFI boot variables With Q35_OVMF we unable to boot non-fallback bootloaders in place different from <esp>/EFI/BOOT/BOOT<arch>.EFI To check this it needed to: 1) Run VM with installed Linux (it doesn't matter what distro) 2) Add a bootloader via efibbotmgr efibootmgr -c -d <boot device> -L "Some Linux" -l \\EFI\\<somevendor>\\grubx64.efi 3) Look at created efi boot variable efibootmgr -v It's present 4) Shutdown VM 5) Run it again 6) Check efi boot variables again efibootmgr -v 7) We got non bootable VM if fallback bootloader is broken or boot default esp/EFI/BOOT/BOOT<arch>.EFI otherwise Signed-off-by: Anton Fadeev <anton.fadeev@red-soft.ru>
...d/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/SaveVmExternalDataCommand.java
Outdated
Show resolved
Hide resolved
The idea looks OK to me, IIRC there are no inherent limitations when keeping NVRAM data. It adds some Vdsm traffic and other machinery, even if the EFI variables are not modified I think, but this doesn't differ from using secure boot. One thing I suggest to check with this patch is whether the NVRAM data is deleted when switching the VM to secure boot and back. Should it be deleted or not? |
b379440
to
40a27df
Compare
NVRAM data will be deleted when switching the VM from secure boot, by this trigger
When switching from Uefi to Secure Boot it will be not deleted. |
Now the question is whether it's good to delete the NVRAM data when switching from secure boot. I'd say it's on the safe side -- deleting any possible secrets stored there. It will delete the other EFI variables too though but one shouldn't switch an installed VM from secure boot without a good reason so it doesn't look like a big problem. Do you agree? |
I think yes. |
Since now |
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.
Looks good, just need to check the stored procedures again, as mentioned above.
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.
Looks good to me and I think we agreed on the behavior regarding data deletion on BIOS switching.
@mz-pdm With this patch, NVRAM data will be available with BIOS type |
@smelamud Ah, I see your point now. Do you think it should be deleted in such a case? There is some reason to delete it when switching from secure boot (due to possible presence of secrets, not to keep them around unnecessarily). Perhaps as a general cleanup in other situations? A related question would be whether there is a confirmation dialog for the given deletion that should be updated too. |
@mz-pdm Yes, as a general cleanup, I think. And the confirmation dialog should be shown, because BIOS type change may be unintentional. |
The confirmation dialog was added, as you described. Please look at this. |
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, some minor suggestions, up to you.
...dmin/modules/uicompat/src/main/resources/org/ovirt/engine/ui/uicompat/UIConstants.properties
Outdated
Show resolved
Hide resolved
...odules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmListModel.java
Outdated
Show resolved
Hide resolved
...odules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmListModel.java
Outdated
Show resolved
Hide resolved
...odules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmListModel.java
Outdated
Show resolved
Hide resolved
Everything has been fixed as posted. Please look at this. |
...odules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmListModel.java
Outdated
Show resolved
Hide resolved
Except 2 small suggestions as a follow-up of the previous code review, LGTM. |
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.
My comments were addressed, LGTM.
But suggest to wait for more approvals.
Hi, looks like DCO test is failing, can you please follow instructions on https://github.com/oVirt/ovirt-engine/pull/887/checks?check_run_id=23100801322 for signing off your commits? |
Fix typo in method name. isUEFI -> isUefi Signed-off-by: Anton Fadeev <anton.fadeev@red-soft.ru>
…nd added a condition in the Remove_nvram_data function When changing the chipset from the Q35 chipset with UEFI SecureBoot to any other and with a Q35 chipset with UEFI, a Q35 chipset with BIOS, or an I440FX chipset with BIOS, the nvram is cleared. A window based on the ConfirmationModel has been added to notify the user about clearing NVRAM when changing the BIOS type Signed-off-by: Mochalin Nikolay <mochalin.nikolay2017@yandex.ru>
eb7c064
to
0541272
Compare
Signed-off-by: Mochalin Nikolay <nikolay.mochalin@red-soft.ru>
Signed-off-by: Mochalin Nikolay <nikolay.mochalin@red-soft.ru>
c0d6729
to
857772b
Compare
All done. Please look at this. |
Build fails on: Error: Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.10.1:compile (default-compile) on project uicommonweb: Compilation failure: Compilation failure:
Error: /github/home/rpmbuild/BUILD/ovirt-engine-4.5.5/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmListModel.java:[799,112] illegal start of expression
Error: /github/home/rpmbuild/BUILD/ovirt-engine-4.5.5/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmListModel.java:[800,117] ';' expected
Error: /github/home/rpmbuild/BUILD/ovirt-engine-4.5.5/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmListModel.java:[807,15] 'else' without 'if' |
Signed-off-by: Anton Fadeev <anton.fadeev@red-soft.ru>
857772b
to
318fbb6
Compare
I'm sorry. already fixed |
...odules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmListModel.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Anton Fadeev <anton.fadeev@red-soft.ru>
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.
Looks good, thanks!
/ost |
1 similar comment
/ost |
/ost |
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.
OST passed, merging based on previous approvals
Save NVRAM data not only if BiosType is Q35_SECURE_BOOT Also save if Q35_OVMF because it also includes EFI boot variables With Q35_OVMF we unable to boot non-fallback bootloaders in place different from /EFI/BOOT/BOOT.EFI
To check this it needed to:
efibootmgr -c -d -L "Some Linux" -l \EFI\\grubx64.efi
efibootmgr -v
It's present
efibootmgr -v
otherwise