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

[Kevin Wolf <kwolf@redhat.com>]

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