[kernel] Move bios_disk_park_all to be PFPROC function #1997
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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):
Since BFPROC is conditionally defined in biosparm.h based on whether the kernel is figured to use a far text segment:
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:
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.