[PATCH] hw/block/fdc: report a missing address mark on an empty drive

[PATCH] hw/block/fdc: report a missing address mark on an empty drive

[Christian Quante]

READ ID on a drive with no medium terminates normally and returns the
made-up sector ID left over from the "Pretend we are spinning" emulation.
The only error path is a data rate mismatch, and media_rate is assigned
solely by pick_geometry(); it is never reset when the medium is removed.
A guest that has just ejected a diskette is therefore told that one is
still present.

READ and WRITE have a related problem: fd_seek() returns 2 both for
"track/head out of range" and for "no medium", and fdctrl_start_transfer()
reports that as ST0 = ABNTERM with ST1 = 0x00.  Without ST1.MA the guest
cannot tell an absent diskette from a transient error.

Fail READ ID with ST0 = ABNTERM and ST1 = MA when the drive is empty, and
set ST1.MA in the fd_seek() case 2 path when no medium is present.  This
does not make guests detect the removal -- real hardware never completes
READ ID on an empty drive, because there are no index pulses, and OS/2 for
one relies on that timeout -- but it stops the controller from claiming a
diskette that is not there.

tests/qtest/fdc-test.c starts QEMU with "-device floppy,id=floppy0" and no
medium, so the drive is empty for the whole run, and test_read_id asserts a
normal termination with a fabricated cylinder 8 / head 1.  That contradicts
its neighbours test_no_media_on_start and test_media_change, which state
that DSKCHG signals an absent medium.  Insert a medium before READ ID and
eject it afterwards -- the rewritten test passes before and after this
change -- and add test_read_id_no_media to cover the empty drive.

Guests checked, reading and writing, with and without a medium: Linux
2.0.34 and 7.0, PC-DOS 7, IBM DOS 5.02, Windows for Workgroups 3.11 and
OS/2 2.11.  None changes behaviour.  No version of the Linux floppy driver
from 1.2.13 to master issues READ ID at all -- FD_READID is defined in the
uapi header for FDRAWCMD users and the driver never sends it -- so Linux
detects an empty drive by stepping the head and reading DSKCHG instead.

Buglink: https://gitlab.com/qemu-project/qemu/-/issues/3971
Signed-off-by: Christian Quante <christian@quante.one>
---
 hw/block/fdc.c         | 29 +++++++++++++++--
 tests/qtest/fdc-test.c | 73 ++++++++++++++++++++++++++++++++++++++----
 2 files changed, 93 insertions(+), 9 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 2c1681b7d0..8895e39cb6 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -196,6 +196,12 @@ static void fd_init(FDrive *drv)
 
 #define NUM_SIDES(drv) ((drv)->flags & FDISK_DBL_SIDES ? 2 : 1)
 
+/* Is a diskette present in the drive? */
+static bool fd_media_present(FDrive *drv)
+{
+    return drv->blk != NULL && blk_is_inserted(drv->blk);
+}
+
 static int fd_sector_calc(uint8_t head, uint8_t track, uint8_t sect,
                           uint8_t last_sect, uint8_t num_sides)
 {
@@ -1476,8 +1482,15 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction)
                                   NUM_SIDES(cur_drv)));
     switch (fd_seek(cur_drv, kh, kt, ks, fdctrl->config & FD_CONFIG_EIS)) {
     case 2:
-        /* sect too big */
-        fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, 0x00, 0x00);
+        /*
+         * Track/head out of range, or no medium at all.  Only the latter can
+         * be told apart by the guest, through ST1.MA: with no diskette in the
+         * drive there is no address mark to be found.  Guests that
+         * distinguish an absent medium from an unreadable one rely on this.
+         */
+        fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM,
+                             fd_media_present(cur_drv) ? 0x00 : FD_SR1_MA,
+                             0x00);
         fdctrl->fifo[3] = kt;
         fdctrl->fifo[4] = kh;
         fdctrl->fifo[5] = ks;
@@ -2303,6 +2316,18 @@ static void fdctrl_result_timer(void *opaque)
     FDCtrl *fdctrl = opaque;
     FDrive *cur_drv = get_cur_drv(fdctrl);
 
