Merge ~ubuntu-core-dev/grub/+git/ubuntu:sil2100/auto-nvram into ~ubuntu-core-dev/grub/+git/ubuntu:ubuntu

Proposed by Łukasz Zemczak
Status: Merged
Approved by: Mathieu Trudel-Lapierre
Approved revision: 7b18c6eb099df89c5bd4a8f1404c52c5267b8c29
Merge reported by: Mathieu Trudel-Lapierre
Merged at revision: 0ef4190ef99c4b0762afd7f4b634e1861a7ac30b
Proposed branch: ~ubuntu-core-dev/grub/+git/ubuntu:sil2100/auto-nvram
Merge into: ~ubuntu-core-dev/grub/+git/ubuntu:ubuntu
Diff against target: 210 lines (+47/-14)
5 files modified
grub-core/osdep/basic/no_platform.c (+3/-2)
grub-core/osdep/unix/platform.c (+23/-2)
grub-core/osdep/windows/platform.c (+3/-2)
include/grub/util/install.h (+3/-2)
util/grub-install.c (+15/-6)
Reviewer Review Type Date Requested Status
Mathieu Trudel-Lapierre Approve
Review via email: mp+341670@code.staging.launchpad.net

Commit message

Add the auto-nvram functionality of only attempting the NVRAM variable update if the system has access to those. Useful for dual BIOS-EFI devices.

Description of the change

Add the auto-nvram functionality of only attempting the NVRAM variable update if the system has access to those. Useful for dual BIOS-EFI devices.

This branch's approach is to add a new parameter to the existing update procedures for EFI/IEEE1275. I like this approach since it's not adding any new functions to the namespace that need re-implementation for each platform. That being said, another MP will be following with the approach of simply adding new functions for doing the NVRAM existence checks which might be 'cleaner'.

This is the first time I'm submitting an MP for grub2 which is git-dpm based. I know this branch probably needs to have a git-dpm update-patches + dch run done, but for review's sake I'm not doing this step right now.

To post a comment you must log in.
Revision history for this message
Łukasz Zemczak (sil2100) wrote :

The 'alternative' approach can be found here:

https://code.launchpad.net/~sil2100/grub2/+git/ubuntu/+ref/sil2100/auto-nvram-alternative

But as I said, I am more in favor of the one that's being proposed for merging here.

Revision history for this message
Mathieu Trudel-Lapierre (cyphermox) wrote :

What is the use of this new feature? How does it differ from --no-nvram?

I'm curious because it seems like this is simply "warning" and skipping setting variables, yet you usually do want to update variables in nearly all cases -- when you can't, this probably should be a critical bug and a proper failure.

I'm also unsure about the implementation, this looks like the behavior of "auto-nvram" and "no-nvram" probably could be merged to some degree.

This won't exactly help dual EFI/non-EFI devices much -- in the case where you're installing from a BIOS-installed system, you won't find the variables anyway to try to update them, and if you're booted in EFI mode, you're detecting whether the variables are there (and they should, as above, if they're not, this is potentially a critical issue; these special cases might well be best served with just --no-nvram). Maybe I've missed some important piece, but I see no logic handling the "I booted in BIOS mode, but I really want both things updated". I also have no idea how to handle such a case anyway.

review: Needs Information
Revision history for this message
Łukasz Zemczak (sil2100) wrote :

It differs from --no-nvram as it does attempt updating the EFI variables if present. So my understanding here, and I'm not an expert of the dual EFI/non-EFI installation, is that we would want grub-install for Ubuntu to use --auto-nvram by default for both BIOS and EFI (in all cases).

If we want to have both grub-pc and shim-efi installed, normally a grub-install --target=x86_64-efi from shim-efi would just fail on a BIOS booted system. We can't append --no-nvram to it unconditionally as then in cases when we're booted in EFI, we won't get the variables updated while we should. Instead of detecting from inside shim-efi if we're running in BIOS mode or not, Steve's proposition was to just add the --auto-nvram option which tells grub-install: "if your target is x86_64-efi but there is no access to the NVRAM, don't fail the install as it just means we're in BIOS mode and this is not a critical error as we're probably handling a dual EFI/non-EFI situation".
This would mean we'd just be doing grub-install --target=x86_64-efi --auto-nvram for shim-efi instead. This way, if I understand it correctly of course, we could have both packages co-installed and not erroring out. It's really just migrating the detection from the packaging side (like doing things in postinst) to grub-install getting slightly smarter.

That's my understanding of what I was working on - I might be terribly wrong of course! Steve can give a more detailed explanation probably as he's the author of the proposal.

Revision history for this message
Mathieu Trudel-Lapierre (cyphermox) :
review: Approve
Revision history for this message
Łukasz Zemczak (sil2100) wrote :

There was an error fetching revisions from git servers. Please try again in a few minutes. If the problem persists, contact Launchpad support.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches