Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support Big Sur #200

Merged
merged 2 commits into from
Nov 13, 2020
Merged

Support Big Sur #200

merged 2 commits into from
Nov 13, 2020

Conversation

shanesmith
Copy link
Contributor

Based on #192 but with fixes to additional issues I've encountered.

Tested on Big Sur 11.0 Beta 10 with Xcode 12.1 and Xcode 12.2 Beta 4.

@shanesmith
Support Big Sur
3e96363 
Based on machyve#192 but with fixes to additional issues I've encountered.

Tested on Big Sur 11.0 Beta 10 with Xcode 12.1 and Xcode 12.2 Beta 4.
shanesmith
shanesmith commented Nov 6, 2020
View reviewed changes
src/block_if.c
@@ -122,7 +122,7 @@ static struct blockif_sig_elem *blockif_bse_head;
#pragma clang diagnostic pop

static ssize_t
preadv(int fd, const struct iovec *iov, int iovcnt, off_t offset)
_preadv(int fd, const struct iovec *iov, int iovcnt, off_t offset)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When building with Xcode 12.2 Beta 4 it fails with the following error:

/Users/shane/Code/xhyve/src/block_if.c:248:15: error: 'preadv' is only available on macOS 11.0 or newer [-Werror,-Wunguarded-availability-new]
                        if ((len = preadv(bc->bc_fd, br->br_iov, br->br_iovcnt,
                                   ^~~~~~
In module 'Darwin' imported from /Users/shane/Code/xhyve/src/block_if.c:30:
/Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.0.sdk/usr/include/sys/uio.h:103:9: note: 'preadv' has been marked as being introduced in macOS 11.0 here, but the deployment target is macOS 10.10.0
ssize_t preadv(int, const struct iovec *, int, off_t) __DARWIN_NOCANCEL(preadv) __API_AVAILABLE(macos(11.0), ios(14.0), watchos(7.0), tvos(14.0));
        ^
/Users/shane/Code/xhyve/src/block_if.c:248:15: note: enclose 'preadv' in a __builtin_available check to silence this warning
                        if ((len = preadv(bc->bc_fd, br->br_iov, br->br_iovcnt,
                                   ^~~~~~

It seems that preadv has been added in Big Sur. I couldn't find an easy way of using the builtin function in Big Sur and this shim for earlier versions so I just avoided the name collision. Suggestions for improvement welcome!

Similarly for pwritev below.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be fine for now, as it does not regress any existing behavior. However, it would be nice to make use of preadv. I think we can fix this later by checking __MAC_OS_X_VERSION_MAX_ALLOWED, like @jeremyhu suggested in #192.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tried to wrap this function as suggested but I get the same error, it seems like it would want the target SDK to be 11.0+ 🤷🏻‍♂️

shanesmith
shanesmith commented Nov 6, 2020
View reviewed changes
src/vmm/intel/vmx.c Outdated
@@ -90,6 +90,7 @@
PROCBASED2_RDRAND_EXITING | \
PROCBASED2_ENABLE_INVPCID /* FIXME */ | \
PROCBASED2_RDSEED_EXITING | \
PROCBASED2_ENABLE_XSAVES_XRSTORS | \
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As seen in #192 I was having similar vmx_set_ctlreg: cap_field: 2 bit: 20 unspecified don't care issues.

Here and below I followed the example of #174 and disabled the bits that would show up in the errors.

I have no clue if this is a good or bad thing, but it makes it work for me in the end. ^^;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an unfortunate problem to have and I'm not sure this ever growing list of flags is the best way to fix this going forwards. Since this is unrelated to macOS Big Sur support, do you think you could pull this change out of the PR and just land the rest? We can then investigate how to fix this more permanently.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do!

shanesmith
shanesmith commented Nov 6, 2020
View reviewed changes
xhyve.xcodeproj/project.pbxproj Outdated
@@ -717,6 +718,7 @@
isa = XCBuildConfiguration;
baseConfigurationReference = 3F3FF9E31BF7C5D5004C89A1 /* xhyve.xcconfig */;
buildSettings = {
ARCHS = x86_64;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Xcode 12.2 Beta 4 includes arm64 as a default target arch, which xhyve does not seem to support and causes build errors.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think you could put this change in the xhyve.xcconfig?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do!

@shanesmith
Copy link
Contributor Author

Also tested successfully on macOS Catalina 10.15.7

@shanesmith
Copy link
Contributor Author

@jasnee @jeremyhu Pardon the tagging, but since Big Sur got released today I was hoping I could get some eyes on this from someone who knows what they're doing here. =)

@rbishop
Copy link

rbishop commented Nov 13, 2020

Just tested this locally on Big Sur (Late 2013 MBP) and xhyve is back up and running for me. Thanks!

@burke
Copy link
Contributor

burke commented Nov 13, 2020

Great—fixes seem good! 👍

@jasnee
Copy link
Contributor

jasnee commented Nov 13, 2020

Thanks for putting this up @shanesmith and other folks for testing it out! Looks like it builds fine against the macOS 11 and macOS 10.15 SDKs.

As noted above, I think this patch should be ideally limited to fixing the macOS 11 SDK build issues. We can investigate the issues around VMX flags separately.

@shanesmith
Copy link
Contributor Author

Thanks for the quick attention @jasnee! I've moved ARCHS as requested and created #201 for the VMX flag fixes.

@jasnee jasnee merged commit b7e589f into machyve:master Nov 13, 2020
@skeyby
Copy link

skeyby commented Nov 25, 2020

@shanesmith After updating to latest xhyve on Big Sur we're getting hv_vm_create failed on different machines.

We tried different things but couldn't get it to work. Maybe it's because of #201 that is not merged yet?

@shanesmith
Copy link
Contributor Author

@skeyby that's exactly what #201 fixes =)

@cmars cmars mentioned this pull request Jun 18, 2021
Merged
11 tasks
SuperSandro2000 pushed a commit to NixOS/nixpkgs that referenced this pull request Jun 20, 2021
@cmars
xhyve: 20191001 -> 20210203
70ef0a4 
Updating fixes xhyve problems on Big Sur, see
machyve/xhyve#200 for example.
@catap catap mentioned this pull request Aug 17, 2021
Merged
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnee jasnee jasnee left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@shanesmith @rbishop @burke @jasnee @skeyby