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

Fix Basilisk II crash on Linux aarch64 #141

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

SegHaxx
Copy link

@SegHaxx SegHaxx commented Dec 1, 2020

I just had an... interesting exchange with upstream then discovered the action is over here. But it did improve my understanding of the various addressing modes. So I offer my patch here. :)

Basilisk II fails to start a VM on Debian aarch64, in my case a Raspi4.

Add a printf debug and you can see this is because MAP_BASE defaults to 0x0 which is unavailable for security reasons on recent Linux distributions thus the memory allocator falls back on a standard allocation way up in 64bit address space, which fails the "sanity check" and returns a misleading "not enough memory" error to the user.

--- a/BasiliskII/src/CrossPlatform/vm_alloc.cpp
+++ b/BasiliskII/src/CrossPlatform/vm_alloc.cpp
@@ -270,6 +270,7 @@ void * vm_acquire(size_t size, int options)
 
        if ((addr = mmap((caddr_t)next_address, size, VM_PAGE_DEFAULT, the_map_flags, fd, 0)) == (void *)MAP_FAILED)
                return VM_MAP_FAILED;
+       printf("mmap next=%p got=%p\n",next_address,addr);
        
        // Sanity checks for 64-bit platforms
        if (sizeof(void *) == 8 && (options & VM_MAP_32BIT) && !((char *)addr <= (char *)0xffffffff))
$ ./build/BasiliskII 
Basilisk II V1.1 by Christian Bauer et al.
mmap next=(nil) got=0x7f9df00000
ERROR: Not enough free memory.

There are two fixes for this. Direct addressing mode can be fixed by bumping MAP_BASE up past the security/debug guard. Direct addressing mode is what configure currently defaults to on aarch64.

The other fix is to use banked addressing mode instead. This still requires a fix to not perform the "sanity check" which is not needed in this case.

This patch fixes both addressing modes. The default remains on direct addressing mode.

I have not touched the linux i386|FreeBSD|HAVE_LINKER_SCRIPT case as I assume its working properly but the more I learn the codebase, the more inclined i am to revise that bit as well... :)

This should fix Basilisk II on aarch64 without breaking current cases. I've also tested Linux x86_64. Please test and tell me if it breaks anything. (windows and macOS have their own code paths and should be unaffected)

I have not gotten around to getting SheepShaver running yet so I have not tested it.

@SegHaxx
Copy link
Author

SegHaxx commented Dec 1, 2020

ugh seems SheepShaver needs some real porting work to run on aarch64: cebix/macemu#197

@SegHaxx
Copy link
Author

SegHaxx commented Dec 1, 2020

I've confirmed building SheepShaver does not hit the DIRECT_ADDRESSING case thus is unaffected by this patch. I haven't broken SheepShaver any more than it was already.

I recommend merging.

@SegHaxx SegHaxx changed the title Fix crash on Linux aarch64 Fix Basilisk II crash on Linux aarch64 Dec 2, 2020
@ianfixes
Copy link
Member

ianfixes commented Dec 7, 2020

Please test and tell me if it breaks anything.

I can't speak for any of the other developers who have contributed to this project over the years, but the single goal of this fork is to build toward functional automated testing. Many of the architectures on which this emulator first ran are now antiques, which (to my mind) is an existential threat to the future of the codebase:

  • if testing means "fire up antique hardware and manually click around in it", we can't provide continuous integration of pull requests
  • if we can't provide continuous integration of pull requests, we can't make informed decisions about accepting patches
  • if we can't make informed decisions about accepting patches, the project is already dead (or at best, up to the gut instincts or capriciousness of the maintainers)

All this is to say: tell me how you'd test for the bug that's fixed by this patch, and I'll be glad to help set up CI to cover that case across all the platforms that modern CI will support. If the tests pass on CI, that's 90% the approval (the rest being whether the docs have been updated, and whether the changes are commented enough for the next contributor to follow along).

I'l update the README with some of this.

@SegHaxx
Copy link
Author

SegHaxx commented Dec 8, 2020

I remembered assert() exists so I improved the error reporting. :) Forcing MAP_BASE to 0x0 results in:

$ ~/src/macemu/BasiliskII/build/BasiliskII 
Basilisk II V1.1 by Christian Bauer et al.
next=(nil) got=0x7f73f00000 size=0x4100000
BasiliskII: /home/seg/src/macemu/BasiliskII/src/Unix/../CrossPlatform/vm_alloc.cpp:278: void* vm_acquire(size_t, int): Assertion `(size_t)addr<0xffffffffL' failed.
Aborted

@lazd
Copy link

lazd commented Jan 22, 2022

Can confirm this branch builds and runs on Raspbian 64 bit (raspios_arm64-2021-11-08) with SDL 2.0.18, though I did have to replace all instances of I_PUSH with TIOCPKT in BasiliskII/src/Unix/sshpty.c to get it to build.

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.

3 participants