cebix / macemu

Basilisk II and SheepShaver Macintosh emulators
1.38k stars 288 forks source link

[BUG] Basilisk II hangs in direct addressing when frame buffer base address is lower than ram base address #203

Closed rickyzhang82 closed 4 years ago

rickyzhang82 commented 4 years ago

I debugged the issues in SDL2. I found the culprit which is hidden for very long time.

As the title suggested, in direct addressing the mapping is a simple math:

RamAddrMac = RamAddrHost - RamBaseHost.

But BII never check when allocating the frame buffer in the host, the FrameBaseHost satisfies the following:

In the absence of the checking above, it cause random hang.

See my last comment in https://github.com/kanjitalk755/macemu/issues/43

rickyzhang82 commented 4 years ago

This is my first attempt to use Mac OS X vm_allocate to allocate frame buffer right after RAM and ROM in the host.

I started acquire RAM in an arbitrary address. Then I used next_address to acquire ROM and frame buffer. It failed randomly due to the fact that host memory can allocate the framebuffer right after RAM+ROM memory allocation. My first attempt doesn't solve the problem. But it seems the similar trick works quite well in Linux.

See below:

Ricky@imac:~/repo/github/macemu/BasiliskII/src$ git diff
diff --git a/BasiliskII/src/CrossPlatform/vm_alloc.cpp b/BasiliskII/src/CrossPlatform/vm_alloc.cpp
index 005cb727..9261dfc6 100644
--- a/BasiliskII/src/CrossPlatform/vm_alloc.cpp
+++ b/BasiliskII/src/CrossPlatform/vm_alloc.cpp
@@ -85,6 +85,10 @@ typedef unsigned long vm_uintptr_t;

 #define MAP_EXTRA_FLAGS (MAP_32BIT)

+#ifdef HAVE_MACH_VM
diff --git a/BasiliskII/src/CrossPlatform/vm_alloc.cpp b/BasiliskII/src/CrossPlatform/vm_alloc.cpp
index 005cb727..9261dfc6 100644
--- a/BasiliskII/src/CrossPlatform/vm_alloc.cpp
+++ b/BasiliskII/src/CrossPlatform/vm_alloc.cpp
@@ -85,6 +85,10 @@ typedef unsigned long vm_uintptr_t;

 #define MAP_EXTRA_FLAGS (MAP_32BIT)

+#ifdef HAVE_MACH_VM
+static void * next_address = (void *)0;
+#endif
+
 #ifdef HAVE_MMAP_VM
 #if (defined(__linux__) && defined(__i386__)) || defined(__FreeBSD__) || HAVE_LINKER_SCRIPT
 /* Force a reasonnable address below 0x80000000 on x86 so that we
@@ -243,11 +247,22 @@ void * vm_acquire(size_t size, int options)

 #if defined(HAVE_MACH_VM)
        // vm_allocate() returns a zero-filled memory region
-       kern_return_t ret_code = vm_allocate(mach_task_self(), (vm_address_t *)&addr, size, TRUE);
+       kern_return_t ret_code;
+       // this is the first time
+       if ((void *)0 == (void *)next_address) {
+               ret_code = vm_allocate(mach_task_self(), (vm_address_t *)&addr, size, TRUE);
+               printf("1st time vm_acquire: addr (%p), next_addr (%p), size (%d)\n", addr, next_address, size);
+       } else {
+               ret_code = vm_allocate(mach_task_self(), (vm_address_t *)&next_address, size, FALSE);
+               addr = next_address;
+               printf("Nth time vm_acquire: addr (%p), next_addr (%p), size (%d)\n", addr, next_address, size);
+       }
        if (ret_code != KERN_SUCCESS) {
                errno = vm_error(ret_code);
+               perror("Error printed by perror");
                return VM_MAP_FAILED;
        }
+       next_address = (char *)addr + size;
 #elif defined(HAVE_MMAP_VM)
        int fd = zero_fd;
        int the_map_flags = translate_map_flags(options) | map_flags;

I believe the better approach is to allocate all memory at once for Mac RAM, Mac ROM and frame buffer in Mac OS X. Then assign them separately.

If you have any better ideas, I'm all ears.

rickyzhang82 commented 4 years ago

@asvitkine Another bug I found from master branch is in the line 201. My C may be rusty. But the line 201 disabled line 202-209 every time it is called.

https://github.com/cebix/macemu/blob/5cbf07e9f537ddd224d7b7ed9922938701ea3daa/BasiliskII/src/SDL/video_sdl.cpp#L198-L213

rickyzhang82 commented 4 years ago

Problem Statement

When the host OS is Mac OS X, direct addressing in BII doesn't guarantee that the allocated memory for frame buffer base address in the host (FrameBaseHost) satisfies the following conditions:

where RamBaseHost refers to the emulated RAM base address in the host.

This may cause the random hang problem where the allocated frame address failed to meet the conditions above.

Because the direct addressing mapping is a simple math:

RamAddrMac = RamAddrHost - RamBaseHost.

Solution

Solution 1

Force Mac OS X uses memory bank addressing in configure.ac.

Pros:

Cons:

Solution 2

Allocate host memory for guest RAM, guest ROM and guest frame buffer all at once. Assign each memory blocks later.

Pros:

Cons:

Solution 3

Check the conditions. If it doesn't meet the conditions, let it crash and print out a nice message to ask people to restart.

Pros:

Cons:

Summary

I haven't fix the old bug yet. If you have a better ideas, please let me know. Linux is quite special. Their mmap doesn't seem to have the problem like Mac OS X. But in Linux it starts the mmap in a very special location address:

https://github.com/cebix/macemu/blob/5cbf07e9f537ddd224d7b7ed9922938701ea3daa/BasiliskII/src/CrossPlatform/vm_alloc.cpp#L88-L96

I can't find any helpful information regarding to the Mac OS X application virtual memory address mapping. Especially with ASLR, this is a myth to me.

rickyzhang82 commented 4 years ago

bii-performance

I benchmark two addressing: direct and memory banks. As I expected, the memory banks is a little bit slower than direct. But in general, there are no noticeable issues at all to run any Apps in BII under direct or memory banks.

But what makes the huge performance differences is the GCC compiler optimization flag:

As you can see, memory banks is 10x slower in no optimization flag.

In summary, I will send a PR of solution 1. In configure.ac, when it detects Mac OS X and Intel platform, it will force addressing to memory banks.

rakslice commented 3 years ago

My C may be rusty. But the line 201 disabled line 202-209 every time it is called. https://github.com/cebix/macemu/blob/5cbf07e9f537ddd224d7b7ed9922938701ea3daa/BasiliskII/src/SDL/video_sdl.cpp#L198-L213

In case you didn't already figure it out, your C is rusty. The static keyword on this line makes fb a static local variable. The statement is not an assignment, it is an initialization. That's important because, given the static lifetime of the variable, its initialization happens only once at the start of the program, and the value otherwise persists between runs of the function. The statement has no effect during the runs of the function.

rickyzhang82 commented 3 years ago

@rakslice

Thanks for pointing it out. This makes much sense now.

For direct addressing, the consecutive memory allocation are not guaranteed in modern OS. We may have to adopt solution 2 or if you have a better idea?