[Qemu-devel] [PATCH v5 0/4] fdc: fix/rewrite seek, media_changed and interrupt handling

[Qemu-devel] [PATCH v5 0/4] fdc: fix/rewrite seek, media_changed and interrupt handling

[Pavel Hrdina]

The fd_seek will return 'track is invalid' if there is no media in drive.

Implied seek should have the same behavior as normal seek. We will use the fd_seek
function instead of a direct modification of head, track and sector values.
The result value is used only while read/write commands were issued.

Fixed sense interrupt status command.

v5:
    - correction of misspelled words

v4:
    - fixed coding style
    - better bit clearing in 'fdctrl_handle_sense_interrupt_status'

v3:
    - added fdc-test
    - rewrited seek and media_changed handling
    - fixed interrupt status handling

v2:
    - better way to detect media missing

Pavel Hrdina (4):
  fdc: fix implied seek while there is no media in drive
  fdc-test: introduced qtest read_without_media
  fdc: rewrite seek and DSKCHG bit handling
  fdc: fix interrupt handling

 hw/fdc.c         |  106 ++++++++++++++++++++++++++++++------------------------
 tests/fdc-test.c |   66 +++++++++++++++++++++++++++++++++
 2 files changed, 125 insertions(+), 47 deletions(-)

-- 
1.7.7.6

[Qemu-devel] [PATCH v5 1/4] fdc: fix implied seek while there is no media in drive

[Pavel Hrdina]

The Windows uses 'READ' command at the start of an instalation
without checking the 'dir' register. We have to abort the transfer
with an abnormal termination if there is no media in the drive.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 hw/fdc.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/hw/fdc.c b/hw/fdc.c
index 30d34e3..be35201 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -159,6 +159,10 @@ static int fd_seek(FDrive *drv, uint8_t head, uint8_t track, uint8_t sect,
         drv->sect = sect;
     }
 
+    if (drv->bs == NULL || !bdrv_is_inserted(drv->bs)) {
+        ret = 2;
+    }
+
     return ret;
 }
 
-- 
1.7.7.6

[Qemu-devel] [PATCH v5 2/4] fdc-test: introduced qtest read_without_media

[Pavel Hrdina]

If you try to read from a floppy drive without a media, you should get
an abnormal termination error.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 tests/fdc-test.c |   66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 66 insertions(+), 0 deletions(-)