+    /*
+     * An empty drive has no address marks to read.  Completing READ ID
+     * successfully, with the made-up sector ID left over from the "spinning"
+     * emulation below, tells the guest that a diskette is still present after
+     * it has been ejected.  The only error path left was a data rate mismatch,
+     * and media_rate is never reset when the medium is removed.
+     */
+    if (!fd_media_present(cur_drv)) {
+        FLOPPY_DPRINTF("read id on empty drive\n");
+        fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_MA, 0x00);
+        return;
+    }
     /* Pretend we are spinning.
      * This is needed for Coherent, which uses READ ID to check for
      * sector interleaving.
diff --git a/tests/qtest/fdc-test.c b/tests/qtest/fdc-test.c
index 1b37a8a4d2..fa57a8b7cf 100644
--- a/tests/qtest/fdc-test.c
+++ b/tests/qtest/fdc-test.c
@@ -64,6 +64,12 @@ enum {
 
     DSKCHG  = 0x80,
 };
+enum {
+    ST0_IC_MASK  = 0xc0,    /* interrupt code */
+    ST0_IC_ABNTERM = 0x40,  /* abnormal termination */
+
+    ST1_MA       = 0x01,    /* missing address mark */
+};
 
 static char *test_image;
 
@@ -270,6 +276,21 @@ static void test_cmos(void)
     g_assert(cmos == 0x40 || cmos == 0x50);
 }
 
+static void media_insert(void)
+{
+    qtest_qmp_assert_success(global_qtest,
+                             "{'execute':'blockdev-change-medium', 'arguments':{"
+                             " 'id':'floppy0', 'filename': %s, 'format': 'raw' }}",
+                             test_image);
+}
+
+static void media_eject(void)
+{
+    qtest_qmp_assert_success(global_qtest,
+                             "{'execute':'eject', 'arguments':{"
+                             " 'id':'floppy0' }}");
+}
+
 static void test_no_media_on_start(void)
 {
     uint8_t dir;
@@ -301,10 +322,7 @@ static void test_media_insert(void)
 
     /* Insert media in drive. DSKCHK should not be reset until a step pulse
      * is sent. */
-    qtest_qmp_assert_success(global_qtest,
-                             "{'execute':'blockdev-change-medium', 'arguments':{"
-                             " 'id':'floppy0', 'filename': %s, 'format': 'raw' }}",
-                             test_image);
+    media_insert();
 
     dir = inb(FLOPPY_BASE + reg_dir);
     assert_bit_set(dir, DSKCHG);
@@ -333,9 +351,7 @@ static void test_media_change(void)
 
     /* Eject the floppy and check that DSKCHG is set. Reading it out doesn't
      * reset the bit. */
-    qtest_qmp_assert_success(global_qtest,
-                             "{'execute':'eject', 'arguments':{"
-                             " 'id':'floppy0' }}");
+    media_eject();
 
     dir = inb(FLOPPY_BASE + reg_dir);
     assert_bit_set(dir, DSKCHG);
@@ -414,6 +430,12 @@ static void test_read_id(void)
     uint8_t st0;
     uint8_t msr;
 
+    /*
+     * READ ID reads the address mark of the sector currently under the head,
+     * so it needs a medium.  The preceding tests left the drive empty.
+     */
+    media_insert();
+
     /* Seek to track 0 and check with READ ID */
     send_seek(0);
 
@@ -491,6 +513,42 @@ static void test_read_id(void)
     g_assert_cmpint(cyl, ==, 8);
     g_assert_cmpint(head, ==, 1);
     g_assert_cmpint(st0, ==, head << 2);
+
+    /* Leave the drive as the following tests expect to find it. */
+    media_eject();
+}
+
+/*
+ * An empty drive spins no diskette, so READ ID finds no address mark and must
+ * terminate abnormally.  Reporting success (with a made-up sector ID) would
+ * tell the guest that a medium is still present after it has been ejected.
+ */
+static void test_read_id_no_media(void)
+{
+    uint8_t drive = 0;
+    uint8_t head = 0;
+    uint8_t st0, st1;
+
+    floppy_send(CMD_READ_ID);
+    g_assert(!get_irq(FLOPPY_IRQ));
+    floppy_send(head << 2 | drive);
+
+    while (!get_irq(FLOPPY_IRQ)) {
+        clock_step(1000000000LL / 50);
+    }
+
+    st0 = floppy_recv();
+    st1 = floppy_recv();
+    floppy_recv();                  /* ST2 */
+    floppy_recv();                  /* cylinder */
+    floppy_recv();                  /* head */
+    floppy_recv();                  /* sector */
+    g_assert(get_irq(FLOPPY_IRQ));
+    floppy_recv();                  /* sector size */
+    g_assert(!get_irq(FLOPPY_IRQ));
+
+    g_assert_cmpint(st0 & ST0_IC_MASK, ==, ST0_IC_ABNTERM);
+    g_assert_cmpint(st1 & ST1_MA, ==, ST1_MA);
 }
 
 static void test_read_no_dma_1(void)
@@ -625,6 +683,7 @@ int main(int argc, char **argv)
     qtest_add_func("/fdc/sense_interrupt", test_sense_interrupt);
     qtest_add_func("/fdc/relative_seek", test_relative_seek);
     qtest_add_func("/fdc/read_id", test_read_id);
