dciabrin / ngdevkit

Open source development for Neo-Geo
GNU Lesser General Public License v3.0
272 stars 26 forks source link

P ROM size is not enough #78

Closed gabordo closed 1 year ago

gabordo commented 1 year ago

Hello, we are working on a new Neo Geo game and we ran out of P ROM space. When it happened first I looked into ngdevkit.ld and increased the length of the ROM memory from 512K to 1M and it solved the problem. Unfortunately we ran out of memory again, probably we will need either 2M of ROM or maybe more with bankswitching. Unfortunately I can't increase ROM size to 2M because it overlaps with the RAM. I tried to add a ROM2 memory and a new section based on the 68k memory map on wiki.neogeodev.org but I couldn't get it work. Could you please help with this?

dciabrin commented 1 year ago

Right now there is no transparent support for bankswitching in ngdevkit, I have to come up with an example in ngdevkit-examples. This will probably require a specific linkscript for bankswitched programs, as well a a specific ROM builder script to extract elf output into the proper P ROM format. I will probably come up with an example that will use tranpoline for bankswitching, and that will allow each bank to share a common RAM area (to share library functions and variables between banks) and have a specific start point in RAM for its own need.

gabordo commented 1 year ago

Thank you! I found a workaround meanwhile using multiple P ROM files. The first one is converted from the elf as usual but the others are made by the data converter and I can use 0x200000+offsets to point to data in the 2nd bank.

dciabrin commented 1 year ago

Thank you! I found a workaround meanwhile using multiple P ROM files. The first one is converted from the elf as usual but the others are made by the data converter and I can use 0x200000+offsets to point to data in the 2nd bank.

A quick heads up here. I started playing with different linkscripts to an example of bank-switching, but after reading the neogeodev wiki in detail, I think I misunderstood your original concern. In fact, your last comment makes total sense now.

It looks like for the time being, you're interested in allowing the full 2MB address space of P-ROM, rather than the 512KB that's provided in the default linkscript. So I'll bump to .code segment limit to 1MB first because it's straightforward. Then I'll have a look at the linkscript syntax to see if I can tell the linker to skip the 0x100000 - 0x1fffff range automatically. If so, I'll bump the .code limit to 2MB. I'll deal with the bankswitching example separately.

gabordo commented 1 year ago

I think increasing 512KB to 1MB is definitely useful. But I'm not sure if it was a good idea to ask for 2MB support in the linker script. It might be useful if 2MB is enough for a project. But if not (or we are not sure) then I think it is probably easier to use multiple binary blobs. But I might be wrong and maybe 2MB limit will be super useful for others :)

dciabrin commented 1 year ago

This is worth a try though, as starting binutils 2.35, ld apparently has the ability to link across non-contiguous-memory-regions. I'm trying that locally at the moment. I'll report if that's something useful, hopefully shortly

dciabrin commented 1 year ago

OK I had good results with a new linkscript that has been completely rewritten to allow linked 1MB ROMs and 2x1MB ROMs. This will be the basis for bankswitch as well. Right now I pushed the new binutils in ngdevkit-toolchain in git.

The new linkscript has a breaking change, in that you're now in charge of padding the generated ROM files. This is not a big deal, but I need to update all example ROMS in ngdevkit-examples.

Once I'll have all the bits in place, I'll push the new linkscript in ngdevkit, along with some information on how to use that.

dciabrin commented 1 year ago

I think the necessary support for large P-ROM has landed in git now.

In https://github.com/dciabrin/ngdevkit-examples/commit/de2877c7e2cc216d47db6247bd264da23a867a81 I've added two new examples to support large P-ROM programs.

To use the full 1MiB ROM space, you need to use the name of a cartridge that is known to have a 1MB P ROM. Look in the makefile for how to do that.

If you want to use the full 2MiB that is allowed by the hardware, you have to use yet another ROM name, but also to use specific link flags and objcopy invocations. The makefile gives an example as well.

The linkscript is good enough to allow bankswitching of the second P-ROM address space, but I still need to come up with an additional example for that.

I let you test and report if it does what you expected.

gabordo commented 1 year ago

I use mame and I generate a description xml for our game so I can set up the exact ROM sizes and calculate the correct hashes.

If somebody needs more than 2MB ROM with bankswitching then I think they also need a way to decide which data goes into which ROM, for example the player sprite data is always needed so it should go into the first ROM but boss sprites can go into bankswitched ROMs. Is this possible now? Using attributes maybe?

gabordo commented 1 year ago

I moved to a new PC and downloaded the latest version of ngdevkit. I rebuilt the project but unfortunately I got only a black screen in mame. If I replace your latest ngdevkit.ld with the previous one (patched to go up to 1mb p rom) then it runs properly. I'm not sure why, I need to sort out a few other things today but I'll try to figure it out next week.

dciabrin commented 1 year ago

I moved to a new PC and downloaded the latest version of ngdevkit. I rebuilt the project but unfortunately I got only a black screen in mame. If I replace your latest ngdevkit.ld with the previous one (patched to go up to 1mb p rom) then it runs properly. I'm not sure why, I need to sort out a few other things today but I'll try to figure it out next week.