diff --git a/tests/fdc-test.c b/tests/fdc-test.c
index 22d24ac..e730398 100644
--- a/tests/fdc-test.c
+++ b/tests/fdc-test.c
@@ -49,6 +49,7 @@ enum {
 enum {
     CMD_SENSE_INT   = 0x08,
     CMD_SEEK        = 0x0f,
+    CMD_READ        = 0xe6,
 };
 
 enum {
@@ -99,6 +100,62 @@ static void ack_irq(void)
     g_assert(!get_irq(FLOPPY_IRQ));
 }
 
+static uint8_t send_read_command(void)
+{
+    uint8_t drive = 0;
+    uint8_t head = 0;
+    uint8_t cyl = 0;
+    uint8_t sect_addr = 1;
+    uint8_t sect_size = 2;
+    uint8_t eot = 1;
+    uint8_t gap = 0x1b;
+    uint8_t gpl = 0xff;
+
+    uint8_t msr = 0;
+    uint8_t st0;
+
+    uint8_t ret = 0;
+
+    floppy_send(CMD_READ);
+    floppy_send(head << 2 | drive);
+    g_assert(!get_irq(FLOPPY_IRQ));
+    floppy_send(cyl);
+    floppy_send(head);
+    floppy_send(sect_addr);
+    floppy_send(sect_size);
+    floppy_send(eot);
+    floppy_send(gap);
+    floppy_send(gpl);
+
+    uint8_t i = 0;
+    uint8_t n = 2;
+    for (; i < n; i++) {
+        msr = inb(FLOPPY_BASE + reg_msr);
+        if (msr == 0xd0) {
+            break;
+        }
+        sleep(1);
+    }
+
+    if (i >= n) {
+        return 1;
+    }
+
+    st0 = floppy_recv();
+    if (st0 != 0x40) {
+        ret = 1;
+    }
+
+    floppy_recv();
+    floppy_recv();
+    floppy_recv();
+    floppy_recv();
+    floppy_recv();
+    floppy_recv();
+
+    return ret;
+}
+
 static void send_step_pulse(void)
 {
     int drive = 0;
@@ -146,6 +203,14 @@ static void test_no_media_on_start(void)
     assert_bit_set(dir, DSKCHG);
 }
 
+static void test_read_without_media(void)
+{
+    uint8_t ret;
+
+    ret = send_read_command();
+    g_assert(ret == 0);
+}
+
 static void test_media_change(void)
 {
     uint8_t dir;
@@ -214,6 +279,7 @@ int main(int argc, char **argv)
     qtest_irq_intercept_in(global_qtest, "ioapic");
     qtest_add_func("/fdc/cmos", test_cmos);
     qtest_add_func("/fdc/no_media_on_start", test_no_media_on_start);
+    qtest_add_func("/fdc/read_without_media", test_read_without_media);
     qtest_add_func("/fdc/media_change", test_media_change);
 
     ret = g_test_run();
-- 
1.7.7.6

[Qemu-devel] [PATCH v5 3/4] fdc: rewrite seek and DSKCHG bit handling

[Pavel Hrdina]

This bit is cleared on every successful seek to a different track (cylinder).
The seek is also called on revalidate or on read/write/format commands which
also clear the DSKCHG bit.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 hw/fdc.c |   68 +++++++++++++++++++++++++++++++-------------------------------
 1 files changed, 34 insertions(+), 34 deletions(-)

diff --git a/hw/fdc.c b/hw/fdc.c
index be35201..d338c3f 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -153,8 +153,12 @@ 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->track != track) {
+            if (drv->bs != NULL && bdrv_is_inserted(drv->bs)) {
+                drv->media_changed = 0;
+            }
             ret = 1;
+        }
         drv->track = track;
         drv->sect = sect;
     }
@@ -170,9 +174,7 @@ static int fd_seek(FDrive *drv, uint8_t head, uint8_t track, uint8_t sect,
 static void fd_recalibrate(FDrive *drv)
 {
     FLOPPY_DPRINTF("recalibrate\n");
-    drv->head = 0;
-    drv->track = 0;
-    drv->sect = 1;
+    fd_seek(drv, 0, 0, 1, 1);
 }
 
 /* Revalidate a disk drive after a disk change */
@@ -711,14 +713,6 @@ static void fdctrl_raise_irq(FDCtrl *fdctrl, uint8_t status0)
         qemu_set_irq(fdctrl->irq, 1);
         fdctrl->sra |= FD_SRA_INTPEND;
     }
-    if (status0 & FD_SR0_SEEK) {
-        FDrive *cur_drv;
-        /* A seek clears the disk change line (if a disk is inserted) */
-        cur_drv = get_cur_drv(fdctrl);
-        if (cur_drv->bs != NULL && bdrv_is_inserted(cur_drv->bs)) {
-            cur_drv->media_changed = 0;
-        }
-    }
 
     fdctrl->reset_sensei = 0;
     fdctrl->status0 = status0;
@@ -1004,30 +998,39 @@ static int fdctrl_seek_to_next_sect(FDCtrl *fdctrl, FDrive *cur_drv)
                    fd_sector(cur_drv));
     /* XXX: cur_drv->sect >= cur_drv->last_sect should be an
        error in fact */
