On Wed, 7 Jan 2026 at 15:31, Pedro J. Ruiz <[email protected]> wrote: > > The floppy disk controller was incorrectly setting the FD_SR0_SEEK > (Seek End) status bit during automatic track crossing in multi-track > read/write operations. This caused legacy operating systems like Minix 2 > to interpret successful read operations as errors, resulting in > "Unrecoverable disk error" messages for blocks that crossed track > boundaries. > > When executing multi-sector READ/WRITE commands with the MT (multi-track) > flag set, the FDC would correctly advance to the next track when needed > to continue the transfer. However, it was incorrectly setting the SE > (Seek End) bit in Status Register 0 (ST0) during this automatic track > advancement.
Hi; thanks for this patch. I'm going to prefix my review by saying that I'm not an expert on this floppy controller, so I am going by what I can make out from the datasheet, which is not super clear on this. > According to the Intel 82078 datasheet and related documentation, the > SE bit (bit 5, value 0x20) in ST0 should only be set: > 1. After explicit SEEK or RECALIBRATE commands > 2. After READ/WRITE commands that perform an "implied seek" (when the > command specifies a different cylinder/head/sector than the current > position and EIS flag is not set) > > The SE bit should NOT be set during automatic track crossing that occurs > as part of an ongoing multi-track data transfer. This automatic track > advancement is part of the normal multi-track operation, not a seek. > > This bug prevented Minix 2 and potentially other legacy operating systems > from booting. The OS floppy driver would detect the unexpected SE bit and > interpret it as a read error, even though the data was transferred > successfully. This particularly affected 1024-byte filesystem blocks that > spanned track boundaries. > > Modified fdctrl_seek_to_next_sect() to remove the line that set > FD_SR0_SEEK when advancing to the next track during multi-track > operations. The function now: > - In multi-track mode: advances tracks/heads as needed WITHOUT setting > the SE bit > - In non-multi-track mode: stops at end of track without seeking (also > without setting SE bit, as no seek occurs) > > The SE bit is still correctly set by explicit SEEK and RECALIBRATE > commands elsewhere in the code. > > Fixes: c5139bd9 (fdc: fix FD_SR0_SEEK for non-DMA transfers and multi > sectors transfers) > Signed-off-by: Pedro J. Ruiz <[email protected]> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1522 > --- > hw/block/fdc.c | 9 ++++++--- > tests/qtest/fdc-test.c | 2 +- > 2 files changed, 7 insertions(+), 4 deletions(-) > > diff --git a/hw/block/fdc.c b/hw/block/fdc.c > index 4585640af9..21713c9c41 100644 > --- a/hw/block/fdc.c > +++ b/hw/block/fdc.c > @@ -1403,14 +1403,17 @@ static int fdctrl_seek_to_next_sect(FDCtrl *fdctrl, > FDrive *cur_drv) > } else { > new_head = 0; > new_track++; > - fdctrl->status0 |= FD_SR0_SEEK; > + /* Don't set FD_SR0_SEEK for implicit track crossing during (minor style nit: multiline comments should start with the "/*" on a line of its own. This file is very old code so it doesn't follow our current style on this point.) > + * multi-track transfers. SEEK bit must only be set for > + * explicit SEEK commands, not automatic track advancement. > + */ So this is the "we are multitrack, we are on head 1 and hit the end of the disk" case. The datasheet https://wiki.qemu.org/images/f/f0/29047403.pdf describes the MT bit like this: # MT: Multi-track selector # When set, this flag selects the multi-track operating mode. # In this mode, the 82078 treats a complete cylinder, under head 0 # and 1, as a single track. The 82078 operates as if this expanded # track started at the first sector under head 0 and ended at the last # sector under head 1. With this flag set, a multitrack read or write # operation will automatically continue to the first sector under # head 1 when the 82078 finishes operating on the last sector under # head 0. That all describes the behaviour for the "at end of head 0" case (which is in the if() block before this), and looking at the code we already do that correctly without claiming that it is a seek. What it doesn't say anything about is automatically rolling from head 1 back to head 0 when we reach the end on head 1. For that we need to look at the info about the read and write commands: Table 6-6 in the datasheet has the information about what the track (C), head (H) and sector (R) should be after a transfer. Summarizing that and numbering the cases: (1) regardless of MT, if we didn't hit end-of-track we just increment the sector count (2) For MT=0, if we did hit end-of-track, reset sector count to 1 and increment track count; don't change the head (3) For MT=1, at end-of-track on head 0, leave the track count alone, flip to head 1, reset sector count to 1 (4) For MT=1 at end-of-track on head 1, flip to head 0, increment the track count, and reset sector count to 1 The current code's handling of new_sect, new_track and new_head seems to me to follow all of that. So maybe our problem is only that we should not set the SEEK bit in the status register... > if ((cur_drv->flags & FDISK_DBL_SIDES) == 0) { > ret = 0; > } > } > } else { > - fdctrl->status0 |= FD_SR0_SEEK; > - new_track++; > + /* Not in multi-track mode: stop at end of track and don't seek. > */ > + FLOPPY_DPRINTF("end of track, stopping transfer\n"); Here we change the MT=0 "reached end of track" behaviour from "go to next track, call it a seek" to "stay at the end of the track". I think that if my reading of the datasheet above is correct, we do still want to increment new_track, but we don't want to set the SEEK bit in the status register. Incidentally I find the 'ret = 0' vs 'ret = 1' handling in this function confusing -- we return 0 for the "hit end of track in MT=0" case, but in the MT=1 case we return 0 only when we hit the end of the track and the disk is not double-sided. This seems inconsistent and perhaps wrong -- maybe the code should always do 'ret = 0;' without the "((cur_drv->flags & FDISK_DBL_SIDES) == 0)" check ? > ret = 0; > } > if (ret == 1) { > diff --git a/tests/qtest/fdc-test.c b/tests/qtest/fdc-test.c > index 1b37a8a4d2..9edfbb5a40 100644 > --- a/tests/qtest/fdc-test.c > +++ b/tests/qtest/fdc-test.c > @@ -519,7 +519,7 @@ static void test_read_no_dma_19(void) > > outb(FLOPPY_BASE + reg_dor, inb(FLOPPY_BASE + reg_dor) & ~0x08); > send_seek(0); > - ret = send_read_no_dma_command(19, 0x20); > + ret = send_read_no_dma_command(19, 0x00); Why do we need to change the test case? The commit message doesn't say. > g_assert(ret == 0); > } Could we add some test cases for the multi-track cases ? thanks -- PMM