+    qtest_add_func("/fdc/read_id_no_media", test_read_id_no_media);
     qtest_add_func("/fdc/verify", test_verify);
     qtest_add_func("/fdc/media_insert", test_media_insert);
     qtest_add_func("/fdc/read_no_dma_1", test_read_no_dma_1);
-- 
2.53.0

Re: [PATCH] hw/block/fdc: report a missing address mark on an empty drive

[Kevin Wolf]

Hi Christian,

really nice writeup of the problem, both here and with the additional
details in the Gitlab report. I don't generally care too much for
reviewing floppy patches, but I love this one.

Am 10.07.2026 um 13:31 hat Christian Quante geschrieben:
> READ ID on a drive with no medium terminates normally and returns the
> made-up sector ID left over from the "Pretend we are spinning" emulation.
> The only error path is a data rate mismatch, and media_rate is assigned
> solely by pick_geometry(); it is never reset when the medium is removed.
> A guest that has just ejected a diskette is therefore told that one is
> still present.
> 
> READ and WRITE have a related problem: fd_seek() returns 2 both for
> "track/head out of range" and for "no medium", and fdctrl_start_transfer()
> reports that as ST0 = ABNTERM with ST1 = 0x00.  Without ST1.MA the guest
> cannot tell an absent diskette from a transient error.

Your solution for this one is the only part where I'm not completely
convinced. You keep the ambiguous 2 as a return value from fd_seek(),
and then just perform the same check again in the caller, involving a
second call of blk_is_inserted(). Probably not terrible in practice, but
also not completely elegant.

The straightforward solution I expected would have been adding a new
return code to fd_seek() for the "no medium" case. Is there anything
that makes this harder than it seems at the first sight?