-    if (cur_drv->sect >= cur_drv->last_sect ||
-        cur_drv->sect == fdctrl->eot) {
-        cur_drv->sect = 1;
+    uint8_t new_head = cur_drv->head;
+    uint8_t new_track = cur_drv->track;
+    uint8_t new_sect = cur_drv->sect;
+
+    int ret = 1;
+
+    if (new_sect >= cur_drv->last_sect ||
+        new_sect == fdctrl->eot) {
+        new_sect = 1;
         if (FD_MULTI_TRACK(fdctrl->data_state)) {
-            if (cur_drv->head == 0 &&
+            if (new_head == 0 &&
                 (cur_drv->flags & FDISK_DBL_SIDES) != 0) {
-                cur_drv->head = 1;
+                new_head = 1;
             } else {
-                cur_drv->head = 0;
-                cur_drv->track++;
+                new_head = 0;
+                new_track++;
                 if ((cur_drv->flags & FDISK_DBL_SIDES) == 0)
-                    return 0;
+                    ret = 0;
             }
         } else {
-            cur_drv->track++;
-            return 0;
+            new_track++;
+            ret = 0;
         }
-        FLOPPY_DPRINTF("seek to next track (%d %02x %02x => %d)\n",
+        if (ret == 1) {
+            FLOPPY_DPRINTF("seek to next track (%d %02x %02x => %d)\n",
                        cur_drv->head, cur_drv->track,
                        cur_drv->sect, fd_sector(cur_drv));
+        }
     } else {
-        cur_drv->sect++;
+        new_sect++;
     }
-    return 1;
+    fd_seek(cur_drv, new_head, new_track, new_sect, 1);
+    return ret;
 }
 
 /* Callback for transfer end (stop or abort) */
@@ -1623,11 +1626,7 @@ static void fdctrl_handle_seek(FDCtrl *fdctrl, int direction)
     /* The seek command just sends step pulses to the drive and doesn't care if
      * there is a medium inserted of if it's banging the head against the drive.
      */
-    if (fdctrl->fifo[2] > cur_drv->max_track) {
-        cur_drv->track = cur_drv->max_track;
-    } else {
-        cur_drv->track = fdctrl->fifo[2];
-    }
+    fd_seek(cur_drv, cur_drv->head, fdctrl->fifo[2], cur_drv->sect, 1);
     /* Raise Interrupt */
     fdctrl_raise_irq(fdctrl, FD_SR0_SEEK);
 }
@@ -1692,9 +1691,10 @@ static void fdctrl_handle_relative_seek_out(FDCtrl *fdctrl, int direction)
     SET_CUR_DRV(fdctrl, fdctrl->fifo[1] & FD_DOR_SELMASK);
     cur_drv = get_cur_drv(fdctrl);
     if (fdctrl->fifo[2] + cur_drv->track >= cur_drv->max_track) {
-        cur_drv->track = cur_drv->max_track - 1;
+        fd_seek(cur_drv, cur_drv->head, cur_drv->max_track - 1,
+                cur_drv->sect, 1);
     } else {
-        cur_drv->track += fdctrl->fifo[2];
+        fd_seek(cur_drv, cur_drv->head, fdctrl->fifo[2], cur_drv->sect, 1);
     }
     fdctrl_reset_fifo(fdctrl);
     /* Raise Interrupt */
@@ -1708,9 +1708,9 @@ static void fdctrl_handle_relative_seek_in(FDCtrl *fdctrl, int direction)
     SET_CUR_DRV(fdctrl, fdctrl->fifo[1] & FD_DOR_SELMASK);
     cur_drv = get_cur_drv(fdctrl);
     if (fdctrl->fifo[2] > cur_drv->track) {
-        cur_drv->track = 0;
+        fd_seek(cur_drv, cur_drv->head, 0, cur_drv->sect, 1);
     } else {
-        cur_drv->track -= fdctrl->fifo[2];
+        fd_seek(cur_drv, cur_drv->head, fdctrl->fifo[2], cur_drv->sect, 1);
     }
     fdctrl_reset_fifo(fdctrl);
     /* Raise Interrupt */
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH v5 3/4] fdc: rewrite seek and DSKCHG bit handling

[Kevin Wolf]

Am 13.06.2012 15:43, schrieb Pavel Hrdina:
> This bit is cleared on every successful seek to a different track (cylinder).
> The seek is also called on revalidate or on read/write/format commands which
> also clear the DSKCHG bit.
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  hw/fdc.c |   68 +++++++++++++++++++++++++++++++-------------------------------
>  1 files changed, 34 insertions(+), 34 deletions(-)

