ghaerr / elks

Embeddable Linux Kernel Subset - Linux for 8086
Other
1.02k stars 108 forks source link

[kernel] Move bios_disk_park_all to be PFPROC function #1997

Closed ghaerr closed 2 months ago

ghaerr commented 2 months ago

Moves recently created bios_disk_park_all function to .fartext via BFPROC declaration.

@vkoskiv, I believe to have figured out why your system crashed when this function was defined as BFPROC during development. The reason is that when was added to sys.c during your development, it was added as the first include, prior to (this was noticed by myself a few days ago and fixed in another commit):

#include <linuxmt/biosparm.h>
#include <linuxmt/config.h>

Since BFPROC is conditionally defined in biosparm.h based on whether the kernel is figured to use a far text segment:

/* places some of this driver in the far text section */
#if defined(CONFIG_FARTEXT_KERNEL) && !defined(__STRICT_ANSI__)
#define BFPROC __far __attribute__ ((far_section, noinline, section (".fartext.bf")))
#else
#define BFPROC
#endif

This had the net effect of calling bios_disk_park_all as a near function, when in fact in bios.c it was a far function. Thus, when that function returned, CS was popped and CRASH!

Yes, it's kind of bad that the order of include files matters, but it gets even hairier when headers include other headers, so this is where we are at for the time being. has to come first, and the order after that should not matter.

I then combined the two functions in bios.c and during that noticed another possible bug: the park function was not in fact parking the head on the last cylinder, but on a cylinder one over what the BIOS reports as the last cylinder. The drivep->cylinders value is set in one place, just above in the function bios_gethdinfo using INT 13h function 8, where the CX register is also packed in the same manner as the INT 13h function 0x0C:

           drivep->cylinders = (((BD_CX & 0xc0) << 2) | (BD_CX >> 8)) + 1;

Shouldn't the park location be the last reported cylinder on the drive? It seems to me that sending an invalid cylinder number to a BIOS make invoke UB (undefined behaviour), but I haven't checked source code in any BIOSes for the seek function.

Please advise.

Note that ELKS can't use a hard drive with > 1024 cylinders (0-1023) because all (early) BIOS CHS functions the BIOS HD driver uses require the cylinder to be in the swapped 8-bit/2-bit format, 10 bits total.

Also, I removed the "drivep->cylinders > 1024" check since given the initialization of drivep->cylinders above, that case will never happen.

I've tested this on QEMU, and no crashing. When you find time, can you test this on your real hardware to check it doesn't crash? I don't think it will.

I'm not sure how to know that the MFM drive is actually "parked" - is there a way to tell on real hardware? It would seem that given your original code, a drive with 1023 cylinders (stored as 1024) would actually overflow the CH register and park the drive at cylinder 0.