> Fail READ ID with ST0 = ABNTERM and ST1 = MA when the drive is empty, and
> set ST1.MA in the fd_seek() case 2 path when no medium is present.  This
> does not make guests detect the removal -- real hardware never completes
> READ ID on an empty drive, because there are no index pulses, and OS/2 for
> one relies on that timeout -- but it stops the controller from claiming a
> diskette that is not there.
> 
> tests/qtest/fdc-test.c starts QEMU with "-device floppy,id=floppy0" and no
> medium, so the drive is empty for the whole run, and test_read_id asserts a
> normal termination with a fabricated cylinder 8 / head 1.  That contradicts
> its neighbours test_no_media_on_start and test_media_change, which state
> that DSKCHG signals an absent medium.  Insert a medium before READ ID and
> eject it afterwards -- the rewritten test passes before and after this
> change -- and add test_read_id_no_media to cover the empty drive.
> 
> Guests checked, reading and writing, with and without a medium: Linux
> 2.0.34 and 7.0, PC-DOS 7, IBM DOS 5.02, Windows for Workgroups 3.11 and
> OS/2 2.11.  None changes behaviour.  No version of the Linux floppy driver
> from 1.2.13 to master issues READ ID at all -- FD_READID is defined in the
> uapi header for FDRAWCMD users and the driver never sends it -- so Linux
> detects an empty drive by stepping the head and reading DSKCHG instead.
> 
> Buglink: https://gitlab.com/qemu-project/qemu/-/issues/3971
> Signed-off-by: Christian Quante <christian@quante.one>
> ---
>  hw/block/fdc.c         | 29 +++++++++++++++--
>  tests/qtest/fdc-test.c | 73 ++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 93 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 2c1681b7d0..8895e39cb6 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -196,6 +196,12 @@ static void fd_init(FDrive *drv)
>  
>  #define NUM_SIDES(drv) ((drv)->flags & FDISK_DBL_SIDES ? 2 : 1)
>  
> +/* Is a diskette present in the drive? */
> +static bool fd_media_present(FDrive *drv)
> +{
> +    return drv->blk != NULL && blk_is_inserted(drv->blk);
> +}
> +
>  static int fd_sector_calc(uint8_t head, uint8_t track, uint8_t sect,
>                            uint8_t last_sect, uint8_t num_sides)
>  {
> @@ -1476,8 +1482,15 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction)
>                                    NUM_SIDES(cur_drv)));
>      switch (fd_seek(cur_drv, kh, kt, ks, fdctrl->config & FD_CONFIG_EIS)) {
>      case 2:
> -        /* sect too big */
> -        fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, 0x00, 0x00);
> +        /*
> +         * Track/head out of range, or no medium at all.  Only the latter can
> +         * be told apart by the guest, through ST1.MA: with no diskette in the
> +         * drive there is no address mark to be found.  Guests that
> +         * distinguish an absent medium from an unreadable one rely on this.
> +         */
> +        fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM,
> +                             fd_media_present(cur_drv) ? 0x00 : FD_SR1_MA,
> +                             0x00);
>          fdctrl->fifo[3] = kt;
>          fdctrl->fifo[4] = kh;
>          fdctrl->fifo[5] = ks;
> @@ -2303,6 +2316,18 @@ static void fdctrl_result_timer(void *opaque)
>      FDCtrl *fdctrl = opaque;
>      FDrive *cur_drv = get_cur_drv(fdctrl);
>  
> +    /*
> +     * An empty drive has no address marks to read.  Completing READ ID
> +     * successfully, with the made-up sector ID left over from the "spinning"
> +     * emulation below, tells the guest that a diskette is still present after
> +     * it has been ejected.  The only error path left was a data rate mismatch,
> +     * and media_rate is never reset when the medium is removed.
> +     */
> +    if (!fd_media_present(cur_drv)) {
> +        FLOPPY_DPRINTF("read id on empty drive\n");
> +        fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_MA, 0x00);
> +        return;
> +    }
>      /* Pretend we are spinning.
>       * This is needed for Coherent, which uses READ ID to check for
>       * sector interleaving.
> diff --git a/tests/qtest/fdc-test.c b/tests/qtest/fdc-test.c
> index 1b37a8a4d2..fa57a8b7cf 100644
> --- a/tests/qtest/fdc-test.c
> +++ b/tests/qtest/fdc-test.c
> @@ -64,6 +64,12 @@ enum {
>  
>      DSKCHG  = 0x80,
>  };
> +enum {
> +    ST0_IC_MASK  = 0xc0,    /* interrupt code */
> +    ST0_IC_ABNTERM = 0x40,  /* abnormal termination */
> +
> +    ST1_MA       = 0x01,    /* missing address mark */
> +};
>  
>  static char *test_image;
>  
> @@ -270,6 +276,21 @@ static void test_cmos(void)
>      g_assert(cmos == 0x40 || cmos == 0x50);
>  }
>  
> +static void media_insert(void)
> +{
> +    qtest_qmp_assert_success(global_qtest,
> +                             "{'execute':'blockdev-change-medium', 'arguments':{"
> +                             " 'id':'floppy0', 'filename': %s, 'format': 'raw' }}",
> +                             test_image);
> +}
> +
> +static void media_eject(void)
> +{
> +    qtest_qmp_assert_success(global_qtest,
> +                             "{'execute':'eject', 'arguments':{"
> +                             " 'id':'floppy0' }}");
> +}
> +
>  static void test_no_media_on_start(void)
>  {
>      uint8_t dir;
> @@ -301,10 +322,7 @@ static void test_media_insert(void)
>  
>      /* Insert media in drive. DSKCHK should not be reset until a step pulse
>       * is sent. */
> -    qtest_qmp_assert_success(global_qtest,
> -                             "{'execute':'blockdev-change-medium', 'arguments':{"
> -                             " 'id':'floppy0', 'filename': %s, 'format': 'raw' }}",
> -                             test_image);
> +    media_insert();
>  
>      dir = inb(FLOPPY_BASE + reg_dir);
>      assert_bit_set(dir, DSKCHG);
> @@ -333,9 +351,7 @@ static void test_media_change(void)
>  
>      /* Eject the floppy and check that DSKCHG is set. Reading it out doesn't
>       * reset the bit. */
> -    qtest_qmp_assert_success(global_qtest,
> -                             "{'execute':'eject', 'arguments':{"
> -                             " 'id':'floppy0' }}");
> +    media_eject();
>  
>      dir = inb(FLOPPY_BASE + reg_dir);
>      assert_bit_set(dir, DSKCHG);
> @@ -414,6 +430,12 @@ static void test_read_id(void)
>      uint8_t st0;
>      uint8_t msr;
>  
> +    /*
> +     * READ ID reads the address mark of the sector currently under the head,
> +     * so it needs a medium.  The preceding tests left the drive empty.

Just as an aside, this doesn't really work like this because you can
select specific test cases to run, so we shouldn't have any dependencies
between test cases. But this is a preexisting problem in fdc-test, so I
don't expect you to fix it here.

> +     */
> +    media_insert();
> +
>      /* Seek to track 0 and check with READ ID */
>      send_seek(0);
>  
> @@ -491,6 +513,42 @@ static void test_read_id(void)
>      g_assert_cmpint(cyl, ==, 8);
>      g_assert_cmpint(head, ==, 1);
>      g_assert_cmpint(st0, ==, head << 2);
> +
> +    /* Leave the drive as the following tests expect to find it. */
> +    media_eject();
> +}
> +
> +/*
> + * An empty drive spins no diskette, so READ ID finds no address mark and must
> + * terminate abnormally.  Reporting success (with a made-up sector ID) would
> + * tell the guest that a medium is still present after it has been ejected.
> + */
> +static void test_read_id_no_media(void)
> +{
> +    uint8_t drive = 0;
> +    uint8_t head = 0;
> +    uint8_t st0, st1;
> +
> +    floppy_send(CMD_READ_ID);
> +    g_assert(!get_irq(FLOPPY_IRQ));
> +    floppy_send(head << 2 | drive);
> +
> +    while (!get_irq(FLOPPY_IRQ)) {
> +        clock_step(1000000000LL / 50);
> +    }
> +
> +    st0 = floppy_recv();
> +    st1 = floppy_recv();
> +    floppy_recv();                  /* ST2 */
> +    floppy_recv();                  /* cylinder */
> +    floppy_recv();                  /* head */
> +    floppy_recv();                  /* sector */
> +    g_assert(get_irq(FLOPPY_IRQ));
> +    floppy_recv();                  /* sector size */
> +    g_assert(!get_irq(FLOPPY_IRQ));
> +
> +    g_assert_cmpint(st0 & ST0_IC_MASK, ==, ST0_IC_ABNTERM);
> +    g_assert_cmpint(st1 & ST1_MA, ==, ST1_MA);
>  }
>  
>  static void test_read_no_dma_1(void)
> @@ -625,6 +683,7 @@ int main(int argc, char **argv)
>      qtest_add_func("/fdc/sense_interrupt", test_sense_interrupt);
>      qtest_add_func("/fdc/relative_seek", test_relative_seek);
>      qtest_add_func("/fdc/read_id", test_read_id);
> +    qtest_add_func("/fdc/read_id_no_media", test_read_id_no_media);
>      qtest_add_func("/fdc/verify", test_verify);
>      qtest_add_func("/fdc/media_insert", test_media_insert);
>      qtest_add_func("/fdc/read_no_dma_1", test_read_no_dma_1);