Nice cleanup! Looks good generally, but this and patch 4 need test cases
as well before I can apply them.

> @@ -1004,30 +998,39 @@ static int fdctrl_seek_to_next_sect(FDCtrl *fdctrl, FDrive *cur_drv)
>                     fd_sector(cur_drv));
>      /* XXX: cur_drv->sect >= cur_drv->last_sect should be an
>         error in fact */
> -    if (cur_drv->sect >= cur_drv->last_sect ||
> -        cur_drv->sect == fdctrl->eot) {
> -        cur_drv->sect = 1;
> +    uint8_t new_head = cur_drv->head;
> +    uint8_t new_track = cur_drv->track;
> +    uint8_t new_sect = cur_drv->sect;
> +
> +    int ret = 1;
> +
> +    if (new_sect >= cur_drv->last_sect ||
> +        new_sect == fdctrl->eot) {
> +        new_sect = 1;
>          if (FD_MULTI_TRACK(fdctrl->data_state)) {
> -            if (cur_drv->head == 0 &&
> +            if (new_head == 0 &&
>                  (cur_drv->flags & FDISK_DBL_SIDES) != 0) {
> -                cur_drv->head = 1;
> +                new_head = 1;
>              } else {
> -                cur_drv->head = 0;
> -                cur_drv->track++;
> +                new_head = 0;
> +                new_track++;
>                  if ((cur_drv->flags & FDISK_DBL_SIDES) == 0)
> -                    return 0;
> +                    ret = 0;

Please use the chance to add braces here.

Can you also add a small header comment to the function that explains
what the return value means? Initially I read 0 as error, but in fact it
seems only to mean "request completed".

Kevin

[Qemu-devel] [PATCH v5 4/4] fdc: fix interrupt handling

[Pavel Hrdina]

If you call the SENSE INTERRUPT STATUS command while there is no interrupt
waiting you get as result unknown command.

Fixed status0 register handling for read/write/format commands.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 hw/fdc.c |   34 +++++++++++++++++++++-------------
 1 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/hw/fdc.c b/hw/fdc.c
index d338c3f..0a5d58d 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -307,6 +307,9 @@ enum {
 };
 
 enum {
+    FD_SR0_DS0      = 0x01,
+    FD_SR0_DS1      = 0x02,
+    FD_SR0_HEAD     = 0x04,
     FD_SR0_EQPMT    = 0x10,
     FD_SR0_SEEK     = 0x20,
     FD_SR0_ABNTERM  = 0x40,
@@ -972,14 +975,15 @@ static void fdctrl_reset_fifo(FDCtrl *fdctrl)
 }
 
 /* Set FIFO status for the host to read */
-static void fdctrl_set_fifo(FDCtrl *fdctrl, int fifo_len, int do_irq)
+static void fdctrl_set_fifo(FDCtrl *fdctrl, int fifo_len, uint8_t status0)
 {
     fdctrl->data_dir = FD_DIR_READ;
     fdctrl->data_len = fifo_len;
     fdctrl->data_pos = 0;
     fdctrl->msr |= FD_MSR_CMDBUSY | FD_MSR_RQM | FD_MSR_DIO;
-    if (do_irq)
-        fdctrl_raise_irq(fdctrl, 0x00);
+    if (status0) {
+        fdctrl_raise_irq(fdctrl, status0);
+    }
 }
 
 /* Set an error: unimplemented/unknown command */
@@ -1040,10 +1044,12 @@ static void fdctrl_stop_transfer(FDCtrl *fdctrl, uint8_t status0,
     FDrive *cur_drv;
 
     cur_drv = get_cur_drv(fdctrl);
+    fdctrl->status0 = status0 | FD_SR0_SEEK | (cur_drv->head << 2) |
+                      GET_CUR_DRV(fdctrl);
+
     FLOPPY_DPRINTF("transfer status: %02x %02x %02x (%02x)\n",
-                   status0, status1, status2,
-                   status0 | (cur_drv->head << 2) | GET_CUR_DRV(fdctrl));
-    fdctrl->fifo[0] = status0 | (cur_drv->head << 2) | GET_CUR_DRV(fdctrl);
+                   status0, status1, status2, fdctrl->status0);
+    fdctrl->fifo[0] = fdctrl->status0;
     fdctrl->fifo[1] = status1;
     fdctrl->fifo[2] = status2;
     fdctrl->fifo[3] = cur_drv->track;
@@ -1056,7 +1062,7 @@ static void fdctrl_stop_transfer(FDCtrl *fdctrl, uint8_t status0,
     }
     fdctrl->msr |= FD_MSR_RQM | FD_MSR_DIO;
     fdctrl->msr &= ~FD_MSR_NONDMA;
-    fdctrl_set_fifo(fdctrl, 7, 1);
+    fdctrl_set_fifo(fdctrl, 7, fdctrl->status0);
 }
 
 /* Prepare a data transfer (either DMA or FIFO) */
@@ -1170,7 +1176,7 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction)
     if (direction != FD_DIR_WRITE)
         fdctrl->msr |= FD_MSR_DIO;
     /* IO based transfer: calculate len */
-    fdctrl_raise_irq(fdctrl, 0x00);
+    fdctrl_raise_irq(fdctrl, FD_SR0_SEEK);
 
     return;
 }
@@ -1598,16 +1604,18 @@ static void fdctrl_handle_sense_interrupt_status(FDCtrl *fdctrl, int direction)
 {
     FDrive *cur_drv = get_cur_drv(fdctrl);
 
-    if(fdctrl->reset_sensei > 0) {
+    if (fdctrl->reset_sensei > 0) {
         fdctrl->fifo[0] =
             FD_SR0_RDYCHG + FD_RESET_SENSEI_COUNT - fdctrl->reset_sensei;
         fdctrl->reset_sensei--;
+    } else if (!(fdctrl->sra & FD_SRA_INTPEND)) {
+        fdctrl->fifo[0] = FD_SR0_INVCMD;
+        fdctrl_set_fifo(fdctrl, 1, 0);
+        return;
     } else {
-        /* XXX: status0 handling is broken for read/write
-           commands, so we do this hack. It should be suppressed
-           ASAP */
         fdctrl->fifo[0] =
-            FD_SR0_SEEK | (cur_drv->head << 2) | GET_CUR_DRV(fdctrl);
+                (fdctrl->status0 & ~(FD_SR0_HEAD | FD_SR0_DS1 | FD_SR0_DS0))
+                | GET_CUR_DRV(fdctrl);
     }
 
     fdctrl->fifo[1] = cur_drv->track;
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH v5 0/4] fdc: fix/rewrite seek, media_changed and interrupt handling

[Kevin Wolf]

Am 13.06.2012 15:43, schrieb Pavel Hrdina:
> The fd_seek will return 'track is invalid' if there is no media in drive.
> 
> Implied seek should have the same behavior as normal seek. We will use the fd_seek
> function instead of a direct modification of head, track and sector values.
> The result value is used only while read/write commands were issued.
> 
> Fixed sense interrupt status command.
> 
> v5:
>     - correction of misspelled words
> 
> v4:
>     - fixed coding style
>     - better bit clearing in 'fdctrl_handle_sense_interrupt_status'
> 
> v3:
>     - added fdc-test
>     - rewrited seek and media_changed handling
>     - fixed interrupt status handling
> 
> v2:
>     - better way to detect media missing
> 
> Pavel Hrdina (4):
>   fdc: fix implied seek while there is no media in drive
>   fdc-test: introduced qtest read_without_media
>   fdc: rewrite seek and DSKCHG bit handling
>   fdc: fix interrupt handling
> 
>  hw/fdc.c         |  106 ++++++++++++++++++++++++++++++------------------------
>  tests/fdc-test.c |   66 +++++++++++++++++++++++++++++++++
>  2 files changed, 125 insertions(+), 47 deletions(-)
> 

Thanks, I applied patches 1 and 2. For the rest I'm waiting for a v6
that includes test cases.

Kevin