Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[kernel] Move bios_disk_park_all to be PFPROC function #1997

Merged
merged 1 commit into from
Sep 5, 2024
Merged

Conversation

ghaerr
Copy link
Owner

@ghaerr ghaerr commented Sep 5, 2024

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 <biosparms.h> was added to sys.c during your development, it was added as the first include, prior to <config.h> (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. <config.h> 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.

@ghaerr ghaerr merged commit 295e9ec into master Sep 5, 2024
2 checks passed
@ghaerr ghaerr deleted the park branch September 5, 2024 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant