-
Notifications
You must be signed in to change notification settings - Fork 642
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
Add optional loading of UEFI firmware via -bios parameter in QEMU #3261
Conversation
I'm, like many people with JVM background, genuinely bad at naming user facing settings. Any better ideas than ugly |
pkg/limayaml/limayaml.go
Outdated
@@ -182,6 +182,11 @@ type Firmware struct { | |||
// LegacyBIOS is ignored for aarch64. | |||
LegacyBIOS *bool `yaml:"legacyBIOS,omitempty" json:"legacyBIOS,omitempty" jsonschema:"nullable"` | |||
|
|||
// CompatUEFIViaBIOS forces QEMU to load concatenated firmware with -bios option | |||
// CompatUEFIViaBIOS is ignored for non x86_64 | |||
// This should be deprecated, if the issue in QEMU is fixed https://gitlab.com/qemu-project/qemu/-/issues/513 |
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.
Since this is already planned to be deprecated since its birth, may this should rather be an env var?
Similar to LIMA_SSH_PORT_FORWARDER
(originally planned to be deprecated in v1.0, now postponed to v1.1)
https://lima-vm.io/docs/config/environment-variables/
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.
I agree. It might never be removed, because fixes in Windows QEMU version are not of the highest priority. If the ergonomics of the environment var would be insufficient it could be changed later into full-fledged option in Lima yaml. I'm moving this into draft to rework.
pkg/qemu/qemu.go
Outdated
var targetArch string | ||
switch arch { | ||
case limayaml.X8664: | ||
targetArch = "i386" |
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.
?
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.
x86_64 and i386 use the same layout for vars, so, there is no x86_64 version in the distribution. One needs to concatenate i386 to x86_64 code. This is the legacy of x86_64 being derivative of i386. Probably this deserves explanation with a comment.
pkg/qemu/qemu.go
Outdated
return "", nil, err | ||
} | ||
defer codeFile.Close() | ||
downloadedFile, err := os.OpenFile(downloadedFirmware, os.O_CREATE|os.O_WRONLY, 0o644) |
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.
Probably the file name should be changed, as the concatenated file would be incompatible with QemuEfiCodeFD
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.
Will change this. No objection here. I just re-used current code for quick prototype, but it is obviously not the most straightforward or easy to understand hack.
Reworked based on received feedback. Now env var is used as the mode switch. And now original and altered code paths has clearer separation in code. Also, downloading mechanism is disabled if compatibility mode is used. I performed manual testing and compared the QEMU command lines in both modes. |
@@ -598,36 +599,89 @@ func Cmdline(ctx context.Context, cfg Config) (exe string, args []string, err er | |||
} | |||
if !legacyBIOS { | |||
var firmware string | |||
firmwareInBios := runtime.GOOS == "windows" | |||
if envVar := os.Getenv("LIMA_QEMU_UEFI_IN_BIOS"); envVar != "" { |
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.
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.
Updated
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.
Thanks
Limiting this to X8664 arch as there is no information if this works for other architectures. This allows to load UEFI firmware on Windows hosts, where pflash can't be used in combination with WHPX acceleration. Default to false on non-Windows hosts and true on Windows (to have working default). Signed-off-by: Arthur Sengileyev <[email protected]>
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.
Thanks
Fixes #3176
Limiting this to X8664 arch as there is no information if this works for other architectures. This allows to load UEFI firmware on Windows hosts, where pflash can't be used in combination with WHPX acceleration. Default to false on non-Windows hosts and true on Windows (to have working default).
The FW resolution if set to true and running amd64 VM works like this
Check all downloads, if any matches then consider it concatenated image and use like regular download(downloading through standard mechanisms is removed to prevent potential incompatibilities)PR update to use env var instead of Lima yaml