Support Rosetta with VZ driver #1155

chancez · 2022-11-15T00:17:38Z

Useful reading: https://developer.apple.com/documentation/virtualization/running_intel_binaries_in_linux_vms_with_rosetta

Implementation based off the sample here: https://github.com/Code-Hex/vz/tree/9de1c1041d5e560f46f9f94fc499a1cc72fd92e5/example/gui-linux

examples/experimental/vz.yaml

pkg/cidata/cidata.TEMPLATE.d/boot/05-rosetta-volume.sh

cedws · 2022-11-16T09:09:03Z

I don't have any comments on the changes other than nice work and LGTM, but do you have any suggested reading material on how this Rosetta acceleration works?

Thanks.

pkg/cidata/cidata.TEMPLATE.d/boot/05-rosetta-volume.sh

chancez · 2022-11-16T18:29:40Z

Okay, PR is updated based on feedback and I like it much better than the initial MVP. Let me know what you all think. I also rebased and moved it from draft->ready to review.

chancez · 2022-11-16T19:21:54Z

@cedws

I don't have any comments on the changes other than nice work and LGTM, but do you have any suggested reading material on how this Rosetta acceleration works?

Thanks.

For how it works with Linux: https://developer.apple.com/documentation/virtualization/running_intel_binaries_in_linux_vms_with_rosetta

Rosetta in general: https://developer.apple.com/documentation/apple-silicon/about-the-rosetta-translation-environment

pkg/limayaml/limayaml.go

balajiv113 · 2022-11-16T19:07:11Z

pkg/vz/rosetta_directory_share.go

