emaculation / macemu

Basilisk II and SheepShaver Macintosh emulators, maintained
74 stars 14 forks source link

Fix Basilisk II crash on Linux aarch64 #141

Open SegHaxx opened 3 years ago

SegHaxx commented 3 years ago

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 commented 3 years ago

ugh seems SheepShaver needs some real porting work to run on aarch64: https://github.com/cebix/macemu/issues/197

SegHaxx commented 3 years ago

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.

ianfixes commented 3 years ago

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:

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 commented 3 years ago

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 commented 2 years ago

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.