From e1517954fbdf53e18f415c96eecef6b02bc4598c Mon Sep 17 00:00:00 2001 From: Sharif Elgamal Date: Mon, 16 May 2022 15:08:35 -0700 Subject: [PATCH 1/7] dynamically determine darwin qemu firmware location --- pkg/drivers/qemu/qemu.go | 17 +++++++------ pkg/minikube/registry/drvs/qemu2/qemu2.go | 29 +++++++++++++++++------ 2 files changed, 30 insertions(+), 16 deletions(-) diff --git a/pkg/drivers/qemu/qemu.go b/pkg/drivers/qemu/qemu.go index ecc1caf5ac9a..26bc62ef3405 100644 --- a/pkg/drivers/qemu/qemu.go +++ b/pkg/drivers/qemu/qemu.go @@ -38,6 +38,7 @@ import ( "github.com/docker/machine/libmachine/mcnutils" "github.com/docker/machine/libmachine/ssh" "github.com/docker/machine/libmachine/state" + "github.com/pkg/errors" pkgdrivers "k8s.io/minikube/pkg/drivers" ) @@ -79,11 +80,9 @@ type Driver struct { DiskPath string CacheMode string IOMode string - // conn *libvirt.Connect - // VM *libvirt.Domain - UserDataFile string - CloudConfigRoot string - LocalPorts string + UserDataFile string + CloudConfigRoot string + LocalPorts string } func (d *Driver) GetMachineName() string { @@ -273,19 +272,19 @@ func parsePortRange(rawPortRange string) (int, int, error) { minPort, err := strconv.Atoi(portRange[0]) if err != nil { - return 0, 0, fmt.Errorf("invalid port range") + return 0, 0, errors.Wrap(err, "Invalid port range") } maxPort, err := strconv.Atoi(portRange[1]) if err != nil { - return 0, 0, fmt.Errorf("invalid port range") + return 0, 0, errors.Wrap(err, "Invalid port range") } if maxPort < minPort { - return 0, 0, fmt.Errorf("invalid port range") + return 0, 0, errors.New("Invalid port range") } if maxPort-minPort < 2 { - return 0, 0, fmt.Errorf("port range must be minimum 2 ports") + return 0, 0, errors.New("Port range must be minimum 2 ports") } return minPort, maxPort, nil diff --git a/pkg/minikube/registry/drvs/qemu2/qemu2.go b/pkg/minikube/registry/drvs/qemu2/qemu2.go index 229f3ba3ccad..b546ad258b06 100644 --- a/pkg/minikube/registry/drvs/qemu2/qemu2.go +++ b/pkg/minikube/registry/drvs/qemu2/qemu2.go @@ -18,8 +18,10 @@ package qemu2 import ( "fmt" + "io/ioutil" "os" "os/exec" + "path" "path/filepath" "runtime" @@ -64,17 +66,30 @@ func qemuSystemProgram() (string, error) { func qemuFirmwarePath() (string, error) { arch := runtime.GOARCH + // For macOS, find the correct brew installation path for qemu firmware + if runtime.GOOS == "darwin" { + //return "/opt/homebrew/Cellar/qemu/6.2.0_1/share/qemu/edk2-aarch64-code.fd", nil + p := "/usr/local/Cellar/qemu" + fw := "share/qemu/edk2-x86_64-code.fd" + if arch == "arm64" { + p = "/opt/homebrew/Cellar/qemu" + fw = "share/qemu/edk2-aarch64-code.fd" + } + + v, err := ioutil.ReadDir(p) + if err != nil { + return "", fmt.Errorf("lookup qemu: %v", err) + } + for _, version := range v { + if version.IsDir() { + return path.Join(p, version.Name(), fw), nil + } + } + } switch arch { case "amd64": - // on macOS, we assume qemu is installed via homebrew for simplicity - if runtime.GOOS == "darwin" { - return "/usr/local/Cellar/qemu/6.2.0_1/share/qemu/edk2-x86_64-code.fd", nil - } return "/usr/share/OVMF/OVMF_CODE.fd", nil case "arm64": - if runtime.GOOS == "darwin" { - return "/opt/homebrew/Cellar/qemu/6.2.0_1/share/qemu/edk2-aarch64-code.fd", nil - } return "/usr/share/AAVMF/AAVMF_CODE.fd", nil default: return "", fmt.Errorf("unknown arch: %s", arch) From f99096fbcc2498b7b10e5ea361ed4e7df8c0385b Mon Sep 17 00:00:00 2001 From: Sharif Elgamal Date: Mon, 16 May 2022 15:32:24 -0700 Subject: [PATCH 2/7] support hardware accel for more machine types --- pkg/drivers/qemu/qemu.go | 9 ++++++++- pkg/minikube/registry/drvs/qemu2/qemu2.go | 2 +- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/pkg/drivers/qemu/qemu.go b/pkg/drivers/qemu/qemu.go index 26bc62ef3405..aaf266f5d422 100644 --- a/pkg/drivers/qemu/qemu.go +++ b/pkg/drivers/qemu/qemu.go @@ -339,7 +339,7 @@ func (d *Driver) Start() error { machineType := d.MachineType if runtime.GOOS == "darwin" { // highmem=off needed, see https://patchwork.kernel.org/project/qemu-devel/patch/20201126215017.41156-9-agraf@csgraf.de/#23800615 for details - machineType += ",accel=hvf,highmem=off" + machineType += ",highmem=off" } startCmd = append(startCmd, "-M", machineType, @@ -379,6 +379,13 @@ func (d *Driver) Start() error { } } + // hardware acceleration is important, it increases performance by 10x + if runtime.GOOS == "darwin" { + startCmd = append(startCmd, "-accel", "hvf") + } else if runtime.GOOS == "linux" { + startCmd = append(startCmd, "-accel", "kvm") + } + startCmd = append(startCmd, "-m", fmt.Sprintf("%d", d.Memory), "-smp", fmt.Sprintf("%d", d.CPU), diff --git a/pkg/minikube/registry/drvs/qemu2/qemu2.go b/pkg/minikube/registry/drvs/qemu2/qemu2.go index b546ad258b06..aa721cae35b1 100644 --- a/pkg/minikube/registry/drvs/qemu2/qemu2.go +++ b/pkg/minikube/registry/drvs/qemu2/qemu2.go @@ -68,7 +68,6 @@ func qemuFirmwarePath() (string, error) { arch := runtime.GOARCH // For macOS, find the correct brew installation path for qemu firmware if runtime.GOOS == "darwin" { - //return "/opt/homebrew/Cellar/qemu/6.2.0_1/share/qemu/edk2-aarch64-code.fd", nil p := "/usr/local/Cellar/qemu" fw := "share/qemu/edk2-x86_64-code.fd" if arch == "arm64" { @@ -86,6 +85,7 @@ func qemuFirmwarePath() (string, error) { } } } + switch arch { case "amd64": return "/usr/share/OVMF/OVMF_CODE.fd", nil From 6f32c7e742947d16cc37c4579fd6bf96bd60cfe4 Mon Sep 17 00:00:00 2001 From: Sharif Elgamal Date: Mon, 16 May 2022 16:01:58 -0700 Subject: [PATCH 3/7] comment out kvm stuff for now --- pkg/drivers/qemu/qemu.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/pkg/drivers/qemu/qemu.go b/pkg/drivers/qemu/qemu.go index aaf266f5d422..c974e304169b 100644 --- a/pkg/drivers/qemu/qemu.go +++ b/pkg/drivers/qemu/qemu.go @@ -380,10 +380,10 @@ func (d *Driver) Start() error { } // hardware acceleration is important, it increases performance by 10x + // kvm acceleration doesn't currently work for linux, it's incompatible with our chosen CPU + // once that's fixed we should add a branch for linux if runtime.GOOS == "darwin" { startCmd = append(startCmd, "-accel", "hvf") - } else if runtime.GOOS == "linux" { - startCmd = append(startCmd, "-accel", "kvm") } startCmd = append(startCmd, @@ -427,9 +427,10 @@ func (d *Driver) Start() error { // other options // "-enable-kvm" if its available - if _, err := os.Stat("/dev/kvm"); err == nil { + // TODO (#14171): re-enable this once kvm acceleration is fixed + /*if _, err := os.Stat("/dev/kvm"); err == nil { startCmd = append(startCmd, "-enable-kvm") - } + }*/ if d.CloudConfigRoot != "" { startCmd = append(startCmd, From 823ea12bc25a99caa5fbcffccb402aad3ad404dc Mon Sep 17 00:00:00 2001 From: Sharif Elgamal Date: Mon, 16 May 2022 23:12:39 -0700 Subject: [PATCH 4/7] let's try kvm accel again --- pkg/drivers/qemu/qemu.go | 11 ++--------- pkg/minikube/registry/drvs/qemu2/qemu2.go | 20 +++++++++++++++----- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/pkg/drivers/qemu/qemu.go b/pkg/drivers/qemu/qemu.go index c974e304169b..2a88e0782f98 100644 --- a/pkg/drivers/qemu/qemu.go +++ b/pkg/drivers/qemu/qemu.go @@ -380,10 +380,10 @@ func (d *Driver) Start() error { } // hardware acceleration is important, it increases performance by 10x - // kvm acceleration doesn't currently work for linux, it's incompatible with our chosen CPU - // once that's fixed we should add a branch for linux if runtime.GOOS == "darwin" { startCmd = append(startCmd, "-accel", "hvf") + } else if runtime.GOOS == "linux" { + startCmd = append(startCmd, "-accel", "kvm") } startCmd = append(startCmd, @@ -425,13 +425,6 @@ func (d *Driver) Start() error { startCmd = append(startCmd, "-daemonize") - // other options - // "-enable-kvm" if its available - // TODO (#14171): re-enable this once kvm acceleration is fixed - /*if _, err := os.Stat("/dev/kvm"); err == nil { - startCmd = append(startCmd, "-enable-kvm") - }*/ - if d.CloudConfigRoot != "" { startCmd = append(startCmd, "-fsdev", diff --git a/pkg/minikube/registry/drvs/qemu2/qemu2.go b/pkg/minikube/registry/drvs/qemu2/qemu2.go index aa721cae35b1..21ce1748d080 100644 --- a/pkg/minikube/registry/drvs/qemu2/qemu2.go +++ b/pkg/minikube/registry/drvs/qemu2/qemu2.go @@ -68,11 +68,16 @@ func qemuFirmwarePath() (string, error) { arch := runtime.GOARCH // For macOS, find the correct brew installation path for qemu firmware if runtime.GOOS == "darwin" { - p := "/usr/local/Cellar/qemu" - fw := "share/qemu/edk2-x86_64-code.fd" - if arch == "arm64" { + var p, fw string + switch arch { + case "amd64": + p = "/usr/local/Cellar/qemu" + fw = "share/qemu/edk2-x86_64-code.fd" + case "arm64": p = "/opt/homebrew/Cellar/qemu" fw = "share/qemu/edk2-aarch64-code.fd" + default: + return "", fmt.Errorf("unknown arch: %s", arch) } v, err := ioutil.ReadDir(p) @@ -109,8 +114,13 @@ func configure(cc config.ClusterConfig, n config.Node) (interface{}, error) { qemuMachine = "" // default qemuCPU = "" // default case "arm64": - qemuMachine = "virt" - qemuCPU = "cortex-a72" + if runtime.GOOS == "darwin" { + qemuMachine = "virt" + qemuCPU = "cortex-a72" + } else if runtime.GOOS == "linux" { + qemuMachine = "gic-version=3" + qemuCPU = "host" + } default: return nil, fmt.Errorf("unknown arch: %s", runtime.GOARCH) } From 41150243208cd7a82360d8ec3abf791cc6229098 Mon Sep 17 00:00:00 2001 From: Sharif Elgamal Date: Mon, 16 May 2022 23:21:11 -0700 Subject: [PATCH 5/7] virt is still the machine type, duh --- pkg/drivers/qemu/qemu.go | 4 ---- pkg/minikube/registry/drvs/qemu2/qemu2.go | 5 +++-- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/pkg/drivers/qemu/qemu.go b/pkg/drivers/qemu/qemu.go index 2a88e0782f98..a444290af949 100644 --- a/pkg/drivers/qemu/qemu.go +++ b/pkg/drivers/qemu/qemu.go @@ -337,10 +337,6 @@ func (d *Driver) Start() error { if d.MachineType != "" { machineType := d.MachineType - if runtime.GOOS == "darwin" { - // highmem=off needed, see https://patchwork.kernel.org/project/qemu-devel/patch/20201126215017.41156-9-agraf@csgraf.de/#23800615 for details - machineType += ",highmem=off" - } startCmd = append(startCmd, "-M", machineType, ) diff --git a/pkg/minikube/registry/drvs/qemu2/qemu2.go b/pkg/minikube/registry/drvs/qemu2/qemu2.go index 21ce1748d080..ed0cb55720a4 100644 --- a/pkg/minikube/registry/drvs/qemu2/qemu2.go +++ b/pkg/minikube/registry/drvs/qemu2/qemu2.go @@ -114,11 +114,12 @@ func configure(cc config.ClusterConfig, n config.Node) (interface{}, error) { qemuMachine = "" // default qemuCPU = "" // default case "arm64": + // highmem=off needed, see https://patchwork.kernel.org/project/qemu-devel/patch/20201126215017.41156-9-agraf@csgraf.de/#23800615 for details if runtime.GOOS == "darwin" { - qemuMachine = "virt" + qemuMachine = "virt,highmem=off" qemuCPU = "cortex-a72" } else if runtime.GOOS == "linux" { - qemuMachine = "gic-version=3" + qemuMachine = "virt,gic-version=3" qemuCPU = "host" } default: From 177d2b98088cd44dc7c832539ff008b8ab279329 Mon Sep 17 00:00:00 2001 From: Sharif Elgamal Date: Tue, 17 May 2022 10:13:47 -0700 Subject: [PATCH 6/7] sanity check port range --- pkg/drivers/qemu/qemu.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/drivers/qemu/qemu.go b/pkg/drivers/qemu/qemu.go index a444290af949..b5e758c76dd8 100644 --- a/pkg/drivers/qemu/qemu.go +++ b/pkg/drivers/qemu/qemu.go @@ -270,6 +270,10 @@ func parsePortRange(rawPortRange string) (int, int, error) { portRange := strings.Split(rawPortRange, "-") + if len(portRange) < 2 { + return 0, 0, errors.New("Invalid port range, must be at least of length 2") + } + minPort, err := strconv.Atoi(portRange[0]) if err != nil { return 0, 0, errors.Wrap(err, "Invalid port range") @@ -378,7 +382,7 @@ func (d *Driver) Start() error { // hardware acceleration is important, it increases performance by 10x if runtime.GOOS == "darwin" { startCmd = append(startCmd, "-accel", "hvf") - } else if runtime.GOOS == "linux" { + } else if _, err := os.Stat("/dev/kvm"); err == nil && runtime.GOOS == "linux" { startCmd = append(startCmd, "-accel", "kvm") } From c9c8b3105f32e77776b839bbe4b1940eeb3fe449 Mon Sep 17 00:00:00 2001 From: Sharif Elgamal Date: Tue, 17 May 2022 10:49:30 -0700 Subject: [PATCH 7/7] only set cpu to host if kvm is present --- pkg/minikube/registry/drvs/qemu2/qemu2.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/minikube/registry/drvs/qemu2/qemu2.go b/pkg/minikube/registry/drvs/qemu2/qemu2.go index ed0cb55720a4..5cd70e58f1e3 100644 --- a/pkg/minikube/registry/drvs/qemu2/qemu2.go +++ b/pkg/minikube/registry/drvs/qemu2/qemu2.go @@ -114,11 +114,12 @@ func configure(cc config.ClusterConfig, n config.Node) (interface{}, error) { qemuMachine = "" // default qemuCPU = "" // default case "arm64": + qemuMachine = "virt" + qemuCPU = "cortex-a72" // highmem=off needed, see https://patchwork.kernel.org/project/qemu-devel/patch/20201126215017.41156-9-agraf@csgraf.de/#23800615 for details if runtime.GOOS == "darwin" { qemuMachine = "virt,highmem=off" - qemuCPU = "cortex-a72" - } else if runtime.GOOS == "linux" { + } else if _, err := os.Stat("/dev/kvm"); err == nil { qemuMachine = "virt,gic-version=3" qemuCPU = "host" }