+)
+
+func createRosettaDirectoryShareConfiguration(install bool) (*vz.VirtioFileSystemDeviceConfiguration, error) {
+	return nil, fmt.Errorf("Rosetta is unsupported on non-ARM64 hosts")


We should do a warning instead of error.

This is to make vz template runnable on both arm & x64

~~Couldn't users just disable Rosetta on x86 in that case?~~ I guess this would be better UX for users so they don't have to worry about it.

I also think in that case, we probably can still return an error here, but we can turn it into a named error and check using errors.Is in the caller.

balajiv113 · 2022-11-16T19:11:12Z

pkg/vz/vm_darwin.go

+
+	if driver.Yaml.Rosetta.Enabled {
+		logrus.Info("Setting up Rosetta share")
+		directorySharingDeviceConfig, err := createRosettaDirectoryShareConfiguration(driver.Yaml.Rosetta.Install)


Just a thought,

Do we needed Rosetta.Install ?
Can we directly install since Rosetta.Enabled is true already ?

I'm not sure. I figured it would make sense to make it configurable in case users want to install Rosetta themselves. I already have it installed, so I'm not sure what it actually looks like if you try without it.

During installation nothing will pop-up or happen. It just like installation via command line. So i think it should be fine.

But let's also hear others opinion on this.

Can we directly install since Rosetta.Enabled is true already ?

👍 , assuming that Enabled = true doesn't show the macOS password popup as long as Rosetta is already installed on the macOS host.

If my assumption is untrue, probably we can't avoid having Install property, but it has to be documented in vz.yaml.

@AkihiroSuda Your assumption is true. Only for installation the password pop-up will be shown. Not for every run of rosetta.

Ok so we want to remove the install configuration altogether. I'll rip that out.

chancez · 2022-11-16T20:31:46Z

There was also some discussion about whether or not this should be enabled by default in the other PR. What do people think?

AkihiroSuda · 2022-11-17T00:16:10Z

There was also some discussion about whether or not this should be enabled by default in the other PR. What do people think?

The mount can be enabled by default, but binfmt should be probably opt-in for now.
The vz.yaml template can have rosetta.binfmt: true by default.

chancez · 2022-11-17T01:02:53Z

There was also some discussion about whether or not this should be enabled by default in the other PR. What do people think?

The mount can be enabled by default, but binfmt should be probably opt-in for now. The vz.yaml template can have rosetta.binfmt: true by default.

Is that best done via FillDefault or should we change the field to Disable so the zero value is that Rosetta is enabled? (Disable: false)

AkihiroSuda · 2022-11-18T00:24:22Z

There was also some discussion about whether or not this should be enabled by default in the other PR. What do people think?

The mount can be enabled by default, but binfmt should be probably opt-in for now. The vz.yaml template can have rosetta.binfmt: true by default.

Is that best done via FillDefault or should we change the field to Disable so the zero value is that Rosetta is enabled? (Disable: false)

Lima itself should leave it disabled by default. The vz.yaml template file may have rosetta.binfmt: true line to override the built-in default value.

chancez · 2022-11-18T17:45:32Z

@AkihiroSuda Sorry I meant for rosetta.enabled.

Also what are your thoughts about the Rosetta installation behavior?

AkihiroSuda · 2022-11-20T06:43:54Z

Looks like installing rosetta requires the root of the macOS host, so it should be disabled by default.

https://developer.apple.com/documentation/virtualization/running_intel_binaries_in_linux_vms_with_rosetta?language=objc

Installing Rosetta is a one-time process per computer that requires the user to grant permission for the system to install Rosetta. If Rosetta is already installed in macOS, the system activates it for use under the Virtualization framework; if Rosetta isn’t installed, the framework downloads the installer from the network and performs the installation. The installer interactively prompts the user for authorization, and your app should handle any of the possible error conditions that could occur during the authorization, download, and installation process.

chancez · 2022-11-21T20:13:43Z

@AkihiroSuda then it sounds like the PR as it is should be correct with the current defaults and vz.yaml. I've rebased.

chancez · 2022-11-22T00:31:39Z

Getting a build error on my errRosettaUnsupported variable from being used, presumably because it's running without the build tags, but I'm not sure quite where/how to fix it currently.

AkihiroSuda · 2022-11-22T03:10:30Z

Thanks, almost looks good, but while Rosetta should be disabled by default, I assume it can be automatically installed to the macOS host when Rosetta is explicitly enabled in the YAML
#1155 (comment)

chancez · 2022-11-22T16:32:49Z

PR is updated to remove the install configuration.

AkihiroSuda

Thanks

balajiv113 · 2022-11-22T18:32:26Z

pkg/vz/rosetta_directory_share.go

@@ -0,0 +1,12 @@
+//go:build darwin && !arm64
+// +build darwin,!arm64


I believe we need to use !no_vz buildtag in both rosetta files to avoid this issue
#1176

balajiv113 · 2022-11-22T18:41:28Z

pkg/vz/errors.go

@@ -0,0 +1,8 @@
+//go:build darwin
+// +build darwin


Do we need this tags ??
errors.go can be generic irrespective of platform (We can move this https://github.com/lima-vm/lima/blob/master/pkg/vz/vz_driver_others.go#L13 as well here)

If we need this tag, then naming the file as errors_darwin.go should be sufficient and consistent across i believe

Without build tags the variable is detected as unused in non-darwin builds.

Renamed the file, and I updated build tags as more were required due to your previous comment: #1155 (comment)

balajiv113 · 2022-11-22T18:49:13Z

Apart from those minor comments. Changes LGTM.
Works great, tested in M1 👍 And throws warning as well for intel :)

I couldn't test install rosetta flow, as i already had it installed.

AkihiroSuda · 2022-11-22T19:32:42Z

pkg/vz/vm_darwin.go

+		if errors.Is(err, errRosettaUnsupported) {
+			logrus.Warnf("Unable to configure Rosetta: %s", err)
+		} else if err != nil {
+			return err


Probably, any error from createRosettaDirectoryShareConfiguration can be a warning.
e.g., failure of installation of Rosetta

I'm still unsure of this. Is there other example cases where Lima fails to do setup and continues on anyways? It seems like continuing on would lead to unexpected results for users who may not notice the warnings, etc.

Is there other example cases where Lima fails to do setup and continues on anyways?

Yes, in a bunch of places (git grep -F .Warn)

e.g.,

lima/pkg/start/start.go

Line 213 in 32baa62

logrus.Warnf("DEGRADED. The VM seems running, but file sharing and port forwarding may not work. (hint: see %q)", haStderrPath)

Gotcha. I've updated it to warn instead.

Signed-off-by: Chance Zibolski <chance.zibolski@gmail.com>

AkihiroSuda

Thanks

AkihiroSuda · 2022-11-23T12:01:41Z

Released beta.1 with Rosetta
https://github.com/lima-vm/lima/releases/tag/v0.14.0-beta.1

AkihiroSuda added platform/ARM component/vz labels Nov 15, 2022

AkihiroSuda reviewed Nov 16, 2022

View reviewed changes

examples/experimental/vz.yaml Outdated Show resolved Hide resolved

AkihiroSuda reviewed Nov 16, 2022

View reviewed changes

pkg/cidata/cidata.TEMPLATE.d/boot/05-rosetta-volume.sh Show resolved Hide resolved

AkihiroSuda reviewed Nov 16, 2022

View reviewed changes

pkg/cidata/cidata.TEMPLATE.d/boot/05-rosetta-volume.sh Outdated Show resolved Hide resolved

jandubois linked an issue Nov 16, 2022 that may be closed by this pull request

Use Rosetta2 as a binfmt_misc handler on macOS Ventura #1088

Closed

balajiv113 reviewed Nov 16, 2022

View reviewed changes

pkg/cidata/cidata.TEMPLATE.d/boot/05-rosetta-volume.sh Outdated Show resolved Hide resolved

chancez force-pushed the pr/chancez/vz_rosetta branch from 6f06b4a to 0d68736 Compare November 16, 2022 18:22

chancez marked this pull request as ready for review November 16, 2022 18:23

chancez force-pushed the pr/chancez/vz_rosetta branch from 0d68736 to 09d71fd Compare November 16, 2022 18:31

balajiv113 reviewed Nov 16, 2022

View reviewed changes

chancez force-pushed the pr/chancez/vz_rosetta branch 2 times, most recently from b53ba06 to badcb22 Compare November 16, 2022 20:42

chancez force-pushed the pr/chancez/vz_rosetta branch 2 times, most recently from 7cc9272 to 9180ce9 Compare November 21, 2022 22:49

chancez force-pushed the pr/chancez/vz_rosetta branch 2 times, most recently from 5fec1fc to 674c567 Compare November 22, 2022 01:08

AkihiroSuda added this to the v0.14 milestone Nov 22, 2022

chancez force-pushed the pr/chancez/vz_rosetta branch from 674c567 to 5162446 Compare November 22, 2022 16:31

chancez requested review from AkihiroSuda and balajiv113 and removed request for AkihiroSuda and balajiv113 November 22, 2022 16:32

AkihiroSuda previously approved these changes Nov 22, 2022

View reviewed changes

balajiv113 reviewed Nov 22, 2022

View reviewed changes

AkihiroSuda reviewed Nov 22, 2022

View reviewed changes

chancez dismissed AkihiroSuda’s stale review via ad4de31 November 22, 2022 21:44

chancez force-pushed the pr/chancez/vz_rosetta branch from 5162446 to ad4de31 Compare November 22, 2022 21:44

chancez added 2 commits November 22, 2022 14:13

Add support for Rosetta with vmType: vz

7638316

Signed-off-by: Chance Zibolski <chance.zibolski@gmail.com>

vz: Warn on rosetta setup if it fails

6bb3078

Signed-off-by: Chance Zibolski <chance.zibolski@gmail.com>

chancez force-pushed the pr/chancez/vz_rosetta branch from ad4de31 to 6bb3078 Compare November 22, 2022 22:13

chancez requested review from balajiv113 and AkihiroSuda and removed request for balajiv113 and AkihiroSuda November 22, 2022 22:34

AkihiroSuda approved these changes Nov 23, 2022

View reviewed changes

AkihiroSuda merged commit 69d032b into lima-vm:master Nov 23, 2022

chancez deleted the pr/chancez/vz_rosetta branch November 23, 2022 16:29

misuzu mentioned this pull request Jan 25, 2023

Allowing NixOS VM's to be run on macOS NixOS/nixpkgs#108984

Closed

		@@ -0,0 +1,12 @@
		//go:build darwin && !arm64
		// +build darwin,!arm64

Support Rosetta with VZ driver #1155

Support Rosetta with VZ driver #1155

Conversation

chancez commented Nov 15, 2022 • edited Loading

cedws commented Nov 16, 2022

chancez commented Nov 16, 2022

chancez commented Nov 16, 2022

Choose a reason for hiding this comment

chancez Nov 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chancez commented Nov 16, 2022

AkihiroSuda commented Nov 17, 2022

chancez commented Nov 17, 2022

AkihiroSuda commented Nov 18, 2022

chancez commented Nov 18, 2022

AkihiroSuda commented Nov 20, 2022

chancez commented Nov 21, 2022

chancez commented Nov 22, 2022 • edited Loading

AkihiroSuda commented Nov 22, 2022

chancez commented Nov 22, 2022

AkihiroSuda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balajiv113 commented Nov 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda left a comment

Choose a reason for hiding this comment

AkihiroSuda commented Nov 23, 2022

chancez commented Nov 15, 2022 •

edited

Loading

chancez Nov 16, 2022 •

edited

Loading

AkihiroSuda Nov 22, 2022 •

edited

Loading

chancez commented Nov 22, 2022 •

edited

Loading