jart / blink

tiniest x86-64-linux emulator
ISC License
6.96k stars 222 forks source link

[metal] Ignore writes to BIOS code area #131

Closed tkchia closed 1 year ago

tkchia commented 1 year ago

Also add a --disable-rom build option which will effectively make the BIOS code area read/write, like normal RAM. This shaves around 200 bytes off a MODE = tiny Blink executable, at the cost of breaking emulation fidelity.

ghaerr commented 1 year ago

Hello @tkchia,

I'm still a bit confused here... if DISABLE_ROM is not defined, why is the Mprotect code still required? Won't that cause a segfault? I thought that this new code effectively checks for and ignores writes within the ROM region if not disabled, thus removing the need for any memory protection kernel services. (And thus, the emulation would in fact be entirely accurate - no segfault or halting the emulator, and writing to ROM locations doesn't alter memory contents).

Thank you!

tkchia commented 1 year ago

Hello @ghaerr,

I thought that this new code effectively checks for and ignores writes within the ROM region if not disabled, thus removing the need for any memory protection kernel services.

My (semblance of a) plan is to use AccessRam() to catch most attempts to write to ROM, and rely on the Mprotect() to catch any remaining write attempts that may have been missed. This way, we can find and fix them.

In particular, the Mprotect() alerted me to the fact that the "enhanced" rep stosb implementation was bypassing the whole AccessRam() mechanism.

Thank you!

tkchia commented 1 year ago

@ghaerr : I am thinking of turning the IsRomAddress() into an inline function, perhaps in blink/biosrom.h. This should hopefully make everything more efficient (and still correct).

ghaerr commented 1 year ago

@tkchia,

I am thinking of turning the IsRomAddress() into an inline function, perhaps in blink/biosrom.h. This should hopefully make everything more efficient (and still correct).

That's a good idea.

In particular, the Mprotect() alerted me to the fact that the "enhanced" rep stosb implementation was bypassing the whole AccessRam() mechanism.

I see. OK.

This looks fine, and shouldn't probably reduce the execution speed measurably. I'll plan on running ./configure --disable-rom so I can continue to use FreeDOS v1.3-rc1.

Thank you!

tkchia commented 1 year ago

@ghaerr : FreeDOS 1.3-rc1 should now work under either --disable-rom or --enable-rom. Thank you!

ghaerr commented 1 year ago

@tkchia,

FreeDOS 1.3-rc1 should now work under either --disable-rom or --enable-rom.

I will test the above after commit. Is the reason that FreeDOS v1.3-rc1 is working with the Mprotect enabled because you're now prohibiting the write in the string move routines as well? I guess so. So the idea of leaving in Mprotect is to catch internal coding issues at this point. As such, I'm ok with committing this now.

Thank you!

tkchia commented 1 year ago

Hello @ghaerr,

So the idea of leaving in Mprotect is to catch internal coding issues at this point.

Yep. Maybe I should clarify this in the code comments.

Let me also see if I can add a test program, before I merge this PR.

Thank you!

tkchia commented 1 year ago

I change my mind — I will write the test case later. While trying to come up with a test case I hit upon some other emulator issues, and I hope to sort them out first.