One more thing I noticed during review: fdctrl_handle_readid() never
seems to call SET_CUR_DRV(), so are we potentially checking the wrong
drive? It looked suspicious to me anyway. (Out of scope for this patch,
of course, but could be a separate fix if it's real.)

Kevin

[PATCH v2 0/2] hw/block/fdc: do not claim a diskette that is not there

[Christian Quante]

From: PC-DOS repro <chrischaan66@googlemail.com>

Kevin, thank you -- that was a far closer read than I expected for a floppy
patch, and all three points were right.

  1/2  The SET_CUR_DRV() omission you spotted.  It is real: every other
       handler latches the drive from fifo[1], READ ID does not, so it
       answers about whichever drive was selected last.  A DOR write
       normally selects the drive as a side effect, which is why nobody
       has tripped over it.  It goes first because 2/2 checks the medium
       of exactly that drive.

  2/2  The main change, now with the fd_seek() return code you suggested
       instead of a second blk_is_inserted() in the caller.  Nothing made
       it harder than it looked -- but it does drag fdctrl_format_sector()
       along, because a new return value would otherwise fall through its
       "default" and let FORMAT TRACK carry on as if nothing happened.  So
       FORMAT on an empty drive now answers ST1.MA as well.  I could not
       get a guest to reach that path: DOS gives up during media sensing
       and never issues the command, and a hand-written FORMAT TRACK needs
       the per-sector data phase to get there at all.  It is the right
       answer semantically, but it is untested on a guest, and I would
       rather say so than bury it.

       The test comment is fixed too -- it claimed the preceding tests
       leave the drive empty, which is exactly the dependency you object
       to.  The two tests now stand on their own; the wider ordering
       problem in fdc-test I left alone, as you asked.

Measured, empty 1.44M drive, INT 13h AH=02h on A: (SeaBIOS 1.17.0):

  before      0.11 s, AH = 0x20 (controller failure)
  after       0.21 s, AH = 0xC0 (no media)

The extra 0.1 s is SeaBIOS working through the data rates now that READ ID
no longer succeeds on the first try.  Booting from a hard disk is unchanged
-- the drive is not touched at all -- and no guest changes what it shows:
DOS shows the same message for 0x20 as for 0xC0, and Windows 3.11 and
OS/2 2.11 show exactly what they showed before.

A SeaBIOS series is on its list [1] that probes the disk change line before
media sensing; with it, the same read takes 0.11 s and answers 0x80 ("not
ready"), which DOS and Windows both render correctly.  The two are
independent -- neither waits for the other.

[1] https://mail.coreboot.org/archives/list/seabios@seabios.org/thread/K2QGADPOTAEEZRQS5T6PKZLDQUBO62HY/

Christian Quante (2):
  hw/block/fdc: select the drive named by the READ ID command
  hw/block/fdc: report a missing address mark on an empty drive

 hw/block/fdc.c         | 54 +++++++++++++++++++++++++++-----
 tests/qtest/fdc-test.c | 70 +++++++++++++++++++++++++++++++++++++-----
 2 files changed, 109 insertions(+), 15 deletions(-)

-- 
2.53.0

[PATCH v2 1/2] hw/block/fdc: select the drive named by the READ ID command

[Christian Quante]

Every other command handler begins by latching the drive from the command
byte:

    SET_CUR_DRV(fdctrl, fdctrl->fifo[1] & FD_DOR_SELMASK);

fdctrl_handle_readid() does not, so it works on whichever drive happened to
be selected last.  A guest that issues READ ID for a drive other than the
one currently selected gets an answer about the wrong one.

It has gone unnoticed because a driver normally writes the DOR to spin up
the motor first, and that write selects the drive as a side effect.  The
controller does not require it, though, and the command carries the drive
number for a reason.

Reported-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Christian Quante <christian@quante.one>
---
 hw/block/fdc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 2c1681b7d0..9b2409cfa4 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -1936,7 +1936,10 @@ static void fdctrl_handle_save(FDCtrl *fdctrl, int direction)
 
 static void fdctrl_handle_readid(FDCtrl *fdctrl, int direction)
 {
-    FDrive *cur_drv = get_cur_drv(fdctrl);
+    FDrive *cur_drv;
+
+    SET_CUR_DRV(fdctrl, fdctrl->fifo[1] & FD_DOR_SELMASK);
+    cur_drv = get_cur_drv(fdctrl);
 
     cur_drv->head = (fdctrl->fifo[1] >> 2) & 1;
     timer_mod(fdctrl->result_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
-- 
2.53.0

[PATCH v2 2/2] hw/block/fdc: report a missing address mark on an empty drive

[Christian Quante]

READ ID on a drive with no medium terminates normally and returns the
made-up sector ID left over from the "Pretend we are spinning" emulation.
The only error path is a data rate mismatch, and media_rate is assigned
solely by pick_geometry(); it is never reset when the medium is removed.
A guest that has just ejected a diskette is therefore told that one is
still present.

READ, WRITE and FORMAT have a related problem: fd_seek() answers 2 both
for "track/head out of range" and for "no medium", so the callers report
ST0 = ABNTERM with ST1 = 0x00 either way.  Without ST1.MA the guest cannot
tell an absent diskette from a transient error.  Give fd_seek() a return
code of its own for an absent medium, and let both switch statements
report the missing address mark for it.

The comments on the two switches were swapped: fd_seek() answers 2 for a
bad track or head and 3 for a sector past last_sect, but case 2 read
"sect too big" and case 3 "track too big".  Both now say what they mean.

This is a behaviour change for FORMAT TRACK on an empty drive as well,
which now answers ST1.MA rather than ST1 = 0x00.  None of the guests
tested reaches that path -- DOS gives up during media sensing and never
issues the command -- but it seemed wrong to leave fdctrl_format_sector()
falling through to "default" for a case fd_seek() now reports explicitly.

Failing READ ID does not make guests detect the removal: real hardware
never completes the command on an empty drive, because there are no index
pulses, and OS/2 for one relies on that timeout.  It does stop the
controller from claiming a diskette that is not there.

tests/qtest/fdc-test.c starts QEMU with "-device floppy,id=floppy0" and
no medium, and test_read_id asserts a normal termination with a made-up
cylinder 8 / head 1.  That contradicts its neighbours
test_no_media_on_start and test_media_change, which state that DSKCHG
signals an absent medium.  Insert a medium before READ ID and eject it
afterwards -- the rewritten test passes before and after this change --
and add test_read_id_no_media for the empty drive.

Guests checked, reading and writing, with and without a medium: Linux
2.0.34 and 7.0, PC-DOS 7, IBM DOS 5.02, Windows for Workgroups 3.11 and
OS/2 2.11.  None changes behaviour.  No version of the Linux floppy driver
from 1.2.13 to master issues READ ID at all -- FD_READID is defined in the
uapi header for FDRAWCMD users and the driver never sends it -- so Linux
detects an empty drive by stepping the head and reading DSKCHG instead.

Buglink: https://gitlab.com/qemu-project/qemu/-/issues/3971
Signed-off-by: Christian Quante <christian@quante.one>
---
 hw/block/fdc.c         | 49 ++++++++++++++++++++++++-----
 tests/qtest/fdc-test.c | 70 +++++++++++++++++++++++++++++++++++++-----
 2 files changed, 105 insertions(+), 14 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 9b2409cfa4..04750f7310 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -196,6 +196,12 @@ static void fd_init(FDrive *drv)
 
 #define NUM_SIDES(drv) ((drv)->flags & FDISK_DBL_SIDES ? 2 : 1)
 
+/* Is a diskette present in the drive? */
+static bool fd_media_present(FDrive *drv)
+{
+    return drv->blk != NULL && blk_is_inserted(drv->blk);
+}
+
 static int fd_sector_calc(uint8_t head, uint8_t track, uint8_t sect,
                           uint8_t last_sect, uint8_t num_sides)
 {
@@ -258,7 +264,7 @@ static int fd_seek(FDrive *drv, uint8_t head, uint8_t track, uint8_t sect,
 #endif
         drv->head = head;
         if (drv->track != track) {
-            if (drv->blk != NULL && blk_is_inserted(drv->blk)) {
+            if (fd_media_present(drv)) {
                 drv->media_changed = 0;
             }
             ret = 1;
@@ -267,8 +273,8 @@ static int fd_seek(FDrive *drv, uint8_t head, uint8_t track, uint8_t sect,
         drv->sect = sect;
     }
 
-    if (drv->blk == NULL || !blk_is_inserted(drv->blk)) {
-        ret = 2;
+    if (!fd_media_present(drv)) {
+        ret = 5;
     }
 
     return ret;
@@ -1476,14 +1482,24 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction)
                                   NUM_SIDES(cur_drv)));
     switch (fd_seek(cur_drv, kh, kt, ks, fdctrl->config & FD_CONFIG_EIS)) {
     case 2:
-        /* sect too big */
+        /* track/head out of range */
         fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, 0x00, 0x00);
         fdctrl->fifo[3] = kt;
         fdctrl->fifo[4] = kh;
         fdctrl->fifo[5] = ks;
         return;
+    case 5:
+        /*
+         * No medium: there is no address mark to be found.  Guests that tell
+         * an absent diskette from an unreadable one rely on ST1.MA.
+         */
+        fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_MA, 0x00);
+        fdctrl->fifo[3] = kt;
+        fdctrl->fifo[4] = kh;
+        fdctrl->fifo[5] = ks;
+        return;
     case 3:
-        /* track too big */
+        /* sector too big */
         fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_EC, 0x00);
         fdctrl->fifo[3] = kt;
         fdctrl->fifo[4] = kh;
@@ -1791,14 +1807,21 @@ static void fdctrl_format_sector(FDCtrl *fdctrl)
                                   NUM_SIDES(cur_drv)));
     switch (fd_seek(cur_drv, kh, kt, ks, fdctrl->config & FD_CONFIG_EIS)) {
     case 2:
-        /* sect too big */
+        /* track/head out of range */
         fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, 0x00, 0x00);
         fdctrl->fifo[3] = kt;
         fdctrl->fifo[4] = kh;
         fdctrl->fifo[5] = ks;
         return;
+    case 5:
+        /* no medium */
+        fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_MA, 0x00);
+        fdctrl->fifo[3] = kt;
+        fdctrl->fifo[4] = kh;
+        fdctrl->fifo[5] = ks;
+        return;
     case 3:
-        /* track too big */
+        /* sector too big */
         fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_EC, 0x00);
         fdctrl->fifo[3] = kt;
         fdctrl->fifo[4] = kh;
@@ -2306,6 +2329,18 @@ static void fdctrl_result_timer(void *opaque)
     FDCtrl *fdctrl = opaque;
     FDrive *cur_drv = get_cur_drv(fdctrl);
 
+    /*
+     * An empty drive has no address marks to read.  Completing READ ID
+     * successfully, with the made-up sector ID left over from the "spinning"
+     * emulation below, tells the guest that a diskette is still present after
+     * it has been ejected.  The only error path left was a data rate mismatch,
+     * and media_rate is never reset when the medium is removed.
+     */
+    if (!fd_media_present(cur_drv)) {
+        FLOPPY_DPRINTF("read id on empty drive\n");
+        fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_MA, 0x00);
+        return;
+    }
     /* Pretend we are spinning.
      * This is needed for Coherent, which uses READ ID to check for
      * sector interleaving.
diff --git a/tests/qtest/fdc-test.c b/tests/qtest/fdc-test.c
index 1b37a8a4d2..1e1dd8659d 100644
--- a/tests/qtest/fdc-test.c
+++ b/tests/qtest/fdc-test.c
@@ -64,6 +64,12 @@ enum {
 
     DSKCHG  = 0x80,
 };
+enum {
+    ST0_IC_MASK  = 0xc0,    /* interrupt code */
+    ST0_IC_ABNTERM = 0x40,  /* abnormal termination */
+
+    ST1_MA       = 0x01,    /* missing address mark */
+};
 
 static char *test_image;
 
@@ -270,6 +276,21 @@ static void test_cmos(void)
     g_assert(cmos == 0x40 || cmos == 0x50);
 }
 
+static void media_insert(void)
+{
+    qtest_qmp_assert_success(global_qtest,
+                             "{'execute':'blockdev-change-medium', 'arguments':{"
+                             " 'id':'floppy0', 'filename': %s, 'format': 'raw' }}",
+                             test_image);
+}
+
+static void media_eject(void)
+{
+    qtest_qmp_assert_success(global_qtest,
+                             "{'execute':'eject', 'arguments':{"
+                             " 'id':'floppy0' }}");
+}
+
 static void test_no_media_on_start(void)
 {
     uint8_t dir;
@@ -301,10 +322,7 @@ static void test_media_insert(void)
 
     /* Insert media in drive. DSKCHK should not be reset until a step pulse
      * is sent. */
-    qtest_qmp_assert_success(global_qtest,
-                             "{'execute':'blockdev-change-medium', 'arguments':{"
-                             " 'id':'floppy0', 'filename': %s, 'format': 'raw' }}",
-                             test_image);
+    media_insert();
 
     dir = inb(FLOPPY_BASE + reg_dir);
     assert_bit_set(dir, DSKCHG);
@@ -333,9 +351,7 @@ static void test_media_change(void)
 
     /* Eject the floppy and check that DSKCHG is set. Reading it out doesn't
      * reset the bit. */
-    qtest_qmp_assert_success(global_qtest,
-                             "{'execute':'eject', 'arguments':{"
-                             " 'id':'floppy0' }}");
+    media_eject();
 
     dir = inb(FLOPPY_BASE + reg_dir);
     assert_bit_set(dir, DSKCHG);
@@ -414,6 +430,9 @@ static void test_read_id(void)
     uint8_t st0;
     uint8_t msr;
 
+    /* READ ID reads an address mark, so it needs a medium in the drive. */
+    media_insert();
+
     /* Seek to track 0 and check with READ ID */
     send_seek(0);
 
@@ -491,6 +510,42 @@ static void test_read_id(void)
     g_assert_cmpint(cyl, ==, 8);
     g_assert_cmpint(head, ==, 1);
     g_assert_cmpint(st0, ==, head << 2);
+
+    /* Leave the drive empty, the way the machine starts up. */
+    media_eject();
+}
+
+/*
+ * An empty drive spins no diskette, so READ ID finds no address mark and must
+ * terminate abnormally.  Reporting success (with a made-up sector ID) would
+ * tell the guest that a medium is still present after it has been ejected.
+ */
+static void test_read_id_no_media(void)
+{
+    uint8_t drive = 0;
+    uint8_t head = 0;
+    uint8_t st0, st1;
+
+    floppy_send(CMD_READ_ID);
+    g_assert(!get_irq(FLOPPY_IRQ));
+    floppy_send(head << 2 | drive);
+
+    while (!get_irq(FLOPPY_IRQ)) {
+        clock_step(1000000000LL / 50);
+    }
+
+    st0 = floppy_recv();
+    st1 = floppy_recv();
+    floppy_recv();                  /* ST2 */
+    floppy_recv();                  /* cylinder */
+    floppy_recv();                  /* head */
+    floppy_recv();                  /* sector */
+    g_assert(get_irq(FLOPPY_IRQ));
+    floppy_recv();                  /* sector size */
+    g_assert(!get_irq(FLOPPY_IRQ));
+
+    g_assert_cmpint(st0 & ST0_IC_MASK, ==, ST0_IC_ABNTERM);
+    g_assert_cmpint(st1 & ST1_MA, ==, ST1_MA);
 }
 
 static void test_read_no_dma_1(void)
@@ -625,6 +680,7 @@ int main(int argc, char **argv)
     qtest_add_func("/fdc/sense_interrupt", test_sense_interrupt);
     qtest_add_func("/fdc/relative_seek", test_relative_seek);
     qtest_add_func("/fdc/read_id", test_read_id);
+    qtest_add_func("/fdc/read_id_no_media", test_read_id_no_media);
     qtest_add_func("/fdc/verify", test_verify);
     qtest_add_func("/fdc/media_insert", test_media_insert);
     qtest_add_func("/fdc/read_no_dma_1", test_read_no_dma_1);
-- 
2.53.0