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.

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

Adding FreeBSD on ARM64 support #474

Closed
wants to merge 5 commits into from
Closed

Adding FreeBSD on ARM64 support #474

wants to merge 5 commits into from

Conversation

darkain
Copy link

@darkain darkain commented Oct 15, 2020

This adds support to open-vm-tools for FreeBSD on aarch64 / ARM64 - tested on FreeBSD 12.2-RC2

Some of these ARM64 edits may also apply to different OSes.

These edits are also based around llvm/clang, so should go through the regression testing on i386 and amd64 as well on all OSes (I'm personally not setup to do all of these tests)

@vmwclabot
Copy link
Member

vmwclabot commented Oct 15, 2020

@darkain, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

@vmwclabot
Copy link
Member

vmwclabot commented Oct 15, 2020

@darkain, VMware has approved your signed contributor license agreement.

Copy link

@claplace-vmw claplace-vmw left a comment

Thank you for the contribution!
If you don't mind I'd like to have 2 small changes.

@@ -139,7 +139,11 @@ static INLINE int EscBitVector_Test(EscBitVector const *bv, int n)
ASSERT(n>=0 && n<ESC_BITVECTOR_SIZE);
#ifdef __GNUC__
{
#ifdef VM_ARM_64

Choose a reason for hiding this comment

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

given the x86 assembly code below, I think we'd better gate that part under VM_X86_ANY and let the arm code fall back to use the C part.

Copy link
Author

@darkain darkain Oct 16, 2020

Choose a reason for hiding this comment

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

I've done some more digging into this section of code. Only one file makes reference to this header, and the code that actually used this... was removed in 2008.

This appears to be a dead file not even doing anything other than throwing compiler errors at me, that I put that hackish #ifdef into to avoid. I think it would be best to just remove it at this point.

ef4a8b2#diff-5066e7d95c24c4e592c9eb6e82d61655014eee22eed34c379fa4c1957cbbf387

@@ -147,7 +154,7 @@ main(int argc, // IN
thisProgramBase = thisProgram;
}

while ((c = getopt(argc, argv, "hvV")) != -1) {
while ((c = getopt(argc, argv, "hvV")) != GETOPT_END) {

Choose a reason for hiding this comment

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

can you define c as int instead?

Copy link
Author

@darkain darkain Oct 15, 2020

Choose a reason for hiding this comment

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

I had assumed that getopt return was of type "char" since it was dealing with character data. But looking further into it, both Linux and FreeBSD define it as "int", so yup, that would be the better code path here for language/OS compatibility.

@johnwvmw
Copy link
Contributor

johnwvmw commented Oct 15, 2020

@darkain
I too would like to thank you for your contribution and signed contributor license agreement.

An internal PR has been filed to track the pull request through the review process. I will coordinate with @claplace-vmw to adjust the suggested changes where necessary to work across other projects that may use common source files.

I will also work with the open-vm-tools maintainer to integrate the "final" patch into the emulators/open-vm-tools project in /usr/ports.

Copy link

@claplace-vmw claplace-vmw left a comment

Thanks @darkain !

@darkain
Copy link
Author

darkain commented Oct 16, 2020

I've now validated that removing the bitvector code entirely compiles and runs just fine.

@johnwvmw
Copy link
Contributor

johnwvmw commented Feb 10, 2021

@darkain

As we are working to integrate you ARM changes for FreeBSD open-vm-tools into the next major release, it was noticed that the proposed asm change to modules/freebsd/vmmemctl/os.c is not correct. The os_ffz() function is to locate the first "low-order" zero bit. The ARM asm has been changed to :

#elif defined(aarch64)
asm("rbit %0, %1; clz %0,%0"
:"=r" (word)
:"r" (~word));

@darkain
Copy link
Author

darkain commented Feb 11, 2021

As long as things are getting fixed up, that's awesome :)

johnwvmw added a commit that referenced this pull request Feb 22, 2021
Updating the FreeBSD specific sections of open-vm-tools to adjust
where necessary for ARM64.   The FreeBSD vmballoon driver (vmmemctl.ko)
will use the backdoorGcc64_arm64.c when built for ARM64.

Pull request: #474
@johnwvmw
Copy link
Contributor

johnwvmw commented Mar 23, 2021

The code changes for this pull request have been comitted to the "devel" branch at 5c27a31

Closing this pull request.

@johnwvmw johnwvmw closed this Mar 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants