-
Notifications
You must be signed in to change notification settings - Fork 611
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
[Test wanted] qemu: adjust the memory size for workarounding M1 panic #796
Conversation
QEMU 7.0 is planned to be released today (https://wiki.qemu.org/Planning/7.0), so I'd like to merge this one and release Lima v0.10 cc @jandubois Probably, Rancher Desktop should continue bundling QEMU 6.2 |
pkg/qemu/qemu.go
Outdated
if !macOSProductVersion.LessThan(*semver.New("12.4.0")) { | ||
return memBytes | ||
} | ||
const safeSize = 1 << 31 // 2GiB |
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.
Why is the safeSize
2GiB and not e.g. 4GiB - 1 or something quite close to 4GiB?
If you ask for 3GiB, you get 3Gib, but if you ask for 5GiB you only get 2GiB, which seems weird. Shouldn't the default be the same as the highest value that we pass through untouched? If 4GiB - 1
doesn't work, then we should not allow that either.
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.
We convert the number into MiB anyways before passing it to qemu, so technically our limit would be 4095MiB.
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.
Because I suppose an "odd" memory size might not be "safe" under some conditions.
(I don't have M1 Pro/Max to confirm)
The user can still specify an odd memory size.
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.
Because I suppose an "odd" memory size might not be "safe" under some conditions.
This sounds like a weird reason, as any number of things might not be "safe" either.
I could see the argument that we should change the default value to 2GiB if the user doesn't specify anything. But if the user specifies 3GiB, they get 3GiB, however, if they specify 4GiB, they only get 2GiB, that just feels wrong to me.
Since this is just a temporary workaround under limited conditions, I won't argue this too much, but ideally we should confirm if 4095MiB is safe or not, and use that if it doesn't show the issue.
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.
Fixed to 4095 MiB.
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 have not tested the code because I only have access to a single M1 machine right now, and don't want to change the installed version of QEMU.
Except for the safeSize
value, everything else looks good to me.
I'll take some time to test it tonight. |
Test Case 1:
Work successfully on 8 GiB, 4 GiB, 2 GiB.
|
Test Case 2:
Work successfully on 2 GiB. But 4GiB/8GiB directly caused the system to crash. masOS error report:panic(cpu 3 caller 0xfffffe002397d814): vm_fault() KERN_FAILURE from guest fault on state 0xfffffe60fac98000 @sleh.c:3117 Debugger message: panic Memory ID: 0x6 OS release type: User OS version: 21E258 Kernel version: Darwin Kernel Version 21.4.0: Fri Mar 18 00:46:32 PDT 2022; root:xnu-8020.101.4~15/RELEASE_ARM64_T6000 Fileset Kernelcache UUID: 0631AF68D2B8D6FEA30E36D7895D4DB4 Kernel UUID: C342869F-FFB9-3CCE-A5A3-EA711C1E87F6 iBoot version: iBoot-7459.101.3 secure boot?: YES Paniclog version: 13 KernelCache slide: 0x000000001bf50000 KernelCache base: 0xfffffe0022f54000 Kernel slide: 0x000000001c700000 Kernel text base: 0xfffffe0023704000 Kernel text exec slide: 0x000000001c7e8000 Kernel text exec base: 0xfffffe00237ec000 mach_absolute_time: 0x182facfa3 Epoch Time: sec usec Boot : 0x62602044 0x0008735f Sleep : 0x00000000 0x00000000 Wake : 0x00000000 0x00000000 Calendar: 0x6260214b 0x00074135Zone info: CORE 0 PVH locks held: None last started kext at 1769942507: com.apple.driver.driverkit.serial 6.0.0 (addr 0xfffffe00235df320, size 3416) ** Stackshot Succeeded ** Bytes Traced 356611 (Uncompressed 940800) ** |
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.
Failed to get the MacOS version without handling newlines.
pkg/qemu/qemu.go
Outdated
if err != nil { | ||
return nil, fmt.Errorf("failed to execute %v: %w", cmd.Args, err) | ||
} | ||
ver, err := semver.NewVersion(string(b)) |
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.
Need remove trailing newline from string.
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, fixed
Signed-off-by: Akihiro Suda <[email protected]>
@junnplus Thanks a lot for testing. Could you try the latest revision? |
@AkihiroSuda There is no problem with the macOS version parsing, but qemu cannot be started. 4GiB:
|
3GiB, 2.9GiB passes, neither 3.9GiB nor 3.5GiB, 3.1GiB work on QEMU 7.0. |
Thanks, adjusted the |
pkg/qemu/qemu.go
Outdated
// This adjustment is required for avoiding host kernel panic. The issue was fixed in macOS 12.4 Beta 1. | ||
// See https://github.com/lima-vm/lima/issues/795 https://gitlab.com/qemu-project/qemu/-/issues/903#note_911000975 | ||
func adjustMemBytesDarwinARM64HVF(memBytes int64, accel string, features *features) int64 { | ||
if memBytes < (1 << 32) { // less than 4GiB |
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.
Doesn't this one have to be adjusted to 3GiB as well?
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.
adjusted to 3 GiB
pkg/qemu/qemu.go
Outdated
@@ -285,7 +356,8 @@ func Cmdline(cfg Config) (string, []string, error) { | |||
// QEMU < 7.0 requires highmem=off to be set, otherwise fails with "VCPU supports less PA bits (36) than requested by the memory map (40)" | |||
// https://github.com/lima-vm/lima/issues/680 | |||
// https://github.com/lima-vm/lima/pull/24 | |||
if !strings.Contains(string(features.MachineHelp), "virt-7.0") { | |||
// But when the memory size is less than 4 GiB, we can always set highmem=off. | |||
if !features.VersionGEQ7 || memBytes < (1<<32) { |
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.
And I wonder if this one is correct too; I couldn't find a definitive source, but I've seen comments that highmem=off
limits memory on ARM to 3GiB.
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.
adjusted to 3 GiB
@AkihiroSuda Works fine after adjusted to 3GiB. Tested on 3b32553 |
Thank you! 👍 |
Adjust the memory to be <= 3 GiB, only when the following conditions are met: - Host OS < macOS 12.4 - Host Arch == arm64 - Accel == hvf - QEMU >= 7.0 This adjustment is required for avoiding host kernel panic. The issue was fixed in macOS 12.4 Beta 1. See https://gitlab.com/qemu-project/qemu/-/issues/903#note_911000975 Signed-off-by: Akihiro Suda <[email protected]>
Pushed a cosmetic change from 3b32553. $ git diff 3b32553 130b300
diff --git a/pkg/qemu/qemu.go b/pkg/qemu/qemu.go
index 77b7d7d..13e8218 100644
--- a/pkg/qemu/qemu.go
+++ b/pkg/qemu/qemu.go
@@ -262,9 +262,8 @@ func getMacOSProductVersion() (*semver.Version, error) {
return verSem, nil
}
-// adjustMemBytesDarwinARM64HVF adjusts the memory to be less than 3 GiB, only when the following conditions are met:
+// adjustMemBytesDarwinARM64HVF adjusts the memory to be <= 3 GiB, only when the following conditions are met:
//
-// - memBytes > 3 GiB
// - Host OS < macOS 12.4
// - Host Arch == arm64
// - Accel == hvf |
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.
Closes #795
Closes #713
Adjust the memory to be <= 3 GiB, only when all the following conditions are met:
This adjustment is required for avoiding host kernel panic. The issue was fixed in macOS 12.4 Beta 1.
See https://gitlab.com/qemu-project/qemu/-/issues/903#note_911000975
Help wanted: testing on M1 (especially M1 Max or Pro)
MacBookPro 2021, M1 Pro
): ...12.3.1
): ...QEMU 7.0.0 can be installed by running
brew install --build-from-source ./qemu.rb
withqemu.rb
from https://github.com/Homebrew/homebrew-core/pull/99652/files