Hmmm that could be many things. The immediate thing that comes to mind is that commit https://github.com/dciabrin/ngdevkit/commit/6f07991d7f2bedcb48b9eb02d6060aa5c8aae7b2 was a breaking change, it now requires the example ROM to pad the generated ROM files explicitly. I have updated the Makefiles in https://github.com/dciabrin/ngdevkit-examples/commit/de2877c7e2cc216d47db6247bd264da23a867a81 to show how things work. In my previous tests this was enough for making MAME work with all the examples.

Also, I just pushed https://github.com/dciabrin/ngdevkit/commit/a83e75129f4bf6d4a7c3f71487b9dfe3372d4189 to support bank-switching of large program. You might want to recompile the toolkit (it comes with a new header) and the examples to see if it fixes your issue?

gabordo commented 1 year ago

I padded ROM1 to 1MB, ROM2 is still generated by the data converter.

I'll try the latest version today. I use MSYS and pacman to update the toolkit, I'm not sure if that is what you meant by recompile.

gabordo commented 1 year ago

Latest version doesn't work either. I used MAME's debugger to look into the issue and I think init_c_runtime in ngdevkit-crt0.s overwrites the stack accidentally:

.Lclearbss:
    /* Zero out bss segment */
    move.l  #__data_end, %d0
    sub.l   #__data_start, %d0
    add.l   #__data_start_in_ram, %d0
    movea.l %d0, %a0
    move.l  #__bss_end, %d0
    sub.l   #__bss_start, %d0
    tst %d0
    beq .Linitdone
.Lcopybss:
    move.b  #0, (%a0)+
    move.b  %d0, REG_WATCHDOGW
    dbra    %d0, .Lcopybss

D0 register is set to 0xa0b2 and A0 is set to 0x10a0b8 so I think the loop overwrites the stack. If I link with the old linker script then D0 is the same but A0 is set to 0x100004 which seems to be the correct value.

dciabrin commented 1 year ago

Ack let me look what I have on my example roms, and why that is that I can't reproduce the issue yet. Thanks for the thorough investigation

dciabrin commented 1 year ago

Could you also tell if this is only happening for your own ROM of all the ROMs in ngdevkit-examples? I haven't checked on my MSYS env yet, but I can't reproduce your issue yet. Could you run m68k-neogeo-elf-nm <your-rom>.elf| grep __ to verify that all the segments are linked the way they are supposed to and see what crt0 is doing? I'm not sure how it could be a stack smash yet, as by default stack is at 0x10f300, so way above what you pasted.

gabordo commented 1 year ago

With the new linker script the loop clears 0x10a0b8-0x11416a, I think that covers the stack. (Old linker script cleared 0x100004-0x1a0b6 which was the correct interval.) The symbols are:

0009ae40 T __bss_end
0008f9e4 T __bss_start
00013ef0 t __CTOR_END__
00013eec t __CTOR_LIST__
0008f9e4 T __data_end
0008f954 T __data_start
00100000 A __data_start_in_ram
0000f770 T __divsi3
00010750 t __do_global_ctors_aux
00000478 t __do_global_dtors_aux
00100000 D __dso_handle
00013ef8 T __DTOR_END__
00013ef4 t __DTOR_LIST__
00013efc t __EH_FRAME_BEGIN__
00010740 T __errno
00013efc t __FRAME_END__
00013f00 t __JCR_END__
00013f00 t __JCR_LIST__
00102474 B __malloc_free_list
0001053c T __malloc_lock
00102470 B __malloc_sbrk_start
00010544 T __malloc_unlock
0000f7d0 T __modsi3
0000f6f0 T __mulsi3
0008f954 T __rodata_end
00013f16 T __rodata_start
0000fa2a T __ssprint_r
0000f928 T __ssputs_r
00013f16 T __text_end
00100090 D __TMC_END__
0000f714 T __udivsi3
0000f7a0 T __umodsi3
dciabrin commented 1 year ago

Ok I since the last commits, the crt0 initializes the runtime memory incorrectly as .bss is now linked prior to .data. Looking at the symbols above and at the code, it didn't cause a immediate stack corruption, that's probably why it didn't show up in the example ROMs from ngdevkit-examples.

I'm still unclear about why your ROM went that far in memory. Anyway, before digging further, could you check that the latest commit fixes your issue? It should have been rebuilt into a new msys2 package ready to be consumed.

gabordo commented 1 year ago

I just updated to the latest ngdevkit, built the game and it is working correctly now.

I have a big (46k currently) uninitalized array in my code which I use for a super simple allocator (alloc,free_all). This might have caused the issue, but this is only a guess. In a hindsight I should have dig a little deeper last time and should have double checked the "clearbss" calculations in ngdevkit-crt0.s.

dciabrin commented 1 year ago

Glad it works now! So with that I think we can close this issue, as the devkit can now (hopefully seamlessly) link big ROMs and non-contiguous ROMs.

gabordo commented 1 year ago

Thank you for implementing this!